Skip to content
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

fix: do not force set translucent nav bar (until it's explicitly specified) #2301

Conversation

kirillzyusko
Copy link
Contributor

@kirillzyusko kirillzyusko commented Aug 12, 2024

Description

I have next navigator structure:

  • JS
    • native-stack

native-stack navigator customizes options as:

{
    headerShown: false,
    statusBarTranslucent: true,
    navigationBarColor: "#FFFFFF",
    navigationBarTranslucent: true,
},

When I go to native-stack - everything works well: the nav bar changes color. However, when I go back, then I'm getting a gray space in the bottom of my screen.

It happens because we disable mode edge-to-edge by calling WindowCompat.setDecorFitsSystemWindows(window, true) (the gray space appears because before we were already in edge-to-edge mode, because I had KeyboardProvider mounted in App.tsx).

So to fix this problem I decided explicitly check for boolean value for navigationBarTranslucent and set decorFitsSystemWindows only when we have an actual boolean value.

If you think that it's a problem in my project, then, please, let me know the way to fix it 😊

Changes

  • call WindowCompat.setDecorFitsSystemWindows(window, true) only if isNavigationBarTranslucent has a boolean value;

Screenshots / GIFs

Before

telegram-cloud-photo-size-2-5321328488650760881-y

After

telegram-cloud-photo-size-2-5321328488650760882-y

Test code and steps to reproduce

I tested in react-native-keyboard-controller example app, but if you need to test it in your code - let me know, and I'll try to prepare a reproduction code.

Checklist

@kirillzyusko
Copy link
Contributor Author

@alduzy since you implemented #2152 - may I ask you to review these changes? 👀

@tboba tboba requested review from tboba and alduzy August 13, 2024 14:50
Copy link
Member

@alduzy alduzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirillzyusko Thanks for the PR. I could not achieve the grey stripe at the bottom of the screen while trying to reproduce it, but I believe I can understand the problem.

I have following concerns:

  1. With the proposed solution edge-to-edge content mode in a native stack won't change until we encounter a screen with this option set to false and then it won't change until we encounter a screen with this option set to true. The screens in between would then have different layout depending on the navigation history, which is undesired.

  2. The JS stack navigator does not enable this option and with proposed solution we are able to change it by setting it in the nested native stack.

I think we should seek another approach to fixing your problem as we want the navigationBarTranslucent to be false by default. The example below demonstrates how setting the options only in Nested Second screen affects other screens and the root JS stack.

          navigationBarTranslucent: true,
          navigationBarColor: 'transparent',
Screen.Recording.2024-08-19.at.11.24.43.mov

@kirillzyusko
Copy link
Contributor Author

Okay @alduzy I see

Well, the problem actually comes from the fact, that before I used navigationBarColor. And prior to 3.31.0 everything was fine

But starting from 3.32.0 it looks like if I use navigationBarColor then we automatically specify navigationBarTranslucent=false (and it'll move app not to edge-to-edge mode).

But what if I enable edge-to-edge mode globally in my app and I don't want to disable it when I go to nested stack navigator?.. (but just want to change navigationBarColor as it was prior to 3.32.0) 🤔

@alduzy
Copy link
Member

alduzy commented Aug 20, 2024

@kirillzyusko prior to #2152 it was impossible to draw behind the navigation bar (for some time at least, more info in the PR's description). You probably shouldn't use navigationBarTranslucent=true at all and it should work just like before 3.32.0. Please let me know if it works.

Btw.

  • How is navigationBarColor affecting the translucency?
  • How do you enable edge-to-edge mode globally?

@kirillzyusko
Copy link
Contributor Author

Please let me know if it works.

It doesn't work, because navigationBarTranslucent will be undefined if we don't specify it. And then the line val translucent = screenForNavBarTranslucent?.isNavigationBarTranslucent ?: false automatically cast it to false and next line disables the edge-to-edge mode (which shouldn't do that, because I don't specify navigationBarTranslucent, i. e. I don't want to change edge-to-edge mode at all).

That was the idea behind my fix - if it's null (undefined), then do not modify edge-to-edge layout 😅

How is navigationBarColor affecting the translucency?

This is what I don't understand fully yet, but:

  • when I specify navigationBarColor="#FFFFFF", then method setNavigationBarTranslucent gets called;
  • if I don't specify navigationBarColor prop, then method setNavigationBarTranslucent is not getting called and everything works as it was working before 🤷‍♂️

Based on your words I assume the correct behavior would be to specify navigationBarColor="#FFFFFF" but method setNavigationBarTranslucent shouldn't be called?

I don't understand all transitive dependencies yet (in terms of functions call) in react-native-screens codebase, but if you can explain me why and who calls setNavigationBarTranslucent when we specify navigationBarColor I would highly appreciate that!

Ignore that, I found it:

        if (didSetNavigationBarAppearance) {
            setNavigationBarColor(screen, activity)
            setNavigationBarTranslucent(screen, activity)
            setNavigationBarHidden(screen, activity)
        }

So it looks like if we set any of three properties: color, translucent, hidden -> we'll automatically force set setNavigationBarTranslucent and even if it's not defined for current screen we anyway will cast the value to boolean and will set that.

Curious to know what is the correct fix in this case then 🤔

How do you enable edge-to-edge mode globally?

Many packages moves the app to edge-to-edge mode globally, for example react-native-bars, react-native-keyboard-provider, react-native-unistyles, react-native-avoid-softinput.

In my case I simply mount KeyboardProvider from react-native-keyboard-controller and it moves the app to edge-to-edge mode: https://github.com/kirillzyusko/react-native-keyboard-controller/blob/0d7ef4373a279eb66166fe5d53285f198762995f/android/src/main/java/com/reactnativekeyboardcontroller/views/EdgeToEdgeReactViewGroup.kt#L123-L128

@alduzy
Copy link
Member

alduzy commented Aug 20, 2024

@kirillzyusko Ok I see

I'm trying to think of a solution that doesn't introduce the problems I mentioned earlier.
Separating the didSetNavigationBarAppearance into smaller pieces, so that the translucency is not set along with the color, as you discovered, would probably solve the problem. However, when the translucency is eventually set the problem will reappear.

I've just now discovered, that the navigationBarColor does introduce the exact problems mentioned.
After setting it in one screen it's going to affect all the other screens that don't have this option set, because the line val color = screenForNavBarColor?.navigationBarColor ?: window.navigationBarColor cast it to current color.

Thus I'm starting to lose my objection to your proposal, as it's kind of consistent to have it this way 😄

Copy link
Member

@alduzy alduzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite my initial concerns about the proposal, it now appears consistent with the other options. I think we can proceed.

kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this pull request Aug 20, 2024
## 📜 Description

Update RN to latest release to get rid off patches & workarounds.

## 💡 Motivation and Context

Simply bumped RN version:
- removed unused patch for RN;
- removed patch for `react-navigation`;
- added patch for RNS
software-mansion/react-native-screens#2301

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### JS

- updated RN to latest version;
- removed patches;
- removed unnecessary `navigationBarTranslucent` option;

### E2E

- update RN KeyboardAvoidingView assets 🤷‍♂️ 

## 🤔 How Has This Been Tested?

Tested on CI

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@alduzy alduzy requested a review from WoLewicki August 22, 2024 10:04
@@ -225,7 +225,11 @@ object ScreenWindowTraits {
val window = activity.window

val screenForNavBarTranslucent = findScreenForTrait(screen, WindowTraits.NAVIGATION_BAR_TRANSLUCENT)
val translucent = screenForNavBarTranslucent?.isNavigationBarTranslucent ?: false
val translucent = screenForNavBarTranslucent?.isNavigationBarTranslucent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it can be collapsed into one line:

val translucent = screenForNavBarTranslucent?.isNavigationBarTranslucent ?: return

Let me know if you are okay with these changes and I'll rework the code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it return when translucent is false too then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think safe-operator + elvis operator will execute "else" only if predicate is null (not false)

At least Android studio offers such refacoring + according to the docs it looks like it's indeed truth: https://kotlinlang.org/docs/null-safety.html#elvis-operator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so if you could check if it really works that way (seems counterintuitive to me 😅 ) than I'm okay with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WoLewicki I double checked and it works as expected - false/trues passed down, but null is filtered out 👍

Below is the image from Android Studio that offered refactoring 😅

image

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works as we would want it to work then I'm OK with it

@kirillzyusko
Copy link
Contributor Author

Are we waiting for a review from @tboba ? 👀

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, let's proceed 😄

@kirillzyusko
Copy link
Contributor Author

Does anything prevent this from being merged? 👀

@tboba
Copy link
Member

tboba commented Sep 2, 2024

@kirillzyusko Nah, I thought you could do that and I was waiting if you could have anything more to commit here 😄
Let me merge this PR then 👍

@tboba tboba merged commit ee7915f into software-mansion:main Sep 2, 2024
4 checks passed
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…ecified) (software-mansion#2301)

## Description

I have next navigator structure:
- JS
   - native-stack

`native-stack` navigator customizes options as:

```ts
{
    headerShown: false,
    statusBarTranslucent: true,
    navigationBarColor: "#FFFFFF",
    navigationBarTranslucent: true,
},
```

When I go to `native-stack` - everything works well: the nav bar changes
color. However, when I go back, then I'm getting a gray space in the
bottom of my screen.

It happens because we disable mode `edge-to-edge` by calling
`WindowCompat.setDecorFitsSystemWindows(window, true)` (the gray space
appears because before we were already in edge-to-edge mode, because I
had `KeyboardProvider` mounted in `App.tsx`).

So to fix this problem I decided explicitly check for boolean value for
`navigationBarTranslucent` and set `decorFitsSystemWindows` only when we
have an actual boolean value.

If you think that it's a problem in my project, then, please, let me
know the way to fix it 😊

## Changes

- call `WindowCompat.setDecorFitsSystemWindows(window, true)` only if
`isNavigationBarTranslucent` has a boolean value;

## Screenshots / GIFs

### Before


![telegram-cloud-photo-size-2-5321328488650760881-y](https://github.com/user-attachments/assets/4c1ef654-3146-44bd-aa00-6a810c2aa0aa)

### After


![telegram-cloud-photo-size-2-5321328488650760882-y](https://github.com/user-attachments/assets/7920cf2c-e5b7-46e9-bb75-a581ce2dab2a)

## Test code and steps to reproduce

I tested in `react-native-keyboard-controller` example app, but if you
need to test it in your code - let me know, and I'll try to prepare a
reproduction code.

## 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
- [x]
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
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [x] Ensured that CI passes
kkafar pushed a commit that referenced this pull request Oct 25, 2024
…ecified) (#2301)

## Description

I have next navigator structure:
- JS
   - native-stack

`native-stack` navigator customizes options as:

```ts
{
    headerShown: false,
    statusBarTranslucent: true,
    navigationBarColor: "#FFFFFF",
    navigationBarTranslucent: true,
},
```

When I go to `native-stack` - everything works well: the nav bar changes
color. However, when I go back, then I'm getting a gray space in the
bottom of my screen.

It happens because we disable mode `edge-to-edge` by calling
`WindowCompat.setDecorFitsSystemWindows(window, true)` (the gray space
appears because before we were already in edge-to-edge mode, because I
had `KeyboardProvider` mounted in `App.tsx`).

So to fix this problem I decided explicitly check for boolean value for
`navigationBarTranslucent` and set `decorFitsSystemWindows` only when we
have an actual boolean value.

If you think that it's a problem in my project, then, please, let me
know the way to fix it 😊

## Changes

- call `WindowCompat.setDecorFitsSystemWindows(window, true)` only if
`isNavigationBarTranslucent` has a boolean value;

## Screenshots / GIFs

### Before

![telegram-cloud-photo-size-2-5321328488650760881-y](https://github.com/user-attachments/assets/4c1ef654-3146-44bd-aa00-6a810c2aa0aa)

### After

![telegram-cloud-photo-size-2-5321328488650760882-y](https://github.com/user-attachments/assets/7920cf2c-e5b7-46e9-bb75-a581ce2dab2a)

## Test code and steps to reproduce

I tested in `react-native-keyboard-controller` example app, but if you
need to test it in your code - let me know, and I'll try to prepare a
reproduction code.

## 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
- [x]
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
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [x] Ensured that CI passes

(cherry picked from commit ee7915f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants