Bug 222028 – [selectors] :focus-visible implementation

• Component: CSS
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Add basic :focus-visible implementation behind the FocusVisibleEnabled experimental flag.

Spec: https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo

[Manuel Rego Casasnovas]

Created attachment 420600 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 420600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420600&action=review

> Source/WebCore/ChangeLog:9
> +        This patch implements the heuristics defined on the spec (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo).

Defined in?

> Source/WebCore/dom/Element.cpp:753
> +        setHasFocusVisible(false);

You are not always calling setHasFocusVisible(...), is that on purpose?

Maybe you meant

setHasFocusVisible(flag && (isTextField() || isContentEditable()))

?

> Source/WebCore/dom/Element.cpp:3041
> +    bool newlyFocusedElementShouldMatchFocusVisible = document->focusedElement() ? document->focusedElement()->hasFocusVisible() : true;

This can be

bool newlyFocusedElementShouldMatchFocusVisible = !document->focusedElement() || document->focusedElement()->hasFocusVisible();

[Manuel Rego Casasnovas]

Comment on attachment 420600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420600&action=review

Thanks for the review.

>> Source/WebCore/ChangeLog:9
>> +        This patch implements the heuristics defined on the spec (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo).
> 
> Defined in?

Fixed.

>> Source/WebCore/dom/Element.cpp:753
>> +        setHasFocusVisible(false);
> 
> You are not always calling setHasFocusVisible(...), is that on purpose?
> 
> Maybe you meant
> 
> setHasFocusVisible(flag && (isTextField() || isContentEditable()))
> 
> ?

No we cannot do that.

"setHasFocusVisible(true)" might have been called before Element::setFocus(), and we don't want to set it to false if it doesn't support keyboard input.
For example, if we're focusing with TAB a <DIV tabindex>, setHasFocusVisible(true) was already called in FocusController::advanceFocusInDocumentOrder(), and we don't want to set it to false here (like it would happen in your proposal).

I'm modifying the ChangeLog entry to explain this part.

>> Source/WebCore/dom/Element.cpp:3041
>> +    bool newlyFocusedElementShouldMatchFocusVisible = document->focusedElement() ? document->focusedElement()->hasFocusVisible() : true;
> 
> This can be
> 
> bool newlyFocusedElementShouldMatchFocusVisible = !document->focusedElement() || document->focusedElement()->hasFocusVisible();

Done.

[Manuel Rego Casasnovas]

Created attachment 420620 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Darin Adler]

Comment on attachment 420620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420620&action=review

This seems baroque and strange. Where can I read this specification? It seems to me that :focus-visible should be based on *invariants*, not based on state transitions. The fact that the implementation causes one focused item to sort of inherit from the last seems wrong to me, and especially the fact that typing a key can take an already-focused element and change it to focus-visible, even if the key ends up having no other effect.

I suspect some of this may be due to the fact that we follow some Mac platform standards and other browsers embrace Windows platform standards (that’s where the button focusing difference comes from) and the strange algorithm is needed to bridge some of the differences.

But if not, where does this strange need come from?

> Source/WebCore/dom/Element.cpp:766
> +    {
> +        Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClassFocusVisible);
> +        setNodeFlag(NodeFlag::HasFocusVisible, flag);
> +    }

No need for these braces, but if you think it’s clearer with them, OK.

