Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document-level click event listening breaks inside web components #582

Open
thibaudcolas opened this issue Sep 21, 2023 · 8 comments
Open

Comments

@thibaudcolas
Copy link

thibaudcolas commented Sep 21, 2023

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.

lb- added a commit to lb-/wagtail that referenced this issue Sep 21, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes wagtail#10924
lb- added a commit to lb-/wagtail that referenced this issue Sep 21, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes wagtail#10924
lb- added a commit to lb-/wagtail that referenced this issue Sep 21, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes wagtail#10924
lb- added a commit to lb-/wagtail that referenced this issue Sep 25, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes wagtail#10924
@KittyGiraudel
Copy link
Owner

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:
Screenshot 2023-09-30 at 14 26 56

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

@laymonage
Copy link

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():

Hope this helps. Thanks!

@KittyGiraudel
Copy link
Owner

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

lb- added a commit to lb-/wagtail that referenced this issue Oct 9, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes wagtail#10924
@digicase

This comment was marked as off-topic.

@laymonage
Copy link

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!

@digicase

This comment was marked as off-topic.

laymonage pushed a commit to wagtail/wagtail that referenced this issue Oct 18, 2023
- Remove `data-a11y-dialog-hide` and use Stimulus data action approach
- This ensures that our code is more agnostic to the third party library and can leverage our own DialogController behaviour
- Works around ally-dialog issue KittyGiraudel/a11y-dialog#582
- Fixes #10924
@gavmck
Copy link

gavmck commented May 16, 2024

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
Copy link

gavmck commented May 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants