Closed Bug 530014 Opened 10 years ago Closed 10 years ago

ARIA single selectable widget should implement nsIAccessibleSelectable

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

the patch should be part of bug 526703.
Attached patch patchSplinter Review
add GetSelectableContainer/GetMultiSelectableContainer, they should helpful for bug 414302. Make GetSelectableContainer to rely on nsIAccessibleSelectable interface.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #413538 - Flags: review?(marco.zehe)
Attachment #413538 - Flags: review?(bolterbugz)
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.
(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 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
Attachment #413538 - Flags: review?(bolterbugz) → review+
(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 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?
(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?
Sorry, I was mistaken. aria-activedescendant is only about focusable/focused, not about selection. Sorry for the noise!
Comment on attachment 413538 [details] [diff] [review]
patch

r=me.
Attachment #413538 - Flags: review?(marco.zehe) → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8eb40c0a3650
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.