1470870 - Load "menu.css" as a document stylesheet

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P3)

Description

[:Paolo Amadini]

This is part of the work tracked in bug 1470830.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1470870 - Load "menu.css" as a document stylesheet.

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

Comment 2

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8babb1867c8e4386ff162160f31c85225d8e5961

Comment 3

[Brian Grinstead [:bgrins]]

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 4

[:Paolo Amadini]

Comment on attachment 8987481 [details]
Bug 1470870 - Load "menu.css" as a document stylesheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252728/diff/1-2/

Comment 5

[:Paolo Amadini]

Attachment: Computed value shown in developer tools

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?

Comment 6

[:Paolo Amadini]

Note that increasing the specificity of the later rule does make "-moz-appearance" apply.

Comment 7

[:Paolo Amadini]

Also, the order is applied correctly for "padding-top" with the same selectors in the same files.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Which element do I need to inspect to see that Paolo?

Comment 9

[:Paolo Amadini]

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.

Comment 10

[:Paolo Amadini]

I've split "menulist" to bug 1471544.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 12

[:Paolo Amadini]

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.

Comment 13

[:Paolo Amadini]

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

[Brian Grinstead [:bgrins]]

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?

Comment 15

[:Paolo Amadini]

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

Comment 16

[:Paolo Amadini]

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.

Comment 17

[:Paolo Amadini]

Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9e2b660af875ea09f62785c43d33b23ca9788e
Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=565da79f2e72c3c94b87891812f2cdfeaebaf2d4

Comment 18

[:Paolo Amadini]

Comment on attachment 8987481 [details]
Bug 1470870 - Load "menu.css" as a document stylesheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252728/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/953772c799a6
Load "menu.css" as a document stylesheet. r=bgrins

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/953772c799a6

Comment 21

[Dão Gottwald [:dao]]

Backed out for causing bug 1474631:

https://hg.mozilla.org/integration/mozilla-inbound/rev/705f8f2b57e7f9b029e485b98ac4f823f9aee3e8

Comment 22

[Brian Grinstead [:bgrins]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb4179d5adc
Load "menu.css" as a document stylesheet. r=bgrins

Comment 24

[:Paolo Amadini]

That was it.

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/aeb4179d5adc