sl-dialog / Modal focusin event creates problems

ceymard commented 1 year ago

Describe the bug

When using outside libraries such as Wijmo, the fact that Modal wants to keep the focus in the dialog creates problems.

To Reproduce

Any library / component that creates popups by appending them to document.body instead of a parent node inside the dialog that are modals themselves, just not from shoelace.

I'm not sure I can post a repro since Wijmo is not open source.

The used library in this instance (Wijmo) creates a nice excel-like grid inside the dialog. Some of the cells have a Select-like widget to select values. When clicking on the value, it focuses itself. Since the select popup is appended to document.body and not to the dialog, Modal chimes in and refocuses the dialog itself.

Inside Wijmo's code, a blur is detected. The select popup is thus removed. It was not possible to select a value.

I'm not certain what should be done, honestly. But this sure doesn't play nice together.

CetinSert commented 1 year ago

Have you contacted Wijmo devs about this issue?

ceymard commented 1 year ago

I do not feel like this is "on their end", more like a "how to tackle focus handling when mixing stuff together"

claviska commented 1 year ago

This is a deeper rooted issue. A lot of libraries use "hoisting" or "portaling" to escape z-index problems by appending popovers to the <body> element. It's kind of a hack, but it was easy and necessary so lots of libraries continue to do it.

Unfortunately, custom elements don't have that luxury because moving a container outside of the shadow root means losing styles and encapsulation. It breaks the principle of separation that the shadow DOM provides. (You can work around it by componentizing the popover, but then you lose accessibility since you can't link ids across shadow boundaries.)

That said, hoisting is still necessary in 2023. The Popover API aims to solve this by allowing popovers access to the top layer. It's expected to land in Chrome 114, but I'm not sure about other browsers.

One thing the upstream library might consider is offering an alternative hoisting mechanism as a stop gap. For example, Shoelace offers a fixed positioning strategy for dropdowns, tooltips, and other components that need to "break out" of an overflow.

It's opt in and it works really well. End users activate it with the hoist attribute.

<sl-dropdown hoist>
  <sl-button slot="trigger" caret>Hoist</sl-button>
  <sl-menu>
    <sl-menu-item>Item 1</sl-menu-item>
    <sl-menu-item>Item 2</sl-menu-item>
    <sl-menu-item>Item 3</sl-menu-item>
  </sl-menu>
</sl-dropdown>

In the past, users have asked for the ability to disable focus trapping. Alas, that's not an option since modal components require it to be accessible. What I am open to is an opt-in way to tell Shoelace's modal utility that "this element is OK to receive focus when the modal is open." Perhaps something like this:

<sl-dialog id="my-dialog">
  ...
</sl-dialog>

<div class="third-party-control" data-sl-modal="my-dialog">
  ...
</div>

But I don't know how helpful this would actually be, because you'd need to be able to hook into the third-party control programmatically to add the attribute. Since most of those popovers are added/removed dynamically, the logic to do that probably won't be easy or clean to write.

If you can't use a Shoelace control in the dialog — which are designed to work within Shoelace dialogs, drawers, etc. — then you should probably open an issue upstream linking back to this one. Shoelace isn't unique here, so if they want to support custom elements, they may want to consider an alternative method of hoisting that's compatible with shadow DOM.

I'm labeling this as wontfix, but I'd be open to a clean PR that allows users to work around this using the method above or something similar. We'll definitely need to weight the cost/benefit of it to make sure it's usable for other libraries and users, though.

claviska commented 1 year ago

For anyone interested in a PR, this module is the one that controls modals. Note that it allows for nested modals, so we want to make sure to preserve that functionality.

https://github.com/shoelace-style/shoelace/blob/next/src/internal/modal.ts

ceymard commented 1 year ago

What about a way to tell sl-dialog (and anyone using Modal) to simply not use modal ? Like a no-modal-tracking attribute or something.

claviska commented 1 year ago

Alas, that's not an option since modal components require it to be accessible.

That breaks the accessibility contract and allows users to tab behind the dialog/drawer/etc.

shoelace-style / shoelace

sl-dialog / Modal focusin event creates problems #1277

Describe the bug

To Reproduce