[selectors] Using a modifier key on an element makes it stop matching :focus-vis...

This is a bug in the current implementation, if you are typing on an <input> it's matching :focus-visible.
But if you type Ctrl key alone (or another modifier key), the element stops matching :focus-visible.
And <input> should always match :focus-visible when focused.

This happens with any element.

For example if a script caused an element to be focused, the element matches :focus-visible. Then if you type Ctrl it stops to match :focus-visbile.
This is a bug in EventHandler::internalKeyEvent(), we shouldn't do anything when the user types a key if the element is already matching :focus-visible.

Created attachment 427130 [details]
Patch

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

Created attachment 427132 [details]
Patch

Created attachment 427136 [details]
Patch

Comment on attachment 427136 [details]
Patch

Looks like tests are hitting assertion failures in Element::setHasFocusVisible. Let’s review a version after this is fixed.

Created attachment 427190 [details]
Patch

(In reply to Darin Adler from comment #6)
> Comment on attachment 427136 [details]
> Patch
> 
> Looks like tests are hitting assertion failures in
> Element::setHasFocusVisible. Let’s review a version after this is fixed.

Yes sorry, one of the asserts was wrong. The new version should have fixed the crashes.

Comment on attachment 427190 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This patches fixed that but avoiding doing any work if the element is already matching :focus-visible when the user type a key.

but *avoids* or *that without doing*

> Source/WebCore/dom/Element.cpp:751
> +    // Elements that support keyboard input (form inputs and contenteditable) always match :focus-visible when focused.
> +    return element.isTextField() || element.isContentEditable();

I guess we can discuss in another issue but why is this special case needed / necessary?
It seems wrong that a shadow host with contenteditable will automatically get a visible focus,
much less any focusable element that's in an editable region.

> Source/WebCore/dom/Element.cpp:783
> +    if (flag)
> +        ASSERT(focused());

This is just:
ASSERT(!flag || focused());
Or alternatively:
ASSERT(!(flag && !focused()));

> Source/WebCore/dom/Element.cpp:785
> +    if (focused() && shouldAlwaysHaveFocusVisibleWhenFocused(*this))
> +        ASSERT(flag);

Similarly here, we can just do:
ASSERT(!focused() || !shouldAlwaysHaveFocusVisibleWhenFocused(*this) || flag);

> Source/WebCore/page/EventHandler.cpp:3548
> +        if (!element.focused() || element.hasFocusVisible())
> +            return;

It's not really good to create this kind of state dependency.
This code relies on some other code to have called setHasFocusVisible, and it adds more conditions on top of it.
I think what we're trying to do is to set hasFocusVisible if isn't already set.
It's far clearer to write code like this instead:
bool userHasInteractedViaKeyword = keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
if (element->focused() && userHasInteractedViaKeyword)
    element->setHasFocusVisible(true);

> Source/WebCore/page/EventHandler.cpp:3552
>          // 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, but Shift + a (or Caps Lock + a) is considered an interaction.
> -        return keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
> +        element.setHasFocusVisible(keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()));

This set of key combinations seem rather arbitrary to me.
Where does it come from?

Created attachment 427238 [details]
Patch

Comment on attachment 427190 [details]
Patch

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

Thanks for the review, uploaded new version applying suggested changes.

>> Source/WebCore/ChangeLog:11
>> +        This patches fixed that but avoiding doing any work if the element is already matching :focus-visible when the user type a key.
> 
> but *avoids* or *that without doing*

Done.

>> Source/WebCore/dom/Element.cpp:751
>> +    return element.isTextField() || element.isContentEditable();
> 
> I guess we can discuss in another issue but why is this special case needed / necessary?
> It seems wrong that a shadow host with contenteditable will automatically get a visible focus,
> much less any focusable element that's in an editable region.

Yeah I guess we could discuss this on a different bug, so I'm not seeing the kind of issue you mention.

For example right now if we have "<div contenteditable><p>foo</p><input></div>" and we set ":focus-visible { background: lime; }", if you focus "foo" you see a green background on the DIV, but if you focus the INPUT, you only see the green background there.

I also tried something like this:
  const shadowRoot = host.attachShadow({mode: 'open', delegatesFocus: true});
  shadowRoot.innerHTML = '<style>:focus-visible { background: magenta; }</style><p>foo</p><div contenteditable>contenteditable</div>';

Similar in this case, if you focus the contenteditable DIV, you see a magenta background, but the shadow host doesn't match :focus-visible.

Maybe you were thinking on a different example.

>> Source/WebCore/dom/Element.cpp:783
>> +        ASSERT(focused());
> 
> This is just:
> ASSERT(!flag || focused());
> Or alternatively:
> ASSERT(!(flag && !focused()));

Done.

>> Source/WebCore/dom/Element.cpp:785
>> +        ASSERT(flag);
> 
> Similarly here, we can just do:
> ASSERT(!focused() || !shouldAlwaysHaveFocusVisibleWhenFocused(*this) || flag);

