[ShadyDOM] Add `ShadyDOM.querySelectorImplementation` setting. #517

bicknellr · 2022-07-29T23:46:55Z

This PR splits the selectors passed to querySelector and querySelectorAll, then traverses the logical tree manually to match them. This fixes :scope by allowing Shady DOM to decide if the candidate elements being tested against simple selectors containing :scope are the context element and should therefore match.

~~Only walk ancestors and inclusive descendants of the context element.~~
Iterate simple selectors in reverse to avoid walking large trees for descendant combinators.
Fix the result order by matching all complex selectors simultaneously.
Add a setting that allows switching between the current implementation, one which calls directly into the native functions, and this implementation, along with documentation explaining why this is configurable and the tradeoffs of each.
Add tests.
Add tests specifically for behavior around selector lists with multiple, top-level complex selectors. (querySelectorAll result ordering, parsing)

Fixes #480.

…lectorAll` wrappers.

… context element.

…te is the context element.

…anually.

…s for the descendant combinator.

…doc examples. (...) The formatter wants to format HTML snippets, but it keeps putting the comments in the examples on their own line, which makes it harder to tell what element they're referring to. To avoid this, I've stopped marking these snippets as HTML.

sorvell

Looking really good. We need:

3 modes: (a) existing impl, (b) native QSA + filtering, (c) this impl.
tests for each mode highlighting what works/fails in each.

sorvell · 2022-08-02T18:25:21Z

packages/shadydom/src/patches/ParentNode.js

+ * @return {!Array<!Element>}
+ */
+const logicalQuerySelectorList = (contextElement, selectorList) => {
+  return deduplicateAndFilterToDescendants(


What about just doing Array.from(new Set(arrayWithDups))?

I think this comment was on an older commit, but yeah this is now how the final deduplication is handled.

sorvell · 2022-08-04T01:58:48Z

packages/shadydom/src/patches/logicalQuerySelectorAll.md

@@ -0,0 +1,204 @@
+`logicalQuerySelectorAll` emulates `querySelectorAll` within Shady DOM's logical


This is awesome. Great write up.

sorvell · 2022-08-04T02:12:45Z

packages/shadydom/src/patches/ParentNode.js

+        combinators,
+      };
+    })
+    .filter(({compoundSelectors}) => compoundSelectors.length > 0);


For what case is this filter needed?

This probably isn't necessary. I just haven't tested how the library responds to all whitespace / the empty string yet.

sorvell · 2022-08-04T02:19:46Z

packages/shadydom/src/patches/ParentNode.js

+          const {compoundSelectors} = complexSelectorParts;
+          const index = compoundSelectors.length - 1;
+          if (matchesCompoundSelector(element, compoundSelectors[index])) {
+            return [


Why does this need to be an array?

This probably doesn't need to be.

Removed this one and a few others.

sorvell · 2022-08-04T02:22:45Z

packages/shadydom/src/patches/ParentNode.js

+            ancestor && ancestor instanceof Element;
+            ancestor = ancestor[utils.SHADY_PREFIX + 'parentNode']
+          ) {
+            if (matchesCompoundSelector(ancestor, compoundSelector)) {


It wasn't obvious to me why results would ever be an array so maybe this should be commented, but the reason is that there may be > 1 candidate in the ancestor tree...

e.g.: given c c c d and <c><c><c><d>: the first cursor will be for d and next we need a cursor for each c, but eventually the one for the top most c will survive. Is that right?

Yeah, each ancestor matching c needs a cursor because any of them could potentially be part of the match when it's only gotten around to testing the rightmost c in the selector. The cursors that point at the shallower <c>s will eventually be unable to find enough candidate <c> ancestors to match the rest of the selector and will be replaced with zero cursors.

That particular case will look something like this:

<c> <c> <c> <d> 

<c>  <c>  <c>  <d>

<c>  <c>  <c>  <d>

During this step, cursor[2] doesn't find any candidates:

<c>  <c>  <c> <d>

<c>  <c>  <c> <d>

During this step, cursor[1] doesn't find any candidates:

<c>  <c> <c> <d>

<c>  <c> <c> <d>

The cursors that point at the shallower <c>s will eventually be unable to find enough candidate <c> ancestors to match the rest of the selector and will be replaced with zero cursors.

Or, maybe I should have said "Eventually, some of the cursors with 'algorithmic ancestor cursors' that pointed at the shallower <c>s will be unable..." since only cursors that have already succeeded in making a complete match match actually 'survive' between iterations - all others are replaced with zero or more new cursors. The overlapping tree / lineage terminology that's shared here between the DOM and the timeline of cursors makes describing this a bit difficult. :/

romainmenke · 2022-08-07T08:23:56Z

packages/shadydom/src/patches/css-selector-splitter.js

@@ -0,0 +1,140 @@
+/**


This package doesn't handle escape sequences :

console.log('a\\,b,c', splitSelector('a\\,b,c')) // a\,b,c { selectors: [ 'a\\', 'b', 'c' ], joiners: [ ',', ',' ] }

console.log('a\\",b,c', splitSelector('a\\",b,c')) // a\",b,c { selectors: [ 'a\\",b,c' ], joiners: [] }

This should work better : https://github.com/mrhenry/core-web/blob/main/packages/core-web-generator/polyfills/%7Eelement-qsa-has.js#L270-L378

…n for IE 11 compatibility.

…orEngine'`.

