-
-
Notifications
You must be signed in to change notification settings - Fork 583
fix(iOS, Stack v4): ensure consistent defaults for obscureBackground
and hideNavigationBar
#3199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obscureBackground
From the PR description I reckon, that we have native UIKit behaviour when this prop is not set at all (undefined
is passed from user), and we do not handle returning from concrete boolean value to system behaviour. Is that accurate? Additionally, we have very similar problem with hideNavigationBar
right?
I think that this PR is good on its own, as it improves the behaviour & we should land it in current shape.
I like the proposal for three-valued enum. If codegen had supported the std::optional
type, then we would use it. It is, what it is, however. Let's create a ticket for this & do it at some time (standard prio).
Yes, the current behavior is not really consistent. Before
|
obscureBackground JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
N/A | native default |
true |
true |
true |
false |
false |
false |
This is okay with the exception that we can't bring back native default after dynamic changes to the prop (but this PR does not address this and I'm not sure if we 100% need this).
Fabric
obscureBackground JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
false |
native default |
true |
true |
true |
false |
false |
native default |
User cannot explicitly set obscureBackground
to false
because this is the default value on Fabric and we don't detect prop change.
Currently, on most devices, the default behavior is false
, so we are lucky, but e.g. on tvOS it is not.
hideNavigationBar
Docs
We state that default value is true
, which is the default in most cases for < iOS 26 (apart from MacCatalyst).
Starting from iOS 26, it changes and the default is that the value "will be determined by context", which might be something
else than stated in our docs as default.
Paper
hideNavigationBar JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
N/A | native default |
true |
true |
true |
false |
false |
false |
Same as obscureBackground
, we can't restore the default after dynamic changes, but I'm not sure if this is a major problem.
Fabric
hideNavigationBar JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
false |
native default |
true |
true |
true |
false |
false |
native default (!) |
This is the largest problem - if user sets hideNavigationBar: false
, because the default value in spec
file is false
, we don't detect prop change and we keep native default, which is true
in most cases.
fabric_before.mov
After this PR
obscureBackground
Paper
obscureBackground JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
N/A | false |
true |
true |
true |
false |
false |
false |
Fabric
obscureBackground JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
false |
false |
true |
true |
true |
false |
false |
false |
hideNavigationBar
Paper
hideNavigationBar JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
N/A | true |
true |
true |
true |
false |
false |
false |
Fabric
hideNavigationBar JS prop (initial) |
prop value passed to native | controller setting |
---|---|---|
undefined |
true |
true |
true |
true |
true |
false |
false |
false |
In this form, we lose native behavior but ensure consistency between platforms/iOS versions/dynamic changes to the prop.
Solution with enums might provide a way to consistently handle native default value on both architectures
when we pass undefined
(restoring default value after dynamic changes is another topic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Let's add enum to enable for default system behaviour in follow up PR.
I've created a ticket to add enums: https://github.com/software-mansion/react-native-screens-labs/issues/415. |
…r`, `obscureBackground` props in SearchBar (#3211) ## Description Follow-up to #3199. Adds support for using native behavior in `hideNavigationBar`, `obscureBackground` props in SearchBar. This will require changes in `react-navigation` docs - ticket for this: software-mansion/react-native-screens-labs#384. ### Values table | `obscureBackground` JS prop (**initial**) | prop value passed to native | controller setting | | ----------------------------------------- | --------------------------- | ------------------ | | `undefined` | `RNSOptionalBooleanUndefined` | native behavior | | `true` | `RNSOptionalBooleanTrue` | `true` | | `false` | `RNSOptionalBooleanFalse` | `false` | Changes from `true`/`false` to `undefined` in runtime are unsupported. ### Native behavior #### `obscureBackground` ```obj-c /* On iOS, default is NO for apps linked on iOS 15.0 and later, YES otherwise. On tvOS, default is NO when contained in UISearchContainerViewController, YES otherwise. */ @Property (nonatomic, assign) BOOL obscuresBackgroundDuringPresentation API_AVAILABLE(ios(9.1)); ``` #### `hideNavigationBar` ```obj-c /// Default is `YES` for apps linked before iOS 26.0, other than on MacCatalyst, where the default is `NO`. /// On iOS 26.0 for apps linked on iOS 26.0 and later, the value is determined by context unless directly set through the API. The default remains `NO` on MacCatalyst. @Property (nonatomic, assign) BOOL hidesNavigationBarDuringPresentation; ``` Closes software-mansion/react-native-screens-labs#415. ## Changes - change native component to use enum for `obscureBackground` and `hideNavigationBar` - add util converting `boolean | undefined` to `'undefined' | 'true' | 'false'` - add `RNSOptionalBoolean` enum and conversions for Paper and Fabric - change native prop setters to handle enum - add warning for dynamic change back to `undefined` ## Test code and steps to reproduce Run `Test758`. You can comment out `obscureBackground`, `hideNavigationBar` props before running the test. ## Checklist - [x] Included code example that can be used to test this change - [x] Updated TS types - [x] Updated documentation: <!-- For adding new props to native-stack --> - [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [x] Ensured that CI passes
Description
Fixes default values for
obscureBackground
andhideNavigationBar
props inRNSSearchBar
.obscureBackground
Even though that
react-navigation
's docs state thatobscureBackground
's default istrue
, this hasn't been the case. On both architectures, setter for the prop would not run (on Fabric, due tofalse
being the default value for boolean; on Paper due to not running the setter if prop wasundefined
), resulting in native behavior:Unfortunately, if we want to use
bool
for this prop, we need to decide which option is the default to ensure consistent behavior (on Fabric, we can't have undefined value for boolean). Aligning this prop tofalse
by default seems reasonable, as this is the default on iOS 15+.If we really want to maintain native behavior but we don't want to change API from boolean to enum, we can consider using
boolean | undefined
for user API inSearchBar
component and enum for native component (mappingtrue
->TRUE
,false
->FALSE
,undefined
->SYSTEM_DEFAULT
). The only problem would be to handle dynamic change fromtrue
/false
toundefined
- we would need to ignore this change and log a warning or create new searchController just to take the default value from it.The change will require updating
react-navigation
docs: https://github.com/software-mansion/react-native-screens-labs/issues/384.hideNavigationBar
On Fabric, spec file did not specify
true
as default value for the prop. When user set the initial value tofalse
, setter would not run (no change in prop value;oldScreenProps.hideNavigationBar == newScreenProps.hideNavigationBar
), maintaining UIKit default:In the current version of this PR, we fix the default value in spec file.
As starting from iOS 26, default value of this prop is determined by context, we can consider taking similar approach as proposed for
obscureBackground
(mappingtrue
->TRUE
,false
->FALSE
,undefined
->SYSTEM_DEFAULT
), however, dynamic change fromtrue
/false
toundefined
will probably need to be handled by logging a warning (if context decides the behavior, we might not be able to just extract the value from other instance).Fixes https://github.com/software-mansion/react-native-screens-labs/issues/404.
Changes
obscureBackground
(false
) by setting default value on inithideNavigationBar
(true
) by setting default value on init and fixing Fabric spec fileScreenshots / GIFs
hideNavigationBar
, Paperpaper_before.mov
paper_after.mov
hideNavigationBar
, Fabricfabric_before.mov
fabric_after.mov
Test code and steps to reproduce
Run
SearchBar
example screen. To test defaultobscureBackground
behavior, you can useTest758
- just comment out this prop.Checklist