Last Comment Bug 706134 - ARIA listitem shouldn't expose selectable state and pick up aria-selected and aria-checked
: ARIA listitem shouldn't expose selectable state and pick up aria-selected and...
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Andrzej Skalski
:
Mentors:
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2011-11-29 08:46 PST by alexander :surkov
Modified: 2012-02-10 19:40 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch proposition (5.55 KB, patch)
2012-02-07 04:22 PST, Andrzej Skalski
surkov.alexander: review-
Details | Diff | Review
Second patch proposition (6.71 KB, patch)
2012-02-08 12:20 PST, Andrzej Skalski
surkov.alexander: review+
Details | Diff | Review
Third patch proposition (6.35 KB, patch)
2012-02-09 06:13 PST, Andrzej Skalski
no flags Details | Diff | Review

Description alexander :surkov 2011-11-29 08:46:12 PST
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 2011-11-29 08:48:35 PST
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 2012-02-07 04:22:00 PST
Created attachment 594986 [details] [diff] [review]
First patch proposition
Comment 3 alexander :surkov 2012-02-07 18:14:19 PST
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 2012-02-08 12:20:00 PST
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 2012-02-08 12:20:47 PST
Created attachment 595486 [details] [diff] [review]
Second patch proposition
Comment 6 alexander :surkov 2012-02-08 20:23:40 PST
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 2012-02-09 06:13:13 PST
Created attachment 595713 [details] [diff] [review]
Third patch proposition
Comment 8 alexander :surkov 2012-02-10 10:23:01 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/f9cbe927b103
Comment 9 Ed Morley [:emorley] 2012-02-10 19:40:55 PST
https://hg.mozilla.org/mozilla-central/rev/f9cbe927b103

Note You need to log in before you can comment on or make changes to this bug.