fix(popover): support shadow tree elements lookup

[yinonov]

querySelector looking for all [popover] attributes in a document will now pierce within
shadow trees recursively.

this introduces a use of a package query-selector-shadow-dom.

What bugs me with this solution is that it the package querySelectorAllDeep method doesn't operate from a specific element, and rather the document element, thus removes the explicit query from ownerDocument of the polyfill (might be good to raise a request for the team query-selector-shadow-dom/issues).

this probably has little effect in browsers, as, commonly, ownerDocument will always point at the document element.

would love to hear maintainers' thoughts on this...

Closes #44

issues to resolve -

• declarative shadow root doesn't trigger attachShadow method, therefore, its template content doesn't get picked up. maybe override connectedCallback instead. can the static method observedAttributes assist to trigger popover attributes change?
• css doesn't pierce in to shadowRoot, therefore not hiding popovers. should a caveat be added to state "does not work apply to shadow Dom" in
  https://github.com/yinonov/popover-polyfill/blob/8bac7a2d5b74428c75ac08696ff197747722c72d/README.md#L29-L33
  ?
• document.addEventListener('click', doesn't trigger actions inside various shadow roots
• identical IDs in different shadow roots may conflict looking up effectedPopover and return a wrong element. should relations between popover button and its control NOT bleed out of same ownerDocument?

[jgerigmeyer]

@yinonov Thanks for the contribution! I'd love to hear what @keithamus thinks about this before merging.

[keithamus]

I'm a little bit concerned looking through the query selector library, as it looks like it might cause slowdowns on pages with lots of shadow roots. It also looks like it won't work on closed shadowRoots.

Given we know we only want to interact with elements that have a popover attribute, I wonder if it would be more beneficial to track just those elements with a MutationObserver.

To use a MutationObserver we could patch attachShadow to get references to shadowRoots and add them to the observer, tracking for child elements that get the popover attribute. The problem with that, however, would be declarative shadowRoots.

[yinonov]

I'm a little bit concerned looking through the query selector library, as it looks like it might cause slowdowns on pages with lots of shadow roots. It also looks like it won't work on closed shadowRoots.

Given we know we only want to interact with elements that have a popover attribute, I wonder if it would be more beneficial to track just those elements with a MutationObserver.

To use a MutationObserver we could patch attachShadow to get references to shadowRoots and add them to the observer, tracking for child elements that get the popover attribute. The problem with that, however, would be declarative shadowRoots.

given the lack of broad support for declarative shadowRoots, can the attachShadow monkey patching and observation considered as first aid?

[keithamus]

It might be worth looking into how declarative shadow roots are assigned to see if it would significantly alter the architecture. Having said that I have a hunch it won't be easy and may require extensive modifications to multiple APIs, and so we might be better off ignoring them for now.

Closed shadow roots are only accessible via attachInternals().shadowRoot but that's not always going to be called, and simple polyfills for DSD dont polyfill attachInternals. Open shadowRoots will assign .shadowRoot but I have a hunch it will do so using Object.defineProperty which is not observable without significant workarounds such as patching defineProperty itself, or using Proxies which have a performance penalty.

I think it's quite reasonable to add "does not work with declarative shadow Dom" to the list of caveats for now. That means the only mechanism for creating shadow roots is with attachShadow which makes for a more straightforward patch.

[yinonov]

I'm up for the "does not work with declarative shadow Dom" caveat as, I believe, is not so very common yet.
boiled something up with patching attachShadow. will push soon

[yinonov]

notes to self:

• mutationObserver as its name implies, would not, eagerly, pick up [popover] elements until an actual mutation. this requires an initial query selector for [popover] elements.
• verify the following pitfall

If you call observe() on a node that's already being observed by the same MutationObserver, all existing observers are automatically removed from all targets being observed before the new observer is activated.

will any shadowRoots within shadowRoots remove parent observation?

[yinonov]

as this seem to be bigger than the initial expectation, I'd ask for a short review before moving forward.

The plan is to move new code to a class that handles all observation of document or shadowRoot.

removed the query-selector-shadow-dom.

[keithamus]

This looks like a very nice implementation. I just have one small concern about nested popovers that happen during a mutation.

[yinonov]

I keep getting Error: Timed out waiting 10000ms from config.webServer. when running npm test. any idea how to resolve? I followed the contribution guide precisely

[jgerigmeyer]

I keep getting Error: Timed out waiting 10000ms from config.webServer. when running npm test. any idea how to resolve? I followed the contribution guide precisely

@yinonov I've hit that recently too, and it seems to be inconsistent for me -- it passes once, then times out. Something in the connection between playwright and esbuild, but I haven't had time to dig into it yet.

[keithamus]

This looks really good to me now 👍

[yinonov]

I keep getting Error: Timed out waiting 10000ms from config.webServer. when running npm test. any idea how to resolve? I followed the contribution guide precisely

@yinonov I've hit that recently too, and it seems to be inconsistent for me -- it passes once, then times out. Something in the connection between playwright and esbuild, but I haven't had time to dig into it yet.

This looks really good to me now 👍

so is it possible to at least run tests in CI pre-merge?

[jgerigmeyer]

so is it possible to at least run tests in CI pre-merge?

We could merge this into a branch and allow CI to run there before merging into main...

I can't reproduce the timeout locally, only occasionally on CI. Locally, can you run npm run serve in one terminal, then comment out the webServer settings in playwright.config.ts and run npm run test:ci?

[yinonov]

webServer

thanks, that did it for now. resolving failures...

[jgerigmeyer]

I think there's a way we can modify the test workflow to run on public fork PRs, but I haven't looked into it yet. @keithamus?

[keithamus]

I think it might be this line stopping them running:

popover-polyfill/.github/workflows/test.yml

Line 6 in dc20d5a

types: [reopened]

I believe this means they’ll only run on a PR that gets reopened.

[yinonov]

facing an issue with playwright locator not supporting close mode shadowRoot

tests won't pass unless host.attachShadow({ mode: 'closed' }); is set to 'open'

we still want to ensure close shadowRoots are tested, don't we?

[yinonov]

facing an issue with playwright locator not supporting close mode shadowRoot

tests won't pass unless host.attachShadow({ mode: 'closed' }); is set to 'open'

we still want to ensure close shadowRoots are tested, don't we?

I don't see a way around this blocker unless heavily extending the testing environment. don't feel right.
'close' shadowRoot content is unreachable, as it is intended to be in browsers.

are we ok with the compromise of testing against 'open' shadowRoot?

[jgerigmeyer]

Functionality looks good! A few minor changes, then I think this is ready to merge.

[jgerigmeyer]

🚀

[yinonov]

@jgerigmeyer @keithamus thank you for all the support!

[jgerigmeyer]

@yinonov Thanks for your contribution! I'll make a new release today or Monday.

[jgerigmeyer]

Published v0.0.5