Some uses of :has throw "not a valid selector"

[aayla-secura]

I found two particular use cases of the :has selector, which work fine in modern browsers but nwsapi throws an error:

• div:has(>div) results in '>div' is not a valid selector (so having > at the start of the subselector string is treated as invalid)
• :has(div) results in '' is not a valid selector (so having :has at the start of the selector string seems to result in it being parsed as empty)

I believe the selectors above are valid and should be supported, am I wrong?

I should say I'm using version 2.2.12

[cee-chen]

+1ing this, the following selectors also throw "not a valid selector" errors for us in Jest (works fine in browsers):

Previous sibling selectors:

&:has(+ .classA) {
  color: red;
}

Nested :has/:is:

*:has(*:is(.classA, .classB)) + *:is(.classC, .classD) {
  color: red;
}

[BAISTM]

@cee-chen The previous sibling selectors work with the version 2.2.7 of nswapi.

[sanpoChew]

it seems to be this MR that caused the regression #98, I can install the latest version and then revert that line and the issue is no longer there

[gmarinov]

nwsapi/src/nwsapi.js

Line 1025 in ee69b43

source = 'if(s.match("' + referenceElement.replace(/\x22/g, '\\"') + '",e) && e.querySelector("'+ expr.replace(/\x22/g, '\\"') +'")){' + source + '}';

