Formalize the concept of "If any event listeners were triggered"

[gterzian]

Problem:

Both unload(see step 9) and prompt to unload(see step 7) mention "If any event listeners were triggered by the earlier dispatch step, then set document's salvageable state to false.", without further specifying what "any event listeners were triggered" means or how it fits in other parts of the (DOM)spec.

This creates some uncertainty around implementing "unload" and "prompt to unload", because it seems unclear how, and when, an event is to be "marked as handled" in some way so as to allow the salvageable state of the document to be determined. It seems to requires a kind of "hack" as part of https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke (see for example: servo/servo#20329 (comment))

It also seems that implementations require some sort of 'handled' flag in practice, for example in servo, we ended up with a canceled attribute of Event that can be either Allowed, Prevented, or Handled, which seems to conflate the concept of the 'canceled' flag with an unspecified 'handled' flag.

Proposed solution:

I propose to clarify, and simplify, this with a new handled flag on event, that could be added to the current list at https://dom.spec.whatwg.org/#canceled-flag, and which would be set after step 2.7 of https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke (or perhaps as a final step 6 of https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm) (I'll refile in https://github.com/whatwg/dom/ if this makes sense)

Then unload(step 9) could read "If the unload's event handled flag is set..."

and prompt to unload(step 7) "If the beforeunload's event handled flag is set..."

(or perhaps "If the event dispatched in the previous step <dfn>was handled</dfn>..." with a link to the DOM spec explaining the concept(the handled flag being set)?

If this is too heavy handed, perhaps at least add a non-normative section explaining the concept of "any event listeners were triggered" and referring to the DOM for hints of how the concept can be implemented?

What do you think?

[annevk]

@gterzian does Servo have that model because of the HTML Standard or because it was found necessary studying other implementations / compat?

The final comments in #1900 suggest that this "were triggered" can be replaced with "has listeners for", which we should indeed formalize better, but would be a less invasive change (and rely upon something that while not encouraged, is more widespread in the platform).

[gterzian]

@annevk Thanks for pointing out the previous issue.

It seems Servo adopted that model as a way to prevent events that were previously 'handled' from being 'handled' again, from an internal implementation perspective, not following a spec and unrelated to this particular issue, see servo/servo@b10bfea#diff-b9c83174d4ef992d8bc1914c8826e220

+/// An enum to indicate whether the default action of an event is allowed.
+///
+/// This should've been a bool. Instead, it's an enum, because, aside from the allowed/canceled
+/// states, we also need something to stop the event from being handled again (without cancelling
+/// the event entirely). For example, an Up/Down `KeyEvent` inside a `textarea` element will
+/// trigger the cursor to go up/down if the text inside the element spans multiple lines. This enum
+/// helps us to prevent such events from being [sent to the constellation][msg] where it will be
+/// handled once again for page scrolling (which is definitely not what we'd want).

@wafflespeanut as the author of that commit, would you have more info to share on the background for it?

So what I ended up doing(and we're still discussing the validity of it), is re-using that concept and "marking an event as handled" as part of https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke, and using that 'mark' to set the salvageable state of the document in both 'unload' and 'prompt_to_unload'.

[annevk]

Okay, "default action" there triggers alarm bells, as events are not really supposed to have those (other than click's rather elaborate handling in the dispatch algorithm). There's a user action of pressing a key, which ends up triggering the events and should keep track of such details as mentioned in that comment.

[gterzian]

Thank you, I'll leave @wafflespeanut to comment on what was meant with it, and it does look to me like no concept of "default action" has leaked further than the wording of that specific comment.

@annevk If you had to implement a sort of flag to determine if a given event "has listeners for", to be used in 'unload' and 'prompt to unload', where the relevant events are 'unload' and 'beforeunload'(which seems to preclude an implementation specific to 'BeforeUnloadEvent'), which part of the spec-following code would be your preference to 'set' such a flag?
Personally I'm thinking checking for a non-empty list of listeners right after Step 6 of #concept-event-listener-invoke

[annevk]

@gterzian I don't think we need to change the dispatch algorithm. We'd have a standalone "has a listener" or some such algorithm that takes an EventTarget and a type and that basically iterates over the event listeners of that EventTarget to see if there's any with that type.

We could then use that hook in the unload/prompt to unload to determine if there are any unload/beforeunload listeners for the document.

On the DOM side this is tracked by whatwg/dom#453. If you want to work on that, that'd be great!

[gterzian]

@annevk Thanks, yes you're rigth, since the EventTarget is readily available, we use it to dispatch those events after all, I can indeed just add a method on it to check for the presence of listeners, and use it to set the document's salvageable state.

When I have a working implementation I'd be happy to work on defining it as part of the issue you link to...

[annevk]

@gterzian one thing you might want to add tests for is when this check is performed. That is, if the listeners remove themselves, is the document salvageable?

[gterzian]

Ok, I will. The answer would be 'not salvageable' right?

[annevk]

If you do the check at where the specification says "If any event listeners were triggered", yes. I'm just wondering if that matches implementations.

[wafflespeanut]

@gterzian It's been a long time since I made that change. EventDefault was a hack introduced in servo/servo#14738 and it was specific to keypress (and Servo's implementation), because some key presses and mouse events were being sent to the constellation when they shouldn't be (I'm not sure what progress has been made after, but I'm pretty sure it's not from the spec). If it's confusing things, please feel free to revert it back.

[gterzian]

@annevk Regarding #3646 (comment), I looked into it and thought that this might be covered already in unloading-documents/unload/007.html, where the listener removes itself and the document is not salvageable, see here

[annevk]

@gterzian that doesn't actually remove the event listener. An event handler only ever adds an event listener (it's the inner logic of that event listener that's affected by setting the event handler to null).

[gterzian]

@annevk Ok, so one would have to actually use removeEventListener inside the event handler, right?

[annevk]

@gterzian you cannot do this with an event handler. You need to add an event listener through addEventListener() and then remove it with removeEventListener() from the event listener's callback.

[gterzian]

Ok, could you please confirm this matches what you have in mind: https://github.com/servo/servo/pull/20837/files#diff-8ab7af18a43b40197cd12c675de4c130R4 (and we are asserting the 'unsalvageableness of the document through this, which matches the structure an existing test for unload)

[annevk]

Yeah, that looks correct.

[domenic]

#5889 removed this from HTML!