Bug 223113 – [selectors] :focus-visible matches body after keyboard event

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

There are 2 situations in which :focus-visible matches body when it shouldn't:
1) You just load a page and use TAB to focus the first element: This makes :focus-visible to match in the newly focused element but also in the body.
2) If in a keyboard event handler, nobody was focused initially, and the event handler changes focus to a different element. Again both the newly focused element and the body match :focus-visible.

This is a bug on how we handle the keyboard events, because document.body is not marked as focused initially, and that causes the problem.

[Manuel Rego Casasnovas]

Created attachment 423032 [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

[Antti Koivisto]

Comment on attachment 423032 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:3543
> +    bool shouldMatchFocusVisible = keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
> +    element->setHasFocusVisible(shouldMatchFocusVisible && element->focused());

It would be nicer to make shouldMatchFocusVisible a lambda that covers the element->focused() test too.

> Source/WebCore/page/EventHandler.cpp:3594
> +        element->setHasFocusVisible(shouldMatchFocusVisible && element->focused());

...invoked again here.

[Manuel Rego Casasnovas]

Created attachment 423035 [details]
Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 423032 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/page/EventHandler.cpp:3543
>> +    element->setHasFocusVisible(shouldMatchFocusVisible && element->focused());
> 
> It would be nicer to make shouldMatchFocusVisible a lambda that covers the element->focused() test too.

Done.

>> Source/WebCore/page/EventHandler.cpp:3594
>> +        element->setHasFocusVisible(shouldMatchFocusVisible && element->focused());
> 
> ...invoked again here.

Done.

[Manuel Rego Casasnovas]

Created attachment 423045 [details]
Patch for landing

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/75368955>