1015450 - Tapping the toolbar during the awesomescreen close animation puts toolbar in inconsistent state

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I slowed down the animations while working on bug 997477 and accidentally put the toolbar in an inconsistent state.

Here's the slowed build: https://people.mozilla.org/~mcomella/apks/mcomella-toolbar_inconsistent.apk

This problem does not occur on the opening animation.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I just realized we don't have an animation without bug 997477.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Nevermind, this happens on phone too.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Prevent toolbar from going into an inconsistent state when tapped when entering editing mode.

Ideally, I think isEditing would not return true if isAnimatingEntry is true (and we'd have UIMode.ANIMATING), however, too much code relies on isEditing to change it cleanly now. Thus, I check both.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8430400 [details] [diff] [review]
Prevent toolbar from going into an inconsistent state when tapped when entering editing mode.

Review of attachment 8430400 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks fine as a protective work-around, but, have you figured out what exactly breaks when we interrupt the animation to change its 'direction'? Strictly speaking, the right fix for this bug would be to just 'reverse' any mid-air animation to/from editing mode as this would make out UI more robust and responsive. I'd really like to at least understand the actual problem here before going ahead with this patch.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Strictly speaking, the right fix for this bug would be to just
> 'reverse' any mid-air animation to/from editing mode as this would make out
> UI more robust and responsive.

I agree that this would be the ideal fix, however, nothing obvious jumps out for the reason for the breakage so is it worth the time to investigate? It's a use case that very few users will witness.

You did also mention that this fix could make the UI more robust, and I agree - but I think that it will take a lot of work to also make the code robust. I assume this code was written with the assumption that this animation would not be interrupted (e.g. [1], why doesn't the UIMode enum contain ANIMATING?), which means it will likely be an effort to find all of the bug points, nevermind safely refactoring it to get it right.

Also, do we have the ability to reverse animations? To avoid that complication, an easier fix (that I don't think we should implement because, again, it's such a small use case) could be to queue the "reverse" until the animation completes, allowing us to avoid messing with the legacy code.

We'll likely be revisiting this code with meta bug 1014848, which might inspire larger refactors - why not worry about this then?

Apply this comment also to bug 1017276.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=703fae2e244c#1000

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8430400 [details] [diff] [review]
Prevent toolbar from going into an inconsistent state when tapped when entering editing mode.

(In reply to Michael Comella (:mcomella) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > Strictly speaking, the right fix for this bug would be to just
> > 'reverse' any mid-air animation to/from editing mode as this would make out
> > UI more robust and responsive.
> 
> I agree that this would be the ideal fix, however, nothing obvious jumps out
> for the reason for the breakage so is it worth the time to investigate? It's
> a use case that very few users will witness.
> 
> You did also mention that this fix could make the UI more robust, and I
> agree - but I think that it will take a lot of work to also make the code
> robust. I assume this code was written with the assumption that this
> animation would not be interrupted (e.g. [1], why doesn't the UIMode enum
> contain ANIMATING?), which means it will likely be an effort to find all of
> the bug points, nevermind safely refactoring it to get it right.
>
> Also, do we have the ability to reverse animations? To avoid that
> complication, an easier fix (that I don't think we should implement because,
> again, it's such a small use case) could be to queue the "reverse" until the
> animation completes, allowing us to avoid messing with the legacy code.
> 
> We'll likely be revisiting this code with meta bug 1014848, which might
> inspire larger refactors - why not worry about this then?
> 
> Apply this comment also to bug 1017276.

My original request for investigation was not about finding all the bugs in the transitions and fixing them in this bug. It was more about getting a better understanding about the limitations of our current transitions as prep work for future improvements.

FWIW, after a quick look, the buggy bits seem to be around the LayoutParams tweaks we do before/after the transitions. Reversing the animations would be a lot simpler they didn't involve layout changes under the hood.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/645ceba6f720

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> FWIW, after a quick look, the buggy bits seem to be around the LayoutParams
> tweaks we do before/after the transitions.

I didn't fully explain what the inconsistent state was and seeing it first hand makes me think there are more problems than this. The toolbar looks like it is in a mix of both display and edit modes (to be expected) and is unusable: tapping it does nothing, but the user is stuck in edit mode.

Perhaps this has something to do with using two animators that both make final state transitions on animation end (e.g. View visibility) with no assumptions about them being cancelled mid-stride. The work with the PreDrawListener (bug 993069) might be useful here.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/645ceba6f720

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8430400 [details] [diff] [review]
Prevent toolbar from going into an inconsistent state when tapped when entering editing mode.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined:
  Users who press the browser toolbar while it closes on phone will put the browser into an inconsistent state that needs on OOM (e.g. swipe close) close of Fennec to fix. Note that this use case is pretty unlikely, however.
 
Testing completed (on m-c, etc.): 
  Local

Risk to taking this patch (and alternatives if risky): Low - we are guarding editing mode from being entered under certain conditions (moderate risk), however, it's a small patch with very direct conditions so problems are unlikely.
 
String or IDL/UUID changes made by this patch: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09f2b783db09