search

KittyGiraudel / a11y-dialog

A very lightweight and flexible accessible modal dialog script.
https://a11y-dialog.netlify.app
MIT License
2.4k stars 132 forks source link

Using data-a11y-dialog-show with web components #712

Closed ella-etch closed 1 week ago

ella-etch commented 1 week ago

Hi!

I’ve just upgraded to v8.1.0 in our web component based design system and it’s fixed a problem we were having with shadow DOM and focus trapping, so thanks 😄

However, we now have an issue when trying to use data-a11y-dialog-show on a button custom element to trigger showing the modal (which is also a custom element). It looks like the change from event.target to event.composedPath()[0] here means that we're now getting back the slot the button is in, rather than the button itself, so .show() isn’t being called.

I noticed there was a previous commit that accounted for a slot being returned and fell back to using event.target - I think reverting to that would fix the issue?

KittyGiraudel commented 1 week ago

Good morning Ella, and thank you for opening an issue! ✨

I was thinking about it on my way home last night, and I think the problem is that Element.prototype.closest does not cross shadow boundaries. So if our click target ends up being resolved as a <slot> (as you rightfully pointed out), then using .closest(..) to find the data-a11y-dialog-show ancestor doesn’t work because it stops at the shadow root.

If I am right about this, then there are two ways to address it:

  1. Find the shadow root first (by using event.target if event.composedPath()[0] yields a <slot> as we’ve suggested in the former commit), so that .closest(..) operates exclusively in the light DOM.
  2. Use a version of Element.prototype.closest that crosses shadow boundaries, so that it doesn’t just stop when reaching the first shadow root.

I think the 2nd version is better because it would work with however many nested web components, which is probably what we want (think a button inside of a card inside of a dialog or something).

I need to spend a bit of time coming up with a reproducible test case first so that I can try fixing it. :)

KittyGiraudel commented 1 week ago

Addressed in #713. Do you want to venture a review, @ella-etch? :)

ella-etch commented 1 week ago

Hey @KittyGiraudel, thanks for doing this so quickly!

I've had a look through the code and also tested it out on our project - it all looks great, so I've left an approving review 💯

KittyGiraudel commented 1 week ago

Solved in 8.1.1. Thanks!