Bug 223113

Summary: [selectors] :focus-visible matches body after keyboard event
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: CSSAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, darin, ews-watchlist, koivisto, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/28051
Bug Depends on:    
Bug Blocks: 185859    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Manuel Rego Casasnovas 2021-03-12 03:47:10 PST
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.
Comment 1 Manuel Rego Casasnovas 2021-03-12 04:05:20 PST
Created attachment 423032 [details]
Patch
Comment 2 EWS Watchlist 2021-03-12 04:06:39 PST
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
Comment 3 Antti Koivisto 2021-03-12 04:20:44 PST
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.
Comment 4 Manuel Rego Casasnovas 2021-03-12 05:05:04 PST
Created attachment 423035 [details]
Patch for landing
Comment 5 Manuel Rego Casasnovas 2021-03-12 05:06:02 PST
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.
Comment 6 Manuel Rego Casasnovas 2021-03-12 07:06:14 PST
Created attachment 423045 [details]
Patch for landing
Comment 7 EWS 2021-03-12 11:03:29 PST
Committed r274365: <https://commits.webkit.org/r274365>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423045 [details].
Comment 8 Radar WebKit Bug Importer 2021-03-12 11:04:19 PST
<rdar://problem/75368955>