Bug 184619 - AX: AOM: respect the accessibility setting for dispatching the accessible events
Summary: AX: AOM: respect the accessibility setting for dispatching the accessible events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-13 18:36 PDT by Nan Wang
Modified: 2018-04-19 10:36 PDT (History)
16 users (show)

See Also:


Attachments
patch (21.35 KB, patch)
2018-04-13 18:48 PDT, Nan Wang
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.71 MB, application/zip)
2018-04-13 19:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.22 MB, application/zip)
2018-04-13 20:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (15.94 MB, application/zip)
2018-04-13 20:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (2.98 MB, application/zip)
2018-04-13 20:45 PDT, Build Bot
no flags Details
patch (21.33 KB, patch)
2018-04-13 23:23 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (20.40 KB, patch)
2018-04-16 15:58 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (20.50 KB, patch)
2018-04-16 17:10 PDT, Nan Wang
rniwa: review-
Details | Formatted Diff | Diff
patch (18.90 KB, patch)
2018-04-17 17:08 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (18.86 KB, patch)
2018-04-17 17:17 PDT, Nan Wang
rniwa: review-
Details | Formatted Diff | Diff
patch (20.64 KB, patch)
2018-04-18 12:12 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2018-04-13 18:36:22 PDT
We should plumb through the accessibility setting to WebKit in order to make accessibility events enable/disable on the fly.

<rdar://problem/39223256>
Comment 1 Nan Wang 2018-04-13 18:48:18 PDT
Created attachment 337939 [details]
patch
Comment 2 Build Bot 2018-04-13 18:50:55 PDT
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.
Comment 3 chris fleizach 2018-04-13 19:52:53 PDT
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...
Comment 4 Build Bot 2018-04-13 19:54:25 PDT
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
Comment 5 Build Bot 2018-04-13 19:54:26 PDT
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
Comment 6 Build Bot 2018-04-13 20:02:05 PDT
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
Comment 7 Build Bot 2018-04-13 20:02:06 PDT
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
Comment 8 Build Bot 2018-04-13 20:28:44 PDT
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
Comment 9 Build Bot 2018-04-13 20:28:52 PDT
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
Comment 10 Build Bot 2018-04-13 20:45:10 PDT
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
Comment 11 Build Bot 2018-04-13 20:45:11 PDT
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
Comment 12 Nan Wang 2018-04-13 23:23:52 PDT
Created attachment 337959 [details]
patch

fixed the tests, the boolean should be true by default
Comment 13 Build Bot 2018-04-13 23:28:55 PDT
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.
Comment 14 chris fleizach 2018-04-16 10:35:17 PDT
Shouldn’t this flag be observable on the Mac too
Comment 15 Nan Wang 2018-04-16 11:34:24 PDT
(In reply to chris fleizach from comment #14)
> Shouldn’t this flag be observable on the Mac too

Yes, will update that.
Comment 16 Nan Wang 2018-04-16 15:58:47 PDT
Created attachment 338052 [details]
patch

Added macOS support
Comment 17 Build Bot 2018-04-16 16:01:55 PDT
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.
Comment 18 chris fleizach 2018-04-16 16:32:43 PDT
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 ?
Comment 19 Nan Wang 2018-04-16 17:06:12 PDT
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!
Comment 20 Nan Wang 2018-04-16 17:10:56 PDT
Created attachment 338060 [details]
patch

update and fix macOS build.
Comment 21 Build Bot 2018-04-16 17:12:46 PDT
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.
Comment 22 chris fleizach 2018-04-17 11:47:40 PDT
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?
Comment 23 Nan Wang 2018-04-17 11:54:13 PDT
(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.
Comment 24 Ryosuke Niwa 2018-04-17 13:44:50 PDT
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?
Comment 25 Nan Wang 2018-04-17 17:08:03 PDT
Created attachment 338162 [details]
patch

update from review
Comment 26 Build Bot 2018-04-17 17:10:35 PDT
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.
Comment 27 Nan Wang 2018-04-17 17:17:07 PDT
Created attachment 338165 [details]
patch

update from review
- made accessibilityEventsEnabled a setting on a page
- reduce including AccessibilitySupport.h in multiple places
Comment 28 Build Bot 2018-04-17 17:19:18 PDT
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.
Comment 29 Ryosuke Niwa 2018-04-17 18:37:04 PDT
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.
Comment 30 Nan Wang 2018-04-17 18:46:17 PDT
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?
Comment 31 Ryosuke Niwa 2018-04-17 20:13:39 PDT
(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).
Comment 32 Ryosuke Niwa 2018-04-17 20:14:11 PDT
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.
Comment 33 Nan Wang 2018-04-18 12:12:57 PDT
Created attachment 338239 [details]
patch

updated from review
- Do not mutate system wide flag
- make the setting conditional
Comment 34 Ryosuke Niwa 2018-04-19 02:09:38 PDT
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.
Comment 35 WebKit Commit Bot 2018-04-19 10:36:46 PDT
Comment on attachment 338239 [details]
patch

Clearing flags on attachment: 338239

Committed r230808: <https://trac.webkit.org/changeset/230808>
Comment 36 WebKit Commit Bot 2018-04-19 10:36:48 PDT
All reviewed patches have been landed.  Closing bug.