Summary: | AccessibilityMenuList does not fire change notification when popup is not open. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominic Mazzoni <dmazzoni> | ||||||||
Component: | Accessibility | Assignee: | Dominic Mazzoni <dmazzoni> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, cfleizach, dtseng, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Dominic Mazzoni
2011-10-05 10:19:50 PDT
Created attachment 109843 [details]
Patch
Comment on attachment 109843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109843&action=review layout test needed. otherwise seems ok > Source/WebCore/accessibility/AccessibilityMenuList.cpp:96 > + popup->didUpdateActiveOption(optionIndex); i think we should take this patch to verify (outside of ASSERT) that childObjects[0].get() actually is a AccessibilityMenuListPopup, by doing something like if (childObjects[0]->isMenuListPopup()) Sure, happy to add that additional check. As for the layout test, tests that check for notifications have to be platform-specific currently. Would it be okay to check for this notification on Mac OS X even though it isn't strictly needed there? It'd be more trouble to develop the test as Win or GTK specific. I'm also planning to add a Chromium AccessibilityController soon. (In reply to comment #3) > Sure, happy to add that additional check. > > As for the layout test, tests that check for notifications have to be platform-specific currently. Would it be okay to check for this notification on Mac OS X even though it isn't strictly needed there? It'd be more trouble to develop the test as Win or GTK specific. Yes I think that's acceptable. With this fix we mainly want to check that a notification is received (not that it's received by a specific platform) > > I'm also planning to add a Chromium AccessibilityController soon. Great! Turns out that this notification doesn't even exist on Mac OS X, so I decided I'm adding a generic test and a Chromium expectations file, and then if we want we can add expectations for any other platform that passes. Created attachment 110487 [details]
Patch
Comment on attachment 110487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110487&action=review r=me with minor layout changes > LayoutTests/accessibility/menu-list-sends-change-notification.html:20 > + layoutTestController.notifyDone(); you should also remove the notification listener when done with this layout test Created attachment 110564 [details]
Patch for landing
Comment on attachment 110564 [details] Patch for landing Clearing flags on attachment: 110564 Committed r97177: <http://trac.webkit.org/changeset/97177> All reviewed patches have been landed. Closing bug. |