Closed
Bug 1470870
Opened 6 years ago
Closed 6 years ago
Load "menu.css" as a document stylesheet
Categories
(Toolkit :: Themes, defect, P3)
Toolkit
Themes
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: Paolo, Assigned: Paolo)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
This is part of the work tracked in bug 1470830.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8babb1867c8e4386ff162160f31c85225d8e5961
Comment 3•6 years ago
|
||
I expect this will resurface Bug 1429857 (originally filed as Bug 1420229), which is what prompted us to do the move to UA sheets in the first place. Can you double check that it reproduces still and then look to see if there's a more permanent fix than what we landed before migrating to UA and ultimately reverted once it wasn't needed anymore: https://hg.mozilla.org/mozilla-central/rev/05bab8e59cd1?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
More platform weirdness. This patch is on top of the one that loads the "widgets.css" stylesheet through "global.css", without modifying the style cache, and I decreased the specificity of one rule in "menu.css" to fix the Windows issue with the menu of the non-default Bookmarks button in the toolbar. While this works for some rules, the one in the screenshot has a strange issue. I'm seeing an inconsistent state in the computed rules view, and the later rule is in fact not applied. What is happening here?
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(emilio)
Assignee | ||
Comment 6•6 years ago
|
||
Note that increasing the specificity of the later rule does make "-moz-appearance" apply.
Assignee | ||
Comment 7•6 years ago
|
||
Also, the order is applied correctly for "padding-top" with the same selectors in the same files.
Comment 8•6 years ago
|
||
Which element do I need to inspect to see that Paolo?
Flags: needinfo?(emilio) → needinfo?(paolo.mozmail)
Assignee | ||
Comment 9•6 years ago
|
||
Ah, sorry for not mentioning that earlier! On a Windows build, you need to add the Bookmarks button to the toolbar, for example using "Library > Bookmarks > Bookmarking Tools > Add Bookmarks Menu to Toolbar", then in the menu you can look for one of the "menuitem" elements and inspect the ".menu-iconic-left" box, which contains the icon image but is not the icon image itself.
Flags: needinfo?(paolo.mozmail) → needinfo?(emilio)
Assignee | ||
Comment 10•6 years ago
|
||
I've split "menulist" to bug 1471544.
Summary: Load "menu.css" and "menulist.css" as document stylesheets → Load "menu.css" as a document stylesheet
Comment 11•6 years ago
|
||
This is working as expected, the sheets in that page are: Array.from(document.styleSheets).map(s => s.href).join(", ") > "chrome://browser/content/browser.css, chrome://browser/content/downloads/downloads.css, chrome://browser/content/places/places.css, chrome://browser/content/usercontext/usercontext.css, chrome://browser/skin/controlcenter/panel.css, chrome://browser/skin/customizableui/panelUI.css, chrome://browser/skin/downloads/downloads.css, chrome://browser/skin/searchbar.css, chrome://browser/skin/places/places.css, chrome://browser/skin/places/editBookmark.css, chrome://browser/skin/browser.css, chrome://browser/content/tabbrowser.css, chrome://browser/skin/compacttheme.css" menu.css is imported via widgets.css, which is imported via global.css, which is imported via chrome://browser/skin/browser.css, that is, comes after panelUI.css, so given they have the same specificity, the rule in menu.css wins, and thus appearance computes to menuimage. Also, getComputedStyle(temp0).paddingTop is 2px in my build (though it's a Linux build with the Windows rule copied over), fwiw. So I'm not sure what the computed view is doing, but it's not doing it right. You can see the ordered rules that match an element with InspectorUtils.getCSSStyleRules(element). There, for that element, the rules are (one per line): InspectorUtils.getCSSStyleRules(temp0).map(r => r.cssText).join("\n") > * { -moz-user-focus: ignore; -moz-user-select: none; display: -moz-box; box-sizing: border-box; } .menu-text, .menu-iconic-left, .menu-iconic-right, .menu-iconic-text { margin-top: 0px !important; margin-bottom: 0px !important; margin-inline-start: 0px !important; margin-inline-end: 2px !important; color: inherit; } .menu-iconic-left, .menu-iconic-right { width: 16px; padding-inline-end: 3px !important; } .subviewbutton > .menu-right, .subviewbutton > .menu-accel-container > .menu-iconic-accel, .subviewbutton > .menu-iconic-left, .subviewbutton > .menu-iconic-text { padding-bottom: 0px; padding-top: 0px; } .subviewbutton > .menu-iconic-left { -moz-appearance: none; margin-inline-end: 0px; } .menu-iconic > .menu-iconic-left, .menuitem-iconic > .menu-iconic-left { -moz-appearance: menuimage; padding-top: 2px; } #BMB_bookmarksPopup .subviewbutton > .menu-iconic-left { margin-inline-end: 3px; } Which makes sense, looking at the sheets they come from :)
Flags: needinfo?(emilio)
Assignee | ||
Comment 12•6 years ago
|
||
Thanks Emilio for the help with debugging! I'll file a developer tools bug. What made this even more confusing is that "getComputedStyle(temp0).paddingTop" is zero for me because of some effect of "-moz-appearance: menuimage", apparently on Windows only.
Assignee | ||
Comment 13•6 years ago
|
||
Rebasing this on top of bug 1421433 fixed the Bookmarks Menu issue. Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bed56a7ebc0402a7b04f0b6d0112b3bd224c15 Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cebbea2c3fca709ee7c04eba17329f50610a317
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8987481 [details] Bug 1470870 - Load "menu.css" as a document stylesheet. https://reviewboard.mozilla.org/r/252728/#review261318 ::: toolkit/themes/windows/global/menu.css:93 (Diff revision 2) > .menu-iconic-icon { > width: 16px; > height: 16px; > } > > -menu.menu-iconic > .menu-iconic-left, > +.menu-iconic > .menu-iconic-left, Is this change still needed after bug 1421433?
Attachment #8987481 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #14) > > -menu.menu-iconic > .menu-iconic-left, > > +.menu-iconic > .menu-iconic-left, > > Is this change still needed after bug 1421433? Yes, and it now behaves as expected, being overridden by the browser rules.
Assignee | ||
Comment 16•6 years ago
|
||
I looked into the screenshot differences in the Preferences window on Linux, and fixed other cases where the "xul|groupbox xul|label:not(...)" rule was applied to too many inner labels. Still, this only applied in part because it was overriden by the following "!important" rule in the UA stylesheet: .menu-iconic-accel { margin-inline-start: 7px !important; } I'll run new screenshots with the fix, and I expect them to show small differences where we are restoring the behavior we had when these were XBL stylesheets.
Assignee | ||
Comment 17•6 years ago
|
||
Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9e2b660af875ea09f62785c43d33b23ca9788e Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=565da79f2e72c3c94b87891812f2cdfeaebaf2d4
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/953772c799a6 Load "menu.css" as a document stylesheet. r=bgrins
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/953772c799a6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 21•6 years ago
|
||
Backed out for causing bug 1474631: https://hg.mozilla.org/integration/mozilla-inbound/rev/705f8f2b57e7f9b029e485b98ac4f823f9aee3e8
Status: RESOLVED → REOPENED
status-firefox62:
affected → ---
status-firefox63:
fixed → ---
Resolution: FIXED → ---
Comment 22•6 years ago
|
||
I think Bug 1474631 was likely caused by global.css not being loaded somewhere, like https://searchfox.org/mozilla-central/source/browser/base/content/webext-panels.xul. We saw asimilar issue with about:config on first landing (bug 1420166).
Comment 23•6 years ago
|
||
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb4179d5adc Load "menu.css" as a document stylesheet. r=bgrins
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aeb4179d5adc
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•