[iOS] Check that Accessibility is enabled when receiving the enable Accessibility notification

[Per Arne Vollan]

Return early when handling this notification if Accessibility is not enabled.

[Per Arne Vollan]

rdar://problem/66498397

[Per Arne Vollan]

Created attachment 405887 [details]
Patch

[youenn fablet]

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.

[Per Arne Vollan]

Created attachment 405918 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 405920 [details]
Patch

[Per Arne Vollan]

Created attachment 405921 [details]
Patch

[Darin Adler]

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?

[Per Arne Vollan]

Created attachment 405938 [details]
Patch

[Darin Adler]

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].

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 405942 [details]
Patch

[EWS]

Committed r265264: <https://trac.webkit.org/changeset/265264>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405942 [details].