Done.

>> Source/WebCore/page/EventHandler.cpp:3548
>> +            return;
> 
> It's not really good to create this kind of state dependency.
> This code relies on some other code to have called setHasFocusVisible, and it adds more conditions on top of it.
> I think what we're trying to do is to set hasFocusVisible if isn't already set.
> It's far clearer to write code like this instead:
> bool userHasInteractedViaKeyword = keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
> if (element->focused() && userHasInteractedViaKeyword)
>     element->setHasFocusVisible(true);

Yeah, that's indeed more clear, I've done that.

>> Source/WebCore/page/EventHandler.cpp:3552
>> +        element.setHasFocusVisible(keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()));
> 
> This set of key combinations seem rather arbitrary to me.
> Where does it come from?

In the spec there's this text (https://drafts.csswg.org/selectors/#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)."

That's what we're implementing here.

There are also some WPT tests that check that if you enter some modifier key (like "Ctrl + y"), :focus-visible doesn't start matching. But if you type a letter, it actually does.

Do you think other cases should be included/excluded here?

Comment on attachment 427190 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:3552
>>> +        element.setHasFocusVisible(keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()));
>> 
>> This set of key combinations seem rather arbitrary to me.
>> Where does it come from?
> 
> In the spec there's this text (https://drafts.csswg.org/selectors/#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)."
> 
> That's what we're implementing here.
> 
> There are also some WPT tests that check that if you enter some modifier key (like "Ctrl + y"), :focus-visible doesn't start matching. But if you type a letter, it actually does.
> 
> Do you think other cases should be included/excluded here?

BTW, one thing that I forgot to mention is that Firefox doesn't do this (https://bugzilla.mozilla.org/show_bug.cgi?id=1688539).
In Firefox :focus-visible matching doesn't change if you have focused an element with the mouse initially, and then you type any letter.

Also Chromium does something a little bit different than what we have in WebKit. Because in Chromium just typing "Shift" alone, makes the element start matching :focus-visible (but that doesn't happen for "Ctrl" key). In WebKit the modifier alone doesn't make any change (neither "Ctrl" or "Shift").

(In reply to Manuel Rego Casasnovas from comment #11)
>
> >> Source/WebCore/dom/Element.cpp:751
> >> +    return element.isTextField() || element.isContentEditable();
> > 
> > I guess we can discuss in another issue but why is this special case needed / necessary?
> > It seems wrong that a shadow host with contenteditable will automatically get a visible focus,
> > much less any focusable element that's in an editable region.
> 
> Yeah I guess we could discuss this on a different bug, so I'm not seeing the
> kind of issue you mention.
> 
> For example right now if we have "<div
> contenteditable><p>foo</p><input></div>" and we set ":focus-visible {
> background: lime; }", if you focus "foo" you see a green background on the
> DIV, but if you focus the INPUT, you only see the green background there.

That's not what I'm talking about. isContentEditable() will return true on every node that's inside an element with contenteditable=true. I don't think what's quite what we want. isRootEditableElement() is probably what you're looking for instead.

> >> Source/WebCore/page/EventHandler.cpp:3552
> >> +        element.setHasFocusVisible(keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()));
> > 
> > This set of key combinations seem rather arbitrary to me.
> > Where does it come from?
> 
> In the spec there's this text
> (https://drafts.csswg.org/selectors/#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)."
> 
> That's what we're implementing here.

I don't see how or why this behavior is desirable? This is totally different from the way WebKit keeps track of whether a given WKWebView has a focus or not. In macOS / iOS, the concept here is whether the current view is the first responder or not. It has nothing to do with whether the user had interacted with keyboard or not. In fact, we need to be showing the focus ring before the user ever types things into text field on iOS / iPad.

> There are also some WPT tests that check that if you enter some modifier key
> (like "Ctrl + y"), :focus-visible doesn't start matching. But if you type a
> letter, it actually does.

But why is that behavior useful / desirable?

Note that there is also a very notable difference between WebKit and other browser engines, which is that when the user tries to type text, we'd move the focus to where the selection is whereas other browsers move the selection to where the focus is.

Comment on attachment 427238 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:750
> +    // Elements that support keyboard input (form inputs and contenteditable) always match :focus-visible when focused.

This comment now just repeats what the code says so please remove.

Created attachment 427245 [details]
Patch

(In reply to Ryosuke Niwa from comment #15)
> Comment on attachment 427238 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427238&action=review
> 
> > Source/WebCore/dom/Element.cpp:750
> > +    // Elements that support keyboard input (form inputs and contenteditable) always match :focus-visible when focused.
> 
> This comment now just repeats what the code says so please remove.

Ok, done.

Let's discuss the open issues on a separated bug.

Committed r276698 (237112@main): <https://commits.webkit.org/237112@main>

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

<rdar://problem/77254863>