Summary: | AX: AccessibilityObject::setFocused(true) should make the webpage focused, and make web content the first responder | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||||
Component: | Accessibility | Assignee: | Tyler Wilcock <tyler_w> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, darin, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=249976 | ||||||||||
Attachments: |
|
Description
Tyler Wilcock
2022-01-05 09:26:40 PST
Created attachment 448409 [details]
Patch
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. (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. Created attachment 448512 [details]
Patch
(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. Created attachment 448630 [details]
Patch
> 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. Comment on attachment 448630 [details]
Patch
Looks good.
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]. |