Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

querySelector/All can accidentally return the scoping element #263

Closed
tabatkins opened this issue Jun 3, 2016 · 8 comments
Closed

querySelector/All can accidentally return the scoping element #263

tabatkins opened this issue Jun 3, 2016 · 8 comments

Comments

@tabatkins
Copy link
Contributor

https://dom.spec.whatwg.org/#selectors

Return the result of evaluate a selector s against node’s root using scoping root node and scoping method scope-filtered. [SELECTORS4].

Scope-filtered selectors still allow the scoping element to be returned. Thus per spec, el.querySelector("*") will return el (plus its descendants of course). However, I don't think this is intended, and it doesn't match implementations, who all omit the scoping element from the results.

You'll want to manually remove the scoping element from the results if they show up.

(Reporting this for @nox so they don't forget.)

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Jun 5, 2016

Finally this will be clarified, @annevk some times ago (~2014) I pointed to you via e-mail the same strange behaviour:

And have small question about selectors API. We have method
element.matches(): so I try test it on Firefox (suppose that
mozMatchesSelector will be the same as matches):

alert(document.body.mozMatchesSelector(":scope"))          // true
alert(document.body.mozMatchesSelector("html :scope"))  // true

and it looks like correct. But why this return null or empty collection:

alert(document.body.querySelector(":scope"))          // null
alert(document.body.querySelectorAll(":scope"))  // empty NodeList

When I read the DOM I see very similar description for this method:

"The querySelectorAll(selectors) method must return the static result
of running evaluate a selectors string selectors against the context
object."
"The matches(selectors) method must return true if the context object
is in the result of running evaluate a selectors string selectors
against the context object, and false otherwise."

and I can't determine why this is happening when read this
description. They use the same algorithm and pass the same context
object (so "running evaluate a selectors string selectors against the
context object" should return the same list). But matches() select
scoping root (return true for :scope) and
querySelector/querySelectorAll not.

In DOM for algorithm "evaluate a selectors string selectors against a
node" step 3 we have:
3. Return the result of match a selector s against node's root using
scoping root node. [SELECTORS].

So scoping root is node (context object) and from Selectors Level 4:

scope-filtered selectors
    With this method of scoping, a selector matches if the subject of
the selector is within the scope, even if other components of the
selector are outside the scope. (A scoping element is considered to be
in scope.)  <<< last in parentheses is important.

Of course this happend for all selector, not only :scope pseud-class:

alert(document.body.mozMatchesSelector("body"))          // true
alert(document.body.querySelectorAll("body"))          // empty NodeList

So descriptions in new DOM for this methods is not precisely or I did
not understand them completely?

Now I see that at that time I make more description: "I see that SELECTORS was changed and DOM was upgrade, but still sth is missing. I have some problem to add topic in mailing list for Selector, so I will try write all to you, especially if I considering only some DOM methods...." << unfortunately this new text never send to you (still don't know why), but here I again touch this "scope-filtered selectors" question.

Regardless of this, @tabatkins can you add to SELECTORS4 spec. some real small example showing difference beetwen scope-contained selectors and scope-filtered selectors because the terminology of this specification is difficult at first contact?

@tabatkins
Copy link
Contributor Author

The difference between scope-contained and scope-filtered isn't germane to this issue; both of them consider the scoping element to be in scope. The difference is just whether the entire selector has to match inside the scope (scope-contained) or if only the final selected element has to be inside the scope (scope-filtered).

@annevk
Copy link
Member

annevk commented Jun 8, 2016

So step 3 of https://dom.spec.whatwg.org/#scope-match-a-selectors-string needs to change to store the result in variable and then remove any instances of node from that variable and then return that variable?

Why does DOM need to handle this and not SELECTORS? Does SELECTORS have other callers that want this kind of behavior?

@tabatkins
Copy link
Contributor Author

Yes. It's what was (intentionally!) used for <style scoped>, and if you use a relative selector (like .query()), it gets taken care of automatically (you'll only ever select the scoping element if you do it on purpose).

It's just this case, with an absolute selector that still wants to act kinda like a relative selector, that's problematic.

@annevk
Copy link
Member

annevk commented Jun 9, 2016

Hmm, so <style scoped> is gone. And query() uses https://dom.spec.whatwg.org/#match-a-relative-selectors-string (I forgot to comment out that algorithm when commenting out query()) which passes different arguments to the algorithm. So either there are more entry points or we should adjust the algorithm a bit?

@tabatkins
Copy link
Contributor Author

Yeah, I guess you're right. Without scoped styles there's no call for scope-contained at all anymore, and you're indeed correct that relative selectors don't actually filter at all (the absolutization process just does something similar automatically).

So I guess I can simplify this to just have a single method of scoping, and fix it to automatically exclude the scoping element. You'll need to adjust your links when I rename things; I'll ping you in a bit to let you know what needs to be changed.

@tabatkins
Copy link
Contributor Author

tabatkins commented Jun 9, 2016

Okay, done. I cut the concepts of "scope-contained" and "scope-filtered", so now there's just "scoped". And scoping doesn't include the scoping element, just its descendants.

@annevk annevk closed this as completed in 1594aa5 Jun 10, 2016
@annevk
Copy link
Member

annevk commented Jun 10, 2016

Thanks!

nox added a commit to nox/servo that referenced this issue Jul 4, 2016
nox added a commit to nox/servo that referenced this issue Jul 4, 2016
nox added a commit to nox/servo that referenced this issue Jul 4, 2016
nox added a commit to nox/servo that referenced this issue Jul 4, 2016
nox added a commit to nox/servo that referenced this issue Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants