Closed Bug 1015450 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file)

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.
I just realized we don't have an animation without bug 997477.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Depends on: 997477
Nevermind, this happens on phone too.
No longer depends on: 997477
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.
Attachment #8430400 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8430400 - Flags: review?(lucasr.at.mozilla) → feedback+
(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
Flags: needinfo?(lucasr.at.mozilla)
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.
Attachment #8430400 - Flags: feedback+ → review+
Flags: needinfo?(lucasr.at.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.
https://hg.mozilla.org/mozilla-central/rev/645ceba6f720
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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
Attachment #8430400 - Flags: approval-mozilla-aurora?
Attachment #8430400 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.