1446800 - Bookmarks menu items have small padding between icon and text

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[joshas]

Attachment: bookmarks-icon-padding.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180315233128

Steps to reproduce:

Open bookmarks menu by clicking "Show your bookmarks" icon.


Actual results:

Bookmark items and folders icons have smaller padding than in sidebar.


Expected results:

Bookmark icons in bookmark menu should have bigger padding, same as in sidebar.

Comment 1

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

I have tested this issue on Windows 10 x64 with the latest Firefox release (59.0.1) and the latest Nightly (61.0a1-20180321040527) and managed to reproduce it.

When checking the bookmark items and folders icons in sidebar and the ones from the bookmarks menu it seems that ones from the sidebar are smaller.

This seems to be more of an enhancement than an issue in my opinion. 
However, I am assigning a component to this issue in order to involve the development team and get an opinion on this.

Comment 2

[Marco Bonardo [:mak]]

they are different kind of views, they don't necessarily need to appear identical or with the same padding.

(In reply to Emil Pasca [:emilpasca], Desktop Engineering QA from comment #1)
> When checking the bookmark items and folders icons in sidebar and the ones
> from the bookmarks menu it seems that ones from the sidebar are smaller.

I put them side by side in a screenshot, and they are identical for me.
If you have further evidence please post it.

Comment 3

[joshas]

Attachment: Bookmarks menu Firefox 58 vs 59 overlay

Comment 4

[joshas]

Attachment: Bookmarks menu Firefox 58 vs 59 overlay 2x version

Comment 5

[joshas]

I've attached overlayed Bookmark Menu versions from Firefox 58 and Firefox 59.0.2 . In Firefox 59 text moved towards icons by 3px. I've also found commit, which broke this layout: https://hg.mozilla.org/mozilla-central/diff/5c9a0be10b65/browser/themes/shared/customizableui/panelUI.inc.css#l1.295
.menu-iconic-left margin was changed from 3px to 0.

Probably the best way to fix this is to update Browser Menu component to new one (like one used in Library) to keep consistency.

Comment 6

[Mark Banner (:standard8)]

Thank you for finding the regression point. Seems this is more theme related so moving it there.

Also needinfo to Paolo as he made that change.

Comment 7

[:Paolo Amadini]

Thanks for reporting and finding the regression point! It makes sense to add a custom style for this menu, which as you noted is a special case for the moment, and at some point we hope to replace with something consistent with the other menus.

Comment 8

[:Paolo Amadini]

Attachment: Bug 1446800 - Bookmarks menu items have small padding between icon and text.

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

Comment 9

[:Paolo Amadini]

Note that this makes the footer slightly misaligned on Windows, and this is difficult to fix due to bug 1457907.

We may want to wait to land this bug until we can fix that issue, or we can land the new margin anyways considering that this is not part of the default user interface and the issue is minor.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8971974 [details]
Bug 1446800 - Bookmarks menu items have small padding between icon and text.

https://reviewboard.mozilla.org/r/240724/#review246736

Unfortunately I don't have a Windows build to hand to see what the footer looks like, and Windows builds are busted on treeherder. Mac looks fine though. I'd have thought the footer would look too bad though, so yes, we could perhaps land this and leave that until later. Maybe pass it by a UX person if you're not fully sure?

Comment 11

[:Paolo Amadini]

I may just land this next cycle. There's progress being made in bug 1457907, so we'll probably figure out the best way to override the required rules then.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/344ff948dc28
Bookmarks menu items have small padding between icon and text. r=Standard8

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/344ff948dc28

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Paolo Amadini from comment #11)
> I may just land this next cycle. There's progress being made in bug 1457907,
> so we'll probably figure out the best way to override the required rules
> then.

Do you want to consider this wontfix for 61 then?

Comment 15

[:Paolo Amadini]

Yep. The padding has already been smaller for a few releases now, and it would be nice to align the footer at the same time once bug 1457907 is fixed.