530014 - ARIA single selectable widget should implement nsIAccessibleSelectable

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

the patch should be part of bug 526703.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

add GetSelectableContainer/GetMultiSelectableContainer, they should helpful for bug 414302. Make GetSelectableContainer to rely on nsIAccessibleSelectable interface.

Comment 2

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

Comment on attachment 413538 [details] [diff] [review]
patch

>+already_AddRefed<nsIAccessible>
>+nsAccUtils::GetSelectableContainer(nsIAccessible *aAccessible, PRUint32 aState)
>+{
>+  if (!aAccessible)
>+    return nsnull;
>+
>+  if (!(aState & nsIAccessibleStates::STATE_SELECTABLE))
>+    return nsnull;
>+
>+  nsCOMPtr<nsIAccessibleSelectable> container;
>+  nsCOMPtr<nsIAccessible> parent, accessible(aAccessible);
>+  while (!container) {

Here we expect the container to support nsIAccessibleSelectable. I'm not sure this will help with browser tabs.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> Here we expect the container to support nsIAccessibleSelectable. I'm not sure
> this will help with browser tabs.

Why? nsXULTabsAccessible implements nsIAccessibleSelectable.

Comment 4

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

Comment on attachment 413538 [details] [diff] [review]
patch

r=me then :)

(I only skimmed the tests. I think I like the directory organization)

>+already_AddRefed<nsIAccessible>
>+nsAccUtils::GetSelectableContainer(nsIAccessible *aAccessible, PRUint32 aState)
>+{
>+  if (!aAccessible)
>+    return nsnull;
>+
>+  if (!(aState & nsIAccessibleStates::STATE_SELECTABLE))
>+    return nsnull;
>+
>+  nsCOMPtr<nsIAccessibleSelectable> container;

Tempting to call this variable 'selectable' but I'm happy either way.


>+++ b/accessible/src/base/nsAccessible.cpp

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 413538 [details] [diff] [review])
> r=me then :)
> 
> (I only skimmed the tests. I think I like the directory organization)

So am I :) We started to have too many tests to keep them in one directory.

> >+  if (!(aState & nsIAccessibleStates::STATE_SELECTABLE))
> >+    return nsnull;
> >+
> >+  nsCOMPtr<nsIAccessibleSelectable> container;
> 
> Tempting to call this variable 'selectable' but I'm happy either way.

I like the "container" name more than 'selectable' because "selectable" keyword is associated with selectable container items. It could be 'selectableContainer' but 'container' is shorter.

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 413538 [details] [diff] [review]
patch

These tests look good. However have you tested if this still works if aria-activedescendant is used to point at one specific of those list items as being focused? In other words, could we enhance test_aria_activedescendant to make sure the nsIAccessibleSelectable is exposed correctly there, too?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 413538 [details] [diff] [review])
> These tests look good. However have you tested if this still works if
> aria-activedescendant is used to point at one specific of those list items as
> being focused? In other words, could we enhance test_aria_activedescendant to
> make sure the nsIAccessibleSelectable is exposed correctly there, too?

Please describe how nsIAccessibleSelectable and aria-activedescedant are related. I would bet aria-activedescedant doesn't affect on nsIAccessibleSelectable exposing and it shouldn't depend on nsIAccessibleSelectable at all. What do I miss?

Comment 8

[Marco Zehe (:MarcoZ)]

Sorry, I was mistaken. aria-activedescendant is only about focusable/focused, not about selection. Sorry for the noise!

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 413538 [details] [diff] [review]
patch

r=me.

Comment 10

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8eb40c0a3650