search

web-platform-tests / interop

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

Retroactive review of user-action-pseudo-classes-in-has.html changes/regressions #606

Closed foolip closed 9 months ago

foolip commented 9 months ago

Test List

https://wpt.fyi/results/css/selectors/invalidation/user-action-pseudo-classes-in-has.html

Rationale

https://github.com/web-platform-tests/wpt/pull/43122 rewrote this test, making it pass in Firefox, but fail in Chrome.

Here are the before/after commits: https://wpt.fyi/results/css/selectors/invalidation/user-action-pseudo-classes-in-has.html?sha=17dcc288d1c558801914f40f07e9735adecea59f https://wpt.fyi/results/css/selectors/invalidation/user-action-pseudo-classes-in-has.html?sha=5fab76582c597542ff5dfc02b3462531842349d8

There are no Safari runs for these exact commits, but these runs sandwich the change, and it looks like it was failing before and now in a different way: https://wpt.fyi/results/css/selectors/invalidation/user-action-pseudo-classes-in-has.html?run_id=6308805735874560&run_id=5148500288864256

foolip commented 9 months ago

This is the failing assert: https://github.com/web-platform-tests/wpt/blob/64f4760de5a6d33a753f5a0a36aa697ba4edc70b/css/selectors/invalidation/user-action-pseudo-classes-in-has.html#L70-L71

@dshin-moz you updated the test, can you say what this part is intending to test?

At first glance it seems like this either has to do with testdriver.js differences, or is about when different pseudo-classes match, not specific to :has().

dshin-moz commented 9 months ago

Hm. It's intended to test that when the currently applied style matching :has() (i.e. One applying .ancestor:has(.descendant1:hover:active) { color: skyblue }) invalidates back to .ancestor:has(.descendant1:hover) { color: blue }. Seems like Chrome's match back to the initial color: black.

Does Chrome's test driver move the pointer at that point?

nt1m commented 9 months ago

@foolip I personally vote for removing this test from Interop. Safari fails this test because of a SafariDriver bug where SafariDriver doesn't always focus the window before starting the test (making :hover / :active / :focus tests timeout).

SafariDriver does occasionally focus the window, in which case the test passes, especially on isolated test runs, as evidenced by the test runs at: https://github.com/web-platform-tests/wpt/pull/43122/checks?check_run_id=18682484760 (with 2/2 passing)

Regarding the functionality this is testing, this is covering :has(:hover) / :has(:active) / :has(:focus) / :has(:focus-visible) invalidation which works fine when you try it manually in Safari.

nt1m commented 9 months ago

Given the inconsistencies with the test harnesses, I think this is probably the best path forward for everyone.

foolip commented 9 months ago

@nt1m I agree that if there are problems unrelated to :has() and we can't work around them, then we should drop the test. @dshin-moz is trying some things in https://github.com/web-platform-tests/wpt/pull/43214 so let's revisit in our next meeting.

foolip commented 9 months ago

I have minified the test into something to show what's happening. This passes in Chrome:

<!DOCTYPE html>
<meta charset="utf-8" />
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-actions.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<div>Hover and click me</div>
<script>
  promise_test(async () => {
    const div = document.querySelector('div');
    assert_false(div.matches(':hover'), 'initial :hover state');
    assert_false(div.matches(':active'), 'initial :active state');

    await new test_driver
      .Actions()
      .pointerMove(1, 1, {origin: div})
      .send();
    assert_true(div.matches(':hover'), ':hover state after mouse move');
    assert_false(div.matches(':active'), ':active state after mouse move');

    await new test_driver
      .Actions()
      .pointerDown()
      .send();
    assert_false(div.matches(':hover'), ':hover state after mouse down');
    assert_false(div.matches(':active'), ':active state after mouse down');

    await new test_driver
      .Actions()
      .pointerUp()
      .send();
    assert_false(div.matches(':hover'), ':hover state after mouse up');
    assert_false(div.matches(':active'), ':active state after mouse up');
  });
</script>

I'm not sure what's going on, needs more debugging.

foolip commented 9 months ago

I have added some logging and concluded that adding .pointerMove(1, 1, {origin: div}) to each call really does make a difference. With the code in the previous comment, the "mousemove" event fired by the 2nd action sequence is at coordinates (0, 0) in both Chrome and Firefox. (I'm not sure about Safari, logs while the test is running don't show how.)

I suppose that it makes sense that each action sequence is independent, so a change that moves the pointer to the same place at the start of each sequence makes sense.

If there are still issue due to https://github.com/web-platform-tests/interop/issues/606#issuecomment-1815610417 I think we should drop the test, however.

dshin-moz commented 9 months ago

I've updated web-platform-tests/wpt#43214 to have each action and follow-up checks be independent. That said, it wouldn't resolve #606 (comment).

foolip commented 9 months ago

https://github.com/web-platform-tests/wpt/pull/43214 has been merged now and from https://wpt.fyi/results/css/selectors/invalidation?label=pr_head&max-count=1&pr=43214 it looks like it will be passing in Chrome, Firefox, and Safari.

However, there may be flakiness issues still. @dshin-moz @nt1m if you see any test failures in Firefox or Safari, can you reopen this? In that case I think we should just exclude the test from Interop 2023.

dshin-moz commented 9 months ago

I can't reproduce this locally on 500 runs, unless if I happen to click during the test run so that the window loses focus. The failure also is on the first :hover invalidation, so I suspect some sort of test environment flakiness. All things considered, +1 on just removing it from Interop 2023.

nt1m commented 9 months ago

Fwiw, I reopened because Safari is still failing this test when run as part of a suite (which is the default configuration on wpt.fyi , see results on wpt.fyi), because of SafariDriver drops focus from the window. When ran individually the test passes though (see @foolip's link).

There is no :has() implementation issue as evidenced by the fact that the test runs fine on our custom WebKitTestRunner harness: https://searchfox.org/wubkat/source/LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/user-action-pseudo-classes-in-has-expected.txt

foolip commented 9 months ago

OK, let's drop this test. @jgraham is that OK for Gecko?

nt1m commented 9 months ago

Sounds like @dshin-moz gave his +1 to remove this test in https://github.com/web-platform-tests/interop/issues/606#issuecomment-1834110874