Bug 184619 – AX: AOM: respect the accessibility setting for dispatching the accessible events

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

We should plumb through the accessibility setting to WebKit in order to make accessibility events enable/disable on the fly.

<rdar://problem/39223256>

[Nan Wang]

Created attachment 337939 [details]
patch

[EWS Watchlist]

Attachment 337939 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 337939 [details]
patch

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

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:682
> +{

This should be able to work on Mac too...

[EWS Watchlist]

Comment on attachment 337939 [details]
patch

Attachment 337939 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7311645

New failing tests:
accessibility/mac/AOM-events-all.html
accessibility/mac/AOM-event-accessiblesetvalue.html
accessibility/mac/AOM-events.html

[EWS Watchlist]

Created attachment 337949 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 337939 [details]
patch

Attachment 337939 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7311793

New failing tests:
accessibility/mac/AOM-event-accessiblesetvalue.html

[EWS Watchlist]

Created attachment 337951 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 337939 [details]
patch

Attachment 337939 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7311823

New failing tests:
accessibility/ios-simulator/AOM-dismiss-event.html

[EWS Watchlist]

Created attachment 337954 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 337939 [details]
patch

Attachment 337939 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7311834

New failing tests:
accessibility/mac/AOM-event-accessiblesetvalue.html

[EWS Watchlist]

Created attachment 337955 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Nan Wang]

Created attachment 337959 [details]
patch

fixed the tests, the boolean should be true by default

[EWS Watchlist]

Attachment 337959 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Shouldn’t this flag be observable on the Mac too

[Nan Wang]

(In reply to chris fleizach from comment #14)
> Shouldn’t this flag be observable on the Mac too

Yes, will update that.

[Nan Wang]

Created attachment 338052 [details]
patch

Added macOS support

[EWS Watchlist]

Attachment 338052 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:275:  _AXSWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 338052 [details]
patch

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

> Source/WTF/wtf/Platform.h:1332
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400)

Should this be cocoa &&( iosversion || macOS version)

> Source/WebKit/Configurations/WebKit.xcconfig:-42
> -WK_ACCESSIBILITY_LDFLAGS = $(WK_ACCESSIBILITY_LDFLAGS_$(WK_COCOA_TOUCH));

Do you still need this change? All platforms should have this

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:210
> +#if __has_include(<AccessibilitySupport.h>)

Should this be has_axevents instead of cocoa

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:270
> +#if PLATFORM(COCOA)

Have_axevents ?

[Nan Wang]

Comment on attachment 338052 [details]
patch

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

>> Source/WTF/wtf/Platform.h:1332
>> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400)
> 
> Should this be cocoa &&( iosversion || macOS version)

I don't think so. By searching through the project, it seems like all the iosversion/macOSversion need to be with PLATFORM(iOS)/PLATFORM(MAC)

>> Source/WebKit/Configurations/WebKit.xcconfig:-42
>> -WK_ACCESSIBILITY_LDFLAGS = $(WK_ACCESSIBILITY_LDFLAGS_$(WK_COCOA_TOUCH));
> 
> Do you still need this change? All platforms should have this

I think so. I just realized that we need to limit the libAccessibility to be linked after 10.14

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:210
>> +#if __has_include(<AccessibilitySupport.h>)
> 
> Should this be has_axevents instead of cocoa

Yes!

[Nan Wang]

Created attachment 338060 [details]
patch

update and fix macOS build.

[EWS Watchlist]

Attachment 338060 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:275:  _AXSWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 338060 [details]
patch

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

> Source/WebCore/testing/Internals.cpp:506
> +    _AXSSetWebAccessibilityEventsEnabled(true);

is this necessary or should we just rely on what the system provides as a default?

[Nan Wang]

