1377184 - Use new Photon hover effect also on "Show more bookmarks" and extensions buttons icons in Bookmarks Toolbar

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: old hover effect.png

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: new hover effect.png

Comment 2

[Dão Gottwald [:dao]]

This belongs in the Theme component.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1377184 - Consistently use the custom toolbar button styling in all browser toolbars.

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

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8895278 [details]
Bug 1377184 - Consistently use the custom toolbar button styling in all browser toolbars.

https://reviewboard.mozilla.org/r/166462/#review171714

::: browser/themes/shared/toolbarbuttons.inc.css:91
(Diff revision 1)
>  .findbar-button {
>    -moz-appearance: none;
>    padding: 0;
>  }
>  
> -#nav-bar .toolbarbutton-1 {
> +toolbar .toolbarbutton-1 {

I wonder why we need the toolbar selector at all here, are there toolbarbutton-1 elements that are not in a toolbar?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> > -#nav-bar .toolbarbutton-1 {
> > +toolbar .toolbarbutton-1 {
> 
> I wonder why we need the toolbar selector at all here, are there
> toolbarbutton-1 elements that are not in a toolbar?

In the overflow panel and the customization palette.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4376773f9cd
Consistently use the custom toolbar button styling in all browser toolbars. r=johannh

Comment 7

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

Backed out for failing browser-chrome's browser_startup_images.js: chevron.svg should have been shown:

https://hg.mozilla.org/integration/autoland/rev/aa5e52bbd13b6c60d1f478c6ea9504e721b5b6c6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4376773f9cd977c8c450f71ae33e937ed4fd026&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122037618&repo=autoland

[task 2017-08-09T17:11:50.494825Z] 17:11:50     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Loaded image chrome://browser/skin/chevron.svg should have been shown. - 
[task 2017-08-09T17:11:50.497645Z] 17:11:50     INFO - Stack trace:
[task 2017-08-09T17:11:50.502813Z] 17:11:50     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup_images.js:null:126

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8895278 [details]
Bug 1377184 - Consistently use the custom toolbar button styling in all browser toolbars.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166462/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/500a01cf896d
Consistently use the custom toolbar button styling in all browser toolbars. r=johannh

Comment 10

[Edo Karabas]

If this bug changed a ">>" button for Bookmarks toolbar to be consistent with other buttons and toolbars, then it was probably not a best idea. 
The button makes a Bookmarks toolbar much higher/bigger in all density options and then consequently the bar is shrinking and expanding depending on this button. 
https://s27.postimg.org/4q68ze8bn/bookmarks.png
So with the new button styling the whole Bookmarks bar height is way beyond photon specs. I think this needs more clarification, because obviously there is no room for both ways.

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm not seeing any differences in "Show more bookmarks" button sizes at all Densities (Compact, Normal and Touch) in Bookmark Toolbar. "Show more bookmarks" button size is always 14 x 24 px, at least in Mozilla Firefox Nightly 57.0a1 (2017-08-10).
Maybe you are thinking about other ">>" button, which is "More tools" button in URL/Address/Location Bar.

Comment 12

[Edo Karabas]

Yes, because patch is not in Nightly 57.0a1 (2017-08-10) yet.
I tried a build from autoland branch.
Well, the thing is that the ">>" "More tools" button in URL bar and ">>" "Show more bookmarks" button in Bookmarks bar are now exactly the same and it causes issues with Bookmarks bar height. 
I see only two options... leave it as it was before for Bookmarks bar or implement different "photon" button for this bar.

Comment 13

[Edo Karabas]

It is not even that easy since on the Bookmarks toolbar you can also put icons from Customization panel.
In the latest Nightly, the buttons are shrinking to the size of Bookmarks bar. And here, the Bookmarks bar is expanding to the size of buttons.
So, in this case a different "photon" button will not help. I think Bookmarks bar just needs to be locked to the intended size, so every added button will shrink accordingly as it was before this patch, but I'm not sure if it is not against the idea of this bug.

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

All buttons/items/etc. should use the same new Photon hover effect, which is this bug about, not buttons sizes.
But, if there will be some regression, like the size of "Show more bookmarks" will get massively huge compared to before, that should be fixed too. So your second option will probably be the best choice for all. I will monitor this upcoming issue, please do the same. :)

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/500a01cf896d

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Thank you very much \o/.

I'm confirming that bug is fixed, starting in Mozilla Firefox 57.0a1 (2017-08-11), so I'm marking this bug as VERIFIED.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

when this landed we see a performance improvement in the tab animation test:
== Change summary for alert #8694 (as of August 09 2017 18:52 UTC) ==

Improvements:

  3%  tart summary windows7-32 opt e10s     6.96 -> 6.78

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8694