dperini
Improve accuracy of `:scope` selector in the real world
Description
Currently, the :scope selector is just a syntactic transform of the :scope selector into the element's name, first class, and id. This sometimes works in real-world situations and generally well on the Web Platform Tests since many elements under test have ids, but in the real world, it causes inaccurate behavior. See fixed issues.
This change uses the Snapshot.from property since that appears to be the context for a given selector. This means that it should work correctly for :has since :has uses querySelector, which will update the context to be the node that it's called on. This could be problematic if :has changes to execute the querySelector on the parentElement, but I suppose that's a later problem.
One thing I'm not sure about is if the mode ternary is necessary. I copied that snippet from the :root pseudoclass and am not entirely sure why it needs to exist there.
Tests
Some wpt tests that were passing are now failing.
Element.closest with context node 'test4' and selector ':scope'
Element.closest with context node 'test4' and selector 'select > :scope'
Element.closest with context node 'test4' and selector ':has(> :scope)' (failing due to bug in :has)
Some of the has-argument-with-explicit-scope that were passing are now failing and vice versa, with 9/13 passing.
I do need some help fixing those since I'm struggling a bit. The previous implementation worked on them since it cheated by using element ids.
The test/scope tests are all passing still.
Should I add some of the reference cases that I used from the linked issues? Where would I add them as regression tests?
Fixed issues
nwsapi
#106 (classname escaping was never added)
jsdom
https://github.com/jsdom/jsdom/issues/3067 (original issue for #63) https://github.com/jsdom/jsdom/issues/2998
Any plans to get this merged in soon? Our tests are currently failing on the following selector :scope > [slot="test"], and this PR addresses the error.
@jablonnc Does "failing" mean returning the incorrect result or throwing an uncaught exception? And does this branch solve that problem when you manually install it?
The the approach in this branch mostly works, but the problem is that it relies on s.from from the internal selector context, which is overridden on each call to any of the internal selection methods. I'd need to add an additional property to said context that is only set when the user-facing Element APIs are called.
@KindaOK I noticed that the 3 wpt test that are still failing are those having :scope at the end (right mangled selectors), so I tried to resolve them in a different way following the older replace attempt, starting with "id" and "class" first then with the '*' as a fall-back and now I have all the 29 closest test passing. Now 3 failing tests remains in "has-argument-with-explicit-scope.html" but I believe those should be handled in the :has() resolvers, probably checking the RE for slipping on class selectors.
@dperini The problem with id fix is that it will always pass wpt tests because every element has an id. That's why the previous implementation of the :scope selector passed almost all of the wpt tests, but would not work consistently in real-world environments that don't put ids on every element.
I think Snapshot.from has problems because any internal calls such as Snapshot.match will change Snapshot.from even though it should always be the element querySelector or whatever was originally called on, so I think we'd need something like Snapshot.scope that is only set on the user-facing methods such as querySelector and then only use Snapshot methods internally. I don't even know where to begin for such a refactor and that's sapping most of my motivation to work on this.
As for the :has problems, I have no idea what's going on there from a quick look.
@KindaOK thank you for your precious help. I added more :scope pseudo-class tests to ensure functionality. These changes were applied in different commits and the code changed enough to render this PR inapplicable. So I am closing this pull request.