Bug 182752 - AX: Keyboard focus not following VoiceOver cursor into web content or within web content.
Summary: AX: Keyboard focus not following VoiceOver cursor into web content or within ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-13 16:24 PST by chris fleizach
Modified: 2018-02-28 17:18 PST (History)
15 users (show)

See Also:


Attachments
patch (1.28 KB, patch)
2018-02-13 16:26 PST, chris fleizach
thorton: review-
Details | Formatted Diff | Diff
patch (10.36 KB, patch)
2018-02-19 16:43 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (16.91 KB, patch)
2018-02-20 17:50 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch (20.78 KB, patch)
2018-02-20 17:53 PST, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2018-02-13 16:24:06 PST
VoiceOver and keyboard focus are no longer tracking within web views if the keyboard focus was outside the web view before interacting with the web view using VoiceOver.

1. Type and address into the address bar and press return.
2. Stop interacting with the address bar in the toolbar, interact with the page.
3. Move to a link or form element that should get VoiceOver keyboard focus when VoiceOver focuses that element.
4. Use VO-F3 and VO-F4 (FN-VO-3 and 4 on DFR) to get spoken output for the VO cursor and keyboard cursor focus.

Actual Results: They are not in sync, 

<rdar://problem/37327881>
Comment 1 Radar WebKit Bug Importer 2018-02-13 16:24:23 PST
<rdar://problem/37518233>
Comment 2 chris fleizach 2018-02-13 16:26:56 PST
Created attachment 333742 [details]
patch
Comment 3 Ryosuke Niwa 2018-02-16 14:47:47 PST
Comment on attachment 333742 [details]
patch

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

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:353
> +    m_pageClient.setShouldSuppressFirstResponderChanges(false);

This doesn't seem right. The whole point of this flag is to avoid becoming the first responder programmatically.
See <rdar://problem/31658967> and <rdar://problem/34669220>.
Comment 4 chris fleizach 2018-02-16 14:53:14 PST
Comment on attachment 333742 [details]
patch

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

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:354
>      m_pageClient.makeFirstResponder();

this is exactly what we need. If someone from WebContent is specifically requesting to become firstResponder it doesn't make sense that that call would never work
Comment 5 mitz 2018-02-16 14:58:25 PST
Comment on attachment 333742 [details]
patch

So, does this patch reintroduce <rdar://problem/30085686> or not?
Comment 6 mitz 2018-02-16 15:00:14 PST
(In reply to mitz from comment #5)
> Comment on attachment 333742 [details]
> patch
> 
> So, does this patch reintroduce <rdar://problem/30085686> or not?

(Apologies to people who aren’t from Apple who’re following this bug. <rdar://problem/30085686> is an Apple bug that was resolved by adopting -setShouldSuppressFirstResponderChanges: in an app).
Comment 7 Tim Horton 2018-02-16 15:15:31 PST
Comment on attachment 333742 [details]
patch

r- because this patch will certainly re-introduce that bug. Perhaps you can introduce a different mechanism for accessibility to cause first-responder changes instead of re-using the same one that web content uses (which needs this restriction to avoid the aforementioned bug).
Comment 8 chris fleizach 2018-02-16 15:25:52 PST
(In reply to Tim Horton from comment #7)
> Comment on attachment 333742 [details]
> patch
> 
> r- because this patch will certainly re-introduce that bug. Perhaps you can
> introduce a different mechanism for accessibility to cause first-responder
> changes instead of re-using the same one that web content uses (which needs
> this restriction to avoid the aforementioned bug).

What if I make a KW2 call to avoid suppressing that can only be triggered from VoiceOver calling into it
Comment 9 Tim Horton 2018-02-16 15:36:12 PST
(In reply to chris fleizach from comment #8)
> (In reply to Tim Horton from comment #7)
> > Comment on attachment 333742 [details]
> > patch
> > 
> > r- because this patch will certainly re-introduce that bug. Perhaps you can
> > introduce a different mechanism for accessibility to cause first-responder
> > changes instead of re-using the same one that web content uses (which needs
> > this restriction to avoid the aforementioned bug).
> 
> What if I make a KW2 call to avoid suppressing that can only be triggered
> from VoiceOver calling into it

Also a reasonable approach.
Comment 10 chris fleizach 2018-02-19 16:43:45 PST
Created attachment 334201 [details]
patch
Comment 11 Ryosuke Niwa 2018-02-20 11:07:40 PST
Comment on attachment 334201 [details]
patch

Can we add a test?
Comment 12 Nan Wang 2018-02-20 17:50:45 PST
Created attachment 334327 [details]
patch

Added test
Comment 13 Nan Wang 2018-02-20 17:53:17 PST
Created attachment 334328 [details]
patch

updated ChangeLog
Comment 14 Ryosuke Niwa 2018-02-20 19:30:22 PST
Comment on attachment 334328 [details]
patch

r=me
Comment 15 WebKit Commit Bot 2018-02-20 20:17:25 PST
Comment on attachment 334328 [details]
patch

Clearing flags on attachment: 334328

Committed r228857: <https://trac.webkit.org/changeset/228857>
Comment 16 WebKit Commit Bot 2018-02-20 20:17:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Tim Horton 2018-02-20 21:49:18 PST
Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit
Comment 18 Nan Wang 2018-02-20 22:35:12 PST
(In reply to Tim Horton from comment #17)
> Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit

Thanks for the fix!
Comment 19 Matt Lewis 2018-02-28 16:41:58 PST
The test accessibility/mac/accessibility-make-first-responder.html that was added with the initial commit is very flaky:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Faccessibility-make-first-responder.html

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r229106%20(3180)/results.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/accessibility/mac/accessibility-make-first-responder-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/accessibility/mac/accessibility-make-first-responder-actual.txt
@@ -6,7 +6,7 @@
 DOM focus
 Window is still the first responder: true
 Accessibility focus
-Window is still the first responder: false
+Window is still the first responder: true
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 20 Matt Lewis 2018-02-28 17:18:27 PST
Bug filed for fix: https://bugs.webkit.org/show_bug.cgi?id=183232