Bug 81069 - Implement :scope for element.querySelector[All]()
Summary: Implement :scope for element.querySelector[All]()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-13 19:04 PDT by Roland Steiner
Modified: 2013-03-13 02:12 PDT (History)
12 users (show)

See Also:


Attachments
Layout test (1.80 KB, text/html)
2012-03-30 04:30 PDT, Roland Steiner
no flags Details
Patch (13.39 KB, patch)
2013-03-05 23:32 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (14.27 KB, patch)
2013-03-11 21:14 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (14.34 KB, patch)
2013-03-11 23:03 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2012-03-13 19:04:51 PDT
If querySelector() or querySelectorAll() are called on an element, that element should act as the context node for :scope
Comment 1 Roland Steiner 2012-03-25 23:34:25 PDT
Implementing this requires changing the code such that selector matching is allowed to exceed the scope/context element, e.g., as done in the patch for bug 79075.
Comment 2 Bronislav Klucka 2012-03-26 00:02:34 PDT
(In reply to comment #1)
> Implementing this requires changing the code such that selector matching is allowed to exceed the scope/context element, e.g., as done in the patch for bug 79075.

Hello, what does it actually mean in terms of "when it will be done"? 

Brona
Comment 3 Roland Steiner 2012-03-26 18:07:28 PDT
(In reply to comment #2)
> Hello, what does it actually mean in terms of "when it will be done"? 

It means we need to assess the benefits of this feature vs. the potential performance cost of its implementation. At least it will require some amount of refactoring for a nice implementation.

At the moment I don't have the spare cycles to implement this, but I'll get back to it in the (hopefully not so distant) future in case nobody else tackled it in the meantime.
Comment 4 Roland Steiner 2012-03-30 04:30:53 PDT
Created attachment 134774 [details]
Layout test

Layout test (to go in LayoutTests/fast/dom/SelectorAPI/element-scope.html)
Comment 5 Bronislav Klucka 2012-05-11 02:35:58 PDT
Any plans here?
Comment 6 Bronislav Klucka 2013-03-04 20:15:19 PST
hi, any news?
Comment 7 Takashi Sakamoto 2013-03-05 23:32:14 PST
Created attachment 191660 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2013-03-06 07:41:12 PST
Comment on attachment 191660 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.h:83
> +        const Node* contextualReferenceNode;

Don't we already have scope stored? It seems bad to have both.
Comment 9 Takashi Sakamoto 2013-03-11 02:51:12 PDT
Comment on attachment 191660 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/SelectorChecker.h:83
>> +        const Node* contextualReferenceNode;
> 
> Don't we already have scope stored? It seems bad to have both.

If only scoped style uses "scope", I will use. However, the scope is also used by :distributed. So I would like to confirm whether we can use ":distributed" in element.querySelectorAll or something. If so, scope value set by distributed is the same as contextual reference node...? I will add hayato@ to CC.
Comment 10 Takashi Sakamoto 2013-03-11 03:40:30 PDT
(In reply to comment #9)
> (From update of attachment 191660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191660&action=review
> 
> Thank you for reviewing.
> 
> >> Source/WebCore/css/SelectorChecker.h:83
> >> +        const Node* contextualReferenceNode;
> > 
> > Don't we already have scope stored? It seems bad to have both.
> 
> If only scoped style uses "scope", I will use. However, the scope is also used by :distributed. So I would like to confirm whether we can use ":distributed" in element.querySelectorAll or something. If so, scope value set by distributed is the same as contextual reference node...? I will add hayato@ to CC.

I found the following in http://www.w3.org/TR/selectors-api2/
"Authors are advised that while the use of pseudo-elements in selectors is permitted, they will not match any elements in the document, and thus would not result in any elements being returned. Therefore, authors are advised to avoid the use of pseudo-elements in selectors that are passed to the methods defined in this specification"

So querySelector with ::distributed returns no results. I will try to re-use "scope".
I have to add a new behavior to SelectorChecker::BehhaviorAtBoundary.
Comment 11 Takashi Sakamoto 2013-03-11 21:14:03 PDT
Created attachment 192630 [details]
Patch
Comment 12 Dimitri Glazkov (Google) 2013-03-11 21:34:49 PDT
Comment on attachment 192630 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.h:51
> +    enum BehaviorAtBoundary { DoesNotCrossBoundary, CrossesBoundary, CrossesScopeNotTreeScope };

StaysWithinTreeScope?
Comment 13 Takashi Sakamoto 2013-03-11 23:03:08 PDT
Created attachment 192645 [details]
Patch
Comment 14 Takashi Sakamoto 2013-03-11 23:03:55 PDT
Comment on attachment 192630 [details]
Patch

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

>> Source/WebCore/css/SelectorChecker.h:51
>> +    enum BehaviorAtBoundary { DoesNotCrossBoundary, CrossesBoundary, CrossesScopeNotTreeScope };
> 
> StaysWithinTreeScope?

I see. Done.
Comment 15 Dimitri Glazkov (Google) 2013-03-12 09:08:25 PDT
Okay. I'll let you land it when you're awake :).
Comment 16 Dimitri Glazkov (Google) 2013-03-12 09:09:39 PDT
FWIW, I think that BehaviorAtBoundary enum looks slightly overloaded now. The difference between DoesNotCrossBoundary and StaysWithinTreeScope is too subtle.
Comment 17 Takashi Sakamoto 2013-03-13 01:32:58 PDT
(In reply to comment #16)
> FWIW, I think that BehaviorAtBoundary enum looks slightly overloaded now. The difference between DoesNotCrossBoundary and StaysWithinTreeScope is too subtle.

I see. I would like to try this issue in another patch.
Comment 18 WebKit Review Bot 2013-03-13 02:11:59 PDT
Comment on attachment 192645 [details]
Patch

Clearing flags on attachment: 192645

Committed r145691: <http://trac.webkit.org/changeset/145691>
Comment 19 WebKit Review Bot 2013-03-13 02:12:04 PDT
All reviewed patches have been landed.  Closing bug.