Document-level click event listening breaks inside web components

[thibaudcolas]

I believe we’ve identified a bug in v8.0.0 when a dialog is within a web component, due to the changes in #387 to support #367. When processed at the document level, the click on a data-a11y-dialog-hide element will be reported with the web component as the event target, rather than the data-a11y-dialog-hide element or one of its descendants.

Here is a minimal demo to reproduce the issue: a11y-dialog v8.0.0 custom elements event delegation – CodePen. In the import map, I added the URL to the v7.4.0 release to confirm that it does work.

---

I can see three possible ways to fix this, though my understanding of event delegation with shadow DOM isn’t very good so there could well be better options.

1. Allow providing a "root" to the A11yDialog constructor to configure where click event listening happens. The default could be the document, while web components implementations could provide the shadowRoot.
2. Make this "listen to click events everywhere all the time" behavior optional, opt-in or opt-out.
3. Switch from event.target to something like event.composedPath()[0], otherwise keeping the logic the same.

From my perspective the most compelling is option 2, or any alternative that also limits the amount of unnecessary click event processing. Three Element.closest calls per click on the page might not seem like a big deal but I would assume it’s very common to have A11yDialog instantiated multiple times (once per component that can trigger its own modal), on pages where there are other interactive elements. In our case I can foresee situations where we’d get close to 1000 unnecessary Element.closest calls per minute while users interact with our app due to click interactions on pages with multiple instances of A11yDialog.

[KittyGiraudel]

Hello Thibaud,

Thank you very much for opening an issue, and my apologies you faced some issue when using a11y-dialog. I have been trying to reproduce the problem with the CodePen you shared, but I get this.shadowRoot is null:

Could you help me reproduce it? Then I’ll try to address it. :)

[laymonage]

Hi @KittyGiraudel, that error was caused by the use of the non-standard shadowrootmode on the <template> element that's only available on Chromium browsers.

I've refactored the codepen to use the standard approach of explicitly calling attachShadow():

• The reproducible bug in v8 is here: https://codepen.io/laymonage/pen/VwqGEBR
• With v7, where the bug doesn't occur: https://codepen.io/laymonage/pen/NWeLOER

Hope this helps. Thanks!

[KittyGiraudel]

Alright, I opened #589. I would love some review on it. :)

[laymonage]

Thanks @digicase but that sounds like a Wagtail issue and not an a11y-dialog issue, so it's best to post it in Wagtail's issue tracker e.g. wagtail/wagtail#10924.

And thank you very much @KittyGiraudel for the fix, I can't review it atm but I've asked my friend to review it if he has time. Hopefully he'll get to it soon!

[gavmck]

Alright, I opened #589. I would love some review on it. :)

LGTM! We are coming across the same issue using a11y dialog in web components when there are multiple dialogs

[gavmck]

I managed to get around this by setting a11y dialog to look at my host element in my modal component. That way the aria-modal attribute is attached at the light dom level and the event bubbling works.

[maddesigns]

Ran into the same issue… we could benefit from the fix. Do you see any soon merge attempt of #589?

[KittyGiraudel]

Hey Sven! If you can confirm that the fix is working fine, I would be happy to release it. I never had any confirmation from anyone that this was indeed a decent fix, so I didn’t dare publishing it. :D

[maddesigns]

I’ll try to test the fix, but maybe this needs some time… this error was reported to me where a team packed our components into a WC... I have to set up a test case.

[KittyGiraudel]

Gentle poke here. If anyone who experiences this issue wants to try out the fix from #589 (perhaps by manually modifying the code in node_modules or something like that) and confirm it works fine for your case, I’ll happily release it. :)

[maddesigns]

Sorry, I’m sad, that I couldn’t test this out yet. :(
As I said, we are using the a11y-dialog in our design system and provide components. A team packed our component into a WC and had this kind of error. I didn’t had the time to reproduce the setup on my own. The team is using another modal dialog now :/

If someone is faster and smarter to set up a test case, I’ll appreciate it.

[KittyGiraudel]

Merged, but not yet released.

[KittyGiraudel]

Done in version 8.1.0. 🎉 I know it took forever, but I’m pretty confident with the fix now. I’ve also added more tests, so this is quite good. I’m looking forward to hearing your thoughts when y’all have tried it. :)