add pseudo SelectAccessible interface

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

similar to bug 589399
Posted patch patchSplinter Review
Attachment #470471 - Flags: review?(marco.zehe)
Attachment #470471 - Flags: review?(bolterbugz)
Attachment #470471 - Flags: approval2.0?
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 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?
items = selectedItems.mRefPtr;
selectedItems.mRefPtr = nsnull;

no addref/release, just grants the pointer
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!
Attachment #470471 - Flags: review?(marco.zehe) → review+
(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 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?
(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
Actually I didn't mean that, but I see we are okay where I was worried. False alarm (3 hrs sleep).
Comment on attachment 470471 [details] [diff] [review]
patch

r+a=me. Nice. I didn't look at the tests... just trusting Marco.
Attachment #470471 - Flags: review?(bolterbugz)
Attachment #470471 - Flags: review+
Attachment #470471 - Flags: approval2.0?
Attachment #470471 - Flags: approval2.0+
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/cbbccc115e55
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 639372
You need to log in before you can comment on or make changes to this bug.