892607 - Don't traverse pivot inside of ignored subtree.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

The pivot position could be inside of an ignored subtree because:
1. A previous traversal rule did not ignore that subtree.
2. A parent node has changed in a way that now its subtree should be ignored.

When we are in this state, doing a moveNext or movePrevious should not move to a node that is supposed to be pruned by the current rule.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Start pivot searches from outside the ignored subtree.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 774144 [details] [diff] [review]
Start pivot searches from outside the ignored subtree.

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

otherwise it looks ok

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +423,5 @@
> +{
> +  Accessible* matched = aAccessible;
> +  *aResult = aCache.ApplyFilter(aAccessible, aFilterResult);
> +
> +  if (aAccessible != mRoot) {

active root I guess

::: accessible/src/base/nsAccessiblePivot.h
@@ +96,5 @@
> +   */
> +  Accessible* GetSearchStartPosition(Accessible* aAccessible,
> +                                     RuleCache& aCache,
> +                                     uint16_t* aFilterResult,
> +                                     nsresult* aResult);

maybe AdjustStartPosition and I'd like to see extended comment describing why the given anchor may be changed

::: accessible/tests/mochitest/pivot.js
@@ +71,5 @@
>  
>  /**
> + * A checker for virtual cursor changed by assignment events.
> + */
> +function VCSimpleChangedChecker(aDocAcc)

I'd name it as VCChangedReasonNoneChecker or would pass reason as an argument

@@ +207,5 @@
>   * @param aRule            [in] traversal rule object
>   * @param aIdOrNameOrAcc   [in] id, accessible or accessible name to expect
>   *                         virtual cursor to land on after performing move method.
>   *                         false if no move is expected.
> + * @param aInitialPos      [in] initial vc position to set before move.

position setting is kind of movement, doesn't it work if you handle it as two separate queue items

@@ +223,4 @@
>      VCChangedChecker.
>        storePreviousPosAndOffset(aDocAcc.virtualCursor);
>      var moved = aDocAcc.virtualCursor[aPivotMoveMethod](aRule);
> +    SimpleTest.is(!!moved, !!expectMove,

I bet that if !!a == !!b then a == b

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 774144 [details] [diff] [review]
> Start pivot searches from outside the ignored subtree.
> 
> Review of attachment 774144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> otherwise it looks ok
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +423,5 @@
> > +{
> > +  Accessible* matched = aAccessible;
> > +  *aResult = aCache.ApplyFilter(aAccessible, aFilterResult);
> > +
> > +  if (aAccessible != mRoot) {
> 
> active root I guess

Should check for both actually, don't want to drift beyond the pivot root if the position is not in the mModalRoot.

> 
> ::: accessible/src/base/nsAccessiblePivot.h
> @@ +96,5 @@
> > +   */
> > +  Accessible* GetSearchStartPosition(Accessible* aAccessible,
> > +                                     RuleCache& aCache,
> > +                                     uint16_t* aFilterResult,
> > +                                     nsresult* aResult);
> 
> maybe AdjustStartPosition and I'd like to see extended comment describing
> why the given anchor may be changed
> 

Sounds good, done.

> ::: accessible/tests/mochitest/pivot.js
> @@ +71,5 @@
> >  
> >  /**
> > + * A checker for virtual cursor changed by assignment events.
> > + */
> > +function VCSimpleChangedChecker(aDocAcc)
> 
> I'd name it as VCChangedReasonNoneChecker or would pass reason as an argument
> 

Yeah, we could do the latter. Originally it wasn't working for me because I was getting dupe event errors, but now I learned to use the match method in the checker.

> @@ +207,5 @@
> >   * @param aRule            [in] traversal rule object
> >   * @param aIdOrNameOrAcc   [in] id, accessible or accessible name to expect
> >   *                         virtual cursor to land on after performing move method.
> >   *                         false if no move is expected.
> > + * @param aInitialPos      [in] initial vc position to set before move.
> 
> position setting is kind of movement, doesn't it work if you handle it as
> two separate queue items
> 

Yeah, good point. I'll separate it.

> @@ +223,4 @@
> >      VCChangedChecker.
> >        storePreviousPosAndOffset(aDocAcc.virtualCursor);
> >      var moved = aDocAcc.virtualCursor[aPivotMoveMethod](aRule);
> > +    SimpleTest.is(!!moved, !!expectMove,
> 
> I bet that if !!a == !!b then a == b

Not really, a could be true, and b could be 1, or a can be false and b can be "". Double ! is like casting to boolean.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Bug 892607 - Start pivot searches from outside the ignored subtree. r=surkov

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #3)

> > >      VCChangedChecker.
> > >        storePreviousPosAndOffset(aDocAcc.virtualCursor);
> > >      var moved = aDocAcc.virtualCursor[aPivotMoveMethod](aRule);
> > > +    SimpleTest.is(!!moved, !!expectMove,
> > 
> > I bet that if !!a == !!b then a == b
> 
> Not really, a could be true, and b could be 1, or a can be false and b can
> be "". Double ! is like casting to boolean.

a small comment would be nice

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e33505d7b796