[popup] It is error prone to have both async and sync 'hide' event

[smaug----]

Mixing sync and async event dispatch is error prone. If one removes a popup from dom (that triggers async hide) and then adds it back and opens the popup, there will be a sync show event, right? And then once the event loop spins, the async hide is dispatched even though the relevant popup is shown.

[mfreed7]

Hmm, this is an excellent point. Do you have suggestions for how to handle the fact that dispatching synchronous events is basically prohibited at some times, e.g. while removing the popup from the tree? The alternative (I guess?) might be to just not fire the event in those cases. Thoughts?

[smaug----]

I guess similar-ish issue might happen with focus/blur. I can't recall how the blur is defined to work in this case data:text/html,<input placeholder="focus me" onfocus="setTimeout(()=> this.remove())" onblur="document.body.textContent = 'blur fired'">

[mfreed7]

I guess similar-ish issue might happen with focus/blur. I can't recall how the blur is defined to work in this case data:text/html,<input placeholder="focus me" onfocus="setTimeout(()=> this.remove())" onblur="document.body.textContent = 'blur fired'">

At least in Chromium, this is a special case added to ContainerNode::RemoveChild():

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=724;drc=34ac5694a0cf645b70049f6c691056165d7e5b41

I'd like to avoid adding another special case for this event also, if possible.

Besides the point about nodes being removed from the tree, the other case I was hoping to avoid was when a pop-up is being hidden because a modal <dialog> or fullscreen element is moving into the top-layer. If we fire a synchronous hide in those cases, the event handler could preempt or mess with the dialog/fullscreen.

[mfreed7]

There seem to be only (?) two potential paths here:

1. The existing behavior: fire hide asynchronously when a) the Element is removed from the Document, or b) a higher-priority top-layer element (e.g. modal dialog or fullscreen) is being shown.
2. Do not fire the hide event at all in the two corner cases above.

Any others? I think my preference is #1, just because it feels even worse to skip events entirely. Yes, #1 can fire show and hide out of order, but only in very corner case situations. A combination of one of the two corner cases above, plus the immediate re-show of the same pop-up. I think out-of-order events likely cause fewer problems than missing events.

[tbondwilkinson]

The main use case for hide in the explainer is to add animations right? In the cases where hide is sent asynchronously, the animation use case is already invalidated.

Unlike show, hide, isn't cancellable so you can't use that to prevent closing (and even if you could, I imagine you wouldn't want to if the popup was removed from the DOM, which seems like something that shouldn't be cancellable).

In my opinion:

1. Not firing hide seems reasonable for when the popup is removed from the DOM. I think it's rare someone will do this, and I think it's reasonable to not get any event if it happens. Removing the element from the DOM is a different operation than "hiding" it.
2. For the top-layer UI case, I suppose it's fine to be asynchronous. I'm not really sure if I agree with the choice for hide to be asynchronous, wouldn't you might want to do simultaneous animations for opening a dialog and closing a popup? But that design decision has probably sailed.

[mfreed7]

Certainly one of the use cases for hide is starting animations. But another is saving state from the pop-up when it hides. For that reason, it would seem like we should always fire it, right?

The reason behind the asynchronous event in the “other top layer UI” case is perhaps outdated or not completely thought through. The idea was to avoid a pop-up being able to cancel a modal dialog in the hide event handler. But that could still happen if the event is asynch. Perhaps we should just fire it normally in that case too? Note that we also skip CSS hide animations in this case, to avoid the delay in closing. But perhaps we should just let them run also? Your example is interesting - you might want the pop-up to fade out while the modal dialog is fading in.

[josepharhar]

If one removes a popup from dom (that triggers async hide) and then adds it back and opens the popup, there will be a sync show event, right? And then once the event loop spins, the async hide is dispatched even though the relevant popup is shown.

How about we just check some state in the async task to fire the hide event to see if it's still sane to fire the hide event...? and/or add some code in other spots to cancel any attempt to fire an async hide event. It seems like the best and easiest path forward to me.

[mfreed7]

Coming back to this issue, it sounds like perhaps there are three paths, not two:

1. The existing behavior: fire hide asynchronously when a) the Element is removed from the Document, or b) a higher-priority top-layer element (e.g. modal dialog or fullscreen) is being shown.
2. Do not fire the hide event at all in the two corner cases above.
3. (NEW) Fire hide synchronously when a higher-priority top-layer element (e.g. modal dialog or fullscreen) is being shown. Do not fire the hide event when the Element is removed from the Document, or perhaps fire it "later" (as discussed in the very related focus fixup conversation).

