653601 - aria-selected ignored for ARIA tabs

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

When an element with @role="tab" is focused, its accessible always receives the selected state, even if @aria-selected is explicitly set to false. It should only receive the selected state if @aria-selected is true or undefined. While this normally isn't a problem, some tab controls do not activate a tab immediately when it is focused, but instead require the user to press the enter key on the tab. In addition, the ARIA spec requires this; see http://www.w3.org/TR/wai-aria/states_and_properties#aria-selected

Str:
1. Open the URL specified in this bug.
2. Tab to the tab control. You will land on the "Top Stories" tab. Check the selected state for this tab accessible.
Result (correct): This tab has the selected state.
3. Press right arrow to move to the "World" tab. Check the selected state for the "World" tab accessible and the "Top stories" tab accessible.
Expected: The "Top Stories" tab accessible should have the selected state. The "World" tab accessible should not.
Actual: The "World" tab accessible has the selected state. The "Top Stories" tab accessible does not.

Tested with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110428 Firefox/6.0a1

Comment 1

[Todd Kloots]

This bug in Firefox is currently affecting the accessibility of the new version of Y! Mail.  Our tabview control operates just like the one provided in the URL for this bug.

- Todd

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

OK, taking.

Comment 3

[Todd Kloots]

Cool, thanks David.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1

I found a bit of time to hack this afternoon, and Yahoo mail needs this. How does it look?

Comment 5

[alexander :surkov (:asurkov)]

you remove some magic (so authors must used aria-selected now for tabs) while they aren't required currently. I agree aria-selected should be respected but I need some reason to get rid this magic for all cases.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Abracadabra!

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 531686 [details] [diff] [review]
patch 1

r=me for the tests. I am not sure everyone will like our setTimeout set to that 200, but I trust this will suffice.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 531686 [details] [diff] [review]
patch 1

Let me make this a bit nicer.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

This version is less invasive. We keep the old behaviour except for the case where aria-selected is explicitly false.

Comment 10

[James Teh [:Jamie]]

(In reply to comment #9)
> This version is less invasive. We keep the old behaviour except for the case
> where aria-selected is explicitly false.
That's actually required by the ARIA spec:
> 1.Single-selection containers where the currently focused item is not selected. The selection normally follows the focus, and is managed by the user agent. Authors need only explicitly specify aria-selected when the focused item is not selected. Otherwise the currently focused item is considered to be selected so aria-selected="true" is redundant.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 531707 [details] [diff] [review]
patch 2

r=me with one nit:
>+  <!-- tab -->
>+  <div role="tablist">
>+  <div id="aria_tab1" role="tab" tabindex="0">unselected tab</div>
>+  <div id="aria_tab2" role="tab" tabindex="0" aria-selected="true">selected tab</div>
>+  <div id="aria_tab3" role="tab" tabindex="0">focused selected tab</div>
>+  <div id="aria_tab4" role="tab" tabindex="0" aria-selected="false">focused explicitly unselected tab</div>
>+  <div id="aria_tab5" role="tab" tabindex="0" aria-selected="true">focused selected tab</div>
>+  </div>

Please indent the individual tabs by two spaces so the parent/child relationship is clear upon glancing at it.

I'm still not too happy with those setTimeout calls, but except if we want to do an event queue here, there's no other way I guess. Let's just hope it doesn't introduce random oranges.

Comment 12

[alexander :surkov (:asurkov)]

I'm not happy with this hacky way. Can we extend nsStateMapEntry and introduce special instance for tab roles to keep this case in more nice way?

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #11)
> Comment on attachment 531707 [details] [diff] [review] [review]
> patch 2
> 
> r=me with one nit:

> Please indent the individual tabs by two spaces so the parent/child
> relationship is clear upon glancing at it.

Thanks. Fixed locally.

> I'm still not too happy with those setTimeout calls, but except if we want
> to do an event queue here, there's no other way I guess. Let's just hope it
> doesn't introduce random oranges.

I didn't like adding the timeouts but they make it explicit how the test works. I might add a queue instead, thanks for the idea.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #12)
> I'm not happy with this hacky way.

Me neither.

> Can we extend nsStateMapEntry and
> introduce special instance for tab roles to keep this case in more nice way?

Thanks. I had considered this idea and thought it was too much of an edge case but I think you are right actually.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

I wish we had an nsHTMLMap.

Comment 16

[Marco Zehe (:MarcoZ)]

(In reply to comment #15)
> I wish we had an nsHTMLMap.

Item for Firefox 7 or such!

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 3

This just moves the application of ARIA states to after the tab special case (in nsAccessible). The new nsStateMapEntry argument allows one to force the removal of the default state. I added this only for 'selected' but we should probably do it more often (followup bug?)

Comment 18

[alexander :surkov (:asurkov)]

It may be not right to change the calls order since activedescedant presence can affect on focused state. Well, the approach is hacky too. I thought about special state map entry for tabs like eARIATabSeelctable that will contain the logic for tab selectable states.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #18)
> It may be not right to change the calls order since activedescedant presence
> can affect on focused state.

Can you explain the activedescendant concern?

> I thought
> about special state map entry for tabs like eARIATabSeelctable that will
> contain the logic for tab selectable states.

I'm not 100% clear what the implementation would look like. Do you mean adding eARIATabSelectable to the eStateMapEntryID enum? I'm not sure how that approach would lead to cleaner code, given our current MapToStates algorithm.

Note I'd prefer a more robust follow up to all this on Bug 656623. What do you think?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> (In reply to comment #18)
> > It may be not right to change the calls order since activedescedant presence
> > can affect on focused state.
> 
> Can you explain the activedescendant concern?

ApplyARIAState() can set focused state because of activedescedant stuff. The tab role logic depends on focused state. So if you move ApplyARIAState() after tab role stuff then you may miss selected state in the case of activedescedant.

> 
> > I thought
> > about special state map entry for tabs like eARIATabSeelctable that will
> > contain the logic for tab selectable states.
> 
> I'm not 100% clear what the implementation would look like. Do you mean
> adding eARIATabSelectable to the eStateMapEntryID enum? I'm not sure how
> that approach would lead to cleaner code, given our current MapToStates
> algorithm.

sure, you should change it for that. You could introduce virtual method that expose additional logic, call it from MapToState, do inheritance (introduce special class for tab role).

> Note I'd prefer a more robust follow up to all this on Bug 656623. What do
> you think?

we have couple bugs like this one already, nobody has good idea what do with that. We can come with more or less nice solution right now or continue to add new hacky ifs. Maybe we should do small steps to keep things nicer instead of filing new cleaning up bugs everytime when we added new if and hope sometime in the future we do really great solution :)

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)

> > > I thought
> > > about special state map entry for tabs like eARIATabSeelctable that will
> > > contain the logic for tab selectable states.
> > 
> > I'm not 100% clear what the implementation would look like. Do you mean
> > adding eARIATabSelectable to the eStateMapEntryID enum? I'm not sure how
> > that approach would lead to cleaner code, given our current MapToStates
> > algorithm.
> 
> sure, you should change it for that. You could introduce virtual method that
> expose additional logic, call it from MapToState, do inheritance (introduce
> special class for tab role).
> 

This might make the most sense.

I'll try to catch you on IRC for other discussion.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Un-owning at least until I have a few hacking hours again. Anyone should feel free to work on this depending on other priorites of course.

Comment 23

[Marco Zehe (:MarcoZ)]

