Closed Bug 1393057 Opened 3 years ago Closed 3 years ago

Remove transition from toolbar button hover state

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

All
Unspecified
enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- fixed

People

(Reporter: phlsa, Assigned: phlsa)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(1 file, 3 obsolete files)

A quick way to improve the perceived responsiveness of the Firefox UI is to remove the hover transition we have on toolbar buttons (currently 150ms).

This seems to be the relevant CSS to do this:
http://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbuttons.inc.css#113
It'd be nice to remove the hover-in (enter) animation, but retain the hover-out (leave) animation, rather than kill the animation entirely.
Whiteboard: [photon-animation][triage]
Flags: qe-verify-
Priority: -- → P4
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
This removes the transition on hover on all toolbar buttons.
It does, however, preserve the animation of the shadow on the back button, since that is much more subtle.

I agree that it would be even better to preserve the fade-out, but for now this is a net-positive for performance, so we should get it into 57 if possible.
We can file follow-up bugs for that.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8907041 - Flags: review?(gijskruitbosch+bugs)
Priority: P4 → P1
Comment on attachment 8907041 [details] [diff] [review]
remove-hover-transition-from-toolbar-icons.patch

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

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +123,5 @@
>  toolbar .toolbarbutton-1 > .toolbarbutton-text,
>  toolbar .toolbarbutton-1 > .toolbarbutton-badge-stack {
>    padding: var(--toolbarbutton-inner-padding);
>    border-radius: var(--toolbarbutton-border-radius);
> +  transition-property: box-shadow;

If the only thing we're still doing this for is the back button, then please move the transition-property and transition-duration to the relevant block (probably https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/toolbarbuttons.inc.css#205 )
Attachment #8907041 - Flags: review?(gijskruitbosch+bugs)
Moved the code accordingly.

Also, I played around a bit with preserving the transition on mouse-out. Unfortunately, that would still give as a perceived perf it, since it leads to a visible lag when moving from one item to another.
Interestingly, the Windows task bar uses a similar effect without looking as laggy. I'm not sure if this is due to our implementation or because of some other parameters. I tried simply shortening the animation time, but that didn't fix it. So this patch still removes the transition entirely.
Attachment #8907041 - Attachment is obsolete: true
Attachment #8907089 - Flags: review?(gijskruitbosch+bugs)
Attachment #8907089 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c55653c204f
Remove hover transition from toolbar buttons. r=Gijs
Keywords: checkin-needed
So, this is a CSS-only change, so it is unlikely that it causes test failures.
That being said, I don't know enough about how our tests work to really back up that claim.
Flags: needinfo?(philipp) → needinfo?(gijskruitbosch+bugs)
I don't see how this would cause any test failures, but stranger things have happened at sea, so here's a trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4aabfd6404bd9f3fb602b6e921ffdc088918c9d
MozReview-Commit-ID: GdBVMsBYI6S
Comment on attachment 8908081 [details] [diff] [review]
Remove hover transition from toolbar buttons,

The patch needed rebasing.
Attachment #8908081 - Flags: review+
Attachment #8907089 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Uh, so I can reproduce this locally, and I have bad news... that test is super-busted. The manifest has it disabled in non-e10s (it's perma-broken for me when running it locally with e10s disabled), and running it in e10s breaks if I make the test's transition promises for the find bar actually wait for the find bar transition, rather than transitions on the buttons on the find bar (which this patch is removing). Worse, the actual screenshots it's producing in e10s are just blank, except for the find bar. So it's basically testing whether the find bar is showing up in both screenshots, which breaks if I adjust the promises to actually wait for the find bar's visibility transition because it doesn't always happen in the cases where we wait for them.

I'm going to file a followup bug to fix the test and re-enable it, and then disable it in this patch and ask Mike to rubberstamp that, and then this should be good to land.
See Also: → 1399845
Attachment #8908081 - Attachment is obsolete: true
Comment on attachment 8908097 [details]
Bug 1393057 - Remove hover transition from toolbar buttons,

https://reviewboard.mozilla.org/r/179784/#review185004
Attachment #8908097 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5f9aea31e446
Remove hover transition from toolbar buttons, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/5f9aea31e446
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.3 - Sep 19
You need to log in before you can comment on or make changes to this bug.