The default bug view has changed. See this FAQ.

aria-selected ignored for ARIA tabs

VERIFIED FIXED in mozilla6

Status

()

Core
Disability Access APIs
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Jamie, Assigned: MarcoZ)

Tracking

({access})

Trunk
mozilla6
x86
Windows 7
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 17 obsolete attachments)

7.48 KB, patch
surkov
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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

6 years ago
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
OK, taking.
Assignee: nobody → bolterbugz

Comment 3

6 years ago
Cool, thanks David.
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?
Attachment #531686 - Flags: review?(surkov.alexander)
Attachment #531686 - Flags: review?(marco.zehe)

Comment 5

6 years ago
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.
Abracadabra!
(Assignee)

Comment 7

6 years ago
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.
Attachment #531686 - Flags: review?(marco.zehe) → review+
Comment on attachment 531686 [details] [diff] [review]
patch 1

Let me make this a bit nicer.
Attachment #531686 - Flags: review?(surkov.alexander)
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.
Attachment #531686 - Attachment is obsolete: true
Attachment #531707 - Flags: review?(surkov.alexander)
Attachment #531707 - Flags: review?(marco.zehe)
(Reporter)

Comment 10

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

Comment 11

6 years ago
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.
Attachment #531707 - Flags: review?(marco.zehe) → review+

Comment 12

6 years ago
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?
(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.
(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.
I wish we had an nsHTMLMap.
(Assignee)

Comment 16

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

Item for Firefox 7 or such!
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?)
Attachment #531707 - Attachment is obsolete: true
Attachment #531707 - Flags: review?(surkov.alexander)
Attachment #531940 - Flags: review?(surkov.alexander)

Comment 18

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

6 years ago
(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 :)
(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.
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.
Assignee: bolterbugz → nobody
(Assignee)

Comment 23

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

Updated

6 years ago
Blocks: 658181
Created attachment 534199 [details] [diff] [review]
(wip) patch

check for the attribute in State() when deciding if to add selected state
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.
Attachment #534206 - Flags: review?(surkov.alexander)

Comment 27

6 years ago
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.
Attachment #534206 - Flags: review?(surkov.alexander) → review-
Created attachment 534211 [details] [diff] [review]
patch5

wrap the whole tab logic in a check for the correct role
Attachment #534211 - Flags: review?(surkov.alexander)

Comment 29

6 years ago
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.
Attachment #534211 - Flags: review?(surkov.alexander) → review-

Updated

6 years ago
Attachment #531940 - Flags: review?(surkov.alexander)
Created attachment 534213 [details] [diff] [review]
patch6

address more nits
Attachment #531940 - Attachment is obsolete: true
Attachment #534199 - Attachment is obsolete: true
Attachment #534206 - Attachment is obsolete: true
Attachment #534211 - Attachment is obsolete: true
Attachment #534213 - Flags: review?(surkov.alexander)

Comment 31

6 years ago
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
Attachment #534213 - Flags: review?(surkov.alexander)
Created attachment 534215 [details] [diff] [review]
bug 653601 - aria-selected ignored for aria tabs

add tests and fix indentation
Attachment #534215 - Flags: review?(surkov.alexander)
Created attachment 534266 [details] [diff] [review]
(wip) tests

first shot at using a event queue
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.
(Assignee)

Comment 35

6 years ago
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.
Attachment #534266 - Attachment is obsolete: true
Attachment #534291 - Attachment is obsolete: true
(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

6 years ago
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
Attachment #534215 - Flags: review?(surkov.alexander)
Created attachment 534653 [details] [diff] [review]
final patch

passes all tests locally, only change needed since last version was to remove an extra ')'
Attachment #534653 - Flags: review?(surkov.alexander)

Comment 39

6 years ago
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 :)
Attachment #534653 - Flags: review?(surkov.alexander)
(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

6 years ago
(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.
Created attachment 534670 [details] [diff] [review]
next aptch

use true state of aria-selected if present
Attachment #534670 - Flags: review?(surkov.alexander)

Comment 43

6 years ago
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
Attachment #534670 - Flags: review?(surkov.alexander) → review-
Created attachment 534673 [details] [diff] [review]
YET ANOTHER PATCH
Attachment #534673 - Flags: review?(surkov.alexander)

Comment 45

6 years ago
(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?
Created attachment 534693 [details] [diff] [review]
updated patch

update tests ADDING ACTIVE DECENDENT TESTS
(Assignee)

Comment 47

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

Comment 48

6 years ago
Created attachment 534704 [details] [diff] [review]
Yet another update, aria-activedescendant in own tablist
Attachment #534213 - Attachment is obsolete: true
Attachment #534215 - Attachment is obsolete: true
Attachment #534448 - Attachment is obsolete: true
Attachment #534653 - Attachment is obsolete: true
Attachment #534670 - Attachment is obsolete: true
Attachment #534673 - Attachment is obsolete: true
Attachment #534693 - Attachment is obsolete: true
Attachment #534673 - Flags: review?(surkov.alexander)
Attachment #534704 - Flags: review?(surkov.alexander)

Comment 49

6 years ago
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?
Attachment #534704 - Flags: review?(surkov.alexander)
(Assignee)

Comment 50

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

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

Comment 52

6 years ago
Created attachment 534738 [details] [diff] [review]
Next round
Attachment #534704 - Attachment is obsolete: true
Attachment #534738 - Flags: review?(surkov.alexander)

Comment 53

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

Comment 54

6 years ago
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.
Attachment #534738 - Attachment is obsolete: true
Attachment #534747 - Flags: review?(surkov.alexander)

Comment 55

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

Comment 56

6 years ago
Landed on all of our behalves on m-c: http://hg.mozilla.org/mozilla-central/rev/6ab5ad925bd8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6

Updated

6 years ago
Assignee: nobody → marco.zehe

Comment 57

6 years ago
(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
Status: RESOLVED → VERIFIED

Comment 58

6 years ago
Just tested the changes and they look (sound) great.  Thanks again for fixing this problem guys!
You need to log in before you can comment on or make changes to this bug.