I'm coming around to at least the first part of #3 - perhaps we should just fire the events synchronously. It's not like developers can't "cancel" the other top layer elements in other ways, and this would enable fading out a popover while a dialog fades in, for example.

How about we just check some state in the async task to fire the hide event to see if it's still sane to fire the hide event...? and/or add some code in other spots to cancel any attempt to fire an async hide event. It seems like the best and easiest path forward to me.

I think we need a definitive set of logic - either we fire the event or we don't. It's hard for developers to code around an event that might or might not get fired, depending on state.

[css-meeting-bot]

The Open UI Community Group just discussed [popup] It is error prone to have both async and sync 'hide' event #578.

The full IRC log of that discussion

<gregwhitworth> Topic: [popup] It is error prone to have both async and sync 'hide' event #578
<gregwhitworth> github: https://github.com/openui/open-ui/issues/578
<hdv> masonf: the issue is: the hide event, that fires when the popover is about to hide, is fired synchronously in most normal cases, but in one particular case you can't fire synchronously when you have a popover that you want to remove from the DOM, we prefer not to fire events when things are removed from the DOM
<hdv> masonf: when another higher priority top layer element is added, like when something goes full screen, currently popovers would get hidden immediately, eg animations don't run
<hdv> masonf: so three things to discuss: asynch hide event is kind of funny (should we do it, maybe not fire event at all, should we do something else?); is it too heavy handed to make full screen higher order, if I have a popover open and a modal dialog opens, it may be nice to animate popover out before modal dialog opens
<JonathanNeal> q+
<flackr> q+
<hdv> masonf: so I'm thinking we could make these things that can be animated when they disappear. But then the third question is, should an event be fired, and if so, should it be sync or async?
<gregwhitworth> ack JonathanNeal
<hdv> JonathanNeal: based on what you said, normally if the element is not removed the event happens before the animation fires synchronously, is that right?
<hdv> masonf: as it currently stands, if a full screen event causes it to disappear the event is fired synchronously, but would like to change that if we can
<hdv> JonathanNeal: what about… that toggle event or the before event, it would only fire when the event can be tracked, which can't happen when it is removed… but the open/opened event of the other thing could always happen async, would that work at all?
<hdv> masonf: so you're saying, when element is removed, don't fire toggle event, but as a help, add the after toggle and fire that async?
<hdv> JonathanNeal: with the risk that it doesn't fire at all until we resolved on an after event
<hdv> JonathanNeal: that makes a lot of sense to me… would advocate for it remaining synchronous… for security and perf reason can understand it wouldn't fire if it wasn't properly closing
<hdv> masonf: so would be weird to fire an event async if it is normally sync?
<hdv> masonf: would be better not to fire it at all?
<hdv> JonathanNeal: yes that would be my opinon
<gregwhitworth> ack flackr
<hdv> s/opinon/opinion
<scotto_> q+
<hdv> flackr: a third option is that we could make it always asynchronous similar to animation events or requestAnimationFrame, where at the start of the next frame, before all the other stuff happens and updating life cycle, we fire the event
<hdv> flackr: nice thing about that would be you're more likely to have a clean style state going into this
<hdv> flackr: so you would have less forced style flashes
<hdv> masonf: so you're proposing deferring all behaviours that happen on hide, fire the event async then run the other behaviours
<hdv> masonf: you could run into some funny things in script when you do that
<gregwhitworth> ack scotto_
<hdv> scotto_: if the toggle button that invoked the popover, if the popover is removed will the button state be corrected?
<hdv> masonf: in the chromium implementation I'm almost 100% sure that aria-expanded is no longer true when the element is not in the DOM element, should add a test for that
<hdv> s/button state/button's aria-expanded state
<hdv> gregwhitworth: ok we'll add to agenda for the next time
<JonathanNeal> If this is async, I would expect some kind of `finished` like promise.
<hdv> gregwhitworth: ok we'll get 10 mins back! Remember, next week we will not meet, in two weeks we'll be in a different venue (not Zoom), I'll update Discord and the Google calendar invite

[mfreed7]

So in our discussion just now, there weren't too many conclusions, but it was a good discussion. I heard the following viewpoints:

1. Don't fire the "hide" event in the case that the element has been removed from the DOM. It is confusing to get what is normally a synchronous event asynchronously, after the thing has happened. (Side point, you might have opinions on https://github.com/whatwg/html/pull/8392)
2. Maybe make the "hide" event always asynchronous, which requires deferring all of the behavior until that async event runs. (My counterpoint: this makes lots of simple Javascript code confusing. E.g. p.showPopover(); p.matches(':open') === false.)
3. Question about ARIA: will aria-expanded still be in the correct state when the popover is removed from the DOM? Answer: yes, it should still be correct. This is just about the events.

