Improve accuracy of :scope selector in the real world
#131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Need to check if the `mode` ternary is actually necessary
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, the
:scopeselector is just a syntactic transform of the:scopeselector 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.fromproperty since that appears to be the context for a given selector. This means that it should work correctly for:hassince:hasuses querySelector, which will update the context to be the node that it's called on. This could be problematic if:haschanges 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
modeternary is necessary. I copied that snippet from the:rootpseudoclass 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
jsdom/jsdom#3067 (original issue for #63)
jsdom/jsdom#2998