Closed Bug 1823658 Opened 3 years ago Closed 1 year ago

Fix wpt failures in pointerevent_after_target_removed.html

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
128 Branch

People

(Reporter: hsinyi, Assigned: masayuki)

References

Details

Attachments

(1 file)

Bug 1823658 - Fix the first expectation of `pointerevent_after_target_removed.html?mouse` to conform to the spec r=smaug!
48 bytes, text/x-phabricator-request
Details | Review

Hmm, I'm not sure whether the test is valid because all tests fail in any browsers.

The test expects that mouse events in parent should be fired in the following order in the parent when removing click target:

  1. mouseenter
  2. mouseover
  3. mouseout
  4. mouseleave

However, both Chrome and Firefox fire them in this order:

  1. mouseenter
  2. mouseover
  3. mouseenter
  4. mouseout
  5. mouseleave

The 3rd mouseenter is redundant. The second one is caused by a default action of synthesized mouse move in this stack:

 #02: mozilla::EnterLeaveDispatcher::Dispatch (M:\src\dom\events\EventStateManager.cpp:4649)
 #03: mozilla::EventStateManager::NotifyMouseOver (M:\src\dom\events\EventStateManager.cpp:4802)
 #04: mozilla::EventStateManager::GenerateMouseEnterExit (M:\src\dom\events\EventStateManager.cpp:4939)
 #05: mozilla::EventStateManager::PreHandleEvent (M:\src\dom\events\EventStateManager.cpp:849)
 #06: mozilla::PresShell::EventHandler::DispatchEvent (M:\src\layout\base\PresShell.cpp:8211)
 #07: mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo (M:\src\layout\base\PresShell.cpp:8183)
 #08: mozilla::PresShell::EventHandler::HandleEventUsingCoordinates (M:\src\layout\base\PresShell.cpp:7129)
 #09: mozilla::PresShell::EventHandler::HandleEvent (M:\src\layout\base\PresShell.cpp:6927)
 #10: mozilla::PresShell::HandleEvent (M:\src\layout\base\PresShell.cpp:6870)
 #11: nsViewManager::DispatchEvent (M:\src\view\nsViewManager.cpp:652)
 #12: mozilla::PresShell::DispatchSynthMouseMove (M:\src\layout\base\PresShell.cpp:3858)
 #13: mozilla::PresShell::ProcessSynthMouseMoveEvent (M:\src\layout\base\PresShell.cpp:5835)
 #14: mozilla::PresShell::nsSynthMouseMoveEvent::WillRefresh (M:\fx64-dbg\dist\include\mozilla\PresShell.h:2034)
 #15: nsRefreshDriver::Tick (M:\src\layout\base\nsRefreshDriver.cpp:2582)

