[selectors] Script focus and :focus-visible

Note You need to log in before you can comment on or make changes to this bug.

Right now we're not doing anything special when a script moves focus regarding :focus-visible.

A set of tests for this have just been merged into WPT defining the behavior in different situations:
https://github.com/web-platform-tests/wpt/pull/27806

Chromium and Firefox implementations pass all those tests as they're written right now.

Created attachment 426100 [details]
Patch

Created attachment 426198 [details]
Patch

Created attachment 426209 [details]
Patch

Comment on attachment 426209 [details]
Patch

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

> Source/WebCore/dom/Document.h:764
> +    bool lastFocusByMouseClick() const { return m_lastFocusByMouseClick; }

This name isn’t linguistically sound. The phrase "last focus by mouse click" sounds like it would be a "focus", not a question with a "yes/no" answer. And the use of "mouse" here is strangely old fashioned. I assume this includes taps on touch screens and clicks on trackpads, so being too explicit here about "mouse" seems wrong. Unless it’s helpful to use that term because that’s what’s done in some specification.

I suggest the name wasLastFocusByClick or focusedElementWasFocusedByClick, but maybe you can come up with one even better. Another option would be to store the FocusTrigger value instead of using a boolean. Since you invented this FocusTrigger concept, why not use it?

At some point we might move both the focused element and this new flag into a separate focus object owned by the document, perhaps along with some other focus and keyboard navigation related things; trying to avoid this trend of Document itself becoming a "god class".

> Source/WebCore/dom/Document.h:2130
> +    bool m_lastFocusByMouseClick { false };

Ditto.

> Source/WebCore/dom/Element.cpp:3088
> +            newTarget->setHasFocusVisible(false);

Do we want to do this unconditionally, or only if we called setHasFocusVisible(true) explicitly above?

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

enum class FocusTrigger : bool

Could do the same for FocusRemovalEventsMode above, too. It’s good to specify an underlying type so we don’t use more memory than necessary to store these integers.

> Source/WebCore/dom/FocusOptions.h:41
> +    FocusTrigger trigger { FocusTrigger::Other };

I think FocusOptions may be the C++ representation of an IDL class from the event specification. If so, I’m not sure we should be adding this FocusTrigger concept. We try to keep things more aligned without adding invisible state like that.

> Source/WebCore/page/EventHandler.cpp:2726
> +    if (page && !page->focusController().setFocusedElement(element.get(), m_frame, { { }, { }, { }, FocusTrigger::Click, { } }))

I’m not sure this is the only place this is needed. The pointer events web API may have introduced additional paths that handle "clicks"?

> LayoutTests/platform/mac/TestExpectations:2294
> +# Buttons are not focusable on Mac so this test doesn't work as expected.

We need to take this up with the others who are making the web standards and the web platform tests. We don’t want a standard that prohibits web browsers from following Apple’s platform standards for the Mac user interface, with Safari and WebKit willfully violating that standard.

Created attachment 426405 [details]
Patch

Comment on attachment 426209 [details]
Patch

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

>> Source/WebCore/dom/Document.h:764
>> +    bool lastFocusByMouseClick() const { return m_lastFocusByMouseClick; }
> 
> This name isn’t linguistically sound. The phrase "last focus by mouse click" sounds like it would be a "focus", not a question with a "yes/no" answer. And the use of "mouse" here is strangely old fashioned. I assume this includes taps on touch screens and clicks on trackpads, so being too explicit here about "mouse" seems wrong. Unless it’s helpful to use that term because that’s what’s done in some specification.
> 
> I suggest the name wasLastFocusByClick or focusedElementWasFocusedByClick, but maybe you can come up with one even better. Another option would be to store the FocusTrigger value instead of using a boolean. Since you invented this FocusTrigger concept, why not use it?
> 
> At some point we might move both the focused element and this new flag into a separate focus object owned by the document, perhaps along with some other focus and keyboard navigation related things; trying to avoid this trend of Document itself becoming a "god class".

"Focus trigger" concept comes from the HTML spec: https://html.spec.whatwg.org/multipage/interaction.html
I could use it here indeed, instead of the bool.

>> Source/WebCore/dom/Element.cpp:3088
>> +            newTarget->setHasFocusVisible(false);
> 
> Do we want to do this unconditionally, or only if we called setHasFocusVisible(true) explicitly above?

If the focus has been moved to a different node, we can call it directly here. "newTarget" won't be focused anymore.

>> Source/WebCore/dom/FocusOptions.h:35
>> +enum class FocusTrigger { Other, Click };
> 
> enum class FocusTrigger : bool
> 
> Could do the same for FocusRemovalEventsMode above, too. It’s good to specify an underlying type so we don’t use more memory than necessary to store these integers.

Ok.

>> Source/WebCore/dom/FocusOptions.h:41
>> +    FocusTrigger trigger { FocusTrigger::Other };
> 
> I think FocusOptions may be the C++ representation of an IDL class from the event specification. If so, I’m not sure we should be adding this FocusTrigger concept. We try to keep things more aligned without adding invisible state like that.

The IDL only includes preventScroll: https://html.spec.whatwg.org/#focus-management-apis
The rest of things are additions on top of the IDL.

>> Source/WebCore/page/EventHandler.cpp:2726
>> +    if (page && !page->focusController().setFocusedElement(element.get(), m_frame, { { }, { }, { }, FocusTrigger::Click, { } }))
> 
> I’m not sure this is the only place this is needed. The pointer events web API may have introduced additional paths that handle "clicks"?

I have been looking into the code and couldn't find anything like that.

>> LayoutTests/platform/mac/TestExpectations:2294
>> +# Buttons are not focusable on Mac so this test doesn't work as expected.
> 
> We need to take this up with the others who are making the web standards and the web platform tests. We don’t want a standard that prohibits web browsers from following Apple’s platform standards for the Mac user interface, with Safari and WebKit willfully violating that standard.

All the :focus-visible tests are kind of "optional" in some sense, because the spec is very open and let browsers do different things (https://drafts.csswg.org/selectors/#the-focus-visible-pseudo):
"While the :focus pseudo-class always matches the currently-focused element, UAs only sometimes visibly indicate focus (such as by drawing a “focus ring”), instead using a variety of heuristics to visibly indicate the focus only when it would be most helpful to the user. The :focus-visible pseudo-class matches a focused element in these situations only, allowing authors to change the appearance of the focus indicator without changing when a focus indicator appears."While the :focus pseudo-class always matches the currently-focused element, UAs only sometimes visibly indicate focus (such as by drawing a “focus ring”), instead using a variety of heuristics to visibly indicate the focus only when it would be most helpful to the user. The :focus-visible pseudo-class matches a focused element in these situations only, allowing authors to change the appearance of the focus indicator without changing when a focus indicator appears.

So this leaves it open to each browser and platform decide what to do in each case.

The fact that some tests use buttons and buttons behave differently in Mac (not in the whole WebKit as they're focusable on Linux ports), doesn't mean that Mac port doesn't follow the standard.

Anyway there're some ongoing discussions trying to standarize more all this, but probably they won't be successful:
* https://github.com/web-platform-tests/wpt/pull/6523
* https://github.com/web-platform-tests/wpt/issues/28505

commit-queue failed to commit attachment 426405 [details] to WebKit repository. To retry, please set cq+ flag again.

Created attachment 426434 [details]
Patch

Committed r276264 (236746@main): <https://commits.webkit.org/236746@main>

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

rdar://76853042