Bug 69440 - AccessibilityMenuList does not fire change notification when popup is not open.
Summary: AccessibilityMenuList does not fire change notification when popup is not open.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 10:19 PDT by Dominic Mazzoni
Modified: 2011-10-11 13:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.04 KB, patch)
2011-10-05 13:00 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2011-10-11 02:12 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (5.93 KB, patch)
2011-10-11 13:27 PDT, 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-10-05 10:19:50 PDT
On Mac OS X, an AccessibilityMenuList only changes when the popup is visible. On other platforms, it can be changed when focused by pressing the arrow key, but no platform accessibility notification is being sent.
Comment 1 Dominic Mazzoni 2011-10-05 13:00:41 PDT
Created attachment 109843 [details]
Patch
Comment 2 chris fleizach 2011-10-05 15:27:34 PDT
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())
Comment 3 Dominic Mazzoni 2011-10-05 15:53:29 PDT
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.
Comment 4 chris fleizach 2011-10-05 15:57:03 PDT
(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!
Comment 5 Dominic Mazzoni 2011-10-11 02:10:30 PDT
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.
Comment 6 Dominic Mazzoni 2011-10-11 02:12:31 PDT
Created attachment 110487 [details]
Patch
Comment 7 chris fleizach 2011-10-11 07:58:24 PDT
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
Comment 8 Dominic Mazzoni 2011-10-11 13:27:08 PDT
Created attachment 110564 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-10-11 13:45:33 PDT
Comment on attachment 110564 [details]
Patch for landing

Clearing flags on attachment: 110564

Committed r97177: <http://trac.webkit.org/changeset/97177>
Comment 10 WebKit Review Bot 2011-10-11 13:45:38 PDT
All reviewed patches have been landed.  Closing bug.