1420229 - Load menu.css (and future XBL sheets) as a UA stylesheet instead of a document stylesheet to limit the risk of regression when migrating the styles out of XBL

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 1

[:Gijs (he/him)]

I wonder if this is because it's this weird pseudo-menu thing and it has its own XBL binding and CSS...

Comment 2

[Brian Grinstead [:bgrins]]

Could someone post a screenshot?  I tried following the STR but am not seeing a difference on the Bookmarks Menu in a Dev Edition build vs Nightly.

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Attachment: bbokmark2017.jpg

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #3)
> Created attachment 8931451 [details]
> bbokmark2017.jpg

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Could someone post a screenshot?  I tried following the STR but am not
> seeing a difference on the Bookmarks Menu in a Dev Edition build vs Nightly.


See attachment

Comment 5

[Brian Grinstead [:bgrins]]

FWIW I can't reproduce this on Nightly Windows 7 or OSX. Either I'm missing something or maybe this is Win10 / profile specific.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: bookmarks-compare-win7.png

This is what I see comparing nightly vs dev edition on Windows 7

Comment 7

[Alice0775 White]

Attachment: 59.0a1 vs 58.0b5

Build ID 	20171125100431
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

definitely increased the height

Comment 8

[David Smith]

Attachment: Bookmarks menu

I can confirm it on Windows 7 as well.  Attachment shows Nov 25 nightly vs Nov 19 nightly.

Comment 9

[Brian Grinstead [:bgrins]]

OK, thanks for confirming the issue. I'll have a look on Monday to see if I can reproduce on a different computer.

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: css-rules-de-58-BMB_bookmarksPopup.txt

Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on DevEdition

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: css-rules-nightly-59-BMB_bookmarksPopup.txt

Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on Nightly

Comment 12

[Brian Grinstead [:bgrins]]

If you search for menuitem[type="checkbox"] in https://bug1420229.bmoattachments.org/attachment.cgi?id=8932171 (58) and https://bug1420229.bmoattachments.org/attachment.cgi?id=8932172 (59), you will see that the position relative to the ".subview-subheader, panelview .toolbarbutton-1, .subviewbutton, .widget-overflow-list .toolbarbutton-1" selector has changed (in this case it is matching.subviewbutton).

This is fitting with what Gijs was saying in https://bugzilla.mozilla.org/show_bug.cgi?id=1416493#c23.  The new behavior is standard, although I'd like to understand better why the old behavior is working the way it is.

Boris, when a stylesheet is loaded via <resources> in XBL it appears that a more specific selector from that sheet matches after a less specific selector from a sheet loaded on the page.  In this case, the menuitem[type="checkbox"] rule from a XBL sheet was previously applied *after* the .subviewbutton rule from a normal sheet. Is that intentional behavior for XBL and/or is it documented anywhere?

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: win7-58-59-comparison.png

screenshot of what I'm seeing on Win7, highlighting one of the rules that has flipped order

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: win7-58-59-comparison.png

Box was in the wrong place on the screenshot, updating

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is that intentional behavior for XBL

Pretty much, yes.  During the "walk all rules from least to most important" algorithm, all XBL rules are walked before all document rules.  They basically behave like presentation hints.

There are some technical reasons it was implemented this way, but also some general considerations for doing it like that: rules from inside a binding are an implementation detail of that binding and it's not good if behavior observable from outside the binding depends on the exact selectors those rules use.  Furthermore, since binding rules supply "default" styles in some sense, it makes sense for document rules to override them.

Fwiw, https://readable-email.org/list/www-style/topic/scoped-style-sheets#content-1 is relevant here, as is possibly that whole thread.

> is it documented anywhere?

I don't know.  Our documentation for XBL is rather sparse...

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> > Is that intentional behavior for XBL
> 
> Pretty much, yes.  During the "walk all rules from least to most important"
> algorithm, all XBL rules are walked before all document rules.  They
> basically behave like presentation hints.

Would loading a CSS file as a UA stylesheet have the same behavior as a XBL sheet? So if we did the same with these CSS files as we do with XULSheet in nsLayoutStylesheetCache.cpp?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Would loading a CSS file as a UA stylesheet have the same behavior as a XBL sheet?

In terms of interaction with the document sheets, yes.

In terms of interaction with user sheets or preshints, no; UA sheets are less specific than user sheets or preshints, while XBL is more specific.  That might be OK, though.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

This provides a place for XBL stylesheets to exist with less important application
than a document sheet, similar to the presentation hint that they currently have

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

Comment 19

