Popover: afterClose event bubbling up to parents?

[stevetsanders]

Describe the bug
Doing any sorting on an AnalyticalTable component within a Dialog that has an onAfterClose callback passed through (which actually closes the dialog) will cause the dialog to close.

Isolated Example
https://codesandbox.io/s/wispy-dew-qlcu7

To Reproduce
Steps to reproduce the behavior:

1. Click on the open dialog button
2. click on the column header
3. click on one of the sort options
4. result => the onAfterClose callback is initialized and the dialog is closed

Expected behavior
The dialog should not close

Screenshots

UI5 Web Components for React Information
@ui5/webcomponents version: 1.0.0rc-7
@ui5/webcomponents-react version: 0.9.11
Operating System: Windows
Browser: Edge (Chromium)

Additional context

[MarcusNotheis]

Hey Steve,
that's a bug in @ui5/[email protected], but as far as I remember this one is already fixed and will be released with rc.8 in roughly two weeks. But I think I found another issue:
It looks like Popovers inside of Dialog are misplaced, see also this example: https://codesandbox.io/s/gifted-cohen-2h27i?file=/src/App.js

I'll forward this issue to the Web Components team, I think they'll look into it.

[ilhan007]

Both issues can be reproduced with the master version:

(1) The afterClose event fired from the popover (inside a dialog) bubbles to the dialog, and the handler (bound to afterClose) of the dialog is called although the dialog itself is not closing.

(2) The Popover placement is wrong when opened within a dialog. For the second there is an issue already:
#1960

[vladitasev]

Hello @stsander07 @MarcusNotheis

(1) the onAfterClose event handler of the popover should stop propagation/immediate propagation, if necessary. Alternatively, the dialog event handler for the same event could check the event target in order to decide whether to act or not. I can see why this looks weird and can be considered a bug even, but this is how events work in HTML and it would be very hard and counter-intuitive for us to try syncing events with the same name, fired by different components. We'd expect apps to control these.

(2) this is how CSS works and is widely considered a shortcoming of the spec - position: fixed is influenced by CSS transforms on the page. Therefore one can't reliably use position: fixed for popups. So we expect popups to be placed top-level, ideally on body level. Please see the more detailed explanation in the linked ticket.

[stevetsanders]

hi @vladitasev ,

Sorry, I'm a little confused. Are you saying this issue is fixed in rc.8 as Marcus thought? Or this is something the developers should handle?

[vladitasev]

Hi @stsander07 ,

Sorry for the confusion. To clarify: yes, we expect applications/third-party libraries to handle this issue.

@stsander07 @MarcusNotheis Marcus, do you have anything against this approach, or any other considerations?

[MarcusNotheis]

Hey @vladitasev,

I have some mixed feelings here to be honest.

I see that the event-bubbling is something that is very specific to HTML/JavaScript and should be taken care of by the developers (and I totally see why you don't want to work around the standard here :) ). Anyhow, I think that this is something that might easily lead to unexpected closing behaviours in applications, so I would at least add it to the documentation to make it very clear to the developers that they should take care of this handling. If there would be something like a console.warn is a popup is closed by event bubbling that would be even better because not everybody reads the documentation 😄

Regarding the transforms:
Isn't there any way you can open the popups without using transform? I see that we could use e.g. createPortal in React but that feels somehow a bit hacky as we would basically need to to that with every single popup / dialog to make sure we get the intended behaviour. This also feels a bit a like a regression because it was working fine in rc.7.

Best regards,
Marcus

[vladitasev]

Hi @MarcusNotheis @stsander07

We had an internal discussion about this today, and we've decided to try out a new thing: namely, mark some events as "non-bubbling" in the metadata, and respectively, fire them with bubbles: false.

We realize this could be a breaking change for some apps - for example if the app attached an event handler for "afterClose" on the body for all dialogs, this would stop working. At the same time, we see how this bubbling behavior might be considered unintentional most of the time, and people would be badly surprised more often than not.

We are going to create a PR for this sometime this week and link it here.

Regards,
Vladi