-
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 related to script focus and :focus-visible #27806
[selectors] Add tests related to script focus and :focus-visible #27806
Conversation
Ok wow... so, with this PR there would be 37 tests about how focus visible works/doesn't. After a lot of looking at things carefully and a number of discussions, and even building some equivalents of each of these test the polyfill comparatively, let me try to summarize where we are, putting as much as I can into one simple "place"... There are two "groups" of tests.. Original ReceipeThese are the 18 that have been merged in to wpt as of now. If you look at the matrix in wpt.fyi results you can see there are 3 places where FF works differently - however I believe that one of those (focus-visible-017) is actually just a style issue. On the other two, the polyfill and Chromium agree. These are:
The other somewhat interesting one here is focus-visible-009, which is an autofocus test. The polyfill is the oddball here, but I think we just had no usage and I'm not particularly worried about it. Extra CrispyBased on some of the conversations in creating the WebKit implementation and test disagreements, Rego created another 19 tests with subtle variations intended to explore where he thought there was some potential disagreements. Actually, based on the conversations and confusion about whether a blur inbetween something had actually happened, most of these are in "pairs" - one that includes and explcit blur and one that doesn't. These are currently sitting in this PR. Of this group there are some interesting disagreements...
|
Mmmm, so I dont' know why I put it shouldn't match. I guess I assumed that a random keypress on a page won't affect this, but probably it should. I'm totally fine changing this to match
I think I'm pretty sure we don't want these to match If we agree on 002 behavior, clicking on a non-focusable element here, is like doing a random click on the page (basically the new active element is the body again), and then you follow 002 and don't match
I think that for these ones we also want to don't match
Again I'd be fine changing this. I think I put it should match
Again like for 006, I'm kind of sure we don't want to match |
Thank you @mrego for writing all of these wonderful tests! It's really helpful for articulating what is probably one of the trickiest parts of this whole proposal 😸 Here's my response to Brian's comment. Maybe we should add one more column to the table where we can state what we decide is the right behavior (match / don't match) for each test. 002 (codepen)
I think Chrome's behavior is wrong. The spec text says (I added emphasis):
If you click on a random part of the page then the
However, I think the rest of the tests demonstrate that really what we care about is if the user has sent a signal that they prefer a mouse or a keyboard. In this case they're sending a signal that they prefer a mouse, so I think the polyfill is correct. I think the subsequent tests support this theory. 004 (codepen)
I think Chrome's behavior is right. Here the browser behavior feels correct to me because the only signal that the user has provided is that they're using the keyboard, so we might assume they're in keyboard modality and match
Part of the problem is that in the initial state the I wonder if the spec needs to be updated to say something like, in the case of programmatic focus, initially the user is assumed to be in keyboard modality and unless another signal is provided (i.e. they click a mouse) then focused elements should match 006 (codepen)
I think Chrome's behavior is wrong. If you mouse click on something then it should put you into mouse modality which means focusing the div with tabindex should not match. As @mrego noted in those two issues, we don't want folks to start doing 008 (codepen) and 009 (codepen)
I think Chrome's behavior is correct. I don't think a programmatic 010 (codepen)
I think Chrome's behavior is correct. Similar point as 008-009, the user hasn't given any signal to opt them out of mouse mode. 016 (codepen)
I think Chrome's behavior is correct. Same point as 008-009. 018
I think Chrome's behavior is correct. Same point as 008-009. |
Correct according to what? That's not what the spec says afaict. If this behavior is more desirable, then we should fix the spec. |
Quick correction to my write up, I revisited the only one that @robdodson and I seemed to disagree on (#16 and #17) and it appears I was misreading the results of my polyfill test on that one/and I think I have a copy/paste error there - but... based on what I actually wrote as the use cases and everying that should have been obvious... So, actually, I and the polyfill agree on that one too. |
It seems like a lot of the spec issues boil down to that "active element" line. That was written with the mistaken assumption that whatever was last interacted with would be the active element - clearly false in hindsight. If we could re-word the existing advice to refer to the element the user last interacted with, instead of the active element, might that resolve these inconsistencies and confusion? |
Thanks for the feedback @robdodson, I believe I agree with you on the interpretation of the expected results on the different tests. I can modify the tests to check that expected behavior, and update the table on the first comment. @emilio the idea is to update the spec text so it reflects the new expected behavior that we all agree here. So indeed we'll need to update the spec for sure (ideally proposing this to HTML spec, as the CSSWG is reluctant to update the current heuristics). We're not checking if browsers are right or wrong according to the current spec, but what would be the ideal behavior in the different cases. @alice that's somehow what was proposed in the CSSWG issue with these rules:
The question is to clarify which is "the element the user last interacted with" in the different test cases explained before. I believe we need to add something related to the mouse modality or mouse mode that Rob is suggesting on his comment. |
a1e0774
to
06fe7dd
Compare
I've updated the tests in the initial comment, to what I believe is the current expectations we believe make sense for each of them. And I've also updated the PASS and FAIL results in Chromium and Firefox. Specially I've changed 4 tests:
The rest of tests are unchanged. |
Ok, so do we want the condition to be something like "last focused element focused by mouse or keyboard did match focus visible" or something like that? That seems reasonable at a glance. |
…lt. r=mac-reviewers,bradwerth,mstange This aligns Mac's focus model with other platforms. Matches Chromium, but not Safari. Reasons why I think it's worth making this change: * Consistency with all other platforms. * Makes the :focus-visible implementation more useful. * Fixes focus navigation after e.g. clicking a button. * Shouldn't cause a lot more outlines to show up (at least not by default). An example of the second point: data:text/html,<button onclick="this.nextElementSibling.focus()">Click</button><button>Imagine I'm a dialog close button or something</button> In non-macOS platforms, we won't show an outline for the button in that case, which matches the developer expectations (links below). We don't show the outline because the focus comes from an element that has been focused by mouse (and thus didn't show an outline). But on macOS that doesn't work, because the button is not focused. For completeness, the actual heuristics for :focus-visible may change a bit as a result of the discussions in: * w3c/csswg-drafts#5885 * web-platform-tests/wpt#27806 But it's not clear to me how to best define this so it works on the macOS focus model. An example of the third point: data:text/html,<input type=text><input type=submit><input type=text> On Safari and Chrome (and Firefox on non-macOS platforms), clicking the button, then pressing tab, goes to the input on the right. In Firefox on macOS it doesn't because the button doesn't gain focus nor is selectable. Differential Revision: https://phabricator.services.mozilla.com/D108808
What we implemented before this patch was basically what the heuristics in the spec said, which used to be normative: https://drafts.csswg.org/selectors/#the-focus-visible-pseudo That has become non-normative and there's ongoing discussion on what should happen for cases like this in: w3c/csswg-drafts#5885 web-platform-tests/wpt#27806 There seems to be agreement on that WPT issue on cases like this one, so let's make it work. Differential Revision: https://phabricator.services.mozilla.com/D108805
What we implemented before this patch was basically what the heuristics in the spec said, which used to be normative: https://drafts.csswg.org/selectors/#the-focus-visible-pseudo That has become non-normative and there's ongoing discussion on what should happen for cases like this in: w3c/csswg-drafts#5885 web-platform-tests/wpt#27806 There seems to be agreement on that WPT issue on cases like this one, so let's make it work. Differential Revision: https://phabricator.services.mozilla.com/D108805
What we implemented before this patch was basically what the heuristics in the spec said, which used to be normative: https://drafts.csswg.org/selectors/#the-focus-visible-pseudo That has become non-normative and there's ongoing discussion on what should happen for cases like this in: w3c/csswg-drafts#5885 web-platform-tests/wpt#27806 There seems to be agreement on that WPT issue on cases like this one, so let's make it work. Differential Revision: https://phabricator.services.mozilla.com/D108805
It looks like @mrego updated the html spec with the following snippet:
I'm not very familiar with the html spec terms but my interpretation is that this is saying "if a mouse click moved focus most recently then do not match focus." Based on my reading that seems like it would satisfy the 010 example. So I think this looks good to me, but am curious to hear what @emilio @alice and @bkardell think. |
It looks like we have agreement on the expected behavior for these tests. Would people be fine landing these set of tests using |
So I'm not sure about the tests that Firefox Nightly fails with this PR applied:
From what I can tell, they're all about clicking a random non-focusable element on the page and expect focus-visible not to match when moved via script focus. That behavior is needed to make the "click button, move focus somewhere else" work on Safari (Firefox on macOS no longer has this behavior) because buttons are not focusable. But that clearly doesn't match the spec PR as written. Afaict, there's nothing that would run the "update focus step" when the focus doesn't change, (and it doesn't in any of these tests). FWIW, I'd be curious if @rniwa, @smfr or someone else from WebKit would be able to comment on Safari's mouse focus behavior. According to testing from @mstange, Safari does have different behavior here in its UI and web content. Maybe we can all agree on focusing form controls by mouse and then the whole model for this can be simpler? |
I agree that there are some weird things in those tests that use a non-focusable element. But not all of them are the same to me. In 002, 003, 006 & 007 the user clicks a random part of the page (002 & 003) or a non-focusable element (006 & 007). I agree that in those cases, by default the focus won't change, as 016 & 017 are different, because there's a focused element before the user clicks in the non-focusable element, and the focus moves to the I guess nothing runs regarding focus when focus doesn't change, so I agree that the spec text is not good for this. |
BTW, I've updated the table on the first comment, with the results in Firefox Nightly and now they match Chromium. The tests listed by Emilio related to non-focusable elements (002, 003, 006, 007, 016 & 017) fail in Firefox but also in Chromium. |
So one idea here could be to change the expectations of (002, 003, 006, 007, 016 & 017) so they pass in Chromium and Firefox and land these set of tests. They'll be testing the current implemented behavior. Then I could implement the same thing in WebKit and see how things go there. We could open a different issue to discuss the expectations of those 6 tests, dunno where exactly (as it looks like progress on the HTML spec will be complex, and the text in CSS is just an example). What do you think about this approach? |
06fe7dd
to
f103b72
Compare
So I went ahead and modified the tests Apart from that, I've a patch for WebKit that also follows this behavior. My plan would be to merge these set of tests and use them for the WebKit patch. And report an issue to discuss if we want to change those tests back to not match @bkardell and/or @emilio would you mind reviewing and approving this PR so I can merge it? 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.
Ok, that seems reasonable to me. We might want to mark the ones that are up for discussion with .tentative.html
, but either with or without that it seems good to have coverage for this.
The tests reflect the current browser behavior, but the user expectations might be different in these. The spec text is still being worked on, so let's mark them as tentative at this stage.
Thanks for the review, I'm marking those tests as |
Reported new issue to continue discussion at #28505. |
These are a set of tests trying to see what should happen with
:focus-visible
in different scenarios when there's a programmatic focus.This is an attempt to have a common understanding in order to be able to write some spec text, probably based on the last proposal on the CSSWG issue where this was discussed. Even when the spec text might end up going to HTML spec instead of selectors' one, as suggested by the CSSWG.
TBH I don't have a clear opinion on which cases should match or not, but I think it's more useful for me to have a good set of tests and agree on them, in order to prepare a potential spec text (and also to know what to implement on the WebKit side, as this particular topic was brought up during the review of the patch to add support for `:focus-visible). I'll be fine changing these tests so some of them match and other not, or whatever is needed. Even add more cases if you think it's useful.
This PR has 19 tests, all of them but 1 are repeated. So for each pair of tests (2 & 3, or 4 & 5, etc) the 1st one does
focus()
and the 2nd one doesblur()
and thenfocus()
. I believe we want both cases to behave the same always.Here is the list of tests and the pass/fail in Chromium and Firefox (updated 2021/04/07):
focus-visible-script-focus-001.html
:focus-visible
focus-visible-script-focus-002.html
:focus-visible
This checks script focus after the user has clicked something irrelevant on the page.
focus-visible-script-focus-003.html
:focus-visible
focus-visible-script-focus-004.html
:focus-visible
This checks script focus after the user has typed a random letter without focusing anything on the page.
focus-visible-script-focus-005.html
:focus-visible
focus-visible-script-focus-006.html
:focus-visible
focus-visible-script-focus-007.html
:focus-visible
focus-visible-script-focus-008.html
:focus-visible
, does NOT match:focus-visible
focus-visible-script-focus-009.html
:focus-visible
, does NOT match:focus-visible
focus-visible-script-focus-010.html
:focus-visible
, does NOT match:focus-visible
focus-visible-script-focus-011.html
:focus-visible
, does NOT match:focus-visible
focus-visible-script-focus-012.html
:focus-visible
focus-visible-script-focus-013.html
:focus-visible
focus-visible-script-focus-014.html
:focus-visible
focus-visible-script-focus-015.html
:focus-visible
focus-visible-script-focus-016.html
:focus-visible
focus-visible-script-focus-017.html
:focus-visible
focus-visible-script-focus-018.html
:focus-visible
focus-visible-script-focus-019.html
:focus-visible
Sorry for the long list and thank you very much for your time. I hope this will allow us to move the topic forward.
CC @alice @emilio @bkardell