[Brian Grinstead [:bgrins]]

The patch here converts menu.css into a UA style (approach 2 in https://bugzilla.mozilla.org/show_bug.cgi?id=1416493#c26). I can confirm this does fix the issue on Windows. Running some talos tests now to see if this regresses anything.

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

:bz, if we decide loading this as a UA sheet is the correct route to go for the frontend, does this patch look generally acceptable? Here's what I think it should be doing:

- Load inside of any XUL document in the parent process
- Load inside of an XHTML document that has XUL elements, like: https://dxr.mozilla.org/mozilla-central/search?q=path%3Axhtml+%22%3Cxul%3A%22&redirect=true. I've checked locally on about:sessionrestore and this seems to be the case
- Do not load inside of any content documents

The reason I made a new CSS file and didn't include this in xul.css is that AFAICT xul.css does get loaded into HTML documents that include <audio> and <video> tags (https://searchfox.org/mozilla-central/source/dom/xul/nsXULElement.cpp#813) and there's no reason why we would want any of these migrated XBL sheets to load in that context

Comment 21

[Brian Grinstead [:bgrins]]

Also: is there a way to load a sheet as a preshint or with the same importance as a xbl sheet so that it would be closer to the current semantics?

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

Converting this to a review request since there's consensus that this is the route we'll want to go with on the frontend as we work through other bindings (unless if there's a way to load the sheets with the exact same importance as they used to be loaded as i.e. Comment 21)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here....

> is there a way to load a sheet as a preshint or with the same importance as a xbl sheet 

Not right now.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

r=me

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

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

Comment 26

[Brian Grinstead [:bgrins]]

Sorry for the review spam, the r+ didn't get copied over into mozreview after I added you to the commit message

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

https://reviewboard.mozilla.org/r/204382/#review214284

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Can we land this?  thanks

Comment 29

[Brian Grinstead [:bgrins]]

There's some bustage, especially on Windows, that needs to get fixed before this can be landed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=516ebedf3baa2cdacb4d91ca0c2aeb052e139104. Looking at it this week.

Comment 30

[Brian Grinstead [:bgrins]]

I think that previous try push had a bad base - here's a more up to date push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aff3cc7a566d267e74644f7669b30abd37571f1

Comment 31

[Brian Grinstead [:bgrins]]

Tracked down the test failure on Windows reftests such as 362901-1.html (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aff3cc7a566d267e74644f7669b30abd37571f1&selectedJob=153909079).  It's a Servo-specific bug due to menu.css including :-moz-window-inactive on Windows and that breaking other CSS on the page - I've filed it as Bug 1428164.

In the meantime, I don't see why we should be loading components.css at all in the content process.  I'm not sure what the use case for LoadsFullXULStyleSheetUpFront() (and more specifically: AllowXULXBL()) to be true in the content process, but since it is for these reftests it makes me wonder if there's a use case I'm missing.  If I guard this sheet loading with XRE_IsParentProcess() those errors go away: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fff15254e892178966674e57b63291c9f8af68).

Comment 32

[Boris Zbarsky [:bzbarsky]]

AllowXULXBL() can be true in the content process in "remote XUL" cases, if that helps.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> AllowXULXBL() can be true in the content process in "remote XUL" cases, if
> that helps.

OK makes sense then I guess, assuming that reftests are one of these cases. If we can fix Bug 1428164 then I guess there's not much harm in also loading components.css in the content process, since this would only be loaded in special cases and not for web pages.

Comment 34

[Brian Grinstead [:bgrins]]

Alright, thanks to the fix in Bug 1428164 the reftests are passing again. Next mystery: an xpcshell test on linux crashes in libgtk with this patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2e86bcf5b2972800319ab6720947e0b85471d86&selectedJob=154482699.

I don't yet understand why any of this machinery, like the nsDocumentViewer causing the new stylesheet to load, is running in an xpcshell test. Or why it only affects this test.

But the crash is apparently being triggered by some CSS rules in menu.css. I can confirm the crash in a local build with the patch from the above try push applied via: `./mach xpcshell-test browser/extensions/formautofill/test/unit/heuristics/`. If I remove all of the contents of menu.css then the crash doesn't happen, and I've at least confirmed that the `color: MenuText` line here has something to do with it: https://dxr.mozilla.org/mozilla-central/rev/81362f7306fe413b19fdba27cd0e9a5525d902e1/toolkit/themes/linux/global/menu.css#19. For instance if I delete everything in menu.css and replace it with:

    foo {
      color: MenuText;
    }

The crash still happens, but if I change it to:

    foo {
      color: red;
    }

The crash doesn't happen anymore.

Comment 35

[Brian Grinstead [:bgrins]]

Oh, and if I add `foo { color: MenuText; }` anywhere in xul.css on a fresh m-c checkout the crash happens as well (linux only), so it doesn't appear to be related to menu.css / components.css specifically.

Comment 36

[Brian Grinstead [:bgrins]]

The fixture file that's causing the problem is https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/test/fixtures/third_party/Walmart/Payment.html. I've narrowed it down to the existence of an svg element in that page. If I keep the extra rule in xul.css from Comment 35 and then replace the contents of Payment.html with `<svg></svg>` I can still reproduce the crash.

The call to DOMParser.parseFromString [0] seems to trigger it. I guess some combination of the DOM parser parsing the svg element and the new `color: MenuText` declaration in xul.css is causing the crash in graphics.

[0] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/modules/MockDocument.jsm#24, which is called by https://dxr.mozilla.org/mozilla-central/rev/81362f7306fe413b19fdba27cd0e9a5525d902e1/browser/extensions/formautofill/test/unit/head.js#106

Comment 37

[Brian Grinstead [:bgrins]]

Here's the call to ForceEnableXULXBL() that's causing xul.css/components.css to load for this mock document in the xpcshell test: https://dxr.mozilla.org/mozilla-central/rev/81362f7306fe413b19fdba27cd0e9a5525d902e1/dom/base/DOMParser.cpp#98-100. After that, it crashes in the call to nsContentUtils::ParseDocumentHTML with:

   pid:66216 (xpcshell:66216): Gtk-CRITICAL **: gtk_settings_get_for_screen: assertion 'GDK_IS_SCREEN (screen)' failed
   pid:66216 (xpcshell:66216): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT (object)' failed
   pid:66216 (xpcshell:66216): GLib-GObject-CRITICAL **: g_object_set: assertion 'G_IS_OBJECT (object)' failed
   pid:66216 (xpcshell:66216): Gtk-CRITICAL **: gtk_settings_get_for_screen: assertion 'GDK_IS_SCREEN (screen)' failed
   pid:66216 (xpcshell:66216): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion 'G_IS_OBJECT (object)' failed

If I force gfxPlatform::IsHeadless to return true then the crash doesn't happen anymore, so this seems to be some weird combination of xpcshell tests not being detected as 'headless' (in the sense that they don't set the MOZ_HEADLESS environment variable), and this particular DOMParser.parseFromString call (which includes both an svg element in the markup and a system font color in CSS) hitting a crash that has been fixed by the headless work.

