Closed Bug 1420229 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Platform:
x86_64
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: jmjjeffery, Assigned: bgrins)

References

Details

Attachments

(10 files, 1 obsolete file)

bbokmark2017.jpg
75.95 KB, image/jpeg
Details
bookmarks-compare-win7.png
81.09 KB, image/png
Details
59.0a1 vs 58.0b5
186.98 KB, image/png
Details
Bookmarks menu
23.39 KB, image/png
Details
css-rules-de-58-BMB_bookmarksPopup.txt
8.15 KB, text/plain
Details
css-rules-nightly-59-BMB_bookmarksPopup.txt
5.31 KB, text/plain
Details
win7-58-59-comparison.png
534.59 KB, image/png
Details
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet
59 bytes, text/x-review-board-request
bzbarsky
: review+
Gijs
: review+
Details
Bug 1420229 - Run formautofill xpcshell tests in headless mode;
59 bytes, text/x-review-board-request
bdahl
: review+
Details
Bug 1420229 - Backed out changeset 05bab8e59cd1 (bug 1429857);
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
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
Blocks: 1416493
Keywords: regression
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bgrinstead)
I wonder if this is because it's this weird pseudo-menu thing and it has its own XBL binding and CSS...
Flags: needinfo?(gijskruitbosch+bugs)
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.
(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
Has Regression Range: --- → yes
Has STR: --- → yes
Version: 58 Branch → Trunk
FWIW I can't reproduce this on Nightly Windows 7 or OSX. Either I'm missing something or maybe this is Win10 / profile specific.
This is what I see comparing nightly vs dev edition on Windows 7
Attached image 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
Attached image Bookmarks menu — 
I can confirm it on Windows 7 as well.  Attachment shows Nov 25 nightly vs Nov 19 nightly.
OK, thanks for confirming the issue. I'll have a look on Monday to see if I can reproduce on a different computer.
Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on DevEdition
Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on Nightly
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?
Flags: needinfo?(bgrinstead) → needinfo?(bzbarsky)
Attached image win7-58-59-comparison.png (obsolete) — 
screenshot of what I'm seeing on Win7, highlighting one of the rules that has flipped order
Box was in the wrong place on the screenshot, updating
Attachment #8932178 - Attachment is obsolete: true
> 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...
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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 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
Attachment #8933461 - Flags: feedback?(bzbarsky)
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?
Blocks: 1422386
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)
Attachment #8933461 - Flags: feedback?(bzbarsky) → review?(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 on attachment 8933461 [details]
Bug 1420229 - Create a new components.css file and load it as a UA stylesheet

r=me
Attachment #8933461 - Flags: review?(bzbarsky) → review+
Sorry for the review spam, the r+ didn't get copied over into mozreview after I added you to the commit message
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
Attachment #8933461 - Flags: review?(bzbarsky) → review+
Assignee: nobody → bgrinstead
Can we land this?  thanks
Flags: needinfo?(bgrinstead)
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.
Flags: needinfo?(bgrinstead)
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
See Also: → 1428164
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).
AllowXULXBL() can be true in the content process in "remote XUL" cases, if that helps.
(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.
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.
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.
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
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.
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.
Flags: needinfo?(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
Flags: needinfo?(bdahl)
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
(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?
Ongoing try push with headless set in the xpcshell.ini for formautofill: https://treeherder.mozilla.org/#/jobs?repo=try&revision=442f4b06dfe8549610a2df47e73e1f64bb323743
(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 on attachment 8940893 [details]
Bug 1420229 - Run formautofill xpcshell tests in headless mode;

https://reviewboard.mozilla.org/r/211154/#review217204
Attachment #8940893 - Flags: review?(bdahl) → review+
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.
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.
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
Depends on: 1429630
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.
See Also: → 1429857
Keywords: regression
Summary: Bookmarks list appears double-spaced → 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
Blocks: 1429464
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.
Depends on: 1409672
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?
Attachment #8933461 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bzbarsky)
No longer depends on: 1429630
(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.
(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.
Flags: needinfo?(bgrinstead)
Thank you Brian. I think I know now how this works.
(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 on attachment 8944865 [details]
Bug 1420229 - Backed out changeset 05bab8e59cd1 (bug 1429857);

https://reviewboard.mozilla.org/r/215026/#review220636
Attachment #8944865 - Flags: review?(bgrinstead) → review+
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.
Flags: needinfo?(bzbarsky)
Oh, and clearly if we can remove the special cases, so much the better.
Pushed by bgrinstead@mozilla.com:
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
See Also: → 1432851
(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.
https://hg.mozilla.org/mozilla-central/rev/23d4aff26260
https://hg.mozilla.org/mozilla-central/rev/2a3561bcb468
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Can/should we uplift this to beta to fix the regression in 59?
Flags: needinfo?(bgrinstead)
(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.
(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.
status-firefox59: affected → unaffected
Flags: needinfo?(bgrinstead)
No longer blocks: 1429464
Depends on: 1457907
No longer depends on: 1458426
Depends on: 1458426
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: