Bug 67720 – <style scoped>: Implement selector matching

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Roland Steiner]

As part of the larger <style scoped> implementation, implement the necessary changes to selector matching.

[Roland Steiner]

Created attachment 111229 [details]
patch (requires 67718, 67790)

Patch implementing scoped selector resolution. Note that a selector does NOT store itself whether it should be scoped or not! Rather, this is an additional parameter to the matching functions. This way selectors do not have to be updated whenever the 'scoped' attribute on a <style> element is set or removed, it also means that a follow-up implementation of queryScopedSelector() will be utterly trivial.

Note that this patch is not stand-alone - it requires the patches from https://bugs.webkit.org/show_bug.cgi?id=67718 and https://bugs.webkit.org/show_bug.cgi?id=67790 .

[Gyuyoung Kim]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Attachment 111229 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10076940

[WebKit Review Bot]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Attachment 111229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10077895

[Early Warning System Bot]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Attachment 111229 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10075932

[Gustavo Noronha (kov)]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Attachment 111229 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10079805

[Roland Steiner]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Seems even without the scope attribute and registering, the patch is still quite large. Will try to break it up further (please consider this a work-in-progress then).

[Daniel Bates]

Comment on attachment 111229 [details]
patch (requires 67718, 67790)

Attachment 111229 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10075999

[Roland Steiner]

Created attachment 115342 [details]
updated patch

Updated & de-conflicted patch - will r? after 67718 and 67790 have landed

[Roland Steiner]

Update: I factored out some smaller refactoring from the patch into bugs 72480 and 72482. Will update patch after those have landed.

[Dimitri Glazkov (Google)]

Antti, could you comment on the general approach?

[WebKit Review Bot]

Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10329013

[Gustavo Noronha (kov)]

Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10425004

[Gyuyoung Kim]

Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10396969

[Early Warning System Bot]

Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10486805

[Antti Koivisto]

As far as I understand, this patch implements a two separate features:
- Stylesheets scoped to a subtree (rules on sheets are only applied to the elements in the subtree).
- Selectors scoped to a subtree (child/descendant selectors only match up the the scope element).

Partly due to above, the logic of the patch gets somewhat lost and it is hard to determine if this is even a good implementation strategy.

These are separate features (is the latter yet specced?) and should be done in separate patches. Doing the first step only first would likely keep things much clearer.

The performance of the regular selector matching code is very critical while the performance of the scoped selector matching does not matter at all yet (as no one is currently using it). You should focus on making a testable implementation that has minimal impact on the existing code so we gain good understanding of the problem. The optimizations can come later.

You should in general avoid unrelated refactoring when implementing something like this.

[Roland Steiner]

Created attachment 116174 [details]
patch, shrunk

New patch, removed all renaming and refactoring that isn't strictly necessary.

[Roland Steiner]

(In reply to comment #15)
> As far as I understand, this patch implements a two separate features:
> - Stylesheets scoped to a subtree (rules on sheets are only applied to the elements in the subtree).
> - Selectors scoped to a subtree (child/descendant selectors only match up the the scope element).
> 
> Partly due to above, the logic of the patch gets somewhat lost and it is hard to determine if this is even a good implementation strategy.
>
> These are separate features (is the latter yet specced?) and should be done in separate patches. Doing the first step only first would likely keep things much clearer.

Yes, but please note that scoped stylesheets are just stored associated with their scope - there is no internal change. The difference only arises when that associated scope is applied to the selector matching algorithm.

Therefore, scoping without scoped stylesheets would have no "client", while scoping stylesheets without the required scoped matching mechanism would be pointless.

I'm therefore a bit at a loss how to divide the patch. One way could be to implement scoped selector matching plus element.find[All]() or document.queryScopedSelector[All] in order to have test cases as one patch, and storing scoped stylesheets in ScopedRuleSets but without scoped selector matching, and implement window.internals.styleSheetsForElement() in order to test that, as a 2nd patch. 

However, I'm not sure doing it this way would make for a much smaller patches, individually.

> The performance of the regular selector matching code is very critical while the performance of the scoped selector matching does not matter at all yet (as no one is currently using it). You should focus on making a testable implementation that has minimal impact on the existing code so we gain good understanding of the problem. The optimizations can come later.
> 
> You should in general avoid unrelated refactoring when implementing something like this.

I uploaded a new patch (no r? set as it still requires 67718 and 67790) that removes all renaming and refactoring.

[Gustavo Noronha (kov)]

Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10644275

[Early Warning System Bot]

Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10639077

[WebKit Review Bot]

Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10646184

[Gyuyoung Kim]

Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10644279

[Antti Koivisto]

(In reply to comment #17)
> Yes, but please note that scoped stylesheets are just stored associated with their scope - there is no internal change. The difference only arises when that associated scope is applied to the selector matching algorithm.
> 
> Therefore, scoping without scoped stylesheets would have no "client", while scoping stylesheets without the required scoped matching mechanism would be pointless.

I don't quite understand what you saying. You certainly can implement <style scoped> as described in the current HTML draft (scoping of the stylesheets) without initially implementing the scoping of the selectors. As far as I can tell, thats your patch up to CSSStyleSelector::matchAuthorRules where you pick the scoped stylesheets for the element. The changes to matchRuleSet/matchRulesForList where you pass the scope down to SelectorChecker::checkSelector are part of the second feature.

You missed my question about the specification status of the scoped selectors. Is that still in level of mailing list discussions?

[Roland Steiner]

(In reply to comment #22)
> You missed my question about the specification status of the scoped selectors. Is that still in level of mailing list discussions?

The change to make selectors scoped to the parent element of <style scoped> is pretty uncontroversial by now - it's just a matter of Hixie changing the spec. See https://lists.webkit.org/pipermail/webkit-dev/2011-November/018589.html and http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033337.html for reference.

[Roland Steiner]

To clarify: I should say it's just a matter of Hixie UPDATING the spec, as he already agreed to the change.

[Antti Koivisto]

Comment on attachment 116174 [details]
patch, shrunk

Setting to r- to clarify I'd like to see this done in steps as explained in the comments above.

[Roland Steiner]

Opened new issues 73190 (scoped stylesheets) and 73192 (scoped selector matching) as requested.