756296 - nsIAccessiblePivot move to coordinate method

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Spatial navigation is important in touch screen devices. Having this live in the pivot would be good. It would be speedy and it could take advantage of the same traversal API.

I am thinking something like
boolean moveToPoint(in nsIAccessibleTraversalRule aRule, in long aCoordX, in long aCoordY);

I'm also thinking that it is important for an observer to know how a pivot was moved. So to
nsIAccessiblePivotObserver.onPivotChanged, I would add:
  void onPivotChanged(in nsIAccessiblePivot aPivot,
                      in nsIAccessible aOldAccessible,
                      in long aOldStart, in long aOldEnd,
                      in PivotChangedReason aReason);

where PivotMovedReason could be:
PIVOT_CHANGED_REASON_ASSIGNED - the position was set directly (i.e. pivot.position = foo)
PIVOT_CHANGED_REASON_MOVED_NEXT - moveNext()
PIVOT_CHANGED_REASON_MOVED_PREV - movePrevious()
PIVOT_CHANGED_REASON_MOVED_FIRST - moveFirst()
PIVOT_CHANGED_REASON_MOVED_LAST - moveLast()
PIVOT_CHANGED_REASON_MOVED_POINT - movePoint()

That might be too high granularity, really I am just interested in assigned, moved in order and moved to point. But if we are doing this, why not...

nsIAccessibleVirtualCursorChangeEvent would get a new equivalent field as well. maybe
nsIAccessibleVirtualCursorChangeEvent.reason

Comment 1

[alexander :surkov (:asurkov)]

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

> I am thinking something like
> boolean moveToPoint(in nsIAccessibleTraversalRule aRule, in long aCoordX, in
> long aCoordY);
> 
> I'm also thinking that it is important for an observer to know how a pivot
> was moved. So to

makes sense to have separate bug on it and get some discussion

Comment 2

[Eitan Isaacson [:eeejay]]

Ok, forget the second half, of the description. I filed bug 756502.

Comment 3

[Trevor Saunders (:tbsaunde)]

so, why can't you just do pivot.position = pivot.root.getDeepestChild(x, y)?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> so, why can't you just do pivot.position = pivot.root.getDeepestChild(x, y)?

I guess to have filtering for free (aRule argument)

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > so, why can't you just do pivot.position = pivot.root.getDeepestChild(x, y)?
> 
> I guess to have filtering for free (aRule argument)

what would filtering even do in this case though? walk up the tree until it finds a match? and return null if the filter never matches?  That seems kind of odd though I guess you could do it

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > so, why can't you just do pivot.position = pivot.root.getDeepestChild(x, y)?
> > 
> > I guess to have filtering for free (aRule argument)
> 
> what would filtering even do in this case though? walk up the tree until it
> finds a match? and return null if the filter never matches?  That seems kind
> of odd though I guess you could do it

It would find the deepest child that matches the rule. And return false if none found, just like the other move methods.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Introduce pivot moveToPoint()

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 756296 - Introduce pivot moveToPoint()

Switched to while instead of do/while.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #4)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > > so, why can't you just do pivot.position = pivot.root.getDeepestChild(x, y)?
> > > 
> > > I guess to have filtering for free (aRule argument)
> > 
> > what would filtering even do in this case though? walk up the tree until it
> > finds a match? and return null if the filter never matches?  That seems kind
> > of odd though I guess you could do it
> 
> It would find the deepest child that matches the rule. And return false if
> none found, just like the other move methods.

so, it occurs to me instead of doing this we could write something more useful and write a nsIAccessible::GetDeepestChildMatching(in nsIAccessibleTraversalRule rule, in long x, in long y);  Its not clear to me that this would be better than GetdeepestChildAt(x, y) and then traversing up the tree yourself, but atleast it wouldn't require the user to already have a pivot at the root they want to search from.  It would also mean we could get rid of this returning true / false and needing to look at how the pivot changes sillyness, since you could just see if GetDeepestMatchAt() returned an accessible.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 627394 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

