Bug 234885 – AX: AccessibilityObject::setFocused(true) should make the webpage focused, and make web content the first responder

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

WebKit should set webpage focus and first respondership when an element is focused via accessibility. Because we don't currently do so, focus rings are not drawn around focused elements when these conditions are true:

1. A WKWebView is embedded in another app
2. An AX client drives focus into the webview (e.g. full keyboard access on iOS)
3. The hosting app does not make web content the first responder

[Radar WebKit Bug Importer]

<rdar://problem/87149186>

[Tyler Wilcock]

Created attachment 448409 [details]
Patch

[Darin Adler]

Comment on attachment 448409 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2797
> +#if PLATFORM(IOS_FAMILY)
> +        // Mark the page as focused so the focus ring can be drawn immediately. The page is also marked
> +        // as focused as part assistiveTechnologyMakeFirstResponder, but that requires some back-and-forth
> +        // IPC between the web and UI processes, during which we can miss the drawing of the focus ring for the
> +        // first focused element. Making the page focused is a requirement for making the page selection focused.
> +        // This is iOS only until there's a demonstrated need for this preemptive focus on other platforms.
> +        if (!page->focusController().isFocused())
> +            page->focusController().setFocused(true);
> +#endif

Once we call ChromeClient::focus, a lot of side effects may have happened; almost anything could happen including arbitrary code running on webpages and doing a lot. Not sure it’s OK to do more work after that using a page pointer which might now be a deallocated object. We may need to reorder things, or re-fetch document, frame, and page.

[Tyler Wilcock]

(In reply to Darin Adler from comment #3)
> Comment on attachment 448409 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448409&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2797
> > +#if PLATFORM(IOS_FAMILY)
> > +        // Mark the page as focused so the focus ring can be drawn immediately. The page is also marked
> > +        // as focused as part assistiveTechnologyMakeFirstResponder, but that requires some back-and-forth
> > +        // IPC between the web and UI processes, during which we can miss the drawing of the focus ring for the
> > +        // first focused element. Making the page focused is a requirement for making the page selection focused.
> > +        // This is iOS only until there's a demonstrated need for this preemptive focus on other platforms.
> > +        if (!page->focusController().isFocused())
> > +            page->focusController().setFocused(true);
> > +#endif
> 
> Once we call ChromeClient::focus, a lot of side effects may have happened;
> almost anything could happen including arbitrary code running on webpages
> and doing a lot. Not sure it’s OK to do more work after that using a page
> pointer which might now be a deallocated object. We may need to reorder
> things, or re-fetch document, frame, and page.
Good point. I think re-ordering it such that this new block is before ChromeClient::focus will be fine.

[Tyler Wilcock]

Created attachment 448512 [details]
Patch

[Darin Adler]

(In reply to Tyler Wilcock from comment #4)
> Good point. I think re-ordering it such that this new block is before
> ChromeClient::focus will be fine.

Sadly I do not think that’s sufficient. FocusController::setFocused has side effects too, so the same problem happens in reverse.

The trick here is to:

1) First, decide what the policy is if a side effect navigates away and does things in the middle of this focusing process, like what it it focuses something else? Unclear how to get that exactly right.

2) Second, write the code so it doesn’t use pointers that could be stale across these; re-getting document, frame, page, etc.

[Tyler Wilcock]

Created attachment 448630 [details]
Patch

[Tyler Wilcock]

> 1) First, decide what the policy is if a side effect navigates away and does things in the middle of this focusing process, like what it it focuses something else? Unclear how to get that exactly right.
If ChromeClient::focus() or FocusController::setFocused(true) were to create a side-effect that focused something else, it would be overridden by the last block in this function, assistiveTechnologyMakeFirstResponder, which focuses the page and makes web content the first responder.

I think this is reasonable behavior / policy. If we’ve gotten to this function call, it means the embedder has allowed focus to enter into web content, and an AX client is trying to focus an element, and thus we should make the page focused so focus rings can be drawn. If a webview embedder doesn’t want to allow focus into web content (maybe it’s intended to be visual but not interactive), they have knobs on the webview delegate they can tweak (i.e. _webViewCanBecomeFocused).

> 2) Second, write the code so it doesn’t use pointers that could be stale across these; re-getting document, frame, page, etc.


I think this should be better now if you wouldn't mind taking another look.

[Darin Adler]

Comment on attachment 448630 [details]
Patch

Looks good.

[EWS]

Committed r287810 (245862@main): <https://commits.webkit.org/245862@main>

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