Brian Grinstead [:bgrins] Assignee
	Description • 7 years ago

+++ This bug was initially created as a clone of Bug #1420229 +++

STR:
Open Customize and drag the Bookmark Icon to the NavBar

Open the bookmarks list and notice that its double-spaced and stretched

20171122220056 960f50c2e0a991ab2ab313132e69fb2c96cb7866 Good
20171123100420 b6bed1b710c3e22cab49f22f1b5f44d80286bcb9 Bad

Mozregress points to:
Bug 1416493

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 7 years ago

This is a clone as per https://bugzilla.mozilla.org/show_bug.cgi?id=1420229#c52, to handle the visual regression for Firefox 59. Screenshots of the issue can be seen in Bug 1420229.

	Brian Grinstead [:bgrins] Assignee
	Comment 3 • 7 years ago

Dão, this fixes the UI regression.  I don't love the patch, but I notice there are other places already in this file where we are specifically overriding menu.css so it doesn't feel too out of place. In those cases we appear to be using !important so I'd be happy to switch to that if you'd prefer (instead of adding .subviewbutton onto each selector).

After debugging this I feel more confident about the plan to deprioritize these XBL sheets as UA sheets instead of loading them as doc styles and reworking specificity during the process of replacing bindings - it would be a ton of work for each one to do this properly.

	Brian Grinstead [:bgrins] Assignee
	Comment 4 • 7 years ago

We could also prefix these with `#BMB_bookmarksPopup` if we want to target the fix to that single piece of UI

	Dão Gottwald [:dao]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8942321 [details]
Bug 1429857 - Override -moz-appearance on bookmarks popup subviewbuttons, and styles on menuitem-iconic-left;

https://reviewboard.mozilla.org/r/212588/#review218998

I haven't tested this, but rs+ since this is supposed to be a temporary hack.

::: browser/themes/shared/customizableui/panelUI.inc.css:1267
(Diff revision 1)
> +  padding-top: 0;
> +  -moz-appearance: none;
> +}
> +
> +menuitem.subviewbutton,
> +menuitem[type="checkbox"].subviewbutton,

This is probably already covered by menuitem.subviewbutton...

::: browser/themes/shared/customizableui/panelUI.inc.css:1269
(Diff revision 1)
> +}
> +
> +menuitem.subviewbutton,
> +menuitem[type="checkbox"].subviewbutton,
> +menuitem[type="checkbox"].subviewbutton > .menu-iconic-left,
> +menuitem.subviewbutton > .menu-iconic-left > .menu-iconic-icon {

.menu-iconic-icon shouldn't have -moz-appearance in the first place

	Brian Grinstead [:bgrins] Assignee
	Comment 7 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #5)
> Comment on attachment 8942321 [details]
> Bug 1429857 - Override -moz-appearance on menuitem and descendants, and
> padding on menuitem-iconic-left;
> 
> https://reviewboard.mozilla.org/r/212588/#review218998
> 
> I haven't tested this, but rs+ since this is supposed to be a temporary hack.
> 
> ::: browser/themes/shared/customizableui/panelUI.inc.css:1267
> (Diff revision 1)
> > +  padding-top: 0;
> > +  -moz-appearance: none;
> > +}
> > +
> > +menuitem.subviewbutton,
> > +menuitem[type="checkbox"].subviewbutton,
> 
> This is probably already covered by menuitem.subviewbutton...

I believe it was needed because some rule in menu.css for type=checkbox had more specificity than `menuitem.subviewbutton`. I went back to revisit this and pushed up a change that is simpler and targets the bookmarks popup specifically. The only selector needed in this rule is now `#BMB_bookmarksPopup .subviewbutton`, which covers all cases.

> ::: browser/themes/shared/customizableui/panelUI.inc.css:1269
> (Diff revision 1)
> > +}
> > +
> > +menuitem.subviewbutton,
> > +menuitem[type="checkbox"].subviewbutton,
> > +menuitem[type="checkbox"].subviewbutton > .menu-iconic-left,
> > +menuitem.subviewbutton > .menu-iconic-left > .menu-iconic-icon {
> 
> .menu-iconic-icon shouldn't have -moz-appearance in the first place

You are right - removed this from the list.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05bab8e59cd1
Override -moz-appearance on bookmarks popup subviewbuttons, and styles on menuitem-iconic-left;r=dao

	Dorel Luca [:dluca]
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/05bab8e59cd1

	Saheda Reza [:Antora]
	Comment 10 • 7 years ago

I have reproduced this bug on nightly according to (2018-01-11)

Fixing bug is verified on Latest Beta--
Build ID    :20180208193705
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0

Tested OS-- Windows7 32bit

	Mohammed Adam
	Comment 11 • 7 years ago

I Have tried to Reproduce the Same Bug in Firefox Nightly  60.0a1 (2018-02-13)

Tested OS: Windows 10 (64bit) & Windows 7 (32bit)

------------------------------------------------------------------------------------------
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180212100154
Firefox Version: 60.0a1
------------------------------------------------------------------------------------------
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180211220056
Firefox Version: 60.0a1
------------------------------------------------------------------------------------------

The bug is verified on Latest Beta too

[Steps to Reproduce] 

1) Open the Firefox with Cleaned Profile
2) Click on View -> Toolbars -> Customize 
3) Drag the Bookmark Icon to the NavBar
4) Open the Bookmarks list 

Actual Results:
The Bookmarks list is displaying fine. there is no sort of stretching or Double-spaced 

Status: Fixed & Verified 

[bugday-20180214]