Summary: | AX: Keyboard focus not following VoiceOver cursor into web content or within web content. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, commit-queue, dmazzoni, ews-watchlist, jcraig, jdiggs, jlewis3, mitz, n_wang, rniwa, samuel_white, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=183232 | ||||||||||||
Attachments: |
|
Description
chris fleizach
2018-02-13 16:24:06 PST
Created attachment 333742 [details]
patch
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 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). 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).
(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 (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. Created attachment 334201 [details]
patch
Comment on attachment 334201 [details]
patch
Can we add a test?
Created attachment 334327 [details]
patch
Added test
Created attachment 334328 [details]
patch
updated ChangeLog
Comment on attachment 334328 [details]
patch
r=me
Comment on attachment 334328 [details] patch Clearing flags on attachment: 334328 Committed r228857: <https://trac.webkit.org/changeset/228857> All reviewed patches have been landed. Closing bug. Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit (In reply to Tim Horton from comment #17) > Build fix for 32-bit: https://trac.webkit.org/changeset/228859/webkit Thanks for the fix! 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 Bug filed for fix: https://bugs.webkit.org/show_bug.cgi?id=183232 |