Bug 72866 – Accessibility: AccessibilityController should support listening to notifications on all elements.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Currently there's no way to add a listener for a notification that will fire on an element when it's first created. 

This is the first step in order to fix: https://bugs.webkit.org/show_bug.cgi?id=72624

[Dominic Mazzoni]

Created attachment 116056 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Collabora GTK+ EWS bot]

Comment on attachment 116056 [details]
Patch

Attachment 116056 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10472582

[Dominic Mazzoni]

Created attachment 116088 [details]
Patch

Fix GTK & Win compile

[Darin Fisher (:fishd, Google)]

Comment on attachment 116088 [details]
Patch

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:120
>      case AXObjectCache::AXActiveDescendantChanged:

Is it possible to just make the enums defined on AXObjectCache and their reflection through the API have exactly the same values such that a simple cast could replace this switch/case statement?  That's how we handle reflecting most other enum values.  Then, to keep the API ones consistent with their WebCore counterparts, we extend AssertMatchingEnums.cpp.

[Dominic Mazzoni]

Comment on attachment 116088 [details]
Patch

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

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:120
>>      case AXObjectCache::AXActiveDescendantChanged:
> 
> Is it possible to just make the enums defined on AXObjectCache and their reflection through the API have exactly the same values such that a simple cast could replace this switch/case statement?  That's how we handle reflecting most other enum values.  Then, to keep the API ones consistent with their WebCore counterparts, we extend AssertMatchingEnums.cpp.

Good idea. I split this into a separate bug - https://bugs.webkit.org/show_bug.cgi?id=72895 - patch coming momentarily. Then I can remove all of those files from this change.

[Dominic Mazzoni]

Created attachment 116113 [details]
Patch

Remove Chromium notification changes that were committed in bug 72895.

[WebKit Review Bot]

Comment on attachment 116113 [details]
Patch

Attachment 116113 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10393436

New failing tests:
accessibility/notification-listeners.html

[Dominic Mazzoni]

Created attachment 116320 [details]
Patch

Try again, cr-linux should pass...

[chris fleizach]

Comment on attachment 116320 [details]
Patch

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

You also need to make this work in WebKitTestRunner, which will basically duplicate a bunch of this code

> Tools/DumpRenderTree/AccessibilityController.h:65
> +#endif

Can these methods be reconciled with windows so we don't need two addNotificationListeners

> Tools/DumpRenderTree/AccessibilityController.h:82
> +#endif

this should probably be a RetainPtr<NotificationHandler>

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:34
> +#import <Cocoa/Cocoa.h>

i doubt you need to import Cocoa here. I believe the prefix header handles this

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:43
> +- (id)initAndListenToAllElements;

i think you should add a setter to set the platform element if you want (so you don't need two different init's)
and i think you should have to call startObserving explicitly.
that way you can remove both the init methods, and add a setPlatformElement:() and make startObserving public

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:47
> +

if these are private they should not be in the header. if they are not private they should not be prefixed with_

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:36
> +#import <Foundation/Foundation.h>

unlikely that you need Foundation here

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:66
> +    m_platformElement = platformElement;

if [super init] returns nil then you should also return nil right away

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:93
> +    // Release the old callback.

comment is not needed.

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:106
> +    [NSClassFromString(@"WebAccessibilityObjectWrapper") accessibilitySetShouldRepostNotifications:YES];

you should assert that NSClassFromString(@"WebAccessibilityObjectWrapper") returns something  (in case the name changes like it did recently)

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:122
> +        JSObjectCallAsFunction([mainFrame globalContext], m_notificationFunctionCallback, 0, 1, &notificationNameArgument, 0);

if you have an if block with a comment (making it more than one line), i believe you should be using brackets

[Dominic Mazzoni]

Comment on attachment 116320 [details]
Patch

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

New patch with WebKitTestRunner changes coming.

>> Tools/DumpRenderTree/AccessibilityController.h:65
>> +#endif
> 
> Can these methods be reconciled with windows so we don't need two addNotificationListeners

Sure, I gave them a "win" prefix.

>> Tools/DumpRenderTree/AccessibilityController.h:82
>> +#endif
> 
> this should probably be a RetainPtr<NotificationHandler>

Done.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:34
>> +#import <Cocoa/Cocoa.h>
> 
> i doubt you need to import Cocoa here. I believe the prefix header handles this

Done.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:43
>> +- (id)initAndListenToAllElements;
> 
> i think you should add a setter to set the platform element if you want (so you don't need two different init's)
> and i think you should have to call startObserving explicitly.
> that way you can remove both the init methods, and add a setPlatformElement:() and make startObserving public

Sure.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:47
>> +
> 
> if these are private they should not be in the header. if they are not private they should not be prefixed with_

Private, removed from the header.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:36
>> +#import <Foundation/Foundation.h>
> 
> unlikely that you need Foundation here

Done.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:66
>> +    m_platformElement = platformElement;
> 
> if [super init] returns nil then you should also return nil right away

Done.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:93
>> +    // Release the old callback.
> 
> comment is not needed.

Done.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:106
>> +    [NSClassFromString(@"WebAccessibilityObjectWrapper") accessibilitySetShouldRepostNotifications:YES];
> 
> you should assert that NSClassFromString(@"WebAccessibilityObjectWrapper") returns something  (in case the name changes like it did recently)

Good idea.

>> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:122
>> +        JSObjectCallAsFunction([mainFrame globalContext], m_notificationFunctionCallback, 0, 1, &notificationNameArgument, 0);
> 
> if you have an if block with a comment (making it more than one line), i believe you should be using brackets

Seems to be inconsistent, I found examples both ways. But brackets is fine.

[Dominic Mazzoni]

Created attachment 117363 [details]
Patch

[chris fleizach]

Comment on attachment 117363 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:37
> +bool AccessibilityController::addNotificationListener(JSValueRef functionCallback) {

bracket should be on the next line

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:52
> +bool AccessibilityController::removeNotificationListener() {

bracket should be on following line

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:68
> +    m_platformElement = nil;

not necessary. this value will be set to nil by default

> LayoutTests/accessibility/notification-listeners.html:66
> +        }, false);

you don't necessary need to do this after a timeout. just calling runTest() should work just as fine. Scripts don't run until the page loads anyway

> LayoutTests/accessibility/notification-listeners.html:76
> +<p id="description"></p>

you're missing the js-test-post include at the bottom of this file

[Dominic Mazzoni]

Created attachment 118417 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 118417 [details]
Patch for landing

Clearing flags on attachment: 118417

Committed r102378: <http://trac.webkit.org/changeset/102378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.