I think the xpcshell tests may be intended to be run as headless by default, but the environment variable is only set if the 'headless' arg is passed: https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#662-664. The weird thing is that the test harness doesn't support the headless arg, so I will try just unconditionally setting the variable and see if any tests fail.

Comment 38

[Brian Grinstead [:bgrins]]

There are a few xpcshell tests that fail once put into headless mode (
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98e41340d3571077049acdac9c7140db2ecd4ceb):

* toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
* toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js
* devtools/server/tests/unit/test_objectgrips-21.js (debug only)
* toolkit/components/aboutmemory/tests/xpcshell/test_gpuprocess.js (debug only)

I've looked at a couple and it seems these are cases where components that worked fine in xpcshell are bailing out now that they are in headless mode. I'm not sure exactly what to do here - I think one of the following:

1) Fix these tests so that the whole xpcshell suite works in headless mode
2) Work around the failing test case (Comments 34-37) by maybe changing the markup in the fixture document to not include an svg element
3) Make it so that dealing with svg + system fonts in CSS doesn't crash (when we aren't in headless mode but we also don't have a screen)

Brendan, any opinion? (see Comment 37 for more context). I tend to think we should aim for (1) but I'd like your opinion on if it's desirable and if so how hard it would be.

Comment 39

[Brendan Dahl [:bdahl]]

xpcshell tests are bit weird in that they run in a headless like mode but are not run by default in the new headless mode. There are some tests where headless mode is enabled though [1].

Could you just enable headless mode for certain tests like in [1]?

[1] https://searchfox.org/mozilla-central/source/widget/headless/tests/xpcshell.ini

Comment 40

[Brian Grinstead [:bgrins]]

Try push with the <svg> element removed from the fixture (i.e. option 2 from Comment 38): https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ae77ae9d31e239536b23c4f4eb78062d626593

Comment 41

[Brian Grinstead [:bgrins]]

