Bug 182752 – AX: Keyboard focus not following VoiceOver cursor into web content or within web content.

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

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>

[Radar WebKit Bug Importer]

<rdar://problem/37518233>

[chris fleizach]

Created attachment 333742 [details]
patch

[Ryosuke Niwa]

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

[chris fleizach]

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 on attachment 333742 [details]
patch

So, does this patch reintroduce <rdar://problem/30085686> or not?

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

[Tim Horton]

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

[chris fleizach]

(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

[Tim Horton]

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

[chris fleizach]

Created attachment 334201 [details]
patch

[Ryosuke Niwa]

Comment on attachment 334201 [details]
patch

Can we add a test?

[Nan Wang]

Created attachment 334327 [details]
patch

Added test

[Nan Wang]

Created attachment 334328 [details]
patch

updated ChangeLog

[Ryosuke Niwa]

Comment on attachment 334328 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 334328 [details]
patch

Clearing flags on attachment: 334328

Committed r228857: <https://trac.webkit.org/changeset/228857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit

[Nan Wang]

(In reply to Tim Horton from comment #17)
> Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit

Thanks for the fix!

[Matt Lewis]

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

[Matt Lewis]

Bug filed for fix: https://bugs.webkit.org/show_bug.cgi?id=183232