I think that it's reasonable to synthesize the mousemove is reasonable for users. On the other hand, I'm not sure about mouseenter. Once the element which is set to mLastOverElement (in the test, it's the div#child), but it's cleared when it's removed from the tree. Therefore, EventStateManager cannot check whether the cursor has already been in div#parent.

Edgar, do you have any ideas? I don't know it's reasonable that we should keep storing another element at removing mLastOverElement since it requires a hittest at mutation. So, dispatching mouseenter in this case is reasonable from both simpler code and the performance points of view.

Flags: needinfo?(echen)

I got the reason why the pointer events tests gets only pointerenter. At removing the element, eMouseMove event is synthesized but not ePointerMove. Therefore, EventStateManager does not handle pointerout etc. I'll post a patch for this.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

I found a bug of the pointer events tests (the expectation conflicts with another test one). I filed an issue for that.

See Also: → https://github.com/web-platform-tests/interop/issues/380

The test result conflicts with the test added in bug 1529904.

See Also: → 1529904

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #1)

Once the element which is set to mLastOverElement (in the test, it's the div#child), but it's cleared when it's removed from the tree. Therefore, EventStateManager cannot check whether the cursor has already been in div#parent.

Edgar, do you have any ideas? I don't know it's reasonable that we should keep storing another element at removing mLastOverElement since it requires a hittest at mutation. So, dispatching mouseenter in this case is reasonable from both simpler code and the performance points of view.

I looks into spec again and run https://codepen.io/edgarchen-the-decoder/pen/jOQxrvp on various browsers,

For pointerenter, spec says:

The user agent MUST fire a pointer event named pointerenter when a pointing device is moved 
into the hit test boundaries of an element or one of its descendants,

and for pointerleave, spec says:

The pointing device is moved out of the hit test boundaries of an element and all of its descendants.

When pointer device is moved to a child element, pointerenter is fired to parent element even if pointer device is not actually "enter" it (from the hittest point of view). So after a child element is removed, it seems reasonable to use its parent to generate corresponding enter/leave event when next hittest happens.

And for pointerout, since both Chrome and Safari fire pointerout event on the element which is removed and spec doesn't say we should suppress when the element is not connected, so we probably should not clear mLastOverElement when it is removed from tree.

About the event timing, per https://github.com/web-platform-tests/interop/issues/380#issuecomment-1639616890, it seems we should generate event when "real" movement is occurred, Safari behave like this also on mouse event.

Does this sound reasonable?

Flags: needinfo?(echen)

(In reply to Edgar Chen [:edgar] from comment #6)

When pointer device is moved to a child element, pointerenter is fired to parent element even if pointer device is not actually "enter" it (from the hittest point of view). So after a child element is removed, it seems reasonable to use its parent to generate corresponding enter/leave event when next hittest happens.

Yeah, I got it, thank you for explaining that.

And for pointerout, since both Chrome and Safari fire pointerout event on the element which is removed and spec doesn't say we should suppress when the element is not connected, so we probably should not clear mLastOverElement when it is removed from tree.

Oh, really? I don't see pointerleave nor pointerout before pointerenter and pointerover when I click the red box in Chrome on Windows/macOS either over green or black box. So, it seems that Chrome just clears the last enter/over element like us, but set it again with the synthetic pointer move.

On the other hand, yeah, Safari works as you expected. However, pointerleave nor pointerout is not fired on the div#parent (probably because of disconnected target in the former). Therefore, the test anyway fails in Safari.

I updated the issue too. I think that it's fine to dispatch mLastOverElement even after it's disconnected. However, it's hard to store all event targets of the preceding pointerenter so that it's hard to dispatch pointerout and pointerleave on parent (note that the test listens to event on parent). Therefore, it seems reasonable that only pointerenter fired on the parent (current Gecko and Safari's result).

Additionally, the test should check the events in the child too.

Anyway, I think that the spec should define this case first.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Oh, really? I don't see pointerleave nor pointerout before pointerenter and pointerover when I click the red box in Chrome on Windows/macOS either over green or black box. So, it seems that Chrome just clears the last enter/over element like us, but set it again with the synthetic pointer move.

Ah, my bad, you are right, Chrome doesn't fire pointerout and pointerleave event on removed element, only Safari fires. (I updated comment#6)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

I updated the issue too. I think that it's fine to dispatch mLastOverElement even after it's disconnected. However, it's hard to store all event targets of the preceding pointerenter so that it's hard to dispatch pointerout and pointerleave on parent (note that the test listens to event on parent). Therefore, it seems reasonable that only pointerenter fired on the parent (current Gecko and Safari's result).

I think it is reasonable that parent doesn't receive pointerout event, as pointerout is only fired on removed element and unable to bubble to its parent.

For pointerenter and pointerleave, it seems we need to store all element in the parent chain of mLastOverElement and compare to the new list which is built from new mLastOverElement, and generate pointerenter and pointerleave events based on the differences of two lists.

Additionally, the test should check the events in the child too.

Anyway, I think that the spec should define this case first.

It would be good if spec can have clear steps on how to handle pointerenter and pointerleave.

It seems that a proposed fix is on the spec issue now. Perhaps it's worth trying it out?

Although I need several days to be back here.
Current our result:

  1. pointerover@child
  2. pointerenter@parent
  3. pointerenter@child
  4. pointerdown@child,
  5. (child-removed)
  6. pointerup@parent
  7. pointerover@parent
  8. pointerenter@parent
  9. pointerdown@parent
  10. pointerup@parent
  11. pointerout@parent
  12. pointerleave@parent

Expected result is:

  1. pointerover@child
  2. pointerenter@parent
  3. pointerenter@child
  4. pointerdown@child
  5. (child-removed)
  6. pointerup@parent
  7. pointerdown@parent
  8. pointerup@parent
  9. pointerout@parent
  10. pointerleave@parent

So, unnecessary pointerover and pointerenter are fired on the parent.

Chrome does:

  1. pointerover@child
  2. pointerenter@parent
  3. pointerenter@child
  4. pointerdown@child
  5. (child-removed)
  6. pointerover@parent
  7. pointerenter@parent
  8. pointerup@parent
  9. pointerdown@parent
  10. pointerup@parent
  11. pointerout@parent
  12. pointerleave@parent

Similar to Gecko, but the unnecessary events are fired before pointerup.

Safari does:

  1. pointerover@child
  2. pointerenter@parent
  3. pointerenter@child
  4. pointerdown@child
  5. (child-removed)
  6. pointerup@parent
  7. pointerout@child
  8. pointerleave@child
  9. pointerover@parent
  10. pointerenter@parent
  11. pointerdown@parent
  12. pointerup@parent
  13. pointerout@parent
  14. pointerleave@parent

Safari works similar to Gecko but dispatch pointerout and pointerleave on the removed node too.

Oh, and also about mouse events:
Gecko:

  1. mouseover@child
  2. mouseenter@parent
  3. mouseenter@child
  4. (child-removed)
  5. mouseover@parent
  6. mouseenter@parent
  7. mouseup@parent
  8. mousedown@parent
  9. mouseup@parent
  10. mouseout@parent
  11. mouseleave@parent

Expectation:

  1. mouseover@child
  2. mouseenter@parent
  3. mouseenter@child
  4. (child-removed)
  5. mouseup@parent
  6. mousedown@parent
  7. mouseup@parent
  8. mouseout@parent
  9. mouseleave@parent

I feel the expectation does not match with UI Events definition because UI Events defines the events should be fired when:

  • mouseleave: "also dispatch this event when the element or one of its descendants moves to be no longer underneath the primary pointing device"
  • mouseover: "or when the element is moved to be underneath the primary pointing device"
  • mouseout: "or when the element is moved to be no longer underneath the primary pointing device"

UI Events uses "element" as subject of the sentence. Therefore, it's unclear in the cases when the element is moved/removed without the cursor move.

So, about Pointer Events, all browsers currently dispatch at least pointerover and pointerenter on the ex-parent of the removed node. However, the WPT expects which should be stopped. So, this could break web apps in the wild.

Smaug, Edgar: WDYT? I think that this is a risky change and we should not do this at least first of the browser vendors due to the market share.

Flags: needinfo?(smaug)
Flags: needinfo?(echen)

oh, that part is not covered by https://github.com/w3c/pointerevents/issues/477#issuecomment-1684634001
I mean that comment is about what happens afterwards.
Could you file a spec issue (assuming there isn't an issue for this already)?
https://github.com/w3c/pointerevents/issues/477 is about *out and *leave events but looks like we the question also about *over and *enter.
Or perhaps it is fine to continue discussion in that same spec issue, but change the title a bit to cover also over and enter events or something like that?

Flags: needinfo?(smaug)

(I'll try to do it in this week, sorry for the delay.)

Flags: needinfo?(masayuki)

Filed a UI Events issue about mouse* events.

Flags: needinfo?(masayuki)
Flags: needinfo?(echen)
See Also: → https://github.com/w3c/uievents/issues/359

I added comments for the current result (here and below).

There are still several issues:

  • All browsers should store all elements fired mouseenter events instead of the deepest descendant and stop dispatching mouseeneter on the parent of the removing element.
  • It seems that all browsers have a performance hack for synthesizing mouseover, mouseout, mouseenter, mouseleave, pointerover, pointerout, pointeenter and pointerleave. Therefore, if pointerdown removes the target, mouseover and pointerover may be dispatched either before or after pointerup.
  • Only Chrome dispatches a fallback mousedown or mouseup to the parent element of removed element which has already been removed by the preceding pointerdown or pointerup. I think that this behavior is not reasonable because users may not expect the click in the parent. However, the test expects this behavior.

So, currently, we could work on changing the mouseenter/mouseleave target management. However, current behavior is compatible with the other browsers. Therefore, "correcting" the behavior may have some risk.

Smaug and/or Edgar, what do you think? Should I change the code right now? Or after the other browsers fix their behavior?

Flags: needinfo?(smaug)
Flags: needinfo?(echen)

I think I don't understand the first point.

I don't understand what the performance hack is. It is about behavior, not about performance. I think that is fine. moves and down/up aren't really connected to each others.

And Chrome's behavior on down/up and then using ancestor if the target was removed seems quite reasonable. That way the page doesn't lose any down/up events.

Flags: needinfo?(smaug)

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #19)

I don't understand what the performance hack is. It is about behavior, not about performance.

In my understanding, it's for performance and ensuring "safe to run script" to synthesize fake mousemove if mouseover'ed element is changed by the DOM mutation or style changes since multiple mutations and/or style changes may occur under the mouse cursor in a tick. Therefore, without the asynchronous dispatching, the changes under the mouse cursor is slower than the cases when the mouse cursor is outside the element.

I think that is fine. moves and down/up aren't really connected to each others.

I hope so, but I think that it should be mentioned in the spec. I guess somebody would file the behavior as a bug of browsers.

And Chrome's behavior on down/up and then using ancestor if the target was removed seems quite reasonable. That way the page doesn't lose any down/up events.

Oh, really? The preceding pointerdown event has already been fired on the removed target. I think it's odd that a fallback mousedown event is fired on different element. Especially, the events insinuate that the user tries to execute something of the removed target element. However, that may cause running something of the bigger/outer target of the element which the user actually clicked.

I wonder, there should be inconsistent results between pointerdown and mousedown:

https://jsfiddle.net/d_toybox/m18jpnae/

e.addEventListener("pointerdown", event => {
  event.target.remove();
});
addEventListener("click", () => alert("clicked"));

https://jsfiddle.net/d_toybox/m18jpnae/1/

e.addEventListener("mousedown", event => {
  event.target.remove();
});
addEventListener("click", () => alert("clicked"));

In the first case, mousedown will be fired on the parent, then, mouseup will be fired on the parent too. Therefore, I expect that click event is fired on it.

In the later case, mousedown is fired on the child, then, mouseup will be fired on the parent. Therefore, click event should not be fired on the parent due to different event targets.

However, Chrome does not dispatch click in either cases. So, it seems that Chrome fixes the candidate of the click event target before dispatching pointerdown. However, click is defined by UI Events and Pointer Events is a different spec. So I feel it's odd that Chrome does not dispatch it in the previous case.

Actually, we fix it at dispatching mousedown:

If mousedown isn't dispatched when target is removed in pointerdown, then the problem is that should also we also not dispatch mouseup.
It is confusing if the page gets pointerdown, (but not mousedown), then pointerup and mouseup.

And click is yes possibly problematic (but click targeting has other issues too, like https://github.com/w3c/pointerevents/issues/356, which isn't fixed yet in browsers, I think, even though the spec has been clarified.)

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #21)

If mousedown isn't dispatched when target is removed in pointerdown, then the problem is that should also we also not dispatch mouseup.

I don't think so. mouseup is a physical device state change event. Therefore not dispatching click event which is a logical event for the target should be enough.

Filed: https://github.com/w3c/pointerevents/issues/492

It is confusing if the page gets pointerdown, (but not mousedown), then pointerup and mouseup.

I think that Pointer Events spec assumes (and maybe recommends) that web apps which does something in pointerdown should prevent its default to stop dispatching the legacy events. However, the testcase does not do that. Therefore, I think this is a tricky case for the spec editors.

Flags: needinfo?(echen)

pointerup should not imply the pointer move and pointerover should be fired
only when the pointer is actually moved. Therefore, pointerover should be
fired after pointerup which is followed by another pointermove instead of
before pointerup.

Depends on D199469

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #9376398 - Attachment description: Bug 1823658 - Fix the first expecation of `pointerevent_after_target_removed.html?mouse` to conform to the spec r=smaug! → Bug 1823658 - Fix the first expectation of `pointerevent_after_target_removed.html?mouse` to conform to the spec r=smaug!
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/6cfd68d8e76b Fix the first expectation of `pointerevent_after_target_removed.html?mouse` to conform to the spec r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44199 for changes under testing/web-platform/tests

Backed out for causing wpt failures on mouse_boundary_events_after_removing_last_over_element.html

Backout link

Push with failures

Failure log

Flags: needinfo?(masayuki)

Huh, how a change to a test caused issues in another test.

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #27)

Huh, how a change to a test caused issues in another test.

Patches of bug 1875424 got backed out due to the same test failure. Bug 1875424 seemed to be the real regressor.

It's a new test added in bug 1875424.

Flags: needinfo?(masayuki)
Upstream PR merged by moz-wptsync-bot
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/f50fbce47f44 Fix the first expectation of `pointerevent_after_target_removed.html?mouse` to conform to the spec r=smaug

https://hg.mozilla.org/mozilla-central/rev/f50fbce47f44

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox124: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch

This was backed out in the upstream.

Status: RESOLVED → REOPENED
status-firefox124: fixed → ---
Resolution: FIXED → ---

Will be fixed in bug 1888013 (D206209).

Status: REOPENED → ASSIGNED
Depends on: 1888013
OS: Unspecified → All
Hardware: Unspecified → All

All the tests under pointerevent_after_target_removed.html?mouse pass after bug 1888013.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago → 1 year ago
Resolution: --- → FIXED
Target Milestone: 124 Branch → 128 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: