1389966 - Bookmarks toolbar is bigger when added a new icon on it

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Edo Karabas]

Attachment: Bookmarks toolbar size.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170813100233

Steps to reproduce:

1. Open the latest Nightly and enable Bookmarks toolbar
2. See height of the toolbar
3. Open Menu - Customization - add any Icon on the Bookmarks toolbar



Actual results:

See that toolbar is higher


Expected results:

Expected results are that Bookmarks toolbar height is not depending on the presence of any icon, manually added or not. 
Size should be consistent and depends only on the UI density option.
Regression observed after Bug 1377184 landed. 

For a comparison see attachment where I've checked all density options.

Behavior before Bug 1377184 was that all buttons on the Bookmarks toolbar have automatically shrinked to the size of the toolbar. 

I think this breaks UX consistency.

Comment 7

[Edo Karabas]

I know that you probably still have more urgent issues to solve, but is there any possibility to make this bug fixed for 57 ? Maybe it's not an issue visible on the first look, but still quite a big one. Duplicates are coming and I can imagine that with beta or even release audience will even more. 
My main concern here is, that with this bug the UI density options, especially the compact one, will be a bit of ruined since the compact UI will occupy the same screen space as the normal UI today.

Comment 9

[:Gijs (he/him)]

Given the frequency of reports, should we reconsider prioritizing this and/or bug 1388794 for 57? Maybe at least for the bookmarks overflow chevron case?

Comment 10

[Dão Gottwald [:dao]]

It's not a high priority for the time being because the bookmarks toolbar is disabled by default, and we usually don't switch between the two heights on the fly but only when the user has many bookmarks in the toolbar or adds another button.

Comment 11

[Edo Karabas]

Using pretty common Windows feature Snap causing higher Bookmarks toolbar as well, so I would say this will hit a lot if not all users pretty quickly. And since previous size will not come back until new session it will be definitely annoying.

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1389966 - Make toolbarbuttons in the bookmarks toolbar adjust to toolbar height.

Regular bookmark items can grow flexibly because they apply hover background
and padding on the parent toolbarbutton element instead of its contents,
like .toolbarbutton-1 does. This patch adds a few hacky overrides to fix that.

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

Comment 13

[:Gijs (he/him)]

Comment on attachment 8909515 [details]
Bug 1389966 - Make toolbarbuttons in the bookmarks toolbar adjust to toolbar height.

https://reviewboard.mozilla.org/r/180988/#review186390

The !important with descendant selectors is a bit sad, but I agree this makes sense, probably moreso than the alternative.

::: browser/themes/shared/toolbarbuttons.inc.css:196
(Diff revision 1)
>  toolbarbutton.bookmark-item[open="true"],
>  toolbar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-icon,
>  toolbar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-text,
>  toolbar .toolbarbutton-1:not([disabled=true]):-moz-any([open],[checked],:hover:active) > .toolbarbutton-badge-stack {
>    background-color: var(--toolbarbutton-active-background);
>    transition-duration: var(--toolbarbutton-active-transition-duration);

It looks like I didn't notice we needed to remove this in bug 1393057. Can you remove it and the variable for it, please?

::: browser/themes/shared/toolbarbuttons.inc.css:273
(Diff revision 1)
> +/* To allow toolbarbuttons in the bookmarks toolbar to grow in
> + * height with the toolbar (like bookmark items), we apply background
> + * and padding to the buttons, not the button contents. This rule
> + * overrides attributes that would otherwise be duplicated. */

Nit: given that the rest of the styling is for 'normal' bookmark buttons, and that's what the header of this section says, can you move these 2 rules to the bottom?

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8909515 [details]
Bug 1389966 - Make toolbarbuttons in the bookmarks toolbar adjust to toolbar height.

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

Comment 15

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #13)
> Comment on attachment 8909515 [details]
> Bug 1389966 - Make toolbarbuttons in the bookmarks toolbar adjust to toolbar
> height.
> 
> https://reviewboard.mozilla.org/r/180988/#review186390
> 
> The !important with descendant selectors is a bit sad, but I agree this
> makes sense, probably moreso than the alternative.

Yeah, I'm not terribly excited about this patch either, but it solves the problem fine, AFAICT.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa4c0d777076
Make toolbarbuttons in the bookmarks toolbar adjust to toolbar height. r=Gijs

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/fa4c0d777076

Comment 18

[Deac Alin]

Tested on Mac OSX 10.12.5 Latest Nightly 57.0a1 Build ID 20170920100426
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0 

The Bookmarks toolbar height is consistent and not depending of addition of new icons(24px). I will mark this as Verified Fixed. Thank you