[selectors] Add focus-visible-013.html test and fix focus-visible-007.html #27156

mrego · 2021-01-12T23:22:50Z

This test had wrong assertions and it was too complex,
so this patch is simplifying it.

A part of this test is extracted to a separated test
focus-visible-013.html, where we check that :focus-visible
does not match after mouse click even if previous focused element
was matching :focus-visible.

And then we simplify the focus-visible-007.html test, to check that
an active element that doesn't match :focus-visible,
will start matching :focus-visible after after a keyboard interaction
that doesn't move the focus (in this case the "Shift" key).

mrego · 2021-01-12T23:26:30Z

This new focus-visible-007.html passes in Chromium and fails in Firefox.

This test is checking this part of the spec:

If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible (i.e. keyboard usage may change whether this pseudo-class matches even if it doesn’t affect :focus).

So there's an element that was focused via mouse without triggering :focus-visible and then the user press the Shift key. And even when the focus doesn't move, the element now matches :focus-visible.

Note that in Chromium this doesn't happen for all keyboard interactions, as focus-visible-012.html is explicitly checking that using a modifier key (like Ctrl) doesn't trigger :focus-visible.

Firefox is not doing this for any keyboard interaction as far as I've tested.

CC @alice

css/selectors/focus-visible-007.html

css/selectors/focus-visible-013.html

css/selectors/focus-visible-007.html

css/selectors/focus-visible-013.html

alice · 2021-01-21T22:23:37Z

The opening comment says the test had some "wrong assertions", but doesn't say what they are, and the change is too complex to figure it out from the diff.

What were the wrong assertions?

alice · 2021-01-21T22:23:55Z

css/selectors/focus-visible-007.html

  </style>
 </head>
 <body>
  This test checks that using the keyboard in a way that does not move focus still causes <code>:focus-visible</code> matching to trigger.
  <ol id="instructions">
    <li>If the user-agent does not claim to support the <code>:focus-visible</code> pseudo-class then SKIP this test.</li>
-    <li>Use the mouse to focus the element below that says "Click me first."</li>
-    <li>If the element has a red outline, then the test result is FAILURE.</li>


Why change this?

I believe the test was kind of complex and testing 2 different things, that's why I split it in 2 different tests.
Then I don't need so many steps and elements on this first test, and that's why I removed this part.

You changed this from a failure to a success, but the test doesn't pass unless both steps (initial focus from click not matching :focus-visible, and later pressing a key triggering :focus-visible matching) pass. So it's misleading to claim that the test is a success after step one. Why not leave this specific text the way it was?

Sorry my previous comment was too quick and wrong.

The steps on the new version of the test are:

If the user-agent does not claim to support the :focus-visible pseudo-class then SKIP this test.

Use the mouse to focus the element below that says "Click me."

If the element has a blue outline, then the test result is a SUCCESS.

Press the SHIFT key.

If the element now has green outline, then the test result is SUCCESS.

The first time you click only :focus matches and not :focus-visible, and it has a blue outline.
Then after clicking SHIFT, :focus-visible matches and the outline changes to green.

I can come back to the previous colors I guess, but changing the test again. The thing was that I was having a hard time understanding the whole test and I believed the new version was better to understand (apart from the issues with the wrong assertions).

I agree with splitting the test into two parts, apologies for not being clear about that.

Splitting this out into two tests means that the later part of the manual steps (click on the second element) are no longer needed, I agree with that.