…ings `ShadyDOM` global object.

…elector`.

…der.

sorvell · 2022-09-06T18:53:17Z

packages/shadydom/src/patches/ParentNode.js

+  const matchesCompoundSelector = (element, compoundSelector) => {
+    return (
+      utils.matchesSelector(element, compoundSelector) &&
+      (compoundSelector.indexOf(':scope') === -1 || element === contextElement)


This check is likely cheaper, do it first?

sorvell · 2022-09-06T18:55:24Z

packages/shadydom/src/patches/ParentNode.js

+   * - `complexSelectorParts` is the decomposed version of the selector that
+   * this cursor is attempting to match.
+   *
+   * - `position` is an element that matches a compound selector in


Nit: position referring to an element is a little odd. Some suggestions: current or cursor or matchedElement?

SGTM, switched to matchedElement.

sorvell · 2022-09-06T19:00:13Z

packages/shadydom/src/utils.js

+ * Deduplicates items in an array.
+ *
+ * This function could normally be implemented as merely `Array.from(new
+ * Set(...))`. However, in IE 11, `Set` does not support being constructed with


This is likely more expensive than doing new Set([...]). What about doing this the good way if it's supported?

There's also the Array.from issue. Do you think this is worth the complexity of checking if we're in the good case?

Think it's fine.

sorvell · 2022-09-06T21:17:40Z

packages/shadydom/src/patches/ParentNode.js

+      return utils.flat(
+        complexSelectors.map((complexSelectorParts) => {
+          const {compoundSelectors} = complexSelectorParts;
+          const index = compoundSelectors.length - 1;


Let's add a little comment here about why we're starting from the end (it's more efficient).

sorvell · 2022-09-06T21:23:16Z

packages/shadydom/src/patches/ParentNode.js

+          // `position`.
+          for (
+            let ancestor = position[utils.SHADY_PREFIX + 'parentNode'];
+            ancestor && ancestor instanceof Element;


Can we check nodeType instead of using instanceof? Should be better perf.

And/or don't we have parentElement? Can we use that?

Yeah, that should work - updated.

On another note, this question just made me realize that :host and :host-context won't work in this implementation. I don't think this is supported by the default implementation either since it also uses matches, but I've added this to the list of unsupported stuff in the docs for this flag.

sorvell · 2022-09-06T21:28:42Z

packages/shadydom/src/patches/ParentNode.js

+        this[utils.NATIVE_PREFIX + 'querySelector'](selector)
+      );
+      const root = this[utils.SHADY_PREFIX + 'getRootNode']();
+      return utils.createPolyfilledHTMLCollection(


Shouldn't this just be the matched element?

(reply in other comment)

sorvell · 2022-09-07T00:38:18Z

packages/shadydom/src/shadydom.js

@@ -118,6 +118,81 @@ if (utils.settings.inUse) {
    'nativeMethods': nativeMethods,
    'nativeTree': nativeTree,
    'patchElementProto': patchElementProto,
+    // Use this setting to choose the implementation of `querySelector` and
+    // `querySelectorAll`. The logical tree exposed by Shady DOM via the DOM


Maybe add: "The options provide different trade offs between accuracy and performance, as explained below."

I added that sentence and reworded this paragraph a bit.

sorvell · 2022-09-07T00:41:17Z

packages/shadydom/src/shadydom.js

+    //     correct. When given a selector that would not parse under a full
+    //     implementation, this implementation may fail silently.
+    //
+    //     - Only top-level combinators and compound selectors are taken into


Maybe change this to "contextual combinators are not supported"?

Yeah, this was a bit hard to describe since there's not really a standard term for this. 'Contextual combinators' is a bit confusing to me also, so I reworded it using 'nested selectors'. Do you think it's clearer after this update?

sorvell · 2022-09-07T00:55:53Z

packages/tests/shadydom/shady-dynamic.html

@@ -2906,6 +3105,352 @@
          document.body.removeChild(d);
        });
      });
+
+      suite('querySelectorImplementation', function () {


Make sure to also add asserts for querySelector in addition to querySelectorAll.

Ok, I added them for the tests for the tree-walking parts, but I left them out of the parseSelectorList tests since those are only to check if they parsed properly.

sorvell · 2022-09-07T00:56:50Z

packages/tests/shadydom/runner.html

@@ -26,6 +26,12 @@
    'shady-dynamic.html?forceCustomElements=true',
    'shady-dynamic.html?noPatch=true&forceCustomElements=true',
    'shady-dynamic.html?noPatch=on-demand&forceCustomElements=true',
+    'shady-dynamic.html?querySelectorImplementation=selectorEngine',


Are there tests for querySelectorImplementation=native?

No, I've just added those though. I had to update some of them since they were doing things that aren't supported with that option.

…atching.

…dates.

…s for this case.

…rySelectorAll`. (...) The `parseSelectorList` tests are not included since they're specifically for determining if the certain selectors are parsed as expected, rather than finding elements.