> Source/WebCore/dom/Element.cpp:2045
> +    if (is<KeyboardEvent>(event) && (event.type() == eventNames().keypressEvent) && focused()) {

We normally would not include the parentheses here.

[Darin Adler]

I’ve now started reading https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo

[Darin Adler]

Comment on attachment 420620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420620&action=review

> Source/WebCore/dom/Element.cpp:2046
> +        // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.

This code doesn’t seem to correspond to "if the user interacts with the page via the keyboard".

To implement that rule, it should not be a default event handler of an element. It needs to go somewhere broader and less limited, something where preventing default behavior won’t stop it, for example.

[Manuel Rego Casasnovas]

Hi Darin, thanks for the review!

Yeah as you mention this is defined in the following spec:
https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo

The main goal of :focus-visible is to avoid the bad pattern of doing ":focus { outline: none; }" on a website, to avoid getting focus indicators in some elements that the web authors don't like.
It's a kind of a11y feature, so the focus-indicator would only be showed when it's actually needed. The people could style it with :focus-visible.

The patch is implementing the heuristics, which are non-normative. I believe these come from some a11y research done while defining this feature.
Anyway the current implementations in Chromium and Firefox follow them, if we could do the same in WebKit that would be great for the web authors.

I can maybe split this patch, and only implement the very clear ones. And implement in separated patches the more controversial ones.
Some are actually under discussion on the CSSWG regarding some details, see 
https://github.com/w3c/csswg-drafts/issues/5885.

Would that makes sense?

(In reply to Darin Adler from comment #8)
> Comment on attachment 420620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420620&action=review
> 
> > Source/WebCore/dom/Element.cpp:2046
> > +        // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
> 
> This code doesn’t seem to correspond to "if the user interacts with the page
> via the keyboard".
> 
> To implement that rule, it should not be a default event handler of an
> element. It needs to go somewhere broader and less limited, something where
> preventing default behavior won’t stop it, for example.

There's a test that calls preventDefault() (focus-visible-011.html) and it's now working right now, but due to a different issue, I was planning to rework the test in a follow-up patch.
I believe I've tested that this was still working with preventDefault() but I'll double-check.
If you had an idea of where we should do this, I'll be happy to hear about that.

[Manuel Rego Casasnovas]

Created attachment 420950 [details]
Patch

[Manuel Rego Casasnovas]

> (In reply to Darin Adler from comment #8)
> > Comment on attachment 420620 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420620&action=review
> > 
> > > Source/WebCore/dom/Element.cpp:2046
> > > +        // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
> > 
> > This code doesn’t seem to correspond to "if the user interacts with the page
> > via the keyboard".
> > 
> > To implement that rule, it should not be a default event handler of an
> > element. It needs to go somewhere broader and less limited, something where
> > preventing default behavior won’t stop it, for example.
> 
> There's a test that calls preventDefault() (focus-visible-011.html) and it's
> now working right now, but due to a different issue, I was planning to
> rework the test in a follow-up patch.
> I believe I've tested that this was still working with preventDefault() but
> I'll double-check.
> If you had an idea of where we should do this, I'll be happy to hear about
> that.

I moved this to Element::dispatchKeyEvent() that is even called when preventDefault() is called.

Anyway the test needs improvements, I'll send a WPT PR to fix it.

[Manuel Rego Casasnovas]

Created attachment 420963 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #11)
> Anyway the test needs improvements, I'll send a WPT PR to fix it.

Fixed focus-visible-011.html test in WPT, now it test event.preventDefault() properly and also run on Mac (where buttons are not focused on click).

[Darin Adler]

(In reply to Manuel Rego Casasnovas from comment #11)
> I moved this to Element::dispatchKeyEvent() that is even called when
> preventDefault() is called.

But why should it only be key events dispatched to the focused elements? What about key events dispatched to other elements?

[Manuel Rego Casasnovas]

(In reply to Darin Adler from comment #14)
> (In reply to Manuel Rego Casasnovas from comment #11)
> > I moved this to Element::dispatchKeyEvent() that is even called when
> > preventDefault() is called.
> 
> But why should it only be key events dispatched to the focused elements?
> What about key events dispatched to other elements?

Not sure how we can dispatch a key event to something that is not focused, is that possible? I'll need to do some tests, but maybe the check for focused() is not needed actually.

Here, I'm trying to implement this from the spec (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo):
> * If the user interacts with the page via keyboard or some other non-pointing device, indicate focus. (This means keyboard usage may change whether this pseudo-class matches even if it doesn’t affect :focus).

[Darin Adler]

(In reply to Manuel Rego Casasnovas from comment #15)
> Not sure how we can dispatch a key event to something that is not focused,
> is that possible? I'll need to do some tests, but maybe the check for
> focused() is not needed actually.
> 
> Here, I'm trying to implement this from the spec
> (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo):
> > * If the user interacts with the page via keyboard or some other non-pointing device, indicate focus. (This means keyboard usage may change whether this pseudo-class matches even if it doesn’t affect :focus).

Yes, and I’d expect the code for this to go somewhere like EventHandler, not in the DOM.

[Manuel Rego Casasnovas]

Created attachment 421177 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Darin Adler from comment #16)
> (In reply to Manuel Rego Casasnovas from comment #15)
> > Not sure how we can dispatch a key event to something that is not focused,
> > is that possible? I'll need to do some tests, but maybe the check for
> > focused() is not needed actually.
> > 
> > Here, I'm trying to implement this from the spec
> > (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo):
> > > * If the user interacts with the page via keyboard or some other non-pointing device, indicate focus. (This means keyboard usage may change whether this pseudo-class matches even if it doesn’t affect :focus).
> 
> Yes, and I’d expect the code for this to go somewhere like EventHandler, not
> in the DOM.

Move this to EventHandler::internalKeyEvent().

[Manuel Rego Casasnovas]

Created attachment 421235 [details]
Patch

[Manuel Rego Casasnovas]

EWS are green. PTAL. Thanks.

[Radar WebKit Bug Importer]

<rdar://problem/74679243>

[Darin Adler]

Comment on attachment 421235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421235&action=review

> Source/WebCore/css/SelectorCheckerTestFunctions.h:475
>  ALWAYS_INLINE bool matchesFocusVisiblePseudoClass(const Element& element)

I don’t understand why this function already existed, with an inspector-only implementation. Do you know why?

> Source/WebCore/dom/Element.cpp:3031
> +    // For script focus the newly focused element should match (or not) :focus-visible if the active element matches (or not) :focus-visible.
> +    // If there's no active element then script focus always matches :focus-visible.
> +    bool newlyFocusedElementShouldMatchFocusVisible = !document->focusedElement() || document->focusedElement()->hasFocusVisible();

This doesn’t seem right. Why is it a good design to check the state of the currently focused element here? Isn’t there a way to determine if the newly focused element should match that is independent of the previously focused one? If so, we should do that. It seems really strange that "unfocusing" one element then focusing another would give a different result from focusing the other without explicitly "unfocusing" first.

Perhaps this does achieve the correct result, but I don’t understand it fully.

> Source/WebCore/dom/Element.cpp:3068
> +        if (!newTarget->hasFocusVisible())
> +            newTarget->setHasFocusVisible(newlyFocusedElementShouldMatchFocusVisible);

This is kind of strange. It calls setHasFocusVisible(false) even if hasFocusVisible is already false. Could write this in a clearer way. But the issue above is the bigger one.

[Manuel Rego Casasnovas]

Comment on attachment 421235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421235&action=review

Thanks for the review. Replied inline.

>> Source/WebCore/css/SelectorCheckerTestFunctions.h:475
>>  ALWAYS_INLINE bool matchesFocusVisiblePseudoClass(const Element& element)
> 
> I don’t understand why this function already existed, with an inspector-only implementation. Do you know why?

This was added with the basic parsing for :focus-visible on the previous patch (see r272983).

>> Source/WebCore/dom/Element.cpp:3031
>> +    bool newlyFocusedElementShouldMatchFocusVisible = !document->focusedElement() || document->focusedElement()->hasFocusVisible();
> 
> This doesn’t seem right. Why is it a good design to check the state of the currently focused element here? Isn’t there a way to determine if the newly focused element should match that is independent of the previously focused one? If so, we should do that. It seems really strange that "unfocusing" one element then focusing another would give a different result from focusing the other without explicitly "unfocusing" first.
> 
> Perhaps this does achieve the correct result, but I don’t understand it fully.

You have a good point here.

This is trying to implement this part of the spec (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo):
"If the previously-focused element indicated focus, and a script causes focus to move elsewhere, the newly focused element should indicate focus.
 Conversely, if the previously-focused element did not indicate focus, and a script causes focus to move elsewhere, the newly focused element should also not indicate focus."

That's why it check the status of the current element, to see what to do with the next one.


However it's true what you say, imagine the following scenarios:
1) User clicks on a <DIV id=1 tabindex>, so the element does't match :focus-visible
2) A script moves focus to another <DIV id=2 tabindex>, the DIV#2 element now doesn't match :focus-visible.

But:
1) User clicks on a <DIV id=1 tabindex>, so the element does't match :focus-visible
2) A script calls blur() and then moves focus to another <DIV id=2 tabindex>, the DIV#2 element will match :focus-visible.

I agree that this is strange.


This particular issue is being discussed on the CSSWG (see https://github.com/w3c/csswg-drafts/issues/5885), thought it has been suggested to move this to the HTML spec. In any case the proposal there is a little bit different to the current spec text:
"If the user has not interacted with the page, and a script (or similar behavior via autofocus) causes focus to be set, the newly focused element should match :focus-visible
  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."

With this proposals both scenarios described above would behave the same. And the DIV#2 element won't match :focus-visible in any of the cases.

But I'm not implementing that yet, as it was not approved or discussed in either CSSWG or HTML spec.

Not sure what would be the best idea so far, keep this initial implementation or avoid implementing anything related to this until the spec text is clarified? WDYT?

>> Source/WebCore/dom/Element.cpp:3068
>> +            newTarget->setHasFocusVisible(newlyFocusedElementShouldMatchFocusVisible);
> 
> This is kind of strange. It calls setHasFocusVisible(false) even if hasFocusVisible is already false. Could write this in a clearer way. But the issue above is the bigger one.

I could write this like:
        if (!newTarget->hasFocusVisible() && newlyFocusedElementShouldMatchFocusVisible)
            newTarget->setHasFocusVisible(newlyFocusedElementShouldMatchFocusVisible);

[Darin Adler]

Comment on attachment 421235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421235&action=review

>>> Source/WebCore/dom/Element.cpp:3031
>>> +    bool newlyFocusedElementShouldMatchFocusVisible = !document->focusedElement() || document->focusedElement()->hasFocusVisible();
>> 
>> This doesn’t seem right. Why is it a good design to check the state of the currently focused element here? Isn’t there a way to determine if the newly focused element should match that is independent of the previously focused one? If so, we should do that. It seems really strange that "unfocusing" one element then focusing another would give a different result from focusing the other without explicitly "unfocusing" first.
>> 
>> Perhaps this does achieve the correct result, but I don’t understand it fully.
> 
> You have a good point here.
> 
> This is trying to implement this part of the spec (https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo):
> "If the previously-focused element indicated focus, and a script causes focus to move elsewhere, the newly focused element should indicate focus.
>  Conversely, if the previously-focused element did not indicate focus, and a script causes focus to move elsewhere, the newly focused element should also not indicate focus."
> 
> That's why it check the status of the current element, to see what to do with the next one.
> 
> 
> However it's true what you say, imagine the following scenarios:
> 1) User clicks on a <DIV id=1 tabindex>, so the element does't match :focus-visible
> 2) A script moves focus to another <DIV id=2 tabindex>, the DIV#2 element now doesn't match :focus-visible.
> 
> But:
> 1) User clicks on a <DIV id=1 tabindex>, so the element does't match :focus-visible
> 2) A script calls blur() and then moves focus to another <DIV id=2 tabindex>, the DIV#2 element will match :focus-visible.
> 
> I agree that this is strange.
> 
> 
> This particular issue is being discussed on the CSSWG (see https://github.com/w3c/csswg-drafts/issues/5885), thought it has been suggested to move this to the HTML spec. In any case the proposal there is a little bit different to the current spec text:
> "If the user has not interacted with the page, and a script (or similar behavior via autofocus) causes focus to be set, the newly focused element should match :focus-visible
>   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."
> 
> With this proposals both scenarios described above would behave the same. And the DIV#2 element won't match :focus-visible in any of the cases.
> 
> But I'm not implementing that yet, as it was not approved or discussed in either CSSWG or HTML spec.
> 
> Not sure what would be the best idea so far, keep this initial implementation or avoid implementing anything related to this until the spec text is clarified? WDYT?

My preference would be:

1) to implement a sensible rule that matches what the specification behavior calls for in all non-unusual cases
2) make sure that we cover the unusual cases in Web Platform Tests instead of having them be untested
3) not worry unduly about exactly matching a specification that doesn’t make really good sense and monitor progress on the specification so we are sure to eventually have interoperability with the other web browsers

