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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jmjjeffery, Assigned: bgrins)

Tracking

Trunk
Firefox 60
x86_64
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

Details

Attachments

(10 attachments, 1 obsolete attachment)

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)

Comment 1

2 years ago
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)
Assignee

Comment 2

2 years ago
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
Assignee

Comment 5

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

Comment 6

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

Comment 7

2 years ago
Posted 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

Comment 8

2 years ago
Posted image Bookmarks menu
I can confirm it on Windows 7 as well.  Attachment shows Nov 25 nightly vs Nov 19 nightly.
Assignee

Comment 9

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

Comment 10

2 years ago
Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on DevEdition
Assignee

Comment 11

2 years ago
Rules applied to the #BMB_bookmarksPopup element copied out of the Browser Toolbox on Nightly
Assignee

Comment 12

2 years ago
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)
Assignee

Comment 13

2 years ago
Posted image win7-58-59-comparison.png (obsolete) —
screenshot of what I'm seeing on Win7, highlighting one of the rules that has flipped order
Assignee

Comment 14

2 years ago
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)
Assignee

Comment 16

2 years ago
(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)
Comment hidden (mozreview-request)
Assignee

Comment 19

2 years ago
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.
Assignee

Comment 20

2 years ago
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)
Assignee

Comment 21

2 years ago
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?
Assignee

Updated

2 years ago
Blocks: 1422386
Assignee

Comment 22

2 years ago
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+
Comment hidden (mozreview-request)
Assignee

Comment 26

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

Comment 27

2 years ago
mozreview-review
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)
Assignee

Comment 29

2 years ago
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)
Assignee

Comment 30

2 years ago
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
Assignee

Updated

2 years ago
See Also: → 1428164
Assignee

Comment 31

2 years ago
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.
Assignee

Comment 33

2 years ago
(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.
Assignee

Comment 34

2 years ago
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.
Assignee

Comment 35

2 years ago
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.
Assignee

Comment 36

2 years ago
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
Assignee

Comment 37

2 years ago
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.
Assignee

Comment 38

2 years ago
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)
Assignee

Comment 40

Last year
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
Assignee

Comment 41

Last year
(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?
Assignee

Comment 42

Last year
Ongoing try push with headless set in the xpcshell.ini for formautofill: https://treeherder.mozilla.org/#/jobs?repo=try&revision=442f4b06dfe8549610a2df47e73e1f64bb323743
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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 47

Last year
mozreview-review
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+
Assignee

Comment 48

Last year
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.
Assignee

Comment 49

Last year
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.
Assignee

Comment 50

Last year
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
Assignee

Updated

Last year
Depends on: 1429630
Assignee

Comment 52

Last year
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.
Assignee

Updated

Last year
See Also: → 1429857
Assignee

Updated

Last year
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
Assignee

Updated

Last year
Blocks: 1429464
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 55

Last year
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 hidden (mozreview-request)

Comment 57

Last year
mozreview-review
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)
Assignee

Comment 59

Last year
(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)
Assignee

Updated

Last year
No longer depends on: 1429630
Assignee

Comment 60

Last year
(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.
Assignee

Comment 61

Last year
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 67

Last year
(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.
Assignee

Comment 68

Last year
mozreview-review
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.

Comment 71

Last year
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
Assignee

Updated

Last year
See Also: → 1432851
Assignee

Comment 72

Last year
(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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/23d4aff26260
https://hg.mozilla.org/mozilla-central/rev/2a3561bcb468
Status: NEW → RESOLVED
Closed: Last year
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.
Assignee

Comment 76

Last year
(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.
Flags: needinfo?(bgrinstead)

Updated

Last year
No longer blocks: 1429464

Updated

Last year
Depends on: 1457907
Assignee

Updated

Last year
No longer depends on: 1458426
Assignee

Updated

Last year
Depends on: 1458426
You need to log in before you can comment on or make changes to this bug.