Tabs panel animation errors

RESOLVED DUPLICATE of bug 1314322

Status

()

Firefox for Android
General
RESOLVED DUPLICATE of bug 1314322
2 years ago
2 years ago

People

(Reporter: Tom Klein, Assigned: Tom Klein)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox52 affected)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

2 years ago
To crib from the upcoming commit message:

The current tabs list has some intermittent timing dependent animation bugs.

If we add a tab A and the currently selected tab B gets moved as a result, and if the UNSELECT on B arrives after the move animation on B has started, the bind on B which occurs as a result of the UNSELECT is reseting translationY to 0, which effectively cancels the move (since it was animating translationY to 0).  If the UNSELECT arrives before the move is queued, which is what usually happens, then the animation runs normally.  We shouldn't be resetting view position attributes in bind.

The other fix is to DefaultItemAnimator's animateChange, which is just generally not well behaved in the case where setSupportsChangeAnimations(false) is set and multiple animations on a view can occur at the same time.

For example, if tab A is added and then the SELECT for tab A arrives, the animateChange call resulting from the SELECT's notifyItemChanged will be rerouted to an animateMove, which cancels the existing add animation without
taking it over (animateMove only checks for already running translation* animations, not alpha animations as in an add).  The add animation cancel causes the added tab to appear immediately instead of waiting to appear after other
tabs have moved to make room for it.  If the SELECT arrives before the animateAdd for the add is called, which is what usually happens, then the animation runs normally.  We shouldn't be changing an animateChange into an animateMove if the change doesn't actually involve a move.

More generally the provided change to animateChange prevents the unneeded passing off of existing running move animations into new pending animations when the animateChange call started off with no new move data.

Video of both errors occurring at once here: https://www.youtube.com/watch?v=eBZYyBo-ELM

The same errors appear in the grid case, so marking it as blocking.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Heads up: my explanation here:

> If we add a tab A and the currently selected tab B gets moved as a result,
> and if the UNSELECT on B arrives after the move animation on B has started,
> the bind on B which occurs as a result of the UNSELECT is reseting
> translationY to 0, which effectively cancels the move (since it was
> animating translationY to 0).

isn't quite right (the animation would probably just keep trucking in that case, ignoring the fact that somebody had set translationY while it was animating it), though the patch is correct.  I'll add some logcats to give the correct explanation and then update the commit message.  Sorry about that!
(Assignee)

Comment 3

2 years ago
To start with, some proper steps to reproduce:

1) Load two tabs in portrait mode in the tabs panel.
2) Close tab 0, then undo the close.

Watch for tab 0 to appear before tab 1's move animation has completed, and watch for tab 1 to jump to its final position before its move animation has completed.  Similar errors do appear with more than two tabs open, it's just not quite so obvious since at least one of the tabs (the one that was neither selected nor unselected) animates correctly in that case.

The error is timing related and seemingly random, but I usually see an error if I repeat step 2) 5-10 times or so.
(Assignee)

Comment 4

2 years ago
Here's a logcat of what's happening to cause tab 1 to appear in its final position without animation when tab 0 gets added.

49.768 D/GeckoBrowserApp: BrowserApp.onTabChanged: 7: ADDED
49.768 D/***adapter.notifyItemAdded: position 0
49.778 D/***BIND: ViewHolder{2b97d33b position=0}
49.778 D/***animateMove: ViewHolder{3be4db7d position=1}; fromY=0, toY=430
49.778 D/***in animateMove: setTranslationY(-430)
49.778 D/***animateAdd: ViewHolder{2b97d33b position=0}
49.788 D/GeckoBrowserApp: BrowserApp.onTabChanged: position 0 SELECTED
49.788 D/GeckoBrowserApp: BrowserApp.onTabChanged: position 1 UNSELECTED
49.788 D/***adapter.notifyItemChanged: position 0 (position was selected)
49.788 D/***adapter.notifyItemChanged: position 1 (position was unselected)
49.799 D/***runPendingAnimations: there are add and move animations
49.799 D/***move animation start: ViewHolder{3be4db7d position=1}, translationY=-430
49.799 D/***BIND: ViewHolder{2b97d33b position=0}
49.799 D/***BIND: ViewHolder{3be4db7d position=1} --> set translationY=0
49.799 D/***animateChange (for the unselected): ViewHolder{3be4db7d position=1} --> change to animateMove
49.799 D/***animateMove: ViewHolder{3be4db7d position=1}, fromY=430, toY=430
49.799 D/***in animateMove: compute deltaX=0, deltaY=0 (because translationY=0)
49.799 D/***in animateMove: call endAnimation on ViewHolder{3be4db7d position=1} to cancel cur animation
49.799 D/***in animateMove: cancel this animateMove since deltaX=0, deltaY=0
Normally this is the point where this animateMove would have picked up the translationY from the
original animateMove for position 1 and continued the animation, but since the bind set translationY=0,
this animateMove decides there's nothing to animate.

The fix is to not reset position values in adapter.bind.  (It would maybe be enough (you might still get some animation flicker) to simply not let an animateChange be turned into an animateMove when fromX=toX and fromY=toY in the animateChange, which is the other fix we apply in this patch, but from a more general standpoint I don't think we should be resetting view properties in bind when bind can get called in the middle of a running animation.)
(Assignee)

Comment 5

2 years ago
And here's a logcat of what's happening to cause tab 0 to appear without animation when it gets added (this is the same run as in comment 4 with different logcat entries highlighted).

49.768 D/GeckoBrowserApp: BrowserApp.onTabChanged: 7: ADDED
49.768 D/***adapter.notifyItemAdded: position 0
49.778 D/***BIND: ViewHolder{2b97d33b position=0}
49.778 D/***animateMove: ViewHolder{3be4db7d position=1}; fromY=0, toY=430
49.778 D/***animateAdd: ViewHolder{2b97d33b position=0}
49.788 D/GeckoBrowserApp: BrowserApp.onTabChanged: position 0 SELECTED
49.788 D/***adapter.notifyItemChanged: position 0
49.788 D/GeckoBrowserApp: BrowserApp.onTabChanged: 1: UNSELECTED
49.788 D/***adapter.notifyItemChanged: position 1
49.799 D/***runPendingAnimations: there are add and move animations
49.799 D/***move animation start: ViewHolder{3be4db7d position=1}, translationY=-430
49.799 D/***BIND: ViewHolder{2b97d33b position=0}
49.799 D/***BIND: ViewHolder{3be4db7d position=1}
49.799 D/***animateChange: ViewHolder{2b97d33b position=0} --> change to animateMove
49.799 D/***animateMove: ViewHolder{2b97d33b position=0}, fromX=fromY=toX=toY=0
49.799 D/***in animateMove: call endAnimation on ViewHolder{2b97d33b position=0}
                            --> cancels the add animation
49.799 D/***in animateMove: compute deltaX=0, deltaY=0
49.799 D/***in animateMove: cancel this animateMove since deltaX=0, deltaY=0
animateMove only picks up the previous animation on its view if the previous animation was
animating translationX or translationY - the add animation was animating alpha, so that
previous animation just gets dropped, so alpha on the view gets set to 1 and position 0
appears immediately (well, as soon as the SELECTED arrives).

The fix here is to not let animateChange be morphed into an animateMove if fromX=toX and fromY=toY in the animateChange.  That fix also stops the needless handing off of an already running move animation into a new pending state, which delays the animation until the next runPendingAnimations (and can cause even further delay if there happens to be a new remove animation in that pending batch, since removes run before adds - I saw this happening when I slowed down animations for testing: you start adding a tab, the tab gets interrupted by a SELECT/UNSELECT and so gets readded to the pending queue, but in the meantime the add animation has pushed one of the tabs offscreen to make room, which triggers a remove animation on that tab, and so you wind up with your initial add animation, which had already been running, paused waiting on a remove animation that's actually occurring off screen!).

This error and the comment 4 error are independent, but usually occur together because the timing that causes one also causes the other.  If the SELECTED and UNSELECTED (which along with the ADD are posted individually) arrive before any animations start then there are no interruptions to the move and add animations, and so they run correctly.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Okay, updated the commit message, no code changes, ready for review.  Sorry for not being more thorough the first time Sebastian!

Comment 8

2 years ago
mozreview-review
Comment on attachment 8804844 [details]
Bug 1313144 - Fix tabs panel animator issues.

https://reviewboard.mozilla.org/r/88690/#review88428

LGTM.

Thank you for debugging this so thoroughly.
Attachment #8804844 - Flags: review?(s.kaspari) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
I realized that the "Close All Tabs" animations weren't being reset after the previous patch (that relied on the old resets in onBindViewHolder, which were removed), and then I (re)discovered RecylerView.onChildAttachedToWindow, which is where the resets should have been happening all along, so ... Hooray for not being thorough enough ;-)  Could you take another look Sebastian?  Thanks!
(Assignee)

Updated

2 years ago
See Also: → bug 1314322
(Assignee)

Comment 11

2 years ago
The changes here were rolled into bug 1314322.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1314322
(Assignee)

Updated

2 years ago
Attachment #8804844 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.