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

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: Andrzej Skalski)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla13
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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)
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com]

Updated

6 years ago
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com] → [good first bug][mentor=surkov.alexander@gmail.com][lang=c++]
(Assignee)

Updated

5 years ago
Assignee: nobody → andrzej.skalski
(Assignee)

Updated

5 years ago
Assignee: andrzej.skalski → askalski
(Assignee)

Comment 2

5 years ago
Created attachment 594986 [details] [diff] [review]
First patch proposition
Attachment #594986 - Flags: review?(surkov.alexander)
(Reporter)

Comment 3

5 years ago
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"
Attachment #594986 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 595486 [details] [diff] [review]
Second patch proposition
Attachment #594986 - Attachment is obsolete: true
Attachment #595486 - Flags: review?(surkov.alexander)
(Reporter)

Comment 6

5 years ago
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"
Attachment #595486 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 595713 [details] [diff] [review]
Third patch proposition
(Reporter)

Updated

5 years ago
Attachment #595486 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cbe927b103
https://hg.mozilla.org/mozilla-central/rev/f9cbe927b103
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.