Let me know if I missed anything, and more importantly, chime in with suggestions for ways forward here. In particular, we didn't discuss my proposal above to make the events synchronous (and animations-respecting) when modal dialogs and fullscreen elements are shown. I'd love input on that too.

[mfreed7]

Ok, at this point, I believe these are the open questions (assuming rough agreement with my comment above):

• Is it ok to not fire the "hide" event if a visible popover is removed from the document?
• If the answer above is "yes", then is it ok to always make "hide" synchronous, including when fired during a dialog.showModal() or requestFullscreen()?

If the answer to both is "yes", then I think we have a resolution to this issue. Note that the second point makes it "ok" to animate the popover "hide" while a dialog/fullscreen is being shown. That actually seems nice, from a user/developer point of view. Note that this will not delay the dialog/fullscreen showing, so I don't think there is much risk of "abuse" type behavior. We can always apply UA limitations to the duration of those hide animations, if needed.

[css-meeting-bot]

The Open UI Community Group just discussed [popup] It is error prone to have both async and sync 'hide' event #578, and agreed to the following:

• RESOLVED: fire the "hide" event asynchronously when the popover is removed from the document. For dialog.showModal and requestFullscreen, synchronously fire "hide" and allow animations.

The full IRC log of that discussion

<gregwhitworth> Topic: [popup] It is error prone to have both async and sync 'hide' event #578
<gregwhitworth> github: https://github.com/openui/open-ui/issues/578
<JonathanNeal> scribe: hdv
<hdv> masonf: so the current behavior and current spec is that the hide event can be either sync or async, depending on what's happening / why popover is being hidden
<hdv> masonf: this seems funny and problematic to me
<hdv> masonf: for instance, it is shown and gets hidden for async reason, but then shown…
<hdv> masonf: there are ways to fix this
<hdv> masonf: two cases where async happens: one is, not okay to fire sync events when containing element is removed from the document
<hdv> masonf: way around it is not fire an event
<hdv> masonf: another place where this happens is when higher prio top layer elements are added, like dialog.showModal or full screen content, they hide popovers immediately and fire hide event async so that they don't get in the way of higher priority top layer elements
<hdv> masonf: we could relax that, as sometimes it might be nice, eg maybe we want to animate the popover out instead of just throwing it out immediately
<JonathanNeal> +1 to not fire the "hide" event if a visible popover is removed from the document
<gregwhitworth> q+
<hdv> masonf: so proposal would be don't fire hide event when popover is thrown out, and fire sync hide events instead
<hdv> gregwhitworth: so the fact that you're not firing an event is what concerns me, I agree with everything you said except for the not firing the event
<hdv> gregwhitworth: to an extent we're assuming the author is aware why the dom node is removed… I can envision scenarios where someone writes crappy code and removes all of the popups out of the DOM tree… can imagine stuff like teaching UI wanting to reset where something was
<hdv> gregwhitworth: I could see that kind of scenario breaking
<hdv> masonf: it's a good question…there is an issue that I linked to on WHATWG, 8292
<hdv> masonf: there is a question about… if you remove a currently focused from the DOM, what should happen in terms of firing events?
<hdv> masonf: so could not fire sync event
<hdv> gregwhitworth: I thought that's what you suggested
<JonathanNeal> q+
<hdv> masonf: two options: no events at all vs firing an async event
<hdv> gregwhitworth: I would argue for the async event
<gregwhitworth> ack greg
<gregwhitworth> ack gregwhitworth
<hdv> masonf: the scenario that people bring up is… showpopover, remove the popover from the document, then add it back and show it immediately… you'll get a sync show event, and then an async hide event, which you no longer expected
<hdv> masonf: not sure how common it is \
<hdv> gregwhitworth: seems like that would be confusing… but at the same time, not providing the event would have its own problems too
<gregwhitworth> ack JonathanNeal
<hdv> gregwhitworth: it would seem to me best to fire the async event… and if you end up on that use case… yeah, I don't know what to do then, seems like an edge case
<hdv> JonathanNeal: could you give us a refresh… why does a non bubbling non cancelleable event could not always be synchronous? or is that the problem?
<hdv> masonf: it is disallowed in Chromium, because a synchronous event can put the node back in the document again, which could lead to weird edge cases
<hdv> JonathanNeal: thanks
<hdv> masonf: eg imagine you're removing an entire tree, and then the browser does that and right in the middle of that operation you start to add it back
<hdv> q+
<gregwhitworth> ack hdv
<gregwhitworth> hdv: I agree with gregwhitworth, it sounds better to get the event and possibly some edge case
<masonf> q?
<gregwhitworth> hdv: but that's probably better than not having any event at all
<masonf> Proposed resolution: fire the "hide" event asynchronously when the popover is removed from the document. For dialog.showModal and requestFullscreen, synchronously fire "hide" and allow animations.
<JonathanNeal> So, synchronous is bad because we could end up writing (effectively) this with events `while (true) { child.remove(); container.append(child); }`?
<masonf> RESOLVED: fire the "hide" event asynchronously when the popover is removed from the document. For dialog.showModal and requestFullscreen, synchronously fire "hide" and allow animations.
<hdv> masonf: is there an example of showing a dialog while something else is open and animate them both, do you have one jhey by any chance?
<hdv> jhey: I don't have one at the moment
<hdv> gregwhitworth: maybe like have a teaching UI open, and then go full screen, show a popup that says 'click this to go full screen', and it would animate aay
<hdv> s/aay/away
<JonathanNeal> I like it. The “_Chat is now available in full screen_” popup.

