Timing of <dialog> 'close' event, popover 'toggle' event, <details> 'toggle' event

[jakearchibald]

https://html.spec.whatwg.org/multipage/popover.html
https://html.spec.whatwg.org/multipage/interactive-elements.html#the-dialog-element
https://html.spec.whatwg.org/multipage/interactive-elements.html#details-notification-task-steps

The show/hide steps for these happen on the same thread. However, a task is queued to fire the "close" and "toggle" events on dialog and popovers respectfully.

This creates a weird situation where the event could fire before or after the next render. Given that "I'm here now!" events are often used by developers as a time to enhance elements, this creates a race condition where the original state may be seen for a frame - a flash of unenhanced content. This would be really difficult to debug.

The solution in the <dialog>/<details> case would be to instead use a mutation observer for the open attribute, since that's guaranteed to happen before rendering. With popover, I guess you could use the "beforetoggle" event, but this event is also cancellable, so it isn't exactly "I'm here now!".

Option 1: Do nothing

Accept that it's a rotten bit of the platform, that will grow, since new features (like popover) will copy the behaviour in the name of consistency.

'select' events on various inputs also queue in this way.

Option 2: Dispatch the event synchronously

This is usually how events work when the action is performed on the main thread.

However, I think there's some reluctance to do this for <dialog> and <details>, since it's related to an attribute change, and for some reason we don't want to fire events in relation to DOM changes?

Also, changing from async to sync might be a compat risk for <dialog> and <details>.

Option 3: Dispatch the event in a microtask

This is how mutation observers work, which are related to DOM changes. It also allows for some debouncing, and it's still async.

Option 4: Dispatch the event in the render steps

This is how resize/scroll/pointer events work.

However, if the dialog is shown after this point in the render steps, you'd still get a flash of unenhanced content.

---

My preference is for option 2 or 3.

I can't decide if "input" events behave this way. The HTML spec suggests it might, but the UI events spec says it must be dispatched immediately.

[jakearchibald]

Open UI resolution: Fire "closed" and "toggle" events in a microtask rather than a task (option 3 above)

Minutes: https://www.w3.org/2023/03/23-openui-minutes.html#t02

[annevk]

I'd like @smaug---- to look at this as well and maybe @rniwa. I tend to agree that events for UI changes should happen before the next paint, but I'm not sure I fully understand the implications of using microtasks for a lot more things.

[jakearchibald]

@annevk I'd also like to hear more why a sync event is bad here. I agree it's a more compatible change for <dialog>, but what about popover and similar features going forward?

[josepharhar]

I'd like @smaug---- to look at this as well and maybe @rniwa. I tend to agree that events for UI changes should happen before the next paint, but I'm not sure I fully understand the implications of using microtasks for a lot more things.

Do yall have any thoughts? I can make a spec PR and implement this in chrome, but it would be great to have your support beforehand

[smaug----]

microtasks were designed for this kind of case - MutationObserver callback should fire before the next rendering update. So, using microtasks also there sounds reasonable, and it is probably quite web compatible, since microtasks are kind of asynchronous from JS point of view (but synchronous from browser point of view).

[josepharhar]

Thanks! I'll go ahead with this change then

[josepharhar]

I created a PR to switch to microtasks: #9775

[josepharhar]

Since this is going to be a breaking change, there is a chance that I have to roll it back once I ship it in chrome.
Assuming that I can ship it without rolling it back, would WebKit also implement this too? I don't want to go through with this unless we will eventually have interop. @rniwa @nt1m

[josepharhar]

I filed standards positions:

• Fire toggle events using microtasks WebKit/standards-positions#263
• Fire toggle events using microtasks mozilla/standards-positions#901

[annevk]

When I look at "show popover" (and "hide popover") we already fire beforetoggle synchronously. Is there any reason to queue a (micro)task for the second event at that point? What does that enable us to do?

Edit: I guess the main thing is that if it happened multiple times you get some deduplication, but is that still true enough with microtasks? And meaningful enough given that beforetoggle exists now and doesn't have deduplication?

cc @domenic

[josepharhar]

Yeah I suppose there isn't as much value for popovers given beforetoggle. Details elements don't have beforetoggle though.

This creates a weird situation where the event could fire before or after the next render

This is one of the main value points as I see it - it would simply make the events more consistent, right? I'm not sure how much this applies to other events in the platform though.

[annevk]

To be clear, I think overall changing our approach here makes sense. I'm just discussing the details to make sure we don't want to change this again a couple years down the road.

• For popover I think synchronous makes sense. I could see us doing a microtask and maintaining the deduplication infrastructure around that (appropriately renamed from task to microtask), but I don't really see the point.
• For details I agree that a microtask makes the most sense.
• For dialog's close event there are three different kinds of callers:
  1. Form submission: is it problematic that at the end of this task the form can now be mutated in some fashion? Where we dispatch events in this algorithm before we have to do some safety checks afterwards. Potentially we could fire synchronously and add the same safety checks?
  2. close(): seems okay to fire synchronously.
  3. The cancel event task: seems preferable to fire synchronously. I don't think the difference with a microtask is observable due to the JS stack being empty, but it just seems cleaner. (I kinda doubt implementations have the double task setup this currently seems to require.)

Thoughts?

[domenic]

I'd like to keep popover, details, and dialog on the same timing if at all possible.

[annevk]

I think details could be synchronous too as it's just an attribute mutation. It obviates the need for the task tracker thing.

[josepharhar]

I agree with domenic that we should keep them all the same, and I don't think that making any of them synchronous instead of async or microtask would be great. The fact that the beforetoggle event for popovers is synchronous (which it has to be in order for it to be useful) has caused us to add a lot of extra logic to handle what happens when the state is changed in the event handler. Maybe the toggle and close events aren't subject to that, but I don't see the value in making them synchronous instead of microtask.

The cancel event task: seems preferable to fire synchronously. I don't think the difference with a microtask is observable due to the JS stack being empty, but it just seems cleaner. (I kinda doubt implementations have the double task setup this currently seems to require.)

The cancel event is already synchronous right...? We should keep it that way so that preventing the closing of dialogs still works, right?

[keithamus]

If I can attempt to move towards a consensus:

• beforetoggle doesn’t change, sync for all three.
• toggle moves from task to micro task for all three.
• Dialogs cancel/close are synchronous (this means making close synchronous).

Does this sound suitable to everyone?

[domenic]

Does this sound suitable to everyone?

This works for me, but the compat impact means implementers need to be the ones making the call.

I will say that having dialog cancel and close have the same timing is nice. Their different timing (cancel sync, close async) caused some pain with the close watcher integration.

[josepharhar]

If yall really want to make dialog close synchronous, then I'll do it carefully with a flag and go back on it if it breaks websites. I feel more confident that that moving things from slow task queueing to microtasks won't have compat issues - although I'm still going to ship that behind a flag and undo it if it breaks websites as well.

[chrishtr]

There are use cases for an async close or toggle event for popover, <details> and <dialog>. One example is analytics code in response to accepting a cookie consent dialog which has no need to block rendering.

Further, since these events can't prevent the default action there is not a reason they have to be synchronous.
And synchronous JavaScript callbacks cause the potential for blocking the user if the script takes a long time to run, so should be avoided when possible. (This unfortunately happens often in practice due to web developers not realizing the time spent in their script callbacks can be quite substantial.)

Therefore, I think we should leave them async but consider extensions to addEventListener to allow the callback to have user-blocking priority.

[jakearchibald]

Since all of the options are (by design) before the next paint, they're all equally blocking in terms of UX. 500ms of JS execution will feel the same whether it's immediate, microtask, or render steps.

[chrishtr]

Since all of the options are (by design) before the next paint, they're all equally blocking in terms of UX. 500ms of JS execution will feel the same whether it's immediate, microtask, or render steps.

I agree that if the developer needs it to block paint, a delay is inevitable if script runs long. My main point is that the default being async is a good thing, for those developers who don't need it to block paint.

[smaug----]

Therefore, I think we should leave them async but consider extensions to addEventListener to allow the callback to have user-blocking priority.

This doesn't really make sense. Event listeners are called synchronously when the event is dispatched. There is not concept of asynchronousness there, nor concept of priority.

[chrishtr]

This doesn't really make sense. Event listeners are called synchronously when the event is dispatched. There is not concept of asynchronousness there, nor concept of priority.

I'm suggesting we add the ability for them to be async and have a priority. I agree that these would be new.