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
Created attachment 116056 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 116056 [details] Patch Attachment 116056 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10472582
Created attachment 116088 [details] Patch Fix GTK & Win compile
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 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.
Created attachment 116113 [details] Patch Remove Chromium notification changes that were committed in bug 72895.
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
Created attachment 116320 [details] Patch Try again, cr-linux should pass...
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, ¬ificationNameArgument, 0); if you have an if block with a comment (making it more than one line), i believe you should be using brackets
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, ¬ificationNameArgument, 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.
Created attachment 117363 [details] Patch
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
Created attachment 118417 [details] Patch for landing
Comment on attachment 118417 [details] Patch for landing Clearing flags on attachment: 118417 Committed r102378: <http://trac.webkit.org/changeset/102378>
All reviewed patches have been landed. Closing bug.