-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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: inactive screen visible for one frame #11315
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @autofix-ci[bot]! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #11315 +/- ##
==========================================
+ Coverage 77.12% 77.16% +0.04%
==========================================
Files 211 211
Lines 6269 6280 +11
Branches 2475 2481 +6
==========================================
+ Hits 4835 4846 +11
Misses 1380 1380
Partials 54 54 ☔ View full report in Codecov by Sentry. |
The Expo app for the example from this branch is ready! |
e78cd13
to
c784087
Compare
Hi @WoLewicki thanks for this fix, is it possible to apply this same fix to @react-navigation/native-stack? could you guide me on which files to modify because I have the same behavior but in this package? Thank you very much |
@jonathanm-tkf |
Correct @WoLewicki, the temporary fix I made was to place a normal |
The Expo app for the example from this branch is ready! |
@@ -609,6 +622,7 @@ export class CardStack extends React.Component<Props, State> { | |||
}) | |||
: STATE_TRANSITIONING_OR_BELOW_TOP; | |||
} | |||
this.lastActivityStateForIndex[index] = isScreenActive; |
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.
This makes render impure. We need to figure out an alternative way that keeps render pure.
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.
Do you have an idea what could we do to achieve it?
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.
Maybe let's have a call to understand the logic and come up with an idea
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.
I don't remember much more than what is written in the description of the PR, but we can do that for sure 😅
The Expo app for the example from this branch is ready! |
d9a76fb
to
6f56ff2
Compare
6f56ff2
to
d384587
Compare
In a scenario when we have 3 screens pushed on the stack (let's name them A, B, C in the nesting order) and we navigate from C to B, the A screen was attached for about one frame (and it should not be since it will not be visible after transition), causing problems with libraries that subscribe to native attach/detach process.
It happened because when the third screen disappears, the length of the routes becomes smaller, so in our case length becomes
2
,activeScreensLimit
is still1
, so we do go intoelse
in line609
for screenA
, which was inactive during the transition. In there, theoutputValue
is set correctly toSTATE_INACTIVE
for after the transition, but it is interpolated from1
to0
(STATE_INACTIVE
) (lines611-616
), so it gets the1
value. The transition ends in the next frame, so it quickly becomes0
again, but still was attached for this one frame.The solution is to keep the information of what was the last state for each index, and if it was
STATE_INACTIVE
and will be such after the transition, there is no sense to make it visible. It would be nice to test it in many scenarios for sure.@graszka22 can confirm that it fixes a problem with native attach/detach in their library.