Syncing wpt PR 24975 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/dom/events/Event-dispatch-click.html
event state during post-click handling: FAIL
redispatch during post-click handling: FAIL
disabled checkbox still has activation behavior, part 2: FAIL
pick the first with activation behavior <input type=checkbox>: FAIL
look at parents only when event bubbles: FAIL

CI Results

Gecko CI (Treeherder)
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1658748 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #0)

Syncing wpt PR 24975 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/dom/events/Event-dispatch-click.html
event state during post-click handling: FAIL
redispatch during post-click handling: FAIL

disabled checkbox still has activation behavior, part 2: FAIL

Fixed in bug 1809689

pick the first with activation behavior <input type=checkbox>: FAIL

Fixed in bug 1809689

look at parents only when event bubbles: FAIL

CI Results

Gecko CI (Treeherder)
GitHub PR Head

Notes

These three are the remaining failures:

• event state during post-click handling
• redispatch during post-click handling
• look at parents only when event bubbles

Hi Vincent, please help this! :)

CC :avandolder to see if he has guidance/comments for this issue.

look at parents only when event bubbles

Dispatching a MouseEvent("click", {bubbles: false}) will cause activation behavior on a parent, as there is no check for mBubbles around the activation behavior in GetEventTargetParent e.g. here. Per the spec here this is not the correct behavior.

event state during post-click handling
redispatch during post-click handling

These originate from the same problem. HTMLInputElement::PostHandleEvent dispatches input and change events here. PostHandleEvent is called by EventDispatcher::HandleEventTargetChain during the target and bubble phase. But per the spec dispatch algorithm, I guess this should happen in step 11 after step 9 where event phase was set to NONE while we do it in step 5.

:avandolder do you have some guidance regarding how I could approach these two issues?

My understanding of the first one is that GetEventTargetParent executes the activation behavior. Though we cannot just introduce a check for mBubbles there. The spec dispatch algorithm has three places 5.5, 5.9.6.1, 5.9.8.1 where the activationTarget is selected and only one of them requires mBubbles to be true. I struggle especially with figuring out how 5.9.8.1 is represented in the code and in what situations it is relevant.

For the second one, PostHandleEvent would need to be called not in HandleEventTargetChain but at the end of Dispatch.