I don’t like implementing literally what the specification says when it doesn’t pass the "common sense" rule.

>>> Source/WebCore/dom/Element.cpp:3068
>>> +            newTarget->setHasFocusVisible(newlyFocusedElementShouldMatchFocusVisible);
>> 
>> This is kind of strange. It calls setHasFocusVisible(false) even if hasFocusVisible is already false. Could write this in a clearer way. But the issue above is the bigger one.
> 
> I could write this like:
>         if (!newTarget->hasFocusVisible() && newlyFocusedElementShouldMatchFocusVisible)
>             newTarget->setHasFocusVisible(newlyFocusedElementShouldMatchFocusVisible);

I would write this:

    if (newlyFocusedElementShouldMatchFocusVisible)
        newTarget->setHasFocusVisible(true);

You might prefer this longer version:

    if (!newTarget->hasFocusVisible() && newlyFocusedElementShouldMatchFocusVisible)
        newTarget->setHasFocusVisible(true);

[Manuel Rego Casasnovas]

Created attachment 421639 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Darin Adler from comment #24)
> > Not sure what would be the best idea so far, keep this initial implementation or avoid implementing anything related to this until the spec text is clarified? WDYT?
> 
> My preference would be:
> 
> 1) to implement a sensible rule that matches what the specification behavior
> calls for in all non-unusual cases
> 2) make sure that we cover the unusual cases in Web Platform Tests instead
> of having them be untested
> 3) not worry unduly about exactly matching a specification that doesn’t make
> really good sense and monitor progress on the specification so we are sure
> to eventually have interoperability with the other web browsers

