Bug 49142

Summary: [Meta] Implement HTML5 style scoped attribute
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: CSSAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED WONTFIX    
Severity: Normal CC: annulen, arv, bronislav.klucka, dominicc, eoconnor, hyatt, ian, jackalmage, kangax, kennyluck, koivisto, mathias, mihnea, mike, mitz, morrita, neonstalwart, peter, piet, rolandsteiner, simon.fraser, syoichi, tabatkins, tc, webkit-bug-importer, webkit.bugzilla, webkit.org, webmaster
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 61264, 67718, 67720, 67790, 72461, 72462, 72472, 72480, 72482, 72848, 73190, 73192, 73893, 77527, 77853, 79074, 79075, 79077, 80389    
Bug Blocks: 32934, 46590, 59827, 82340    
Attachments:
Description Flags
work-in-progress
none
work-in-progress (cleaned)
none
work-in-progress (scoped)
none
patch, full HTML5 version
none
patch, HTML5 version, cleaned
none
Work-in-progress
none
Patch
none
work-in-progress before break-up
none
work-in-progress (fix conflicts) none

Description Dimitri Glazkov (Google) 2010-11-07 07:45:57 PST
Per http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#attr-style-scoped

I couldn't find an existing bug.
Comment 1 Roland Steiner 2011-05-20 02:25:58 PDT
I started looking into this. My angle of attack would be:

1.) move m_elemStyle, m_mappedElementStyle, m_styleSheets, m_styleSheetCandidateNodes, and m_styleSelector from Document into TreeScope, i.e., having a style selector for each scope. m_pageUserSheet and m_pageGroupuserSheets remain on Document.

2.) change the code to call style selection on the TreeScope rather than the document.

3.) Adapt JS GC code as necessary

Now, before starting to actually implement this (and potentially wasting a lot of time), I wanted to confirm that this is a reasonable approach. Any remarks or suggestions?
Comment 2 Hajime Morrita 2011-05-22 19:19:15 PDT
Having these member variables to TreeScope means only subclassses of TreeScope can have 
@scope attribute, that wouldn't satisfy the spec. Which allows us to put @scope anywhere.

