Corrected transition animation when replacing stack #838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fluently navigate from A -> B -> C to A -> D - currently this does a pop enter animation for D. B and D were given same key so it was like a refresh navigation instead of a replace. B and C should exit and D enter - depends on native platform how it shows that though. Changed the keys to be crumb-stateKey (same as on mobile so scenes are reused if matching state). But then also made sure the top most scene uses the new key - it used to use the previous key. So in the scenario D would have a new key. This also works for A -> B -> C to A -> D -> C then back 1. The first navigation D has B's key because D isn't on top. Then when going back give D it's own key and then should get B and C exiting and D entering. Haven't tested it yet but that's the idea. Also changed Scene to use url to update - copied this over from mobile too. Otherwise B would turn into D when it was exiting. Need B to stay as B and D to be D - so only updating Scene when url matches gives this. Then changed the native stack to run the replace after the back. Need both to run because it's back to B then replace to D.
It works on ios although it plays the push exit on A when should really play the pop exit. That's ok - it's how it is. On Android it was trickier. Had to popBackStackImmediate otherwise the replace didn't work - no idea why - tried things like combining the pop backs but didn't work. Ran the wrong animation by default. It was running the pop Enter on D - makes sense because that's what you'd run on A when popping B off. But now D is treated like a new scene so need to run push enter. Added clause to crumb = oldCrumb on React Native side and returned unmount style for enter instead of crumb style
Navigate from A -> B to A then don't want to do back replace But if go from A -> B to D then do. Used reactViewController to check for now - but planning on switching to a bool on the SceneView to indicate whether it's been added or not. Feels cleaner and don't have to worry about timing - when it's added to window, for example
It looked odd to run the pop first and then the replace - the typical scenario is welcome -> login to home. Don't want the welcome scene to come back first and then home to enter and welcome to pop. That's what was happening. Looks better if home enters and welcome exits (ideally pop exit but can't get that on ios)
Thought this didn't work for some reason, but it's the same check that's in the crumb == currentCrumb case
There was a time when the later keys were longer than earlier. But that was a while back when using the ++ notation for replace. Haven't done that for a while plus there' no need to sort scenes at the same crumb. There's only one scene per crumb in the stack so it doesn't matter which is rendered first
Was using superview but that's not set for a scene in a crumb. Need to tell apart a back navigation from a back and replace navigation. With the first want to animate because popping to existing scene. With the second don't want to animate because replacing the scene - so don't animate the pop but animate the replace
Can use getParent on android, unlike superview on ios, because it still returns true for scene in crumb. But prefer to match the stacked boolean on ios and also can't guarantee when the parent gets set on android because the fragment stuff is async. Safer and consistent to use own stacked boolean to track once a scene's added to stack
That Pair warning has been annoying me for a long time, so switched from array to array list and then can use generics again
When swipe or tap back then scene B changed to D. So would get D entering and exiting. That's because the url on B would update to D on the fluent navigation. Changed to update all keys instead of trying to keep the crumbs with the same keys as now. But then still when swipe back on Android scene B changed into D. Then would get D entering and exiting. That's because the peek navigate would run on B and update state context to D. Added check so only updates context if url matches - same as in getDerivedStateFromProps. Not sure this is the correct behaviour but it's consistent with other fluent. Maybe should only do push replace when the replaced scene is on top of stack. Here B is crumb when it's replaced so maybe that should silently replace instead of push replace when D appears? Also but when after swiping back twice to A and then navigating with double taps, instead of navigating 2 scenes it would jump back to A. Set the duration to 1s to give time for the second tap. Turns out it was running the resumeNavigationRef again - guess the rest happened with the second tap. This was probably always a bug but only noticed when testing this.
Don't want this to do a back then push replace because it's too weird. An example that shows how weird is if on stack A -> B -> C and click deep link to D -> E -> C then would get back then push replace when going back to E and then again to D. Then only time a (back and) push replace should happen is when the change is visible - so when it's the top scene. So, A -> B -> C to A -> D should do back to B then push replace with D. And obviously then one that started it A -> B to A -> C should also do push replace. That's because both cases the scene that's replaced is on top of stack and so visible. But with A -> B -> C to A -> D -> C the replace isn't visible. So when going back to it the replace has already happened silently so no push replace. Changed the keys back to how they were but only changed the current key if the scene has changed. So compared state with state of prev crumb in that position and only changed key if they're different. So A -> B -> C to A -> D -> C then back to A -> D - this won't change key because the back shows D hasn't changed. But A -> B -> C to A -> D changes key because D different to B. Also put peek in Scene back how it was because swipe back should always update. Swipe back will never lead to push replace so it should always update to latest.
This matches the check in the keys. Checks if, when navigating back, the state is different to the previous state at that crumb - if so then doing a push replace so need different animation because not skipping the back animation in favour of the push replace. Think it worked before because the back animations aren't used anyway - unless it's a push replace. The back animations already set up when the scene is first pushed onto stack and popped as a crumb
The crumbs don't include the current state (unlike the keys) so don't subtract 1. It fell over when navigating back to home because crumbs length is 0
Navigate fluently from A -> B -> C to A -> D then want D to animate with push enter not pop enter because it's new. But navigate fluently from A -> B -> C to A -> B*, where state is the same but data is different, should do pop enter as now. Also A -> B -> C to A -> D -> C then back should do pop enter because only do the push enter when going to that state, not when silently replacing. Already done these changes on native so copying them over. The key and scene navigation event logic now identical on native and web/mobile
The test was failing but it basically turned the test into A -> B to C because the middle and end navigations happened together. Wrapping in act restored the correct behavior and got it passing - because it shouldn't to a push replace because the replace is silent (C not visible when replacing A)
A -> B -> C to B keeps 4 scenes in because B gets different key to A now. A and first B aren't rendered in stack though because they're fluently navigated over
A -> B to C keeps A in stack because C is different state
Same as previous commit, A -> B to C keeps all three in stack because C gets different key to A
Same as previous 2 commits, A -> B to C keeps all 3 in stack because C has different key to A now
Navigate fluently from A -> B -> C to A -> D then url said B instead of D - so going back and forward in history went back to B instead of D. So when going back in history to match navigation did a replace history at the end - used the backCrumb to store the end url. Do this whenever there's a backCrumb because that only happens when history is catching up with state context.
Stack gives C a different key and keeps A and B around for browser history
Stack gives D a different key to C and keeps A, B and C around for browser history
If history forward to B then should remember state
Navigate from A -> B -> C -> D then go back twice fast using cmd + backspace. Weird things would happen - scene C would replace B instead of going back to B. Introduced this bug in commit a415179 when working around Android bug. Switched to use keys instead of the crumb on the scene and that's no good when fast navigating because the keys don't change. It's ok for the first back but when the second one happens the keys haven't changed so it sends the same back crumb to JavaScript and so nothing happens. The back still happens on the native side and that's why funny things happen - JavaScript sends back down A-> B -> C with next state update (onRest) and end up doing a replace because crumb matches old crumb. There's only A -> B in the fragment container so end up with A -> C - but things are still out of step. JavaScript has 3 keys and native only 2 fragments. Can't go back to using crumb because the original commit was working round Android bug. So changed to use back stack entry count. Tested on Android 34 and it works (will test on 33 later) - the back stack changes between back taps
Tested on 33 and it didn't work. The BackStackEntry count was after the remove, not before like on Android 34. Tried to go back to the old code (before commit a415179) but the isRemoving bug was still there - it was different on Android 33 and 34. Tried to use isVisible in combo with isRemoving and could do it (isVisible ? isVisible : isRemoving) but this didn't work on Android 35. The double tab back went back 3 instead of 2. So put it back to using BackStackEntry and added <= 33 check to subtract 1 from the entry count. Prefer to have backward check than a forward check - assume it's gonna keep working how it does now in 34 and 35
This is for A -> B -> C to A -> D - makes the back replace animation a push enter instead of pop enter
grahammendick
added a commit
that referenced
this pull request
Dec 13, 2024
In the zoom example navigate to red then back then to quickly tap on crimson. This would do nothing. And no matter how many times tapping crimson did nothing still. The fast crimson navigation happened before the red scene was removed from the stack on the React side. Because of #838 both red and crimson get the same key so on native side the scene is already added to the stack because it's the same key. So nothing happens. The same scenario would sometimes give an error instead of doing nothing. If the pop from native red scene fired at just the right time then the scene would be removed and the view on the native crimson scene (same as the red scene) would be null. Then findNavigationBar would throw an error trying to append crumb to string. Realised that #838 had actually caused a regression of #585. Giving scenes the same key meant that funny things could happen when navigating fast. The native pop could happen and remove scenes that should be there. It’s actually hard to recreate #585 error scenario normally - but shared element navigation made it easy for some reason. Guess that popping keeps the popped scene around a bit longer so it’s easier to navigate again before the scene destructor is called by iOS. Anyway put back the count from #585 and the problem is fixed. Will check later whether #838 still works.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace navigation is where you change the scene at a given crumb. For example, fluently navigating from A → B to A → C replaces scene B with C; or navigating from A → B to C replaces A with C. In the first example the un/mount style animation ran for C but in the second the crumb style animation ran for C. This PR corrects the second example so that the un/mount style animation runs for C, too. Did this for native and mobile.
The typical replace example is where the login stack is replaced once the user logs in. Always want the home scene of the logged-in stack to run the un/mount style not the crumb style. Before this PR this would only work if there was a single login scene. Now it works no matter how many scenes are involved in the login sequence.
This PR only runs the un/mount style animation if the replacing scene is at the top of the stack. For example, fluently navigating from A → B to C → B still runs the crumb style animation when navigating back to C. A is replaced by C silently because C isn't visible when it replaces A. The typical example is a deep link that might navigate from A → B → C to D → E → B. It would be too disruptive and confusing to run the un/mount style animation when navigating back to E and again when back to D.