Table: `selection-change` of child components is bubbling up

[bertdeterd]

Describe the bug When you select an item for a multicombobox in the filterconfiguration for the filterbar an error is shown in the chrome console.

main.8266569b.iframe.bundle.js:2 Uncaught TypeError: Cannot read properties of undefined (reading 'reduce') at Table.handleCheckBoxChange (main.8266569b.iframe.bundle.js:2:2822830) at ComboBox._fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2016115) at ComboBox.fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2015528) at ComboBox._selectItem (2596.1f2f48bf.iframe.bundle.js:2:5986070) at H.handleEvent (2596.1f2f48bf.iframe.bundle.js:2:9865068) at List._fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2015912) at List.fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2015528) at List.onItemPress (2596.1f2f48bf.iframe.bundle.js:2:6182047) at StandardListItem._fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2015912) at StandardListItem.fireEvent (2596.1f2f48bf.iframe.bundle.js:2:2015528)

Isolated Example See filterbar in https://sap.github.io/ui5-webcomponents-react/?path=/docs/layouts-floorplans-filterbar--default-story and go to the filterconfiguration in the example, press show values and select / deselect an item in 'multbox w initial selected'

To Reproduce Steps to reproduce the behavior:

1. Go to 'example'
2. Click on 'Filters'
3. Click on Show Values
4. Scroll down to selection item 'Multbox w/ initail selected'
5. Select / deselect an item
6. See error in chrome console

Expected behavior No errors

[Lukas742]

Hi @bertdeterd

the selection-change event of the MultiComboBox is bubbling up to the Table component. This is happening for all components implementing this event. This PR will solve this issue. Until it's released you can invoke stopPropagation in the onSelectionChange handler of the MultiComboBox.

Since I'm not sure if this is the intended behavior, I'm going to forward this issue to the UI5 Web Components repo, as both components are developed there.

---

Hi colleagues,

when components which are implementing the selection-change event are used inside the Table component, the selection-change event is bubbling up, invoking the handler of the respective component and the Table as well. Could you please check if this is the intended behavior.

I prepared a codeSandbox that should give more information: https://codesandbox.io/s/ui5-webcomponents-forked-eqbtb6?file=/src/index.js

[kineticjs]

Hi @Lukas742,

Thank you very much for the codeSandbox sample and the linked PR. As I understand, you plan to merge the PR, so the problem with the filterbar error will be solved.

I can forward this issue to the colleagues for their feedback, but at this stage I'm not convinced myself that there is real need to stop the propagation, because: -- the listeners for the "selection-change" event can/should check the event target before further interacting -- I recall the notes from e.g. https://javascript.info/bubbling-and-capturing , section "Don’t stop bubbling without a need!" that explain the downside of stopPropagation and list the preferred alternatives.

Do you have a strong opinion regarding the proposal to stop the propagation or did you intend rather to get feedback from us on the issue?

[Lukas742]

Hi @kineticjs

I'm not sure if bubbling should be supported in this case, since the events are only the same by their name and I don't see the advantage of event propagation in this case. Can you think of any?

E.g. the selection-change of the table could be typed like this:

  onSelectionChange?: (
    event: Ui5CustomEvent<TableDomRef, { selectedRows: unknown[]; previouslySelectedRows: unknown[] }>
  ) => void;

And the one of the MCB like this:

  onSelectionChange?: (event: Ui5CustomEvent<MultiComboBoxDomRef, { items: unknown[] }>) => void;

As they're custom events, they all accept the detail property, which differs from component to component unlike with for example the native change event.

But even if a ui5-table would be wrapped inside a ui5-table (which is most probably bad practice/not supported at all), I wouldn't expect the event to bubble as again I see no advantage of event propagation here.

-- I recall the notes from e.g. https://javascript.info/bubbling-and-capturing , section "Don’t stop bubbling without a need!" that explain the downside of stopPropagation and list the preferred alternatives.

I think this only applies to native events that are bubbling per default (e.g. click, change, etc.) and even for native events there are some that don't bubble (focus, load).

---

Having said that I don't have a strong opinion in preventing bubbling per default, but I feel like this could improve Developer Experience and prevent a pitfall that many developers most probably will not have on their radar.

[kineticjs]

Hi @SAP/ui5-webcomponents-topic-rl,

Both the ui-table and the ui-combobox support an event named "selectionChange". When a combobox is nested inside a table and the combobox fires "selectionChange", the event bubbles and any listener for "selectionChange" on the table will also be notified. The author suggest that this is problematic, see the earlier communication for details. Can you comment from your side?

[vladitasev]

Hello @Lukas742

This kind of issue comes up occasionally, we should consider documenting it. This is something related to the way HTML works, and not necessarily with the components.

If nested HTML elements fire events with the same names, there will be a mix-up unless the application checks who fired the event.

Please see this modified example: https://codesandbox.io/s/ui5-webcomponents-forked-fntj9n

document.getElementById("table1").addEventListener("selection-change", (e) => {
  if (e.target.hasAttribute("ui5-table")) {
    alert("selection-change of Table was fired");
  }
});

In summary, there are 2 solutions:

• the application attaches an event listener to the combobox, and calls stopPropagation
• the application checks in the event handler whether the event originated from the expected component (recommended)

Regards, Vladi

p.s. Let me know if this solution works for the customer

[Lukas742]

Hi @vladitasev

Thanks for the reply. I still think that this behavior is a bit strange since it doesn't provide any benefit, however I primarily wanted to make sure that this is the intended behavior. Since I fixed the issue where this caused strange behavior in one of our components as soon as the author pointed it out to us, I think this issue can be closed. Thanks :)