I think what we should have seems separate "StyleScope" class or something like that.
It could be in ElementRareData.
Comment 3 Ian 'Hixie' Hickson 2011-05-22 19:42:45 PDT
I may be misunderstanding, but scoped="" in HTML only has an effect on <style> elements (which may themselves then affect any descendants of the <style> element's parent).

(Furthermore, it's only allowed on <style> elements in <body> in certain places. But that is irrelevant for Web browser implementors.)
Comment 4 Hajime Morrita 2011-05-22 19:51:09 PDT
(In reply to comment #3)
> I may be misunderstanding, but scoped="" in HTML only has an effect on <style> elements (which may themselves then affect any descendants of the <style> element's parent).
> 
> (Furthermore, it's only allowed on <style> elements in <body> in certain places. But that is irrelevant for Web browser implementors.)
You are right. I confused.
The point is that the style is applied the subtree of any <style> hosting parent element.
(That's what @scoped is for.)
Comment 5 Roland Steiner 2011-05-22 20:58:14 PDT
(In reply to comment #2)
> I think what we should have seems separate "StyleScope" class or something like that.
> It could be in ElementRareData.

Putting this into a separate class would definitely be a good idea to future-proof the code. 

However, for the time being I would propose to only implement this for shadow roots (i.e., the TreeScope class) for the following reason:

Currently, an element asks the Document object to resolve its style. Changing this to TreeScope is no problem. OTOH, having any arbitrary element be able to host a scoped <style> would mean either

a) climbing the tree for style resolution rather than O(1) access to Document/TreeScope, or
b) adding a separate style scope concept, requiring a separate style scope pointer on Element (memory overhead + updating overhead).
Comment 6 Hajime Morrita 2011-05-22 21:26:06 PDT
> However, for the time being I would propose to only implement this for shadow roots (i.e., the TreeScope class) for the following reason:
Sounds reasonable starting point. And it should be done as a sub-bug of this, as you might be planning.

By the way, introducing a separate class (that is hold by TreeScope) is possibly good even in this case.
Let's take this as a refactoring opportunity ;-)
Comment 7 Dimitri Glazkov (Google) 2011-05-23 13:46:21 PDT
Just to recap the latest thinking:

The easy-peasy way to implement this is to treat selectors in scoped styles as if they were prefixed with some unique id that matches only the parent node of the scoped style element.

So this makes the work as follows:
* define a new type of unique identifier, build a way to maintain a map of them at Document
* add plumbing to add/remove ids on the parent of scoped style element
* make CSS match this unique id on an element
* modify CSS selector parser to prefix each scoped style selector with a sub-selector that matches this unique id
* make sure changing scoped attribute causes all stylesheets to be reparsed.
* write nice tests

I don't think TreeScope really comes into play until we start dealing with shadow DOM. Once there, we should abstract out the notion of id/className cache in CSSStyleSelector and instead of using document(), use treeScope().

Does this make any sense?
Comment 8 Ian 'Hixie' Hickson 2011-05-23 14:00:13 PDT
Note that http://www.whatwg.org/specs/web-apps/current-work/complete.html#attr-style-scoped also says:

"For scoped CSS resources, the effect of @-rules must be scoped to the scoped sheet and its subresources, even if the @-rule in question would ordinarily apply to all style sheets that affect the Document. Any '@page' rules in scoped CSS resources must be ignored."

...which likely will complicate this a little (sorry).
Comment 9 Roland Steiner 2011-05-23 22:43:30 PDT
(In reply to comment #7)
> The easy-peasy way to implement this is to treat selectors in scoped styles as if they were prefixed with some unique id that matches only the parent node of the scoped style element.

Interesting approach, but it strikes me as a bit hacky. It also means that that unique artificial id mustn't take part in the normal specificity resolution. If I understand correctly, it would also meant that we have to update the Document-wide style rule set whenever an element is inserted or removed from the DOM that has a scoped style associated in its shadow DOM.

I would have intended to have a separate CSSStyleSelector on every TreeScope that builds the combined style rule set either on the fly, or on instantiation (This is probably the same overhead as above for instantiation, but there is no cost on deletion, as the Document style data itself isn't touched). It should also be more flexible to tackle issues such as the one mentioned by Hixie in comment #8.

(In reply to comment #8)

@Hixie: Speaking of specificity: I would have assumed that a scoped style sheet takes precedence of "global" styles, but that isn't mentioned in the spec. So am I correct to assume that the following is true?

<style>
    p p { color: red }
</style>
<p>
    <style scoped="scoped">p { color: green}</style>
    <p>Still red</p>
</p>
 
I.e., the scoped style doesn't take, because the global style is more specific (?).
Comment 10 Dimitri Glazkov (Google) 2011-05-24 07:38:19 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > The easy-peasy way to implement this is to treat selectors in scoped styles as if they were prefixed with some unique id that matches only the parent node of the scoped style element.
> 
> Interesting approach, but it strikes me as a bit hacky.

I don't see anything hacky. To me, it seems like a natural way to view scoped stylesheets.

>It also means that that unique artificial id mustn't take part in the normal specificity resolution.

Can you explain a bit more here? Specificity is just an order in which the rules are applied. I don't understand how this approach would complicate anything here.

>If I understand correctly, it would also meant that we have to update the Document-wide style rule set whenever an element is inserted or removed from the DOM that has a scoped style associated in its shadow DOM.

I think that's something that needs to be tackled orthogonally. This problem is still in place with all other stylesheets removal/addition.

> 
> I would have intended to have a separate CSSStyleSelector on every TreeScope that builds the combined style rule set either on the fly, or on instantiation (This is probably the same overhead as above for instantiation, but there is no cost on deletion, as the Document style data itself isn't touched).

I would caution that this is some of the most performance-sensitive code in WebKit. If you're thinking of a different approach, I would like to first understand how you will accomplish this.

> It should also be more flexible to tackle issues such as the one mentioned by Hixie in comment #8.
> 
> (In reply to comment #8)
> 
> @Hixie: Speaking of specificity: I would have assumed that a scoped style sheet takes precedence of "global" styles, but that isn't mentioned in the spec. So am I correct to assume that the following is true?
> 
> <style>
>     p p { color: red }
> </style>
> <p>
>     <style scoped="scoped">p { color: green}</style>
>     <p>Still red</p>
> </p>
> 
> I.e., the scoped style doesn't take, because the global style is more specific (?).

It should work just like it would without the scoped attribute.
Comment 11 Roland Steiner 2011-05-24 20:00:13 PDT
(In reply to comment #10)

Disclaimer: Note that below I don't want to argue against your approach (it indeed is probably simpler than mine) - just trying to understand the implications!

> (In reply to comment #9)
> >It also means that that unique artificial id mustn't take part in the normal specificity resolution.
> 
> Can you explain a bit more here? Specificity is just an order in which the rules are applied. I don't understand how this approach would complicate anything here.

If we just rewrite a scoped selector "span" to "#<ID-of-scope-element> span" then the specificity would change, affecting style resolution later on. E.g.

<style>
    .red { color: red }
</style>
<p>
    <style scoped>
        span { color: green }
    </style>
    <span class="red">Should be red</span>
</p>

Without the rewrite, the ".red" selector has higher specificity, and the text should be red. With the rewrite, the scoped "#<id-of-p> span" selector has higher specificity, and the text would be green.

Also, the scope element might already have an ID of its own that can't be overwritten. Although we may be able to re-use that instead of generating a new one, that is another issue that needs to be tracked.

> >If I understand correctly, it would also meant that we have to update the Document-wide style rule set whenever an element is inserted or removed from the DOM that has a scoped style associated in its shadow DOM.
>
>I think that's something that needs to be tackled orthogonally. This problem is still in place with all other stylesheets removal/addition.

But inserting or removing an <x-foo> that happens to have a scoped style declaration in its shadow DOM is likely a far more common occurrence than specifically adding/removing a style sheet to/from the Document. Esp. note that it doesn't have to be the <x-foo> directly - attaching/detaching any subtree that contains an <x-foo> would need to trigger the update, as would adding/removing an <x-bar> that happens to be composed of an <x-foo>. 

Also, I would argue that adding/removing style-sheets to/from the Document is understood to have performance implications, while adding/removing <x-...> elements may not, esp. if the <x-foo> comes from some widget library.
Comment 12 Dimitri Glazkov (Google) 2011-05-25 08:46:18 PDT
> Also, the scope element might already have an ID of its own that can't be overwritten. Although we may be able to re-use that instead of generating a new one, that is another issue that needs to be tracked.

Ah, I think I understand the source of confusion now. The scoped-id is not going to interfere with element id. It's a parallel structure that's like id.

> > >If I understand correctly, it would also meant that we have to update the Document-wide style rule set whenever an element is inserted or removed from the DOM that has a scoped style associated in its shadow DOM.
> >
> >I think that's something that needs to be tackled orthogonally. This problem is still in place with all other stylesheets removal/addition.
> 
> But inserting or removing an <x-foo> that happens to have a scoped style declaration in its shadow DOM is likely a far more common occurrence than specifically adding/removing a style sheet to/from the Document. Esp. note that it doesn't have to be the <x-foo> directly - attaching/detaching any subtree that contains an <x-foo> would need to trigger the update, as would adding/removing an <x-bar> that happens to be composed of an <x-foo>. 
> 
> Also, I would argue that adding/removing style-sheets to/from the Document is understood to have performance implications, while adding/removing <x-...> elements may not, esp. if the <x-foo> comes from some widget library.

This is a good point. The approach with scoped-id creates no room for reuse optimizations. It is the approach that we know we'll have to re-evaluate once we get to that point. So here's what we need to determine:

1) Overall design of the approach where CSSStyleSelector is TreeScope-based, and the rules are coalesced using TreeScope hierarchy.

2) Whether this design's complexity and performance impact outweighs the quick gains we can make with scoped-id approach.

I can see that this is a classic pay-now or pay-later decision.

Also, cc-ing Tab, because he had bright ideas on scoped stylesheet specificity.
Comment 13 Ian 'Hixie' Hickson 2011-05-25 12:03:08 PDT
Re comment 9, this feature is not intended to affect the cascade in any way other than changing which rules are in scope. The last sentence of comment 10 is correct. HTH.
Comment 14 Roland Steiner 2011-05-25 22:11:45 PDT
(In reply to comment #13)
> Re comment 9, this feature is not intended to affect the cascade in any way other than changing which rules are in scope. The last sentence of comment 10 is correct. HTH.

Heh - this comment actually got me more confused ;). The spec says "... styles are intended just for the subtree rooted at the style element's parent element, as opposed to the whole Document."

I.e., I assumed selectors of scoped style-sheets match up to that style-sheet's parent, but not up to the document root. That would be another way (besides being in effect or not) how style resolution for scoped style rules would be different from document-wide style rules. E.g.

<div>
    <p>
        <style scoped>
            div span { color: red }
        </style>
        <span>Not actually red</span>
    </p>
</div>

In this case, the style would not match, because selector matching stops at <p>, while in the case where the only thing that changes is whether the style rules are in effect or not, it _would_ match.

Now the obvious follow-up question is whether a selector "p span" would match. From a strict reading I would guess yes, but that might be counter-intuitive.
Comment 15 Ian 'Hixie' Hickson 2011-05-25 22:32:48 PDT
> <div>
>     <p>
>         <style scoped>
>             div span { color: red }
>         </style>
>         <span>Not actually red</span>
>     </p>
> </div>

Well this particular example is bad for unrelated reasons -- <style> isn't allowed in <p>. But ignoring that for a second, the above will be red, yes. The _only_ thing that this does is change which style sheets apply. It doesn't affect the cascade, it doesn't affect selector matching, it doesn't affect specificity, it doesn't affect inheritance, it doesn't affect anything else.

It would be helpful for me if you could quote the part of the specification that led you to conclude otherwise, so that I could fix it.
Comment 16 Roland Steiner 2011-05-25 22:52:50 PDT
(In reply to comment #12)
> This is a good point. The approach with scoped-id creates no room for reuse optimizations. 

Actually, thinking a bit more about the issue (and maybe this is what you intended all along, and I just misunderstood you): 

If we generate the scoped-id from the MD5 hash (or similar) of the actual contents of the style sheet, then the need to constantly update the rule set within Document would vanish. An element matches the (prefixed to every rule) scoped-id iff it has a <style scoped> child whose contents would generate that id. This obviously cached, and wouldn't even needed to be re-calculated when instantiating shadow trees - the same style sheet replicated in various shadow DOMs would not create additional rule entries. We can add those entries when parsing the <template>, and adding or removing <x-foo> elements would be a no-op with regard to scoped style sheet updates.
It should also beautifully work with <style scoped> elements anywhere in the DOM (i.e., wouldn't need to be restricted to TreeScopes).

Furthermore, we would technically not need to ever remove rules from the Document (rules that reference an obsolete MD5 hash scope-id would just never match), just add new rules if a new unknown <style scoped> comes along. CSSOM could wreak havoc here, but for this we could add periodic rules garbage collection.
Comment 17 Roland Steiner 2011-05-25 23:21:07 PDT
(In reply to comment #15)
> Well this particular example is bad for unrelated reasons -- <style> isn't allowed in <p>. But ignoring that for a second, the above will be red, yes. The _only_ thing that this does is change which style sheets apply. It doesn't affect the cascade, it doesn't affect selector matching, it doesn't affect specificity, it doesn't affect inheritance, it doesn't affect anything else.

This is probably not the right forum for this (and likely repeating previous arguments), but that would mean that a scoped style applies to basically everything under that style's parent if any ancestor happens to also match that style's selector - I would argue that that's counterintuitive and diminishes the usefulness of scoped style sheets, again requiring very specific classes, etc. to avoid that. It would also preclude "recursive" use of the same scoped stylesheet - esp. in the case of shadow DOM/XBL one couldn't use both scoped stylesheets and @allow-selectors-through for recursive templates.

For the issue of how to implement <style scoped>, this means that a scoped-id implementation cannot just apply normal selector matching, but the scoped-id must be matched separately after regular selector matching as "any ancestor has a scoped style sheet with that id".

> It would be helpful for me if you could quote the part of the specification that led you to conclude otherwise, so that I could fix it.

From http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#the-style-element:

"The scoped attribute is a boolean attribute. If set, it indicates that the styles are intended just for the subtree rooted at the style element's parent element, as opposed to the whole Document."

"If the scoped attribute is present, then the user agent must apply the specified style information only to the style element's parent element (if any), and that element's child nodes. Otherwise, the specified styles must, if applied, be applied to the entire document."

I understood "styles are intended just for..." and "specified style information" to include the selectors, compounded by the "otherwise" statement (and probably fueled by some preconceived expectations as explained above).
Comment 18 Ian 'Hixie' Hickson 2011-05-26 00:35:08 PDT
For bindings there's a bunch of other stuff that has to happen (e.g. XBL gives the author control over whether the styles apply to the bound element's explicit descendants, whether selectors can cross the shadow boundary, and so on), which don't really apply here. <style scoped> is intended for syndication cases, not cases where you'd end up with similar structure inside and out, or cases where you'd end up with nesting. But let's take the design discussion to the webkit-dev or whatwg lists.
Comment 19 Dimitri Glazkov (Google) 2011-05-26 09:09:08 PDT
I agree with Hixie on design discussion. On implmentation: in the interest of making progress and understanding the problem better by solving at least some of it, let's pick the simplest possible design and go with it. Nothing is in stone, we can change this later.
Comment 20 Roland Steiner 2011-05-31 01:31:10 PDT
(In reply to comment #19)
> ... let's pick the simplest possible design and go with it.

In this case I would suggest a hybrid of your original suggestion and my last one:

1.) create a scope-ID by simply having an incrementing counter
2.) automatically add all scoped stylesheets in <template>s
3.) add an additional flag for scoped style-sheets that are just instances of one in a <template>

Scoped stylesheets with the flag set are not added/removed from the Document (their rules are already present by virtue of 2.) OTOH, scoped stylesheets that are added otherwise are dynamically added/removed as originally suggested.

This should be easy enough, while still avoiding any overhead on inserting/removing <x-foo> elements.

If there are no objections, I could give this version a go.
Comment 21 Dimitri Glazkov (Google) 2011-05-31 09:03:34 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > ... let's pick the simplest possible design and go with it.
> 
> In this case I would suggest a hybrid of your original suggestion and my last one:
> 
> 1.) create a scope-ID by simply having an incrementing counter
> 2.) automatically add all scoped stylesheets in <template>s
> 3.) add an additional flag for scoped style-sheets that are just instances of one in a <template>
> 
> Scoped stylesheets with the flag set are not added/removed from the Document (their rules are already present by virtue of 2.) OTOH, scoped stylesheets that are added otherwise are dynamically added/removed as originally suggested.
> 
> This should be easy enough, while still avoiding any overhead on inserting/removing <x-foo> elements.
> 
> If there are no objections, I could give this version a go.

SGTM.
Comment 22 Roland Steiner 2011-06-06 02:13:44 PDT
To give a quick update: I started implementing this as discussed, with the following modifications:

.) Use the address of the scoping element (parent of <style scoped>) as the "ID". This can be shared by multiple <style scoped> under the same element and doesn't require bookkeeping.

.) Collect the rules of every scope (global, and for every scoping element) within separate hash maps. The idea is that we don't need to to even try matching rules if their scoping element isn't an ancestor.

.) add the scoping element address as "hash" to the bloom filter, so as to be able to quickly discard scoped style sheets that don't apply.

However, esp. the 2nd point leads to a bit of yak-shave to adapt all the functions that only expect a single, global RuleSet (originally containing only a single set of rules maps, i.e. hash maps that are distinguished by what the last selector of the rule is - ID, class, pseudo-class, tag name or "universal").

A simpler implementation would be to add the scoping "ID"/address to RuleData. This has the disadvantage that it increases the size of RuleData, and that we'd have to compare that for every rule rather than once per scope.

I'd therefore be inclined to continue with the initially mentioned yak-shave, unless someone chimes in with a better idea.
Comment 23 Dimitri Glazkov (Google) 2011-06-06 09:34:06 PDT
Can you not just modify RuleSet to add an AtomRuleMap for these special ids, then add an extra check to RuleSet::addRule to add the rules containing special id selector there? That should save you quite a bit of work and will match how the rest of rule collection/matching is implemented.
Comment 24 Roland Steiner 2011-06-07 02:57:25 PDT
Created attachment 96223 [details]
work-in-progress

Missing: update when 'scoped' is set or unset, tests
Comment 25 Roland Steiner 2011-06-07 03:03:24 PDT
(In reply to comment #23)
> Can you not just modify RuleSet to add an AtomRuleMap for these special ids, then add an extra check to RuleSet::addRule to add the rules containing special id selector there? That should save you quite a bit of work and will match how the rest of rule collection/matching is implemented.

Uploaded a work-in-progress patch that follows this advice.

However, this has the drawback that all rules in scoped style-sheets are not filtered by ID/class/pseudo-ID/tag as global rules are. Perhaps this is not an issue as long as scoped style sheets are rare, but should they become more prevalent, this needs to be addressed (i.e., going back to my initial yak-shaving approach).

Also note that the scope "ID" also cannot be just added to the selector and has to be tested for separately, as I noted in comment #17 (there is no specific position where to add it) .
Comment 26 Roland Steiner 2011-06-07 03:08:14 PDT
Addendum: I also intend to add the address of elements that contain scoped style sheets to the bloom filter (didn't make it into the work-in-progress patch).
Comment 27 Roland Steiner 2011-06-07 04:57:14 PDT
Addendum 2: I also see that fastStyleSheetScopeIsAnAncestor has reverted itself to a primordial stage @_@. This should actually directly pull the applicable ancestor's scopes from the ParentStack (rather than just filtering).
Comment 28 Simon Fraser (smfr) 2011-06-07 07:49:57 PDT
Please try to catch Dave Hyatt on #webkit and discuss your approach with him.
Comment 29 Dimitri Glazkov (Google) 2011-06-07 09:15:21 PDT
Comment on attachment 96223 [details]
work-in-progress

View in context: https://bugs.webkit.org/attachment.cgi?id=96223&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:325
> +typedef Vector<RuleData> RulesVector;

Avoid cleanups in non-trivial patches. Makes for very painful archeology.

> Source/WebCore/dom/ElementRareData.h:90
> +    ++m_numberOfScopedHTMLStyleChildren;

I don't think this is necessary.
Comment 30 Dimitri Glazkov (Google) 2011-06-07 09:29:20 PDT
> For the issue of how to implement <style scoped>, this means that a scoped-id implementation cannot just apply normal selector matching, but the scoped-id must be matched separately after regular selector matching as "any ancestor has a scoped style sheet with that id".

I totally missed this.  I still don't understand what you mean here? Why can't we just prefix the selector chain? For example, for:

<div>
    <style scoped>
        #foo { color: red; }
   </style>
   <div id=foo></div>
</div>

We just make sure that when parsing, the selector is added as "${ID} #foo", where "${ID}" is some unique identifier we associate with the parent div (its address, as you suggested).

What is wrong with this approach?
Comment 31 Dimitri Glazkov (Google) 2011-06-07 09:29:46 PDT
(In reply to comment #29)
> (From update of attachment 96223 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96223&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:325
> > +typedef Vector<RuleData> RulesVector;
> 
> Avoid cleanups in non-trivial patches. Makes for very painful archeology.
> 
> > Source/WebCore/dom/ElementRareData.h:90
> > +    ++m_numberOfScopedHTMLStyleChildren;
> 
> I don't think this is necessary.

^^ I should correct myself -- I didn't understand why it was necessary.
Comment 32 Dimitri Glazkov (Google) 2011-06-07 09:40:57 PDT
(In reply to comment #28)
> Please try to catch Dave Hyatt on #webkit and discuss your approach with him.

That is a splendid idea. Roland -- can you find a slot of time to talk with hyatt?
Comment 33 Roland Steiner 2011-06-07 19:22:54 PDT
Created attachment 96356 [details]
work-in-progress (cleaned)

Still missing: update when 'scoped' is set or unset, tests
Comment 34 Roland Steiner 2011-06-07 19:28:26 PDT
(We discussed this in today's meeting, just summarizing below for posterity)

(In reply to comment #30)
> > For the issue of how to implement <style scoped>, this means that a scoped-id implementation cannot just apply normal selector matching, but the scoped-id must be matched separately after regular selector matching as "any ancestor has a scoped style sheet with that id".
> 
> I totally missed this.  I still don't understand what you mean here? Why can't we just prefix the selector chain? For example, for:

According to the HTML5 spec, selectors still match globally, not locally - see comment #15. 

(In reply to comment #31)
> (In reply to comment #29)
> > > Source/WebCore/dom/ElementRareData.h:90
> > > +    ++m_numberOfScopedHTMLStyleChildren;
> > 
> > I don't think this is necessary.
> 
> ^^ I should correct myself -- I didn't understand why it was necessary.

In my application, <style scoped> elements register with their containing elements, a sort of ref-counting if you will. This allows us to quickly determine whether an element needs to be considered a scope for style matching during style resolution.
Comment 35 Dave Hyatt 2011-06-08 09:01:08 PDT
Comment on attachment 96356 [details]
work-in-progress (cleaned)

It seems like you've lost the ability to filter rules by id/tag/class within scopes. This is because you made being scoped another category alongside id/tag/class. It seems like it would make more sense to have separate RuleSets for each scope.  Then you could still maintain id/tag/class filtering per-scope.
Comment 36 Roland Steiner 2011-06-08 19:16:59 PDT
(In reply to comment #35)
> (From update of attachment 96356 [details])
> It seems like you've lost the ability to filter rules by id/tag/class within scopes. This is because you made being scoped another category alongside id/tag/class. It seems like it would make more sense to have separate RuleSets for each scope.  Then you could still maintain id/tag/class filtering per-scope.

Yes, that was my original approach, and I completely agree that this is a better solution, but as mentioned above, I ran into quite a bit of yak-shaving with that. However, in the meantime I got an idea how to avoid it, so I'll try and see what I can come up with.
Comment 37 Roland Steiner 2011-06-09 01:38:55 PDT
Created attachment 96559 [details]
work-in-progress (scoped)

The patch is similar to the previous work-in-progress patch, but now has complete RuleSets scoped, as suggested. It still misses the same things the previous patches did (updating after 'scoped' attribute change, tests). I will add these, unless there are objections with the directions the patch is going.
Comment 38 Roland Steiner 2011-06-09 01:40:54 PDT
One more question regarding my patch is whether lumping all sibling rules together is still the right thing to do, or whether they also need to be scoped somehow.
Comment 39 Dimitri Glazkov (Google) 2011-06-09 09:22:16 PDT
Comment on attachment 96559 [details]
work-in-progress (scoped)

View in context: https://bugs.webkit.org/attachment.cgi?id=96559&action=review

I think this looks pretty good.

> Source/WebCore/css/CSSStyleSelector.cpp:291
> +typedef CSSStyleSelector::StyleSheetScope StyleSheetScope;

I think these typedefs are a bit misleading. If you want to understand what StyleSheetScope is, you have to traverse the code to discover.
Comment 40 Roland Steiner 2011-06-13 20:24:24 PDT
Created attachment 97062 [details]
patch, full HTML5 version

full implementation, following the HTML5 spec.
Comment 41 Roland Steiner 2011-06-13 21:08:12 PDT
Caveat: I'm not sure if all "@rules-must-be scoped" concerns are addressed with this patch.
Comment 42 Dominic Cooney 2011-06-13 21:20:14 PDT
Comment on attachment 97062 [details]
patch, full HTML5 version

View in context: https://bugs.webkit.org/attachment.cgi?id=97062&action=review

This is an exciting patch.

Have you considered testing boundary cases such as:
- selectors matching completely in ancestors of the element with style scoped
- selectors matching in part in ancestors and descendants
- selectors matching completely in descendants
- attaching and removing style scoped elements as sibling of the body, as child of the body, in a form element, etc.
- testing cascade between scoped and not scoped styles; between different sets of scoped styles
- cloning a style scoped
- using style scoped in a document fragment
- style scoped including @page rules, etc.

> LayoutTests/fast/css/style-scoped-attach.html:37
> +            var divElem =document.getElementById('DIV');

Two spaces around =

> LayoutTests/fast/css/style-scoped-attach.html:40
> +            styleElem.innerHTML = "div { color: red; } p { color: green; }";

Don’t needlessly switch between " and '-quoted literals.

> Source/WebCore/css/CSSStyleSelector.cpp:458
> +    if (!ownerNode || !ownerNode->isHTMLElement() || !ownerNode->hasTagName(HTMLNames::styleTag))

When is a stylesheet’s owner not a <style>? Inline styles?

> Source/WebCore/css/CSSStyleSelector.cpp:842
> +static inline bool compareRules(const RuleData* r1, const RuleData* r2)

Could you give this a better name? Because "compareRules" returning boolean could be confused with equality comparison.

> Source/WebCore/html/HTMLStyleElement.cpp:69
> +                unregisterWithScopingElement();

Why?
Comment 43 Roland Steiner 2011-06-13 22:06:56 PDT
Created attachment 97067 [details]
patch, HTML5 version, cleaned

full implementation with requested clean-up, same caveats as before (@-rules, handling of sibling rule features)
Comment 44 Dominic Cooney 2011-06-13 22:08:30 PDT
(In reply to comment #43)

And the tests?
Comment 45 Roland Steiner 2011-06-13 22:24:29 PDT
(In reply to comment #42)
> Have you considered testing boundary cases such as:
> - selectors matching completely in ancestors of the element with style scoped
> - selectors matching in part in ancestors and descendants

Extended style-scoped-basic.html

> - selectors matching completely in descendants

This was already handled in style-scoped-basic.html

> - attaching and removing style scoped elements as sibling of the body, as child of the body, in a form element, etc.

Attaching, removing, setting and un-setting of 'scoped' is tested in style-scoped-attach/detach/set-scoped/remove-scoped.html. 

> - testing cascade between scoped and not scoped styles; between different sets of scoped styles
> - cloning a style scoped
> - using style scoped in a document fragment

The implementation does not touch the cascade, and a <style scoped> is not special in any way until it is inserted into the document.

> - style scoped including @page rules, etc.

@page rules should be ignored by the implementation, but this, as well as all other @-rules may indeed need further scrutiny and testing.

> > LayoutTests/fast/css/style-scoped-attach.html:37
> > +            var divElem =document.getElementById('DIV');
> 
> Two spaces around =

Done.

> > LayoutTests/fast/css/style-scoped-attach.html:40
> > +            styleElem.innerHTML = "div { color: red; } p { color: green; }";
> 
> Don’t needlessly switch between " and '-quoted literals.

Cleaned-up.

> > Source/WebCore/css/CSSStyleSelector.cpp:458
> > +    if (!ownerNode || !ownerNode->isHTMLElement() || !ownerNode->hasTagName(HTMLNames::styleTag))
> 
> When is a stylesheet’s owner not a <style>? Inline styles?

<link>, processing instructions, and perhaps some SVG stuff. A style-sheet may also be @import-ed, etc.

> > Source/WebCore/css/CSSStyleSelector.cpp:842
> > +static inline bool compareRules(const RuleData* r1, const RuleData* r2)
> 
> Could you give this a better name? Because "compareRules" returning boolean could be confused with equality comparison.

That isn't originally my code, I just had to move it to before sortAndTransferMatchedRules(). But renamed it to 'lessSpecific()'.

> > Source/WebCore/html/HTMLStyleElement.cpp:69
> > +                unregisterWithScopingElement();
> 
> Why?

At this point, the <style> element is no longer scoped, so it has to be unregistered from its parent, which in turn no longer acts as a scoping element (unless there are more <style scoped> children) - but perhaps I misunderstand the intent of the question (?).
Comment 46 Roland Steiner 2011-06-14 00:40:43 PDT
Summarizing the remaining questions and differences to the HTML5 spec (so, "full HTML5 version" in attachment #4 was perhaps overstating things a wee bit ^_^; ):

.) Scoping @-rules is largely unimplemented, esp. @font-face, which is mentioned in the spec. Ignoring @page is implemented, but untested (awaiting feedback on @-rules as a whole). Looking at the code, scoping @font-face seems to be quite tricky (suggestions would be very welcome).

.) No check whether a <style scoped> is indeed the first child of its parent, or only preceded by other <style> elements or inter-element white-space (implementing this seems to be very expensive for little to no gain).

.) Implementation ignores 'scoped' if the <style> element is within the <head>, which is not specced as such, but seems intuitive, IMHO.

.) Implementation-detail question: whether there needs to be a special handling with regard of sibling rules.
Comment 47 Ian 'Hixie' Hickson 2011-06-14 00:47:23 PDT
> .) Scoping @-rules is largely unimplemented, esp. @font-face, which is mentioned in the spec. Ignoring @page is implemented, but untested (awaiting feedback on @-rules as a whole). Looking at the code, scoping @font-face seems to be quite tricky (suggestions would be very welcome).

@font-face in particular is pretty important, because not doing this would allow one scoped style sheet to dramatically affect the rest of the document.

@import is presumably also pretty important.

The various animation @rules are likely pretty important to prevent clashes that break animations.


> .) No check whether a <style scoped> is indeed the first child of its parent, or only preceded by other <style> elements or inter-element white-space (implementing this seems to be very expensive for little to no gain).

Where does the spec require you to do that?


> .) Implementation ignores 'scoped' if the <style> element is within the <head>, which is not specced as such, but seems intuitive, IMHO.

That's a bug. Don't do that. :-)
Comment 48 Roland Steiner 2011-06-14 15:53:44 PDT
(In reply to comment #47)
> > .) No check whether a <style scoped> is indeed the first child of its parent, or only preceded by other <style> elements or inter-element white-space (implementing this seems to be very expensive for little to no gain).
> 
> Where does the spec require you to do that?

"Contexts in which this element can be used:
[...]
If the scoped attribute is present: where flow content is expected, but before any other flow content other than other style elements and inter-element whitespace."

But I guess checking this well-formed-ness would be rather the job of the parser.
Comment 49 Ian 'Hixie' Hickson 2011-06-14 16:04:17 PDT
First of all, the "contexts in which this element can be used" bits is explicitly non-normative:

   http://www.whatwg.org/specs/web-apps/current-work/complete.html#element-definitions

But even if it was normative (as the "content model" line is), it would be a requirement on documents, not on browsers. Only implement what the spec says you need to implement, not more. :-)
Comment 50 Roland Steiner 2011-07-26 02:56:03 PDT
Created attachment 101982 [details]
Work-in-progress

Work-in-progress patch (only because some layout tests fail for unfathomable reasons - most likely because of all the trees in front of the forest.)

Changes to previous patch:

-) As discussed on the ML, selectors are now scoped to the scoping element and its descendants
-) Addressed Hixie's remarks

The new version is somewhat more complicated, because it has to interfere with selector matching. Esp. RuleData grew by 4 bytes - I could not find an efficient way to avoid this. @keyframe and @font-face are also not yet handled - they are ignored for the time being (I would prefer to handle that in a follow-up patch). :root and :scope are also not yet handled - those, too, would be another patch for another bug(s).
Comment 51 Dimitri Glazkov (Google) 2011-07-26 09:08:07 PDT
(In reply to comment #50)
> Created an attachment (id=101982) [details]
> Work-in-progress
> 
> Work-in-progress patch (only because some layout tests fail for unfathomable reasons - most likely because of all the trees in front of the forest.)
> 
> Changes to previous patch:
> 
> -) As discussed on the ML, selectors are now scoped to the scoping element and its descendants
> -) Addressed Hixie's remarks
> 
> The new version is somewhat more complicated, because it has to interfere with selector matching. Esp. RuleData grew by 4 bytes - I could not find an efficient way to avoid this. @keyframe and @font-face are also not yet handled - they are ignored for the time being (I would prefer to handle that in a follow-up patch). :root and :scope are also not yet handled - those, too, would be another patch for another bug(s).

This may be a dumb thought, but with this approach, can we not just get back to the wonderful world of prefixing selector chain with a unique id?
Comment 52 Roland Steiner 2011-07-26 17:47:21 PDT
(In reply to comment #51)
> This may be a dumb thought, but with this approach, can we not just get back to the wonderful world of prefixing selector chain with a unique id?

Technically yes, but I chose the current implementation approach for the following reasons:

.) Prepending a special identifier would require re-creating the selector lists whenever @scoped is set or removed. AFAICT there is no code path that does that currently, so I'd have to implement that instead.

.) The majority of responses were in favor of the scoping element to be eligible for matching. A simple prefix (that presumably would have a 'descendant' combinator) would not work for this, but would rather have to be implemented as a flag or some such.

.) With a prefix, most selectors would be matched up to the root, just then to fail with the prefix at the end. Inasmuch my implementation should be faster as it'll abort when passing the scoping element. OTOH, the prefix might be easier to add to the fast selector matching path...

OTOH, it just occurred to me there may indeed be a way to have a prefix and eat it, too, if we allow for a special "prefix" combinator (which may also be beneficial when it comes to template instances)... 
I'll nonetheless try to get the current code running as it's almost finished, and put it up before going off on that tangent.
Comment 53 Roland Steiner 2011-07-27 00:28:56 PDT
Created attachment 102100 [details]
Patch
Comment 54 Roland Steiner 2011-07-27 00:47:08 PDT
Patch comments: By twisting around a few knobs, the new patch should now achieve the best of all worlds (even if I do say so myself)! :) In detail:

.) Scoped style sheets continue to be stored in their own RuleSets. That means that only the relevant style sheet rules are tested for for a given element. IOW, scoped stylesheets don't "pollute the global namespace"

.) The scope is not stored within the rule, nor the selector, so updating is not necessary even if @scoped is inserted or removed. Instead, the scope is passed in to the matching functions. This also means that in the future we can re-use the same RuleSet for XBL-like template instantiation.

