1966551 - Fix wpt failures in pointerevent_pointerout_no_pointer_movement.html

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The test was changed for conforming to the latest spec, that is, pointer boundary events should be dispatched when the layout is changed under the pointer.

According to the test result, Chrome already supports the new behavior in release builds too.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, the test expects that another synthesized eMouseMove should happen when ePointerOut is fired by a synthesized eMouseMove.

However, we do not allow that for preventing infinite reflow loop. I think that this limitation is reasonable. That could cause infinite flickering and/or making a clickable element unavailable. So, I think we should change the test rather than removing the limitation. WDYT?

Comment 2

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

I wonder what other browsers do in that case. Do they process the loop for some time? Or perhaps they don't have any limitations?
The flickering would be a site bug basically, and similar (perhaps not the same) can of course happen also if one uses JS in a certain way.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

At least, Chrome passes the test, so, Chrome must allow that at least the first nesting. Safari timeouts the test too, but I guess it's caused by not trying to dispatch pointer events at layout changes. So, I'm not sure about Safari.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

According to this testcase, Chrome does not allow the nesting too. I'm not sure how they pass the test...

Comment 5

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

Mustaq, do you understand why Chrome passes the wpt when comment 4 shows it does not allow loop?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(I think that whether it should work with/without spinning the event loop is not the main purpose of the test. So, I think we can change the test.)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1966551 - Make `PointerEventHandler::DispatchPointerFromMouseOrTouch()` dispatch synthesized `ePointerMove` for synthesized `eMouseMove` if it's caused by hoverable pointer r=smaug!,#dom-core

Currently, we don't dispatch synthesized ePointerMove unless it's
required for dispatch the boundary events after dispatching
ePointerLostCapture event [1] since Pointer Events defined that the
boundary events should be fired only when before dispatching a pointer
event. However, it's changed, Point Events currently defines that the
boundary events should be fired if the element under the pointer is
changed without a pointermove [2] if and only if the pointer supports
hover.

Therefore, this patch makes PresShell store the last input source
whose event set the mouse location at last and
PresShell::ProcessSynthMouseMoveEvent() sets the input source to make
PointerEventHandler::DispatchPointerFromMouseOrTouch() can consider
whether it needs to dispatch pointer boundary events or not for the
pointer.

Additionally, the mochitests for the manual WPTs under
dom/events/test/pointerevents checks pointerId. Therefore, this
patch makes PresShell also store the last pointerId and set it to
the synthesized eMouseMove too.

I think that this approach is not correct approach to fix this bug
because there could be multiple hoverable pointers, but we synthesize
pointer boundary events only for the last input device. I think it's
enough for now because we've not supported pen well (we've not supported
the test API yet!), so, we only support only mouse input well as
hoverable inputs. I think we should extend PointerInfo and make a
synthesizer of ePointerMove later.

Note that this patch changes 2 WPTs which both are in the scope of
Interop.

The expectation of
pointerevent_pointer_boundary_events_after_removing_last_over_element.html
needs to be changed for conforming to the latest spec. I wrote this
test before the spec change and it wasn't updated when the spec is
changed. I filed this issue to interop [3].

The changes for pointerevent_pointerout_no_pointer_movement.html is
required for avoiding the timeout. Gecko does not allow recursive
synthesized eMouseMove to prevent infinite reflow loops without moving
the mouse cursor. However, the test expects that and that causes
requiring the hack for Chrome too. Therefore, I split the test to
make each step run in different event loop and I removed the hack for
Chrome.

Note that this patch also removes 2 sets of mochitests for WPT manual
tests because they are now tested with the test driver [4][5] and they
fail without maintained.

1. https://searchfox.org/mozilla-central/rev/f571db8014431de31d245017e2f5457046aec4ea/dom/events/PointerEventHandler.cpp#494-503
2. https://w3c.github.io/pointerevents/#boundary-events-caused-by-layout-changes
3. https://github.com/web-platform-tests/interop/issues/961
4. https://wpt.fyi/results/pointerevents/pointerevent_boundary_events_in_capturing.html%3Fmouse?label=master&label=experimental&aligned&view=interop
5. https://wpt.fyi/results/pointerevents/pointerevent_releasepointercapture_events_to_original_target.html%3Fmouse?label=master&label=experimental&aligned&view=interop

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think this hack for Chrome is required for the same issue. However, I cannot reproduce the failure in my environment with running the test with Chrome Canary. The patch deletes the hack, but I'm not sure my guess is right.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe63e89a0316
Make `PointerEventHandler::DispatchPointerFromMouseOrTouch()` dispatch synthesized `ePointerMove` for synthesized `eMouseMove` if it's caused by hoverable pointer r=smaug

Comment 10

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

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

Comment 11

[Mustaq Ahmed]

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

I think this hack for Chrome is required for the same issue. However, I cannot reproduce the failure in my environment with running the test with Chrome Canary. The patch deletes the hack, but I'm not sure my guess is right.

Sorry I missed the ask in comment 5, and looks like Masayuki already found the answer. Yes, Chrome fires a synthetic mousemove when main frame is ticked after a layout change, to be able to update hover/active states after a scroll, for example. The synthetic event is not sent to JS.

Re the hack: we (Chrome) would love to remove this Chrome-only hack but the main frame is not ticked after the second layout change in the test as per the crbug discussion. So our ToT build fails consistently w/o it!

Comment 12

[Sandor Molnar[:smolnar]]

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

Comment 13

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

Upstream PR merged by moz-wptsync-bot

Comment 14

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe63e89a0316
Make PointerEventHandler::DispatchPointerFromMouseOrTouch() dispatch
synthesized ePointerMove for synthesized eMouseMove if it's caused by
hoverable pointer r=smaug

Perfherder has detected a browsertime performance change from push fe63e89a0316a6a6cc84d02b0d7b98a0f1ba863f.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
14%	microsoft FirstVisualChange	windows11-64-24h2-shippable	fission warm webrender	105.50 -> 91.10	Before/After
10%	microsoft fcp	windows11-64-24h2-shippable	fission warm webrender	67.87 -> 61.40	Before/After
8%	microsoft largestContentfulPaint	windows11-64-24h2-shippable	fission warm webrender	74.72 -> 68.80	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 45262

The following documentation link provides more information about this command.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's really wired. The patch should not affect to the performance especially for improvements because it does new things but not skips anything which we are doing. I guess that the benchmarks have hacks for browsers which won't dispatch pointer boundary events when layout change occurs and that is not cheap. So, the behavior change could cut the runtime cost of the expensive hacky paths with the behavior change.