Dismissing Tag inside Modal triggers modal close

[filipjnc]

Scale Version 3.0.0.beta.56 / 3.0.0.beta.108

Framework and version @telekom/scale-components-react

Current Behavior Dismissing a ScaleTagin a ScaleModal leads to the modal closing itself. It happens especially when the modal visibility (opened) is controlled via state.

Expected Behavior Dismissing a tag should not trigger a modal close.

Code Reproduction A minimal reproduction case of the bug, you may fork one of the available sandboxes as a base:

Scale and React18 with Wrapper Package

Desktop (please complete the following information):

• OS: Windows 11
• Browser: MS Edge 104.0.1293.47

[acstll]

Hey @filipjnc, thanks for opening this.

This is a known issue. The scale-close event from the ScaleTag bubbles into ScaleModal. We've been debating recently whether we should prefix all custom events with the component name: scale-close -> scl-modal-close or something similar. What do you think?

Until we figure this out… (because if we change it, it would be quite a breaking change) The way I would solve this is by listening to the event on the ScaleTag and calling event.stopPropagation(). I forked your sandbox (thanks for it) to test the idea: https://codesandbox.io/s/sad-austin-ieg91g?file=/src/App.tsx:240-263

I hope this helps.

[filipjnc]

@acstll Thanks for the tip, it fixes it indeed.

I don't think it's worth introducing a breaking change in the component API just for this small issue. Maybe there is a possibility to include event.stopPropagation() in the event handler of the web component itself.

Example from the material-ui Chip component:

  const handleDeleteIconClick = (event) => {
    // Stop the event from bubbling up to the `Chip`
    event.stopPropagation();
    if (onDelete) {
      onDelete(event);
    }
  };

Source code

Update: I just noticed the internals of scale are completely different to React-native UI libraries, so forget the material-ui example. I found this unrelated comment, which led me to three untested ideas:

Idea 1 Using this.scaleClose.emit(e) instead of emitEvent('scaleClose')?

Idea 2 Declaring @Event({ bubbles: false }) scaleClose; in the tag web component? This was suggested as a solution for a related issue on the ionic repo.

Idea 3 Declaring @Listen({ bubbles: false }) scaleClose; in the modal web component? Or maybe add an event listener that checks if the target is the component ref itself before it fires.

[acstll]

@filipjnc nice to know. Thanks a lot for the feedback highly appreciated.

Regarding Idea 1, emitEvent is a helper that fires both camel-case (old, scaleChange) and dash-case events (current, scale-change). We wanted to introduce the dash-case events without making it a breaking change, so this helper fires both. What would change in this case?

Regarding Idea 2 and 3, I think bubbles is no longer an option in Stencil.

However, if I understand it correctly, I think Idea 3 could work like this: we listen to scale-change on the Modal, and call stopPropagation if it comes from something else inside… But I'm not sure whether this is a solution that could be implemented generally to solve the problem globally. Plus, I just tried testing it and I couldn't make it work.

Let's keep this open and think about it a bit more. Thanks again.

[acstll]

After doubting forever, I have made an opinion based on this article.

developers must understand that many events bubble — including native ones — and they should be doing these checks any time elements are nested to avoid unexpected behaviors.

So the fix for this imho is adding a small section to the docs and keep our events as they are.