[css-meeting-bot]

The Open UI Community Group just discussed [popup] It is error prone to have both async and sync 'hide' event.

The full IRC log of that discussion

<una> masonf: current behavior is beforetoggle happens mostly synchronously, and in the show case it can be canceled, can't be canceled in hide case. One case that isn't synchronous is when a popover is showing in the DOM and then that element is removed from the DOM, previously resolved that we would keep firing the event synchronously - last sticking point on the spec PR. question is - is that decision the right one? Should we fire any events?
<una> Should it be async? Are there demos that show this particular case and could help us understand the problems with the current behavior
<una> masonf: this is a very corner case, don't want to overcomplicate the API to make this one corner case easier
<una> masonf: trying to understand how corner of a case this is
<JonathanNeal> I’m not joking about having a passive removal listener.
<una> masonf: my pushback has been that you can with existing APIs handle this situation, it just takes more work
<una> masonf: i.e. mutationObserver or the existing event
<JonathanNeal> q?
<una> JonathanNeal: when i'm designing something, i might want to account for a DOM removal event separately
<una> JonathanNeal: a usecase is a library that adds and removes things from the DOM. Has other side effects, i.e. pausing videos
<una> JonathanNeal: an option being a removal listener shouldnt be thrown out
<una> JonathanNeal: I have to know the container, wheras a popup listener for open/close is on the element
<una> JonathanNeal: I want a way to listen to that element's removal and then the issue of how things fire become more moot
<una> masonf: you think its very important to keep firing the event is one point you're making
<una> masonf: that is against the case of not firing the event
<una> JonathanNeal: no i agree with that
<una> JonathanNeal: i add an eventlistener when the popup is open. if i want to handle if the popup is removed i would add a similar api
<una> masonf: you can use a mutationObserver on the parent element
<una> JonathanNeal: i've tried this in many situations, and there's something very disconecting bw switching bw an event listener on the elem itself to a mutation observer
<una> JonathanNeal: I would agree with your original point that it didn't need to fire if i had another way to listen to it
<una> masonf: maybe there are 2 events here
<una> masonf: it sounds like you're asking for a more general improvement to mutationobserver that watches for its removal
<una> JonathanNeal: when i might remove something in my experience, it felt if i had this even ti wouldnt be trying to create a popup remove event
<una> JonathanNeal: this one would be passive
<una> JonathanNeal: i have to change my paradigm of thinking for this one use case
<JonathanNeal> Sorry for repeating myself so much. Thank you for listening!
<una> dbaron: one other thought about this set of problems is the risk of getting some of these events out of order when some are sync and some are async
<una> dbaron: one thing that makes this much less scary is to be able to look at the state and see if the popup is open or closed
<una> masonf: you can do this with a matches call
<una> masonf: talked about making an attribute to add some syntactic sugar
<una> dbaron: this seems good enough
<dbaron> (I don't have a strong opinion)
<una> masonf: mental model i've had is that this is a corner case, but there needs to be a way to detect that this has happened (that you've disconnected and are no longer showing), the current ways are cumbersome but there is a way
<una> masonf: if there are further comments, please put them on #578 - this is the main blocker to the spec

[mfreed7]

FYI @jonathantneal and others, @domenic pointed out this feature request, from 2017, for exactly the Mutation Observer addition you requested. I.e. the ability to observe removals on the element itself. Perhaps you'd like to comment there? It doesn't seem like a big implementation burden, but perhaps I'm missing something.