Focus trap in 6.0.0 less strict than before?

[yellowsunday]

Hi, first, thanks for all your work on this library!

The normal 'trap' behaviour i.e. tabbing through the dialog and cycling through the elements all works as expected - my question is about an edge case.

I previously used version 5.3.1 combined with the modifications in #95 to allow tabbing into popper.js dropdowns that overflowed the dialog's bounds.

I'm now trying out version 6, and was surprised to find the change to _maintainFocus is no longer necessary for my scenario. I can use JS to focus on elements outside the dialog container (in my popper dropdowns) and the focus trap doesn't notice anymore.

I would be delighted not to have to maintain a fork, but thought I'd better check first that this was an intentional change :)

It seems to be due to the new condition introduced for nested dialogs:

if (
    this.shown &&
    !this.container.contains(event.target) &&
    dialogTarget === this.container.id
  )

The focusable elements in my dropdown don't have the dialogTarget at all (from 'data-a11y-dialog-show' attribute), so the trap doesn't reset the focus.

Thanks!

[KittyGiraudel]

Hello!

So this was introduced with #141, particularly this commit b798b34#diff-ac01aecb43fba3ac92cc0b3796c637d1b9ad371e25fe4fda051afcfdcddb3eadR332. This solution was originally suggested in #80, and completed by myself in #141.

I must say I am not sure I fully grasp the condition right now (it’s been a long day), but it doesn’t seem to make too much sense. I think this is a mistake that was introduced in order to provide support for nested dialog. I need to spend some time on it to see if that’s really how it should work but my gut feeling is telling me that no. It should not.

Stay tuned, I’ll try to find some time to check!

[KittyGiraudel]

Alright, I think this is how it should work: #175. I think I’ll issue this fix on v7 (currently on the main branch), and a similar one for v6 (the selector will be different because v6 doesn’t use aria-modal="true") released in 6.1.1.

What do you think?

[yellowsunday]

That makes sense to me. I tried out your approach locally with 6.0.0 and the focus now gets trapped as before, while the nested dialogs still seem to behave well.

I can also still add a workaround to exclude my outside-dialog elements in the same way, e.g. && !event.target.closest(".modal-focus-trap-ignore").

Thanks!

[KittyGiraudel]

What I’m not opposed to do in v7 (v7.1.0 I‘d say) is to add an escape hatch so you don’t have to maintain a fork. Something like you mentioned:

&& !event.target.closest("data-a11y-dialog-ignore-trap")

We could add some documentation (with cautious warnings) and maybe even an explicit case like yours where it’s needed?

[yellowsunday]

That would be brilliant :) If it helps, here's a very simple working example: https://codesandbox.io/s/a11y-dialog-with-popup-forked-jn3fl

I've added a button inside the dialog that toggles a popup. It includes some very basic focus control between the popup and the toggle button to give an idea of how I'm using this.

Designs using this have included informational popups, select-dropdowns, and a complex datepicker UI, all of which can end up needing to escape from the bounds of what could be a smaller dialog. Another reason is that my dialog content might need to scroll vertically - it's not nice UX, but when writing a generic component I have to account for the possibility - and a popup/dropdown looks and feels very strange trapped inside a scrolling region.

[KittyGiraudel]

I released v7 earlier (7.0.1 actually). I’m going to release #175 in 7.0.2 today or tomorrow. And I will check how to backport the fix onto v6 as well as discussed yesterday. Once all of this is done, we can check how to do the workaround for 7.1. :)

[yellowsunday]

That's great - and an excellent reason for me to update to 7 :) I like the new simplified instantiation, that makes it a lot easier to use across different sites without needing to worry about where the main content lives.

[KittyGiraudel]

Fixed in v6.1.1 and v7.0.2! 🎉

If I could ask you to open a new issue with an explanation of what problem you face with that method and how you’d like to approach it, that would be great, @yellowsunday. This way we can draft some documentation update as well for people who run into similar problems. :)