Can we take David's latest patch for Firefox 6, so Yahoo! and NVDA can keep moving with the new Yahoo! mail, and save the potential refactoring work that this sounds like for after Firefox 6 has been branched in a week from now?

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Makes sense to me but we should make sure we don't break aria-activedescendant. In the meantime, I threw what I have at try server: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-63787dbdbae6/
(our tests don't break)

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: (wip) patch

check for the attribute in State() when deciding if to add selected state

Comment 26

[Trevor Saunders (:tbsaunde)]

Attachment: patch4

rework of patch 2 as a temporary fix.  Change to not use our own logic in either case of aria-selected being set.

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 534206 [details] [diff] [review]
patch4

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

::: accessible/src/base/nsAccessible.cpp
@@ +1538,5 @@
>  
> +  if (GetContent()->AttrValueIs(kNameSpaceID_None,
> +                                nsAccessibilityAtoms::aria_selected,
> +                                nsAccessibilityAtoms::_true, eCaseMatters)) {
> +    state |= states::SELECTED;

wrong, aria-selected is not applied to any node, note ApplyARIAState do necessary logic

@@ +1551,3 @@
>        state |= states::SELECTED;
>      } else {
> +      // If focus is in a child of the tab panel surely the tab is selected!

also wrong, makes else block work for non tab roles.

Comment 28

[Trevor Saunders (:tbsaunde)]

Attachment: patch5

wrap the whole tab logic in a check for the correct role

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 534211 [details] [diff] [review]
patch5

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

one more patch please

::: accessible/src/base/nsAccessible.cpp
@@ +1543,1 @@
>        state |= states::SELECTED;

ApplyARIAState + nsARIAMap fix do this already

@@ +1543,5 @@
>        state |= states::SELECTED;
> +    } else if (!GetContent()->AttrValueIs(kNameSpaceID_None,
> +                                          nsAccessibilityAtoms::aria_selected,
> +                                          nsAccessibilityAtoms::_false,
> +                                          eCaseMatters)) {

mContent works too

you can keep this condition in one if that allows you to avoid extra indentation for existing code.

Comment 30

[Trevor Saunders (:tbsaunde)]

Attachment: patch6

address more nits

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 534213 [details] [diff] [review]
patch6

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

get back David's mochitests please

::: accessible/src/base/nsAccessible.cpp
@@ +1537,4 @@
>    ApplyARIAState(&state);
>  
> +  if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB &&
> +    !mContent->AttrValueIs(kNameSpaceID_None,

make proper identation

Comment 32

[Trevor Saunders (:tbsaunde)]

Attachment: bug 653601 - aria-selected ignored for aria tabs

add tests and fix indentation

Comment 33

[Trevor Saunders (:tbsaunde)]

Attachment: (wip) tests

first shot at using a event queue

Comment 34

[Trevor Saunders (:tbsaunde)]

Attachment: updated test

I am now passing in what I think should be invokable objects, and got rid of the copied tests.  However m-c doesn't build here in such a way that I can't currently run tests, and haven't been able to unbreak in a couple hours of trying, so Marco would you mind trying to finish the test?  I can try tomorrow if you can't.

Comment 35

[Marco Zehe (:MarcoZ)]

Attachment: Updated WIP

Hm, the code of the patch itself (not the tests) seems to have a severe flaw. If I try to run the test harness, I get a crash after 90 or so entries into mochitest.log, and it only runs for at most 1 second. it does not get anywhere *near* the new file, so I suspect something introduced in this new code causes severe instability.

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to comment #35)
> Created attachment 534448 [details] [diff] [review] [review]
> Updated WIP
> 
> Hm, the code of the patch itself (not the tests) seems to have a severe
> flaw. If I try to run the test harness, I get a crash after 90 or so entries
> into mochitest.log, and it only runs for at most 1 second. it does not get
> anywhere *near* the new file, so I suspect something introduced in this new
> code causes severe instability.

ok, I'm also seeing crashes, the stack I got was eric/nsFrameList.h:69
69        nsFrameList(const nsFrameList& aOther) :
(gdb) bt
#0  0x00007ffff5840e2e in nsFrameList::nsFrameList (this=Cannot access memory at address 0x7fffff7feff8
) at /home/tbsaunde/src/mozilla-central/layout/forms/../generic/nsFrameList.h:69
#1  0x00007ffff5878839 in nsContainerFrame::GetChildList (this=0x7fffd357db30, aListName=0x0)
    at /home/tbsaunde/src/mozilla-central/layout/generic/nsContainerFrame.cpp:302
#2  0x00007ffff579e99c in nsIFrame::GetFirstChild (this=0x7fffd357db30, aListName=0x0)
    at /home/tbsaunde/src/mozilla-central/layout/base/../generic/nsIFrame.h:1050
#3  0x00007ffff581cc82 in nsIPresShell::GetRootScrollFrame (this=0x7fffd3361c00) at /home/tbsaunde/src/mozilla-central/layout/base/nsPresShell.cpp:3372
#4  0x00007ffff581cce8 in nsIPresShell::GetRootScrollFrameAsScrollable (this=0x7fffd3361c00)
    at /home/tbsaunde/src/mozilla-central/layout/base/nsPresShell.cpp:3381
#5  0x00007ffff581f6f3 in PresShell::GetRectVisibility (this=0x7fffd3361c00, aFrame=0x7fffd582d7a0, aRect=..., aMinTwips=720)
    at /home/tbsaunde/src/mozilla-central/layout/base/nsPresShell.cpp:4331
#6  0x00007ffff65b1aa1 in nsAccessible::IsVisible (this=0x7fffd5730c40, aIsOffscreen=0x7fffff7ff3bc)
    at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:655
#7  0x00007ffff65b1ed5 in nsAccessible::NativeState (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:737
#8  0x00007ffff65d7405 in nsHyperTextAccessible::NativeState (this=0x7fffd5730c40)
    at /home/tbsaunde/src/mozilla-central/accessible/src/html/nsHyperTextAccessible.cpp:177
#9  0x00007ffff65b4d9c in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1535
#10 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#11 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#12 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#13 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#14 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#15 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#16 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#17 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#18 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#19 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#20 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#21 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#22 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#23 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545
#24 0x00007ffff65b4e5f in nsAccessible::State (this=0x7fffd5730c40) at /home/tbsaunde/src/mozilla-central/accessible/src/base/nsAccessible.cpp:1545

So, it looks like what happened is that at some point during this patch state & states::FOCUSED  changed to State() & states::FOCUSED which could ofcourse cause infinite recursion, corrected patch will hopefully beready soon

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 534215 [details] [diff] [review]
bug 653601 - aria-selected ignored for aria tabs


> PRUint64
> nsAccessible::State()
> {
>   if (IsDefunct())
>     return states::DEFUNCT;
> 
>   PRUint64 state = NativeState();
>-  // Apply ARIA states to be sure accessible states will be overriden.
>+  // Apply ARIA states to be sure accessible states will be overridden.
>   ApplyARIAState(&state);
> 
>-  if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB) {
>-    if (state & states::FOCUSED) {
>+  if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB &&
>+      !mContent->AttrValueIs(kNameSpaceID_None,
>+                             nsAccessibilityAtoms::aria_selected,
>+                             nsAccessibilityAtoms::_false, eCaseMatters)) {
>+    // Special case: for tabs, focused implies selected, unless explicitly
>+    // false, i.e. aria-selected="false".
>+    if (State() & states::FOCUSED) {

yep, this is recursion as you said

Comment 38

[Trevor Saunders (:tbsaunde)]

Attachment: final patch

passes all tests locally, only change needed since last version was to remove an extra ')'

Comment 39

[alexander :surkov (:asurkov)]

Comment on attachment 534653 [details] [diff] [review]
final patch

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

::: accessible/src/base/nsAccessible.cpp
@@ +1538,5 @@
>  
> +  if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB &&
> +      !mContent->AttrValueIs(kNameSpaceID_None,
> +                             nsAccessibilityAtoms::aria_selected,
> +                             nsAccessibilityAtoms::_false, eCaseMatters)) {

It sounds the aria-selected attribute presence means we shouldn't have any special processing for selected state.

::: accessible/tests/mochitest/states/test_aria_tabs.html
@@ +80,5 @@
> +      // make sure our cache of what currently has focus is correct, which
> +      // we update asyncronously.
> +      gQueue.push(new ariaTabSelected("aria_tab3"));
> +      gQueue.push(new ariaTabNotSelected("aria_tab4"));
> +      gQueue.push(new ariaTabSelected("aria_tab5"));

maybe name it as focusARIATab(aID, aIsSelected)?

btw, I would love to see aria-activedescedant tests for the case David tried to break :)

Comment 40

[Trevor Saunders (:tbsaunde)]

(In reply to comment #39)
> Comment on attachment 534653 [details] [diff] [review] [review]
> final patch
> 
> Review of attachment 534653 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessible.cpp
> @@ +1538,5 @@
> >  
> > +  if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB &&
> > +      !mContent->AttrValueIs(kNameSpaceID_None,
> > +                             nsAccessibilityAtoms::aria_selected,
> > +                             nsAccessibilityAtoms::_false, eCaseMatters)) {
> 
> It sounds the aria-selected attribute presence means we shouldn't have any
> special processing for selected state.

I'm pretty sure I did that at somepoint but I'll fix this again

> 
> ::: accessible/tests/mochitest/states/test_aria_tabs.html
> @@ +80,5 @@
> > +      // make sure our cache of what currently has focus is correct, which
> > +      // we update asyncronously.
> > +      gQueue.push(new ariaTabSelected("aria_tab3"));
> > +      gQueue.push(new ariaTabNotSelected("aria_tab4"));
> > +      gQueue.push(new ariaTabSelected("aria_tab5"));
> 
> maybe name it as focusARIATab(aID, aIsSelected)?
> 
> btw, I would love to see aria-activedescedant tests for the case David tried
> to break :)

sure definetely agree with 2, but given the time and wanting to have something for Yahoo can we leave these for the refactor patch we've discussed?

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)

> > btw, I would love to see aria-activedescedant tests for the case David tried
> > to break :)
> 
> sure definetely agree with 2, but given the time and wanting to have
> something for Yahoo can we leave these for the refactor patch we've
> discussed?

we can forget about that. But it shouldn't take more than 30 minutes and thus it's ok to add test now I think. By doing a little bit more now you save more time in the future.

Comment 42

[Trevor Saunders (:tbsaunde)]

Attachment: next aptch

use true state of aria-selected if present

Comment 43

[alexander :surkov (:asurkov)]

Comment on attachment 534670 [details] [diff] [review]
next aptch

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

::: accessible/src/base/nsAccessible.cpp
@@ +1543,5 @@
> +    // Special case: for tabs, focused implies selected, unless explicitly
> +    // false, i.e. aria-selected="false".
> +    if (mContent->AttrValueIs(kNameSpaceID_None,
> +                              nsAccessibilityAtoms::aria_selected,
> +                              nsAccessibilityAtoms::_true, eCaseMatters) ||

you don't need that because ApplyARIAState did all mappings

::: accessible/tests/mochitest/states/test_aria_tabs.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +
> +<head>
> +  <title>Test tab accessible selected state</title>

ARIA

Comment 44

[Trevor Saunders (:tbsaunde)]

Attachment: YET ANOTHER PATCH

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> Created attachment 534673 [details] [diff] [review] [review]
> YET ANOTHER PATCH

everything looks ok but mochitest, is the next version on the way?

Comment 46

[Trevor Saunders (:tbsaunde)]

Attachment: updated patch

update tests ADDING ACTIVE DECENDENT TESTS

Comment 47

[Marco Zehe (:MarcoZ)]

Comment on attachment 534693 [details] [diff] [review]
updated patch

>+    function activeDecendent(aTabID, aTabListID, aIsSelected)
This should be activeDescendant(...

>+    {
>+      this.tabDOMNode = getNode(aTabID);
>+      this.tabListDOMNode = getNode(aTabListID);

The variable must be this.DOMNode AFAIK. That's the interface, so you should do:

+      this.DOMNode = getNode(aTabListID);

>+      this.invoke = function activeDecendent_invoke()
>+      {
>+        this.tabListDOMNode.focus();
>+      }

No, you must set the properta aria-activedescendant on the tablist node (the this.DOMNode we assigned above, to the passed in tab ID. So this should be:

+      this.invoke = function activeDescendant_invoke()
+      {
+        this.DOMNode.setAttribute("aria-activedescendant", aTabID);
+      }

And then in the checker you do:

+      this.check = function activeDescendant_check(aEvent)
+      {
+        testStates(aTabID, aIsSelected ? STATE_SELECTED : 0, 0,
+                   aIsSelected ? 0 : STATE_SELECTED);
+      }

testStates will automatically get the accessible.

Also of course, down below when checking for the aria-activedescendant checkers, adjust the spelling to activeDescendant as well etc.

Hope this clears up things a bit!

Comment 48

[Marco Zehe (:MarcoZ)]

Attachment: Yet another update, aria-activedescendant in own tablist

Comment 49

[alexander :surkov (:asurkov)]

Comment on attachment 534704 [details] [diff] [review]
Yet another update, aria-activedescendant in own tablist

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

::: accessible/tests/mochitest/states/test_aria_tabs.html
@@ +36,5 @@
> +      }
> +
> +      this.getID = function focusARIATab_getID()
> +      {
> +        return "ARIA Tab selected state on " + prettyName(aID);

please provide more descriptive ID, including the value aIsSelected, like "Focused ARIA tab having aria-selected=" + (aIsSelected ? "true, should" : "false, shouldn't") + " have selected state, ID: " + prettyName(aID);

@@ +40,5 @@
> +        return "ARIA Tab selected state on " + prettyName(aID);
> +      }
> +    }
> +
> +    function tabActiveDescendant(aTabID, aTabListID, aIsSelected)

maybe focus activeDescendantTab?

@@ +46,5 @@
> +      this.DOMNode = getNode(aTabListID);
> +
> +      this.invoke = function tabActiveDescendant_invoke()
> +      {
> +        this.DOMNode.setAttribute("aria-activedescendant", aTabID);

you should focus it, otherwise test doesn't make sense

@@ +57,5 @@
> +      }
> +
> +      this.getID = function tabActiveDescendant_getID()
> +      {
> +        return "ARIA Tab selected state with activeDescendant on " + prettyName(aTabID);

please correct this message as the previous one

@@ +75,5 @@
> +      gQueue = new eventQueue(EVENT_FOCUS);
> +
> +      // simple tabs
> +      testStates("aria_tab1", 0, 0, STATE_SELECTED);
> +      testStates("aria_tab2", STATE_SELECTED);

maybe it makes sense to do that before gQueue =

@@ +85,5 @@
> +      gQueue.push(new focusARIATab("aria_tab4", false));
> +      gQueue.push(new focusARIATab("aria_tab5", true));
> +
> +      // selection through aria-activedescendant
> +      testStates("aria_tab6", STATE_SELECTED);  // initially selected tab

makes sense to put it where these other tests are, btw, you keep tab1/tab2 separately from tab3-tab5 for testing (testStates vs queue tests), but you share tab6/tab7 between tests. I'd be consistent on that.

@@ +119,5 @@
> +  
> +  <!-- test activeDescendant -->
> +  <div id="aria_tablist2" role="tablist" aria-activedescendant="aria_tab6">
> +    <div id="aria_tab6" role="tab" tabindex="0">initially selected tab</div>
> +    <div id="aria_tab7" role="tab" tabindex="0">later selected tab</div>

if items are focusable themselves then active descedant doens't make sense, right?

Comment 50

[Marco Zehe (:MarcoZ)]

(In reply to comment #49)
> @@ +85,5 @@
> > +      gQueue.push(new focusARIATab("aria_tab4", false));
> > +      gQueue.push(new focusARIATab("aria_tab5", true));
> > +
> > +      // selection through aria-activedescendant
> > +      testStates("aria_tab6", STATE_SELECTED);  // initially selected tab
> 
> makes sense to put it where these other tests are, btw, you keep tab1/tab2
> separately from tab3-tab5 for testing (testStates vs queue tests), but you
> share tab6/tab7 between tests. I'd be consistent on that.

That was actually intentional: The first one (aria_tab6) is pre-defined by setting aria-activedescendant in the HTML, whereas the second (aria_tab7) will be focused via changing the attribute.
That is why I tested the states down there before pushing the invoker.

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)
> (In reply to comment #49)
> > @@ +85,5 @@
> > > +      gQueue.push(new focusARIATab("aria_tab4", false));
> > > +      gQueue.push(new focusARIATab("aria_tab5", true));
> > > +
> > > +      // selection through aria-activedescendant
> > > +      testStates("aria_tab6", STATE_SELECTED);  // initially selected tab
> > 
> > makes sense to put it where these other tests are, btw, you keep tab1/tab2
> > separately from tab3-tab5 for testing (testStates vs queue tests), but you
> > share tab6/tab7 between tests. I'd be consistent on that.
> 
> That was actually intentional: The first one (aria_tab6) is pre-defined by
> setting aria-activedescendant in the HTML, whereas the second (aria_tab7)
> will be focused via changing the attribute.
> That is why I tested the states down there before pushing the invoker.

You can do testing and pushing invokers whenever you want, just do that before eventQueue.invoke().

I'm fine if you do all testing on tab6/tab7 but my point was is if you do that then do the same for tab1-tab5. For example, you can test tab3 instead tab1, tab5 instead tab2.

Comment 52

[Marco Zehe (:MarcoZ)]

Attachment: Next round

Comment 53

[alexander :surkov (:asurkov)]

Comment on attachment 534738 [details] [diff] [review]
Next round

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

otherwise is good, r=me

::: accessible/tests/mochitest/states/test_aria_tabs.html
@@ +44,5 @@
> +    }
> +
> +    function focusActiveDescendantTab(aTabID, aTabListID, aIsSelected)
> +    {
> +      this.DOMNode = getNode(aTabListID);

as per irc, should be tab (to handle focus event)

@@ +49,5 @@
> +
> +      this.invoke = function focusActiveDescendantTab_invoke()
> +      {
> +        this.DOMNode.setAttribute("aria-activedescendant", aTabID);
> +        this.DOMNode.focus();

tablist

@@ +91,5 @@
> +      gQueue.push(new focusARIATab("aria_tab4", false));
> +      gQueue.push(new focusARIATab("aria_tab5", true));
> +
> +      // selection through aria-activedescendant
> +      gQueue.push(new focusActiveDescendantTab("aria_tab7", "aria_tablist2", true));

please add a second test where it's focused but aria-selected=false

Comment 54

[Marco Zehe (:MarcoZ)]

Attachment: Final(?) patch

Changed the test once more to address final nits and to check both activedescendant tabs through the queue, first the initial focus, then the traversal from one tab to the next.

Comment 55

[alexander :surkov (:asurkov)]

Comment on attachment 534747 [details] [diff] [review]
Final(?) patch


>+  <!-- test activeDescendant -->
>+  <div id="aria_tablist2" role="tablist" tabindex="0">
>+    <div id="aria_tab6" role="tab">initially selected tab</div>
>+    <div id="aria_tab7" role="tab">later selected tab</div>

can you add please test for 
<div id="aria_tab8" role="tab" aria-selected="false"></div>?

Comment 56

[Marco Zehe (:MarcoZ)]

Landed on all of our behalves on m-c: http://hg.mozilla.org/mozilla-central/rev/6ab5ad925bd8

Comment 57

[AndreiD[QA]]

(In reply to comment #0)
> Tested with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110428
> Firefox/6.0a1

The issue is not reproducible anymore on the latest Firefox 6 Beta build, with the STR in the Description: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Setting this as Verified

Comment 58

[Todd Kloots]

Just tested the changes and they look (sound) great.  Thanks again for fixing this problem guys!