joshas Reporter
	Description • 6 years ago

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.

	Emil Pasca [:emilpasca]
	Comment 1 • 6 years ago

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.

	Marco Bonardo [:mak]
	Comment 2 • 6 years ago

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.

	joshas Reporter
	Comment 5 • 6 years ago

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.

	Mark Banner (:standard8)
	Comment 6 • 6 years ago

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.

	:Paolo Amadini Assignee
	Comment 7 • 6 years ago

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.

	:Paolo Amadini Assignee
	Comment 9 • 6 years ago

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.

	Mark Banner (:standard8)
	Comment 10 • 6 years ago
mozreview-review

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?

	:Paolo Amadini Assignee
	Comment 11 • 6 years ago

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.

	Pulsebot
	Comment 12 • 6 years ago

Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/344ff948dc28
Bookmarks menu items have small padding between icon and text. r=Standard8

	Raul Gurzau (:RaulG)
	Comment 13 • 6 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago

(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?

	:Paolo Amadini Assignee
	Comment 15 • 6 years ago

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.