(In reply to Brendan Dahl [:bdahl] from comment #39)
> xpcshell tests are bit weird in that they run in a headless like mode but
> are not run by default in the new headless mode. There are some tests where
> headless mode is enabled though [1].
> 
> Could you just enable headless mode for certain tests like in [1]?
> 
> [1]
> https://searchfox.org/mozilla-central/source/widget/headless/tests/xpcshell.
> ini

Interesting, I didn't realize this was an option - I'll try.  I had assumed this wouldn't work for individual tests due to caching of the environment variable in graphics code i.e. https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/gfx/thebes/gfxPlatform.cpp#906-915, but maybe each test/directory runs its own process?

Comment 42

[Brian Grinstead [:bgrins]]

Ongoing try push with headless set in the xpcshell.ini for formautofill: https://treeherder.mozilla.org/#/jobs?repo=try&revision=442f4b06dfe8549610a2df47e73e1f64bb323743

Comment 43

[Brian Grinstead [:bgrins]]

Attachment: Bug 1420229 - Run formautofill xpcshell tests in headless mode;

This is needed because DOMParser is invoked on some fixture HTML that contains an
<svg> element (Walmart/Payment.html). When this happens **and** there is
a declaration for `color: MenuText` in a UA stylesheet (which is added in the next
changeset), there is a graphics crash. This is fixed in the new headless mode,
so this changeset forces the directory to run in it.

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

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

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

Comment 45

[Brendan Dahl [:bdahl]]

(In reply to Brian Grinstead [:bgrins] from comment #41)
> but maybe each test/directory runs its own process?

Every single xpcshell test run's in its own process. I was also very confused when I discovered this, but it was part of some work awhile ago to parallelize xpcshell tests.

Comment 46

[Brian Grinstead [:bgrins]]

Talos shows a few improvements and one regression, but I'm unsure if it's noise: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9949d25409d5ad2dfe9c9e45ba3e8d9361c74a9b&newProject=try&newRevision=b0a55a96d8077d112661d7f18d211456983efc65&framework=1&showOnlyImportant=1.

Comment 47

[Brendan Dahl [:bdahl]]

Comment on attachment 8940893 [details]
Bug 1420229 - Run formautofill xpcshell tests in headless mode;

https://reviewboard.mozilla.org/r/211154/#review217204

Comment 48

[Brian Grinstead [:bgrins]]

There's some orange, but I see a lot of orange on the base try push without any patches applied as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7c62f66a211b8f556b499310ad576f3e41a418c.

Comment 49

[Brian Grinstead [:bgrins]]

Unfortunately it looks like this is triggering a perma-orange on layout/style/test/test_transitions.html in Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=126883f1065e4ce6c6c21375896f8c26f1aa5f94&group_state=expanded&selectedJob=155405354. It must be some kind of timing issue, as none of the rules should apply to anything in that test (all the CSS is XUL namespaced).

I will look closer on Windows soon, but if this can't be landed before the soft freeze for 60 and we think it's too risky to land after that then the backup option would be to land a CSS fix for this visual issue with the bookmarks menu and then load the sheets as UA in a new bug, post-60.

Comment 50

[Brian Grinstead [:bgrins]]

Even if we ditch the components.css file entirely and instead import menu.css directly from xul.css the perma-orange from Comment 49 persists: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed1a227a7a4bb01b431fb6acc614e1a3ddb5c16&selectedJob=155453764

Comment 51

[Brian Grinstead [:bgrins]]

So.. this looks like another moz-window-inactive issue. The test is passing if I remove the moz-window-inactive selector from menu.css: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43dc981b70940d00f5a35533210e4895696a697c&group_state=expanded&selectedJob=155475849 / https://hg.mozilla.org/try/rev/3388c02febac14e328a9fdb6f9df7015f7825c55#l5.12

Comment 52

[Brian Grinstead [:bgrins]]

Since Bug 1429630 won't be fixed for 60, I'm going to suggest we spin off a new bug to fix the visual regression before the merge, and keep this one opened to migrate menu.css to UA styles since there's a lot of context for that work already here.

Comment 53

[Brian Grinstead [:bgrins]]

Comment on attachment 8940893 [details]
Bug 1420229 - Run formautofill xpcshell tests in headless mode;

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

Comment 54

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

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

Comment 55

[Brian Grinstead [:bgrins]]

Gijs, I think this should be green now that bug 1409672 has landed. I want to get another set of eyes before trying to land it though. The idea here is that we are going to put sheets that used to be in <xbl:resources> sheets over here as UA sheets. The reason I did it this way - in a new file loaded from the style sheet cache instead of doing an @import at the top of xul.css which is already being loaded is:

1) It seems like these rules belong after those in xul.css so they get priority over them (although I doubt there's a lot of overlap - xul.css doesn't set too many properties).
2) xul.css does seem to get loaded into HTML docs sometimes - see https://dxr.mozilla.org/mozilla-central/rev/5faab9e619901b1513fd4ca137747231be550def/dom/xul/nsXULElement.cpp#798. Although I'm just looking into this and I think this may be out of date - I don't think IsInFeedSubscribeLine is ever true and <videocontrols> seems to only generate HTML content... need to investigate more.

