search

web-platform-tests / interop

web-platform-tests Interop project
https://wpt.fyi/interop
278 stars 28 forks source link

`mousemove_prevent_default_action.tentative.html` unnecessarily listens for `selectionchange` events when asserting `dragstart` is fired #576

Closed aprotyas closed 10 months ago

aprotyas commented 10 months ago

Test List

https://wpt.fyi/results/uievents/mouse/mousemove_prevent_default_action.tentative.html

Rationale

The second subtest in the mousemove_prevent_default_action.tentative.html WPT asserts that a cancelled mousemove event fires a dragstart, which is a fine expectation on its own. Unfortunately, since this behavior is asserted by comparing the logged events (obtained from respective event listeners) against a static set of expected events (["mousemove", "mousedown", "mousemove", "dragstart"]), the test may listen to selectionchange events and include any selectionchange events in the test assertion, producing an unnecessary test failure.

We should not care whether or not a selectionchange event was fired in this case, since we only want to assert that a dragstart event fires from a cancelled mousemove event on a draggable element. These two events are not mutually exclusive in any manner relevant to this WPT.

In fact, the same argument also applies for not listening to dragstart events when we only want to assert that a selectionchange event was fired.

We should only be listening for selectionchange events, and not also dragstart events, in the first subtest. Similarly, we should only be listening for dragstart events, and not also selectionchange events, in the second subtest.

aprotyas commented 10 months ago

cc @flackr @foolip for Chromium input.

cc @EdgarChen @smaug---- for Mozilla input.

aprotyas commented 10 months ago

I've implemented the test change proposal in https://github.com/web-platform-tests/wpt/pull/42471. Hopefully that makes it easier to review!

smaug---- commented 10 months ago

The test was initially added to explicitly test selectionchange. See also https://github.com/w3c/uievents/issues/278

smaug---- commented 10 months ago

Oh, the pr is just moving where selectionchange listener is registered. Yeah, I think that change is fine.

aprotyas commented 10 months ago

@smaug---- yes, testing for selectionchange is totally fine here, I'm proposing we don't keep the listener around for the dragstart test. Thanks!

aprotyas commented 10 months ago

Based on the comments here and on the PR, I think there's consensus that we want to make this test change. I'll merge https://github.com/web-platform-tests/wpt/pull/42471 to reflect that shortly.