This plan sounds good, but I've been thinking about 1) and I'm not sure what a sensible rule would be when I start to think in more cases than the basic ones...

So I've been thinking that maybe we could land the rest of the patch, that adds support for :focus-visible behind an experimental flag. And deal with the script focus in a separated bug. I'm going to upload a new version of the patch without this part.

My plan regarding script focus is to write a bunch of WPT tests covering both the basic cases, and the unusual ones too. And check how those tests work in other browsers, and if people from different implementations agree on what are the expectations for each of them. And then once there's some kind of agreement around that work on a new patch that passes all those tests.

If you prefer to wait for that before landing anything here, that could be an option too, but I'll need some time to come back with the tests and some kind of common understanding on how they should work.

So let me know your thoughts about the best way to proceed.


> I don’t like implementing literally what the specification says when it
> doesn’t pass the "common sense" rule.

Yeah, that's a good point.

[Manuel Rego Casasnovas]

Created attachment 421648 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #26)
> My plan regarding script focus is to write a bunch of WPT tests covering
> both the basic cases, and the unusual ones too. And check how those tests
> work in other browsers, and if people from different implementations agree
> on what are the expectations for each of them. And then once there's some
> kind of agreement around that work on a new patch that passes all those
> tests.

Here is a PR with 19 new tests checking different common and uncommon situations. Once we have a clear agreement on that I'll try to implement that on the WebKit side.

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #28)
> (In reply to Manuel Rego Casasnovas from comment #26)
> > My plan regarding script focus is to write a bunch of WPT tests covering
> > both the basic cases, and the unusual ones too. And check how those tests
> > work in other browsers, and if people from different implementations agree
> > on what are the expectations for each of them. And then once there's some
> > kind of agreement around that work on a new patch that passes all those
> > tests.
> 
> Here is a PR with 19 new tests checking different common and uncommon
> situations. Once we have a clear agreement on that I'll try to implement
> that on the WebKit side.

Forgot the link to the PR: https://github.com/web-platform-tests/wpt/pull/27806

[Darin Adler]

Looks like a couple more test results need to be rebased.

[Manuel Rego Casasnovas]

Created attachment 421673 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Darin Adler from comment #30)
> Looks like a couple more test results need to be rebased.

Yeah, uploaded new version, let's see if now things are green (as I'm running the tests in WebKitGTK and some things vary).

