1416493 - Remove "menuseparator" binding and move accessibility into XUL based on tag name instead of XBL role

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

I realized that the "menuseparator" binding doesn't do anything besides assign a role: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/menu.xml#286. Even though it does extend other bindings it doesn't seem to use any of those properties / fields / etc as none of the mochitests are failing when the binding is not applied [0]. 

Here's the inheritance:

basecontrol
    basetext
        control-item 
            menuitem-base
                menuseparator

It looks like there may be an opportunity here to move the accessibility code to key off tag name similar to what was done for image (Bug 1403231) and the delete the binding.

One potential downside is that a menuseparator alone wouldn't trigger loading of chrome://global/skin/menu.css via #menuitem-base, but I don't think we have any cases where we have a menuseparator without a menuitem (which would trigger loading the same sheet). We might want to separately consider just loading menu.css in all XUL documents anyway instead of relying on XBL for it.

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=945ae4672c12842e980598863fcc3ef159d2b5cd&selectedJob=144007745

Comment 1

[Brian Grinstead [:bgrins]]

Paolo, given Comment 0 what do you think about this approach?

Comment 2

[:Paolo Amadini]

It would be interesting to understand what the accessibility code does with nsIDOMXULSelectControlItemElement and nsIDOMXULContainerItemElement. The fact that tests aren't failing may just indicate there is missing coverage, like in the "image" case.

If the interfaces are irrelevant for separators and don't affect things like menu navigation, then removing the binding and assigning the role in the C++ code sounds good to me. It's also very easy to verify that the CSS is still applied even if the binding does not import the stylesheet explicitly.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #2)
> It would be interesting to understand what the accessibility code does with
> nsIDOMXULSelectControlItemElement and nsIDOMXULContainerItemElement. The
> fact that tests aren't failing may just indicate there is missing coverage,
> like in the "image" case.

To be clear, the accessibility tests *are* failing, but the b-c tests are passing.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1416493 - Remove the menuseparator XBL binding;

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

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

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

I'd like some feedback on this approach regarding how we should load stylesheets without XBL. I can think of 3 main approaches we could take with this and other similar stylesheets (toolbarseparator and toolbar.css, for example).

1) Move the stylesheet loading out of XBL <resources> and instead loading it directly from global.css
- Pros:
-- It works now (this patch is demonstrating it)
- Cons:
-- If a document doesn't load "chrome://global/skin" the styles won't be loaded
-- If a document doesn't use any of the bindings the styles still get loaded
-- Styles 'leak out' of the binding. So sheets that use very generic selectors could end up accidentally applying to existing content.
-- I was originally suspecting that it would change the application order of stylesheets - but with or without the patch, browser content and skin files appear to be applied *after* menu.css.
2) Use Custom Elements and insert a scoped stylesheet as non-anonymous content within the element
- Pros: 
-- Only loads the stylesheet once an element is inserted
- Cons:
-- Unsure if we have platform support for this
-- Potentially slow (lots of mutations, may not work with existing XUL caching)
3) Use Shadow DOM and load <link rel="stylesheet" />
- Pros:
-- Only loads the stylesheet once an element is inserted
-- This should eventually work as per Bug 1410578 and be fast as per Bug 1410579
- Cons:
-- We need Shadow DOM support first
-- For some elements that don't need any behavior attached (i.e. menuseparator/toolbarseparator) we would still need to create a CE with Shadow DOM loading the sheet just to load the resource.

IMO the ideal here is (3) and we shouldn't do (2) at all - I'm not even sure if it would work and it would just be a stopgap.

So I guess the main questions are:
- Am I missing an alternate approach?
- Is it worth taking approach (1) while we await platform support for (3)? This would let us make forward progress on binding removal, but if it will cause maintenance headaches we'd have to take that into consideration.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

