Bug 69440 – AccessibilityMenuList does not fire change notification when popup is not open.

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 109843 [details]
Patch

[chris fleizach]

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())

[Dominic Mazzoni]

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.

[chris fleizach]

(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!

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 110487 [details]
Patch

[chris fleizach]

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

[Dominic Mazzoni]

Created attachment 110564 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 110564 [details]
Patch for landing

Clearing flags on attachment: 110564

Committed r97177: <http://trac.webkit.org/changeset/97177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.