However, previously the manual test made it clear that not passing the first part of the test (clicking the element shouldn't trigger :focus-visible matching) is an instant fail, meaning the second part of the test (pressing shift) is irrelevant. That's why it had the mechanism for setting up that red border to demonstrate that.

Can we have separate changes for splitting this out into two tests, and simplifying focus-visible-007.html? That might make this review easier for both of us.

Ok, maybe I went too far away changing this test, I know understand your point. Sorry for all the back and forth.

I can separate this in different PRs, or maybe simplify this PRs by just removing the last steps in focus-visible-007.html but keep the rest (fixing the wrong assertions), and then add the proper manual instructions to focus-visible-013.html. Sorry for the mess.

Thank you for your patience with all my comments! Even just splitting this change into two separate commits for the two different changes would help, or otherwise just keep this change as the splitting out (uncontroversial and we can land it right away), and we can work on the simplification/leak fixing etc. separately.

So I've tried to just split this in two tests, but focus-visible-007.html has other problems too.

The test starts by sending a SHIFT key, then it sends the SHIFT key again after focus, and at that point it starts to test things. I believe I can fix focus-visible-007.html in a separated PR and keep this PR for adding a new test that covers part of focus-visible-007.html that I'll get rid of later.

So I've finally done 2 different commits.
One of them adds a new focus-visible-013.html test, which is the same than before but with instructions to run it manually.
And then another commit fixes focus-visible-007.html, keeping most of its current design, but removing the wrong parts and the extra checks that are now tested in focus-visible-013.html.

Thank you so much for this extra work! This cleanup looks excellent.

css/selectors/focus-visible-013.html

mrego · 2021-01-21T22:27:52Z

css/selectors/focus-visible-007.html

-        });
-
-        const test_modality_change = t.step_func(() => {
-          assert_equals(getComputedStyle(one).outlineColor, "rgb(59, 153, 252)");


These asserts that were checking "weird" colors are the wrong assertions.

That's odd, they would have passed when I added them. When did they begin failing?

It looks like this is somehow failing since long time ago, when test_driver.send_keys() was implemented in Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/1652635

Looking at https://crbug.com/976438, I see the original version of this test is also triggering a leak. I'm not sure whether that's a problem with the test, or the code under test.

That leak is due to test_driver.click() that is causing a leak on Chromium unless we wait for the promise to resolve.

The Chromium bug is https://bugs.chromium.org/p/chromium/issues/detail?id=1164600

I've fixed similar leaks in focus-visible tests in https://bugs.chromium.org/p/chromium/issues/detail?id=1163172

And once the fixed version of focus-visible-007 gets imported into Chromium, I'll check if it has leaks or not there, and fix them in a similar way.

Wonderful, thanks for the investigation and explanation!

mrego

I've replied inline in one of the wrong assertions checking weird colors, not the ones defined on the CSS of the test.

Apart from that you can see the final status of the test in https://github.com/web-platform-tests/wpt/blob/638eb0680fcac43d587eea75e26733e2edb741b3/css/selectors/focus-visible-007.html, so it's easier to understand.

alice · 2021-01-24T22:39:16Z

css/selectors/focus-visible-007.html

  </style>
 </head>
 <body>
  This test checks that using the keyboard in a way that does not move focus still causes <code>:focus-visible</code> matching to trigger.
  <ol id="instructions">
    <li>If the user-agent does not claim to support the <code>:focus-visible</code> pseudo-class then SKIP this test.</li>
-    <li>Use the mouse to focus the element below that says "Click me first."</li>
-    <li>If the element has a red outline, then the test result is FAILURE.</li>


Thank you so much for this extra work! This cleanup looks excellent.

css/selectors/focus-visible-013.html

This test is extracted from focus-visible-007.html that has other problems. In focus-visible-013.html we check that :focus-visible does not match after mouse click even if previous focused element was matching :focus-visible.

The test had a bunch of issues that are fixed by this patch. Also a part of this tests has been extracted to focus-visible-013.html which allows us to further simplify it.

mrego · 2021-01-25T05:50:38Z

Thanks for the review, uploaded new version updating the instructions for focus-visible-013 and will merge it.

focus-visible-007.html has been fixed in WPT by web-platform-tests/wpt#27156 It looks like the test is no longer leaking, neither it needs different expectations for Mac. Bug: 976438, 1170284 Change-Id: I1b474b6c4748893ffe1a48d395416807e89c2e6e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2645012 Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#846709}

focus-visible-007.html has been fixed in WPT by web-platform-tests/wpt#27156 It looks like the test is no longer leaking, neither it needs different expectations for Mac. Bug: 976438, 1170284 Change-Id: I1b474b6c4748893ffe1a48d395416807e89c2e6e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2645012 Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#846709} GitOrigin-RevId: e5e407bbc95167f228fdad099a0bbe8521a48b65

mrego added the selectors label Jan 12, 2021

mrego requested review from emilio and Loirooriol January 12, 2021 23:22

wpt-pr-bot assigned tabatkins Jan 12, 2021

wpt-pr-bot requested review from fantasai, frivoal, plinss and tabatkins January 12, 2021 23:22

github-actions bot requested a deployment to wpt-preview-27156 January 12, 2021 23:23 Pending

Loirooriol reviewed Jan 12, 2021

View reviewed changes

css/selectors/focus-visible-007.html Outdated Show resolved Hide resolved

Loirooriol reviewed Jan 13, 2021

View reviewed changes

css/selectors/focus-visible-007.html Outdated Show resolved Hide resolved

css/selectors/focus-visible-013.html Outdated Show resolved Hide resolved

css/selectors/focus-visible-007.html Show resolved Hide resolved

css/selectors/focus-visible-007.html Outdated Show resolved Hide resolved

mrego force-pushed the focus-visible-007 branch from e970922 to 4efead6 Compare January 14, 2021 09:56

github-actions bot requested a deployment to wpt-preview-27156 January 14, 2021 09:57 Pending

Loirooriol approved these changes Jan 14, 2021

View reviewed changes

css/selectors/focus-visible-013.html Outdated Show resolved Hide resolved

mrego force-pushed the focus-visible-007 branch from 4efead6 to 638eb06 Compare January 14, 2021 13:51

github-actions bot requested a deployment to wpt-preview-27156 January 14, 2021 13:52 Pending

alice reviewed Jan 21, 2021

View reviewed changes

css/selectors/focus-visible-013.html Show resolved Hide resolved

mrego commented Jan 21, 2021

View reviewed changes

mrego force-pushed the focus-visible-007 branch from 638eb06 to e0c1d41 Compare January 21, 2021 23:55

github-actions bot temporarily deployed to wpt-preview-27156 January 21, 2021 23:56 Inactive

mrego changed the title ~~[selectors] Rework focus-visible-007.html~~ [selectors] Add focus-visible-013.html test Jan 21, 2021

mrego changed the title ~~[selectors] Add focus-visible-013.html test~~ [selectors] Add focus-visible-013.html test Jan 21, 2021

github-actions bot temporarily deployed to wpt-preview-27156 January 22, 2021 00:11 Inactive

mrego changed the title ~~[selectors] Add focus-visible-013.html test~~ [selectors] Add focus-visible-013.html test and fix focus-visible-007.html Jan 22, 2021

alice approved these changes Jan 24, 2021

View reviewed changes

[selectors] Add focus-visible-013.html test

bebf2c2

This test is extracted from focus-visible-007.html that has other problems. In focus-visible-013.html we check that :focus-visible does not match after mouse click even if previous focused element was matching :focus-visible.

[selectors] Fix focus-visible-007.html

8fef923

The test had a bunch of issues that are fixed by this patch. Also a part of this tests has been extracted to focus-visible-013.html which allows us to further simplify it.

mrego force-pushed the focus-visible-007 branch from ce59535 to 8fef923 Compare January 25, 2021 05:49

github-actions bot deployed to wpt-preview-27156 January 25, 2021 05:49 View deployment

mrego merged commit 82877d9 into web-platform-tests:master Jan 25, 2021

mrego deleted the focus-visible-007 branch January 25, 2021 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[selectors] Add focus-visible-013.html test and fix focus-visible-007.html #27156

[selectors] Add focus-visible-013.html test and fix focus-visible-007.html #27156

mrego commented Jan 12, 2021

mrego commented Jan 12, 2021

alice commented Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

mrego Jan 22, 2021

alice Jan 24, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego Jan 21, 2021

alice Jan 21, 2021

mrego left a comment

alice Jan 24, 2021

mrego commented Jan 25, 2021

[selectors] Add focus-visible-013.html test and fix focus-visible-007.html #27156

[selectors] Add focus-visible-013.html test and fix focus-visible-007.html #27156

Conversation

mrego commented Jan 12, 2021

mrego commented Jan 12, 2021

alice commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrego commented Jan 25, 2021