LGTM. Super-nitty nit: I believe convention is chrome://global/skin/, even if it works without the trailing slash.

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8927607 [details]
> Bug 1416493 - Remove the menuseparator XBL binding;
> 
> I'd like some feedback on this approach regarding how we should load
> stylesheets without XBL. I can think of 3 main approaches we could take with
> this and other similar stylesheets (toolbarseparator and toolbar.css, for
> example).
> 
> 1) Move the stylesheet loading out of XBL <resources> and instead loading it
> directly from global.css
> - Pros:
> -- It works now (this patch is demonstrating it)
> - Cons:
> -- If a document doesn't load "chrome://global/skin" the styles won't be
> loaded
> -- If a document doesn't use any of the bindings the styles still get loaded
> -- Styles 'leak out' of the binding. So sheets that use very generic
> selectors could end up accidentally applying to existing content.
> -- I was originally suspecting that it would change the application order of
> stylesheets - but with or without the patch, browser content and skin files
> appear to be applied *after* menu.css.
> 2) Use Custom Elements and insert a scoped stylesheet as non-anonymous
> content within the element
> - Pros: 
> -- Only loads the stylesheet once an element is inserted
> - Cons:
> -- Unsure if we have platform support for this
> -- Potentially slow (lots of mutations, may not work with existing XUL
> caching)
> 3) Use Shadow DOM and load <link rel="stylesheet" />
> - Pros:
> -- Only loads the stylesheet once an element is inserted
> -- This should eventually work as per Bug 1410578 and be fast as per Bug
> 1410579
> - Cons:
> -- We need Shadow DOM support first
> -- For some elements that don't need any behavior attached (i.e.
> menuseparator/toolbarseparator) we would still need to create a CE with
> Shadow DOM loading the sheet just to load the resource.
> 
> IMO the ideal here is (3) and we shouldn't do (2) at all - I'm not even sure
> if it would work and it would just be a stopgap.
> 
> So I guess the main questions are:
> - Am I missing an alternate approach?
> - Is it worth taking approach (1) while we await platform support for (3)?
> This would let us make forward progress on binding removal, but if it will
> cause maintenance headaches we'd have to take that into consideration.

I think we should take approach 1. In fact, for these mini-bindings I don't think we want (3) at all because of the CE it requires. I don't know what shadow DOM and CE perf is gonna be like, but given Chrome and Servo both bailed out of doing scoped stylesheets because (in part) perf, I don't have super-high hopes, and it seems silly to use it for something as tiny as this.

I think in general, with the reduction in reuse of the XUL platform (TB is kinda-sorta not really supported, no more XUL themes), we should aim towards simplification of our CSS resources. We have oodles of files, most of them (ie toolkit global stuff) could be consolidated, and some of the other ones are way too heavy (browser.css * 4, panelUI.css and friends, I'm looking at you).

IMHO we should switch to a web-y approach with smaller files that we concat together for builds (maybe minifying/reducing whitespace?), and use source maps for local/iterative development. We can do lazy-loading where appropriate, but in this case I don't think it would really get us much. I guess I'm skeptical of the potential of perf wins gained through having fewer CSS rules or less parsing or something. I have no data to back up that skepticism, so I guess if there is data that proves me wrong I would need to revise my opinions. (I don't currently have time to gather data to support my opinion...) :-)

If I were wrong (very possible!) then we could try to postpone loading menu.css until a/any menu actually got opened. That would need other mechanics anyway, though - we'd need some way to avoid FOUC and things like that...

Comment 8

[:Gijs (he/him)]

Brief addendum that I do not see a near future where there are no menuitems/menuseparators in browser.xul, so delaying the load until such elements get constructed seems like it would net us approx. nothing in practice anyway.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #8)
> Brief addendum that I do not see a near future where there are no
> menuitems/menuseparators in browser.xul, so delaying the load until such
> elements get constructed seems like it would net us approx. nothing in
> practice anyway.

Yeah, I think the same is true for a lot of our toolkit widgets. I will do some talos pushes to confirm but I don't think waiting to load them will be much of a help for performance.

Comment 10

[Brian Grinstead [:bgrins]]

Talos push shows a couple of improvements and one regression, but they don't really seem like tests that would be affected by this change, so maybe it's just noise: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ffe0074f8312909fd63e8af5f6ea4b1f45a873e8&newProject=try&newRevision=c8a2ea3cfa6e895d9ae5e4dfae196d6fd59c8158&framework=1&showOnlyImportant=1

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a2ea3cfa6e895d9ae5e4dfae196d6fd59c8158

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

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

Comment 12

[:Gijs (he/him)]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

https://reviewboard.mozilla.org/r/198900/#review207534

Comment 13

[Brian Grinstead [:bgrins]]

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=782a6b4a0579408478581167d10c5fe77df8d844&newProject=try&newRev=73a84230b93ec350c4509ec46850ec7d6b504cb7

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8927607 [details]
Bug 1416493 - Remove the menuseparator XBL binding;

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f57907f645b
Remove the menuseparator XBL binding;r=Gijs

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5f57907f645b

Comment 17

[Dão Gottwald [:dao]]

This patch looks rather wrong to me. global.css shouldn't include menu.css. Instead, only the relevant styles should move to global.css. I'd like to back this out, also for causing bug 1420229.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #17)
> This patch looks rather wrong to me. global.css shouldn't include menu.css.

Let me explain why I think this is a bad idea:

1) Afaik these stylesheets get different priorities in the style system which affects the cascade.

2) Because widget stylesheets are scoped, they may contain generic selectors that match too much when applied globally.

