Bug 194742 – AX: PSON: Going back from apple.com to search results, cannot interact with HTML content. Disabling Swap Processes on Cross-Site Navigation resolves the issue.

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

[chris fleizach]

1. Open Safari, enable VOiceOver.
2. Command-l with no windows open.
3. In the resulting search field type iphone xs.
4. Press return, then navigate VO to the HTML content.
5. Interact, then VO-Command-h to the first web result, not ad, with apple.com.
6. VO-space to activate the link.
7. Navigate the page to the first heading, then VO-right to the first paragraph.
8. Enough surfing this page, Command-[ (left bracket) to go back.
9. Attempt to navigate the search results page again.

Actual Results: Focus tossed out of HTML content, and no access to the HTML content at this point.

Expected Results: VO should have actually landed on the link pressed in step 6.


<rdar://problem/47677951>

[chris fleizach]

Created attachment 362212 [details]
patch

[chris fleizach]

Created attachment 362213 [details]
patch

[EWS Watchlist]

Comment on attachment 362213 [details]
patch

Attachment 362213 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11171514

New failing tests:
accessibility/mac/input-type-change-crash.html
accessibility/mac/loaded-notification.html

[EWS Watchlist]

Created attachment 362215 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362213 [details]
patch

Attachment 362213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11171543

New failing tests:
accessibility/mac/input-type-change-crash.html
accessibility/mac/loaded-notification.html

[EWS Watchlist]

Created attachment 362216 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362213 [details]
patch

Attachment 362213 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11171560

New failing tests:
accessibility/mac/input-type-change-crash.html
accessibility/mac/loaded-notification.html

[EWS Watchlist]

Created attachment 362217 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Geoffrey Garen]

Comment on attachment 362213 [details]
patch

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

Thanks for the patch, Chris. I want to review and help get this fixed, but I'm new to this code, and I have some questions.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:522
> +    // send a load complete before VoiceOver has time to register for the notification.

send => sends

Do we need this change on iOS too? What about other accessibility platforms?

From this comment, it sounds like we still have a race condition, only less so. Why did moving code to the UI process reduce the race condition? Is there a way to eliminate it completely?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:717
> +    platformReinitialize();

Why don't we need to send the RegisterWebProcessAccessibilityToken message when we first create a page (in WebProcess::createWebPage())?

[Geoffrey Garen]

These test failures look real:

  accessibility/mac/input-type-change-crash.html [ Timeout ]
  accessibility/mac/loaded-notification.html [ Timeout ]

[chris fleizach]

Comment on attachment 362213 [details]
patch

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

>> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:522
>> +    // send a load complete before VoiceOver has time to register for the notification.
> 
> send => sends
> 
> Do we need this change on iOS too? What about other accessibility platforms?
> 
> From this comment, it sounds like we still have a race condition, only less so. Why did moving code to the UI process reduce the race condition? Is there a way to eliminate it completely?

without hearing feedback about issues on iOS, I'd rather just leave as is since we probably won't be able to get enough testing coverage in time.

I don't think the other platforms handle this notification. They also likely have different notification requirements. This specific problem is local to macOS only as far as I can tell (due to the AX architecture, I also don't think we have this problem on iOS).

I actually think this removes the race condition. VO is going to register for notifications on UIProcess right away. Then regardless of when these WebProcess's come and go it can still receive notifications.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:717
>> +    platformReinitialize();
> 
> Why don't we need to send the RegisterWebProcessAccessibilityToken message when we first create a page (in WebProcess::createWebPage())?

I believe it gets sent in platformInitialize() which looks like it happens when WebPageProxy is created

[Geoffrey Garen]

> > Do we need this change on iOS too? What about other accessibility platforms?
> > 
> > From this comment, it sounds like we still have a race condition, only less so. Why did moving code to the UI process reduce the race condition? Is there a way to eliminate it completely?
> 

> without hearing feedback about issues on iOS, I'd rather just leave as is
> since we probably won't be able to get enough testing coverage in time.

Sounds good. Probably best to remove the change to WebPageIOS then.

> I actually think this removes the race condition. VO is going to register
> for notifications on UIProcess right away. Then regardless of when these
> WebProcess's come and go it can still receive notifications.

Sounds good. Can you update the comment to explain moving the code to the UI process, where VO registers for notifications, synchronizes the code with VO?

> > Why don't we need to send the RegisterWebProcessAccessibilityToken message when we first create a page (in WebProcess::createWebPage())?
> 
> I believe it gets sent in platformInitialize() which looks like it happens
> when WebPageProxy is created

Can platformInitialize() and reinitialize() share a function to do this registration?

[chris fleizach]

(In reply to Geoffrey Garen from comment #12)
> > > Do we need this change on iOS too? What about other accessibility platforms?
> > > 
> > > From this comment, it sounds like we still have a race condition, only less so. Why did moving code to the UI process reduce the race condition? Is there a way to eliminate it completely?
> > 
> 
> > without hearing feedback about issues on iOS, I'd rather just leave as is
> > since we probably won't be able to get enough testing coverage in time.
> 
> Sounds good. Probably best to remove the change to WebPageIOS then.

We still need that specific iOS change.

We have 2 things going on in this patch

1) Handling the platformRe-initialization due to the new process model
2) Improve reliability of AXLoaded notifications which got noticeably worse with the new process model. 

We only need 2) on the Mac (for now). But we'll need 1) on both platforms (because they both use new the process model)

> 
> > I actually think this removes the race condition. VO is going to register
> > for notifications on UIProcess right away. Then regardless of when these
> > WebProcess's come and go it can still receive notifications.
> 
> Sounds good. Can you update the comment to explain moving the code to the UI
> process, where VO registers for notifications, synchronizes the code with VO?
> 
> > > Why don't we need to send the RegisterWebProcessAccessibilityToken message when we first create a page (in WebProcess::createWebPage())?
> > 
> > I believe it gets sent in platformInitialize() which looks like it happens
> > when WebPageProxy is created
> 
> Can platformInitialize() and reinitialize() share a function to do this
> registration?

[chris fleizach]

Created attachment 362246 [details]
patch

[chris fleizach]

Created attachment 362247 [details]
patch

[Chris Dumez]

Comment on attachment 362247 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 362247 [details]
patch

Clearing flags on attachment: 362247

Committed r241721: <https://trac.webkit.org/changeset/241721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

It appears to me that the new calls to NSAccessibilityPostNotification cause accessibility to be enabled in all Web Content processes for seemingly no reason. Is that right/is that a problem?

[chris fleizach]

(In reply to Tim Horton from comment #19)
> It appears to me that the new calls to NSAccessibilityPostNotification cause
> accessibility to be enabled in all Web Content processes for seemingly no
> reason. Is that right/is that a problem?

I think by design accessibility is generally always on in AppKit apps. WebKit goes above and beyond by tracking this and doing different work.

So with that said, it probably makes sense to check if accessibility is enabled before calling these post notifications