[webkit-reviews] review granted: [Bug 222028] [selectors] :focus-visible implementation : [Attachment 421673] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 222028: [selectors] :focus-visible implementation
https://bugs.webkit.org/show_bug.cgi?id=222028

Attachment 421673: Patch

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




--- Comment #34 from Darin Adler <darin at apple.com> ---
Comment on attachment 421673
  --> https://bugs.webkit.org/attachment.cgi?id=421673
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.