Closed Bug 1389966 Opened 3 years ago Closed 3 years ago

Bookmarks toolbar is bigger when added a new icon on it

Categories

(Firefox :: Theme, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- disabled
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: Eddwardiq, Assigned: johannh)

References

(Depends on 1 open bug, Regressed 1 open bug)

Details

(Keywords: regression, ux-consistency, Whiteboard: [reserve-photon-visual] )

Attachments

(2 files)

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.
Blocks: 1377184
Has STR: --- → yes
Component: Untriaged → Theme
Whiteboard: [photon-visual][triage]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 1390851
Flags: qe-verify+
Priority: -- → P4
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
QA Contact: brindusa.tot
Whiteboard: [reserve-photon-visual] → [reserve-photon-visual]
Duplicate of this bug: 1386593
Depends on: 1392931
See Also: 1396325
Duplicate of this bug: 1396325
See Also: → 1395596
Duplicate of this bug: 1396679
Duplicate of this bug: 1397641
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.
Duplicate of this bug: 1392931
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?
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P4 → P1
Iteration: --- → 57.3 - Sep 19
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?
Attachment #8909515 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa4c0d777076
Make toolbarbuttons in the bookmarks toolbar adjust to toolbar height. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/fa4c0d777076
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1401537
Depends on: 1401823
Regressions: 1559909
You need to log in before you can comment on or make changes to this bug.