Github Use :focus-visible in "Phrasing Content" section. by emilio · P...

Contributor

domenic commented 25 days ago

Interesting that this differs so much from https://chromium-review.googlesource.com/c/chromium/src/+/2602429/ . The -webkit pseudo stuff I can understand, but the html, body, embed, iframe, object, input, textarea, select styles are strange. (Not that we should block on that.)

The tests at web-platform-tests/wpt#27015 don't seem like they test the default stylesheet though. In particular there should be a test that the value of outline changes from whatever its initial value is, to auto, with no user stylesheets involved. Previously we've tested default stylesheets with getComputedStyle(); see also web-platform-tests/wpt#5625.

/cc @mrego

emilio commented 25 days ago

Interesting that this differs so much from https://chromium-review.googlesource.com/c/chromium/src/+/2602429/ . The -webkit pseudo stuff I can understand, but the html, body, embed, iframe, object, input, textarea, select styles are strange. (Not that we should block on that.)

Yeah, Blink has a lot of focus rules as well... Gecko's rules are much simpler.

The tests at web-platform-tests/wpt#27015 don't seem like they test the default stylesheet though. In particular there should be a test that the value of outline changes from whatever its initial value is, to auto, with no user stylesheets involved. Previously we've tested default stylesheets with getComputedStyle(); see also web-platform-tests/wpt#5625.

Well, they do test the computed styles (or lack thereof on this case). In particular, they test that a focused element via mouse (which should match focus but not focus-visible) has no outline, which is the desired behavior and a behavior difference from this patch.

domenic commented 24 days ago

Well, they do test the computed styles (or lack thereof on this case). In particular, they test that a focused element via mouse (which should match focus but not focus-visible) has no outline, which is the desired behavior and a behavior difference from this patch.

It's the "lack thereof" that I'm concerned about. I'd like to have a test which distinguishes between outline: auto and outline: none or outline: thick double #32a1ce or outline: 1px dotted. I believe a browser which used any of those others in their :focus-visible UA stylesheet would pass the test in question, which is bad.

mrego commented 23 days ago

Regarding the Chromium default stylesheet and the complicated rules there, I guess that's something out of the scope of this PR. Probably a Chromium bug is the proper place to discuss that.

Regarding the tests, the initial test in web-platform-tests/wpt#27015 (focus-visisble-013.html) is passing in Firefox and failing in Chromium. That's expected as Chromium is using :focus in the UA stylesheet.
I've added a new test in the PR that checks that the outline-style is auto (focus-visisble-017.html), but that test right now passes in Chromium (though Chromium uses :focus pseudo-class) and fails in Firefox, because it uses outline: 1px dotted; so the style is dotted. I guess that kind of test is what @domenic was asking for.

domenic commented 22 days ago •

Regarding the Chromium default stylesheet and the complicated rules there, I guess that's something out of the scope of this PR. Probably a Chromium bug is the proper place to discuss that.

Well, if we think the spec should be changed, we'll want a spec bug. (But I'm not sure whether we do.) I agree it's out of scope for this PR.

I've added a new test in the PR that checks that the outline-style is auto (focus-visisble-017.html), but that test right now passes in Chromium (though Chromium uses :focus pseudo-class) and fails in Firefox, because it uses outline: 1px dotted; so the style is dotted. I guess that kind of test is what @domenic was asking for.

Perfect. Could you link to the test pull request?

mrego commented 22 days ago

Perfect. Could you link to the test pull request?

I've put both test files on the same PR: web-platform-tests/wpt#27015.

mrego commented 9 days ago

So @lilles has replied on the Chromium bug, and @alice has also shown agreement on the tests and CSSWG issue.

@domenic is that enough to unblock this? or do we need something else? Thanks.

domenic commented 8 days ago

Yes, thank you for the ping! Let's get this merged.