Bug 67720

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 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 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 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 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 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 on attachment 111229 [details] patch (requires 67718, 67790) Attachment 111229 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10075999

Created attachment 115342 [details] updated patch Updated & de-conflicted patch - will r? after 67718 and 67790 have landed

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

Antti, could you comment on the general approach?

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 on attachment 115342 [details] updated patch Attachment 115342 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10425004

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

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

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.

Created attachment 116174 [details] patch, shrunk New patch, removed all renaming and refactoring that isn't strictly necessary.

(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 on attachment 116174 [details] patch, shrunk Attachment 116174 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10644275

Comment on attachment 116174 [details] patch, shrunk Attachment 116174 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10639077

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 on attachment 116174 [details] patch, shrunk Attachment 116174 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10644279

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

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

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

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.

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