Full Text Bug Listing

Dimitri Glazkov (Google)

Reported 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.

Roland Steiner

Comment 1 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?

Hajime Morrita

Comment 2 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.

Ian 'Hixie' Hickson

Comment 3 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.)

Hajime Morrita

Comment 4 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.)

Roland Steiner

Comment 5 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).

Hajime Morrita

Comment 6 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 ;-)

Dimitri Glazkov (Google)

Comment 7 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?

Ian 'Hixie' Hickson

Comment 8 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).

Roland Steiner

Comment 9 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 (?).

Dimitri Glazkov (Google)

Comment 10 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.

Roland Steiner

Comment 11 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.

Dimitri Glazkov (Google)

Comment 12 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.

Ian 'Hixie' Hickson

Comment 13 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.

Roland Steiner

Comment 14 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.

Ian 'Hixie' Hickson

Comment 15 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.

Roland Steiner

Comment 16 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.

Roland Steiner

Comment 17 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).

Ian 'Hixie' Hickson

Comment 18 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.

Dimitri Glazkov (Google)

Comment 19 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.

Roland Steiner

Comment 20 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.

Dimitri Glazkov (Google)

Comment 21 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.

Roland Steiner

Comment 22 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.

Dimitri Glazkov (Google)

Comment 23 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.

Roland Steiner

Comment 24 2011-06-07 02:57:25 PDT

Created attachment 96223 [details] work-in-progress Missing: update when 'scoped' is set or unset, tests

Roland Steiner

Comment 25 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) .

Roland Steiner

Comment 26 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).

Roland Steiner

Comment 27 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).

Simon Fraser (smfr)

Comment 28 2011-06-07 07:49:57 PDT

Please try to catch Dave Hyatt on #webkit and discuss your approach with him.

Dimitri Glazkov (Google)

Comment 29 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.

Dimitri Glazkov (Google)

Comment 30 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?

Dimitri Glazkov (Google)

Comment 31 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.

Dimitri Glazkov (Google)

Comment 32 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?

Roland Steiner

Comment 33 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

Roland Steiner

Comment 34 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.

Dave Hyatt

Comment 35 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.

Roland Steiner

Comment 36 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.

Roland Steiner

Comment 37 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.

Roland Steiner

Comment 38 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.

Dimitri Glazkov (Google)

Comment 39 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.

Roland Steiner

Comment 40 2011-06-13 20:24:24 PDT

Created attachment 97062 [details] patch, full HTML5 version full implementation, following the HTML5 spec.

Roland Steiner

Comment 41 2011-06-13 21:08:12 PDT

Caveat: I'm not sure if all "@rules-must-be scoped" concerns are addressed with this patch.

Dominic Cooney

Comment 42 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?

Roland Steiner

Comment 43 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)

Dominic Cooney

Comment 44 2011-06-13 22:08:30 PDT

(In reply to comment #43) And the tests?

Roland Steiner

Comment 45 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 (?).

Roland Steiner

Comment 46 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.

Ian 'Hixie' Hickson

Comment 47 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. :-)

Roland Steiner

Comment 48 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.

Ian 'Hixie' Hickson

Comment 49 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. :-)

Roland Steiner

Comment 50 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).

Dimitri Glazkov (Google)

Comment 51 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?

Roland Steiner

Comment 52 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.

Roland Steiner

Comment 53 2011-07-27 00:28:56 PDT

Created attachment 102100 [details] Patch

Roland Steiner

Comment 54 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).

Hajime Morrita

Comment 55 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 ?

Roland Steiner

Comment 56 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.

Hajime Morrita

Comment 57 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.

Dimitri Glazkov (Google)

Comment 58 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?

Roland Steiner

Comment 59 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".

piet

Comment 60 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);

Tab Atkins

Comment 61 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.

Dominic Cooney

Comment 62 2011-09-06 10:42:55 PDT

Could you update this patch to head (eg with fast checkable selectors?)

Roland Steiner

Comment 63 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.

Roland Steiner

Comment 64 2011-09-07 11:06:13 PDT

Note: Change to HTMLStyleElement.idl is missing - will add it to upcoming (smaller) patches.

Roland Steiner

Comment 65 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.

Antti Koivisto

Comment 66 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.

Roland Steiner

Comment 67 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.

Ian 'Hixie' Hickson

Comment 68 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

Radar WebKit Bug Importer

Comment 69 2014-02-21 09:54:43 PST

<rdar://problem/16134460>

Konstantin Tokarev

Comment 70 2016-08-11 12:31:35 PDT

Attachments
work-in-progress (30.11 KB, patch) 2011-06-07 02:57 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
work-in-progress (cleaned) (27.58 KB, patch) 2011-06-07 19:22 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
work-in-progress (scoped) (32.97 KB, patch) 2011-06-09 01:38 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
patch, full HTML5 version (55.82 KB, patch) 2011-06-13 20:24 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
patch, HTML5 version, cleaned (56.56 KB, patch) 2011-06-13 22:06 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
Work-in-progress (73.32 KB, patch) 2011-07-26 02:56 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
Patch (77.46 KB, patch) 2011-07-27 00:28 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
work-in-progress before break-up (77.45 KB, patch) 2011-09-06 12:35 PDT, Roland Steiner	no flags	Details Formatted Diff Diff
work-in-progress (fix conflicts) (113.65 KB, text/plain) 2011-09-07 15:17 PDT, Roland Steiner	no flags	Details
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Bug 49142