[Manuel Rego Casasnovas]

Rebaselined expectations. EWSs are now green. PTAL, thanks.

[Darin Adler]

Comment on attachment 421673 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421673&action=review

The code has one significant mistake, but I think it’s helpful to land this soon anyway.

> Source/WebCore/page/EventHandler.cpp:3538
> +    // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
> +    // Just typing a modifier key is not considered user interaction with the page.
> +    if (!keydown->ctrlKey() && !keydown->metaKey() && !keydown->altGraphKey() && !keydown->shiftKey())
> +        element->setHasFocusVisible(true);

The code here does not match the comment.

The checks here are checking if the key down is "with the control, meta, alt graph, or shift key down". But the comment is talking about events where the event itself is due to pressing one of those modifier keys. With the code like this, typing A with the shift key down is will not set focus visible. That is definitely wrong.

I’m pretty sure that we don’t dispatch keydown events when the change is only a change to a modifier key. If I am right, then no check is needed. If I am wrong, the check needs to be different from the one above.

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to Darin Adler from comment #34)
> Comment on attachment 421673 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421673&action=review
> 
> The code has one significant mistake, but I think it’s helpful to land this
> soon anyway.
> 
> > Source/WebCore/page/EventHandler.cpp:3538
> > +    // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
> > +    // Just typing a modifier key is not considered user interaction with the page.
> > +    if (!keydown->ctrlKey() && !keydown->metaKey() && !keydown->altGraphKey() && !keydown->shiftKey())
> > +        element->setHasFocusVisible(true);
> 
> The code here does not match the comment.
> 
> The checks here are checking if the key down is "with the control, meta, alt
> graph, or shift key down". But the comment is talking about events where the
> event itself is due to pressing one of those modifier keys. With the code
> like this, typing A with the shift key down is will not set focus visible.
> That is definitely wrong.