.) Because of the above change I was able to again get rid of the 4-byte size increase of RuleData.

.) Selector matching can still abort early if the scoping element would be passed.

.) Scoped styles can now also participate in the fast selector matching path.

Things not implemented:

.) The patch includes no support for @keyframe and @font-face rules. Rather than scoping them, they are just ignored for the time being. I would prefer this to be addressed in future patches rather than cramming everything into this patch. @page is ignored as per the HTML5 spec, and @import is handled as normal.

.) Allowing selector matching to go past the scoping element if the selector contains :root or :scope is also a task for a future patch(es).

.) I extended the layout tests a bit, but can certainly add more (wanted to finish the code and get feedback first).
Comment 55 Hajime Morrita 2011-07-29 01:20:48 PDT
This change looks to big. How about to divide it into
- Handling @scoped attribute in DOM world - which could be testable with window.internals.
- Handling CSS parser part
?
Comment 56 Roland Steiner 2011-07-29 02:30:23 PDT
(In reply to comment #55)
> This change looks to big. How about to divide it into

Heh, it's not even close to my worst! ;) Also, please consider that the patch contains 5 layout tests.

> - Handling @scoped attribute in DOM world - which could be testable with window.internals.
> - Handling CSS parser part

I don't quite follow: the new patch doesn't touch the CSS parser, only the selector matching and the <style> element, either of which change is meaningless without the other.

In any case, I believe a "non-native" implementation would be counter-productive. Furthermore, in addition to the obvious applications, this patch opens up quite a few possibilities, e.g., a scoped querySelectorAll (with an optional 'scope' parameter, or in a more limited 'querySelectorScoped' version). Also, stylesheets that use rules like "#menu .submenu div" a lot could now be shunted verbatim into a <style scoped>. On browsers that don't honor @scoped this wouldn't change anything, but in WebKit, those rules now wouldn't even be looked at (!) unless at an element that is in that sub-tree.
Comment 57 Hajime Morrita 2011-07-29 03:08:17 PDT
> 
> I don't quite follow: the new patch doesn't touch the CSS parser, only the selector matching and the <style> element, either of which change is meaningless without the other.
I'm sorry for my confusing comment. I meant to say is css selector.
Comment 58 Dimitri Glazkov (Google) 2011-07-29 10:26:52 PDT
Comment on attachment 102100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102100&action=review

This looks pretty hot. I agree it's largish, and I would really like Hyatt to take a look.

> Source/WebCore/css/CSSStyleSelector.cpp:3422
> +                        // FIXME: We don't add @font-face rules of scoped style sheets for the moment.
> +                        // Find a way to have scoped @font-face rules.

A good practice is to file a bug on this and mention it here.

> Source/WebCore/css/CSSStyleSelector.cpp:3431
> +                        // Find a way to have scoped @keyframe rules.

Ditto.

> Source/WebCore/css/CSSStyleSelector.h:239
> +        void matchRulesGlobally(RuleSet*, int& firstRuleIndex, int& lastRuleIndex, bool includeEmptyRules);

Can be just matchRules as before?
Comment 59 Roland Steiner 2011-07-31 18:39:43 PDT
(In reply to comment #58)
> View in context: https://bugs.webkit.org/attachment.cgi?id=102100&action=review
> 
> This looks pretty hot. I agree it's largish, and I would really like Hyatt to take a look.

I agree - could you please do the IM-pinging when you see him around?

> > Source/WebCore/css/CSSStyleSelector.cpp:3422
> > +                        // FIXME: We don't add @font-face rules of scoped style sheets for the moment.
> > +                        // Find a way to have scoped @font-face rules.
> 
> A good practice is to file a bug on this and mention it here.

I intended to add these on landing after confirming it's ok to leave out in the first patch.

> > Source/WebCore/css/CSSStyleSelector.h:239
> > +        void matchRulesGlobally(RuleSet*, int& firstRuleIndex, int& lastRuleIndex, bool includeEmptyRules);
> 
> Can be just matchRules as before?

I renamed it to make it (more) explicit that using this function will not scope the matching, even if one should call it on a RuleSet that comes from a <style scoped>. But if that's deemed obvious I can of course go back to "matchRules".
Comment 60 piet 2011-08-01 13:42:36 PDT
(In reply to comment #56)
> Furthermore, in addition to the obvious applications, this patch opens up quite a few possibilities, e.g., a scoped querySelectorAll (with an optional 'scope' parameter, or in a more limited 'querySelectorScoped' version). Also, stylesheets that use rules like "#menu .submenu div" a lot could now be shunted verbatim into a <style scoped>. On browsers that don't honor @scoped this wouldn't change anything, but in WebKit, those rules now wouldn't even be looked at (!) unless at an element that is in that sub-tree.

I'm not sure what a scoped querySelectorAll() would do because element.querySelectorAll() is already "scoped" to the node it is called upon (http://www.w3.org/TR/selectors-api/#queryselectorall). Otherwise, you are correct: this patch allows the specification of scopes through other means than the 'scoped' attribute, for instance an @scope at-rule that would work like an @media at-rule, or a 'scope' attribute in an @import at-rule. Examples:

@scope #id1, #id2 {
    ...
    /* rules */
    ...
}

@import (/path/to/style.css) screen, projection, scope(#id1, #id2);
Comment 61 Tab Atkins 2011-08-01 13:48:06 PDT
(In reply to comment #60)
> (In reply to comment #56)
> > Furthermore, in addition to the obvious applications, this patch opens up quite a few possibilities, e.g., a scoped querySelectorAll (with an optional 'scope' parameter, or in a more limited 'querySelectorScoped' version). Also, stylesheets that use rules like "#menu .submenu div" a lot could now be shunted verbatim into a <style scoped>. On browsers that don't honor @scoped this wouldn't change anything, but in WebKit, those rules now wouldn't even be looked at (!) unless at an element that is in that sub-tree.
> 
> I'm not sure what a scoped querySelectorAll() would do because element.querySelectorAll() is already "scoped" to the node it is called upon (http://www.w3.org/TR/selectors-api/#queryselectorall).

Not quite.  querySelector *filters* its output based on the node it's called on, to exclude any matches outside of the node's subtrees.  Scoping is slightly different.
Comment 62 Dominic Cooney 2011-09-06 10:42:55 PDT
Could you update this patch to head (eg with fast checkable selectors?)
Comment 63 Roland Steiner 2011-09-06 12:35:04 PDT
Created attachment 106466 [details]
work-in-progress before break-up

Work-in-=progress patch before breaking it up into smaller chunks, mainly to fix merge conflicts. On a side note, extending querySelector[All] from r94089 to be scoped should be trivial, but isn't included in this patch.
Comment 64 Roland Steiner 2011-09-07 11:06:13 PDT
Note: Change to HTMLStyleElement.idl is missing - will add it to upcoming (smaller) patches.
Comment 65 Roland Steiner 2011-09-07 15:17:25 PDT
Created attachment 106656 [details]
work-in-progress (fix conflicts)

One more work-in-progress to resolve conflicts from changeset 94656.
Comment 66 Antti Koivisto 2011-09-20 02:26:00 PDT
You might be able to avoid adding branchiness and somewhat simplify the code by providing the scope element also in non-scoped case (document root). Current null tests would be replaced by scope elements tests everywhere.
Comment 67 Roland Steiner 2011-11-20 22:31:52 PST
(In reply to comment #66)
> You might be able to avoid adding branchiness and somewhat simplify the code by providing the scope element also in non-scoped case (document root). Current null tests would be replaced by scope elements tests everywhere.

In theory yes, but that would require knowing the root node in all cases. This could be the document, but it could also be a DocumentFragment or a ShadowRoot. Now, once bug 59816 has been implemented, this will be trivial, but till then, a NULL pointer for "no scope" is IMHO easier and safer.
Comment 68 Ian 'Hixie' Hickson 2012-01-30 15:42:49 PST
Spec is updated; let me know if there's anything wrong with it:

http://www.whatwg.org/specs/web-apps/current-work/#attr-style-scoped
Comment 69 Radar WebKit Bug Importer 2014-02-21 09:54:43 PST
<rdar://problem/16134460>
Comment 70 Konstantin Tokarev 2016-08-11 12:31:35 PDT
<style scoped> was removed from tree in r156683, as well as from spec.