`sl-dialog` closes when contained `sl-select` is closed

[ChristophP]

Describe the bug I have a sl-dialog which contains an sl-select component. Since the dialog listens to sl-hide events and the sl-select uses those as well the dialog closes when choosing one of the select options.

To Reproduce Steps to reproduce the behavior:

<sl-dialog open>
  <sl-select>
    <sl-menu-item value="hello">Hello</sl-menu-item>
    <sl-menu-item value="foo">Foo</sl-menu-item>
  </sl-select>
</sl-dialog>

1. Open Dialog
2. Choose an option in the select
3. Watch the dialog close

Expected behavior The dialog stays open

Desktop (please complete the following information):

• Manjaro Linux
• Firefox
• 84.0.2

Additional context As a workaround one needs to stopPropagation on all sl-selects inside the sl-dialog

However, it would be more intuitive the dialog only reacted to sl-hide events that originate from itself.

[ChristophP]

Another thought that came to my mind: It is acutally wuite surprising to me that the dialog closes based on events overall. I would expect it to trigger an event when closing but not have the event control the closing.

Analogy: Triggering a change event on an input does not change the value but setting .value does. Similarly I think setting .open on the dialog should control opening and closing, it should fire an event for other code to hook into but the event itself shouldn't cause it to close.

I hope that wasn't too confusing.

[claviska]

I can't seem to replicate this using the code sample you posted above.

Since the dialog listens to sl-hide events and the sl-select uses those as well the dialog closes when choosing one of the select options.

Are you sure this isn't a listener in your code? Dialogs emit the sl-hide event, but they don't listen for it to close. A dialog will only clause if you press escape, click the close button, click the overlay, or call hide() or set open to false.

[ChristophP]

Are you sure this isn't a listener in your code? Dialogs emit the sl-hide event, but they don't listen for it to close. A dialog will only clause if you press escape, click the close button, click the overlay, or call hide() or set open to false.

Oh, the behavior that you are describing is how I expected it to work. In my code (which is somewhat more complex than the example I posted) I had the impression that it didn't work like that. Let me double check my code again to make sure it's not something else.

[ChristophP]

Now checking, it does seem to be a handler in my code. What I was trying to do is manage the open state of the dialog in my model and open/close by setting the open property on the dialog based on the model.

For this, I added a handler listening to sl-hide on the dialog, which sets the open state to false. That one got triggered when a an sl-hide event of one of the <sl-select> fields was bubbling up.

What I actually want to know is whether someone clicked on the close button of the dialog so that I can change my state and not whether a hide event was triggered. Is listening to sl-hide the right of way doing this?

[claviska]

sl-hide is the way to go but, like any event, it can be emitted any time by arbitrary components. Try looking at the target to ensure it comes from the dialog and not a child element:

const dialog = document.querySelector('sl-dialog');

dialog.addEventListener('sl-hide', event => {
  if (event.target === dialog) {
    // sl-hide was emitted by the dialog
  }
});

[ChristophP]

I tried your suggestion and it solves the problem about the dialog closing because of select sl-hide events. So thanks for that. There's another problem that I have: Since I want to manage the open/closed state in my application I don't want the dialog to close when the close button is clicked but I only want to be notified about it so that I can change my state . To achieve this I am calling preventDefault in the handler for sl-hide. This prevents the dialog from closing but also prevents the dialog from closing when the dialog.open is set to false in the subsequent render. It just stays open and dialog.open is reporting true despite running dialog.open = false. What would you recommend if one wants to control the open/closed state externally to the dialog component?

As a side note: I use Elm, not sure if you're familiar with it but it works like a React/Redux pattern where the ui is a function of state. So to put it into those terms: How can I use the dialog as a controlled component vs an uncontrolled component?

[claviska]

I try to avoid that pattern because it requires boilerplate to make things like the close button and overlay clicks work. Shoelace components work more like HTML elements (e.g. <details>) and since open is mutable by design, you'd need to listen for and prevent three things:

