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(iOS): white flash on tab change when using native stack #2188

Merged
merged 10 commits into from
Jul 9, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Jun 17, 2024

Description

This PR gets rid of undesired white flashes during maybeAddToParentAndUpdateContainer. The white flash was present on paper architecture when unmountOnBlur option was set to true on parent bottomStackNavigator (see repro).

The affected logic was previously introduced or changed by following PRs:

The removed _hasLayout was initially added by #600 in order to resolve an issue: #432.
However the logic was later changed by #613 in order to fix another issue and the added _hasLayout may not fix anything eventually, as stated by this comment.

Fixes #1645.

Changes

  • removed _hasLayout variable
  • added repros

Screenshots / GIFs

Before

Screen.Recording.2024-06-17.at.17.56.22.mov

After

Screen.Recording.2024-06-17.at.17.55.15.mov

Test code and steps to reproduce

  • added Test1645.js repro to test examples
  • added Test432.tsx repro to test examples

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

ios/RNSScreenStack.mm Outdated Show resolved Hide resolved
@alduzy alduzy requested a review from tboba June 17, 2024 17:12
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I started looking and have some initial question before we can proceed

ios/RNSScreenStack.mm Show resolved Hide resolved
@alduzy alduzy requested a review from kkafar June 18, 2024 13:31
@alduzy alduzy force-pushed the @alduzy/white-flash-on-tab-change-fix branch from 26df1fa to 2ba71d0 Compare June 28, 2024 08:09
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I've played around with the PR for 10 minutes & haven't found any broken behaviour.

Provided that you tested it thoroughly I believe we can proceed.

Thanks a lot!

ios/RNSScreenStack.mm Outdated Show resolved Hide resolved
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Approving under the same conditions as above! Great job!

@alduzy
Copy link
Member Author

alduzy commented Jul 8, 2024

Tested it using the provided Test1645 and Test432 repos, a few other Test* scenarios and ScreensExample app. I haven't found any undesired behaviour.

@tboba
Copy link
Member

tboba commented Jul 8, 2024

Let me do a quick final check on examples if this works and I guess we can proceed with this one!

@tboba
Copy link
Member

tboba commented Jul 8, 2024

Why are we need to test scenario from Test432? Is it also in the scope of this fix?

@alduzy
Copy link
Member Author

alduzy commented Jul 8, 2024

@tboba The _hasLayout variable, that is being removed by this PR was supposed to fix #432, but It looks like it's not the case anymore. Please have a look at the description for more details.

@tboba
Copy link
Member

tboba commented Jul 8, 2024

Alright, I understand 👍

@WoLewicki
Copy link
Member

Did you test on all supported iOS versions? Maybe it was the case only on older ones?

@tboba
Copy link
Member

tboba commented Jul 8, 2024

I've just tested the change on iOS 15.5 and cannot spot any issues there 👍

ios/RNSScreenStack.mm Outdated Show resolved Hide resolved
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.

LGTM!

@alduzy alduzy merged commit b4aa349 into main Jul 9, 2024
5 checks passed
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…-mansion#2188)

## Description

This PR gets rid of undesired white flashes during
`maybeAddToParentAndUpdateContainer`. The white flash was present on
paper architecture when `unmountOnBlur` option was set to true on parent
bottomStackNavigator (see repro).

The affected logic was previously introduced or changed by following
PRs:
- software-mansion#600
- software-mansion#613
- software-mansion#643


The removed `_hasLayout` was initially added by
software-mansion#600 in
order to resolve an issue:
software-mansion#432.
However the logic was later changed by
software-mansion#613 in
order to fix another issue and the added `_hasLayout` may not fix
anything eventually, as stated by [this
comment](software-mansion#432 (comment)).

Fixes software-mansion#1645.

## Changes

- removed `_hasLayout` variable
- added repros

## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/91994767/226a32d7-728b-48dd-b21a-6a1e4195add2

### After


https://github.com/software-mansion/react-native-screens/assets/91994767/32febcf1-d159-4a9d-ae3a-373042732a6d

## Test code and steps to reproduce

- added `Test1645.js` repro to test examples
- added `Test432.tsx` repro to test examples

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
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.

White flash on tab change with @react-navigation/bottom-tabs
5 participants