packages/shadydom/src/patches/logicalQuerySelectorAll.md

sorvell

LGTM

bicknellr · 2022-09-13T17:43:51Z

I've been looking into the Sauce tests since they were passing before and nothing meaningful happened between then and now that should have caused them to start completely refusing to run on a handful of older browsers. I see the same thing when running the full list of browsers on sauce locally but running those same browsers individually seems to work. I tried debugging a run with the full list and found that the older browsers were being served code that wasn't correctly compiled for their feature sets, so my best guess as of now is that Sauce Connect Proxy might be caching responses between different browser sessions that the single tunnel is used for, causing the older browsers later in the list to see code generated for the newer browsers earlier in the list.

Sauce Connect Proxy used to have a --no-proxy-caching flag which is now deprecated, so next I'm going to try to inject Cache-Control: no-cache into the responses.

bicknellr · 2022-09-14T00:06:45Z

I think #522 will resolve the test issue.

…tor-scope-parse

bicknellr · 2022-09-15T23:59:02Z

The test timeout probably needs to be increased; I had to restart them a few times.

bicknellr added 11 commits July 28, 2022 16:39

Add <https://github.com/perry-mitchell/css-selector-splitter>

e8441da

css-selector-parser: Make the API minification safe; use real exports.

7ed74e4

Add logicalQuerySelectorAll and use in querySelector and `querySe…

0432af9

…lectorAll` wrappers.

css-selector-splitter: Fix another minification error.

cf21398

Deduplicate results and filter to only those within the same root.

d112b07

Use Set to simplify deduplicateArray.

ed4caf3

Fix attempts to call properties that are getters, not functions.

0a04a1c

Fix filtering so that results are always exclusive descendants of the…

3697e08

… context element.

Fix early returns in descendant and child combinators.

3695eeb

Fix contains call to use the Shady DOM wrapper.

b569f0b

Simple selectors containing :scope should only match if the candida…

c4edbf4

…te is the context element.

bicknellr requested a review from sorvell July 29, 2022 23:46

bicknellr changed the title ~~[ShadyDOM] Fix :scope in querySelector{,All} by traversing the logical tree.~~ [ShadyDOM] Fix :scope in querySelector{,All} by traversing the logical tree manually to match selectors. Jul 30, 2022

bicknellr added 9 commits July 29, 2022 17:38

Add a default parameter value to satisfy Closure.

bc11939

Array's flatMap is not compiled out by Closure, so implement it m…

b23f0db

…anually.

Evaluate selectors in reverse to avoid walking entire descendant tree…

028d03e

…s for the descendant combinator.

Closure isn't compiling out includes; replace with indexOf.

569d6d3

Remove utils.flatMap; use utils.flat directly.

bf1897e

Replace "simple selector" with "compound selector" where appropriate.

c0d92e9

Add documentation.

