Github [selectors] Add tests related to script focus and :focus-visible by mrego...

Copy link

Member

bkardell commented 27 days ago •

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 Receipe

These 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:

• focus-visible-007
  Basically, this is a div tab index="0" which you click on, and doesn't match - and then you press the enter key, which has no effect here except to trigger focus-visible matching. After some feedback the test itself was changed, but still FF is the only one that disagrees here. Bug

• focus-visible-011
  This checks that keypresses on a div which have focus and include *a preventDefault() still trigger matching - everything but FF agree. Seems like a bug to me as certain interface elements like tabs would encounter this and many prevent. Bug

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 Crispy

Based 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...

• focus-visible-script-002 and 03: checks that if you have a document level click handler that moves focus to a div with a tab-index, this will not match focus visible. In both implementations, it does - but in the polyfill, it does not. In other words, Rego's test match the polyfill behavior, which does not match either implementation.

• focus-visible-script-004 and 05: checks that if you have a document level key handler that moves focus to a div with a tab-index, this would not match... No implementation agrees with the test and my own intuition makes me think the test assertions are backward. I'd like to understand why rego interprets otherwise.

• focus-visible-script-006 and 007: checks that if (from a fresh state) you have a click handler on an element that isn't normally focusable and that programatically moves your focus to a div tabindex=0, that this will not match. These are among the more interesting ones for several reasons: First, because the tests, the polyfill, and my own intuition agree but neither browser implementation does. Second, because this is effectively a platform agnostic version of what will happen on some elements on mac by default. Like, basically it lets us talk about that problem without even talking about platforms... And we don't agree. Why?

• focus-visible-script-008 and 009: are an example of a pair where the blur matters: Everybody passes 06, but FF disagrees on 07 because a blur happened. I think we need to sort this out somehow by better defining because FF is err'ing on the wrong side. That is, if we show it when we aren't supposed to we're encouraging people to disable it.

• focus-visible-script-010 and 011: Its an input element, you click it and the click handler moves focus programatically to a div tabindex 0. Chrome and the polyfill agree that this doesn't match, but the test and FF both think it should. 11 varies this by adding an explict blur, but we already don't agree on 10. Note that the polyfill spec stuff about programatic focus at all begins (I think) with WICG/focus-visible#88 and "It'll use whatever state it was previously in." - but I think the crux here is we haven't described the state well or how it is maintained.

• focus-visible-script-012 and 013, and 14 and 15: Everybody agrees, yay.

• focus-visible-script-016 and 017: Given an input, a non-focusable div with a click handler and a focusable div - if you click on the non-focusable and that then sets your focus to the focusable div: should it match. Like 6 and 7, this is curious to me because because the tests, the polyfill, and my own intuition agree but neither browser implementation does and I don't understand why. Similarly, we know this case isn't probably rare - interfaces that put little descriptive 'popups' or 'expanders' next to inputs which, when clicked move focus into the div containing the contents for various reasons happens a bunch, I'd guess.

• focus-visible-script-018 and 019: These are basically a variant of 16 and 17 except that the thing you click on actually is focusable. Here, everything agrees on 18, but it is another example of where FF disagrees with everything in 19 "because blur".

mrego commented 27 days ago

• focus-visible-script-004 and 05: checks that if you have a document level key handler that moves focus to a div with a tab-index, this would not match... No implementation agrees with the test and my own intuition makes me think the test assertions are backward. I'd like to understand why rego interprets otherwise.

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 :focus-visible.

• focus-visible-script-006 and 007: checks that if (from a fresh state) you have a click handler on an element that isn't normally focusable and that programatically moves your focus to a div tabindex=0, that this will not match. These are among the more interesting ones for several reasons: First, because the tests, the polyfill, and my own intuition agree but neither browser implementation does. Second, because this is effectively a platform agnostic version of what will happen on some elements on mac by default. Like, basically it lets us talk about that problem without even talking about platforms... And we don't agree. Why?

I think I'm pretty sure we don't want these to match :focus-visible. It could be a <button> on Mac, or just a simple <div> that is not focusable and is doing the trick. Otherwise people might end up doing :focus-visible { outline: none; } which we want to avoid. See the comment on the CSSWG issue: w3c/csswg-drafts#5885 (comment). Also one of the comments in the original discussion: WICG/focus-visible#88 (comment)

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 :focus-visible as your last focus change was via mouse.

