Last Comment Bug 653601 - aria-selected ignored for ARIA tabs
: aria-selected ignored for ARIA tabs
Status: VERIFIED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla6
Assigned To: Marco Zehe (:MarcoZ)
:
: alexander :surkov
Mentors:
http://public.yahoo.com/kloots/aria/t...
Depends on:
Blocks: 2011ymaila11y
  Show dependency treegraph
 
Reported: 2011-04-28 16:32 PDT by James Teh [:Jamie]
Modified: 2011-07-27 22:58 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (5.73 KB, patch)
2011-05-11 11:10 PDT, David Bolter [:davidb]
mzehe: review+
Details | Diff | Splinter Review
patch 2 (6.38 KB, patch)
2011-05-11 12:10 PDT, David Bolter [:davidb]
mzehe: review+
Details | Diff | Splinter Review
patch 3 (12.25 KB, patch)
2011-05-12 08:36 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
(wip) patch (1.12 KB, patch)
2011-05-20 23:02 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch4 (2.67 KB, patch)
2011-05-21 02:29 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
patch5 (3.39 KB, patch)
2011-05-21 02:50 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
patch6 (2.36 KB, patch)
2011-05-21 03:15 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 653601 - aria-selected ignored for aria tabs (6.02 KB, patch)
2011-05-21 03:34 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
(wip) tests (8.44 KB, patch)
2011-05-21 19:38 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
updated test (5.77 KB, patch)
2011-05-22 05:53 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
Updated WIP (6.50 KB, patch)
2011-05-23 09:00 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
final patch (5.56 KB, patch)
2011-05-23 18:59 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
next aptch (5.79 KB, patch)
2011-05-23 21:25 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
YET ANOTHER PATCH (5.61 KB, patch)
2011-05-23 21:49 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
updated patch (6.07 KB, patch)
2011-05-24 01:15 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
Yet another update, aria-activedescendant in own tablist (7.21 KB, patch)
2011-05-24 03:18 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
Next round (7.46 KB, patch)
2011-05-24 05:37 PDT, Marco Zehe (:MarcoZ)
surkov.alexander: review+
Details | Diff | Splinter Review
Final(?) patch (7.48 KB, patch)
2011-05-24 06:26 PDT, Marco Zehe (:MarcoZ)
surkov.alexander: review+
Details | Diff | Splinter Review

Description James Teh [:Jamie] 2011-04-28 16:32:11 PDT
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 2011-05-11 08:31:11 PDT
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] 2011-05-11 08:46:23 PDT
OK, taking.
Comment 3 Todd Kloots 2011-05-11 08:53:01 PDT
Cool, thanks David.
Comment 4 David Bolter [:davidb] 2011-05-11 11:10:09 PDT
Created attachment 531686 [details] [diff] [review]
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 2011-05-11 11:15:32 PDT
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] 2011-05-11 11:34:30 PDT
Abracadabra!
Comment 7 Marco Zehe (:MarcoZ) 2011-05-11 11:36:47 PDT
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] 2011-05-11 11:37:03 PDT
Comment on attachment 531686 [details] [diff] [review]
patch 1

Let me make this a bit nicer.
Comment 9 David Bolter [:davidb] 2011-05-11 12:10:08 PDT
Created attachment 531707 [details] [diff] [review]
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] 2011-05-11 13:39:46 PDT
(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) 2011-05-11 22:52:00 PDT
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 2011-05-12 02:32:59 PDT
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] 2011-05-12 05:25:36 PDT
(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] 2011-05-12 05:33:32 PDT
(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] 2011-05-12 05:42:34 PDT
I wish we had an nsHTMLMap.
Comment 16 Marco Zehe (:MarcoZ) 2011-05-12 05:52:39 PDT
(In reply to comment #15)
> I wish we had an nsHTMLMap.

Item for Firefox 7 or such!
Comment 17 David Bolter [:davidb] 2011-05-12 08:36:19 PDT
Created attachment 531940 [details] [diff] [review]
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 2011-05-12 09:12:37 PDT
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] 2011-05-12 10:31:19 PDT
(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 2011-05-13 02:42:32 PDT
(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] 2011-05-13 06:35:22 PDT
(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] 2011-05-16 06:17:40 PDT
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) 2011-05-17 06:15:57 PDT
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] 2011-05-17 11:28:07 PDT
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/dbolter@mozilla.com-63787dbdbae6/
(our tests don't break)
Comment 25 Trevor Saunders (:tbsaunde) 2011-05-20 23:02:14 PDT
Created attachment 534199 [details] [diff] [review]
(wip) patch

check for the attribute in State() when deciding if to add selected state
Comment 26 Trevor Saunders (:tbsaunde) 2011-05-21 02:29:06 PDT
Created attachment 534206 [details] [diff] [review]
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 2011-05-21 02:34:20 PDT
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) 2011-05-21 02:50:11 PDT
Created attachment 534211 [details] [diff] [review]
patch5

