Bug 234077 – [selectors] :focus-visible not matching on accessKey focus after focusing something via mouse

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

[Manuel Rego Casasnovas]

We've a problem in :focus-visible implementation, when we call Element::focus() if we had previously focused something via mouse click, we might not be setting "FocusVisibility::Visible" for things that are expected to have it.

Example:
  <div tabindex="0">foo</div>
  <div tabindex="0" accessKey="B">bar</div>

If you first focus the "foo" element via mouse click, and then use "Alt+B" to focus the "bar" element, the "bar" element doesn't match :focus-visible, when it should.
The problem is that by default when you use "Alt+B", document->wasLastFocusByClick() is FALSE, so we set FocusVisibility::Visible manually in Element::focus(). But when we do that after clicking a focusable element, document->wasLastFocusByClick() is TRUE so we don't set FocusVisibility::Visible.

We should always match :focus-visible in the "bar" element in when accessing it with the accessKey.


JFYI, we were experiencing a similar issue before the patch on bug #233924 for SELECT element.

[Manuel Rego Casasnovas]

The problem here is related to this code in Element::focus():
        FocusOptions optionsWithVisibility = options;
        if (!document->wasLastFocusByClick())
            optionsWithVisibility.visibility = FocusVisibility::Visible;

That code is needed to make focus-visible-script-focus-* tests to pass.
Basically when you have clicked on a focusable element (maybe a <div tabindex> or a <button> on Linux) if the event handler of that click, focuses something else (e.g. something in a dialog that it opens), you shouldn't not get a focus-ring in the newly focused element.

The problem is that this Element::focus() is used from JavaScript focus(), but is also used in many other places internally.
For all those internal usages, we're getting this behavior that might be not correct (depending on if the previous focused element was focused by click or not).
One of those places is accessKeyAction() methods, for them we always want to show focus-visible, so we don't care about if the last focused element was by click or not.

I'm not sure what's the best way we can differentiate if Element::focus() has been called from JavaScript, or it has been called internally.
One idea could be to have a Element::internalFocus() that we only use internally, and keep Element::focus() only for script focus.
Another could be adding a new FocusTrigger::Internal, and pass it properly in all the internal callers to Element::focus().
Not really sure what's the best approach, or if there are better alternatives.

[Radar WebKit Bug Importer]

<rdar://problem/86572561>

[Manuel Rego Casasnovas]

Created attachment 448040 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 448205 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 448207 [details]
Patch

[Antti Koivisto]

Comment on attachment 448207 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1947
> +        downcast<Element>(*node).focus({ { }, { }, { }, FocusTrigger::Internal, { } });

This is pretty ugly and done in many places. Maybe Internal should be the default value as I think it is the common case.

> Source/WebCore/dom/FocusOptions.h:35
> -enum class FocusTrigger : bool { Other, Click };
> +enum class FocusTrigger : uint8_t { Other, Click, Internal };

I don't really understand what Other now means. Internal seems to be the JS API, autofocus and similar. It also seems to be used for keyboard focus. Can Other be named to indicate what it specifically does?

Also Internal could use a more specific name.

[Antti Koivisto]

> Can Other be named to indicate what it specifically does?

I mean indicate what cases it is used in.

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to Antti Koivisto from comment #6)
> Comment on attachment 448207 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448207&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1947
> > +        downcast<Element>(*node).focus({ { }, { }, { }, FocusTrigger::Internal, { } });
> 
> This is pretty ugly and done in many places. Maybe Internal should be the
> default value as I think it is the common case.

Yeah I agree. I don't like the solution. That's why I was asking for ideas in #c1. Not sure if the other idea there would look better.

They key is that we need to differentiate from when a web page does "element.focus()" in JavaScript (and thus call Element::focus()) vs when we call Element::focus() manually in some WebKit code.
I don't know if it's possible to distinguish if Element::focus() has been called from JavaScript or internally. If we set the default to "Internal", then we won't be able to set it to "Other" (maybe we should call this "Script") when it's called from JavaScript.
Do you have any idea about the best way to differentiate both cases?

> > Source/WebCore/dom/FocusOptions.h:35
> > -enum class FocusTrigger : bool { Other, Click };
> > +enum class FocusTrigger : uint8_t { Other, Click, Internal };
> 
> I don't really understand what Other now means. Internal seems to be the JS
> API, autofocus and similar. It also seems to be used for keyboard focus. Can
> Other be named to indicate what it specifically does?
> 
> Also Internal could use a more specific name.

Yeah the naming is not great, let's see if we agreed on a general approach to fix this before thinking on good names for this.

[Antti Koivisto]

Maybe have focusForBindings() (with [ImplementedAs=focusForBindings] in IDL) for the DOM API version and keep focus() as the internal call?

The technical approach seems fine in itself but I think it could be less confusing.

[Manuel Rego Casasnovas]

Created attachment 448219 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Antti Koivisto from comment #9)
> Maybe have focusForBindings() (with [ImplementedAs=focusForBindings] in IDL)
> for the DOM API version and keep focus() as the internal call?

Thank you very much, that's what I was looking for. This makes the patch way simpler and easier to read and understand.

I've uploaded a new version of the patch using that.

[Antti Koivisto]

Comment on attachment 448219 [details]
Patch

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

> Source/WebCore/dom/Element.h:429
> +    virtual void focusForBindings(const FocusOptions& = { });

Probably doesn't need default value.

> Source/WebCore/dom/FocusOptions.h:35
> +enum class FocusTrigger : uint8_t { Other, Click, JavaScriptBindings };

Just Bindings is enough.

[Antti Koivisto]

Comment on attachment 448219 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:3102
> +        if ((options.trigger == FocusTrigger::JavaScriptBindings) && document->wasLastFocusByClick())

No need for () around the test.

[Antti Koivisto]

Comment on attachment 448219 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:3122
> +void Element::focusForBindings(const FocusOptions& options)
> +{
> +    FocusOptions optionsWithTrigger = options;
> +    optionsWithTrigger.trigger = FocusTrigger::JavaScriptBindings;
> +    focus(optionsWithTrigger);
> +}

Maybe the function can take FocustOptions&& and you don't need to make a copy?

[Manuel Rego Casasnovas]

Created attachment 448275 [details]
Patch for landing

[Manuel Rego Casasnovas]

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

[Antti Koivisto]

Comment on attachment 448275 [details]
Patch for landing

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

> LayoutTests/ChangeLog:12
> +        Writing an internal test as WPT tests for this don't work due to webkit.org/b/234139.
> +
> +        * fast/selectors/focus-visible-accesskey-expected.txt: Added.
> +        * fast/selectors/focus-visible-accesskey.html: Added.

This patch is missing the test case.

[Manuel Rego Casasnovas]

Comment on attachment 448275 [details]
Patch for landing

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

>> LayoutTests/ChangeLog:12
>> +        * fast/selectors/focus-visible-accesskey.html: Added.
> 
> This patch is missing the test case.

Oops, yeah it was not included (my mistake sorry).

[Manuel Rego Casasnovas]

Created attachment 448283 [details]
Patch for landing

[EWS]

Committed r287563 (245698@main): <https://commits.webkit.org/245698@main>

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

[Ryan Haddad]

The test added with this change is failing on iOS: https://results.webkit.org/?suite=layout-tests&test=fast%2Fselectors%2Ffocus-visible-accesskey.html

Disabled it in r287662.

[Brent Fulgham]

This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.