You're right this is code is wrong and is not setting :focus-visible for Shift + A.

> I’m pretty sure that we don’t dispatch keydown events when the change is
> only a change to a modifier key. If I am right, then no check is needed. If
> I am wrong, the check needs to be different from the one above.

I've checked and the keydown event is dispatched even when you just click "Shift" (or other modifier keys), so I'll need to find a proper way to avoid modifier keys here somehow.

[Manuel Rego Casasnovas]

Created attachment 421919 [details]
Patch for landing

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #35)
> Thanks for the review.
> 
> (In reply to Darin Adler from comment #34)
> > Comment on attachment 421673 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=421673&action=review
> > 
> > The code has one significant mistake, but I think it’s helpful to land this
> > soon anyway.
> > 
> > > Source/WebCore/page/EventHandler.cpp:3538
> > > +    // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
> > > +    // Just typing a modifier key is not considered user interaction with the page.
> > > +    if (!keydown->ctrlKey() && !keydown->metaKey() && !keydown->altGraphKey() && !keydown->shiftKey())
> > > +        element->setHasFocusVisible(true);
> > 
> > The code here does not match the comment.
> > 
> > The checks here are checking if the key down is "with the control, meta, alt
> > graph, or shift key down". But the comment is talking about events where the
> > event itself is due to pressing one of those modifier keys. With the code
> > like this, typing A with the shift key down is will not set focus visible.
> > That is definitely wrong.
> 
> You're right this is code is wrong and is not setting :focus-visible for
> Shift + A.
> 
> > I’m pretty sure that we don’t dispatch keydown events when the change is
> > only a change to a modifier key. If I am right, then no check is needed. If
> > I am wrong, the check needs to be different from the one above.
> 
> I've checked and the keydown event is dispatched even when you just click
> "Shift" (or other modifier keys), so I'll need to find a proper way to avoid
> modifier keys here somehow.

I've changed this check by:

    if (keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()))
        element->setHasFocusVisible(true);

This sets focus-visible for "Shift + a", but also when Caps Lock is set and you type "a". However it won't set it for Shift alone, or Caps Lock alone (or other modifiers alone or in combination with other keys).

[Manuel Rego Casasnovas]

Created attachment 421926 [details]
Patch for landing

[Antti Koivisto]

Comment on attachment 421926 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=421926&action=review

> Source/WebCore/dom/Element.cpp:753
> +    if (flag) {
> +        // Elements that support keyboard input (form inputs and contenteditable) always match :focus-visible when focused.
> +        if (isTextField() || isContentEditable())
> +            setHasFocusVisible(true);
> +    } else
> +        setHasFocusVisible(false);

I'd use a lambda along the lines of

auto computeHasFocusVisible = [&] {
    if (!flag)
         return false;
    return isTextField() || isContentEditable();
};
setHasFocusVisible(computeHasFocusVisible()):

> Source/WebCore/dom/Element.h:322
>      bool hovered() const { return isUserActionElement() && isUserActionElementHovered(); }
>      bool focused() const { return isUserActionElement() && isUserActionElementFocused(); }
>      bool isBeingDragged() const { return isUserActionElement() && isUserActionElementDragged(); }
> +    bool hasFocusVisible() const { return hasNodeFlag(NodeFlag::HasFocusVisible); };

This could use UserActionElementSet similar to focused/hovered. On the other hand I think that exist because earlier Node flag shortage. We currently have a bunch free so it would be mostly for consistency. I guess adding a node flag is fine for now too.

