1393057 - Remove transition from toolbar button hover state

Status: RESOLVED FIXED

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

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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

Comment 1

[Chris Lord [:cwiiis]]

It'd be nice to remove the hover-in (enter) animation, but retain the hover-out (leave) animation, rather than kill the animation entirely.

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: remove-hover-transition-from-toolbar-icons.patch

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.

Comment 3

[:Gijs (he/him)]

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 )

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: remove-hover-transition-from-toolbar-icons.patch

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c55653c204f
Remove hover transition from toolbar buttons. r=Gijs

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This (or the other bug from this checkin-needed push) seems to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=130440527&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/77a07c56db0dab0d8dc7ece3c6466746f80e7076

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 8

[:Gijs (he/him)]

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

Comment 9

[:Gijs (he/him)]

Attachment: Remove hover transition from toolbar buttons,

MozReview-Commit-ID: GdBVMsBYI6S

Comment 10

[:Gijs (he/him)]

Comment on attachment 8908081 [details] [diff] [review]
Remove hover transition from toolbar buttons,

The patch needed rebasing.

Comment 11

[:Gijs (he/him)]

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.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1393057 - Remove hover transition from toolbar buttons,

Review commit: https://reviewboard.mozilla.org/r/179784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179784/

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908097 [details]
Bug 1393057 - Remove hover transition from toolbar buttons,

https://reviewboard.mozilla.org/r/179784/#review185004

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f9aea31e446
Remove hover transition from toolbar buttons, r=mikedeboer

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5f9aea31e446