wrap the whole tab logic in a check for the correct role
Comment 29 alexander :surkov 2011-05-21 02:57:29 PDT
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) 2011-05-21 03:15:01 PDT
Created attachment 534213 [details] [diff] [review]
patch6

address more nits
Comment 31 alexander :surkov 2011-05-21 03:20:30 PDT
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) 2011-05-21 03:34:29 PDT
Created attachment 534215 [details] [diff] [review]
bug 653601 - aria-selected ignored for aria tabs

add tests and fix indentation
Comment 33 Trevor Saunders (:tbsaunde) 2011-05-21 19:38:07 PDT
Created attachment 534266 [details] [diff] [review]
(wip) tests

first shot at using a event queue
Comment 34 Trevor Saunders (:tbsaunde) 2011-05-22 05:53:39 PDT
Created attachment 534291 [details] [diff] [review]
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) 2011-05-23 09:00:10 PDT
Created attachment 534448 [details] [diff] [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.
Comment 36 Trevor Saunders (:tbsaunde) 2011-05-23 16:00:43 PDT
(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 2011-05-23 18:17:13 PDT
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) 2011-05-23 18:59:55 PDT
Created attachment 534653 [details] [diff] [review]
final patch

passes all tests locally, only change needed since last version was to remove an extra ')'
Comment 39 alexander :surkov 2011-05-23 19:11:51 PDT
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) 2011-05-23 20:55:27 PDT
(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 2011-05-23 21:19:10 PDT
(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) 2011-05-23 21:25:12 PDT
Created attachment 534670 [details] [diff] [review]
next aptch

use true state of aria-selected if present
Comment 43 alexander :surkov 2011-05-23 21:30:12 PDT
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) 2011-05-23 21:49:56 PDT
Created attachment 534673 [details] [diff] [review]
YET ANOTHER PATCH
Comment 45 alexander :surkov 2011-05-24 01:00:35 PDT
(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) 2011-05-24 01:15:24 PDT
Created attachment 534693 [details] [diff] [review]
updated patch

update tests ADDING ACTIVE DECENDENT TESTS
Comment 47 Marco Zehe (:MarcoZ) 2011-05-24 01:48:34 PDT
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) 2011-05-24 03:18:43 PDT
Created attachment 534704 [details] [diff] [review]
Yet another update, aria-activedescendant in own tablist
Comment 49 alexander :surkov 2011-05-24 03:40:00 PDT
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) 2011-05-24 04:46:26 PDT
(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 2011-05-24 05:19:36 PDT
(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) 2011-05-24 05:37:34 PDT
Created attachment 534738 [details] [diff] [review]
Next round
Comment 53 alexander :surkov 2011-05-24 06:00:28 PDT
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) 2011-05-24 06:26:32 PDT
Created attachment 534747 [details] [diff] [review]
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 2011-05-24 06:36:59 PDT
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) 2011-05-24 07:06:31 PDT
Landed on all of our behalves on m-c: http://hg.mozilla.org/mozilla-central/rev/6ab5ad925bd8
Comment 57 AndreiD[QA] 2011-07-27 04:30:43 PDT
(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 2011-07-27 22:58:53 PDT
Just tested the changes and they look (sound) great.  Thanks again for fixing this problem guys!

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