1402075 - Bookmark toolbar buttons in normal density mode are the size of compact density

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[David Smith]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170921100141

Steps to reproduce:

A recent change in the bookmark toolbar design has rendered the bookmarks and folders on the toolbar much more difficult to use.

Sizes measured from screenshots.

FF 57:

At NORMAL density:

Active target area of bookmark/folder on bookmark toolbar: 19px vertical
Top clearance: 4px
Bottom clearance: 2px

At COMPACT density:

Active target area of bookmark/folder on bookmark toolbar: 19px vertical
Top clearance: 3px
Bottom clearance: 2px

At TOUCH density:

Active target area of bookmark/folder on bookmark toolbar: 25px vertical
Top clearance: 4px
Bottom clearance: 2px


FF 56:

At NORMAL density:

Active target area of bookmark/folder on bookmark toolbar: 24px vertical
Top clearance: 7px
Bottom clearance: 0px

At COMPACT density:

Active target area of bookmark/folder on bookmark toolbar: 24px vertical
Top clearance: 2px
Bottom clearance: 0px


Active target area of bookmark/folder on bookmark toolbar with bookmark menu in toolbar: 26px vertical


In all cases, normal bookmark target heights are 22px.


A recently fixed bug made it so that buttons on the toolbar do not cause the toolbar size to change.  With that, I would then expect the target area height to be 24px with or without the bookmark menu.

Instead, the target area has been reduced to 19 px, a 20%-25% reduction in target area vertical size, and equal to the compact density level.

Meanwhile, normal bookmarks within the menus have a vertical target size of 22px, so the toolbar bookmarks are shorter even than normal bookmarks.

Touch density makes the bookmark toolbar item heights usable, but also modifies the rest of the theme in undesired ways.





Actual results:

Bookmark toolbar items have had their height reduced to compact levels.


Expected results:

Bookmark toolbar item target heights at normal density should be sized to a usable level, such as that matching normal bookmarks (22px) or the old default (24px).

Comment 1

[YF (Yang)]

Dupe of bug 1401523?

Comment 2

[David Smith]

I do not believe so.  Bug 1401523 is about padding around the bookmark items, as seen in the discussion about 2px/4px vs 1px/4px (the noted bottom and top clearance in my description above).  This bug is about the actual targetable height of the bookmark itself.

Comment 3

[Marco Bonardo [:mak]]

The change was done by design, to change it you need UX involvement since then the design should change.

I also think these buttons need a bit more padding, in my opinion even just changing the current padding from 1px 4px to 2px 4px would be a great change. But what is currently in the product is acordingly to the Photon mockups.

Hopefully Stephen can re-evaluate.

Comment 4

[Stephen Horlander [:shorlander]]

(In reply to Marco Bonardo [::mak] from comment #3)
> The change was done by design, to change it you need UX involvement since
> then the design should change.
> 
> I also think these buttons need a bit more padding, in my opinion even just
> changing the current padding from 1px 4px to 2px 4px would be a great
> change. But what is currently in the product is acordingly to the Photon
> mockups.

Yeah I agree it's a little on the short side at the moment. Changing it from 1px 4px to 2px 4px would be a good change.

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 1402075 - Increase vertical bookmark-item padding.

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

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8922814 [details]
Bug 1402075 - Increase vertical bookmark-item padding.

https://reviewboard.mozilla.org/r/193968/#review199600

This looks good, cheers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1abbf079b653
Increase vertical bookmark-item padding. r=daleharvey

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1abbf079b653

Comment 9

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=3f627210c55d6b2b0a0635683cc4a29102a90270&newProject=mozilla-central&newRev=ae1d655ea7c7101543ac56e88414de4527bd7540

Comment 10

[Valentina Claudia Ona]

I verified this issue using latest Nightly 58.0a1 with Build ID 20171106100122 on Windows 10 x64 and Windows 7 x64.
I will mark this as verified fixed.