8f1ebda

Match all selectors in a selector list simultaneously.

603e00f

bicknellr force-pushed the shadydom-querySelector-scope-parse branch from e703702 to 603e00f Compare August 4, 2022 00:14

sorvell requested changes Aug 4, 2022

View reviewed changes

romainmenke reviewed Aug 7, 2022

View reviewed changes

bicknellr added 3 commits August 8, 2022 16:29

Avoid Set iterable constructor and Array.from during deduplicatio…

0b5467a

…n for IE 11 compatibility.

Add a new querySelectorImplementation setting.

9fa7976

Run all Shady DOM tests with `querySelectorImplementation === 'select…

f8706be

…orEngine'`.

bicknellr force-pushed the shadydom-querySelector-scope-parse branch from bf502b5 to f8706be Compare August 9, 2022 01:23

Add docs for querySelectorImplementation and expose on the non-sett…

8feea24

…ings `ShadyDOM` global object.

bicknellr requested a review from sorvell August 31, 2022 01:05

bicknellr added 5 commits August 30, 2022 18:09

Test native selector support using Shady DOM's stored copy of `queryS…

32075fb

…elector`.

Test that results from queries with selector lists are in document or…

59e6ce9

…der.

Use a template in a test instead of building the tree manually.

2f36c06

Use ShadyDOM.wrapIfNeeded(...) where necessary.

199a201

Add ShadyDOM.flush(); before querying in tests.

0fc78a1

bicknellr force-pushed the shadydom-querySelector-scope-parse branch from 0c81de3 to 0fc78a1 Compare August 31, 2022 22:29

sorvell requested changes Sep 7, 2022

View reviewed changes

bicknellr added 10 commits September 8, 2022 13:44

Reorder an expression to take advantage of short-circuiting.

faa3fc1

position -> matchedElement

86eb168

Add a comment explaining why selectors are iterated backwards while m…

87028ba

…atching.

Use parentElement instead of parentNode while searching for candi…

386c48c

…dates.

Note in the docs that :host and :host-context are not supported.

b5548b2

Remove an outdated comment.

ab9dda1

Fix the wrappers using the 'native' option and enable and update test…

52103e5

…s for this case.

Update a comment for clarity.

f0c953b

Update a comment for clarity.

39868fe

Tree-walking tests now check results of both querySelector and `que…

67076d5

…rySelectorAll`. (...) The `parseSelectorList` tests are not included since they're specifically for determining if the certain selectors are parsed as expected, rather than finding elements.

bicknellr requested a review from sorvell September 9, 2022 00:58

bicknellr commented Sep 9, 2022

View reviewed changes

packages/shadydom/src/patches/logicalQuerySelectorAll.md Outdated Show resolved Hide resolved

position -> matchedElement in the logicalQuerySelectorAll docs.

78d1f65

sorvell approved these changes Sep 13, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into shadydom-querySelec…

15c66c1

…tor-scope-parse

bicknellr changed the title ~~[ShadyDOM] Fix :scope in querySelector{,All} by traversing the logical tree manually to match selectors.~~ [ShadyDOM] Add ShadyDOM.querySelectorImplementation setting. Sep 15, 2022

Update change log.

8621eeb

bicknellr merged commit 4756d22 into master Sep 15, 2022

bicknellr deleted the shadydom-querySelector-scope-parse branch September 15, 2022 23:59

		@@ -0,0 +1,204 @@
		`logicalQuerySelectorAll` emulates `querySelectorAll` within Shady DOM's logical

[ShadyDOM] Add ShadyDOM.querySelectorImplementation setting. #517

[ShadyDOM] Add ShadyDOM.querySelectorImplementation setting. #517

Conversation

bicknellr commented Jul 29, 2022 • edited Loading

sorvell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

bicknellr Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

romainmenke Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell left a comment

Choose a reason for hiding this comment

bicknellr commented Sep 13, 2022

bicknellr commented Sep 14, 2022

bicknellr commented Sep 15, 2022

[ShadyDOM] Add `ShadyDOM.querySelectorImplementation` setting. #517

[ShadyDOM] Add `ShadyDOM.querySelectorImplementation` setting. #517

bicknellr commented Jul 29, 2022 •

edited

Loading

bicknellr Aug 4, 2022 •

edited

Loading

bicknellr Aug 4, 2022 •

edited

Loading

romainmenke Aug 7, 2022 •

edited

Loading