Closed Bug 1291515 Opened 8 years ago Closed 7 years ago

disable <style scoped> in content documents

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: emilio, Assigned: heycam)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

Bug 1291515 - Part 1: Introduce a pref for <style scoped>.
59 bytes, text/x-review-board-request
dbaron
: review+
bholley
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1291515 - Part 2: Disable pref for <style scoped>.
59 bytes, text/x-review-board-request
dbaron
: review+
jcristau
: approval-mozilla-beta+
Details 
It has recently removed from the HTML spec[1], and no other vendors plan to implement this[2].

This would allow make more performant style scoping for shadow trees (using the shadow root pointer instead of the set of flags we now have in the element), recover two BoolFlags for nsINode, and ease the implementation details for stylo too.

[1]: https://github.com/whatwg/html/issues/552
[2]: https://lists.w3.org/Archives/Public/www-style/2016Jun/0010.html
What do you think of this Cameron? Happy to give it a shot.
Flags: needinfo?(cam)
To me, Web Components seems kind of heavyweight for restricting styles to a subtree, so I'm not sure I buy the arguments in the issue that authors should just use those.  But I'm sympathetic to the argument that nobody else wants to implement <style scoped>, so I lean towards removing support for it.

In terms of blocking stylo work, I think we can just ignore the scoped="" attribute when in a document that IsStyledByServo().  If it becomes critical to recover some nsINode flags, we can prioritise this.

But I'll defer to David for whether we should remove the feature.
Flags: needinfo?(cam) → needinfo?(dbaron)
I'd probably prefer to wait a bit longer to see how things shake out and get a bit more time for feedback on the spec's removal decision.
Flags: needinfo?(dbaron)
I would wait before removing this. I assume once shadow DOM is implemented everywhere and the styling it requires is supported as well, having this in all the browsers might not be so hard, as far as I understand.
This just came to my attention, since Snooze Tabs uses it when injecting its bar at the top of the page, and I was looking into a way to do the same thing. Added dev-doc-needed so we know to watch this if anything happens with it.
(In reply to Eric Shepherd [:sheppy] from comment #6)
> This just came to my attention, since Snooze Tabs uses it when injecting its
> bar at the top of the page, and I was looking into a way to do the same
> thing.

Does this mean this bug should be flagged for addon-compat as well?
Are we leaning to remove the support of <style scope>? Otherwise, we'll need to implement it for stylo in bug 1345702.
See Also: → 1345702
Over to dbaron/heycam for a call on this one.
Flags: needinfo?(dbaron)
Flags: needinfo?(cam)
(Also, TY: Can you estimate how much work this is, now that we have the relevant machinery for XBL stylesheets?)
Flags: needinfo?(tlin)
(Having the answer to comment 10 would be useful for answering comment 9.)
Sorry for the terrible lag.  Per discussion with heycam in person, I don't feel we can reuse the machinery for XBL stylesheets to implement <style scoped> easily. XBL stylesheets are processed in nsXBLPrototypeResources, and be queried in selector matching and cascading if the element or its binding parent has any bindings, whereas <style scoped> stylesheets probably need to be added to the main style set of the document during parsing, so we'll need to reinvent something on servo to extract the correct rules.

I feel this is trickier than implementing XBL stylesheets. I took 4~5 weeks to implement XBL stylesheets, so this might takes longer for me (after I sort out all the remaining follow-ups of XBL stuff).
Flags: needinfo?(tlin)
I think the biggest argument at this point for removing it, regardless of the complexity (which I also think it's high), is that no other vendor plans to implement this.
Ok, that sounds to me like we should remove it. dbaron?
OK, I guess I'm OK with removing it, then.

(We may need to reconsider readding it in the future if we or others can talk other browsers into adding it.)
Flags: needinfo?(dbaron)
In the meeting today we decided that we should pref it off on beta 55 to give us maximum lead time to determine web compat issues. Cameron is going to take this.
Assignee: nobody → cam
Blocks: stylo-release
Priority: -- → P1
Summary: Consider removing <style scoped> → Try removing <style scoped>
Keywords: site-compat
Turns out we don't have a pref for <style scoped> itself, only for the :scope rule.  I'll try to make a minimal patch that introduces a pref and disables <style scoped> (so that if we do have to re-enable it later in the Beta cycle, that could be done just with a pref flip).
Flags: needinfo?(cam)
Summary: Try removing <style scoped> → disable <style scoped> in content documents
See Also: → 1375804
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

https://reviewboard.mozilla.org/r/151138/#review157386

r=dbaron with the following two things

::: commit-message-416c3:3
(Diff revision 1)
> +Bug 1291515 - Part 1: Introduce a pref for <style scoped>. r?dbaron
> +
> +MozReview-Commit-ID: 1J9IvPrC0xh

I think you also need to change the function IsScopedStyleElement in nsStyleLinkElement.cpp.

::: modules/libpref/init/all.js:2850
(Diff revision 1)
>  // Are "-webkit-{min|max}-device-pixel-ratio" media queries supported?
>  // (Note: this pref has no effect if the master 'layout.css.prefixes.webkit'
>  // pref is set to false.)
>  pref("layout.css.prefixes.device-pixel-ratio-webkit", false);
>  
> +// Is support for <style scoped> enabled in content documents?

Please document also that this disables exposing the DOM APIs in both content and chrome.
Attachment #8879772 - Flags: review?(dbaron) → review+
Comment on attachment 8879773 [details]
Bug 1291515 - Part 2: Disable pref for <style scoped>.

https://reviewboard.mozilla.org/r/151140/#review157388
Attachment #8879773 - Flags: review?(dbaron) → review+
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

https://reviewboard.mozilla.org/r/151138/#review157386

> I think you also need to change the function IsScopedStyleElement in nsStyleLinkElement.cpp.

I think this is OK, since we only ever call it when we are updating a style sheet that we previously determined was a scoped style sheet.  But it's probably safer to update it, in case we call it elsewhere later.
Part 2 needs DOM peer review.
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

https://reviewboard.mozilla.org/r/151138/#review157484

rs=me on the webidl bits. Thanks for taking care of this.
Attachment #8879772 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e8b918dc410
Part 1: Introduce a pref for <style scoped>. r=bholley,dbaron
https://hg.mozilla.org/integration/autoland/rev/4fac24cc0437
Part 2: Disable pref for <style scoped>. r=dbaron
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

(This applies to part 2, too.)

Approval Request Comment
[Feature/Bug causing the regression]: -
[User impact if declined]: We'll have less time to determine whether there are Web compatibility issues with disabling <style scoped> in time for Stylo to ship.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, just landed on autoland
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: The change is reasonable small.  It introduces a pref, and checks it in a few places.
[String changes made/needed]: none
Attachment #8879772 - Flags: approval-mozilla-beta?
The failures are probably due to the reftests in e.g. layout/reftests/css-display/ that set the pref on temporarily, but by the time the document is deleted, the pref is off again.
Flags: needinfo?(cam)
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

Can you take another look at this?  I've added state to nsIDocument to record the decision about whether scoped styles are enabled for the document or not.  (That seemed safer than adjusting the logic in nsStyleLinkElement/HTMLStyleElement to support enabling and disabling while a document is loaded.)
Attachment #8879772 - Flags: review+ → review?(dbaron)
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

https://reviewboard.mozilla.org/r/151138/#review157500
Attachment #8879772 - Flags: review?(dbaron) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90018589717e
Part 1: Introduce a pref for <style scoped>. r=bholley,dbaron
https://hg.mozilla.org/integration/autoland/rev/9ed72cb74889
Part 2: Disable pref for <style scoped>. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/90018589717e
https://hg.mozilla.org/mozilla-central/rev/9ed72cb74889
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Can we find out about webcompat issues some other way?  E.g. with telemetry or shipping something like this to a subset of our release population via a system add-on or shield?  Although I guess we can always flip the pref back if it turns out to cause issues in beta in the next month.
Rather than whether there is a significant number of uses of it (which I think is a question Telemetry is well placed to answer), I think we want to know whether there is significant breakage from disabling <style scoped>.  So I feel like getting that through bug reports is going to give us a better indication of whether we need to scramble to implement <style scoped> in Stylo.  Getting this information as soon as we can is important.  And, as you say, flipping the pref back on again is an easy thing to do.  So I'd like to confirm the beta uplift request.
Flags: needinfo?(jcristau)
No other browsers support <style scoped> so the risk should be low. I'll post a site compatibility note anyway though.
Comment on attachment 8879772 [details]
Bug 1291515 - Part 1: Introduce a pref for <style scoped>.

alright, let's get this in 55.0b7
Flags: needinfo?(jcristau)
Attachment #8879772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8879773 [details]
Bug 1291515 - Part 2: Disable pref for <style scoped>.

[Triage Comment]
Attachment #8879773 - Flags: approval-mozilla-beta+
setting flags :)
status-firefox55: --- → fixed
(In reply to Cameron McCormack (:heycam) from comment #30)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no, just landed on autoland
> [Needs manual test from QE? If yes, steps to reproduce]: none

Setting qe-verify- based on Cameron's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
This breaks the reader mode button on Android.
Depends on: 1377144
Those docs look good, thanks!
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #50)
> I've had a go at documenting this; see the compat tables on:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLStyleElement/
> scoped#Browser_compatibility
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/
> style#Browser_compatibility
> 
> and the note on the Fx56 rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/56#HTML_2
> 
> Let me know if this looks OK. Thanks!

This change was uplifted to fx55 (comment 46), shouldn't mdn reflect that?
Flags: needinfo?(cmills)
(In reply to Julien Cristau [:jcristau] from comment #53)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #50)
> > I've had a go at documenting this; see the compat tables on:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/HTMLStyleElement/
> > scoped#Browser_compatibility
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/
> > style#Browser_compatibility
> > 
> > and the note on the Fx56 rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/56#HTML_2
> > 
> > Let me know if this looks OK. Thanks!
> 
> This change was uplifted to fx55 (comment 46), shouldn't mdn reflect that?

Ooops, I missed this detail - thanks for pointing it out. I've updated the docs to state Fx 55 for this change.
Flags: needinfo?(cmills)
Depends on: 1388682
Depends on: 1389870
Regressions: 1542645
No longer regressions: 1542645
See Also: → 1542645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: