Bug 234077

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.

<rdar://problem/86572561>

Created attachment 448040 [details] Patch

Created attachment 448205 [details] Patch

Created attachment 448207 [details] Patch

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.

> Can Other be named to indicate what it specifically does? I mean indicate what cases it is used in.

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.

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.

Created attachment 448219 [details] Patch

(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 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 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 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?

Created attachment 448275 [details] Patch for landing

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

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 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).

Created attachment 448283 [details] Patch for landing

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].

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.

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