>--- a/accessible/public/nsIAccessiblePivot.idl
>+++ b/accessible/public/nsIAccessiblePivot.idl
>@@ -106,16 +106,26 @@ interface nsIAccessiblePivot : nsISupports

you need to update its uuid if you change it.

>+  if (mRoot && mRoot->IsDefunct())
>+    return NS_ERROR_NOT_IN_TREE;

not sure what this is supposed to be, afaik mRoot can never be null, and if it was somehow   that wouldn't cause you to return.

>+  while (child && mRoot != child) {

since you know mRoot is not null you only need mRoot != child

>+    // Ignore any matching nodes that were below this one
>+    if (filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE)
>+      match = nsnull;

its kind of weird, since its not really the deepest match, but the deepest match where all accessibles between it and the root are matches.  I'd say this is a good reason to consider doing this traversal in js as the consumer of GetDeepestChild().

I'm clearly not a fan of this idea, but I cann't find anything else that is wrong with the patch for what it is trying to do so over to surkov.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 627394 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +303,5 @@
> +  if (mRoot && mRoot->IsDefunct())
> +    return NS_ERROR_NOT_IN_TREE;
> +
> +  RuleCache cache(aRule);
> +  nsAccessible *match = nsnull;

nit: type* name

@@ +318,5 @@
> +    // Match if no node below this is a match
> +    if ((filtered & nsIAccessibleTraversalRule::FILTER_MATCH) && !match)
> +      match = child;
> +
> +    child = child->Parent();

The point can be out of parent boundaries so basically moveToPoint method can move the pivot to an accessible that doesn't contain the given point. Can you give clear description what the method is supposed to do?

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #11)
> Comment on attachment 627394 [details] [diff] [review]
> Bug 756296 - Introduce pivot moveToPoint()
> 
> Review of attachment 627394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +303,5 @@
> > +  if (mRoot && mRoot->IsDefunct())
> > +    return NS_ERROR_NOT_IN_TREE;
> > +
> > +  RuleCache cache(aRule);
> > +  nsAccessible *match = nsnull;
> 
> nit: type* name
> 
> @@ +318,5 @@
> > +    // Match if no node below this is a match
> > +    if ((filtered & nsIAccessibleTraversalRule::FILTER_MATCH) && !match)
> > +      match = child;
> > +
> > +    child = child->Parent();
> 
> The point can be out of parent boundaries so basically moveToPoint method
> can move the pivot to an accessible that doesn't contain the given point.
> Can you give clear description what the method is supposed to do?

Good point.. We would encounter the same issue if we did top-bottom as well (calling with direct child). Might need to call GetFrameForPoint and do similar steps that nsAccessible::ChildAtPoint does.

The method is supposed to return the deepest matching child for the traversal rule in the pivot root's subtree that contains the given screen coordinates. It should essentially behave the same as getDeepestChild, but on a filtered version of the accessible tree (defined by traversal rule).

Comment 13

[alexander :surkov (:asurkov)]

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

> The method is supposed to return the deepest matching child for the
> traversal rule in the pivot root's subtree that contains the given screen
> coordinates. It should essentially behave the same as getDeepestChild, but
> on a filtered version of the accessible tree (defined by traversal rule).

where the pivot moves if there's no an accessible at the point matching to traversal rule. For example, if I navigate by headings and clicking somewhere where there's no heading. Btw, I'm not sure I see the usability of moveToPoint method in heading case.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> so, it occurs to me instead of doing this we could write something more
> useful and write a nsIAccessible::GetDeepestChildMatching(in
> nsIAccessibleTraversalRule rule, in long x, in long y); 

not sure I like to introduce traversal rule into getChildAtPoint if the unique consumer is a pivot.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 627394 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

canceling review until comments are addressed

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > so, it occurs to me instead of doing this we could write something more
> > useful and write a nsIAccessible::GetDeepestChildMatching(in
> > nsIAccessibleTraversalRule rule, in long x, in long y); 
> 
> not sure I like to introduce traversal rule into getChildAtPoint if the
> unique consumer is a pivot.

Well, note that 1 I'm not necessarily a fan of any part of the idea just trying to make it better, and 2 we wouldn't necessarily need to implement the xpcom method in ChildAtPoint() we could add a new method ChildMatchingRuleAtPoint() or something to implement it.  I'm not sure what it would mean the only consumer is pivots, it might be the only way eeejay chooses to use this method on nsIAccessible is to set a pivots position to its return value, but other people could do what they like.  It would also make testing simpler since we can test which child is retrieved and pivots independantly.

Comment 17

[alexander :surkov (:asurkov)]

Well, the comment is the same: if we don't have consumers other than pivot then I'd keep the logic in the pivot. In my mind traversal rule is something that belongs to pivot so we should have a good reason to take it away from it. Simplified testing is not a reason that should define an API.

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #13)
> (In reply to Eitan Isaacson [:eeejay] from comment #12)
> 
> > The method is supposed to return the deepest matching child for the
> > traversal rule in the pivot root's subtree that contains the given screen
> > coordinates. It should essentially behave the same as getDeepestChild, but
> > on a filtered version of the accessible tree (defined by traversal rule).
> 
> where the pivot moves if there's no an accessible at the point matching to
> traversal rule. For example, if I navigate by headings and clicking
> somewhere where there's no heading.

Then it returns false. Just like a failed move attempt in the other methods.

> Btw, I'm not sure I see the usability of
> moveToPoint method in heading case.

I don't see a traversal rule for headings as a use case either. This will probably be used only with the simple object rule (which is not all that "simple"). This will provide consistency as to what objects are visible to a user. In other words, the user could navigate in a linear fashion (moveBackward, moveForward), and in a spatial fashion while discovering the identical set of objects on the page.

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> Well, the comment is the same: if we don't have consumers other than pivot
> then I'd keep the logic in the pivot. In my mind traversal rule is something
> that belongs to pivot so we should have a good reason to take it away from
> it. Simplified testing is not a reason that should define an API.

Well, I'd say if somebody chooses to combine pivot.SetPosition() and accessible.GetChild() then pivot isn't the consumer, but the code using both of them.

I sort of see the traversal rule is only used with pivots argument, but on the other hand I don't think that's an rgument for putting everything and the kitchen sink on pivots.

simpler testing shouldn't necessarily define the api, but I think it shows that if you write the api that way you get more options for how to use it since the things are seperate blocks instead of one massive thing.

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #17)
> > Well, the comment is the same: if we don't have consumers other than pivot
> > then I'd keep the logic in the pivot. In my mind traversal rule is something
> > that belongs to pivot so we should have a good reason to take it away from
> > it. Simplified testing is not a reason that should define an API.
> 
> Well, I'd say if somebody chooses to combine pivot.SetPosition() and
> accessible.GetChild() then pivot isn't the consumer, but the code using both
> of them.
> 
> I sort of see the traversal rule is only used with pivots argument, but on
> the other hand I don't think that's an rgument for putting everything and
> the kitchen sink on pivots.
> 
> simpler testing shouldn't necessarily define the api, but I think it shows
> that if you write the api that way you get more options for how to use it
> since the things are seperate blocks instead of one massive thing.

I don't think the kitchen sink is in the pivot API. I find the features in it are coherent and within a specific scope. move to point is a natural extension of the linear move methods, nothing more. It takes advantage of the same traversal rule API, the statefulness of the pivot, and the move notification observer pattern. This interface is taking shape nicely, I'm sorry you don't think so.

We made a decision (after endless wiki drafts and bugzilla comments) to integrate traversal in the pivot. I don't see why we need to depart from that decision now, especially without any clear use case in mind. And (arguably) simpler testing is not a use case.

Comment 21

[alexander :surkov (:asurkov)]

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

> > where the pivot moves if there's no an accessible at the point matching to
> > traversal rule. For example, if I navigate by headings and clicking
> > somewhere where there's no heading.
> 
> Then it returns false. Just like a failed move attempt in the other methods.

and pivot doesn't move, correct?

> > Btw, I'm not sure I see the usability of
> > moveToPoint method in heading case.
> 
> I don't see a traversal rule for headings as a use case either. This will
> probably be used only with the simple object rule (which is not all that
> "simple"). This will provide consistency as to what objects are visible to a
> user. 

I guess "simple" rule is more than visible objects? otherwise pivot.position = accessible.getChildAtPoint() is enough.

Comment 22

[alexander :surkov (:asurkov)]

I'm ok if moveToPoint is a part of pivot and I'm not very happy with getChildAtPointMatchingToRule() but I can live with either option.

Comment 23

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #21)
> (In reply to Eitan Isaacson [:eeejay] from comment #18)
> 
> > > where the pivot moves if there's no an accessible at the point matching to
> > > traversal rule. For example, if I navigate by headings and clicking
> > > somewhere where there's no heading.
> > 
> > Then it returns false. Just like a failed move attempt in the other methods.
> 
> and pivot doesn't move, correct?
> 

Correct.

> > > Btw, I'm not sure I see the usability of
> > > moveToPoint method in heading case.
> > 
> > I don't see a traversal rule for headings as a use case either. This will
> > probably be used only with the simple object rule (which is not all that
> > "simple"). This will provide consistency as to what objects are visible to a
> > user. 
> 
> I guess "simple" rule is more than visible objects? otherwise pivot.position
> = accessible.getChildAtPoint() is enough.

In your example, what is "accessible"?

By simple rule, I mean a generic traversal rule (not just headings or just list items, etc.), for example, the rule in AccessFu:
http://dxr.lanedo.com/mozilla-central/accessible/src/jsat/VirtualCursorController.jsm.html#l168

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #23)
> (In reply to alexander :surkov from comment #21)
> > (In reply to Eitan Isaacson [:eeejay] from comment #18)
> > 
> > > > where the pivot moves if there's no an accessible at the point matching to
> > > > traversal rule. For example, if I navigate by headings and clicking
> > > > somewhere where there's no heading.
> > > 
> > > Then it returns false. Just like a failed move attempt in the other methods.
> > 
> > and pivot doesn't move, correct?
> > 
> 
> Correct.
> 
> > > > Btw, I'm not sure I see the usability of
> > > > moveToPoint method in heading case.
> > > 
> > > I don't see a traversal rule for headings as a use case either. This will
> > > probably be used only with the simple object rule (which is not all that
> > > "simple"). This will provide consistency as to what objects are visible to a
> > > user. 
> > 
> > I guess "simple" rule is more than visible objects? otherwise pivot.position
> > = accessible.getChildAtPoint() is enough.
> 
> In your example, what is "accessible"?

it can be a document in conjunction with getDeepestChild

> By simple rule, I mean a generic traversal rule (not just headings or just
> list items, etc.), for example, the rule in AccessFu:
> http://dxr.lanedo.com/mozilla-central/accessible/src/jsat/
> VirtualCursorController.jsm.html#l168

I might read the code wrong, so can you give me an example when
pivot.moveToPoint(simpleRole, x, y) is different from 
var acc = document.getDeepestCHildAt(x, y);
if (acc)
  pivot.position = acc;

if it's equivalent then it makes sense to do that (since nothing else than simple rule isn't supposed to be used with the method).

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> I'm ok if moveToPoint is a part of pivot and I'm not very happy with
> getChildAtPointMatchingToRule() but I can live with either option.

I already thought the related code should die in a fire, so I think I can live with either though I still think adding methods to nsIAccessible is better than special things on pivots.  However I care more about sleep and forgetting this code exists than on arguing the point further.

Comment 26

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #24)
> (In reply to Eitan Isaacson [:eeejay] from comment #23)
> > (In reply to alexander :surkov from comment #21)
> > > (In reply to Eitan Isaacson [:eeejay] from comment #18)
> > > 
> > > > > where the pivot moves if there's no an accessible at the point matching to
> > > > > traversal rule. For example, if I navigate by headings and clicking
> > > > > somewhere where there's no heading.
> > > > 
> > > > Then it returns false. Just like a failed move attempt in the other methods.
> > > 
> > > and pivot doesn't move, correct?
> > > 
> > 
> > Correct.
> > 
> > > > > Btw, I'm not sure I see the usability of
> > > > > moveToPoint method in heading case.
> > > > 
> > > > I don't see a traversal rule for headings as a use case either. This will
> > > > probably be used only with the simple object rule (which is not all that
> > > > "simple"). This will provide consistency as to what objects are visible to a
> > > > user. 
> > > 
> > > I guess "simple" rule is more than visible objects? otherwise pivot.position
> > > = accessible.getChildAtPoint() is enough.
> > 
> > In your example, what is "accessible"?
> 
> it can be a document in conjunction with getDeepestChild
> 
> > By simple rule, I mean a generic traversal rule (not just headings or just
> > list items, etc.), for example, the rule in AccessFu:
> > http://dxr.lanedo.com/mozilla-central/accessible/src/jsat/
> > VirtualCursorController.jsm.html#l168
> 
> I might read the code wrong, so can you give me an example when
> pivot.moveToPoint(simpleRole, x, y)

Will not land on text leafs that are whitespace only.
Will land exactly (and not on child text leafs) of form controls.
Etc.

> var acc = document.getDeepestCHildAt(x, y);
> if (acc)
>   pivot.position = acc;
> 
> if it's equivalent then it makes sense to do that (since nothing else than
> simple rule isn't supposed to be used with the method).

Like I said above, the main advantage is for consistency of what the user sees when navigating linearly and spatially. It should be the exact same tree. If we use the same rule to filter the tree, it will remain consistent. If we let the user navigate linearly with the filtered tree, and navigate spatially by unfiltered leafs, there will be a disparity between the two modes that could be confusing.

Comment 27

[alexander :surkov (:asurkov)]

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

> > I might read the code wrong, so can you give me an example when
> > pivot.moveToPoint(simpleRole, x, y)
> 
> Will not land on text leafs that are whitespace only.
> Will land exactly (and not on child text leafs) of form controls.
> Etc.

ok

> > if it's equivalent then it makes sense to do that (since nothing else than
> > simple rule isn't supposed to be used with the method).
> 
> Like I said above, the main advantage is for consistency of what the user
> sees when navigating linearly and spatially. It should be the exact same
> tree. If we use the same rule to filter the tree, it will remain consistent.
> If we let the user navigate linearly with the filtered tree, and navigate
> spatially by unfiltered leafs, there will be a disparity between the two
> modes that could be confusing.

do you mean if moveToPoint was equivalent to position = getDeepestChildAtPoint(x, y) then it makes sense to introduce that method? but not sure I follow you why you talk about this since they aren't equivalent anyway.

Comment 28

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #27)
> (In reply to Eitan Isaacson [:eeejay] from comment #26)
> 
> > > I might read the code wrong, so can you give me an example when
> > > pivot.moveToPoint(simpleRole, x, y)
> > 
> > Will not land on text leafs that are whitespace only.
> > Will land exactly (and not on child text leafs) of form controls.
> > Etc.
> 
> ok
> 
> > > if it's equivalent then it makes sense to do that (since nothing else than
> > > simple rule isn't supposed to be used with the method).
> > 
> > Like I said above, the main advantage is for consistency of what the user
> > sees when navigating linearly and spatially. It should be the exact same
> > tree. If we use the same rule to filter the tree, it will remain consistent.
> > If we let the user navigate linearly with the filtered tree, and navigate
> > spatially by unfiltered leafs, there will be a disparity between the two
> > modes that could be confusing.
> 
> do you mean if moveToPoint was equivalent to position =
> getDeepestChildAtPoint(x, y) then it makes sense to introduce that method?
> but not sure I follow you why you talk about this since they aren't
> equivalent anyway.

Right, they are not equivalent. I just explained how that would affect the user experience if we did not introduce moveToPoint, and only relied on getDeepestChildAtPoint.

Comment 29

[Eitan Isaacson [:eeejay]]

Attachment: Bug 756296 - Introduce pivot moveToPoint()

Fixed nits, added a check to see if point is still in accessible's bounds. This helps with the false positive of having the deepest child be out of bounds of the actual matching accessible.

It is not perfect since we could still get false negatives when a matching accessible is at the point's coordinates, but the out-of-bounds deepest child of another branch "hijacks" the point. But this seems to be a more acceptable issue than false positives.

Note: After reading Accessible::ChildAtPoint carefully, I am pretty sure that it is vulnerable to the exact same issues. It will return false positives and negatives.

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 631469 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +308,5 @@
> +NS_IMETHODIMP
> +nsAccessiblePivot::MoveToPoint(nsIAccessibleTraversalRule* aRule, PRInt32 aX, PRInt32 aY, bool* aResult)
> +{
> +  NS_ENSURE_ARG(aResult);
> +  NS_ENSURE_ARG(aRule);

we use NS_ENSURE_ARG_POINTER

@@ +323,5 @@
> +    PRUint16 filtered = nsIAccessibleTraversalRule::FILTER_IGNORE;
> +    nsresult rv = cache.ApplyFilter(child, &filtered);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Ignore any matching nodes that were below this one

nit: dot in the end

@@ +325,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Ignore any matching nodes that were below this one
> +    if (filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE)
> +      match = nsnull;

for correctness you should go from top to the match, otherwise you fail if if you have a filter like 'don't go into headings and match everything else' and your match is something inside heading

@@ +329,5 @@
> +      match = nsnull;
> +
> +    // Match if no node below this is a match
> +    if ((filtered & nsIAccessibleTraversalRule::FILTER_MATCH) && !match) {
> +      PRInt32 childX, childY, childWidth, childHeight;

initialize values

Comment 31

[Eitan Isaacson [:eeejay]]

Oops, this slipped my radar, sorry.

(In reply to alexander :surkov from comment #30)
> Comment on attachment 631469 [details] [diff] [review]
> Bug 756296 - Introduce pivot moveToPoint()
> 
> Review of attachment 631469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +325,5 @@
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    // Ignore any matching nodes that were below this one
> > +    if (filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE)
> > +      match = nsnull;
> 
> for correctness you should go from top to the match, otherwise you fail if
> if you have a filter like 'don't go into headings and match everything else'
> and your match is something inside heading
> 

So build a lineage array and then iterate through it from top-down? I am pretty sure what we do here is correct and works in the case you describe.

Comment 32

[Eitan Isaacson [:eeejay]]

Attachment: Bug 756296 - Introduce pivot moveToPoint()

Sorry, the patch got updated in the meantime in my local queue, so I am going to nominate this one for review again. The change here is a new argument aIgnoreNoMatch, which if true, does not make the pivot position null if no match was foind at this point.

This is useful for Android's explore by touch implementation since TalkBack announces explore enters but not exits. And for small web content it could be confusing for a user to hit something, only to lift their finger and have it unset without knowing it.

In B2G we may have something more robust, and not need this. Just like iOS does not.

Comment 33

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

Comment on attachment 633682 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

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

Conditional r+ with nits fits and a nod from Alexander (since I doubt you are going to get a happy dance from Trevor on this one). I was hesitant to r+ a new method that was "interesting" but didn't have many obvious use cases. That said, I think I grok why you did it this way.

Alexander seems okay that this method is in the pivot, but he doesn't like the traversal rule finesse. You guys need to bang that out. First person to acquiesce wins.

I'd like Marco's mobile a11y Ux feedback on possible usage.

::: accessible/public/nsIAccessiblePivot.idl
@@ +129,5 @@
> +   * @param aX             [in]  screen's x coordinate
> +   * @param aY             [in]  screen's y coordinate
> +   * @param aIgnoreNoMatch [in]  don't unset position if no object was found at
> +   *                       point.
> +   * @return true on success, false if the pivot has not been moved.

I'd say "true if moved, false if not". The idea of no match + aIgnoreNoMatch == "success" hurts my puny brain.

@@ +133,5 @@
> +   * @return true on success, false if the pivot has not been moved.
> +   */
> +  boolean moveToPoint(in nsIAccessibleTraversalRule aRule,
> +                      in long aX, in long aY,
> +                      in boolean aIgnoreNoMatch);

Please update the uuid (a community assurance).

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +311,5 @@
> +
> +  *aResult = false;
> +
> +  if (mRoot && mRoot->IsDefunct())
> +    return NS_ERROR_NOT_IN_TREE;

I think Trevor wanted this check to be more like:
if (mRoot->IsDefunct())
Without the mRoot check. Note Trevor I know you want this method to DIAF - no need to reiterate :P

@@ +317,5 @@
> +  RuleCache cache(aRule);
> +  Accessible* match = nsnull;
> +  Accessible* child = mRoot->ChildAtPoint(aX, aY, Accessible::eDeepestChild);
> +  while (child && mRoot != child) {
> +    PRUint16 filtered = nsIAccessibleTraversalRule::FILTER_IGNORE;

The subsequent ApplyFilter call also sets this in the first line of the method. I think setting it here makes it look like an in-param and I'd remove the line.

@@ +334,5 @@
> +      // of bounds. This assures we don't return a false positive.
> +      if (aX >= childX && aX < childX + childWidth &&
> +          aY >= childY && aY < childY + childHeight)
> +        match = child;
> +    }

OK I see you don't return here since we need to worry about a potential ignore-subtree type of ancestor.

Comment 34

[Marco Zehe (:MarcoZ)]

Comment on attachment 633682 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

f=me, since we definitely want the explore by touch to not land on useless whitespace. Comment #26 and comment #32 explain this well.

Comment 35

[Eitan Isaacson [:eeejay]]

Attachment: Bug 756296 - Introduce pivot moveToPoint()

Added mochitests. Started a try run to make sure these tests work on all platforms.
https://tbpl.mozilla.org/?tree=Try&rev=e608fd9f7709

Comment 36

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

Comment on attachment 634668 [details] [diff] [review]
Bug 756296 - Introduce pivot moveToPoint()

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

::: accessible/tests/mochitest/pivot.js
@@ +213,5 @@
> + *                       virtual cursor to land on after performing move method.
> + *                       false if no move is expected.
> + */
> +function moveVCCoordInvoker(aDocAcc, aX, aY, aIgnoreNoMatch,
> +                            aRule, aIdOrNameOrAcc)

I assume you thought about reusing setVCPosInvoker and passing in moveToPoint as aPivotMoveMethod but that it is easier to create a new function that takes the extra args? (This is fine)

Comment 37

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #36)
> Comment on attachment 634668 [details] [diff] [review]
> Bug 756296 - Introduce pivot moveToPoint()
> 
> Review of attachment 634668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/pivot.js
> @@ +213,5 @@
> > + *                       virtual cursor to land on after performing move method.
> > + *                       false if no move is expected.
> > + */
> > +function moveVCCoordInvoker(aDocAcc, aX, aY, aIgnoreNoMatch,
> > +                            aRule, aIdOrNameOrAcc)
> 
> I assume you thought about reusing setVCPosInvoker and passing in
> moveToPoint as aPivotMoveMethod but that it is easier to create a new
> function that takes the extra args? (This is fine)

Yeah, kind of like how we did that for text ranges as well.

Comment 38

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/30a83fad22e5

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30a83fad22e5

Comment 40

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #33)

> Conditional r+ with nits fits and a nod from Alexander (since I doubt you
> are going to get a happy dance from Trevor on this one). I was hesitant to
> r+ a new method that was "interesting" but didn't have many obvious use
> cases. That said, I think I grok why you did it this way.
> 
> Alexander seems okay that this method is in the pivot, but he doesn't like
> the traversal rule finesse. You guys need to bang that out. First person to
> acquiesce wins.

iirc my concern was the method can return incorrect results. The worst thing can happen because of this is the user clicks at thing #1 but thing #2 is clicked. Maybe it doesn't really matter for screen reader users. But since it can affect on "accuracy" of the product so it makes sense to investigate the issue.