Comment 19

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to Dão Gottwald [::dao] from comment #17)
> > This patch looks rather wrong to me. global.css shouldn't include menu.css.
> 
> Let me explain why I think this is a bad idea:
> 
> 1) Afaik these stylesheets get different priorities in the style system
> which affects the cascade.

This was addressed in comment 6:

> -- I was originally suspecting that it would change the application order of
> stylesheets - but with or without the patch, browser content and skin files
> appear to be applied *after* menu.css.

Do we have evidence that this is not true?

> 2) Because widget stylesheets are scoped, they may contain generic selectors
> that match too much when applied globally.

They may, but that seems unlikely. I don't see anything in menu.css that gives me concern. Do you have a concrete example?

Given the deprecation of <style scoped> and the desire to get rid of XBL, this (style rules that are overly generic and match nodes they "shouldn't") is going to be a continuing source of concern regardless of what we do with this patch. If there are problematic rules in menu.css (or other XBL-related files), I'd rather fix the rules than continuing to rely on some kind of separation existing. These stylesheets cause annoying problems anyway, they generally overuse !important (which then causes more use of !important in the browser styles) and get duplicated in the style editor, etc. If we can tidy them up and merge them into global.css (either directly or via %include / @import) then that'd be a good thing.

(In reply to Dão Gottwald [::dao] from comment #17)
> Instead, only the relevant styles should move to global.css.

We could do this if there is concrete evidence for this being the source of the problem. As it is, I haven't seen any - there's no analysis of the problem in bug 1420229, nor an explanation why only that menu is affected.

> I'd like to back this out, also for causing bug 1420229.

I do not think bug 1420229 is severe enough to warrant a backout. Some extra padding in a menu that's not even in the default toolbar setup, on nightly only, with merge day almost 2 months away, just doesn't warrant it. We should just fix that bug. If removing the menu.css imports from global.css and moving only menuseparator styles across to global.css actually fixes bug 1420229, maybe you could just put up a patch to do that in bug 1420229? I'll happily review - but Brian's in MTV so probably out for Thanksgiving.

Comment 20

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #19)
> > 1) Afaik these stylesheets get different priorities in the style system
> > which affects the cascade.
> 
> This was addressed in comment 6:
> 
> > -- I was originally suspecting that it would change the application order of
> > stylesheets - but with or without the patch, browser content and skin files
> > appear to be applied *after* menu.css.
> 
> Do we have evidence that this is not true?

Yeah, bug 1420229, at least that's my initial suspicion. I've seen global.css override other styles when you wouldn't expect this, forcing people to use !important. I think it takes precedence in some other fashion independent from the order.

> > 2) Because widget stylesheets are scoped, they may contain generic selectors
> > that match too much when applied globally.
> 
> They may, but that seems unlikely. I don't see anything in menu.css that
> gives me concern. Do you have a concrete example?

Why would that seem unlikely? Making selectors more specific is pointless when you know they only apply in a known sub tree. I'm pretty sure I've written styles with that in mind.

I don't know if menu.css is affected, it depends on whether any of these classes are used outside of the binding. Since Brian wants to use this as an example for other bindings to follow, it's a concern regardless.

> > Instead, only the relevant styles should move to global.css.
> 
> We could do this if there is concrete evidence for this being the source of
> the problem. As it is, I haven't seen any - there's no analysis of the
> problem in bug 1420229, nor an explanation why only that menu is affected.

That's a weird argument to make (unless you're questioning whether bug 1420229 was caused by this at all; I'd be happy to test that locally). The fact that we don't immediately understand bug 1420229 shouldn't boost anyone's confidence in these patches, it rather makes them more problematic.

> > I'd like to back this out, also for causing bug 1420229.
> 
> I do not think bug 1420229 is severe enough to warrant a backout. Some extra
> padding in a menu that's not even in the default toolbar setup, on nightly
> only, with merge day almost 2 months away, just doesn't warrant it.

Two months with the All Hands, extended holidays and likely some PTO. But that wasn't my concern anyway. Bug 1420229 just supports my other concerns. I think the approach taken here is problematic and will either need to be tweaked significantly if not completely replaced with something else. I see no point in leaving this in the tree as-is, and there's a risk that this will confuse others working with any of the affected elements.