(In reply to Vincent Hilla [:vhilla] from comment #5)

:avandolder do you have some guidance regarding how I could approach these two issues?

My understanding of the first one is that GetEventTargetParent executes the activation behavior. Though we cannot just introduce a check for mBubbles there. The spec dispatch algorithm has three places 5.5, 5.9.6.1, 5.9.8.1 where the activationTarget is selected and only one of them requires mBubbles to be true. I struggle especially with figuring out how 5.9.8.1 is represented in the code and in what situations it is relevant.

For the second one, PostHandleEvent would need to be called not in HandleEventTargetChain but at the end of Dispatch.

I can not think of any other way to solve this other than implementing spec behavior, especially for second one, PostHandleEvent is designed to be called after a event target handled a event, so we cannot delay it to the end of Dispatch.

To implement the spec behavior, we could

• Have a new virtual function, bool HasActivationBehavior(), default return false in EventTarget to indicate whether the EventTarget has a activation behavior defined.
• Have new virtual functions, void LegacyPreActivationBehavior(), void ActivationBehavior and void LegacyCanceledActivationBehavior(), in EventTarget that implement the behavior.
• We could store the activation target in EventChainPreVisitor and call the relevant *ActivationBehavior() in Dispatch at point specified in spec.

And the main logic of finding parent target is in https://searchfox.org/mozilla-central/rev/9a4666e63199bd1bcfc9095f6efec3488c358458/dom/base/FragmentOrElement.cpp#745, we could setup activation target there. smaug, does this sound reasonable?

(In reply to Vincent Hilla [:vhilla] from comment #5)

I struggle especially with figuring out how 5.9.8.1 is represented in the code and in what situations it is relevant.

So I believe 5.9.8.1 is relevant in situations where parent is an ancestor of the shadow root the target is contained within. It makes sure that events don't bubble past the shadow root boundary. As it is, the spec has diverged from when the current handling code was written, but I https://searchfox.org/mozilla-central/source/dom/base/FragmentOrElement.cpp#952 is roughly the portion where steps 5.9.6 through 5.9.8 occur.

Other than that, I think :edgar covered everything.

We can't really add more virtual calls to event dispatch. That would slow it down. But we can store activation target in the event target chain while creating it.

And PostHandleEvent already happens effectively after Dispatch, since PostHandleEvent is called during system event group.

That PostHandleEvent is called during system event group might not be enough. Activation behavior should run in step 11 of the dispatch algorithm when the event phase is none and the event's dispatch flag is cleared.

This bug is concerned with these subtests failing where a checkbox is clicked, it's onchange handler is then invoked during the activation behavior and used to check the event state as well as dispatch the same event again.

During GetEventTargetParent, we can store the activation target in the event target chain, as suggested, avoiding the virtual HasActivationBehavior. But then have LegacyPreActivationBehavior, ActivationBehavior and LegacyCanceledActivationBehavior in EventTarget.

Depends on D183265

Depends on D183988

Depends on D183989

Depends on D183990

Depends on D183991

It would be good if we don't need to have [mActivationTarget] which increases the size of event, we usually would like to keep event size small if possible.

Avoid [syncing item data and item flags in PreHandleEvent] by checking for aEvent.mActivationTarget in GetEventTargetParent instead of PreHandleEvent.

I thought a bit more about these review comments, both problems only arise because of our behavior around form planned navigation. Apart from changing form navigation first or leaving the code as is, I have two ideas.

Use legacy pre activation behavior method
As we discussed, we only want to use this method in the context described by the spec and the (non-normative) note forbids my suggestion.
Despite that, I feel like it could be the cleanest solution and our behavior around planned navigation could be considered legacy as well as pre-activation.
The submit click begin code would move from PreHandleEvent to LegacyPreActivation, mActivationTarget wouldn't be needed anymore.
Aligning our code on planned navigation would necessitate removing these submit click begin parts. So in contrast to the other option, nothing might be left behind.
With the same argument we can land the button changes, which would be more consistent. Although this bug only tests the checkbox, button shows the same wrong behavior. And while the input element has LegacyCanceledActivationBehavior by the spec, we already need to do the end form submit tasks there, which is not part of the spec.

Have elements determine activation target
As we discussed, to avoid item flags in PreHandleEvent, already check if we will become the activation target in GetEventTargetParent, do not land button changes.
To avoid the mActivationTarget pointer, introduce a smaller bit flag and method HasActivationTarget. HTMLInputElement changes like this:

-  if (outerActivateEvent && HasActivationBehavior()) {
-    aVisitor.mWantsActivationBehavior = true;
+  if (outerActivateEvent && HasActivationBehavior() &&
+      (aVisitor.mEvent->mFlags.mBubbles || aVisitor.mEvent->mTarget == this) &&
+      !aVisitor.mEvent->HasActivationTarget()) {
+    aVisitor.mWantsActivationBehavior = true;
+    // already perform submit click begin tasks here

Though I guess this would be hacky. Someone might change the conditions around activation target in dispatch without updating them here too.
Thus, I would have all relevant elements do these checks themselves and EventDispatcher.cpp change like this:

-      if (preVisitor.mWantsActivationBehavior && !activationTarget &&
-          aEvent->mFlags.mBubbles) {
-        aEvent->mActivationTarget = parentEtci->CurrentTarget();
+      if (preVisitor.mIsActivationTarget) {
+        MOZ_ASSERT(!activationTarget);
+        aEvent->FoundActivationTarget();

This makes the elements slightly more complicated.
After aligning on planned navigation, it would be nice to clean up the code by moving the conditions back to dispatch and removing mHasActivationTarget.

:edgar, you were against using the legacy functions in a way not described in the spec. But after trying our idea, I feel like the first solution might be the better option?

Depends on D183988

(In reply to Vincent Hilla [:vhilla] from comment #16)

:edgar, you were against using the legacy functions in a way not described in the spec. But after trying our idea, I feel like the first solution might be the better option?

Per offline discussion:
As long as the other parts can be landed first independently, then yes. In theory, using legacy methods could make our form-submission behavior a tiny bit more like planned navigation (because form-submission becomes a bit later after event dispatching, so it should cover a little more cases, e.g. some form value is modified during event dispatching). And if possible, I would also expect to see some test cases which show the behavior difference and we pass after using legacy methods.

I think part 1 and part 2 are good to land. Lets try to land them first in this bug, and split remaining part into a follow-up bug.