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.
Summary: AX: PSON: Going back from apple.com to search results, cannot interact with H...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-16 01:17 PST by chris fleizach
Modified: 2019-07-10 23:40 PDT (History)
14 users (show)

See Also:


Attachments
patch (10.45 KB, patch)
2019-02-16 01:25 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (10.44 KB, patch)
2019-02-16 01:27 PST, chris fleizach
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.45 MB, application/zip)
2019-02-16 02:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.73 MB, application/zip)
2019-02-16 02:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.06 MB, application/zip)
2019-02-16 03:15 PST, Build Bot
no flags Details
patch (23.90 KB, patch)
2019-02-17 13:19 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (13.42 KB, patch)
2019-02-17 13:23 PST, chris fleizach
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 2019-02-16 01:17:31 PST
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>
Comment 1 chris fleizach 2019-02-16 01:25:25 PST
Created attachment 362212 [details]
patch
Comment 2 chris fleizach 2019-02-16 01:27:14 PST
Created attachment 362213 [details]
patch
Comment 3 Build Bot 2019-02-16 02:32:11 PST
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
Comment 4 Build Bot 2019-02-16 02:32:13 PST
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
Comment 5 Build Bot 2019-02-16 02:47:43 PST
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
Comment 6 Build Bot 2019-02-16 02:47:45 PST
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
Comment 7 Build Bot 2019-02-16 03:15:06 PST
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
Comment 8 Build Bot 2019-02-16 03:15:07 PST
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
Comment 9 Geoffrey Garen 2019-02-16 09:08:44 PST
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())?
Comment 10 Geoffrey Garen 2019-02-16 09:09:21 PST
These test failures look real:

  accessibility/mac/input-type-change-crash.html [ Timeout ]
  accessibility/mac/loaded-notification.html [ Timeout ]
Comment 11 chris fleizach 2019-02-16 09:41:12 PST
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
Comment 12 Geoffrey Garen 2019-02-16 13:11:27 PST
> > 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?
Comment 13 chris fleizach 2019-02-17 12:35:59 PST
(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?
Comment 14 chris fleizach 2019-02-17 13:19:50 PST
Created attachment 362246 [details]
patch
Comment 15 chris fleizach 2019-02-17 13:23:00 PST
Created attachment 362247 [details]
patch
Comment 16 Chris Dumez 2019-02-18 07:51:13 PST
Comment on attachment 362247 [details]
patch

r=me
Comment 17 WebKit Commit Bot 2019-02-18 09:17:51 PST
Comment on attachment 362247 [details]
patch

Clearing flags on attachment: 362247

Committed r241721: <https://trac.webkit.org/changeset/241721>
Comment 18 WebKit Commit Bot 2019-02-18 09:17:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Tim Horton 2019-07-10 23:35:30 PDT
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?
Comment 20 chris fleizach 2019-07-10 23:40:35 PDT
(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