Bug 234077 - [selectors] :focus-visible not matching on accessKey focus after focusing something via mouse
Summary: [selectors] :focus-visible not matching on accessKey focus after focusing som...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: InRadar
Depends on: 72359
Blocks: 185859
  Show dependency treegraph
 
Reported: 2021-12-09 05:44 PST by Manuel Rego Casasnovas
Modified: 2022-02-04 14:54 PST (History)
23 users (show)

See Also:


Attachments
Patch (22.51 KB, patch)
2021-12-28 01:16 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.99 KB, patch)
2022-01-02 22:15 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.99 KB, patch)
2022-01-02 22:44 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2022-01-03 03:03 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (5.91 KB, patch)
2022-01-04 03:07 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (8.55 KB, patch)
2022-01-04 04:37 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2021-12-09 05:44:31 PST
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.
Comment 1 Manuel Rego Casasnovas 2021-12-13 04:23:18 PST
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.
Comment 2 Radar WebKit Bug Importer 2021-12-16 05:45:17 PST
<rdar://problem/86572561>
Comment 3 Manuel Rego Casasnovas 2021-12-28 01:16:15 PST
Created attachment 448040 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2022-01-02 22:15:41 PST
Created attachment 448205 [details]
Patch
Comment 5 Manuel Rego Casasnovas 2022-01-02 22:44:40 PST
Created attachment 448207 [details]
Patch
Comment 6 Antti Koivisto 2022-01-03 00:54:21 PST
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.
Comment 7 Antti Koivisto 2022-01-03 00:55:37 PST
> Can Other be named to indicate what it specifically does?

I mean indicate what cases it is used in.
Comment 8 Manuel Rego Casasnovas 2022-01-03 01:17:33 PST
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.
Comment 9 Antti Koivisto 2022-01-03 01:28:33 PST
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.
Comment 10 Manuel Rego Casasnovas 2022-01-03 03:03:39 PST
Created attachment 448219 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2022-01-03 03:04:37 PST
(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.
Comment 12 Antti Koivisto 2022-01-03 04:16:07 PST
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.
Comment 13 Antti Koivisto 2022-01-03 04:16:58 PST
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.
Comment 14 Antti Koivisto 2022-01-03 04:26:37 PST
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?
Comment 15 Manuel Rego Casasnovas 2022-01-04 03:07:14 PST
Created attachment 448275 [details]
Patch for landing
Comment 16 Manuel Rego Casasnovas 2022-01-04 03:07:51 PST
Thanks for the review, uploaded new version applying the suggested changes.
Comment 17 Antti Koivisto 2022-01-04 04:09:57 PST
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.
Comment 18 Manuel Rego Casasnovas 2022-01-04 04:36:56 PST
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).
Comment 19 Manuel Rego Casasnovas 2022-01-04 04:37:30 PST
Created attachment 448283 [details]
Patch for landing
Comment 20 EWS 2022-01-04 05:30:19 PST
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].
Comment 21 Ryan Haddad 2022-01-05 17:21:30 PST
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.
Comment 22 Brent Fulgham 2022-02-04 14:54:02 PST
This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.