Bug 203798 – AX: WKWebView does not shift Accessibility Focus for Catalyst

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

[Eric Liang]

WKWebViews do not respond to JavaScript that tell them to `focus()`, changing the Accessibility Element selected.

Steps To Reproduce:
1. Enable VoiceOver
2. Open a webpage that auto-shifts Accessibility focus via JavaScript.
3. Verify that no shift occurs.

(See Notes section for a working sample and instructions)

Results:
The WKWebView should adjust its Accessibility focus, but does not.

Regression:
This feature works great on iPad, just not in Catalyst.

Notes:
1. Download simpleCommands.zip containing HTML, CSS, and JS files.
2. Enable VoiceOver
3. Open simpleCommands.html inside Safari. After 5 seconds, the focus should switch and VoiceOver should begin reading out ‘today’s date’.

4. Now open the project found in WebpageCutscene.zip. It uses the same webpage as before.
5. Build and Run this project targeting Mac Catalyst. You will see that the VoiceOver focus does not shift when ran inside an app.

[Radar WebKit Bug Importer]

<rdar://problem/56851887>

[Eric Liang]

Created attachment 382712 [details]
Patch

[chris fleizach]

Comment on attachment 382712 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentView.mm:578
> +    [self _updateRemoteAccessibilityRegistration:NO];

there are a number of places to cleanup. check the other places where we do accessibility cleanup

[chris fleizach]

You should also get a WebKit2 contributors point of view, like Daniel Bates or Brent Fulgham

[Eric Liang]

(In reply to chris fleizach from comment #3)
> Comment on attachment 382712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382712&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:578
> > +    [self _updateRemoteAccessibilityRegistration:NO];
> 
> there are a number of places to cleanup. check the other places where we do
> accessibility cleanup

Even though we do more cleanups in other places, if the process identifier didn't change, do we need to unregister the pid for ax?

[Eric Liang]

(In reply to chris fleizach from comment #3)
> Comment on attachment 382712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382712&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:578
> > +    [self _updateRemoteAccessibilityRegistration:NO];
> 
> there are a number of places to cleanup. check the other places where we do
> accessibility cleanup

the Mac version cleans up when 1) page closed, 2) process swaps or exits

[chris fleizach]

Comment on attachment 382712 [details]
Patch

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

>>>> Source/WebKit/UIProcess/ios/WKContentView.mm:578
>>>> +    [self _updateRemoteAccessibilityRegistration:NO];
>>> 
>>> there are a number of places to cleanup. check the other places where we do accessibility cleanup
>> 
>> Even though we do more cleanups in other places, if the process identifier didn't change, do we need to unregister the pid for ax?
> 
> the Mac version cleans up when 1) page closed, 2) process swaps or exits

ok I think that's right

[Tim Horton]

Comment on attachment 382712 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentView.mm:73
> +SOFT_LINK_FRAMEWORK(AppKit)
> +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement)

Why are you softlinking AppKit? WebKit links AppKit directly.

> Source/WebKit/UIProcess/ios/WKContentView.mm:79
> +@interface NSObject(NSAccessibilityRemoteUIElement_Private)
> +- (void)registerRemoteUIProcessIdentifier:(pid_t)pid;
> +- (void)unregisterRemoteUIProcessIdentifier:(pid_t)pid;
> +@end
> +#endif

This should be in a SPI header, not here.

[Eric Liang]

Comment on attachment 382712 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentView.mm:73
>> +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement)
> 
> Why are you softlinking AppKit? WebKit links AppKit directly.

Correct me if I'm wrong, but this is the iOS code so I don't think we can link or import anything from AppKit because of unavailability build errors.

[Tim Horton]

(In reply to Eric Liang from comment #9)
> Comment on attachment 382712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382712&action=review
> 
> >> Source/WebKit/UIProcess/ios/WKContentView.mm:73
> >> +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement)
> > 
> > Why are you softlinking AppKit? WebKit links AppKit directly.
> 
> Correct me if I'm wrong, but this is the iOS code so I don't think we can
> link or import anything from AppKit because of unavailability build errors.

Oh, macCatalyst might not link AppKit directly, that's a fair point.

In that case, you should have -Softlinking headers like other frameworks.

[Eric Liang]

Created attachment 382757 [details]
Patch

[chris fleizach]

Comment on attachment 382757 [details]
Patch

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

> Source/WebKit/UIProcess/ios/AppKitSoftLink.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019

> Source/WebKit/UIProcess/ios/AppKitSoftLink.mm:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019

[Eric Liang]

Created attachment 382774 [details]
Patch

[Tim Horton]

Comment on attachment 382774 [details]
Patch

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

> Source/WebKit/UIProcess/ios/AppKitSoftLink.h:38
> +@interface NSObject(NSAccessibilityRemoteUIElement_Private)
> +- (void)registerRemoteUIProcessIdentifier:(pid_t)pid;
> +- (void)unregisterRemoteUIProcessIdentifier:(pid_t)pid;
> +@end

These go in AppKitSPI, not AppKitSoftLink. Probably in a macCatalyst-only section.

Also, the category name should have a space before the opening paren.

[Eric Liang]

Created attachment 382786 [details]
Patch

[Tim Horton]

Comment on attachment 382786 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/NSAccessibilitySPI.h:66
> +SOFT_LINK_FRAMEWORK_FOR_HEADER(PAL, AppKit)
> +SOFT_LINK_CLASS_FOR_HEADER(PAL, NSAccessibilityRemoteUIElement)

You moved too much :) These (soft linking bits) should be in -SoftLinking.h/.mm files like they were. Just the forward declarations needed to move here.

Other than that this looks good.

[Eric Liang]

Created attachment 382995 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382995 [details]
Patch

Clearing flags on attachment: 382995

Committed r252175: <https://trac.webkit.org/changeset/252175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 383143 [details]
Patch

[chris fleizach]

Comment on attachment 383143 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        AX: WKWebView does not shift Accessibility Focus for Catalyst

seems like the wrong bug right?

[Tim Horton]

Comment on attachment 383143 [details]
Patch

Seems like a typo