473732 - Provide actions to set ARIA sort and expanded

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

According to the ARIA spec, these need to be settable by the AT. When these ARIA properties are present, not "" and not "undefined", we should add an action to toggle them.

Comment 1

[Aaron Leventhal]

Also, not everything supports aria-sort so we should make sure only to set the property on roles that do.

And a reminder: we must update the ARIA implementation guide once we do this.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: early wip

Comment 3

[alexander :surkov (:asurkov)]

assigning to David

Comment 4

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

I see your approach is based on role. I was thinking of an approach that is based on the aria-attribute. So, if the dom node has an aria-sort attribute, then expose an accessible action(s) for it. Thoughts?

Comment 5

[alexander :surkov (:asurkov)]

thoughts

1) there is not aria-grab, only aria-grabbed, I don't know how to deal with
this
2) aria-sort should be applied to columnheader, rowheader
3) defined aria-sort on these objects should be result of proper actions,
4) actions is "ascending" when aria-sort is "descending", action is
"descending" when aria-sort is "ascending"
5) how to deal with "other" value?
6) expanded should be applied to any role?
7) actions are exposed if aria-expanded is defined
8) action is expanded if aria-expanded is expanded, and vice versa.
9) when action is performed then should we change value of proper
aria-attribute?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> I see your approach is based on role. I was thinking of an approach that is
> based on the aria-attribute. So, if the dom node has an aria-sort attribute,
> then expose an accessible action(s) for it. Thoughts?

David, I think aria-expanded should be based on attribute approach, but I'm not sure it's applicable for aria-sort (http://www.w3.org/WAI/PF/aria/#aria-sort)

Forgot
10) aria-sort should be exposed as object attribute

Comment 7

[alexander :surkov (:asurkov)]

11) as well we need ensure we fire proper state change events and expose right states for aria-expanded

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> 9) when action is performed then should we change value of proper
> aria-attribute?

Ok, I see it's true http://www.w3.org/TR/wai-aria-practices/#aria-write

Comment 9

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

