In web process, the accessibility bundle path isn't set for MACCATALYST
<rdar://problem/53530200>
Created attachment 374882 [details] Patch
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"]
Created attachment 374894 [details] Patch
Created attachment 374924 [details] Patch
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?
Need it because in the webprocessloader bundle, we are checking whether application accessibility is enabled, which reads the pref in the iOS realm.
Created attachment 374925 [details] Patch
(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?
(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?
(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
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.
(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.
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.
Comment on attachment 374925 [details] Patch Clearing flags on attachment: 374925 Committed r247864: <https://trac.webkit.org/changeset/247864>
All reviewed patches have been landed. Closing bug.
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?
(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
sure
https://bugs.webkit.org/show_bug.cgi?id=200367