Animations "leak" into nested drawers

[grncdr]

Describe the bug

To Reproduce

Steps to reproduce the behavior:

1. Have nested sl-drawer elements:

   <sl-drawer>
     <!-- content etc. blah blah -->
     <sl-drawer>
       <!-- more content -->
     </sl-drawer>
   </sl-drawer>

2. Add an sl-request-close event handler to the parent drawer that prevents it from being closed.

3. Open both drawers

4. Add a breakpoint on this line

   shoelace/src/components/drawer/drawer.ts

   Line 138 in a2e58b7

   animateTo(this.panel, animation.keyframes, animation.options);

5. Press Escape

Note that the "child" drawer is animated when closing of the "parent" drawer is prevented.

Demo

CodePen: https://codepen.io/grncdr/pen/RwqMOOe

The code pen also illustrates how I ended up here: I wanted drawers to "stack" so that pressing Escape would only close the most recently opened drawer.

I would be happy to "fix" this bug by integrating the drawerStack concept directly into the sl-drawer component, so that Escape only attempts to close the "top" drawer.

Browser / OS

• OS: OS X Ventura
• Browser: Firefox 115.0.2, Safari 16.5.1

Note: the visual issues are much worse on Firefox.

Additional information

Provide any additional information about the bug here.

[claviska]

I would be happy to "fix" this bug by integrating the drawerStack concept directly into the sl-drawer component, so that Escape only attempts to close the "top" drawer.

Thanks for posting this. I'm wondering if we can stop propagation instead, the challenge being the listener is currently on the document (maybe it shouldn't be).

[grncdr]

Thanks for posting this. I'm wondering if we can stop propagation instead, the challenge being the listener is currently on the document (maybe it shouldn't be).

I've updated the code pen to call stopPropagation() in the case we want the drawer to close, but it doesn't change the observed behavior.

I think the challenge there is that the "parent" drawer fires an sl-request-close event before the child drawer. (Presumably because it started listening to the Escape key first).

[grncdr]

Ah sorry, I realize now you probably meant the 'keydown' listener and calling stopPropagation() on that event.

It seems like that's already what Shoelace is trying to do here:

shoelace/src/components/drawer/drawer.ts

Lines 155 to 156 in a2e58b7

	event.stopPropagation();
	this.requestClose('keyboard');

but if it's receiving that event on the document then there's no propagation left to stop. Possibly moving the listener to the base (or overlay) would fix it?

I'm not yet set up to actually develop Shoelace itself, but I could take a stab at it tomorrow if you think that's a viable approach.

[claviska]

but if it's receiving that event on the document then there's no propagation left to stop. Possibly moving the listener to the base (or overlay) would fix it?

Yeah, exactly. We may need to move it up, which should be OK since drawers are modal and focus should technically never be outside of them. It's worth a try.

[grncdr]

It looks like the event handler used to be bound on the base <div> of the drawer but was moved to a document level listener in 5eeb98d which was a fix for #925

However, #925 was about scrolling and the issue with Escape is not really mentioned outside the changelog. I'll set up my dev env tomorrow and see if moving the listener seems to work.

[claviska]

Good catch. Let me know if I can help in any way!

[claviska]

I managed to fix this in a much simpler way. I used the existing stack for modals to ensure only the active drawer/dialog is closed. This solves your original use case and all tests are green.

Thanks again for reporting this!

[grncdr]

Nice, I’m glad it worked out in a simpler way. Looking forward to the next release 🙂