I don't feel too strongly about doing it one way vs the other though - happy to change the approach.

Comment 56

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

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

Comment 57

[:Gijs (he/him)]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

https://reviewboard.mozilla.org/r/204382/#review220460

Yeah, I'm happy with this. I haven't done a visual check but I'm pretty confident given the in-depth look various people have had at this stuff already - let me know if you were expecting me to do a visual review...

Do you want to remove the stuff from bug 1429857 here immediately or do it in a separate bug?

Comment 58

[Richard Marti (:Paenglab)]

Brian, would it be somehow possible for Thunderbird to insert the in m-c xul.css removed and in c-c re-added binding styles during build time or with a override into this components.css file? This would make our life easier.

Comment 59

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #55)
> 2) xul.css does seem to get loaded into HTML docs sometimes - see
> https://dxr.mozilla.org/mozilla-central/rev/
> 5faab9e619901b1513fd4ca137747231be550def/dom/xul/nsXULElement.cpp#798.
> Although I'm just looking into this and I think this may be out of date - I
> don't think IsInFeedSubscribeLine is ever true and <videocontrols> seems to
> only generate HTML content... need to investigate more.

Boris, I looked into this a bit more and did a try push where I did NS_ASSERTION(false) in the `!XULElementsRulesInMinimalXULSheet(NodeInfo()->NameAtom())` block where we dynamically load xul.css, and I don't think that this ever actually happens anymore (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed97338f841849d793b387024f7e171d8ed5efd). I can spin this off into another bug to remove this functionality unless if there's a case you're aware of that I'm missing.

Comment 60

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (lower availability until Jan 27) from comment #57)
> Comment on attachment 8933461 [details]
> Bug 1420229 - Create a new components.css file and load it as a UA stylesheet
> 
> https://reviewboard.mozilla.org/r/204382/#review220460
> 
> Yeah, I'm happy with this. I haven't done a visual check but I'm pretty
> confident given the in-depth look various people have had at this stuff
> already - let me know if you were expecting me to do a visual review...
> 
> Do you want to remove the stuff from bug 1429857 here immediately or do it
> in a separate bug?

I will do an extra visual check on Windows with the stuff from bug 1429857 reverted before landing. We may as well land the backout as part of this bug.

Comment 61

[Brian Grinstead [:bgrins]]

