Bug 67720

Summary: <style scoped>: Implement selector matching
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: DOMAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED FIXED    
Severity: Normal CC: arv, dglazkov, dominicc, gns, hayato, koivisto, macpherson, morrita, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 67790, 70223, 72480, 72482, 73190, 73192    
Bug Blocks: 68093, 49142    
Attachments:
Description Flags
patch (requires 67718, 67790)
rolandsteiner: commit-queue-
updated patch
none
patch, shrunk koivisto: review-, rolandsteiner: commit-queue-

Description Roland Steiner 2011-09-07 11:10:09 PDT
As part of the larger <style scoped> implementation, implement the necessary changes to selector matching.
Comment 1 Roland Steiner 2011-10-17 02:26:07 PDT
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 .
Comment 2 Gyuyoung Kim 2011-10-17 02:33:04 PDT
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
Comment 3 WebKit Review Bot 2011-10-17 02:33:50 PDT
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
Comment 4 Early Warning System Bot 2011-10-17 02:35:58 PDT
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
Comment 5 Gustavo Noronha (kov) 2011-10-17 02:40:01 PDT
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
Comment 6 Roland Steiner 2011-10-17 03:07:22 PDT
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).
Comment 7 Daniel Bates 2011-10-17 06:40:41 PDT
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
Comment 8 Roland Steiner 2011-11-16 00:25:30 PST
Created attachment 115342 [details]
updated patch

Updated & de-conflicted patch - will r? after 67718 and 67790 have landed
Comment 9 Roland Steiner 2011-11-16 02:21:04 PST
Update: I factored out some smaller refactoring from the patch into bugs 72480 and 72482. Will update patch after those have landed.
Comment 10 Dimitri Glazkov (Google) 2011-11-16 08:53:52 PST
Antti, could you comment on the general approach?
Comment 11 WebKit Review Bot 2011-11-16 09:09:39 PST
Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10329013
Comment 12 Gustavo Noronha (kov) 2011-11-16 09:11:48 PST
Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10425004
Comment 13 Gyuyoung Kim 2011-11-16 09:14:21 PST
Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10396969
Comment 14 Early Warning System Bot 2011-11-16 10:07:14 PST
Comment on attachment 115342 [details]
updated patch

Attachment 115342 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10486805
Comment 15 Antti Koivisto 2011-11-21 11:23:59 PST
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.
Comment 16 Roland Steiner 2011-11-21 20:56:20 PST
Created attachment 116174 [details]
patch, shrunk

New patch, removed all renaming and refactoring that isn't strictly necessary.
Comment 17 Roland Steiner 2011-11-21 22:47:43 PST
(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.
Comment 18 Gustavo Noronha (kov) 2011-11-23 20:55:10 PST
Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10644275
Comment 19 Early Warning System Bot 2011-11-23 20:57:50 PST
Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10639077
Comment 20 WebKit Review Bot 2011-11-23 20:59:06 PST
Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10646184
Comment 21 Gyuyoung Kim 2011-11-23 21:06:50 PST
Comment on attachment 116174 [details]
patch, shrunk

Attachment 116174 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10644279
Comment 22 Antti Koivisto 2011-11-24 00:03:50 PST
(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?
Comment 23 Roland Steiner 2011-11-24 18:12:35 PST
(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.
Comment 24 Roland Steiner 2011-11-24 18:13:59 PST
To clarify: I should say it's just a matter of Hixie UPDATING the spec, as he already agreed to the change.
Comment 25 Antti Koivisto 2011-11-26 03:03:20 PST
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.
Comment 26 Roland Steiner 2011-11-27 23:41:49 PST
Opened new issues 73190 (scoped stylesheets) and 73192 (scoped selector matching) as requested.