> Source/WebCore/page/FrameView.cpp:2277
> -        if (anchorElement->isFocusable())
> +        if (anchorElement->isFocusable()) {
> +            anchorElement->setHasFocusVisible(true);
>              document.setFocusedElement(anchorElement.get());
> -        else {
> +        } else {
>              document.setFocusedElement(nullptr);
>              document.setFocusNavigationStartingNode(anchorElement.get());
>          }

Why doesn't the other branch clear the flag?

> Source/WebCore/style/ElementRuleCollector.cpp:161
> +    if (matchesFocusVisiblePseudoClass(element()))
> +        collectMatchingRulesForList(matchRequest.ruleSet->focusVisiblePseudoClassRules(), matchRequest);

I don't think you need any of the ElementRuleCollector and RuleSet changes in this patch. Focus is popular as universal rule *:focus so it has this special optimization. None of this should be needed for functionality and is better done separately if needed.

[Manuel Rego Casasnovas]

Created attachment 421956 [details]
Patch

[Antti Koivisto]

> > Source/WebCore/page/FrameView.cpp:2277
> > -        if (anchorElement->isFocusable())
> > +        if (anchorElement->isFocusable()) {
> > +            anchorElement->setHasFocusVisible(true);
> >              document.setFocusedElement(anchorElement.get());
> > -        else {
> > +        } else {
> >              document.setFocusedElement(nullptr);
> >              document.setFocusNavigationStartingNode(anchorElement.get());
> >          }
> 
> Why doesn't the other branch clear the flag?

Actually doesn't document.setFocusedElement() call Element:::setFocus() making the separate setHasFocusVisible call here redundant?

[Manuel Rego Casasnovas]

Comment on attachment 421926 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=421926&action=review

Thanks for the review Antii, uploaded a new version applying your suggestions. PTAL.

>> Source/WebCore/dom/Element.cpp:753
>> +        setHasFocusVisible(false);
> 
> I'd use a lambda along the lines of
> 
> auto computeHasFocusVisible = [&] {
>     if (!flag)
>          return false;
>     return isTextField() || isContentEditable();
> };
> setHasFocusVisible(computeHasFocusVisible()):

I've used a lambda, but I have to add hasFocusVisible() as condition in the last return. Otherwise we'll be setting it to false when we don't want, for example if we focus with TAB a DIV#tabindex, it's not a text field or a contenteditable but it should still match :focus-visible.

>> Source/WebCore/dom/Element.h:322
>> +    bool hasFocusVisible() const { return hasNodeFlag(NodeFlag::HasFocusVisible); };
> 
> This could use UserActionElementSet similar to focused/hovered. On the other hand I think that exist because earlier Node flag shortage. We currently have a bunch free so it would be mostly for consistency. I guess adding a node flag is fine for now too.

Good point. This is consistent with :focus-within thought. :-)

What about changing both :focus-visible and :focus-within in a follow up patch?

>> Source/WebCore/page/FrameView.cpp:2277
>>          }
> 
> Why doesn't the other branch clear the flag?

We could clear it there but it doesn't seem required.

In the else branch we're going to be end up calling Element::focus(false), if there's any focused element right now. That would clear the flag for that element (if it's the anchorElement or any other). So I don't think we need to do anything here.

>> Source/WebCore/style/ElementRuleCollector.cpp:161
>> +        collectMatchingRulesForList(matchRequest.ruleSet->focusVisiblePseudoClassRules(), matchRequest);
> 
> I don't think you need any of the ElementRuleCollector and RuleSet changes in this patch. Focus is popular as universal rule *:focus so it has this special optimization. None of this should be needed for functionality and is better done separately if needed.

:focus-visible is expected to replace :focus (now :-webkit-direct-focus in WebKit) in the default UA style sheet. Anyway it's true this is not needed for functionality for this patch, and if we want to add this, we could do it later. I've removed it from the patch.

[Manuel Rego Casasnovas]

(In reply to Antti Koivisto from comment #41)
> > > Source/WebCore/page/FrameView.cpp:2277
> > > -        if (anchorElement->isFocusable())
> > > +        if (anchorElement->isFocusable()) {
> > > +            anchorElement->setHasFocusVisible(true);
> > >              document.setFocusedElement(anchorElement.get());
> > > -        else {
> > > +        } else {
> > >              document.setFocusedElement(nullptr);
> > >              document.setFocusNavigationStartingNode(anchorElement.get());
> > >          }
> > 
> > Why doesn't the other branch clear the flag?
> 
> Actually doesn't document.setFocusedElement() call Element:::setFocus()
> making the separate setHasFocusVisible call here redundant?

Element:::setFocus() is not always going to always end up calling setHasFocusVisible(true|false).

If we navigate with the keybaord, or in this case with anchors, we call setHasFocusVisible(true) before Element:::setFocus().
Element:::setFocus() will only call setHasFocusVisible(true) for inputs and contenteditable elements. Or call setHasFocusVisible(false) if the element loses focus.

[Antti Koivisto]

> :focus-visible is expected to replace :focus (now :-webkit-direct-focus in
> WebKit) in the default UA style sheet. Anyway it's true this is not needed
> for functionality for this patch, and if we want to add this, we could do it
> later. I've removed it from the patch.

Ok, in that case optimizing for it makes sense. Still better done separately.

[EWS]

Committed r273812: <https://commits.webkit.org/r273812>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421956 [details].