Comment 21

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #20)
> > > Instead, only the relevant styles should move to global.css.
> > 
> > We could do this if there is concrete evidence for this being the source of
> > the problem. As it is, I haven't seen any - there's no analysis of the
> > problem in bug 1420229, nor an explanation why only that menu is affected.
> 
> That's a weird argument to make (unless you're questioning whether bug
> 1420229 was caused by this at all; I'd be happy to test that locally). The
> fact that we don't immediately understand bug 1420229 shouldn't boost
> anyone's confidence in these patches, it rather makes them more problematic.

I'm not convinced moving just the separator styles would fix the root cause here. It would presumably fix some of the padding issue because we wouldn't alter anything for in-non-separator-menuitem nodes (and their padding in the BMB popup is clearly not right on Windows right now), but it wouldn't fix any issues with the separator styles themselves unless we also update styles there (and/or elsewhere) as necessary.

Point being, that doesn't seem like a radically different approach, just delaying something we'd do eventually by only moving a subset of menu.css . If we're going to be following the same pattern (move affected styles to global.css when moving XUL nodes off XBL bindings), seems we might as well port the entire sheet and fix all the rules up so there's no fallout either way, given that the menuitem bindings will want to also move eventually.

But it kind of sounds like you think even that pattern is wrong? I'm a bit confused, because in part (e.g. comment 17) you seem to be saying we "should only move the relevant styles" and in part you seem to be saying "this approach is wrong" (comment 18). Is the only way in which you think the approach is wrong the fact that the patch is also moving the other menuitem styles?

> I think the approach taken here is problematic and will either need to be
> tweaked significantly if not completely replaced with something else. I see
> no point in leaving this in the tree as-is, and there's a risk that this
> will confuse others working with any of the affected elements.

If you think we should be doing something else entirely, I think rather than saying "back this out", we at least need a concrete suggestion as to what you think is a better approach.

As a proposal, I can probably put up patches on bug 1420229 on Monday to fix up selectors such that that bug gets fixed. This would be my preferred solution because I believe it's work that needs to be done anyway when we move menuitems off XBL, too. Is that acceptable? Alternatively, I could put up a clone of the existing cset here that only appends the menuseparator styles to global.css and leaves the rest of menu.css alone, if that's better in your eyes? Is there some third/other option you would prefer that I missed?

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #21)
> I'm not convinced moving just the separator styles would fix the root cause
> here. It would presumably fix some of the padding issue because we wouldn't
> alter anything for in-non-separator-menuitem nodes (and their padding in the
> BMB popup is clearly not right on Windows right now), but it wouldn't fix
> any issues with the separator styles themselves unless we also update styles
> there (and/or elsewhere) as necessary.
> 
> Point being, that doesn't seem like a radically different approach, just
> delaying something we'd do eventually by only moving a subset of menu.css .
> If we're going to be following the same pattern (move affected styles to
> global.css when moving XUL nodes off XBL bindings), seems we might as well
> port the entire sheet and fix all the rules up so there's no fallout either
> way, given that the menuitem bindings will want to also move eventually.
> 
> But it kind of sounds like you think even that pattern is wrong? I'm a bit
> confused, because in part (e.g. comment 17) you seem to be saying we "should
> only move the relevant styles" and in part you seem to be saying "this
> approach is wrong" (comment 18). Is the only way in which you think the
> approach is wrong the fact that the patch is also moving the other menuitem
> styles?

Moving only the relevant subset to global.css would avoid those issues for what would remain in menu.css, but for <menuseparator> issue 1) in particular would presumably persist. 2) wouldn't be an immediate problem anymore although it would remain a concern in principle as we're looking for a solution that works for other bindings as well.

> > I think the approach taken here is problematic and will either need to be
> > tweaked significantly if not completely replaced with something else. I see
> > no point in leaving this in the tree as-is, and there's a risk that this
> > will confuse others working with any of the affected elements.
> 
> If you think we should be doing something else entirely, I think rather than
> saying "back this out", we at least need a concrete suggestion as to what
> you think is a better approach.

This doesn't really follow. We can establish that the current solution is broken without having the right solution at hand, and then revert to the last working state; "Let's back this out and figure out the right way to do this" seems like a perfectly fine conclusion.

> As a proposal, I can probably put up patches on bug 1420229 on Monday to fix
> up selectors such that that bug gets fixed.

