-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[selectors] Add tests for :focus-visible in the default UA style sheet #27015
Conversation
Actually, hold on, I think this might need HTML spec changes. |
css/selectors/focus-visible-013.html
Outdated
} | ||
</style> | ||
|
||
<p>This test checks that using the mouse to focus an element does not trigger <code>:focus-visible</code> matching, and the element doesn't show any focus ring.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first part of this is already well covered by existing tests; what exactly is this test checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test current fails in Chromium and passes in Firefox. So it's not totally covered by other tests.
This checks that when you focus via mouse click, the element doesn't get a focus ring. As it doesn't match :focus-visble
it shouldn't get any outline by default. The default UA style sheet rule is going to be changed from :focus { outline: auto; }
to :focus-visible { outline: auto; }
, so Chromium shouldn't show any outline by default when :focus-visible
doesn't match, like in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the explanation.
That being the case, I think we don't need the custom styles (red/green background), since the selector matching part is already covered by other tests.
Instead, we could reword the test as something like "... by default, using the mouse to focus a generic element does not show a focus ring". Then, we could just check the outline style on focus. If the selector isn't supported, or isn't the UA default, this will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the customs styles are not needed for the test, but I think they're useful as it's covered in other tests. But I don't think it's bad to have them, as that makes easier to identify a failure if :focus-visible
is not supported at all.
I've reworded things as suggested but I'll keep the background colors unless you see a big problem with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem if you strongly prefer to keep the background colours, but why not just have something like the following?
.warning {
display: none;
}
:supports not (:focus-visible) {
.warning {
display: initial;
}
}
<div class="warning">
<code>:focus-visible</code> is not supported. This test will not pass.
</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that warning is that it won't change anything on the test, and maybe the script tests could be true without any :focus-visible
support at all. Probably not this particular test but it might happen in other tests.
I want that a browser that doesn't support :focus-visible
at all, fail on the automatic tests. So using this kind of red outlines/backgrounds is usually useful for both the automatic testing and manual testing.
Also I've added them in most of the test on the suite in previous patches (before that some tests were passing in WebKit without any kind of implementation), so maybe if we want to change that we could do for all of them together in follow-up patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe just add a comment explaining what the background colours are testing then, so we don't accidentally remove them later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the response, and for taking this work on!
css/selectors/focus-visible-013.html
Outdated
} | ||
</style> | ||
|
||
<p>This test checks that using the mouse to focus an element does not trigger <code>:focus-visible</code> matching, and the element doesn't show any focus ring.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the explanation.
That being the case, I think we don't need the custom styles (red/green background), since the selector matching part is already covered by other tests.
Instead, we could reword the test as something like "... by default, using the mouse to focus a generic element does not show a focus ring". Then, we could just check the outline style on focus. If the selector isn't supported, or isn't the UA default, this will fail.
css/selectors/focus-visible-013.html
Outdated
@@ -0,0 +1,46 @@ | |||
<!DOCTYPE html> | |||
<meta charset="utf-8" /> | |||
<title>CSS Test (Selectors): Mouse focus does not match :focus-visible</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also need to update this. Something like "mouse focus does not cause an outline to be shown by default".
Thanks for the review! I've uploaded a new version rewording the test title, expectations and so on. Let me know what do you think about the new version. |
I've uploaded a new version of the 2 tests, and also updated the initial comment and commit message to explain them. I've removed the background colors and instead added a I've added manual instructions to both tests too. I hope the new version of the tests and description in the commit message are good enough. Thanks for the reviews. |
This patch adds 2 new tests to verify that the default UA style sheet uses `:focus-visible { outline: auto; }`. See: whatwg/html#6256 & w3c/csswg-drafts#4278 * focus-visible-017.html: This test checks that when you focus an element via script, it show a focus ring with `outline-style: auto`. Currently Chromium passes this test, because despite they don't use `:focus-visible` in the UA stylesheet, it's painting an auto style outline when an element is focused. However Firefox fails it, because even when it uses `:-moz-focusring` (the equivalent to `:focus-visible`) in the UA stylesheet, it uses dotted style for the outline. WebKit doesn't support `:focus-visible` yet an it fails, thought it's painting an auto style outline (the test is specifically checking for `:focus-visible` support). * focus-visible-018.html: This test checks that when you click an element to focus it, it doesn't show any kind of focus ring. Currently Firefox passes this test, by Chromium fails it because Chromium is using `:focus` on the default UA stylesheet and is adding an outline on the element, despite it doesn't match `:focus-visible` (see https://crbug.com/1162070).
BTW, the HTML spec change has been merged, so this should be ready to be merged once @alice does the last review pass. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your patience and work on this!
Thanks for the reviews. |
This patch adds 2 new tests to verify that the default UA style sheet
uses
:focus-visible { outline: auto; }
.See: whatwg/html#6256 & w3c/csswg-drafts#4278
focus-visible-017.html:
This test checks that when you focus an element via script,
it show a focus ring with
outline-style: auto
.Currently Chromium passes this test,
because despite they don't use
:focus-visible
in the UA stylesheet,it's painting an auto style outline when an element is focused.
However Firefox fails it, because even when it uses
:-moz-focusring
(the equivalent to
:focus-visible
) in the UA stylesheet,it uses dotted style for the outline.
WebKit doesn't support
:focus-visible
yet an it fails,thought it's painting an auto style outline
(the test is specifically checking for
:focus-visible
support).focus-visible-018.html:
This test checks that when you click an element to focus it,
it doesn't show any kind of focus ring.
Currently Firefox passes this test, by Chromium fails it
because Chromium is using
:focus
on the default UA stylesheetand is adding an outline on the element, despite it doesn't match
:focus-visible
(see https://crbug.com/1162070).