Provide actions to set ARIA sort and expanded

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: aaronlev, Assigned: davidb)

Tracking

(Blocks: 1 bug, {access})

unspecified
x86
All
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

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

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
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.
assigning to David
Assignee: nobody → david.bolter
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?
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?
(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
11) as well we need ensure we fire proper state change events and expose right states for aria-expanded
(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
(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.
Created attachment 365892 [details] [diff] [review]
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.
Attachment #365803 - Attachment is obsolete: true
Created attachment 365904 [details] [diff] [review]
WIP3 (click approach)

Throwing this on for safe keeping. Will do the spec way now (just changing the attribute value).
Attachment #365892 - Attachment is obsolete: true
Created attachment 365915 [details] [diff] [review]
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?)
Created attachment 365988 [details] [diff] [review]
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.
Attachment #365915 - Attachment is obsolete: true
Attachment #365988 - Attachment is patch: true
Attachment #365988 - Attachment mime type: application/octet-stream → text/plain
It's interesting idea to expose several actions, I think I like it, though I need to think a bit more
Created attachment 366394 [details] [diff] [review]
WIP3 (uncovered action weirdness?)

I think this is getting close. Requesting review for thoughts.
Attachment #365904 - Attachment is obsolete: true
Attachment #365988 - Attachment is obsolete: true
Attachment #366394 - Flags: review?(surkov.alexander)
Surkov please note my comments in the test file. There is something odd going on.
Note to self: modify where I set aria-sort (use IsSafeToRunScript or nsIRunnable).
Attachment #366394 - Attachment is patch: true
Attachment #366394 - Attachment mime type: application/octet-stream → text/plain
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.
(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.
(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).
(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).
Summary: Provide actions to set ARIA sort, grab and expanded → Provide actions to set ARIA sort and expanded
Spun off bug 482491 for aria-grabbed.
Status: NEW → ASSIGNED
Created attachment 366592 [details] [diff] [review]
patch 1
Attachment #366394 - Attachment is obsolete: true
Attachment #366592 - Flags: review?(marco.zehe)
Attachment #366394 - Flags: review?(surkov.alexander)
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?
Attachment #366592 - Flags: review?(surkov.alexander)
Attachment #366592 - Flags: review?(marco.zehe)
Attachment #366592 - Flags: review+
(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 :)
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 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
Attachment #366592 - Flags: review?(surkov.alexander) → review+
(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.
Hmmm, although hold off on pushing. We might not want to introduce expand/collapse, just in case we end up calling that open/close.
(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.
Created attachment 366821 [details] [diff] [review]
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.
Attachment #366592 - Attachment is obsolete: true
Attachment #366821 - Flags: review?(marco.zehe)
Attachment #366821 - Attachment is patch: true
Attachment #366821 - Attachment mime type: application/octet-stream → text/plain
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?
Attachment #366821 - Flags: review?(marco.zehe) → review+
Created attachment 366827 [details] [diff] [review]
patch to push

Marco nice catches, those were both cruft.
Attachment #366821 - Attachment is obsolete: true
Attachment #366827 - Attachment is patch: true
Attachment #366827 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #366827 - Flags: review?(surkov.alexander)
David, I don't understand how your patch brings this regression. But it must be fixed before landing.
(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.
(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?
Created attachment 367821 [details] [diff] [review]
updated push patch (push 483832 first)
Attachment #366827 - Attachment is obsolete: true
Attachment #367821 - Flags: review?(surkov.alexander)
Attachment #366827 - Flags: review?(surkov.alexander)
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.
Duplicate of this bug: 483832
Created attachment 368611 [details] [diff] [review]
improved patch

nice to hack with Alex at CSUN :)
Attachment #367821 - Attachment is obsolete: true
Attachment #368611 - Flags: review?(surkov.alexander)
Attachment #367821 - Flags: review?(surkov.alexander)
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
Attachment #368611 - Flags: review?(surkov.alexander) → review+
Created attachment 368642 [details] [diff] [review]
patch with new document action test

Agreed :)
Attachment #368611 - Attachment is obsolete: true
Created attachment 368647 [details] [diff] [review]
removed some bogus comments
Attachment #368642 - Attachment is obsolete: true
Attachment #368647 - Flags: review?(surkov.alexander)
Attachment #368647 - Flags: review?(marco.zehe)
Attachment #368647 - Flags: review?(surkov.alexander) → review+

Updated

10 years ago
Attachment #368647 - Flags: review?(marco.zehe) → review+
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.
Created attachment 369113 [details] [diff] [review]
patch (addresses Marco's nit)

Thanks Marco, patch updated.
Attachment #368647 - Attachment is obsolete: true
Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/50940a1eb1e9
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
Oops, got sloppy here (forgot bug 482713).
Depends on: 482713
(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.
Created attachment 371925 [details] [diff] [review]
removed doc tests
Attachment #369113 - Attachment is obsolete: true
Attachment #371925 - Flags: review?(surkov.alexander)
Attachment #371925 - Flags: review?(marco.zehe)
(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 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.
Attachment #371925 - Flags: review?(marco.zehe) → review+
(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?
(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.
(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.
Attachment #371925 - Flags: review?(surkov.alexander) → review+
Comment on attachment 371925 [details] [diff] [review]
removed doc tests

r=me
Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/62f1e1dfb3ee
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 59

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

10 years ago
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.  :)
(In reply to comment #61)

Thanks for checking in. Yes, with the work on bug 474294, FF supports aria-expanded everywhere.
You need to log in before you can comment on or make changes to this bug.