Summary: | [iOS] Check that Accessibility is enabled when receiving the enable Accessibility notification | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, darin, ggaren, webkit-bug-importer, youennf | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2020-08-03 17:00:55 PDT
Created attachment 405887 [details]
Patch
Comment on attachment 405887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405887&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:704 > + return; unblockPreferenceServiceIfNeeded() is called in various places like in m_enhancedAccessibilityObserver. Should this check be added in unblockPreferenceServiceIfNeeded instead of in this handler? That would mirror what unblockAccessibilityServerIfNeeded is ding. Created attachment 405918 [details]
Patch
(In reply to youenn fablet from comment #3) > Comment on attachment 405887 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405887&action=review > > > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:704 > > + return; > > unblockPreferenceServiceIfNeeded() is called in various places like in > m_enhancedAccessibilityObserver. Should this check be added in > unblockPreferenceServiceIfNeeded instead of in this handler? > That would mirror what unblockAccessibilityServerIfNeeded is ding. Good catch! I added a similar check for macOS. I did keep the check in the notification handlers, since I thought it might also make sense to prevent the rest of the handler code to run if accessibility is not enabled. Thanks for reviewing! Created attachment 405920 [details]
Patch
Created attachment 405921 [details]
Patch
Comment on attachment 405887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405887&action=review > Source/WebKit/ChangeLog:3 > + [iOS] Check that Accessibility is enabled when receiving the enable Accessibility notification Why? Created attachment 405938 [details]
Patch
Comment on attachment 405938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405938&action=review > Source/WebCore/PAL/ChangeLog:3 > + [iOS] Check that Accessibility is enabled when receiving the enable Accessibility notification Why? For security? Performance? Correctness? > Tools/TestWebKitAPI/Tests/WebKit/EnableAccessibility.mm:66 > + [NSApp accessibilitySetEnhancedUserInterfaceAttribute:[NSNumber numberWithBool:YES]]; I suggest @(YES) instead of [NSNumber numberWithBool:YES]. (In reply to Darin Adler from comment #10) > Comment on attachment 405938 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405938&action=review > > > Source/WebCore/PAL/ChangeLog:3 > > + [iOS] Check that Accessibility is enabled when receiving the enable Accessibility notification > > Why? For security? Performance? Correctness? > Yes, this is for security. I will add a note to the WebKit change log. > > Tools/TestWebKitAPI/Tests/WebKit/EnableAccessibility.mm:66 > > + [NSApp accessibilitySetEnhancedUserInterfaceAttribute:[NSNumber numberWithBool:YES]]; > > I suggest @(YES) instead of [NSNumber numberWithBool:YES]. Will fix. Thanks for reviewing! Created attachment 405942 [details]
Patch
Committed r265264: <https://trac.webkit.org/changeset/265264> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405942 [details]. |