fix(Android, Tabs): fix flickering tab bar height & contents when entering on ScreenStackFragment#4161
Draft
kkafar wants to merge 1 commit into
Draft
fix(Android, Tabs): fix flickering tab bar height & contents when entering on ScreenStackFragment#4161kkafar wants to merge 1 commit into
kkafar wants to merge 1 commit into
Conversation
This is a small commit, however it introduces profound changes and is based on delicate interactions between our components and the framework. The intention here is to fix an issue, where the contents of a `BottomNavigationView`, when nested inside a `ScreenStack` (stack v4) would jump on enter animation, the activity indicator view would appear mid transition etc. This very issue is fixed by swapping usage of `postFrameCallback` with `post` in `TabsHost`. Let's explain first what is the difference between the two. `ReactChoreographer` is a `Choreographer.FrameCallback` and registers itself as the callback on the actual `Choreographer` instance. `Choreographer`'s frame callbacks are triggered on VSync signal from the platform's renderer, by posting an asynchronous message to the main thread's message queue. This means, that when we add a callback to the `ReactChoreographer` we have a guarantee to be executed before the next frame is drawn, because the view traversals (measure, layout, draw) are executed after user-added frame callbacks. This has one big drawback - the layout we schedule with `postFrameCallback` will be executed BEFORE system dispatches insets, therefore the result of the layout calculation won't take them into consideration and effectively won't be correct. If we draw a frame before we recompute the layout again with insets, we will observe a `BottomNavigationView` height jump. The solution here is usage of the `post` to simply defer the layout computation until the insets are dispatched. Do we have a guarantee that the insets will be dispatched before our `post` callback is executed? Yes, provided that before we call `post` we (or anybody else) request inset dispatch from the hierarchy root (`BottomNavigationView` does this for us). When insets are requested, the `ViewRootImpl` adds a *barrier message* to the message queue, which naturally will be processed before our `post` callback, and will block processing of subsequent *regular messages* until vsync comes and traversals are executed. Okay, so the problem of jumping tab bar height is fixed, but there is still another problem with *activity indicator* (a round view that emphasizes the currently selected item icon). THe problem is that the `NavigationBarItemView` instances are not created and added to `NavigationBarMenuView` until we create & add items. This is done in `TabsContainer.performContainerUpdate`, just before we trigger a fragment transaction adding the selected tab, in turn, this happens synchronously after (sometimes before) we schedule the layout with `post`. That would be fine if the *activity indicator* had been laid out synchronously with the rest of the `BottomNavigationView` contents, but its not the case. The `NavigationBarItemView` schedules with `post` a callback that updates the *activity indicator* **layout params** (not the frame!), which triggers `parent.requestLayout()` and effectively schedules yet another `post` callback from `TabsHost` with the layout callback. Therefore it seems, that the activity indicator would appear after a couple frames of the transition have already been drawn, but it is not the case. **I'm not sure why**, but the activity indicator appears right now immediately on the very first animation frame in the right place and has good dimensions. I'm gonna let this be just because it works, but it could definitely be investigated further (maybe it takes a couple of frames to start the transition? Maybe it's `SafeAreaView` `ViewTreeObserver.OnPreDrawListener` that blocks the first frame from being drawn until the layout is correct? Not sure). Okay, so the tab bar now seems to be working correctly, but we have created yet another problem. This time we've broken the exit transition of the screen we're pushed on top of. Stack v4 uses the *Animation API* to animate the screens. When, in result of a *fragment transaction* fragment's view will be removed from the view hierarchy, the framework will mark the view as disappearing by calling `screenStack.startViewTransition(child)` and later observing that the `child` is detached from the hierarchy. This allows the disappearing view to be still drawn during the transition. It has to call `endViewTransition` at some point. To ensure it's called "at the right moment", [just after it starts the view transition it also schedules an accompanying `Animation` on the view](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:fragment/fragment/src/main/java/androidx/fragment/app/DefaultSpecialEffectsController.kt;l=562-591;drc=c53e65302e39ba824260a5b9e0831d872aa786d1). This is a subclass of `Animation`: `FragmentAnim.EndViewTransitionAnimation`. It ends the transition on animation end OR after [it detects that it has to been queried for transformation between two *post callbacks*.](https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:fragment/fragment/src/main/java/androidx/fragment/app/FragmentAnim.java;l=226-268;drc=c53e65302e39ba824260a5b9e0831d872aa786d1). This way it determines that the animation is "no longer active" and calls the `endViewTranstion`. This "heuristic" is completely wrong though, in case ANY `ViewTreeObserver.OnPreDrawListener` blocks the draw process by returning `false` from `onPreDraw`. In particular our `SafeAreaView` does exactly that, because it needs to wait until the layout is correct and insets are applied before it allows the first frame to be drawn. This means that the transition will be ended prematurely, right after the first frame is drawn, which causes the screen to simply disappear and therefore a flash. To workaround the interaction described above I've added a hack in `ScreensCoordinatorLayout` & `ScreenStack` that blocks the premature calls to `endViewTransition` and moves the responsibility of ending the transition to the `ScreenStack`. We've considered an alternative approach, but this one, despite being a hack, seemed like the least disruptive one. Considered alternative: * Simply return `true` from `SafeAreaView`'s `onPreDraw`. This would allow the transition to be ended at the right moment, but it might cause a flash of un-inset content during the first frame of the transition, which is even worse than the current issue. I haven't been able to trigger that effect though. The `SafeAreaView` seemed to be working correctly despite that change, but decided it's a real risk. It seems that this is not an end of the problems. We've noticed that when we set a large font size for the tab bar item label, the safe area view receives few incorrect layouts. The tab bar is thankfully (:D) stable, however the safe area jump is visible.
This file contains hidden or 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
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.
This is a small commit, however it introduces profound changes and is
based on delicate interactions between our components and the framework.
The intention here is to fix an issue, where the contents of a
BottomNavigationView,when nested inside a
ScreenStack(stack v4) would jump on enter animation,the activity indicator view would appear mid transition etc.
This very issue is fixed by swapping usage of
postFrameCallbackwithpostinTabsHost. Let's explain first what is the difference betweenthe two.
ReactChoreographeris aChoreographer.FrameCallbackand registersitself as the callback on the actual
Choreographerinstance.Choreographer's frame callbacks are triggered on VSync signal from theplatform's renderer, by posting an asynchronous message to the main
thread's message queue. This means, that when we add a callback to the
ReactChoreographerwe have a guarantee to be executed before the nextframe is drawn, because the view traversals (measure, layout, draw) are
executed after user-added frame callbacks. This has one big drawback -
the layout we schedule with
postFrameCallbackwill be executed BEFOREsystem dispatches insets, therefore the result of the layout
calculation won't take them into consideration and effectively won't be
correct. If we draw a frame before we recompute the layout again with insets,
we will observe a
BottomNavigationViewheight jump.The solution here is usage of the
postto simply defer the layoutcomputation until the insets are dispatched. Do we have a guarantee
that the insets will be dispatched before our
postcallback is executed?Yes, provided that before we call
postwe (or anybody else) requestinset dispatch from the hierarchy root (
BottomNavigationViewdoes thisfor us). When insets are requested, the
ViewRootImpladds abarrier message to the message queue, which naturally will be
processed before our
postcallback, and will block processing ofsubsequent regular messages until vsync comes and traversals are executed.
Okay, so the problem of jumping tab bar height is fixed, but there is
still another problem with activity indicator (a round view that
emphasizes the currently selected item icon). THe problem is that the
NavigationBarItemViewinstances are not created and added toNavigationBarMenuViewuntil we create & add items. This is done inTabsContainer.performContainerUpdate, just before we trigger afragment transaction adding the selected tab, in turn, this happens
synchronously after (sometimes before) we schedule the layout with
post.That would be fine if the activity indicator had been laid out
synchronously with the rest of the
BottomNavigationViewcontents, butits not the case. The
NavigationBarItemViewschedules withposta callbackthat updates the activity indicator layout params (not the
frame!), which triggers
parent.requestLayout()and effectivelyschedules yet another
postcallback fromTabsHostwith the layout callback.Therefore it seems, that the activity indicator would appear after a
couple frames of the transition have already been drawn, but it is not
the case. I'm not sure why, but the activity indicator appears right
now immediately on the very first animation frame in the right place and
has good dimensions. I'm gonna let this be just because it works, but
it could definitely be investigated further (maybe it takes a couple of
frames to start the transition? Maybe it's
SafeAreaViewViewTreeObserver.OnPreDrawListenerthat blocks the first frame frombeing drawn until the layout is correct? Not sure).
Okay, so the tab bar now seems to be working correctly, but we have
created yet another problem. This time we've broken the exit transition
of the screen we're pushed on top of.
Stack v4 uses the Animation API to animate the screens. When, in
result of a fragment transaction fragment's view will be removed from
the view hierarchy, the framework will mark the view as disappearing by
calling
screenStack.startViewTransition(child)and later observingthat the
childis detached from the hierarchy. This allows thedisappearing view to be still drawn during the transition. It has to
call
endViewTransitionat some point. To ensure it's called "at theright moment", just after it starts the view transition it also
schedules an accompanying
Animationon the view. This is a subclass ofAnimation:FragmentAnim.EndViewTransitionAnimation. It ends thetransition on animation end OR after it detects that it has to been
queried for transformation between two post callbacks..
This way it determines that the animation is "no longer active" and calls
the
endViewTranstion. This "heuristic" is completely wrong though, incase ANY
ViewTreeObserver.OnPreDrawListenerblocks the draw process byreturning
falsefromonPreDraw. In particular ourSafeAreaViewdoes exactly that, because it needs to wait until the layout is correct
and insets are applied before it allows the first frame to be drawn.
This means that the transition will be ended prematurely, right after the
first frame is drawn, which causes the screen to simply disappear and
therefore a flash.
To workaround the interaction described above I've added a hack in
ScreensCoordinatorLayout&ScreenStackthat blocks the prematurecalls to
endViewTransitionand moves the responsibility of ending thetransition to the
ScreenStack.We've considered an alternative approach, but this one, despite being
a hack, seemed like the least disruptive one. Considered alternative:
Simply return
truefromSafeAreaView'sonPreDraw.This would allow the transition to be ended at the right moment, but it
might cause a flash of un-inset content during the first frame of the transition,
which is even worse than the current issue.
I haven't been able to trigger that effect though. The
SafeAreaViewseemed to be working correctly despite that change,but decided it's a real risk.
It seems that this is not an end of the problems. We've noticed that
when we set a large font size for the tab bar item label, the safe area
view receives few incorrect layouts. The tab bar is thankfully (:D)
stable, however the safe area jump is visible.