• focus-visible-script-008 and 009: are an example of a pair where the blur matters: Everybody passes 06, but FF disagrees on 07 because a blur happened. I think we need to sort this out somehow by better defining because FF is err'ing on the wrong side. That is, if we show it when we aren't supposed to we're encouraging people to disable it.

I think that for these ones we also want to don't match :focus-visible. This is similar to 006.

• focus-visible-script-010 and 011: Its an input element, you click it and the click handler moves focus programatically to a div tabindex 0. Chrome and the polyfill agree that this doesn't match, but the test and FF both think it should. 11 varies this by adding an explict blur, but we already don't agree on 10. Note that the polyfill spec stuff about programatic focus at all begins (I think) with WICG/focus-visible#88 and "It'll use whatever state it was previously in." - but I think the crux here is we haven't described the state well or how it is maintained.

Again I'd be fine changing this. I think I put it should match :focus-visible due to my understanding of the current heuristics on the spec:

If the previously-focused element indicated focus, and a script causes focus to move elsewhere, the newly focused element should indicate focus.

• focus-visible-script-016 and 017: Given an input, a non-focusable div with a click handler and a focusable div - if you click on the non-focusable and that then sets your focus to the focusable div: should it match. Like 6 and 7, this is curious to me because because the tests, the polyfill, and my own intuition agree but neither browser implementation does and I don't understand why. Similarly, we know this case isn't probably rare - interfaces that put little descriptive 'popups' or 'expanders' next to inputs which, when clicked move focus into the div containing the contents for various reasons happens a bunch, I'd guess.

Again like for 006, I'm kind of sure we don't want to match :focus-visible in these situations.

robdodson commented 21 days ago •

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)

focus-visible-script-002 and 03: checks that if you have a document level click handler that moves focus to a div with a tab-index, this will not match focus visible. In both implementations, it does - but in the polyfill, it does not. In other words, Rego's test match the polyfill behavior, which does not match either implementation.

I think the browser implementation might be wrong.

The spec text says (I added emphasis):

If the active element matches :focus-visible, and a script causes focus to move elsewhere, the newly focused element should match :focus-visible.

If you click on a random part of the page then the activeElement will be body which does not match :focus-visible. So it's following this algorithm:

• The activeElement is body
• Does body match :focus-visible? No.
• Then the newly focused element should not match :focus-visible

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)

focus-visible-script-004 and 05: checks that if you have a document level key handler that moves focus to a div with a tab-index, this would not match... No implementation agrees with the test and my own intuition makes me think the test assertions are backward. I'd like to understand why rego interprets otherwise.

I think the browser implementation might be 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 :focus-visible. But it does violate the algorithm:

• The activeElement is body
• Does body match :focus-visible? No.
• Then the newly focused element should not match :focus-visible

Part of the problem is that in the initial state the activeElement is always body which never matches :focus-visible because it's non-interactive, so it's as if the user always starts in mouse modality. Whereas in the polyfill I don't think we check the previous activeElement, we just start the user in keyboard modality and toggle between the two modes if we see a mouse click or keyboard press.

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 :focus-visible?

006 (codepen)

focus-visible-script-006 and 007: checks that if (from a fresh state) you have a click handler on an element that isn't normally focusable and that programatically moves your focus to a div tabindex=0, that this will not match. These are among the more interesting ones for several reasons: First, because the tests, the polyfill, and my own intuition agree but neither browser implementation does. Second, because this is effectively a platform agnostic version of what will happen on some elements on mac by default. Like, basically it lets us talk about that problem without even talking about platforms... And we don't agree. Why?

I think the browser implementation might be 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 :focus-visible { outline: none }

008 (codepen) and 009 (codepen)

focus-visible-script-008 and 009: are an example of a pair where the blur matters: Everybody passes 06, but FF disagrees on 07 because a blur happened. I think we need to sort this out somehow by better defining because FF is err'ing on the wrong side. That is, if we show it when we aren't supposed to we're encouraging people to disable it.

I think Chrome's behavior is correct.

I don't think a programmatic blur or focus should opt the user out of mouse mode. The user hasn't sent any signal that they suddenly prefer using the keyboard.

010 (codepen)

focus-visible-script-010 and 011: Its an input element, you click it and the click handler moves focus programatically to a div tabindex 0. Chrome and the polyfill agree that this doesn't match, but the test and FF both think it should. 11 varies this by adding an explict blur, but we already don't agree on 10. Note that the polyfill spec stuff about programatic focus at all begins (I think) with WICG/focus-visible#88 and "It'll use whatever state it was previously in." - but I think the crux here is we haven't described the state well or how it is maintained.

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)