1. Close button clicks
2. Escape key presses
3. Overlay clicks

That's probably not convenient, so I'm wondering if there's a way to bind your open to sl-show|hide instead of dialog.open. Otherwise, you'd have to be clever in the logic of your sl-hide handler to determine if it was emitted by a user-action or a prop change.

[claviska]

Closing since this isn't a bug, but feel free to continue the discussion if you have questions.

[ChristophP]

I see, sounds like you're suggesting to keep my application state in sync with the dialogs open state by listening to sl-show / sl-hide events. I'm pretty sure that would work. Sounds like doing it the other way would mean having to have some complex logic in the listener like you mentioned. It's just kind of the opposite of what I'm trying to do: Syncing my state to the components vs. having components react to my state. Lot's of libraries like React, Vue with Vuex or so work with this pattern to internalize this kind of state so it would be nice to have a way to follow that.

I can see how the design decisions make sense from the perspective of the component. There's just a few things that surprised me when using the component.

1. I can use the open property to open the dialog, but not really to close it , because for that I'd need to have an event that tell me "someone is trying to close" without acutally closing it.
2. When the sl-hide event is used to call preventDefault(), open is no longer writeable and even stays true when explicitly set to false.

I realize this moving pretty for away from the original issue, so I'll be happy to move this to a different issue for further discussion. For now, I am using your advice to sync my state with the component.

[claviska]

Lot's of libraries like React, Vue with Vuex or so work with this pattern to internalize this kind of state so it would be nice to have a way to follow that.

That's a different paradigm, for sure. React is top-down with immutable props. HTML doesn't work that way, though, and Shoelace tries to stick to the platform as much as possible.

Looking at <details> for example, its open state is mutable as well (and I don't believe there's a way to prevent it from toggling it). However, dialogs and drawers have more complex interactions and being able to control them programmatically is important. So that's why the sl-hide events are preventable in Shoelace.

Of course, anything with a mutable prop will conflict with an immutable paradigm, but aside from framework consistency the DX is usually better. In most cases, a <sl-dialog> "just works." I don't think it makes sense for users to have to do bind events for every dialog they use to handle a basic close operation:

<sl-dialog>
  ...
</sl-dialog>

<script>
  const dialog = document.querySelector('sl-dialog');

  // If we use a "hide" event, it can mislead ancestor elements because the user may or may not close the dialog and they may or may not cancel or stop the event from bubbling. This isn't optimal:
  dialog.addEventListener('sl-hide', () => dialog.open = false);

  // Maybe we could emit more granular events, but now there's even more boilerplate and additional events could be added later on
  dialog.addEventListener('sl-overlay-clicked', () => dialog.open = false);
  dialog.addEventListener('sl-close-button-clicked', () => dialog.open = false);
  dialog.addEventListener('sl-escape-key-pressed', () => dialog.open = false);
</script>

They should get this out of the box, which the current approach offers, and it also works properly with platform events.

I'm wondering if there's a happy medium, perhaps a thin wrapper or utility that makes components such as these more immutable-friendly for React et al. 🤔

[ChristophP]

I just some research on the HTML native dialog element and just thought I'd share my thoughts with you here:

One difference to sl-dialog I noticed is that you can't close it without adding a handler. I know you mentioned that users shouldn'y have to add handlers to do a basic close operation and I agree that this is desirable. On the other hand, I thought that you probably need JS anyway to open the dialog, so maybe it's not too much to ask from the user to add another line to close it as well.

someButton.addEventListener('click', () => { dialog.open = true; }) // needed for opening anyway
dialog.addEventListener('sl-hide', () => { dialog.open = false; }) // would be needed additionally

Another route would be to have some property like addDefaultHandlers or noDefaultHandlers which let the user opt-in/out of having the default handlers for closing. I realize this would kind of make the API more confusing.

