Bug 144653 – AX: [ATK] ARIA menu items should not have anonymous block children

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

ARIA menu items should not have anonymous block children. At least not for ATK, though I'm wondering about other platforms.

[Radar WebKit Bug Importer]

<rdar://problem/20830373>

[Joanmarie Diggs]

Created attachment 252435 [details]
Patch

[Joanmarie Diggs]

Chris: When I rewrote the aria-menubar-menuitems.html test to be more cross-platform friendly, I made it a hierarchy dump. Your platform has AXListMarker children. Is that really expected? If not, I'd love to come up with a shared fix and defer this to WebCore to handle.

[Joanmarie Diggs]

(Adding Chris to the CC list, which I failed to do when I asked him my question.)

When you have a chance, please take a look at the patch which includes the accessible hierarchy of OS X for an ARIA menu made out of a HTML unordered list if we change nothing for your platform. If that is the expected hierarchy for your platform, cool. Please review for the change to my platform. :) Otherwise, if you tell me what the hierarchy should be, I'll work on a new patch. Thanks!

[chris fleizach]

(In reply to comment #3)
> Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> cross-platform friendly, I made it a hierarchy dump. Your platform has
> AXListMarker children. Is that really expected? If not, I'd love to come up
> with a shared fix and defer this to WebCore to handle.

I believe yes we do want AXListMarker elements

[chris fleizach]

(In reply to comment #5)
> (In reply to comment #3)
> > Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> > cross-platform friendly, I made it a hierarchy dump. Your platform has
> > AXListMarker children. Is that really expected? If not, I'd love to come up
> > with a shared fix and defer this to WebCore to handle.
> 
> I believe yes we do want AXListMarker elements

Actually in this case since the <li> elements are menu item roles, i don't think we should be exposing them. perhaps the problem is that we are returning children for this element when menu items shouldn't be allowed to do that

[Joanmarie Diggs]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> > > cross-platform friendly, I made it a hierarchy dump. Your platform has
> > > AXListMarker children. Is that really expected? If not, I'd love to come up
> > > with a shared fix and defer this to WebCore to handle.
> > 
> > I believe yes we do want AXListMarker elements
> 
> Actually in this case since the <li> elements are menu item roles, i don't
> think we should be exposing them.

Good. That's what I think as well.

> perhaps the problem is that we are
> returning children for this element when menu items shouldn't be allowed to
> do that

I was hoping it would be as simple as adding menu items, and roles that subclass them, to the list of ARIA roles that cannot have children. But menu items with submenus CAN have children as is demonstrated in the hierarchies shown in the patch. Assuming that the correct thing to do in that case is NOT ignore the menu item and make the submenu a sibling of other menu items, we'll have to be more clever.

Related aside: As I work on these and other bugs, I'm finding the hierarchy dumps in layout tests quite enlightening in terms of catching oddities like this one, identifying regressions/changes, and understanding the different platforms. These benefits, along with it being cross-platform friendly, make me think this is the way we should proceed with as many accessibility layout tests as is possible.

[chris fleizach]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #3)
> > > > Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> > > > cross-platform friendly, I made it a hierarchy dump. Your platform has
> > > > AXListMarker children. Is that really expected? If not, I'd love to come up
> > > > with a shared fix and defer this to WebCore to handle.
> > > 
> > > I believe yes we do want AXListMarker elements
> > 
> > Actually in this case since the <li> elements are menu item roles, i don't
> > think we should be exposing them.
> 
> Good. That's what I think as well.
> 
> > perhaps the problem is that we are
> > returning children for this element when menu items shouldn't be allowed to
> > do that
> 
> I was hoping it would be as simple as adding menu items, and roles that
> subclass them, to the list of ARIA roles that cannot have children. But menu
> items with submenus CAN have children as is demonstrated in the hierarchies
> shown in the patch. Assuming that the correct thing to do in that case is
> NOT ignore the menu item and make the submenu a sibling of other menu items,
> we'll have to be more clever.
> 

Right. I think the fix is probably that we should not make AXListMarker children if the parent is not a ListItemRole

> Related aside: As I work on these and other bugs, I'm finding the hierarchy
> dumps in layout tests quite enlightening in terms of catching oddities like
> this one, identifying regressions/changes, and understanding the different
> platforms. These benefits, along with it being cross-platform friendly, make
> me think this is the way we should proceed with as many accessibility layout
> tests as is possible.

The downside i've found is that the cause a lot more spurious regressions. for example, if we make <pre> into a group instead of being ignored that might cause a dozen tests to fail because the hierarchy is different. so in that sense, tests become more of a catchall rather then specific tests.

[Joanmarie Diggs]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #3)
> > > > > Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> > > > > cross-platform friendly, I made it a hierarchy dump. Your platform has
> > > > > AXListMarker children. Is that really expected? If not, I'd love to come up
> > > > > with a shared fix and defer this to WebCore to handle.
> > > > 
> > > > I believe yes we do want AXListMarker elements
> > > 
> > > Actually in this case since the <li> elements are menu item roles, i don't
> > > think we should be exposing them.
> > 
> > Good. That's what I think as well.
> > 
> > > perhaps the problem is that we are
> > > returning children for this element when menu items shouldn't be allowed to
> > > do that
> > 
> > I was hoping it would be as simple as adding menu items, and roles that
> > subclass them, to the list of ARIA roles that cannot have children. But menu
> > items with submenus CAN have children as is demonstrated in the hierarchies
> > shown in the patch. Assuming that the correct thing to do in that case is
> > NOT ignore the menu item and make the submenu a sibling of other menu items,
> > we'll have to be more clever.
> > 
> 
> Right. I think the fix is probably that we should not make AXListMarker
> children if the parent is not a ListItemRole

What about the StaticText children? I just used the Accessibility Inspector to look at a native menu and its items, and native menu items don't seems to have children unless it's a submenu.

[chris fleizach]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #3)
> > > > > > Chris: When I rewrote the aria-menubar-menuitems.html test to be more
> > > > > > cross-platform friendly, I made it a hierarchy dump. Your platform has
> > > > > > AXListMarker children. Is that really expected? If not, I'd love to come up
> > > > > > with a shared fix and defer this to WebCore to handle.
> > > > > 
> > > > > I believe yes we do want AXListMarker elements
> > > > 
> > > > Actually in this case since the <li> elements are menu item roles, i don't
> > > > think we should be exposing them.
> > > 
> > > Good. That's what I think as well.
> > > 
> > > > perhaps the problem is that we are
> > > > returning children for this element when menu items shouldn't be allowed to
> > > > do that
> > > 
> > > I was hoping it would be as simple as adding menu items, and roles that
> > > subclass them, to the list of ARIA roles that cannot have children. But menu
> > > items with submenus CAN have children as is demonstrated in the hierarchies
> > > shown in the patch. Assuming that the correct thing to do in that case is
> > > NOT ignore the menu item and make the submenu a sibling of other menu items,
> > > we'll have to be more clever.
> > > 
> > 
> > Right. I think the fix is probably that we should not make AXListMarker
> > children if the parent is not a ListItemRole
> 
> What about the StaticText children? I just used the Accessibility Inspector
> to look at a native menu and its items, and native menu items don't seems to
> have children unless it's a submenu.

It seems like it would be the same. the only problem that would occur is if someone put a button or a link in a menu item. ostensibly it's an author error, but it would render that stuff inaccessible

[Joanmarie Diggs]

Comment on attachment 252435 [details]
Patch

Clearing the r? flag.

Tomorrow I'll do a new patch which includes getting the AXListItemMarker out of the Mac tree.

Thanks for the thoughts and info Chris!

[Joanmarie Diggs]

Created attachment 252761 [details]
Patch

[Joanmarie Diggs]

The new patch takes care of the list markers. Wound up doing it for all platforms. I hope that won't break Windows. But once we have a patch we agree upon, I'll alert Brent.

Given that this particular test is so dependent upon child count and child index,  any changes like what Chris mentioned in comment 8 will break this test whether it dumps a hierarchy or not. So I still turned it into a hierarchy dump, but limiting it to just the menubar descendants and dumping the accessible title so that we can more easily identify the ARIA UI. Hope that's ok.

[Build Bot]

Comment on attachment 252761 [details]
Patch

Attachment 252761 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6729772839206912

New failing tests:
platform/mac/accessibility/search-predicate.html

[Build Bot]

Created attachment 252763 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Joanmarie Diggs]

Created attachment 252766 [details]
Patch

[chris fleizach]

Comment on attachment 252766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252766&action=review

> LayoutTests/resources/accessibility-helper.js:26
> +function dumpAccessibilityTreeWithTitle(accessibilityObject, stopElement, indent) {

can we modify dumpAccessibilityTree to take another argument instead of copying this method

[Joanmarie Diggs]

Created attachment 252781 [details]
Patch

[Joanmarie Diggs]

Brent: Heads up that when this patch lands, it will require new results for an existing Windows test, and may cause other test failures (though I hope not).

If a test does fail, it will almost certainly be because that test assumes a child count and/or child index based on an accessible listmarker that is no longer in the tree -- and which (presumably) doesn't belong in the tree -- and the fix for that failure will be to correct/update the test.

Mind if I land this patch, or would you like to run the Windows tests first?

[Joanmarie Diggs]

Comment on attachment 252781 [details]
Patch

Ok, I'm going to land it and just wait to be pinged about any failing Windows tests I need to look at.

[WebKit Commit Bot]

Comment on attachment 252781 [details]
Patch

Clearing flags on attachment: 252781

Committed r184213: <http://trac.webkit.org/changeset/184213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Yes, accessibility/aria-menubar-menuitems.html is now broken on Windows. Please fix.

[Joanmarie Diggs]

(In reply to comment #23)
> Yes, accessibility/aria-menubar-menuitems.html is now broken on Windows.
> Please fix.

If that's the only one, I think you just need to generate new results for Windows. Which I'd do if I had a Windows machine, but I don't. Is this something you're able to do? I'd be happy to sanity check the resulting expectations.

[Joanmarie Diggs]

(In reply to comment #24)
> (In reply to comment #23)
> > Yes, accessibility/aria-menubar-menuitems.html is now broken on Windows.
> > Please fix.
> 
> If that's the only one, I think you just need to generate new results for
> Windows. Which I'd do if I had a Windows machine, but I don't. Is this
> something you're able to do? I'd be happy to sanity check the resulting
> expectations.

Looking at https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r184217%20(51780)/accessibility/aria-menubar-menuitems-pretty-diff.html, what I said above is clearly not accurate. I opened bug 144936 for this issue and will see if I can modify the test to make win happy.