Stop loading full xul.css to non-XUL document

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Blocks 2 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We currently load the full xul.css in HTML documents when there is a <xul:videocontrols> (more specifically, when additional XUL elements are found within them).

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/xul/nsXULElement.cpp#785-801

I would like to also load components.css there so that we can move a few <resources> away from XBL bindings into it. This is needed to make sure touchControls continue to work & styled correctly. This does not affact desktop video controls because it is implemented in HTML.
Which XBL bindings get used by touchControls content?
Also, maybe we should update the NS_ASSERTION to ensure we are on mobile only just to make sure we don't somehow accidentally start loading it in desktop.
The alternative would be to never move any sheets to components.css if the binding is found to be loaded by any element within touchControls. If we want to go this route, the list of bindings (and potentially future custom elements) should be listed and annotated in the codebase.
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Which XBL bindings get used by touchControls content?

I don't have a definite list, from the markup I can see at least button, progressmeter, label, scale, ...

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Also, maybe we should update the NS_ASSERTION to ensure we are on mobile
> only just to make sure we don't somehow accidentally start loading it in
> desktop.

I agree.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > Which XBL bindings get used by touchControls content?
> 
> I don't have a definite list, from the markup I can see at least button,
> progressmeter, label, scale, ...

If we could start to trim the list down by doing things like switching from <label> / <button> to their HTML alternatives and possibly <scale> to <input type="number"> that would be a help going forward (both for what CSS needs to be loaded and for in-content conversions using something like Bug 1431255). Do you think that'd be viable - or would we need to rewrite the whole UI in one go?
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Do you think that'd be viable - or would we need to rewrite the
> whole UI in one go?

It's going to be tedious, based on my feeling from the experience of bug 1271765. There are problems specifically around cancelable HTML DOM Events being leaked to the content, though I assume we can always mirror what's done on the desktop control.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > Do you think that'd be viable - or would we need to rewrite the
> > whole UI in one go?
> 
> It's going to be tedious, based on my feeling from the experience of bug
> 1271765. There are problems specifically around cancelable HTML DOM Events
> being leaked to the content, though I assume we can always mirror what's
> done on the desktop control.

I think we are going to want to do this eventually anyway (ultimately I'd prefer to not run any XUL in the content process), but it sounds like this is a separate project on it's own. I wonder if the UA Shadow Root proposal in Bug 1431255 will have an effect on this DOM Event leaking thing.

But regardless: since suppressChangeEvent will be the only instance of <scale> once we sort out what to do with the Canvas Debugger instance, I think we could fold this into the scale binding (i.e. don't even send an event out to the document).
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #6)
> > (In reply to Brian Grinstead [:bgrins] from comment #5)
> > > Do you think that'd be viable - or would we need to rewrite the
> > > whole UI in one go?
> > 
> > It's going to be tedious, based on my feeling from the experience of bug
> > 1271765. There are problems specifically around cancelable HTML DOM Events
> > being leaked to the content, though I assume we can always mirror what's
> > done on the desktop control.
> 
> I think we are going to want to do this eventually anyway (ultimately I'd
> prefer to not run any XUL in the content process), but it sounds like this
> is a separate project on it's own. I wonder if the UA Shadow Root proposal
> in Bug 1431255 will have an effect on this DOM Event leaking thing.
> 
> But regardless: since suppressChangeEvent will be the only instance of
> <scale> once we sort out what to do with the Canvas Debugger instance, I
> think we could fold this into the scale binding (i.e. don't even send an
> event out to the document).

Now that I think about it more, with the way we are planning to handle in-content widgets in Bug 1431255 I think it wouldn't work to include our Custom Elements inside of one of the widgets (since the XUL Custom Elements will be defined only in our XUL documents and not in-content). So bindings that currently run in-content, and use <content> that refer to other elements with XBL bindings, I think we'll have to switch to plain HTML/XUL elements (and not elements that used to be XBL and get converted to Custom Elements).

IOW: I think we should get start to break down the work required and get bugs on file to convert touchControls to HTML.

Comment 10

a year ago
mozreview-review
Comment on attachment 8957332 [details]
Bug 1444193 - Load components.css into non-XUL document also when we load xul.css

https://reviewboard.mozilla.org/r/226240/#review232174

The main behavior change here is that we'll load more styles than we used to in this case (since components.css will grow as we begin to land Custom Elements in tree, and we wouldn't have loaded those XBL styles previously unless if they were used by touchControls). I think that's OK, given that:

1) This will only happen in pages on mobile with videocontrols
2) I think we'll need to re-write this content to be HTML only anyway as per Comment 9

But, I'd like to hear Neil's opinion.
Attachment #8957332 - Flags: review?(bgrinstead) → review+
I've filed bug 1444489 to gather information on overall approach, as this bug only deals with stylesheets for the in-content bindings.
Should we close this bug, given that the approach in Bug 1444489 will mean we shouldn't ever need to load xul.css or components.css in content?
Flags: needinfo?(timdream)
Yes, I believe xul.css is no longer needed.

I would instead like to remove the ability of loading full xul.css and keep the assertions in this bug.
Flags: needinfo?(timdream)
Summary: Load components.css into non-XUL document also when we load xul.css → Stop loading full xul.css to non-XUL document
Comment hidden (mozreview-request)
Attachment #8957332 - Attachment is obsolete: true
Attachment #8957332 - Flags: review?(enndeakin)
This can't land until bug 1444489 lands.
Depends on: 1444489
See Also: 1444489

Comment 16

a year ago
mozreview-review
Comment on attachment 8959036 [details]
Bug 1444193 - Remove the ability to load full xul.css in non-XUL document.

https://reviewboard.mozilla.org/r/227910/#review233878
Attachment #8959036 - Flags: review?(enndeakin) → review+
Priority: -- → P2

Comment 17

a year ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d13b21732722
Remove the ability to load full xul.css in non-XUL document. r=enndeakin+6102

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d13b21732722
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 19

10 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e60e3fca0f3
followup: Remove comment which is no longer accurate. r=me
You need to log in before you can comment on or make changes to this bug.