(In reply to comment #8)
> (In reply to comment #5)
> 
> > 9) when action is performed then should we change value of proper
> > aria-attribute?
> 
> Ok, I see it's true http://www.w3.org/TR/wai-aria-practices/#aria-write

Right. Here's my initial thinking:

For each of these aria attributes, we expose an action for each possible token
value. For whichever action is performed, we make sure the aria attribute gets
the updated to the new value.

This assumes that that the web app author is responding to attribute mutation.

I think we can forget doing anything useful for sort's "other" unless we watch
for other values and cache them but that seems fragile.

I'll probably spin off some bugs tomorrow after some re-think.

Comment 10

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

Attachment: WIP 2

Surkov, I ended up liking your approach for aria-sort mapping to a click. This won't require web devs to listen to DOM mutation. I simplified the action name to "sort" simply because we have to deal with "other", so this I feel to be a reasonable approach. I guess I should check for "none", but I feel that is a problem in the spec.

Comment 11

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

Attachment: WIP3 (click approach)

Throwing this on for safe keeping. Will do the spec way now (just changing the attribute value).

Comment 12

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

Attachment: WIP1 (ma,sa approach -- see comment)

Different approach.

This is a multiple actions, set attribute, approach, still need to perform the setting of the attribute. Posting in case someone gives feedback while I lunch.  (Do I post WIPs too often?)

Comment 13

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

Attachment: WIP2 (multiple actions)

Still need a few more tests to confirm setting of attribute via action interface. I'm not sure about putting the sorting action enum in nsAriaMap.h. I'm not sure I feel good about nsAccessible.cpp growing. Otherwise the approach seems solid.

Comment 14

[alexander :surkov (:asurkov)]

It's interesting idea to expose several actions, I think I like it, though I need to think a bit more

Comment 15

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

Attachment: WIP3 (uncovered action weirdness?)

I think this is getting close. Requesting review for thoughts.

Comment 16

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

Surkov please note my comments in the test file. There is something odd going on.

Comment 17

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

Note to self: modify where I set aria-sort (use IsSafeToRunScript or nsIRunnable).

Comment 18

[alexander :surkov (:asurkov)]

David, ok, thinking more I didn't find useful idea to expose several actions.

1. I can't imagine UI interface that makes available several actions in the same time. Usually it's one action, either sort-ascending or sort-descending or sort-node or sort-other if you like
2. We should try to not do any assumption what actions are supported.
3. I don't see a reason to expose action "other" because no one won't ever request to invoke this action until he doesn't know what it means.

I would guess real applications can use "ascending" and "descending" actions and "ascending", "descending", "none" (and "other" probably which should be equivalent from users point of view, i.e. there is no information about sort order) sort order values.

I think we could assume if sort order is "ascending" then we could invoke "descending" action and visa versa. I don't think it's correct to do any assumptions when sort order is "none" or "other".

From this point of view it's reasonable to extend ARIA to introduce "aria-sort-actions" attribute - the list of supported sort actions. In this case we shouldn't do any assumptions and expose values supported by widget author. Note, in this case we could expose several actions in the same time.

In the meantime it sounds to be correct to expose click action if aria-sort is presented. We could set ascending and descending values if the current value is descending or ascending (an assumption but it sounds to be reasonable).

I think we should discuss this stuff with ARIA working group.

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #18)
> 1. I can't imagine UI interface that makes available several actions in the
> same time. Usually it's one action, either sort-ascending or sort-descending or
> sort-node or sort-other if you like

For this usecase I agree. There was a discussion in the IA2 group a while back whether an image map should expose several actions, with each action index corresponding to each of the map's link children, so the general idea of having more than one action exposed is not bad, I am not sure however that this particular bug is a usecase for that. Unless I'm missing something.

Comment 20

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

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

Thanks for your comments, and I will discuss it in the WAI-ARIA groups. 

Use case: Imagine a sighted user who uses speech recognition. If we expose all these actions for sort it would be easily possible through platform AT to allow her to say something like "sort column 3 descending".

Alexander, aria-sort-actions is the kind of thing they want to worry about in "ARIA 2.0".

Hmmm, "other" is probably not a good value to use to drive behaviour. This is probably a value that web devs will use for describing any ordering (and there may be more than one) that is not ascending or descending. Since this leaves us with "ascending" and "descending" I'll go for the single action approach afterall.

Note, we will probably at some point need a way of aggregating multiple actions, for example, for a node that has both an aria-grabbed, and aria-sort. (Spun off as bug 482459).

Comment 21

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

(In reply to comment #18)

> In the meantime it sounds to be correct to expose click action if aria-sort is
> presented. We could set ascending and descending values if the current value is
> descending or ascending (an assumption but it sounds to be reasonable).

I think if we perform a click, we should not set the attribute ourselves because the web dev probably already handles the click (and sets aria-sort accordingly).

So in the end this aria-sort becomes trivial (for ARIA 1.0).

Comment 22

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

Spun off bug 482491 for aria-grabbed.

Comment 23

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

Attachment: patch 1

Comment 24

[Marco Zehe (:MarcoZ)]

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

r=me for the tests. Question: Do we want to expose the current sorting state "ascending" or "descending" as an object attribute so the screen reader knows what the current sort state is?

Comment 25

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

(In reply to comment #24)
> (From update of attachment 366592 [details] [diff] [review])
> r=me for the tests. Question: Do we want to expose the current sorting state
> "ascending" or "descending" as an object attribute so the screen reader knows
> what the current sort state is?

Yep, and we do :)

Comment 26

[alexander :surkov (:asurkov)]

David and Marco, I really realize sometimes several actions might be good and very useful in the case if UI doesn't have access to them. Though it may be information dupes in the case when UI has access to them. But actually here I care about assumption we do and I happy to see the last patch without any assumptions :)

Comment 27

[alexander :surkov (:asurkov)]

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


>+   case eExpandAction:
>+     // tempting to merge with eOpenCloseAction, but we should be consistent
>+     // with the aria attribute name.

we expose expand/collapse actions for xul tree as well, I don't think we should merge them with open/close actions which are exposed for comboboxes because AT seems to deal well with them. So I don't find this comment useful and correct.

>   // Get an action based on ARIA role.
>   if (mRoleMapEntry)
>     return mRoleMapEntry->actionRule;
>+  
>+  if (nsAccUtils::HasDefinedARIAToken(content, 
>+                                      nsAccessibilityAtoms::aria_expanded))
>+    return eExpandAction;

we won't use aria-expanded actions event if role doesn't provide default actions. I would suggest to try aria-exapnded if there is no default actions reserved for role.

with these fixed r=me

Comment 28

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

(In reply to comment #27)
> (From update of attachment 366592 [details] [diff] [review])
> 
> >+   case eExpandAction:
> >+     // tempting to merge with eOpenCloseAction, but we should be consistent
> >+     // with the aria attribute name.
> 
> we expose expand/collapse actions for xul tree as well, I don't think we should
> merge them with open/close actions which are exposed for comboboxes because AT
> seems to deal well with them. So I don't find this comment useful and correct.

Yeah, I can remove this comment, but see below:

> 
> >   // Get an action based on ARIA role.
> >   if (mRoleMapEntry)
> >     return mRoleMapEntry->actionRule;
> >+  
> >+  if (nsAccUtils::HasDefinedARIAToken(content, 
> >+                                      nsAccessibilityAtoms::aria_expanded))
> >+    return eExpandAction;
> 
> we won't use aria-expanded actions event if role doesn't provide default
> actions. I would suggest to try aria-exapnded if there is no default actions
> reserved for role.
>

OK I just tried moving the check above the role mapping, but this breaks the "combobox_expanded" test as it expects "open" or "close". I can see a few ways of fixing, but I think the right fix is probably merging eExpandAction and eOpenCloseAction... at least for comboboxes. This is a bit of a pickle. I'll probably have to engage the vendors on this one.

I propose we commit my patch as is, as it is an improvement, and open a bug about the open/close expand/collapse dichotomy.

> with these fixed r=me

Thanks for your review.

Comment 29

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

Hmmm, although hold off on pushing. We might not want to introduce expand/collapse, just in case we end up calling that open/close.

Comment 30

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

(In reply to comment #27)

> we won't use aria-expanded actions event if role doesn't provide default
> actions. I would suggest to try aria-exapnded if there is no default actions
> reserved for role.

I understand now. Patch coming.

Comment 31

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

Attachment: patch 2 (see comment)

Alexander, this patch addresses your comments thanks. Marco, please note I filed spin off bug 482713 because this patch seems to uncover a bug.

Comment 32

[Marco Zehe (:MarcoZ)]

Comment on attachment 366821 [details] [diff] [review]
patch 2 (see comment)

>-        // No actions wanted on doc
>-        is(docAcc.numActions, 0, "Wrong number of actions for document!");
>+        // No actions wanted on doc        

Nit: This only adds unnecessary whitespace. I'll remove this on checkin unless you submit another patch.

>+    <div id="sortable" role="columnheader" aria-sort="ascending">
>+      Columnheader
>+    </div>
>+    <div id="nosortable" role="columnheader">
>+      Columnheader
>+    </div>

You're not testing the second columnheader at all. Why is ti in here, or do you plan on testing for an empty action name?

Comment 33

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

Attachment: patch to push

Marco nice catches, those were both cruft.

Comment 34

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

Comment on attachment 366827 [details] [diff] [review]
patch to push

Alexander, can you take one last look at this, and please note the todo test. It is worrisome.

Comment 35

[alexander :surkov (:asurkov)]

David, I don't understand how your patch brings this regression. But it must be fixed before landing.

Comment 36

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

(In reply to comment #35)
> David, I don't understand how your patch brings this regression. But it must be
> fixed before landing.

My patch exposes a need to strengthen HasDefinedARIAToken (filed dependency bug
483832 for this). nsIAccessibleDocument doesn't support nsIContent AFAIK.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> (In reply to comment #35)
> > David, I don't understand how your patch brings this regression. But it must be
> > fixed before landing.
> 
> My patch exposes a need to strengthen HasDefinedARIAToken (filed dependency bug
> 483832 for this). nsIAccessibleDocument doesn't support nsIContent AFAIK.

David, is it more correct to use GetRoleContent where it's appropriate?

Comment 38

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

Attachment: updated push patch (push 483832 first)

Comment 39

[alexander :surkov (:asurkov)]

David, I think we should pass GetRoleContent into GetActionRule because if html:body has onclick or ARIA role having action then we would like to expose it on document.

Comment 41

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

Attachment: improved patch

nice to hack with Alex at CSUN :)

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 368611 [details] [diff] [review]
improved patch

(In reply to comment #41)
> Created an attachment (id=368611) [details]
> improved patch
> 
> nice to hack with Alex at CSUN :)

me too, with you :)

>+  NS_ASSERTION(aContent != nsnull,
>+               "aContent is null in call to HasDefinedARIAToken!");
>+

NS_ASSERTION(aContent, "") is enough I guess.

Once you change GetActionRule to use GetContentRole please add mochitest to check action name on document, I guess html:body with onlcick attribute will be good.

r=me

Comment 43

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

Attachment: patch with new document action test

Agreed :)

Comment 44

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

Attachment: removed some bogus comments

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 368647 [details] [diff] [review]
removed some bogus comments

>+      var docAcc = getAccessible(document, [nsIAccessibleDocument]);
>+      
>+      ok(docAcc, "Could not QI an nsIAccessibleDocument!")

Nit: This is unnecessary, since getAccessible already prints out an error message if either the accessible retrieval or the QI to the wanted interfaces fails.

Instead, you should do an

if (!docAcc) {
  simpleTest.finish();
  return;
}

and then continue:

>+      is(docAcc.numActions, 1, "Wrong number of actions for document!");
>+      is(docAcc.getActionName(0), "click", "Wrong action name for document!");
>+      
>+      SimpleTest.finish();
>+    }

r=me with that change, unless you had a very specific reason to do it the way you did.

Comment 46

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

Attachment: patch (addresses Marco's nit)

Thanks Marco, patch updated.

Comment 47

[Marco Zehe (:MarcoZ)]

Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/50940a1eb1e9

Comment 48

[Boris Zbarsky [:bzbarsky]]

Backed out, because it caused consistent crashes on the Linux unit test box.  Every single cycle after this went in has been orange with this stack:

Crash reason:  SIGSEGV
Crash address: 0x409da226

Thread 0 (crashed)
 0  libxul.so!nsAccUtils::HasDefinedARIAToken(nsIContent*, nsIAtom*) [nsAccUtils.cpp:50940a1eb1e9 : 326 + 0x0]
    eip = 0x409da226   esp = 0xbf8d7ca0   ebp = 0xbf8d7cbc   ebx = 0x40dd55d0
    esi = 0x00000000   edi = 0x421ad364   eax = 0x00000000   ecx = 0xbf8d7ce4
    edx = 0x469473a0   efl = 0x00210202
 1  libxul.so!nsAccessible::GetActionRule(unsigned int) [nsAccessible.cpp:50940a1eb1e9 : 3262 + 0xf]
    eip = 0x409e357a   esp = 0xbf8d7cc4   ebp = 0xbf8d7cfc
 2  libxul.so!nsAccessible::GetNumActions(unsigned char*) [nsAccessible.cpp:50940a1eb1e9 : 2279 + 0xa]
    eip = 0x409e35e5   esp = 0xbf8d7d04   ebp = 0xbf8d7d2c
 3  libxul.so!nsAccessibleWrap::CreateMaiInterfaces() [nsAccessibleWrap.cpp:50940a1eb1e9 : 428 + 0x9]
    eip = 0x40a05742   esp = 0xbf8d7d34   ebp = 0xbf8d7dac
 4  libxul.so!nsAccessibleWrap::GetNativeInterface(void**) [nsAccessibleWrap.cpp:50940a1eb1e9 : 381 + 0x8]
    eip = 0x40a05f4c   esp = 0xbf8d7db4   ebp = 0xbf8d7ddc
 5  libxul.so!nsAccessibleWrap::GetAtkObject(nsIAccessible*) [nsAccessibleWrap.cpp:50940a1eb1e9 : 411 + 0x9]
    eip = 0x40a0510d   esp = 0xbf8d7de4   ebp = 0xbf8d7e0c
 6  libxul.so!nsAccessibleWrap::FirePlatformEvent(nsIAccessibleEvent*) [nsAccessibleWrap.cpp:50940a1eb1e9 : 1149 + 0xa]
    eip = 0x40a068d8   esp = 0xbf8d7e14   ebp = 0xbf8d7e6c
 7  libxul.so!nsAccessibleWrap::FireAccessibleEvent(nsIAccessibleEvent*) [nsAccessibleWrap.cpp:50940a1eb1e9 : 1135 + 0xb]
    eip = 0x40a050d9   esp = 0xbf8d7e74   ebp = 0xbf8d7e8c
 8  libxul.so!nsDocAccessible::FireDocLoadEvents(unsigned int) [nsDocAccessible.cpp:50940a1eb1e9 : 912 + 0x10]
    eip = 0x409d4572   esp = 0xbf8d7e94   ebp = 0xbf8d7eec
 9  libxul.so!nsAccessibilityService::ProcessDocLoadEvent(nsITimer*, void*, unsigned int) [nsAccessibilityService.cpp:50940a1eb1e9 : 246 + 0xc]
    eip = 0x409de350   esp = 0xbf8d7ef4   ebp = 0xbf8d7f4c
10  libxul.so!nsAccessibilityService::EndLoadCallback(nsITimer*, void*) [nsAccessibilityService.cpp:50940a1eb1e9 : 269 + 0x10]
    eip = 0x409dca08   esp = 0xbf8d7f54   ebp = 0xbf8d7f6c
11  libxul.so!nsTimerImpl::Fire() [nsTimerImpl.cpp:50940a1eb1e9 : 427 + 0x7]
    eip = 0x40a8a74f   esp = 0xbf8d7f74   ebp = 0xbf8d7f9c
12  libxul.so!nsTimerEvent::Run() [nsTimerImpl.cpp:50940a1eb1e9 : 519 + 0x8]
    eip = 0x40a8ae95   esp = 0xbf8d7fa4   ebp = 0xbf8d7fbc
13  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:50940a1eb1e9 : 510 + 0x5]
    eip = 0x40a877a9   esp = 0xbf8d7fc4   ebp = 0xbf8d7ffc
14  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
    eip = 0x40a5512f   esp = 0xbf8d8004   ebp = 0xbf8d802c
15  libxul.so!nsThread::Shutdown() [nsThread.cpp:50940a1eb1e9 : 465 + 0xb]
    eip = 0x40a879bf   esp = 0xbf8d8034   ebp = 0xbf8d806c
16  libxul.so!NS_GetXPTCallStub_P + 0x30
    eip = 0x40a94d0b   esp = 0xbf8d8074   ebp = 0xbf8d8088
17  libxul.so!nsProxyObjectCallInfo::Run() [nsProxyEvent.cpp:50940a1eb1e9 : 181 + 0x13]
    eip = 0x40a8ca5d   esp = 0xbf8d8090   ebp = 0xbf8d80a8
18  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:50940a1eb1e9 : 510 + 0x5]
    eip = 0x40a877a9   esp = 0xbf8d80b0   ebp = 0xbf8d80e8
19  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
    eip = 0x40a5512f   esp = 0xbf8d80f0   ebp = 0xbf8d8118
20  libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp:50940a1eb1e9 : 170 + 0x9]
    eip = 0x409acc4e   esp = 0xbf8d8120   ebp = 0xbf8d8138
21  libxul.so!nsAppStartup::Run() [nsAppStartup.cpp:50940a1eb1e9 : 192 + 0x5]
    eip = 0x40873402   esp = 0xbf8d8140   ebp = 0xbf8d8158
22  libxul.so!XRE_main [nsAppRunner.cpp:50940a1eb1e9 : 3340 + 0x7]
    eip = 0x401b8729   esp = 0xbf8d8160   ebp = 0xbf8d8788
23  firefox-bin!main [nsBrowserApp.cpp:50940a1eb1e9 : 156 + 0xe]
    eip = 0x080495b1   esp = 0xbf8d8790   ebp = 0xbf8d87e8
24  libc-2.5.so + 0x15deb
    eip = 0x00aa6dec   esp = 0xbf8d87f0   ebp = 0xbf8d8858

At a guess, based on registers and line number, aContent is null in frame 0.

Comment 49

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

Oops, got sloppy here (forgot bug 482713).

Comment 50

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

(In reply to comment #42)
> Once you change GetActionRule to use GetContentRole please add mochitest to
> check action name on document, I guess html:body with onlcick attribute will be
> good.
> 
> r=me

Surkov, adding tests for document actions is orthogonal to this bug and has being really problematic. I think we should add the document actions tests separately; perhaps on bug 482713. I'll work up a patch that addresses just this bug.

Comment 51

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

Attachment: removed doc tests

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #50)

> Surkov, adding tests for document actions is orthogonal to this bug and has
> being really problematic. I think we should add the document actions tests
> separately; perhaps on bug 482713. I'll work up a patch that addresses just
> this bug.

David, we don't avoid the problem if we remove the tests covering the problem. I'm ok with the patch with doc tests disabled if this problem exists now, i.e. if your patch doesn't introduce this problem.

Comment 53

[Marco Zehe (:MarcoZ)]

Comment on attachment 371925 [details] [diff] [review]
removed doc tests

>diff --git a/accessible/tests/mochitest/test_nsIAccessibleDocument.html b/accessible/tests/mochitest/test_nsIAccessibleDocument.html
>
>     function doTest()
>     {
>-      // Get accessible for body tag.
>       var docAcc = getAccessible(document, [nsIAccessibleDocument]);

Why did you remove this comment? Is this an unintentional change?

r=me with that answered/fixed.

Comment 54

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

(In reply to comment #52)
> (In reply to comment #50)
> 
> > Surkov, adding tests for document actions is orthogonal to this bug and has
> > being really problematic. I think we should add the document actions tests
> > separately; perhaps on bug 482713. I'll work up a patch that addresses just
> > this bug.
> 
> David, we don't avoid the problem if we remove the tests covering the problem.
> I'm ok with the patch with doc tests disabled if this problem exists now, i.e.
> if your patch doesn't introduce this problem.

My patch doesn't introduce the problem, and I have the new doc action test sitting as a patch on bug 482713. I'd really prefer that test to go in with the patch that fixes it if you are okay with that?

Comment 55

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

(In reply to comment #53)
> (From update of attachment 371925 [details] [diff] [review])
> >diff --git a/accessible/tests/mochitest/test_nsIAccessibleDocument.html b/accessible/tests/mochitest/test_nsIAccessibleDocument.html
> >
> >     function doTest()
> >     {
> >-      // Get accessible for body tag.
> >       var docAcc = getAccessible(document, [nsIAccessibleDocument]);
> 
> Why did you remove this comment? Is this an unintentional change?
> 

Yes it is an intentional removal based on a chat I had with Alex, which I no longer recall very well. The comment should probably have said "Get nsIAccessibleDocument for this document" which wouldn't add any useful information beyond the actual line of code.

> r=me with that answered/fixed.

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)

> > >-      // Get accessible for body tag.
> > >       var docAcc = getAccessible(document, [nsIAccessibleDocument]);
> > 
> > Why did you remove this comment? Is this an unintentional change?
> > 
> 
> Yes it is an intentional removal based on a chat I had with Alex, which I no
> longer recall very well. The comment should probably have said "Get
> nsIAccessibleDocument for this document" which wouldn't add any useful
> information beyond the actual line of code.


This talk was probably on CSUN in persons :). I guess I said this comment doesn't make sense because technically body tag is not accessible and we create here document accessible which is obvious. So no comment is needed I think.

Comment 57

[alexander :surkov (:asurkov)]

Comment on attachment 371925 [details] [diff] [review]
removed doc tests

r=me

Comment 58

[Marco Zehe (:MarcoZ)]

Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/62f1e1dfb3ee

Comment 59

[Martin Kliehm]

Just as an addendum, I wonder if you considered this use case: quite often there are accordion menus on websites. A link or a button toggles an element to display:none or display:block

<!-- Control button for accordion menu -->
<p id="control">
    <a aria-controls="list" role="button">Show list</a>
</p>
<!-- Accordion menu -->
<ul id="list" role="region" aria-expanded="false" >
    <li><a href="#1">Lorem</a></li>
    <li><a href="#2">Ipsum</a></li>
    <li><a href="#3">Dolor</a></li>
    <li><a href="#4">Sit amet</a></li>
</ul>

Comment 60

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

Martin, could you explain what an accordion menu is? I want to be sure I understand what the interaction is.

Here are examples of accordions I am familiar with:
http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/layout/test_AccordionContainer.html
http://docs.jquery.com/UI/Accordion

Comment 61

[Martin Kliehm]

David, you are right, this is not an accordion *menu* in the narrow sense. It's more a button / control element that opens and closes another region.

There could be just one or several. It is often used to hide large blocks of information, for example in a FAQ. Here is a (bad table-layout without ARIA) example at the German Railways website:

http://www.bahn.de/international/view/en/home/info/ticket_booking.shtml

At the bottom of the page are gray areas titled "Fahrkartenshop", "Ticket vending machines" etc. When you click on an area (or tab to it and use enter, space, or an arrow key), the associated information opens.

The attribute best describing the state of the information areas would be "aria-expanded", and the relationship is defined through "aria-controls" (perhaps also "aria-labelledby"). So I would suggest supporting it in such use cases as well. I'm not sure if the way "aria-expanded" is currently implemented supports a wide range of uses or just in the narrow context of a classical menu, so I rather mention it that authors are likely to use it that way.  :)

Comment 62

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

(In reply to comment #61)

Thanks for checking in. Yes, with the work on bug 474294, FF supports aria-expanded everywhere.