Pivot fails to traverse backwards to previous sibling if current node does not match rule

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 626638 [details]
Test case

Here is a test case that fails in AccessFu. When traversing forwards you go 'WTF', 'Hello', 'World'. When traversing backwards you go 'World', 'WTF'.
(Assignee)

Comment 1

5 years ago
Created attachment 626641 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot.
Attachment #626641 - Flags: review?(surkov.alexander)

Comment 2

5 years ago
Comment on attachment 626641 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot.

Review of attachment 626641 [details] [diff] [review]:
-----------------------------------------------------------------

it seems right but I had some hard time to read that code. Some things:

1) searchCurrent and rv are arguments names
2) 
while (idxInParent > 0) {
  if (!(accessible = parent->GetChildAt(--idxInParent)))
    continue;

is never null since --idxInParent >= 0 (at least after the patch). If I miss something and it can be null and that's expected then it's better to rework this code.

3) assigning under if is really hard to read like in example above or
if (!(accessible = parent))
Attachment #626641 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #2)
> Comment on attachment 626641 [details] [diff] [review]
> Fix backward sibling traversal in nsIAccessiblePivot.
> 
> Review of attachment 626641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it seems right but I had some hard time to read that code. Some things:
> 
> 1) searchCurrent and rv are arguments names
> 2) 
> while (idxInParent > 0) {
>   if (!(accessible = parent->GetChildAt(--idxInParent)))
>     continue;
> 
> is never null since --idxInParent >= 0 (at least after the patch). If I miss
> something and it can be null and that's expected then it's better to rework
> this code.
> 
> 3) assigning under if is really hard to read like in example above or
> if (!(accessible = parent))

I don't think if (!(foo = br)) is any different from while (foo = bar), but its seems like that code should probably use PrevSibling() and of course its not clear to me what the null check that is never true does.
(Assignee)

Comment 4

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > Comment on attachment 626641 [details] [diff] [review]
> > Fix backward sibling traversal in nsIAccessiblePivot.
> > 
> > Review of attachment 626641 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > it seems right but I had some hard time to read that code. Some things:
> > 
> > 1) searchCurrent and rv are arguments names
> > 2) 
> > while (idxInParent > 0) {
> >   if (!(accessible = parent->GetChildAt(--idxInParent)))
> >     continue;
> > 
> > is never null since --idxInParent >= 0 (at least after the patch). If I miss
> > something and it can be null and that's expected then it's better to rework
> > this code.
> > 
> > 3) assigning under if is really hard to read like in example above or
> > if (!(accessible = parent))
> 
> I don't think if (!(foo = br)) is any different from while (foo = bar), but
> its seems like that code should probably use PrevSibling() and of course its
> not clear to me what the null check that is never true does.

From https://bugzilla.mozilla.org/show_bug.cgi?id=698823#c106

@@ +307,5 @@
> +      return aNode;
> +  }
> +
> +  while (aNode != mRootNode) {
> +    while (nsAccessible* previousSibling = aNode->PrevSibling()) {

accessible tree is array oriented, it's preferable to walk by indices
(Assignee)

Comment 5

5 years ago
(In reply to alexander :surkov from comment #2)
> 2) 
> while (idxInParent > 0) {
>   if (!(accessible = parent->GetChildAt(--idxInParent)))
>     continue;
> 
> is never null since --idxInParent >= 0 (at least after the patch). If I miss
> something and it can be null and that's expected then it's better to rework
> this code.

This is inspired by the equally redundant line here (it should be in the debug block):
http://dxr.lanedo.com/mozilla-central/accessible/src/base/nsAccessible.cpp.html#l2665

If a function I call shows "return nsnull", I null check. Even if it is an impossible code path. The fact is that whoever wrote that function entertained the possibility of it returning null is enough.

Not checking it, and hoping that the index in parent is never broken seems wrong to me. Especially since this is a potential crasher.
(Assignee)

Comment 6

5 years ago
Created attachment 626899 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot. r=surkov

Renamed searchCurrent to aSearchCurrent.
Renamed rv to aResult.
Attachment #626641 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f387bc4ee25e
(Assignee)

Updated

5 years ago
Assignee: nobody → eitan
Target Milestone: --- → mozilla15

Comment 8

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > 3) assigning under if is really hard to read like in example above or
> > if (!(accessible = parent))
> 
> I don't think if (!(foo = br)) is any different from while (foo = bar)

I'm ok with while ((assigning_statement)), we use it from time to time but we don't use if ((assignment_statement)) and I don't like it, I don't consider that as a good style. If you wish then for example, you don't like ++idx which is pretty fine with me.

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f387bc4ee25e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.