(In reply to chris fleizach from comment #22)
> Comment on attachment 338060 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338060&action=review
> 
> > Source/WebCore/testing/Internals.cpp:506
> > +    _AXSSetWebAccessibilityEventsEnabled(true);
> 
> is this necessary or should we just rely on what the system provides as a
> default?

I think this is necessary that we need to make sure the system is in the default state.

[Ryosuke Niwa]

Comment on attachment 338060 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2160
> -    return RuntimeEnabledFeatures::sharedFeatures().accessibilityObjectModelEnabled();
> +    return RuntimeEnabledFeatures::sharedFeatures().accessibilityObjectModelEnabled() && this->page()->accessibilityEventsEnabled();

accessibilityEventsEnabled should either be a setting on a page or another runtime enabled features.
r- because adding one-off feature flag like this has proven records of accidentally shipping or disabling the feature in a wrong platform.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:3072
> +    _page->updateAccessibilityEventsEnabled();

Why can't this pass in the boolean to updateAccessibilityEventsEnabled instead of including AccessibilitySupport.h once again in WebPage.cpp?

[Nan Wang]

Created attachment 338162 [details]
patch

update from review

[EWS Watchlist]

Attachment 338162 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nan Wang]

Created attachment 338165 [details]
patch

update from review
- made accessibilityEventsEnabled a setting on a page
- reduce including AccessibilitySupport.h in multiple places

[EWS Watchlist]

Attachment 338165 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.cpp:270:  _AXSSetWebAccessibilityEventsEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 338165 [details]
patch

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

> Source/WTF/wtf/Platform.h:1333
> +#define HAVE_ACCESSIBILITY_EVENTS 1

I think we should have ENABLE flag instead.

> Source/WebCore/page/Settings.yaml:735
> +  initial: true

This should be conditional to the build flag. It's weird to have an on-by-default flag which doesn't do anything loser OS

> Source/WebCore/testing/Internals.cpp:270
> +void _AXSSetWebAccessibilityEventsEnabled(Boolean);

Does this feature enable it for this web content process, or is it system wide?
We can't mutate a global system state because we run tests concurrency.

Also, this should be done via internals.settings instead of any.

[Nan Wang]

Comment on attachment 338165 [details]
patch

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

>> Source/WebCore/testing/Internals.cpp:270
>> +void _AXSSetWebAccessibilityEventsEnabled(Boolean);
> 
> Does this feature enable it for this web content process, or is it system wide?
> We can't mutate a global system state because we run tests concurrency.
> 
> Also, this should be done via internals.settings instead of any.

This is system wide. 
If we do this via internals.settings instead, we are not going to test the CFNotification right?

[Ryosuke Niwa]

(In reply to Nan Wang from comment #30)
> Comment on attachment 338165 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338165&action=review
> 
> >> Source/WebCore/testing/Internals.cpp:270
> >> +void _AXSSetWebAccessibilityEventsEnabled(Boolean);
> > 
> > Does this feature enable it for this web content process, or is it system wide?
> > We can't mutate a global system state because we run tests concurrency.
> > 
> > Also, this should be done via internals.settings instead of any.
> 
> This is system wide. 
> If we do this via internals.settings instead, we are not going to test the
> CFNotification right?

But we can't mutate the system wide settings like this. It's okay if CFNotification path is not tested in layout tests. You should write an API test which manually dispatches a notification (again, without mutating the system wide setting).

[Ryosuke Niwa]

Comment on attachment 338165 [details]
patch

r- because it's not okay to mutate the global state via internals.settings as it's currently done.

[Nan Wang]

Created attachment 338239 [details]
patch

updated from review
- Do not mutate system wide flag
- make the setting conditional

[Ryosuke Niwa]

Comment on attachment 338239 [details]
patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:702
> +    CFNotificationCenterAddObserver(CFNotificationCenterGetDarwinNotifyCenter(), (__bridge const void *)(self), accessibilityEventsEnabledChangedCallback, kAXSWebAccessibilityEventsEnabledNotification, 0, CFNotificationSuspensionBehaviorDeliverImmediately);

You should add a WebKitAPI test which sends this notification to UI process.
You can do that in a separate patch though.

[WebKit Commit Bot]

Comment on attachment 338239 [details]
patch

Clearing flags on attachment: 338239

Committed r230808: <https://trac.webkit.org/changeset/230808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.