Sometimes it is desireable to move the pivot from a certain point in the tree that is not necessarily the pivot's current position. For example, to interoperate with focus or caret position. Where we would want the next move to land on something past the focus or caret (or on it).

can you give me a use case, for example, "move to the link next to focus" doesn't seem valid scenario.

(In reply to alexander :surkov from comment #2) > can you give me a use case, for example, "move to the link next to focus" > doesn't seem valid scenario. Why not?

ok, I'm not screen reader user and I dont' see what benefits I get having this. What screen readers do that too?

(In reply to Eitan Isaacson [:eeejay] from comment #3) > (In reply to alexander :surkov from comment #2) > > can you give me a use case, for example, "move to the link next to focus" > > doesn't seem valid scenario. > > Why not? Scenario 1: User clicks "skip to content" link. User navigates through links in content. Scenario 2: When a site is behaving correctly, and it shows/hides elements, it will call focus() on where the user's attention should be. Screen readers use this. When we are navigating with virtual cursor with the generic rule we would want it to land on the element that has recieved focus. The problem is, the element might not conform to the traversal rule (for example links with children are ignored). So we want the vc to be moved to the first node that matches the generic traversal rule after (preorder) the focused node.

The alternative way of doing this now is to set the pivot position and then move it from there, the problem is: 1. you get two moved events instead of one. 2. the moved events' old position is not relevant since you move it twice. 3. you can't apply the rule to the current node (although you could set the position to one node behind it in preorder, but that would be hard and hackish).

I'd say the vc should have an option to follow the focus but I don't mind if you want to extend pivot capabilities to move from arbitrary anchor accessible.

(In reply to alexander :surkov from comment #7) > I'd say the vc should have an option to follow the focus but I don't mind if > you want to extend pivot capabilities to move from arbitrary anchor > accessible. I thought about that, but it could get ugly and cyclical if you want focus to move along with the vc as well. I think this gives more fine-grained control. Maybe in the future when we have it all figured out we could add focus tracking.

Comment on attachment 628192 [details] [diff] [review] Introduce pivot movePreviousFrom and moveNextFrom. Review of attachment 628192 [details] [diff] [review]: ----------------------------------------------------------------- change nsIAccessiblePivot uuid ::: accessible/public/nsIAccessiblePivot.idl @@ +77,5 @@ > + * @return true on success, false if pivot has not been moved. > + */ > + boolean moveNextFrom(in nsIAccessibleTraversalRule aRule, > + in nsIAccessible aStartAccessible, > + in boolean aIncludeStart); I'd say to extend moveNext/movePrev by extra optional arguments, please don't use accessible in arguments name, for example, aStartAccessible -> aAnchor ::: accessible/src/base/nsAccessiblePivot.cpp @@ +200,5 @@ > } > > NS_IMETHODIMP > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > + nsIAccessible *aStartAccessible, type* name @@ +203,5 @@ > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > + nsIAccessible *aStartAccessible, > + bool aIncludeStart, bool* aResult) > +{ > + NS_ENSURE_ARG(aResult); you should initialize aResult @@ +209,5 @@ > + NS_ENSURE_ARG(aStartAccessible); > + > + nsRefPtr<nsAccessible> startAcc = do_QueryObject(aStartAccessible); > + > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) if this method is continuously called while for example the user keeps arrow key pressed then you end up with perf problem @@ +512,5 @@ > PRInt32 aOldStart, PRInt32 aOldEnd) > { > + if (aOldPosition == mPosition && > + aOldStart == mStartOffset && aOldEnd == mEndOffset) > + return; I'd like to see a comment why it happens, the method name doesn't say it's valid scenario so it may confuse a reader

(In reply to alexander :surkov from comment #9) > Comment on attachment 628192 [details] [diff] [review] > Introduce pivot movePreviousFrom and moveNextFrom. > > Review of attachment 628192 [details] [diff] [review]: > ----------------------------------------------------------------- > > change nsIAccessiblePivot uuid > > ::: accessible/public/nsIAccessiblePivot.idl > @@ +77,5 @@ > > + * @return true on success, false if pivot has not been moved. > > + */ > > + boolean moveNextFrom(in nsIAccessibleTraversalRule aRule, > > + in nsIAccessible aStartAccessible, > > + in boolean aIncludeStart); > > I'd say to extend moveNext/movePrev by extra optional arguments, > > please don't use accessible in arguments name, for example, aStartAccessible > -> aAnchor > > ::: accessible/src/base/nsAccessiblePivot.cpp > @@ +200,5 @@ > > } > > > > NS_IMETHODIMP > > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > > + nsIAccessible *aStartAccessible, > > type* name > > @@ +203,5 @@ > > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > > + nsIAccessible *aStartAccessible, > > + bool aIncludeStart, bool* aResult) > > +{ > > + NS_ENSURE_ARG(aResult); > > you should initialize aResult > > @@ +209,5 @@ > > + NS_ENSURE_ARG(aStartAccessible); > > + > > + nsRefPtr<nsAccessible> startAcc = do_QueryObject(aStartAccessible); > > + > > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) > > if this method is continuously called while for example the user keeps arrow > key pressed then you end up with perf problem > > @@ +512,5 @@ > > PRInt32 aOldStart, PRInt32 aOldEnd) > > { > > + if (aOldPosition == mPosition && > > + aOldStart == mStartOffset && aOldEnd == mEndOffset) > > + return; > > I'd like to see a comment why it happens, the method name doesn't say it's > valid scenario so it may confuse a reader

(In reply to alexander :surkov from comment #9) > Comment on attachment 628192 [details] [diff] [review] > Introduce pivot movePreviousFrom and moveNextFrom. > > Review of attachment 628192 [details] [diff] [review]: > ----------------------------------------------------------------- > > change nsIAccessiblePivot uuid > > ::: accessible/public/nsIAccessiblePivot.idl > @@ +77,5 @@ > > + * @return true on success, false if pivot has not been moved. > > + */ > > + boolean moveNextFrom(in nsIAccessibleTraversalRule aRule, > > + in nsIAccessible aStartAccessible, > > + in boolean aIncludeStart); > > I'd say to extend moveNext/movePrev by extra optional arguments, > I'm not in love with the idea of changing the current methods, but I could manage if you feel strongly about it. > please don't use accessible in arguments name, for example, aStartAccessible > -> aAnchor > OK. Anchor is a good word for this. > ::: accessible/src/base/nsAccessiblePivot.cpp > @@ +200,5 @@ > > } > > > > NS_IMETHODIMP > > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > > + nsIAccessible *aStartAccessible, > > type* name > Oops. > @@ +203,5 @@ > > +nsAccessiblePivot::MoveNextFrom(nsIAccessibleTraversalRule* aRule, > > + nsIAccessible *aStartAccessible, > > + bool aIncludeStart, bool* aResult) > > +{ > > + NS_ENSURE_ARG(aResult); > > you should initialize aResult > Oops again. > @@ +209,5 @@ > > + NS_ENSURE_ARG(aStartAccessible); > > + > > + nsRefPtr<nsAccessible> startAcc = do_QueryObject(aStartAccessible); > > + > > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) > > if this method is continuously called while for example the user keeps arrow > key pressed then you end up with perf problem > Why? because of climbing up the hierarchy? It seems like lightweight linked list traversal, especially considering the fact that the next few lines does a heavier tree search. How is it different from SetPosition? What alternatives are there? > @@ +512,5 @@ > > PRInt32 aOldStart, PRInt32 aOldEnd) > > { > > + if (aOldPosition == mPosition && > > + aOldStart == mStartOffset && aOldEnd == mEndOffset) > > + return; > > I'd like to see a comment why it happens, the method name doesn't say it's > valid scenario so it may confuse a reader A comment in the notify method? OK.

(In reply to Eitan Isaacson [:eeejay] from comment #11) > > > + boolean moveNextFrom(in nsIAccessibleTraversalRule aRule, > > > + in nsIAccessible aStartAccessible, > > > + in boolean aIncludeStart); > > > > I'd say to extend moveNext/movePrev by extra optional arguments, > > > > I'm not in love with the idea of changing the current methods, but I could > manage if you feel strongly about it. it's polymorphism, in js you can do pivot.moveNext(rule, anchorAcc) to move from anchor or pivot.moveNext(rule) to move from current pos and it seems to be nice. anyway let's ask Trevor for opinion. I think I can live with either version. > > > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) > > > > if this method is continuously called while for example the user keeps arrow > > key pressed then you end up with perf problem > > > > Why? because of climbing up the hierarchy? It seems like lightweight linked > list traversal, yes. basically it means o(n*m), where n - is number of calls, m is tree length, it's not fast by default. You can hit perf issue depending on ratio between number of calls per second you receive when key is pressed and number of calls per second you can process. if you're not in time to process them then you're in trouble (like user releases the key but you still navigate). > especially considering the fact that the next few lines does > a heavier tree search. while it can be a problem but it's a little different because it doesn't matter how long you keep arrow key pressed because in either way it traverses the tree one time only. > How is it different from SetPosition? What > alternatives are there? I would do optimization for a document since it's your use case and I think actually a primary use case of pivots (I mean vc). > > I'd like to see a comment why it happens, the method name doesn't say it's > > valid scenario so it may confuse a reader > > A comment in the notify method? OK. yes, either comment it or prefix the method name by 'Maybe'.

(In reply to alexander :surkov from comment #12) > (In reply to Eitan Isaacson [:eeejay] from comment #11) > > > > > + boolean moveNextFrom(in nsIAccessibleTraversalRule aRule, > > > > + in nsIAccessible aStartAccessible, > > > > + in boolean aIncludeStart); > > > > > > I'd say to extend moveNext/movePrev by extra optional arguments, > > > > > > > I'm not in love with the idea of changing the current methods, but I could > > manage if you feel strongly about it. > > it's polymorphism, in js you can do > pivot.moveNext(rule, anchorAcc) to move from anchor or pivot.moveNext(rule) > to move from current pos and it seems to be nice. anyway let's ask Trevor > for opinion. I think I can live with either version. > Oh, good point. Since the defaults are null (anchor) and false (match current).

(In reply to alexander :surkov from comment #12) > (In reply to Eitan Isaacson [:eeejay] from comment #11) > > > > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) > > > > > > if this method is continuously called while for example the user keeps arrow > > > key pressed then you end up with perf problem > > > > > > > Why? because of climbing up the hierarchy? It seems like lightweight linked > > list traversal, > > yes. basically it means o(n*m), where n - is number of calls, m is tree > length, it's not fast by default. You can hit perf issue depending on ratio > between number of calls per second you receive when key is pressed and > number of calls per second you can process. if you're not in time to process > them then you're in trouble (like user releases the key but you still > navigate). > > > especially considering the fact that the next few lines does > > a heavier tree search. > > while it can be a problem but it's a little different because it doesn't > matter how long you keep arrow key pressed because in either way it > traverses the tree one time only. > It traverses the tree n times, just like above. Why only once? > > How is it different from SetPosition? What > > alternatives are there? > > I would do optimization for a document since it's your use case and I think > actually a primary use case of pivots (I mean vc). > Are you suggesting getting the root, seeing if it is a document and using IsInDocument if it is, and falling back to IsRootDescendant if not?

(In reply to Eitan Isaacson [:eeejay] from comment #14) > (In reply to alexander :surkov from comment #12) > > (In reply to Eitan Isaacson [:eeejay] from comment #11) > > > > > + if (startAcc->IsDefunct() || !IsRootDescendant(startAcc)) > > > > > > > > if this method is continuously called while for example the user keeps arrow > > > > key pressed then you end up with perf problem > > > > > > > > > > Why? because of climbing up the hierarchy? It seems like lightweight linked > > > list traversal, > > > > yes. basically it means o(n*m), where n - is number of calls, m is tree > > length, it's not fast by default. You can hit perf issue depending on ratio > > between number of calls per second you receive when key is pressed and > > number of calls per second you can process. if you're not in time to process > > them then you're in trouble (like user releases the key but you still > > navigate). > > > > > especially considering the fact that the next few lines does > > > a heavier tree search. > > > > while it can be a problem but it's a little different because it doesn't > > matter how long you keep arrow key pressed because in either way it > > traverses the tree one time only. > > > > It traverses the tree n times, just like above. Why only once? > > > > How is it different from SetPosition? What > > > alternatives are there? > > > > I would do optimization for a document since it's your use case and I think > > actually a primary use case of pivots (I mean vc). > > > > Are you suggesting getting the root, seeing if it is a document and using > IsInDocument if it is, and falling back to IsRootDescendant if not? Also, what if it is an accessible in a child document? We would need to do IsInDocument checks up to the vc's root.

(In reply to Eitan Isaacson [:eeejay] from comment #14) > It traverses the tree n times, just like above. Why only once? each key down moves you one item next until you reach the end of the document, after that you don't move. After all you traversed a tree only once. (In reply to Eitan Isaacson [:eeejay] from comment #15) > > Are you suggesting getting the root, seeing if it is a document and using > > IsInDocument if it is, and falling back to IsRootDescendant if not? > > Also, what if it is an accessible in a child document? We would need to do > IsInDocument checks up to the vc's root. I suggest to optimize IsRootDescendant(). If mRoot is a document then get anchor document and move up by documents chain until you reach the top or mRoot. If mRoot is not a document then do what you do. Since an accessible always has a document until it shutdown then it makes sense to keep IsInDocument check before document chain traversal (again if you don't hit a weird behavior from bug 758884).

Here is an updated patch. I removed the bit in NotifyPivotChanged because I am not sure if that is the desired effect. In bug 758884 I landed a patch that adds a comment about bug 759875. Until that bug is not resolved, we will not optimize with IsInDocument().

Comment on attachment 628509 [details] [diff] [review] Introduce optional arguments in pivot moveNext movePrevious. Review of attachment 628509 [details] [diff] [review]: ----------------------------------------------------------------- I like it, r=me ::: accessible/src/base/nsAccessiblePivot.cpp @@ +190,5 @@ > + *aResult = false; > + > + nsRefPtr<Accessible> anchor = > + (aArgc > 0) ? do_QueryObject(aAnchor) : mPosition; > + if (anchor && (anchor->IsDefunct() || !IsRootDescendant(anchor))) you might want to add XXX comment that you don't need IsRootDescendant in case of mPosition when that bug is fixed @@ +195,5 @@ > return NS_ERROR_NOT_IN_TREE; > > nsresult rv = NS_OK; > + Accessible* accessible = > + SearchForward(anchor, aRule, (aArgc > 1) ? aIncludeStart : false, &rv); so when aAnchor is given as null then you start a search from root. Is that desired behavior?

(In reply to alexander :surkov from comment #18) > Comment on attachment 628509 [details] [diff] [review] > Introduce optional arguments in pivot moveNext movePrevious. > > Review of attachment 628509 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like it, r=me > > ::: accessible/src/base/nsAccessiblePivot.cpp > @@ +190,5 @@ > > + *aResult = false; > > + > > + nsRefPtr<Accessible> anchor = > > + (aArgc > 0) ? do_QueryObject(aAnchor) : mPosition; > > + if (anchor && (anchor->IsDefunct() || !IsRootDescendant(anchor))) > > you might want to add XXX comment that you don't need IsRootDescendant in > case of mPosition when that bug is fixed > I think we will need IsRootDescendant in any case as a sanity check that the position is not detached from the pivot root. It will be optimized with IsInDocument like I said in the comment (and we talked about earlier). > @@ +195,5 @@ > > return NS_ERROR_NOT_IN_TREE; > > > > nsresult rv = NS_OK; > > + Accessible* accessible = > > + SearchForward(anchor, aRule, (aArgc > 1) ? aIncludeStart : false, &rv); > > so when aAnchor is given as null then you start a search from root. Is that > desired behavior? Yup.

(In reply to Eitan Isaacson [:eeejay] from comment #19) > > you might want to add XXX comment that you don't need IsRootDescendant in > > case of mPosition when that bug is fixed > I think we will need IsRootDescendant in any case as a sanity check that the > position is not detached from the pivot root. It will be optimized with > IsInDocument like I said in the comment (and we talked about earlier). I don't see how it will work but ok