Closed Bug 1429857 Opened 2 years ago Closed 2 years ago

Bookmarks list appears double-spaced

Categories

(Firefox :: Theme, defect)

x86_64
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Keywords: regression)

Attachments

(1 file)

+++ 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
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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.
Component: Bookmarks & History → Theme
We could also prefix these with `#BMB_bookmarksPopup` if we want to target the fix to that single piece of UI
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
Attachment #8942321 - Flags: review?(dao+bmo) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/05bab8e59cd1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [bugday-20180214]
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]
You need to log in before you can comment on or make changes to this bug.