but with :has(> ...), the e.querySelector("'+ expr.replace(/\x22/g, '\\"') part needs :scope > ...

[dperini]

Commit 1a3a5c5 (0094625 was a wrong commit reference, sorry) is an attempt at fixing the handling of mangled selectors as argument to the :has() pseudo-class.

Please test & report possible issues. Thank you.

[cpmotion]

Hi @dperini. Thanks for looking into this. We tested your change and @gmarinov's together and separately. For our use case, which is essentially the same as the OP's(:has(> img)), @gmarinov's :scope change alone solved the failures:

- source = 'if(s.match("' + referenceElement.replace(/\x22/g, '\\"') + '",e) && e.querySelector("'+ expr.replace(/\x22/g, '\\"') +'")){' + source + '}';
+ source = 'if(s.match("' + referenceElement.replace(/\x22/g, '\\"') + '",e) && e.querySelector(":scope '+ expr.replace(/\x22/g, '\\"') +'")){' + source + '}';

Yours alone in 0094625 did not, we still had the same invalid selector errors. Not sure if that addresses other issues though. Thanks!

[dperini]

@cpmotion
thank you for reviewing ongoing changes ...
The commit reference in my previous message above is wrong, also the description in commit 0094625 is correct.
It should have referenced @gmarinov's suggested change, that I have delayed/postponed due to doubts I have.

While testing I realized that maybe ":scope " should only be added in case of mangled selectors arguments, not unconditionally. I may be wrong so your comments and those of other developers are welcome.

Also adding "* " instead of ":scope ", when the argument to :has() is a mangled selector, will yield the he same results.

@gmarinov thank you for your help and suggestions. Your comments are welcome too and much appreciated.

[gmarinov]

Also adding "* " instead of ":scope ", when the argument to :has() is a mangled selector, will yield the he same results.

i think * won’t work for more complex trees inside the element the mangled selector is scoped to, hence :scope as we’re looking only for a top level element.

indeed, :scope may not always be the right solution for :has(), i just wanted to add a data point to the cause of the issue. the idea of adding it conditionally sounds more appropriate depending on the mangling logic

[KindaOK]

For the :has(div) problem (same issue as in #120), that's because of the below segment that assumes that referenceElement is always a selector when it can sometimes be an empty string.

nwsapi/src/nwsapi.js

Lines 1023 to 1025 in 0094625

	referenceElement = selector_string.split(':')[0];
	expr = match[2].replace(REX.CommaGroup, ',').replace(REX.TrimSpaces, '');
	source = 'if(s.match("' + referenceElement.replace(/\x22/g, '\\"') + '",e) && e.querySelector("'+ expr.replace(/\x22/g, '\\"') +'")){' + source + '}';

Not sure how to make it work with your recent changes, but if we're working off of the linked commit, the fix could be something like below

source = 'if(' + (referenceElement ? 's.match("' + referenceElement.replace(/\x22/g, '\\"') + '",e) && ' : '') + 'e.querySelector("'+ expr.replace(/\x22/g, '\\"') +'")){' + source + '}';

That gets us passing results for a good chunk of the has-basic test from web platform tests.

I've been working on this problem myself and adding :scope with an implicit descendant selector doesn't fix the issue. It's over here https://github.com/KindaOK/nwsapi/tree/fix-has-selector

I manually pattern match on the relative selector if present and then change the behavior. This works for simple relative selectors, but more complicated ones don't work properly since they aren't established in the right context. I'm not sure what the best approach to make a fully-working version is, but my change would work with most common scenarios for relative selector lists.

Test results
has-basic 17/18 (all except subsequent-sibling ~ selector which I haven't added a proper implementation for)
has-relative-argument 19/35
has-argument-with-explicit-scope 12/13

[dperini]

@KindaOK
thank you so much for the contribution.
I confirm that your code is almost complete regarding the minimal :has() support (for WPT basic and invalidation tests).

I am trying to add a local WPT testing to make it easy for other developer to test and suggest changes for the :has() pseudo-class. Originally the :has() pseudo-class was not supported since this selectors implementation was written in between 2014 and 2016 and at that time neither :scope nor :has() was part of the w3 specifications.

Actually the previous selector engine implementation was named "nwmatcher" and included a similar partial implementation of mangled selectors resolution see here by adding left/right context to mangled selector strings:

https://github.com/dperini/nwmatcher/blob/master/src/modules/nwmatcher-shortcuts.js

this "shortcut" module was added about 12 years ago to the "nwmatcher" selector engine project but might still be useful in the current context to make the implementation compatible with the new specifications.

I will try to keep this conversation updated until we can ship these new resolver, maybe not yet fully completed but still very useful looking to other developers interest.

Again, your work and help is much appreciated.

[dperini]

@KindaOK
here is the state of my tests proceedings using your suggestions:

[KindaOK]

@dperini Thanks for the response. I've pushed changes to my fork that get me the following test results.
has-basic 18/18
has-argument-with-explicit-scope 12/13
has-relative-argument.html 20/35

I took a look at your shortcut matcher, but the problem with it is that it changes the DOM by adding an id to the starting element. It did work though (at least for the + next sibling combinator, I'd assume the others would too).

The only correct solution that I can think of at the moment would be to use something like querySelectorAll with an adjusted relative selector either on the current element (for > and ) or on it's parent (for ~ and +) and then walking up the parent tree until we find an element with the same identity as our current element. I think that would be a slow approach, but :has is known to be slow.

[dperini]

@KindaOK
what you suggested seems to be adequate for the task.
So let's get a reference to the current :scope element using "querySelector" via the existing makeref() function and without messing with the"id" or "class" which might exist already.

[KindaOK]

@dperini My thought was to not use :scope or makeref at all because neither of those work generally for elements without a distinct first class or id. Issue #96 has some more details. One approach I tried earlier was adding :scope to all of the relative selectors, but it didn't work for all the test cases—and the w3c tests are the ideal scenario for them since everything has an id. I might've done something wrong though since using element's ids gave some promising results. Will need to give that a go again.

Using querySelectorAll and walking up the parents of the matches until we get a parent that's the original element we started at at the appropriate depth could work for > and . The descendant selector is easy since we can just walk up, but the child combinator will be a lot harder since we don't know how deep the final matching elements we get from querySelectorAll (cases like :has(> div > div > [data-x=">"])).

The same approach with querySelectorAll could also work for the sibling combinators: call querySelectorAll on the parent, walk up on matches until we get to a node with a parent equal to the starting parent, then peek back for the original element. The problem of course is a selector like :has(+ .a + .b + .c) since querySelectorAll would give us the final .c, meaning we'd have to parse out the selector ahead of time to determine how many siblings we need to look back, which is probably a horrible route to go down.

I need to try some POCs to see if any of these could actually work now that I'm writing this down because my thoughts are not seeming promising.

[KindaOK]

@dperini So it looks like the Chrome team did some work on :scope and :has interactions in the past. They reference absolutizing (what we tried with adding :scope in from of the relative selector for :has), but that had issues. It looks like the implementation also explicitly handles the different types of relative combinators, which means we will likely need to do the same. I'll be taking a look at how they handle relative selectors since my experiments from the above post have had their expected results, but completely break relative > for simple cases. It looks like Chrome tracks distance traveled during :has. (portion of the Chrome PR linked from the csswg issue)

It inspired some experimentation, and I've submitted a draft PR #131 to fix :scope to work correctly when nodes are not identifiable based on their name/classname/id. It's failing some of the wpt tests, but passing most other existing scope tests and it fixes the problems that some other people were having with :scope in jsdom.

[dperini]

@KindaOK great help and info above. Thank you for your time.
I also believe the arguments to :has() would need to be treated as specific cases, differently by different combinators.
However I am tempted to give a try at not do that and instead treat those arguments as standard nested selectors and use the current combinators resolvers as if they were normal nested seectors without engaging the loop inside the :has() resolver itself. Maybe we could give good use to a couple of hidden functionality already embedded in nwsapi, the ability to pass the scoped arguments down the resolver chain coupled to a callback for every matching element.

Hmm ... Rereading the above I am not sure to be able to explain well enough such complexity in English.
For this reason I would prefer writing some proof code that explains this first to then discuss it here.

[dperini]

@KindaOK ....
I just realized that yesterday you found out about Snapshot.from.

This change uses the Snapshot.from property since that appears to be the context for a given selector.

this is very helpful in nwsapi. ;-)