It's not clear to me how you'd fix up selectors to avoid running issue 1). At best I think this would be a workaround, not something we'd want to use as a model for other bindings.

> This would be my preferred
> solution because I believe it's work that needs to be done anyway when we
> move menuitems off XBL, too. Is that acceptable? Alternatively, I could put
> up a clone of the existing cset here that only appends the menuseparator
> styles to global.css and leaves the rest of menu.css alone, if that's better
> in your eyes? Is there some third/other option you would prefer that I
> missed?

I don't have a good complete proposal right now. I think we need to include Brian in this discussion and should back this out in the meantime, given the cascade issue that could affect others who might try to override the style of a <menu*> element.

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #22)
> (In reply to :Gijs from comment #21)
> > I'm not convinced moving just the separator styles would fix the root cause
> > here. It would presumably fix some of the padding issue because we wouldn't
> > alter anything for in-non-separator-menuitem nodes (and their padding in the
> > BMB popup is clearly not right on Windows right now), but it wouldn't fix
> > any issues with the separator styles themselves unless we also update styles
> > there (and/or elsewhere) as necessary.
> > 
> > Point being, that doesn't seem like a radically different approach, just
> > delaying something we'd do eventually by only moving a subset of menu.css .
> > If we're going to be following the same pattern (move affected styles to
> > global.css when moving XUL nodes off XBL bindings), seems we might as well
> > port the entire sheet and fix all the rules up so there's no fallout either
> > way, given that the menuitem bindings will want to also move eventually.
> > 
> > But it kind of sounds like you think even that pattern is wrong? I'm a bit
> > confused, because in part (e.g. comment 17) you seem to be saying we "should
> > only move the relevant styles" and in part you seem to be saying "this
> > approach is wrong" (comment 18). Is the only way in which you think the
> > approach is wrong the fact that the patch is also moving the other menuitem
> > styles?
> 
> Moving only the relevant subset to global.css would avoid those issues for
> what would remain in menu.css, but for <menuseparator> issue 1) in
> particular would presumably persist. 2) wouldn't be an immediate problem
> anymore although it would remain a concern in principle as we're looking for
> a solution that works for other bindings as well.

Sorry, but what are (1) and (2) ? I don't see a clear reference in your comment, or in my quoted comment.

> > > I think the approach taken here is problematic and will either need to be
> > > tweaked significantly if not completely replaced with something else. I see
> > > no point in leaving this in the tree as-is, and there's a risk that this
> > > will confuse others working with any of the affected elements.
> > 
> > If you think we should be doing something else entirely, I think rather than
> > saying "back this out", we at least need a concrete suggestion as to what
> > you think is a better approach.
> 
> This doesn't really follow. We can establish that the current solution is
> broken without having the right solution at hand, and then revert to the
> last working state; "Let's back this out and figure out the right way to do
> this" seems like a perfectly fine conclusion.

We disagree on how broken this is. I think this is basically minor followup stuff, and you think it's a breaking problem that invalidates the approach.

Just so we're on the same page, from what I can tell, the issue is that rules from XBL stylesheets never override rules from "normal" stylesheets, even if the XBL rules have higher specificity. Now that the rules have moved, they have started overriding such rules based on specificity, so:

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/menu.css#99-103

menu.menu-iconic > .menu-iconic-left,
menuitem.menuitem-iconic > .menu-iconic-left {
  -moz-appearance: menuimage;
}

overrides

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1237-1240

.subviewbutton > .menu-iconic-left {
  -moz-appearance: none;
}

I think it would be fine to simply increase the specificity of the panelUI.inc.css rule or decrease the specificity of the menu.css rule, and take a similar approach for other styles that are now no longer applying as originally intended, and then in future to take a similar approach to other styles that would cause issues with other migrations when we encounter them.

It seems like you think this is not acceptable, is that right?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #23)
> Just so we're on the same page, from what I can tell, the issue is that
> rules from XBL stylesheets never override rules from "normal" stylesheets,
> even if the XBL rules have higher specificity. Now that the rules have
> moved, they have started overriding such rules based on specificity, so:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> menu.css#99-103
> 
> menu.menu-iconic > .menu-iconic-left,
> menuitem.menuitem-iconic > .menu-iconic-left {
>   -moz-appearance: menuimage;
> }
> 
> overrides
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUI.inc.css#1237-1240
> 
> .subviewbutton > .menu-iconic-left {
>   -moz-appearance: none;
> }
> 
> I think it would be fine to simply increase the specificity of the
> panelUI.inc.css rule or decrease the specificity of the menu.css rule, and
> take a similar approach for other styles that are now no longer applying as
> originally intended, and then in future to take a similar approach to other
> styles that would cause issues with other migrations when we encounter them.

Assuming this is indeed the problem and we agree on the approach for fixing it (and it makes sense to me - this is moving from non-standard behavior to standard), I can think of at least two things we could do to mitigate this going forward:

1) Enhance mozscreenshots to cover more configurations so issues can get detected on try runs before landing. I suspect menus in particular are poorly covered right now, while tabs / toolbars / customizing have better coverage but may still need to be expanded.
2) Modify the browser-instrumentation test run to report situations where we hit the case of a more specific selector applying after a less specific one. There's already some code from Bug 1376546 that traverses every element in browser.xul to generate graphs of the most commonly used XBL bindings: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#130. If we can detect this case it would tell us there's a potential issue when changing how a CSS files loads

Comment 25

[Brian Grinstead [:bgrins]]

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1420229#c15, I believe that Gijs' analysis of the problem in Comment 23 is right.

Another thing I noticed is that browser/skin/ (and thus global.css) is loaded after other browser/skin stylesheets in browser.xul (panel.css, panelUI.css) [0]. This means that even if we match the specificity in global.css and panelUI.css, the global styles would get priority over the panelUI styles, which seems conceptually wrong.

I'm doing a mozscreenshots run now to see if switching the order of these sheets causes any obvious problems. If not then I'd propose we make that change as well as those outlined in Comment 23, assuming that we agree that modifying the specificity to match normal CSS semantics is the right thing to do.

[0]: https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#12-14

Comment 26

[Brian Grinstead [:bgrins]]

I'm thinking of two main approaches we could take for porting XBL stylesheets going forward:

1) Import the styles in global.css as we do here, find and fix regressions caused in each one due to the changing importance of the sheet
2) Convert these sheets into UA styles (which aren't exactly the same but will interact with document stylesheets in the same way - https://bugzilla.mozilla.org/show_bug.cgi?id=1420229#c17). This could be done either by porting all of global.css into a UA style or by adding a new css file just for converted components.

A pro to (1) is that we'd end up in a more standard place as far as CSS semantics. And possibly simplify styles (like removing !important usage as in Comment 19)
A pro to (2) is that it would streamline conversion of each binding by removing time spent detecting and fixing regressions. I suspect this would end up making the project significantly easier just based on the potential for subtle breakage in each one.

Something else to consider for further in the future is that that if we ultimately want to use scoped styles with Shadow DOM, those stylesheets seem to have higher priority than the document styles, so likely would need to have similar fixes to (1) to get there.

Do you have a preference between those two, or any different approaches in mind?

Comment 27

[:Gijs (he/him)]

I would lean towards (1) because I think having only specificity to worry about will simplify our lives, but I could live with either, tbh.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #27)
> I would lean towards (1) because I think having only specificity to worry
> about will simplify our lives, but I could live with either, tbh.

I also like (1) in principle, but am concerned that coupling together the specificity changes with Custom Element migrations could make both things more difficult than they would be separately. I've put up a WIP for (2) in Bug 1420229 at least so we have something concrete to compare with.

Comment 29

[Brian Grinstead [:bgrins]]

I'd like to take a decision here so we can get Bug 1420229 resolved. I propose that we load stylesheets that are currently <resources> in XBL as UA styles imported from new component.css file that gets loaded automatically in all docs that allow XUL - see the patch in Bug 1420229 for the idea here.

I think this will make the XBL replacement process for individual bindings much simpler than also needing to modify the specificity of selectors (in the XBL and/or toolkit/browser sheets). We'll need to revisit specificity if/when we migrate some components to Shadow DOM, but in the meantime I think this is the option least likely to cause regressions. Gijs, Dão, what do you think?

Comment 30

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> I'd like to take a decision here so we can get Bug 1420229 resolved. I
> propose that we load stylesheets that are currently <resources> in XBL as UA
> styles imported from new component.css file that gets loaded automatically
> in all docs that allow XUL - see the patch in Bug 1420229 for the idea here.
> 
> I think this will make the XBL replacement process for individual bindings
> much simpler than also needing to modify the specificity of selectors (in
> the XBL and/or toolkit/browser sheets). We'll need to revisit specificity
> if/when we migrate some components to Shadow DOM, but in the meantime I
> think this is the option least likely to cause regressions. Gijs, Dão, what
> do you think?

Sounds fine to me.