Bug 200122 – AX: web process should load correct bundle path for MACCATALYST

• Component: Accessibility
• Status: RESOLVED FIXED

[Eric Liang]

In web process, the accessibility bundle path isn't set for MACCATALYST

[Radar WebKit Bug Importer]

<rdar://problem/53530200>

[Eric Liang]

Created attachment 374882 [details]
Patch

[chris fleizach]

Comment on attachment 374882 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:324
> +    NSString *accessibilityBundlePath = [(NSString *)GSSystemRootDirectory() stringByAppendingString:

this is weird to have a #define in the middle of statement. I think it should be

NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
#if PLATFORM(MACCATALYST)
accessibilityBundlePath = [accessibilityBundlePath stringByAppendingPath:@"System/iOSSupport"]
#endif
accessibilityBundlePath = [accessibilityBundlePath stringByAppendingPath:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]

[Eric Liang]

Created attachment 374894 [details]
Patch

[Eric Liang]

Created attachment 374924 [details]
Patch

[chris fleizach]

Comment on attachment 374924 [details]
Patch

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

> Source/WebKit/ChangeLog:17
> +        AX: web process should load correct bundle path for MACCATALYST

looks like we got two changelogs here

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:484
> +        "com.apple.Accessibility"

are you sure we need this one? what do we need it for?

[Eric Liang]

Need it because in the webprocessloader bundle, we are checking whether application accessibility is enabled, which reads the pref in the iOS realm.

[Eric Liang]

Created attachment 374925 [details]
Patch

[chris fleizach]

(In reply to Eric Liang from comment #7)
> Need it because in the webprocessloader bundle, we are checking whether
> application accessibility is enabled, which reads the pref in the iOS realm.

Yea I can see how we probably need that for catalyst now

Brent, Per is that OK in your opinion? or should we do something else?

[Brent Fulgham]

(In reply to chris fleizach from comment #9)
> (In reply to Eric Liang from comment #7)
> > Need it because in the webprocessloader bundle, we are checking whether
> > application accessibility is enabled, which reads the pref in the iOS realm.
> 
> Yea I can see how we probably need that for catalyst now
> 
> Brent, Per is that OK in your opinion? or should we do something else?

I need to look at this more closely. I think this is wrong, because we invested work to make it possible to dynamically add this connection when it is needed by the WebContent process.

At present, I think this patch is wrong.

Per Arne: is the dynamic AX loading something that works on macOS?

[chris fleizach]

(In reply to Brent Fulgham from comment #10)
> (In reply to chris fleizach from comment #9)
> > (In reply to Eric Liang from comment #7)
> > > Need it because in the webprocessloader bundle, we are checking whether
> > > application accessibility is enabled, which reads the pref in the iOS realm.
> > 
> > Yea I can see how we probably need that for catalyst now
> > 
> > Brent, Per is that OK in your opinion? or should we do something else?
> 
> I need to look at this more closely. I think this is wrong, because we
> invested work to make it possible to dynamically add this connection when it
> is needed by the WebContent process.
> 
> At present, I think this patch is wrong.
> 
> Per Arne: is the dynamic AX loading something that works on macOS?

With Catalyst, we need to probably do the same stuff we're doing on iOS

[Eric Liang]

The iOS web process had this same com.apple.accessibility pref read on its sandbox profile. On Catalyst, the web process is the iOS code but used macOS sandbox profile. This just add it to the macOS profile because it is used by the catalyst web process.

[Brent Fulgham]

(In reply to Eric Liang from comment #12)
> The iOS web process had this same com.apple.accessibility pref read on its
> sandbox profile. On Catalyst, the web process is the iOS code but used macOS
> sandbox profile. This just add it to the macOS profile because it is used by
> the catalyst web process.

Yes -- I was having some kind of episode. I saws your pref read change and thought "xpc service connection".

The preference read is fine.

[Brent Fulgham]

Comment on attachment 374925 [details]
Patch

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

r=me based on Chris's review of the Accessibility bundle change, and I approve the preference read change.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:484
> +        "com.apple.Accessibility"

Yes -- this is fine. It's a preference read, not an XPC service connection as I mistakenly asserted in an early comment.

[WebKit Commit Bot]

Comment on attachment 374925 [details]
Patch

Clearing flags on attachment: 374925

Committed r247864: <https://trac.webkit.org/changeset/247864>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 374925 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:328
> +    NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
> +#if PLATFORM(MACCATALYST)
> +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/iOSSupport"];
> +#endif
> +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"];

Seems really unfortunate to have these paths hard-coded here in WebKit open source. Is there some way to do this better in the future? Anything in the OS that can get us the path to the accessibility bundles directory in a more principled way?

[chris fleizach]

(In reply to Darin Adler from comment #17)
> Comment on attachment 374925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374925&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:328
> > +    NSString *accessibilityBundlePath = (NSString *)GSSystemRootDirectory();
> > +#if PLATFORM(MACCATALYST)
> > +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/iOSSupport"];
> > +#endif
> > +    accessibilityBundlePath = [accessibilityBundlePath stringByAppendingString:@"System/Library/AccessibilityBundles/WebProcessLoader.axbundle"];
> 
> Seems really unfortunate to have these paths hard-coded here in WebKit open
> source. Is there some way to do this better in the future? Anything in the
> OS that can get us the path to the accessibility bundles directory in a more
> principled way?

Yes this is doable. 

@Eric, can you stage a change in AccessibilitySupport.h that returns the path for this bundle

[Eric Liang]

sure

[Eric Liang]

https://bugs.webkit.org/show_bug.cgi?id=200367