566551 - ARIA grid and accessible selectable methods shouldn't use GetNextSibling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

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

Nit:
>+  var acc = getAccessible(aIdentifier, [nsIAccessibleSelectable]);
>+  var len = aSelectedChildren.length;

Please bail if acc is null.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 445924 [details] [diff] [review])
> Nit:
> >+  var acc = getAccessible(aIdentifier, [nsIAccessibleSelectable]);
> >+  var len = aSelectedChildren.length;
> 
> Please bail if acc is null.

ok

Comment 4

[[e-mail]]

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

>+nsAccessible*
>+nsAccIterator::GetNext()
>+{
>+  if (!mState)
>+    return nsnull;
>+
>+  nsAccessible *child = mState->mParent->GetChildAt(mState->mIndex++);
>+  if (!child) {
>+    IteratorState *tmp = mState;
>+    mState = mState->mParentState;
>+    delete tmp;
>+
>+    return GetNext();
>+  }
>+
>+  PRBool isComplying = mFilterFunc(child);
>+  if (isComplying)
>+    return child;
>+
>+  if (mIsDeep) {
>+    IteratorState *childState = new IteratorState(child, mState);
>+    mState = childState;
>+  }
>+
>+  return GetNext();
>+}
It might look nice but I'm scared by this "loop" ;-) Would you mind using a while (mState) loop instead?

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Neil's comment is addressed

Comment 6

[alexander :surkov (:asurkov)]

Neil, is the patch ok?

Comment 7

[[e-mail]]

Yeah, that looks OK now thanks!

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 445942 [details] [diff] [review]
patch2

After discussing on IRC, -Alex and I agreed that I do the code review in David's absence.

r=me with nits/requests:
>+  /**
>+   * Return next accessible complying with filter function.
>+   */

Please add a sentence saying that when calling for the first time, returns the first item (so that it is clear there is no extra method for getting the first item).

>+      // The index out of range.

The index *is* out of range.

>+    ++ (*aSelectionCount);

Remove space before ( please.

Comment 9

[alexander :surkov (:asurkov)]

landed on 1.9.3 with addressed Marco's comments - http://hg.mozilla.org/mozilla-central/rev/f646d72f612d