Another difference is that the HTML dialog has a close event which is purely informational (there's no default action which can be prevented).

Maybe an idea of how to make more immutable friendly without interfering much with the current behavior is to still allow setting open to false even when the default is prevented for sl-hide.

I know those API design decisions are not easy and it's almost impossible to make everyone happy, but I just wanna say that I appreciate Shoelace components a lot 😄

[tomcatmwi]

I just ran into the same problem. It happens when I use sl-select in a sl-dialog. I think this is definitely a bug.

The sl-select will receive the click, then bubble it up, and the sl-dialog mistakenly thinks it was a click on the overlay (because it's not a click on the dialog itself).

The dialog's sl-hide event is triggered when this happens, and event.srcElement is the sl-select.

Interestingly, sl-dialog seems to ignore class="dialog-deny-close".

I am using Shoelace with Vue 3. I don't observe this behavior in vanilla Javascript.

[claviska]

Can you please post a repro? This was also mentioned today in #1307 but my CodePen works as expected.

https://codepen.io/claviska/pen/ExdgGyx?editors=1010

The only thing I can think of is that, when <sl-select> is open and you click on the dialog's overlay to close the menu, I do see that the dialog also gets dismissed because overlay clicks do that by default (unless you override that behavior).

[tomcatmwi]

Yes, it doesn't seem to occur with vanilla HTML/JS. I am setting up a new Vue project to reproduce it. I'll be back with it ASAP.

[tomcatmwi]

I managed to reproduce it without a hitch. Please take a look: https://github.com/tomcatmwi/sl-bug

[claviska]

I’m betting this is running because of the select’s event bubbling up.

https://github.com/tomcatmwi/sl-bug/blob/9d2d045e1b5d751fa982220fbb4c8d97ed363484/src/App.vue#L22

Looking at this: https://vuejs.org/guide/essentials/event-handling.html#event-modifiers

It seems like the self modifier will solve it.

@sl-hide.self="onHide"

[tomcatmwi]

I'm only using sl-hide in this demo to show that the event arrives from the sl-select, not the sl-dialog. I'm opening and closing the dialog with the open property. The modifier would solve nothing.

[claviska]

It’s hard to run it on mobile, so I’m just guessing by looking at the code. If it’s only happening in Vue though, I’m not inclined to spend a ton of time diving in. Does it happen with non-Shoelace components? Does it happen in other frameworks?

Issues like this can become a significant time sink. If anyone has any insight, it would be greatly appreciated.

[tomcatmwi]

It doesn't happen with any non-Shoelace components. In fact, I tried replacing sl-select with a regular select and it doesn't cause this problem. I can try reproducing it in React.

It is a time sink, but also for those who will encounter it. Right now I don't see a solution, and the only way to circumvent this bug is to write a whole new Vue component instead of sl-select. I think it would be way easier to stop propagation in Shoelace. Perhaps sl-dialog shouldn't accept click events that aren't coming from its own overlay?

[tomcatmwi]

I can confirm, the bug is present in React too. https://github.com/tomcatmwi/sl-bug-react

[claviska]

Both of those examples listen for sl-after-hide and set open to false.

• https://github.com/tomcatmwi/sl-bug/blob/9d2d045e1b5d751fa982220fbb4c8d97ed363484/src/App.vue#L21
• https://github.com/tomcatmwi/sl-bug-react/blob/main/src/App.js#L21

Since open is bound to the dialog, closing the select emits sl-after-hide, that event bubbles up, and your code responds to it by hiding the dialog.

In Vue, I would expect this to work:

@sl-after-hide.self="open = false"

In React, you'd need to check event.target or use any of the methods mentioned here:

https://www.abeautifulsite.net/posts/custom-event-names-and-the-bubbling-problem/

That article also describes the problem in more detail. Yes, it's a bit confusing if you haven't dealt with it before, but it's how the platform works and you'd have the same problem listening to, for example, a click event.

[tomcatmwi]

Correct, adding .self made it work in Vue. Thank you!