706134 - ARIA listitem shouldn't expose selectable state and pick up aria-selected and aria-checked

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spec points it's used for ARIA role="list" and aria-selected and aria-checked aren't applied to it - http://www.w3.org/TR/2010/WD-wai-aria-20100916/roles#listitem

Comment 1

[alexander :surkov (:asurkov)]

pretty easy fix "listitem" entry in nsARIAMap.cpp - http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.cpp#269

please make sure mochitests passes (python runtests.py --a11y)

Comment 2

[Andrzej Skalski]

Attachment: First patch proposition

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 594986 [details] [diff] [review]
First patch proposition

Review of attachment 594986 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/focus/test_takeFocus.html
@@ -50,4 @@
>        gQueue.push(new takeFocusInvoker("aria-link"));
>        gQueue.push(new takeFocusInvoker("aria-link2"));
>        gQueue.push(new takeFocusInvoker("link"));
> -      gQueue.push(new takeFocusInvoker("item2"));

nah, please change "listitem" to "option"

::: accessible/tests/mochitest/focus/test_takeFocus.xul
@@ -79,4 @@
>  
>        gQueue.push(new setTreeView("tree", new nsTableTreeView(5)));
>        gQueue.push(new takeFocusInvoker("tree", getLastChild));
> -      gQueue.push(new takeFocusInvoker("listitem2"));

wrong, "listitem2" is XUL listitem of XUL listbox

::: accessible/tests/mochitest/selectable/test_aria.html
@@ +48,5 @@
>        var select = getAccessible(id, [nsIAccessibleSelectable]);
> +      // listitem is not selectable
> +      //select.addChildToSelection(0);
> +      //testSelectableSelection(id, [ ]);
> +      //select.removeChildFromSelection(0);

remove role="list" block and remove corresponding HTML

btw, it wasn't listed by bug description but you should remove aria-multiselectable support from role="list"

@@ -79,5 @@
> -      select.removeChildFromSelection(0);
> -      testSelectableSelection(id, [ ]);
> -      select.selectAllSelection();
> -      testSelectableSelection(id, [ "listbox2_item1", "listbox2_item2" ]);
> -      select.clearSelection();

don't remove them but replace role="listitem" on role="option"

::: accessible/tests/mochitest/test_aria_token_attrs.html
@@ +249,3 @@
>    </div>
>    <div id="listbox_multiselectable_undefined" role="listbox" aria-multiselectable="undefined">
> +    <div id="listitem_checked_undefined" role="listitem">item</div>

don't change this but replace role="listitem" on role="option"

Comment 4

[Andrzej Skalski]

Thanks for your comments Alexander, it did not occur to me to change listitem to option, now it changes everything. I hope new patch is better.

Comment 5

[Andrzej Skalski]

Attachment: Second patch proposition

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 595486 [details] [diff] [review]
Second patch proposition

Review of attachment 595486 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: accessible/tests/mochitest/focus/test_takeFocus.xul
@@ -84,1 @@
>        gQueue.invoke(); // Will call SimpleTest.finish();

not sure if it makes sense to remove empty line here

::: accessible/tests/mochitest/selectable/test_aria.html
@@ +149,1 @@
>    </div>

you forgot to remove "list1"

Comment 7

[Andrzej Skalski]

Attachment: Third patch proposition

Comment 8

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cbe927b103

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9cbe927b103