(In reply to Richard Marti (:Paenglab) from comment #58)
> Brian, would it be somehow possible for Thunderbird to insert the in m-c
> xul.css removed and in c-c re-added binding styles during build time or with
> a override into this components.css file? This would make our life easier.

I'm not sure I understand the question, so let me know if this doesn't answer it. This patch doesn't affect xul.css so there shouldn't need to be any changes there.

As we migrate other bindings into components.css (i.e. toolbars.css) they will move out of <xbl:resources> and into this sheet which gets loaded as a UA style. This has slightly different importance than a XBL sheet (UA sheets are less important than user sheets and preshints, while XBL sheets are more important than both of them) so as sheets move there could well need to be some changes. If you plan to fork and maintain the XBL bindings as they go away in m-c then you could continue to load the sheets as <xbl:resources /> and just make components.css an empty file.  Or alternatively, you should be able to just use components.css as-is in c-c, especially if you expect to migrate to Custom Elements alongside m-c.

Comment 62

[Richard Marti (:Paenglab)]

Thank you Brian. I think I know now how this works.

Comment 63

[Brian Grinstead [:bgrins]]

Attachment: Bug 1420229 - Backed out changeset 05bab8e59cd1 (bug 1429857);

We don't need this workaround anymore now that we are loading menu.css as a UA sheet

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

Comment 64

[Brian Grinstead [:bgrins]]

Comment on attachment 8940893 [details]
Bug 1420229 - Run formautofill xpcshell tests in headless mode;

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

Comment 65

[Brian Grinstead [:bgrins]]

Comment on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204382/diff/5-6/

Comment 66

[Brian Grinstead [:bgrins]]

Comment on attachment 8944865 [details]
Bug 1420229 - Backed out changeset 05bab8e59cd1 (bug 1429857);

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

Comment 67

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #60)
> (In reply to :Gijs (lower availability until Jan 27) from comment #57)
> > Comment on attachment 8933461 [details]
> > Bug 1420229 - Create a new components.css file and load it as a UA stylesheet
> > 
> > https://reviewboard.mozilla.org/r/204382/#review220460
> > 
> > Yeah, I'm happy with this. I haven't done a visual check but I'm pretty
> > confident given the in-depth look various people have had at this stuff
> > already - let me know if you were expecting me to do a visual review...
> > 
> > Do you want to remove the stuff from bug 1429857 here immediately or do it
> > in a separate bug?
> 
> I will do an extra visual check on Windows with the stuff from bug 1429857
> reverted before landing. We may as well land the backout as part of this bug.

Just checked and confirmed that the styles are applied in the original order with the patches applied (panelUI.css first, then menu.css, then xul.css, then minimal-xul.css). Since the issue in Bug 1429857 goes away I've included a backout in the series here.

Comment 68

[Brian Grinstead [:bgrins]]

Comment on attachment 8944865 [details]
Bug 1420229 - Backed out changeset 05bab8e59cd1 (bug 1429857);

https://reviewboard.mozilla.org/r/215026/#review220636

Comment 69

[Boris Zbarsky [:bzbarsky]]

Looks like the feedreaderUI binding is gone, right?  So I agree about the IsInFeedSubscribeLine bit.

As far as videocontrols, they do generate some XUL elements in the "noControls" and "touchControls" bindings.  The former only uses <vbox> and <box>, which aren't really something that needs styling too much.  The latter has <stack> and all sorts of other stuff; not sure whether it needs styling.

Looks to me like "touchControls" is only used on Android.  I don't know whether it just doesn't end up needing anything from xul.css, or whether there's something else going with NS_ASSERTION in Android tests or what.

Comment 70

[Boris Zbarsky [:bzbarsky]]

Oh, and clearly if we can remove the special cases, so much the better.

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23d4aff26260
Run formautofill xpcshell tests in headless mode;r=bdahl
https://hg.mozilla.org/integration/autoland/rev/2a3561bcb468
Create a new components.css file and load it as a UA stylesheet;r=bz,Gijs
https://hg.mozilla.org/integration/autoland/rev/4118a23fa0b3
Backed out changeset 05bab8e59cd1 (bug 1429857);r=bgrins

Comment 72

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #69)
> Looks like the feedreaderUI binding is gone, right?  So I agree about the
> IsInFeedSubscribeLine bit.
> 
> As far as videocontrols, they do generate some XUL elements in the
> "noControls" and "touchControls" bindings.  The former only uses <vbox> and
> <box>, which aren't really something that needs styling too much.  The
> latter has <stack> and all sorts of other stuff; not sure whether it needs
> styling.
> 
> Looks to me like "touchControls" is only used on Android.  I don't know
> whether it just doesn't end up needing anything from xul.css, or whether
> there's something else going with NS_ASSERTION in Android tests or what.

Ah OK - I switched it to a MOZ_CRASH and am seeing failures in a few tests now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a63f2c8d8440679ffa5e7bc8a0b3c3c8b38b3f08&group_state=expanded&selectedJob=158260253. Good to know - I've filed bug 1432851 to remove the feedreader part of the assertion and will have to look more closely at noControls and touchControls.

Comment 73

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/23d4aff26260
https://hg.mozilla.org/mozilla-central/rev/2a3561bcb468

Comment 74

[Julien Cristau [:jcristau]]

Can/should we uplift this to beta to fix the regression in 59?

Comment 75

[:Gijs (he/him)]

(In reply to Julien Cristau [:jcristau] from comment #74)
> Can/should we uplift this to beta to fix the regression in 59?

I think this happened in bug 1429857, so perhaps this should be marked unaffected/disabled. bgrins can confirm.

Comment 76

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #75)
> (In reply to Julien Cristau [:jcristau] from comment #74)
> > Can/should we uplift this to beta to fix the regression in 59?
> 
> I think this happened in bug 1429857, so perhaps this should be marked
> unaffected/disabled. bgrins can confirm.

Yes, that's right - there was a workaround for 59 landed in Bug 1429857. Updating the flag.