focus-visible-script-016 and 017: Given an input, a non-focusable div with a click handler and a focusable div - if you click on the non-focusable and that then sets your focus to the focusable div: should it match. Like 6 and 7, this is curious to me because because the tests, the polyfill, and my own intuition agree but neither browser implementation does and I don't understand why. Similarly, we know this case isn't probably rare - interfaces that put little descriptive 'popups' or 'expanders' next to inputs which, when clicked move focus into the div containing the contents for various reasons happens a bunch, I'd guess.

I think Chrome's behavior is correct. Same point as 008-009.

focus-visible-script-018 and 019: These are basically a variant of 16 and 17 except that the thing you click on actually is focusable. Here, everything agrees on 18, but it is another example of where FF disagrees with everything in 19 "because blur".

I think Chrome's behavior is correct. Same point as 008-009.

emilio commented 21 days ago

I don't think a programmatic blur or focus should opt the user out of mouse mode. The user hasn't sent any signal that they suddenly prefer using the keyboard.

I think Chrome's behavior is correct.

I don't think a programmatic blur or focus should opt the user out of mouse mode. The user hasn't sent any signal that they suddenly prefer using the keyboard.

Correct according to what? That's not what the spec says afaict. If this behavior is more desirable, then we should fix the spec.

bkardell commented 21 days ago

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.

alice commented 21 days ago

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?

mrego commented 21 days ago

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:

• If the user's last interaction with the page would cause an element to match :focus-visible, and a script causes focus to move elsewhere, the newly focused element should match :focus-visible.

• Conversely, if the user's last interaction with the page would cause an element to to not match :focus-sible, and a script causes focus to move elsewhere, the newly focused element should not match :focus-visible.

The question is to clarify which is "the element the user last interacted with" in the different test cases explained before.
Also I'm not sure that's what we want, for example in test 010. The test is a click on an <input>, and the click event handler moves focus elsewhere. It looks we agree that the newly focused element shouldn't match :focus-visible, but the element the user last interacted with is the <input> and it does match :focus-visible always.

I believe we need to add something related to the mouse modality or mouse mode that Rob is suggesting on his comment.

mrego commented 21 days ago

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:

• 004 & 005 now check that :focus-visible matches. They PASS in both Chromium and Firefox.
• 010 & 011 now check that :focus-visible does NOT match. They PASS in Chromium and FAIL in Firefox.

The rest of tests are unchanged.

emilio commented 20 days ago

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.

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.

It looks like @mrego updated the html spec with the following snippet:

If a script causes focus to be set:
If the last time focus update steps have been run focus trigger was "<code data-x="">click</code>", don't indicate focus.

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.

mrego commented 7 days ago

It looks like we have agreement on the expected behavior for these tests.

Would people be fine landing these set of tests using .tentative suffix? If so, can someone review this PR? Thanks!

emilio commented 6 days ago

So I'm not sure about the tests that Firefox Nightly fails with this PR applied:

Running 19 tests in web-platform-tests
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-002.html:
  │ FAIL [expected PASS] Script focus after mouse click does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-002.html:57:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-002.html:52:12
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-003.html:
  │ FAIL [expected PASS] Script focus after blur after mouse click does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-003.html:58:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-003.html:53:12
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-006.html:
  │ FAIL [expected PASS] Script focus after mouse click on a NOT focusable element does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-006.html:58:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-006.html:53:12
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-007.html:
  │ FAIL [expected PASS] Script focus after blur after mouse click on a NOT focusable element does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-007.html:59:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-007.html:54:12
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-016.html:
  │ FAIL [expected PASS] Script focus after mouse click on a NOT focusable element after editing an input does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-016.html:64:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-016.html:55:12
 
  ▶ Unexpected subtest result in /css/selectors/focus-visible-script-focus-017.html:
  │ FAIL [expected PASS] Script focus after blur after mouse click on a NOT focusable element after editing an input does NOT match :focus-visible
  │   → assert_equals: backgroundColor for DIV#target should be lime expected "rgb(0, 255, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-017.html:65:20
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2095:25
  │ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:2138:32
  └ @http://web-platform.test:8000/css/selectors/focus-visible-script-focus-017.html:56:12
 
Ran 19 tests finished in 28.9 seconds.
  • 13 ran as expected. 0 tests skipped.
  • 6 tests had unexpected subtest results

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?