Bug 72866 - Accessibility: AccessibilityController should support listening to notifications on all elements.
Summary: Accessibility: AccessibilityController should support listening to notificati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 312.x
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on:
Blocks: 72624
  Show dependency treegraph
 
Reported: 2011-11-21 01:36 PST by Dominic Mazzoni
Modified: 2011-12-08 13:27 PST (History)
8 users (show)

See Also:


Attachments
Patch (43.87 KB, patch)
2011-11-21 02:44 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (43.81 KB, patch)
2011-11-21 07:57 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (38.45 KB, patch)
2011-11-21 11:45 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (38.45 KB, patch)
2011-11-22 22:42 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (70.20 KB, patch)
2011-12-01 00:22 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (67.95 KB, patch)
2011-12-08 10:31 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2011-11-21 01:36:18 PST
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
Comment 1 Dominic Mazzoni 2011-11-21 02:44:44 PST
Created attachment 116056 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-21 02:46:50 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Collabora GTK+ EWS bot 2011-11-21 04:30:12 PST
Comment on attachment 116056 [details]
Patch

Attachment 116056 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10472582
Comment 4 Dominic Mazzoni 2011-11-21 07:57:33 PST
Created attachment 116088 [details]
Patch

Fix GTK & Win compile
Comment 5 Darin Fisher (:fishd, Google) 2011-11-21 08:59:43 PST
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.
Comment 6 Dominic Mazzoni 2011-11-21 09:39:21 PST
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.
Comment 7 Dominic Mazzoni 2011-11-21 11:45:59 PST
Created attachment 116113 [details]
Patch

Remove Chromium notification changes that were committed in bug 72895.
Comment 8 WebKit Review Bot 2011-11-21 12:18:27 PST
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
Comment 9 Dominic Mazzoni 2011-11-22 22:42:44 PST
Created attachment 116320 [details]
Patch

Try again, cr-linux should pass...
Comment 10 chris fleizach 2011-11-28 12:00:07 PST
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
Comment 11 Dominic Mazzoni 2011-12-01 00:16:17 PST
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.
Comment 12 Dominic Mazzoni 2011-12-01 00:22:01 PST
Created attachment 117363 [details]
Patch
Comment 13 chris fleizach 2011-12-01 08:58:47 PST
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
Comment 14 Dominic Mazzoni 2011-12-08 10:31:43 PST
Created attachment 118417 [details]
Patch for landing
Comment 15 WebKit Review Bot 2011-12-08 13:27:36 PST
Comment on attachment 118417 [details]
Patch for landing

Clearing flags on attachment: 118417

Committed r102378: <http://trac.webkit.org/changeset/102378>
Comment 16 WebKit Review Bot 2011-12-08 13:27:42 PST
All reviewed patches have been landed.  Closing bug.