polyfills: ShadyDOM breaks querySelector polyfills

I’ve just pushed new releases generated at https://github.com/webcomponents/polyfills/commit/a75b94ed3a7d76889fb381156c2f5404d7248097 (versions in commit message). Docs for the new querySelectorImplementation setting are here: https://github.com/webcomponents/polyfills/blob/a75b94ed3a7d76889fb381156c2f5404d7248097/packages/shadydom/src/shadydom.js#L121-L195

Sorry, I hit a lot of snags while trying to import but I haven’t forgotten about this.

Option 5. Just native qSA + filter proposed here is really the only viable fix that makes sense right now.

1. If users opt-in to the change it should fix the issue except in corner cases expected to be exceptionally rare. It’s really a wash in terms of correctness: we trade not matching undistributed nodes for matching :scope + potentially other newer selectors.
2. The polyfill is in maintenance mode and we’re strongly disinclined to add new features / code to it.
3. The code already exists in ShadyDOM, the change would just be to expose it better via an option.

Makes sense; those issues were initially tied but now it seems they should be addressed separately. Let’s follow-up on the jQuery aspect of it in the original jQuery issue, https://github.com/jquery/jquery/issues/5032. I’ll migrate my questions there.

@romainmenke:

BTW, I think my characterization of option 3 in my last comment was wrong since it wouldn’t have to give up control over the candidate list assuming the wrapper would still manually walk and call matches - i.e. it would still be able to find unassigned nodes.

It looks like this issue is happening because Shady DOM’s wrappers for these methods walk the nodes in the relevant shadow root and call matches on them with the provided selector to build the list of matched nodes, which doesn’t make sense for a selector containing :scope (and a few other things, for that matter)

Can we make a list of selectors that are impacted by this?

I think I was also wrong here: after looking through https://drafts.csswg.org/selectors/, I haven’t found anything other than :scope that obviously depends on a reference element.

A full CSS parser would not be needed by the way. This code might need a bit of refinement and a bunch of unit tests but it should be ok to use. expand for snippet

True, we don’t actually need a full parser, just enough for selector lists, but we still would like to avoid this if we can since we can’t really anticipate future selector syntax. @sorvell, WDYT?

@mgol:

My proposal was to put that check in a short “support test” so that jQuery users can override it as they can do with the other support test results.

Yeah, that seems like it would give jQuery users a simple workaround at least.

Thanks for the repro! It looks like this issue is happening because Shady DOM’s wrappers for these methods walk the nodes in the relevant shadow root and call matches on them with the provided selector to build the list of matched nodes, which doesn’t make sense for a selector containing :scope (and a few other things, for that matter). I’m not the right person to ask about whether or not this is known / considered in scope for Shady DOM. Also, the comment in Shady DOM there points to https://github.com/webcomponents/shadydom/pull/210 and https://github.com/webcomponents/shadydom/pull/216, which seem relevant but I’ve only skimmed them so far.

I see that the report that jQuery got was with window.ShadyDOM = {force: true}; here’s a modified version to load just ShadyDOM instead of the whole Web Components bundle and to show the issue without the use of jQuery: https://codepen.io/mgol/pen/JjLGLZy?editors=1010

@blackjackyau what’s the reason you’re forcing ShadyDOM to apply even in browsers with native Shadow DOM support?