1658996 - New wpt failures in /dom/events/Event-dispatch-click.html

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 3

[Vincent Hilla [:vhilla]]

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.

Comment 4

[Vincent Hilla [:vhilla]]

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.

Comment 5

[Vincent Hilla [:vhilla]]

: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.

Comment 6

[Edgar Chen [:edgar] (PTO until 02/09)]

(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?

Comment 7

[Adam Vandolder [:avandolder]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Vincent Hilla [:vhilla]]

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.

Comment 10

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 1: Introduce new methods around click event activation behavior. r=edgar

Comment 11

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 2: Activation behavior method for non-form submission input elements. r=edgar

Depends on D183265

Comment 12

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 4: Activation behavior method for button element. r=edgar

Depends on D183988

Comment 13

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 5: Activation behavior method for label. r=edgar

Depends on D183989

Comment 14

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 6: Activation behavior method for links. r=edgar

Depends on D183990

Comment 15

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 7: Activation behavior method for summary element. r=edgar

Depends on D183991

Comment 16

[Vincent Hilla [:vhilla]]

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?

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: Bug 1658996 - Part 3: Use activation behavior for submit and image input type. r=edgar

Depends on D183988

Comment 18

[Edgar Chen [:edgar] (PTO until 02/09)]

(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.

Comment 19

[Edgar Chen [:edgar] (PTO until 02/09)]

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.

Comment 20

[Phabricator Automation]

Comment on attachment 9351006 [details]
Bug 1658996 - Part 3: Use activation behavior for submit and image input type. r=edgar

Revision D187185 was moved to bug 1851970. Setting attachment 9351006 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9344631 [details]
Bug 1658996 - Part 4: Activation behavior method for button element. r=edgar

Revision D183989 was moved to bug 1851970. Setting attachment 9344631 [details] to obsolete.

Comment 22

[Phabricator Automation]

Comment on attachment 9344632 [details]
Bug 1658996 - Part 5: Activation behavior method for label. r=edgar

Revision D183990 was moved to bug 1851970. Setting attachment 9344632 [details] to obsolete.

Comment 23

[Phabricator Automation]

Comment on attachment 9344633 [details]
Bug 1658996 - Part 6: Activation behavior method for links. r=edgar

Revision D183991 was moved to bug 1851970. Setting attachment 9344633 [details] to obsolete.

Comment 24

[Phabricator Automation]

Comment on attachment 9344634 [details]
Bug 1658996 - Part 7: Activation behavior method for summary element. r=edgar

Revision D183992 was moved to bug 1851970. Setting attachment 9344634 [details] to obsolete.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2355070d2ad
Part 1: Introduce new methods around click event activation behavior. r=edgar
https://hg.mozilla.org/integration/autoland/rev/aa52d3591e17
Part 2: Activation behavior method for non-form submission input elements. r=edgar

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41860 for changes under testing/web-platform/tests

Comment 27

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/c2355070d2ad
https://hg.mozilla.org/mozilla-central/rev/aa52d3591e17

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

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

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.

I could note that PostHandleEvent is called after the normal event dispatch, not during normal target/bubble phase.
PostHandleEvent is part of the system group, not default group.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

And not saying the change here is bad or anything, just that other option might have been to set Event's phase to None when PostHandleEvent is called.