590176 - add pseudo SelectAccessible interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

similar to bug 589399

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

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

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

>+  return index >= 0 ? 1 : 0;

Please "return (index >= 0) ? 1: 0;"

(still going... this isn't just renames and code movement)

Comment 3

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

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

>+  nsIMutableArray* items = nsnull;
>+  selectedItems.forget(&items);
>+  return items;
>+}

I'm not familiar with calling forget with an argument. I assume this keeps the reference while nulling out our comptr?

Comment 4

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

I mean the ref count.

Comment 5

[alexander :surkov (:asurkov)]

items = selectedItems.mRefPtr;
selectedItems.mRefPtr = nsnull;

no addref/release, just grants the pointer

Comment 6

[Marco Zehe (:MarcoZ)]

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

>+  // XXX: nsIAccessibleText and nsIAccessibleSelectable both have
>+  // selectionCount property.
>+  //is(acc.selectionCount, aSelectedChildren.length,
>+  //   "selectionCount: wrong selected children count for " + prettyName(aIdentifier));

Do we have a bug filed for this so that it always behaves consistently and we can uncomment this?

Other than that, r=me for the tests, thanks!

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Comment on attachment 470471 [details] [diff] [review]
> patch
> 
> >+  // XXX: nsIAccessibleText and nsIAccessibleSelectable both have
> >+  // selectionCount property.
> >+  //is(acc.selectionCount, aSelectedChildren.length,
> >+  //   "selectionCount: wrong selected children count for " + prettyName(aIdentifier));
> 
> Do we have a bug filed for this so that it always behaves consistently and we
> can uncomment this?

technically we don't but I plan to fix this as part of dexpcom'ing.

Comment 8

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

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

>+++ b/accessible/src/base/nsAccUtils.cpp
>@@ -371,26 +371,21 @@ nsAccessible *
> nsAccUtils::GetSelectableContainer(nsAccessible *aAccessible, PRUint32 aState)
> {
>   if (!aAccessible)
>     return nsnull;
> 
>   if (!(aState & nsIAccessibleStates::STATE_SELECTABLE))
>     return nsnull;
> 
>-  nsCOMPtr<nsIAccessibleSelectable> container;
>-  nsAccessible *parent = aAccessible;
>-  while (!container) {
>-    parent = parent->GetParent();
>-    if (!parent || Role(parent) == nsIAccessibleRole::ROLE_PANE)
>+  nsAccessible* parent = aAccessible;
>+  while ((parent = parent->GetParent()) && !parent->IsSelect()) {
>+    if (Role(parent) == nsIAccessibleRole::ROLE_PANE)
>       return nsnull;

What about null checking parent before accessing it?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Comment on attachment 470471 [details] [diff] [review]

> What about null checking parent before accessing it?

because 

> > nsAccUtils::GetSelectableContainer(nsAccessible *aAccessible, PRUint32 aState)
> > {
> >   if (!aAccessible)
> >     return nsnull;

...

> >+  nsAccessible* parent = aAccessible;

if you mean that

Comment 10

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

Actually I didn't mean that, but I see we are okay where I was worried. False alarm (3 hrs sleep).

Comment 11

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

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

r+a=me. Nice. I didn't look at the tests... just trusting Marco.

Comment 12

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/cbbccc115e55