698823 - Introduce virtual cursor/soft focus functionality to a11y API

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

We need an API for object navigation in the document. This is required for "thin" accessibility APIs like the one in Android that don't have support for hierarchies and the screen reader does not have fancy navigation modes built in.

Comment 1

[Eitan Isaacson [:eeejay]]

Here is a rough sketch:
https://wiki.mozilla.org/Accessibility/Traversal

Comment 2

[Eitan Isaacson [:eeejay]]

OK. Split the issue into two. One being the actual traversal and another being "soft focus".

I simplified the traversal proposal, got rid of the singleton crap and the directional navigation.
https://wiki.mozilla.org/Accessibility/Traversal

I would like to propose this API for retaining a pseudo-focus state in the document:
https://wiki.mozilla.org/Accessibility/SoftFocus

The latter is more urgent. The former could be done client-side and in JS in the meantime.

Comment 3

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

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

> https://wiki.mozilla.org/Accessibility/SoftFocus

Exciting stuff!

Comment 4

[alexander :surkov (:asurkov)]

We need a place where we can keep discussion, maybe another wiki page is more suitable for that than bugzilla (or event this bug because it's summary doesn't make me think it's about virtual cursors) or inline comments on existing wiki.

Anyway my concerns are:
# it's not exposed outside the Gecko so do not mix existing nsIAccessible techniques exposed to AT APIs and nsIAccessible additions you introduce
# pivot per document is not we want (at least that should be considered carefully), because pivot is user oriented not DOM oriented
# provide usecase for soft selections
# I don't like softFocus and softSelection terms, we have too many focus terms to introduce another one and honestly it's confusing however I should say it's not devoid of charm
# until pivot concept has visual presentation or exposed through AT APIs and has any semantics it's just a point in accessible tree but everybody gets notified the point was changed. It sounds pretty useless.

Comment 5

[Eitan Isaacson [:eeejay]]

Ok, sorry of all that spam. Unloaded the tree walking API to bug 703284. We could discuss the Soft Focus bug here exclusively. Alex, I think this is the best forum (not on the wiki). When I amend the proposal on the wiki I will provide meaningful change messages.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to alexander surkov from comment #4)
> We need a place where we can keep discussion, maybe another wiki page is
> more suitable for that than bugzilla (or event this bug because it's summary
> doesn't make me think it's about virtual cursors) or inline comments on
> existing wiki.
> 
> Anyway my concerns are:
> # it's not exposed outside the Gecko so do not mix existing nsIAccessible
> techniques exposed to AT APIs and nsIAccessible additions you introduce

What would be an alternative? Where would it live? It needs to be scriptable. Also, this may one day land in one platform API or another. The Android wrapper will be using this immediately, and would emit Android focus events on internal soft focus events.

> # pivot per document is not we want (at least that should be considered
> carefully), because pivot is user oriented not DOM oriented

That is exactly the point of this feature! The idea is to retain a state that is exclusive to our layer and is independent of any kind of DOM state.

> # provide usecase for soft selections

line/word/character navigation.

> # I don't like softFocus and softSelection terms, we have too many focus
> terms to introduce another one and honestly it's confusing however I should
> say it's not devoid of charm

Yes, it has charm. Where else do we use focus? I could think of only one other kind of focus. I find the use of the word "cursor" in virtual cursor to be even more vague. Although I understand that the term has been used in this context already. The "soft" keyword is unique to this feature and it is very telling of the expected behavior, IMHO.

> # until pivot concept has visual presentation or exposed through AT APIs and
> has any semantics it's just a point in accessible tree but everybody gets
> notified the point was changed. It sounds pretty useless.

I'm thinking of this as model-view-controller. Where this is the model. If there is no view or controller present, this feature is dormant. It is up to the controller to add semantics to this and drive it. And it is up to the view to present it appropriately, either directly in Gecko or through the platform API.

In the Android case, we will be emitting Android focus events when we get soft focus events. And the chrome JS will have basic object traversal bound to the d-pad. This JS will only be used when accessibility is enabled.

Comment 7

[Eitan Isaacson [:eeejay]]

Also, as we introduce this feature, it would be cool to have an XUL element (or stack child) in browser.xul that has display:none by default. It could display a soft focus ring for pivot 0.

Comment 8

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> > # pivot per document is not we want (at least that should be considered
> > carefully), because pivot is user oriented not DOM oriented
> That is exactly the point of this feature! The idea is to retain a state
> that is exclusive to our layer and is independent of any kind of DOM state.
What about XUL/browser chrome? (I don't quite understand what Gecko calls a "document".) Also, sub-documents (iframes, etc.) probably shouldn't have their own pivots.

> Yes, [soft focus/selection] has charm. Where else do we use focus? I could think of only one
> other kind of focus.
> The "soft" keyword is unique to this feature and it is
> very telling of the expected behavior, IMHO.
It might be argued that soft is not specific enough. Perhaps it might be worth coming up with a term to encompass this whole "virtual navigation" thing. Something like a11y navigator; e.g. a11yNavigatorFocus. Obviously, that's a bit ugly, but just some food for thought.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #8)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> > > # pivot per document is not we want (at least that should be considered
> > > carefully), because pivot is user oriented not DOM oriented
> > That is exactly the point of this feature! The idea is to retain a state
> > that is exclusive to our layer and is independent of any kind of DOM state.
> What about XUL/browser chrome? (I don't quite understand what Gecko calls a
> "document".) Also, sub-documents (iframes, etc.) probably shouldn't have
> their own pivots.
> 

Good Q. I think as a rule browser chrome should not have this. In Android we don't really have XUL for one, and in Linux/Windows the chrome bits really look and behave like native widgets, so the screen reader should just treat them as such. In the B2G case this will need to be re-examined since there won't be any external screen reader in the picture.

I think only the top-level non-chrome document should manage soft focus. This would also mean that each tab would have its own soft focus as well.

> > Yes, [soft focus/selection] has charm. Where else do we use focus? I could think of only one
> > other kind of focus.
> > The "soft" keyword is unique to this feature and it is
> > very telling of the expected behavior, IMHO.
> It might be argued that soft is not specific enough. Perhaps it might be
> worth coming up with a term to encompass this whole "virtual navigation"
> thing. Something like a11y navigator; e.g. a11yNavigatorFocus. Obviously,
> that's a bit ugly, but just some food for thought.

I feel like "navigator" in the context of a web browser is confusing. Ever since Netscape Navigator came out. And that name still seems to have meaning in the Mozilla codebase, it is the object that manages forward/back/history, etc. But it keeps coming up in my mind too, so perhaps...

Comment 10

[alexander :surkov (:asurkov)]

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

> I think only the top-level non-chrome document should manage soft focus.
> This would also mean that each tab would have its own soft focus as well.

that address a point from my 2nd item of comment #4

Comment 11

[alexander :surkov (:asurkov)]

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

> > # it's not exposed outside the Gecko so do not mix existing nsIAccessible
> > techniques exposed to AT APIs and nsIAccessible additions you introduce
> 
> What would be an alternative? Where would it live? It needs to be
> scriptable. Also, this may one day land in one platform API or another. The
> Android wrapper will be using this immediately, and would emit Android focus
> events on internal soft focus events.

Alternative to not mix things. We can't fire state change events for unknown state. You can hack hardly like this state change event coming from pivot stuffs so don't fire it to AT but that makes implementation tricky.

> > # provide usecase for soft selections
> 
> line/word/character navigation.

why doesn't DOM selection/caret navigation mode suites here?

> > # until pivot concept has visual presentation or exposed through AT APIs and
> > has any semantics it's just a point in accessible tree but everybody gets
> > notified the point was changed. It sounds pretty useless.
> 
> I'm thinking of this as model-view-controller. Where this is the model. If
> there is no view or controller present, this feature is dormant. It is up to
> the controller to add semantics to this and drive it. And it is up to the
> view to present it appropriately, either directly in Gecko or through the
> platform API.

no semantics no sense to use standard mechanism since no one consumer has idea how to use that, for pivots providing no semantics I'd suggest to make the pivot to register observers. So no view or controller doesn't mean the feature is dormant since it's exposed through Gecko accessibility API.

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to alexander surkov from comment #10)
> (In reply to Eitan Isaacson [:eeejay] from comment #9)
> 
> > I think only the top-level non-chrome document should manage soft focus.
> > This would also mean that each tab would have its own soft focus as well.
> 
> that address a point from my 2nd item of comment #4

Right :) we discussed this in IRC.

Comment 13

[Eitan Isaacson [:eeejay]]

Excuse the radio silence. I am trying to incorporate the initial design and your suggestions into code so I could see what works and what doesn't.

(In reply to alexander surkov from comment #11)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> 
> > > # it's not exposed outside the Gecko so do not mix existing nsIAccessible
> > > techniques exposed to AT APIs and nsIAccessible additions you introduce
> > 
> > What would be an alternative? Where would it live? It needs to be
> > scriptable. Also, this may one day land in one platform API or another. The
> > Android wrapper will be using this immediately, and would emit Android focus
> > events on internal soft focus events.
> 
> Alternative to not mix things. We can't fire state change events for unknown
> state. You can hack hardly like this state change event coming from pivot
> stuffs so don't fire it to AT but that makes implementation tricky.

Yes, I have been implementing this. And I find that we really need a dedicated event for soft focus. One reason is what you say here, another being the fact that the event's consumer needs to know what pivot the event is for.

> 
> > > # provide usecase for soft selections
> > 
> > line/word/character navigation.
> 
> why doesn't DOM selection/caret navigation mode suites here?
> 
> > > # until pivot concept has visual presentation or exposed through AT APIs and
> > > has any semantics it's just a point in accessible tree but everybody gets
> > > notified the point was changed. It sounds pretty useless.
> > 
> > I'm thinking of this as model-view-controller. Where this is the model. If
> > there is no view or controller present, this feature is dormant. It is up to
> > the controller to add semantics to this and drive it. And it is up to the
> > view to present it appropriately, either directly in Gecko or through the
> > platform API.
> 
> no semantics no sense to use standard mechanism since no one consumer has
> idea how to use that, for pivots providing no semantics I'd suggest to make
> the pivot to register observers. So no view or controller doesn't mean the
> feature is dormant since it's exposed through Gecko accessibility API.

Not sure what you mean here, I'll try to catch you later to understand. I think that pivot index 0 should be by convention the "primary" pivot: it gets a focus ring, and it scrolls to view.

Comment 14

[alexander :surkov (:asurkov)]

You mix traversal pure API stuffs and semantically rich things. Until pivot provides semantics they are pure API things and shouldn't be mapped to existing APIs. If primary pivot or all pivots are used as virtual cursors then do as you do and provide them some semantics, allow customization if you want. But since virtual cursor is sort of unique then you should think of what means to have several pivots and how ATs and users can manage them.

So you should have accessible point or accessible range (refer to DOM point and DOM range concepts) (basically that's your pivots) and ability to move the accessible point or change accessible range by given criteria (for example heading navigation) (that's your traversal apis). Let's call this a virtual point. But it isn't supposed to be mapped to existing APIs. Provide it by observers mechanism if you want to watch it.

Develop virtual cursor concept which has visual representation (platform dependent, customization allowed) and has semantics. Map it to existing APIs or suggest existing APIs extension. Virtual cursor implementation is based on virtual points. Think about having few virtual cursors for same document or/and changing virtual point objects so virtual cursor(s) can be managed.

Comment 15

[Eitan Isaacson [:eeejay]]

OK, I talked to Alex this morning, and I think (hope) I understood his input. Since "soft focus" never caught, it will now be named "virtual cursor". Check out the wiki page for a new API[1]. I think these changes alleviate the dilemma we had earlier with nested documents, since the state is not accessed through the accessible child anymore. Essentially the controller should take responsibility as to which document's virtual cursor it is accessing, and only use the sensible one (for example, top-level tab document), so XUL chrome navigation is possible if it is necessary.

1. https://wiki.mozilla.org/Accessibility/SoftFocus

Comment 16

[alexander :surkov (:asurkov)]

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

> 1. https://wiki.mozilla.org/Accessibility/SoftFocus

it's still unclear, you should describe each interfaces, methods

for example, the doc makes me think than any document can create own pivot and allowed to have own virtual cursor pivot. However this contradicts to comment #12.

or it's not clear what event mechanism is going to be used for pivots notifications. If accessible events are supposed to be used then my comment #14 is not addressed.

or it's not clear what means the setting the not text accessible (setAccessible) and offsets (setStartOffset, setEndOffset).

Comment 17

[Eitan Isaacson [:eeejay]]

(In reply to alexander surkov from comment #16)
> (In reply to Eitan Isaacson [:eeejay] from comment #15)
> 
> > 1. https://wiki.mozilla.org/Accessibility/SoftFocus
> 
> it's still unclear, you should describe each interfaces, methods

I added docstrings.

> 
> for example, the doc makes me think than any document can create own pivot
> and allowed to have own virtual cursor pivot. However this contradicts to
> comment #12.

This is what I addressed in comment #15. I don't think it is an antipattern to have any document manage its own pivots and virtual cursor. In the previous API that behavior would simply break things since it's "soft focus" state was set via the child accessible, and it would be ambiguous as to which document that soft focus state would be associated with.

In this version, nothing utterly breaks if each document has its own virtual cursor. For example, a user could use the chrome document's virtual cursor and eventually "activate" a content document, and use the virtual cursor of that document. That would allow both chrome navigation and document navigation while saving the state in multiple tabs. It is up to the controller and view to control/display the correct virtual cursor.

> 
> or it's not clear what event mechanism is going to be used for pivots
> notifications. If accessible events are supposed to be used then my comment
> #14 is not addressed.

Yeah, the current draft uses accessible events fired with the document as the target. This doesn't have to be this way, but I just am not clear on how to implement it otherwise. For example, is it OK to create a new nsIObserverService topic?

I think it would be good to have pivots be standalone and not tied to specific documents (but still have root accessibles defined that you can't traverse above), both in instantiation (ie. get rid of document.createVirtualPivot) and in events (ie. have new event interface with the pivot being the target).

> 
> or it's not clear what means the setting the not text accessible
> (setAccessible) and offsets (setStartOffset, setEndOffset).

I added documentation on that. An exception is thrown.

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #17)
> I think it would be good to have pivots be standalone and not tied to
> specific documents (but still have root accessibles defined that you can't
> traverse above), both in instantiation (ie. get rid of
> document.createVirtualPivot) and in events (ie. have new event interface
> with the pivot being the target).

If we could pull this off, the only amendment to current interfaces would be nsIAccessibleDocument.virtualCursor.

Comment 19

[Eitan Isaacson [:eeejay]]

OK. Changed https://wiki.mozilla.org/Accessibility/SoftFocus

I implemented this too. Not ready for review, but here are the patches anyway.

Comment 20

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

Where are they? :)

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: Added nsIAccessiblePivot interface and implementation.

Comment 22

[Eitan Isaacson [:eeejay]]

Attachment: Add factory method for pivots in nsIAccessibleRetrieval & implement

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Add virtualCursor pivot to nsIAccessibleDocument & implement

Comment 24

[alexander :surkov (:asurkov)]

I still don't see harmonic behind proposal. Let me think aloud.

Pivot is just a point in accessible tree (may be a range) that allows anyone interested to be notified. Without notification mechanism is a useless thing.

Pivot usecase is virtual cursor, which is basically a point or a range having visual presentation. Virtual cursor concept is presented in many screen readers and I heard ideas to introduce this concept in ARIA 2. So I think virtual cursor implemented on browser side is something that can happen in nearest future. That means it should be flexible and useful (incorporate traversal functionality) and reuse existing APIs (use API specific callback mechanism, for example, accessible events).

Also we talked about the idea to have more than one virtual cursor. Idea of active virtual cursor or introduction of state of the virtual cursor (which can be saved and restored) sounds good. But we need to figure out how to workaround possible collisions when multiple ATs manage the virtual cursor.

Idea to have a virtual cursor per DOM document doesn't sound clear since it's not evident how AT can navigate between documents. I agree chrome traversal is optional but nice. So the on one hand we should support optional documents and should provide integrity between nested DOM documents.

If we don't have other usecases of pivots then I suggest to keep them as pure programmatic interface used for virtual cursor implementation and polish them after virtual cursor.

Comment 25

[Eitan Isaacson [:eeejay]]

(In reply to alexander surkov from comment #24)
> I still don't see harmonic behind proposal. Let me think aloud.
> 
> Pivot is just a point in accessible tree (may be a range) that allows anyone
> interested to be notified. Without notification mechanism is a useless thing.
> 

The pivot is observable via nsIAccessiblePivotObserver. I did this because I though that is how you believed it should be done, as opposed to being yet another accessible event that is not mapped in platform APIs.

> Pivot usecase is virtual cursor, which is basically a point or a range
> having visual presentation. Virtual cursor concept is presented in many
> screen readers and I heard ideas to introduce this concept in ARIA 2. 

Really? Is this discussion happening anywhere online?

> So I think virtual cursor implemented on browser side is something that can
> happen in nearest future. That means it should be flexible and useful
> (incorporate traversal functionality) and reuse existing APIs (use API
> specific callback mechanism, for example, accessible events).
> 

I don't know what you mean by traversal functionality, if you mean something similar to DOM tree walking, I think it should be distinct. We have a bug for that bug 703284.

> Also we talked about the idea to have more than one virtual cursor. Idea of
> active virtual cursor or introduction of state of the virtual cursor (which
> can be saved and restored) sounds good. 

This is where the generic pivot API comes into place. An AT could use them in addition to the document's virtual cursor and tie new display and control semantics to them.

> But we need to figure out how to
> workaround possible collisions when multiple ATs manage the virtual cursor.
> 

There is only one "virtual cursor", that is the whole point of having it in the first place. It is the one canonical focal point where the user's attention is at that very moment. Its meaning is objective, and it does not change from one AT to the next. Just like the text caret position means the same thing to the user no matter what AT they are accessing it with.

The virtual cursor itself does not hold the "traversal functionality", ie. it does not have a next() method. It is the AT's responsibility to traverse and place the cursor on the next spot. So if two ATs are being used interchangeably, one hops to links and another to headers, they will still work as expected.

> Idea to have a virtual cursor per DOM document doesn't sound clear since
> it's not evident how AT can navigate between documents. I agree chrome
> traversal is optional but nice. So the on one hand we should support
> optional documents and should provide integrity between nested DOM documents.
> 

I think we could move forward without having the answers for all of that. By convention, we would simply care about the top-level content documents for now, and not about chrome or content sub-documents.

The questions for the future are about how to control and view those nested cursors, not really about this model API. For example, how do you step in and out of chrome? How do we not confuse the user with more than one cursor being presented?

> If we don't have other usecases of pivots then I suggest to keep them as
> pure programmatic interface used for virtual cursor implementation and
> polish them after virtual cursor.

The only change would be dropping nsIAccessibleRetrieval.createAccessiblePivot().

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #25)
> (In reply to alexander surkov from comment #24)
> > I still don't see harmonic behind proposal. Let me think aloud.
> > 
> > Pivot is just a point in accessible tree (may be a range) that allows anyone
> > interested to be notified. Without notification mechanism is a useless thing.
> > 
> 
> The pivot is observable via nsIAccessiblePivotObserver. I did this because I
> though that is how you believed it should be done, as opposed to being yet
> another accessible event that is not mapped in platform APIs.

That's right. I don't argue. I just did a statement about what pivot is.

> > Pivot usecase is virtual cursor, which is basically a point or a range
> > having visual presentation. Virtual cursor concept is presented in many
> > screen readers and I heard ideas to introduce this concept in ARIA 2. 
> 
> Really? Is this discussion happening anywhere online?

Nah, as far as I know this is just idea. Ask Aaron for details.

> > So I think virtual cursor implemented on browser side is something that can
> > happen in nearest future. That means it should be flexible and useful
> > (incorporate traversal functionality) and reuse existing APIs (use API
> > specific callback mechanism, for example, accessible events).
> > 
> 
> I don't know what you mean by traversal functionality, if you mean something
> similar to DOM tree walking, I think it should be distinct. We have a bug
> for that bug 703284.

We do. But I suggest to think about general picture. For example if pivots would incorporate navigational features then they should be more useful.

> > Also we talked about the idea to have more than one virtual cursor. Idea of
> > active virtual cursor or introduction of state of the virtual cursor (which
> > can be saved and restored) sounds good. 
> 
> This is where the generic pivot API comes into place. An AT could use them
> in addition to the document's virtual cursor and tie new display and control
> semantics to them.

Current API doesn't allow anything but creation a pivot. So it doesn't help AT to manage pivots. Why do you think it's not applicable to virtual cursors, i.e. we shouldn't provide an API to manage virtual cursors or virtual cursor state?

> > But we need to figure out how to
> > workaround possible collisions when multiple ATs manage the virtual cursor.
> > 
> 
> There is only one "virtual cursor", that is the whole point of having it in
> the first place. It is the one canonical focal point where the user's
> attention is at that very moment. Its meaning is objective, and it does not
> change from one AT to the next. Just like the text caret position means the
> same thing to the user no matter what AT they are accessing it with.

ok, so active virtual cursor might be not good since it assumes more than one vc. But what about vc states?

> The virtual cursor itself does not hold the "traversal functionality", ie.
> it does not have a next() method. It is the AT's responsibility to traverse
> and place the cursor on the next spot. So if two ATs are being used
> interchangeably, one hops to links and another to headers, they will still
> work as expected.

Yeah, why you don't want to have traversal functionality? How do you think headers and links navigation by different AT gets resolved?

> > Idea to have a virtual cursor per DOM document doesn't sound clear since
> > it's not evident how AT can navigate between documents. I agree chrome
> > traversal is optional but nice. So the on one hand we should support
> > optional documents and should provide integrity between nested DOM documents.
> > 
> 
> I think we could move forward without having the answers for all of that. By
> convention, we would simply care about the top-level content documents for
> now, and not about chrome or content sub-documents.

Do you say we should ignore content sub documents or something else?

> The questions for the future are about how to control and view those nested
> cursors, not really about this model API. For example, how do you step in
> and out of chrome? How do we not confuse the user with more than one cursor
> being presented?

I'd say it's great to think well about virtual cursor and relatives because if virtual cursors goes into APIs then either they will adopt our model and invent new one and then we need to adopt their model.

Comment 27

[alexander :surkov (:asurkov)]

Virtual cursor defines a point (a single accessible) or a range (text accessible and offsets within it).

The accessible document providing virtual cursor access (could be tab or root document accessible) implements nsIAccessibleContentDocument interface. When virtual cursor position is changed then EVENT_VIRTUALCURSOR_CHANGE event targeted to accessible content document is fired.

At this point virtual cursor is abstract concept and doesn't have visual or behavioral representation. AT can listen vc change event to supply representation.

Simplest approach:
interface nsIAccessibleContentDocument : public nsIAccessibleDocument
{
  // virtual cursor position
  attribute nsIAccessible virtualCursorAnchor;

  // virtual cursor selection
  readonly attribute int virtualCursorSelectionAnchor;
  readonly attribute int virtualCursorSelectionStartOffset;
  readonly attribute int virtualCursorSelectionEndOffset;

  void setVirtualCursorSelection(in in nsIAccessibleText anchor,
                                 in int startOffset, in int endOffset);
};

Sophisticated approach.
1) introduce pivot concept that manages the accessible range and expose traversal capabilities
2) virtual cursor is a pivot, AT can change a pivot for virtual cursor. That introduce vc state concept, i.e. when AT is going to move virtual cursor then it sets own pivot. But we need to provide protection mechanism to prevent ATs to change the pivot provided by other AT.

That's more interesting approach but things could look more complicated from AT point of view. That makes me uncertain about doing this way right now. The same time it's more flexible since incorporates traversal stuffs.

We could do the simplest one for now.

Comment 28

[Eitan Isaacson [:eeejay]]

(In reply to alexander surkov from comment #27)
> Virtual cursor defines a point (a single accessible) or a range (text
> accessible and offsets within it).
> 
> The accessible document providing virtual cursor access (could be tab or
> root document accessible) implements nsIAccessibleContentDocument interface.
> When virtual cursor position is changed then EVENT_VIRTUALCURSOR_CHANGE
> event targeted to accessible content document is fired.
> 
> At this point virtual cursor is abstract concept and doesn't have visual or
> behavioral representation. AT can listen vc change event to supply
> representation.
> 
> Simplest approach:
> interface nsIAccessibleContentDocument : public nsIAccessibleDocument
> {
>   // virtual cursor position
>   attribute nsIAccessible virtualCursorAnchor;
> 
>   // virtual cursor selection
>   readonly attribute int virtualCursorSelectionAnchor;

What does that do ^^

>   readonly attribute int virtualCursorSelectionStartOffset;
>   readonly attribute int virtualCursorSelectionEndOffset;
> 
>   void setVirtualCursorSelection(in in nsIAccessibleText anchor,
>                                  in int startOffset, in int endOffset);
> };
> 
> Sophisticated approach.
> 1) introduce pivot concept that manages the accessible range and expose
> traversal capabilities

Like I said on IRC, I think this is problematic since it doesn't allow ATs to interoperate well, like you mention below.

BAD:
pivot.setWalker(nsIAccessibleTreeWalker);
pivot.next();

That is bad because when another AT does next() it will not behave as expected, and they will both be paranoid about the vc's current mode and keep changing it. On the other hand, we could introduce an API for traversing without having the pivot store policy:

GOOD:
pivot.next(nsIAccessibleFilter);

I could see a few convenience methods existing in addition to this:
pivot.nextLink();
pivot.nextHeader();
pivot.nextWord();
etc.


> 2) virtual cursor is a pivot, AT can change a pivot for virtual cursor. That
> introduce vc state concept, i.e. when AT is going to move virtual cursor
> then it sets own pivot. But we need to provide protection mechanism to
> prevent ATs to change the pivot provided by other AT.

The document's virtual cursor's instance could actually be swapped with a different one? This seems problematic on a few levels. Most of all what you already mention. I think my proposal above solves this (don't store policy).

> 
> That's more interesting approach but things could look more complicated from
> AT point of view. That makes me uncertain about doing this way right now.
> The same time it's more flexible since incorporates traversal stuffs.

It is definitely more interesting, and I already have patches, just need to add more stuff (see below). I think if we introduce traversal in the pivot API than pivots become useful in their own right and not just as virtual cursors. I would then propose to keep the factory function (nsIAccessibleRetrieval.createAccessiblePivot()) to allow ATs to manage their own pivots.

Things to do:
- Add traversal API (nsIAccessibleFilter, nsIAccessiblePivot.next(), etc.)
- Add event EVENT_VIRTUALCURSOR_CHANGE
- Add nsIAccessibleContentDocument <- is this really necessary? It seems like it adds unnecessary complexity to the implementation (but I am a C++ newb, so maybe not). Also the name is a misnomer since it doesn't have to be a content document. It could be in the chrome as well in the future.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #28)
> (In reply to alexander surkov from comment #27)
> > Virtual cursor defines a point (a single accessible) or a range (text
> > accessible and offsets within it).
> > 
> > The accessible document providing virtual cursor access (could be tab or
> > root document accessible) implements nsIAccessibleContentDocument interface.
> > When virtual cursor position is changed then EVENT_VIRTUALCURSOR_CHANGE
> > event targeted to accessible content document is fired.
> > 
> > At this point virtual cursor is abstract concept and doesn't have visual or
> > behavioral representation. AT can listen vc change event to supply
> > representation.
> > 
> > Simplest approach:
> > interface nsIAccessibleContentDocument : public nsIAccessibleDocument
> > {
> >   // virtual cursor position
> >   attribute nsIAccessible virtualCursorAnchor;
> > 
> >   // virtual cursor selection
> >   readonly attribute int virtualCursorSelectionAnchor;
> 
> What does that do ^^

int was supposed be nsIAccessibleText. Idea is we have either cursor setting on accessible or selection. I'm not sure how to keep it nicer from API point of view.

> >   readonly attribute int virtualCursorSelectionStartOffset;
> >   readonly attribute int virtualCursorSelectionEndOffset;
> > 
> >   void setVirtualCursorSelection(in in nsIAccessibleText anchor,
> >                                  in int startOffset, in int endOffset);
> > };
> > 
> > Sophisticated approach.
> > 1) introduce pivot concept that manages the accessible range and expose
> > traversal capabilities
> 
> Like I said on IRC, I think this is problematic since it doesn't allow ATs
> to interoperate well, like you mention below.
>
> BAD:
> pivot.setWalker(nsIAccessibleTreeWalker);
> pivot.next();
> 
> That is bad because when another AT does next() it will not behave as
> expected, and they will both be paranoid about the vc's current mode and
> keep changing it. On the other hand, we could introduce an API for
> traversing without having the pivot store policy:

You shouldn't consider this option out of context of 2nd item.

> > 2) virtual cursor is a pivot, AT can change a pivot for virtual cursor. That
> > introduce vc state concept, i.e. when AT is going to move virtual cursor
> > then it sets own pivot. But we need to provide protection mechanism to
> > prevent ATs to change the pivot provided by other AT.
> 
> The document's virtual cursor's instance could actually be swapped with a
> different one? This seems problematic on a few levels. Most of all what you
> already mention. I think my proposal above solves this (don't store policy).

please elaborate this

> > That's more interesting approach but things could look more complicated from
> > AT point of view. That makes me uncertain about doing this way right now.
> > The same time it's more flexible since incorporates traversal stuffs.
> 
> It is definitely more interesting, and I already have patches, just need to
> add more stuff (see below). I think if we introduce traversal in the pivot
> API than pivots become useful in their own right and not just as virtual
> cursors.

And that was the idea.

> I would then propose to keep the factory function
> (nsIAccessibleRetrieval.createAccessiblePivot()) to allow ATs to manage
> their own pivots.

If pivot is a point or a range with notification capabilities and nothing else then it doesn't look useful.

> Things to do:
> - Add traversal API (nsIAccessibleFilter, nsIAccessiblePivot.next(), etc.)

is it for future or which approach do we follow?

> - Add event EVENT_VIRTUALCURSOR_CHANGE
> - Add nsIAccessibleContentDocument <- is this really necessary? It seems
> like it adds unnecessary complexity to the implementation (but I am a C++
> newb, so maybe not). 

getting virtual cursor on any document is confusing and sounds like a getting vc on any accessible

> Also the name is a misnomer since it doesn't have to be
> a content document. It could be in the chrome as well in the future.

right, ideas?

Comment 30

[Eitan Isaacson [:eeejay]]

After Alex and I talked today, we sketched up a workable solution, and I added it to the wiki.
https://wiki.mozilla.org/Accessibility/SoftFocus

Main changes:
- Special interface for documents that manage a virtual cursor (nsIAccessibleVirtualCursor).
- New event type and interface (EVENT_VIRTUALCURSOR_CHANGED, nsIAccessibleVirtualCursorChangeEvent).
- New traversal API (nsIAccessiblePivot.next*, nsIAccessiblePivot.previous* and nsIAccessibleTraversalRule).

And now, to implement!

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #30)
> After Alex and I talked today, we sketched up a workable solution, and I
> added it to the wiki.
> https://wiki.mozilla.org/Accessibility/SoftFocus

Please do idl file with interfaces as a patch so we could polish API inline.

Comment 32

[Eitan Isaacson [:eeejay]]

Attachment: Interface proposal

Here are the IDLs as they are currently in my working directory. Managed to implement everything aside from traversal functionality. Will tackle that next.
I hate the name nsIAccessibleVirtualCursor as an interface for a document that manages a virtual cursor, but I can't think of anything else!

Comment 33

[alexander :surkov (:asurkov)]

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

> I hate the name nsIAccessibleVirtualCursor as an interface for a document
> that manages a virtual cursor, but I can't think of anything else!

yeah, I don't like it too :) that would be one of my comments. Let's think of alternatives.

Comment 34

[Eitan Isaacson [:eeejay]]

I thought maybe nsIAccessibleVirtualCursorManager, but that is too long, IMHO.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 578347 [details] [diff] [review]
Interface proposal

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

::: accessible/public/nsIAccessibleEvent.idl
@@ +443,4 @@
>    const unsigned long EVENT_OBJECT_ATTRIBUTE_CHANGED = 0x0055;
>  
>    /**
> +   * An object's virtual cursor changed.

nit: more detailed description including what is a target of the event

@@ +553,5 @@
>    readonly attribute long numRowsOrCols;
>  };
>  
> +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> +interface nsIAccessibleVirtualCursorChangeEvent: nsISupports

do we really need interface for this? it might be tricky to map it to desktop APIs like MSAA. What's the usecase?

::: accessible/public/nsIAccessiblePivot.idl
@@ +64,5 @@
> +
> +  /*
> +   * The accessible the pivot is currently pointed at.
> +   */
> +  readonly attribute nsIAccessible accessible;

I'd like to have something more descriptive than generic accessible. Maybe point? But that one collides with DOM point which is different.

btw, why do you prefer to keep getter and setter separately (attribute accessible vs method setAccessible).

@@ +104,5 @@
> +   *   nsIAccessibleText inteface.
> +   * @throws NS_ERROR_FAILURE when the offset exceeds the accessible's
> +   *   character count.
> +   */
> +  void setTextOffset(in long aStartOffset, in long aEndOffset);

I'd add extra argument, a text accessible. What's the usecase of this method? Don't you like selection term for (text accessible, startoffset, endoffset) bunch? Maybe range?

@@ +125,5 @@
> +   * @param aFirst traverse to the first occurance in the document.
> +   * @return true on success, false if there are no new nodes to traverse to.
> +   */
> +  boolean previousObject(in nsIAccessibleTraversalRule aRule, in boolean aFirst);
> +

I'd use simple next/prev names
btw, does pivot word has a meaning of point or position? If so would it be more correct to use moveNext/movePrev?

::: accessible/public/nsIAccessibleTraversalRule.idl
@@ +45,5 @@
> +  const short FILTER_ACCEPT = 1;
> +  /* Reject this accessible object and don't traverse its children */
> +  const short FILTER_REJECT = 2;
> +  /* Skip this accessible object */
> +  const short FILTER_SKIP = 3;

I'm not sure I ever liked this constants in DOM walker. We could try to have more descriptive things. Example: MATCHED, IGNORED, IGNORED_WITH_SUBTREE. 
Also are you sure that tri-state is enough, do we need to have ACCEPT with options to traverse subtree or don't traverse it?

@@ +79,5 @@
> +  /**
> +   * A mask of extended states to skip. An object with any of the given states
> +   * is skipped.
> +   */
> +  readonly attribute long skipExtStates;

you example on wiki makes me think that it might be better to introduce predefined set of constants for navigation. And if author needs something special then he uses a function. For example, acceptRoles is used to take heading, skipStates for invisible and defunct. We could have something like:
NAVIGATE_HEADINGS or NAVIGATE_LINKS what makes navigate heading or links having no invisible or defunct states. NAVIGATE_ALL to navigate every object

@@ +85,5 @@
> +  /**
> +   * Traversal order for this rule, either ORDER_DEPTH_FIRST or
> +   * ORDER_BREADTH_FIRST.
> +   */
> +  readonly attribute short traversalOrder;

that belongs rather to walker than to rule I'd say. Objections of doing this?

::: accessible/public/nsIAccessibleVirtualCursor.idl
@@ +45,5 @@
> +{
> +  /**
> +   * The virtual cursor pivot this object manages.
> +   */
> +  readonly attribute nsIAccessiblePivot virtualCursor;

it's weird that virtual cursor object has virtual cursor property that returns an object different from virtual cursor object. This interface is going to be implemented by document accessible so it'd be nice if interface would contain 'document' in name. Maybe nsIAccessibleVCDocument?

Comment 36

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #35)
> Comment on attachment 578347 [details] [diff] [review] [diff] [details] [review]
> Interface proposal
> 
> Review of attachment 578347 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/public/nsIAccessibleEvent.idl
> @@ +443,4 @@
> >    const unsigned long EVENT_OBJECT_ATTRIBUTE_CHANGED = 0x0055;
> >  
> >    /**
> > +   * An object's virtual cursor changed.
> 
> nit: more detailed description including what is a target of the event
> 

Will do.

> @@ +553,5 @@
> >    readonly attribute long numRowsOrCols;
> >  };
> >  
> > +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> > +interface nsIAccessibleVirtualCursorChangeEvent: nsISupports
> 
> do we really need interface for this? it might be tricky to map it to
> desktop APIs like MSAA. What's the usecase?
> 

No, at least not at this point. We could drop it for now.

> ::: accessible/public/nsIAccessiblePivot.idl
> @@ +64,5 @@
> > +
> > +  /*
> > +   * The accessible the pivot is currently pointed at.
> > +   */
> > +  readonly attribute nsIAccessible accessible;
> 
> I'd like to have something more descriptive than generic accessible. Maybe
> point? But that one collides with DOM point which is different.
> 

Maybe object? Or is that even worse?

> btw, why do you prefer to keep getter and setter separately (attribute
> accessible vs method setAccessible).
> 

I don't, it was idl newbness :) I'll fix that.

> @@ +104,5 @@
> > +   *   nsIAccessibleText inteface.
> > +   * @throws NS_ERROR_FAILURE when the offset exceeds the accessible's
> > +   *   character count.
> > +   */
> > +  void setTextOffset(in long aStartOffset, in long aEndOffset);
> 
> I'd add extra argument, a text accessible. What's the usecase of this
> method? Don't you like selection term for (text accessible, startoffset,
> endoffset) bunch? Maybe range?
> 

Good idea, that fixes it a bit and doesn't allow setting offsets on non-text accessibles.
The term selection used in many contexts, and it signifies an actual DOM state. I would use range.

void setTextRange(in nsIAccessibleText aTextAccessible, in long aStartOffset, in long aEndOffset);

> @@ +125,5 @@
> > +   * @param aFirst traverse to the first occurance in the document.
> > +   * @return true on success, false if there are no new nodes to traverse to.
> > +   */
> > +  boolean previousObject(in nsIAccessibleTraversalRule aRule, in boolean aFirst);
> > +
> 
> I'd use simple next/prev names
> btw, does pivot word has a meaning of point or position? If so would it be
> more correct to use moveNext/movePrev?

Pivot has a few meanings, one is axis. So the motion a pivot does is rotate :)
We could use "move", if we do, we should be consistent:
moveNext()
movePrevious()
moveTextNext()
moveTextPrevious()

^^ does that look good?


> 
> ::: accessible/public/nsIAccessibleTraversalRule.idl
> @@ +45,5 @@
> > +  const short FILTER_ACCEPT = 1;
> > +  /* Reject this accessible object and don't traverse its children */
> > +  const short FILTER_REJECT = 2;
> > +  /* Skip this accessible object */
> > +  const short FILTER_SKIP = 3;
> 
> I'm not sure I ever liked this constants in DOM walker. We could try to have
> more descriptive things. Example: MATCHED, IGNORED, IGNORED_WITH_SUBTREE. 

I feel like there are advantages of using names that have the same meaning elsewhere. But I don't care enough :)

> Also are you sure that tri-state is enough, do we need to have ACCEPT with
> options to traverse subtree or don't traverse it?
> 

In the example in the wiki I show how to do that, just examine parent accessible, and reject. If we depart from the DOM tree walker, as you suggest, than sure. We could add a fourth state MATCHED_WITHOUT_SUBTREE.

> @@ +79,5 @@
> > +  /**
> > +   * A mask of extended states to skip. An object with any of the given states
> > +   * is skipped.
> > +   */
> > +  readonly attribute long skipExtStates;
> 
> you example on wiki makes me think that it might be better to introduce
> predefined set of constants for navigation. And if author needs something
> special then he uses a function. For example, acceptRoles is used to take
> heading, skipStates for invisible and defunct. We could have something like:
> NAVIGATE_HEADINGS or NAVIGATE_LINKS what makes navigate heading or links
> having no invisible or defunct states. NAVIGATE_ALL to navigate every object
> 

I think it is premature to know what is needed. Plus, an AT would need to consider a whole set of exceptions and special cases for different websites, bugs, quirks, etc. I don't think these should be "hard-coded" into Firefox.

On the other hand, I am not very happy with the states filters (skip/accept[Ext]States), I think they are a bit complicated. Maybe we could introduce ignore flags like this:
- IGNORE_DEFUNCT   = 0x01
- IGNORE_INVISIBLE = 0x02
- IGNORE_OFFSCREEN = 0x04

We could extend it in the future as we find more useful things to ignore. What do you think?

> @@ +85,5 @@
> > +  /**
> > +   * Traversal order for this rule, either ORDER_DEPTH_FIRST or
> > +   * ORDER_BREADTH_FIRST.
> > +   */
> > +  readonly attribute short traversalOrder;
> 
> that belongs rather to walker than to rule I'd say. Objections of doing this?
> 

You mean an attribute in nsIAccessiblePivot? That would break the rule of not storing traversal policy in the pivot.

Originally this was an argument in nextObject, but it seemed like a lot of arguments, should I add it back?

> ::: accessible/public/nsIAccessibleVirtualCursor.idl
> @@ +45,5 @@
> > +{
> > +  /**
> > +   * The virtual cursor pivot this object manages.
> > +   */
> > +  readonly attribute nsIAccessiblePivot virtualCursor;
> 
> it's weird that virtual cursor object has virtual cursor property that
> returns an object different from virtual cursor object. This interface is
> going to be implemented by document accessible so it'd be nice if interface
> would contain 'document' in name. Maybe nsIAccessibleVCDocument?

I don't like having an acronym. Maybe nsIAccessibleTraversableDocument? or nsIAccessibleTraversable?

Comment 37

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #36)
> (In reply to alexander :surkov from comment #35)
> > ::: accessible/public/nsIAccessiblePivot.idl
> > @@ +64,5 @@
> > > +
> > > +  /*
> > > +   * The accessible the pivot is currently pointed at.
> > > +   */
> > > +  readonly attribute nsIAccessible accessible;
> > 
> > I'd like to have something more descriptive than generic accessible. Maybe
> > point? But that one collides with DOM point which is different.
> > 
> 
> Maybe object? Or is that even worse?
> 

maybe 'position'? I'm going with position for now.

Comment 38

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #36)
> (In reply to alexander :surkov from comment #35)
> > Comment on attachment 578347 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > 
> > ::: accessible/public/nsIAccessibleTraversalRule.idl
> > @@ +45,5 @@
> > > +  const short FILTER_ACCEPT = 1;
> > > +  /* Reject this accessible object and don't traverse its children */
> > > +  const short FILTER_REJECT = 2;
> > > +  /* Skip this accessible object */
> > > +  const short FILTER_SKIP = 3;
> > 
> > I'm not sure I ever liked this constants in DOM walker. We could try to have
> > more descriptive things. Example: MATCHED, IGNORED, IGNORED_WITH_SUBTREE. 
> 
> I feel like there are advantages of using names that have the same meaning
> elsewhere. But I don't care enough :)
> 
> > Also are you sure that tri-state is enough, do we need to have ACCEPT with
> > options to traverse subtree or don't traverse it?
> > 
> 
> In the example in the wiki I show how to do that, just examine parent
> accessible, and reject. If we depart from the DOM tree walker, as you
> suggest, than sure. We could add a fourth state MATCHED_WITHOUT_SUBTREE.
> 

Ok, how about a bit field:
MATCH = 0x0
IGNORE = 0x1
IGNORE_SUBTREE = 0x2

A match (like FILTER_ACCEPT) would be a return value of 0x0.
To ignore this node, but traverse subtree (like FILTER_SKIP), return value would be 0x1
To ignore node and not traverse subtree (FILTER_REJECT), return IGNORE | IGNORE_SUBTREE (0x3)
To match node, but ignore subtree, return IGNORE_SUBTREE (0x2).

Comment 39

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #38)
> (In reply to Eitan Isaacson [:eeejay] from comment #36)
> > (In reply to alexander :surkov from comment #35)
> > > Comment on attachment 578347 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > 
> > > ::: accessible/public/nsIAccessibleTraversalRule.idl
> > > @@ +45,5 @@
> > > > +  const short FILTER_ACCEPT = 1;
> > > > +  /* Reject this accessible object and don't traverse its children */
> > > > +  const short FILTER_REJECT = 2;
> > > > +  /* Skip this accessible object */
> > > > +  const short FILTER_SKIP = 3;
> > > 
> > > I'm not sure I ever liked this constants in DOM walker. We could try to have
> > > more descriptive things. Example: MATCHED, IGNORED, IGNORED_WITH_SUBTREE. 
> > 
> > I feel like there are advantages of using names that have the same meaning
> > elsewhere. But I don't care enough :)
> > 
> > > Also are you sure that tri-state is enough, do we need to have ACCEPT with
> > > options to traverse subtree or don't traverse it?
> > > 
> > 
> > In the example in the wiki I show how to do that, just examine parent
> > accessible, and reject. If we depart from the DOM tree walker, as you
> > suggest, than sure. We could add a fourth state MATCHED_WITHOUT_SUBTREE.
> > 
> 
> Ok, how about a bit field:
> MATCH = 0x0
> IGNORE = 0x1
> IGNORE_SUBTREE = 0x2
> 
> A match (like FILTER_ACCEPT) would be a return value of 0x0.
> To ignore this node, but traverse subtree (like FILTER_SKIP), return value
> would be 0x1
> To ignore node and not traverse subtree (FILTER_REJECT), return IGNORE |
> IGNORE_SUBTREE (0x3)
> To match node, but ignore subtree, return IGNORE_SUBTREE (0x2).

I just realized that this is a slightly more nuanced enum :)

Comment 40

[alexander :surkov (:asurkov)]

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

> > > +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> > > +interface nsIAccessibleVirtualCursorChangeEvent: nsISupports
> > 
> > do we really need interface for this? it might be tricky to map it to
> > desktop APIs like MSAA. What's the usecase?
> > 
> No, at least not at this point. We could drop it for now.

makes sense to keep thins simpler until we need a feature


> > I'd use simple next/prev names
> > btw, does pivot word has a meaning of point or position? If so would it be
> > more correct to use moveNext/movePrev?
> 
> Pivot has a few meanings, one is axis. So the motion a pivot does is rotate
> :)
> We could use "move", if we do, we should be consistent:
> moveNext()
> movePrevious()
> moveTextNext()
> moveTextPrevious()
> 
> ^^ does that look good?

even maybe
moveForward/moveBackward

moveForwardByText ?

> > Also are you sure that tri-state is enough, do we need to have ACCEPT with
> > options to traverse subtree or don't traverse it?
> > 
> 
> In the example in the wiki I show how to do that, just examine parent
> accessible, and reject. If we depart from the DOM tree walker, as you
> suggest, than sure. We could add a fourth state MATCHED_WITHOUT_SUBTREE.

yep, it should be handy

> > @@ +79,5 @@
> > > +  /**
> > > +   * A mask of extended states to skip. An object with any of the given states
> > > +   * is skipped.
> > > +   */
> > > +  readonly attribute long skipExtStates;
> > 
> > you example on wiki makes me think that it might be better to introduce
> > predefined set of constants for navigation. And if author needs something
> > special then he uses a function. For example, acceptRoles is used to take
> > heading, skipStates for invisible and defunct. We could have something like:
> > NAVIGATE_HEADINGS or NAVIGATE_LINKS what makes navigate heading or links
> > having no invisible or defunct states. NAVIGATE_ALL to navigate every object
> > 
> 
> I think it is premature to know what is needed. Plus, an AT would need to
> consider a whole set of exceptions and special cases for different websites,
> bugs, quirks, etc. I don't think these should be "hard-coded" into Firefox.
> 
> On the other hand, I am not very happy with the states filters
> (skip/accept[Ext]States), I think they are a bit complicated. Maybe we could
> introduce ignore flags like this:
> - IGNORE_DEFUNCT   = 0x01
> - IGNORE_INVISIBLE = 0x02
> - IGNORE_OFFSCREEN = 0x04

they could be by default so if you need to move by invisibles then you should implement traversal function. But not offscreen if vc is supposed to scroll the client area.

Do you have an example to move by set of roles otherwise we could have a role attribute? Consider to have constant to move by controls and tabnavigable items.

> We could extend it in the future as we find more useful things to ignore.
> What do you think?

we need to provide minimal interface for handy navigation for you.

> > @@ +85,5 @@
> > > +  /**
> > > +   * Traversal order for this rule, either ORDER_DEPTH_FIRST or
> > > +   * ORDER_BREADTH_FIRST.
> > > +   */
> > > +  readonly attribute short traversalOrder;
> > 
> > that belongs rather to walker than to rule I'd say. Objections of doing this?
> > 
> 
> You mean an attribute in nsIAccessiblePivot? That would break the rule of
> not storing traversal policy in the pivot.

> Originally this was an argument in nextObject, but it seemed like a lot of
> arguments, should I add it back?

adding methods like
moveTop and/or moveFirst (moveBottom and/or moveLast)

> > it's weird that virtual cursor object has virtual cursor property that
> > returns an object different from virtual cursor object. This interface is
> > going to be implemented by document accessible so it'd be nice if interface
> > would contain 'document' in name. Maybe nsIAccessibleVCDocument?
> 
> I don't like having an acronym. Maybe nsIAccessibleTraversableDocument? or
> nsIAccessibleTraversable?

that doesn't say anything about vc what I don't like. Let's do things we are certain about and then provide list of option for features we have doubts about and ask others.

Comment 41

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #37)
> (In reply to Eitan Isaacson [:eeejay] from comment #36)
> > (In reply to alexander :surkov from comment #35)
> > > I'd like to have something more descriptive than generic accessible. Maybe
> > > point? But that one collides with DOM point which is different.
> > > 
> > 
> > Maybe object? Or is that even worse?
> > 
> 
> maybe 'position'? I'm going with position for now.

position might be good. What about anchor? Too much linky? :)

(In reply to Eitan Isaacson [:eeejay] from comment #39)
> > Ok, how about a bit field:
> > MATCH = 0x0
> > IGNORE = 0x1
> > IGNORE_SUBTREE = 0x2
> > 
> > A match (like FILTER_ACCEPT) would be a return value of 0x0.
> > To ignore this node, but traverse subtree (like FILTER_SKIP), return value
> > would be 0x1
> > To ignore node and not traverse subtree (FILTER_REJECT), return IGNORE |
> > IGNORE_SUBTREE (0x3)
> > To match node, but ignore subtree, return IGNORE_SUBTREE (0x2).
> 
> I just realized that this is a slightly more nuanced enum :)

they aren't really bit fieds, maybe 4 constants is a way to go

Comment 42

[Eitan Isaacson [:eeejay]]

OK, after a while of trying, I am concluding the breadth order tree traversal is just not worth it. Any implementation would just be an inefficient ugly mess. I know that the traditional algorithm is about 4 lines with a queue, but to be able to start the search from a non-root node, and to be able to perform a reverse search is just too much.

I really can't think of many use-cases for a breadth search order, so I am dropping this for now. I think I will remove it from the proposed API altogether. If there are objections please let me know.

Jamie, I would love to know if you think breadth search is useful in screen reader use cases.

Comment 43

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #40)
> (In reply to Eitan Isaacson [:eeejay] from comment #36) 
> > We could use "move", if we do, we should be consistent:
> > moveNext()
> > movePrevious()
> > moveTextNext()
> > moveTextPrevious()
> > 
> > ^^ does that look good?
> 
> even maybe
> moveForward/moveBackward
> 
> moveForwardByText ?
> 

I am a bit nervous about forward/backward. It is a departure from similar search and tree nav functions that use next/previous. I think we should keep the language as close as possible to similar features. So I am hesitant to change that.

> > 
> > I think it is premature to know what is needed. Plus, an AT would need to
> > consider a whole set of exceptions and special cases for different websites,
> > bugs, quirks, etc. I don't think these should be "hard-coded" into Firefox.
> > 
> > On the other hand, I am not very happy with the states filters
> > (skip/accept[Ext]States), I think they are a bit complicated. Maybe we could
> > introduce ignore flags like this:
> > - IGNORE_DEFUNCT   = 0x01
> > - IGNORE_INVISIBLE = 0x02
> > - IGNORE_OFFSCREEN = 0x04
> 
> they could be by default so if you need to move by invisibles then you
> should implement traversal function. But not offscreen if vc is supposed to
> scroll the client area.
> 

I don't understand how that would work. If not traversing invisible nodes is the default, how does the filter function get to it? There would need to be a TRAVERSE_INVISIBLE flag, which I think is confusing if some flags are excluding (IGNORE_), and some are including (TRAVERSE_), which take precedent? I think all flags should be excluding, that is also the function of a "pre-filter", to filter out user defined nodes and lighten the load on the filter function.

> Do you have an example to move by set of roles otherwise we could have a
> role attribute?

Example use case of more than one role: header navigation, if we want to navigate headers we would need "header" nodes, but we also might want to check text leaves to see if a portion of text is unusually large and consider it a header because a website might not be semantically correct and may be relying on visual styling. This is just a hypothetical example, but my point is that we will need to provide some flexibility here.

Seems easy and simple enough to have an array with a few possible roles. Again, this is a prefilter so if we provide an option for only one prefiltered role, and we suddenly have a use-case for 2 roles, than we can't use this prefilter at all, and we need to use the function for everything. On the other hand, if we have a roles array as a prefilter, and it only has one element, no harm done. 

> Consider to have constant to move by controls and
> tabnavigable items.

Yep. Maybe IGNORE_NOT_FOCUSABLE. This is also an example of where the roles attribute is useful. Have the prefilter set to IGNORE_NOT_FOCUSABLE and have a list of roles that are form controls.

> 
> > We could extend it in the future as we find more useful things to ignore.
> > What do you think?
> 
> we need to provide minimal interface for handy navigation for you.
> 

Right. The ones that are there, plus IGNORE_NOT_FOCUSABLE, are good for now. I'll open another bug if another flag is needed in the future :)

Comment 44

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #41)
> (In reply to Eitan Isaacson [:eeejay] from comment #37)
> > (In reply to Eitan Isaacson [:eeejay] from comment #36)
> > > (In reply to alexander :surkov from comment #35)
> > > > I'd like to have something more descriptive than generic accessible. Maybe
> > > > point? But that one collides with DOM point which is different.
> > > > 
> > > 
> > > Maybe object? Or is that even worse?
> > > 
> > 
> > maybe 'position'? I'm going with position for now.
> 
> position might be good. What about anchor? Too much linky? :)

Position it is!

> 
> (In reply to Eitan Isaacson [:eeejay] from comment #39)
> > > Ok, how about a bit field:
> > > MATCH = 0x0
> > > IGNORE = 0x1
> > > IGNORE_SUBTREE = 0x2
> > > 
> > > A match (like FILTER_ACCEPT) would be a return value of 0x0.
> > > To ignore this node, but traverse subtree (like FILTER_SKIP), return value
> > > would be 0x1
> > > To ignore node and not traverse subtree (FILTER_REJECT), return IGNORE |
> > > IGNORE_SUBTREE (0x3)
> > > To match node, but ignore subtree, return IGNORE_SUBTREE (0x2).
> > 
> > I just realized that this is a slightly more nuanced enum :)
> 
> they aren't really bit fieds, maybe 4 constants is a way to go

Yeah, dopes. We will have 4 constants.

Comment 45

[Eitan Isaacson [:eeejay]]

Attachment: Add nsIAccessiblePivot interface and implement.

Comment 46

[Eitan Isaacson [:eeejay]]

Attachment: Add pivot factory function to nsIAccessibleRetrieval and implement.

Comment 47

[Eitan Isaacson [:eeejay]]

Attachment: Add nsIAccessibleCursorable and implement.

Comment 48

[Eitan Isaacson [:eeejay]]

Attachment: Add EVENT_VIRTUALCURSOR_CHANGED and implement.

Comment 49

[Eitan Isaacson [:eeejay]]

Trevor, for your review. Mochitests will follow soon.

Comment 50

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581825 [details] [diff] [review]
Add pivot factory function to nsIAccessibleRetrieval and implement.


>+nsAccessibilityService::CreateAccessiblePivot(nsIAccessible *aAccessibleRoot,
>+                                              nsIAccessiblePivot **aPivot)

nit, type* var

>+{
>+  NS_ENSURE_ARG_POINTER(aPivot);
>+  NS_ENSURE_TRUE(aAccessibleRoot, NS_ERROR_INVALID_ARG);

use NS_ENSURE_ARG_POINTER for both.

also we usually set out  ptrs to null at the beginning.

>+
>+  nsRefPtr<nsAccessible> accessibleRoot(do_QueryObject(aAccessibleRoot));

there's no gaurentee this gets you an nsAccessible*, js or something could implemnt that interface.  I think the write fix here might be to mark nsIAccessible as [builtin] or whatever it is, but unless you want to block on that I'd suggest NS_ENSURE_TRUE(accessibleRoot, NS_ERROR_INVALID_ARG);

also, nit accessibleRoot = ...
>+  nsAccessiblePivot *pivot = new nsAccessiblePivot(accessibleRoot);
>+  NS_ENSURE_TRUE(pivot, NS_ERROR_OUT_OF_MEMORY);

new is infalable.

>+  NS_ADDREF(*aPivot = pivot);

consider nsRefPtr:swap / forget

general nit, you don't need so many blank lines.

Comment 51

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #50)
> Comment on attachment 581825 [details] [diff] [review]
> >+
> >+  nsRefPtr<nsAccessible> accessibleRoot(do_QueryObject(aAccessibleRoot));
> 
> there's no gaurentee this gets you an nsAccessible*, js or something could
> implemnt that interface.  I think the write fix here might be to mark
> nsIAccessible as [builtin] or whatever it is, but unless you want to block
> on that I'd suggest NS_ENSURE_TRUE(accessibleRoot, NS_ERROR_INVALID_ARG);

That is a good point. I originally didn't have the do_QueryObject() and simply stored a nsIAccessible. Maybe we should go back to that? It means using a lot of xpcom methods instead of the convenient nsAccessible ones. I knew there was a risk to down casting, but I have seen precedents for the above in other code.

I'll fix the other issues.

Comment 52

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #50)
> Comment on attachment 581825 [details] [diff] [review]
> >+
> >+  nsRefPtr<nsAccessible> accessibleRoot(do_QueryObject(aAccessibleRoot));
> 
> there's no gaurentee this gets you an nsAccessible*, js or something could
> implemnt that interface.  I think the write fix here might be to mark
> nsIAccessible as [builtin] or whatever it is, but unless you want to block
> on that I'd suggest NS_ENSURE_TRUE(accessibleRoot, NS_ERROR_INVALID_ARG);

That is a good point. I originally didn't have the do_QueryObject() and simply stored a nsIAccessible. Maybe we should go back to that? It means using a lot of xpcom methods instead of the convenient nsAccessible ones. I knew there was a risk to down casting, but I have seen precedents for the above in other code.

I'll fix the other issues.

Comment 53

[Trevor Saunders (:tbsaunde)]

(In reply to Eitan Isaacson [:eeejay] from comment #52)
> (In reply to Trevor Saunders (:tbsaunde) from comment #50)
> > Comment on attachment 581825 [details] [diff] [review]
> > >+
> > >+  nsRefPtr<nsAccessible> accessibleRoot(do_QueryObject(aAccessibleRoot));
> > 
> > there's no gaurentee this gets you an nsAccessible*, js or something could
> > implemnt that interface.  I think the write fix here might be to mark
> > nsIAccessible as [builtin] or whatever it is, but unless you want to block
> > on that I'd suggest NS_ENSURE_TRUE(accessibleRoot, NS_ERROR_INVALID_ARG);
> 
> That is a good point. I originally didn't have the do_QueryObject() and
> simply stored a nsIAccessible. Maybe we should go back to that? It means
> using a lot of xpcom methods instead of the convenient nsAccessible ones. I
> knew there was a risk to down casting, but I have seen precedents for the
> above in other code.

I think I prefer the error out approach.  I don't see much reason to support  extensions creating objects that implement nsIAccessibleFoo., or atleast use them here, unless Alex disagrees of course.

Comment 54

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581824 [details] [diff] [review]
Add nsIAccessiblePivot interface and implement.

Jonas, I'm sure the impl needs work, but would be nice if you could look over interfaces and concept.

Comment 55

[Eitan Isaacson [:eeejay]]

Comment on attachment 581824 [details] [diff] [review]
Add nsIAccessiblePivot interface and implement.

Removing this from the review queue since there are some obvious bugs in it. A new one should be up shortly.

Comment 56

[Eitan Isaacson [:eeejay]]

Attachment: Add nsIAccessiblePivot interface and implement.

Comment 57

[Eitan Isaacson [:eeejay]]

Comment on attachment 582889 [details] [diff] [review]
Add nsIAccessiblePivot interface and implement.

This fixes a few issues I ran into when I actually wrote tests for it :P

Comment 58

[Eitan Isaacson [:eeejay]]

Attachment: Mochitests

Here are the mochitests

Comment 59

[Trevor Saunders (:tbsaunde)]

Comment on attachment 582889 [details] [diff] [review]
Add nsIAccessiblePivot interface and implement.


>       nsIXBLAccessible.idl \
>+      nsIAccessiblePivot.idl \
>+      nsIAccessibleTraversalRule.idl \

keep in order please.

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+/* ***** BEGIN LICENSE BLOCK 

add vim line too.

>+  const nsAccessiblePivotBoundary BOUNDARY_CHAR            = 0;
>+  const nsAccessiblePivotBoundary BOUNDARY_WORD            = 1;
>+  const nsAccessiblePivotBoundary BOUNDARY_LINE            = 2;
>+  const nsAccessiblePivotBoundary BOUNDARY_ATTRIBUTE_RANGE = 3;

I think I'd rather just use the boundary constants we already have for nsIAccessibleText.

>+   * The start offset in the accessible's text. Only supported when the accessible has
>+   * the nsIAccessibleText interface. If no explicit offset 

s/has/implements/

>+   * The end offset in the accessible's text. Only supported when the accessible has
>+   * the nsIAccessibleText interface. If no explicit offset 

same

was set or if it is not
>+   * supported in the pivot's accessible this is -1.
>+   */
>+  readonly attribute long endOffset;
>+
>+  /**
>+   * Set the pivot's text range in a text accessible.
>+   *
>+   * @param aStartOffset the start offset to set.
>+   * @param aEndOffset the end offset to set.
>+   * @throws NS_ERROR_NO_INTERFACE when the pivot's accessible does not have the
>+   *   nsIAccessibleText inteface.
>+   * @throws NS_ERROR_FAILURE when the offset exceeds the accessible's
>+   *   character count.
>+   */
>+  void setTextRange(in nsIAccessibleText aTextAccessible,
>+                    in long aStartOffset, in long aEndOffset);

it seems a little odd this changes the pivit as well as the selection.

>+   * Move pivot to next text range.
>+   *
>+   * @param aBoundary type of boundary for next text range, character, word, etc.
>+   * @param aLast traverse to the last occurance in the document.

the document? I assume you mean this accessible?

>+   * Move pivot to previous text range.
>+   *
>+   * @param aBoundary type of boundary for previous text range, character, word,
>+   *   etc.
>+   * @param aFirst traverse to the first occurance in the document.

this part makes no sense, the argument doesn't appear to exist.

>+   * @return true on success, false if there are is no more text.

whatdo these methods do when the pivit isn't a text accessible?

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+/* ***** BEGIN LICENSE BLOCK *****

same, add vim line.

>+  /* Ignore this accessible object */
>+  const short FILTER_IGNORE = 0x0;

but not its children right?

>+  /* Accept this accessible object */
>+  const short FILTER_MATCH = 0x1;
>+  /* Don't traverse accessibles children */
>+  const short FILTER_IGNORE_SUBTREE = 0x2;

the accessible itself should also be ignored right?

>+  readonly attribute nsIArray matchRoles;

Jonas, what is the prefered way of doing arrays in idl?  It seems the [retval, array, size_is()] out type array); might make the implementation nicer.

>   nsTextAccessible.cpp \
>   nsTextEquivUtils.cpp \
>   nsTextAttrs.cpp \
>   TextUpdater.cpp \
>+  nsAccessiblePivot.cpp \

please keep in order.

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****

vim line again.

>+#include "nsISupportsPrimitives.h"
>+#include "nsIAccessibleText.h"
>+
>+#include "nsAccessiblePivot.h"
>+#include "nsAccUtils.h"
>+
>+#include "nsArrayUtils.h"
>+#include "nsComponentManagerUtils.h"

please order these, as usual foo.h then other a11y headers, then general headers.

>+
>+nsAccessiblePivot::nsAccessiblePivot() :
>+  mRootNode(nsnull)
>+{
>+}
>+
>+nsAccessiblePivot::nsAccessiblePivot(nsAccessible* aRootNode) :
>+  mRootNode(aRootNode),
>+  mPosition(nsnull),
>+  mStartOffset(-1),
>+  mEndOffset(-1)

I'd think you could put some of those on the same line.

>+nsAccessiblePivot::GetRootNode(nsIAccessible** aRootNode)
>+{
>+  NS_ENSURE_ARG_POINTER(aRootNode);
>+  NS_IF_ADDREF(*aRootNode = mRootNode);

is there actually a case in which there isn't a root?  If not I'd say you should just use NS_ADDREF()

>+bool
>+nsAccessiblePivot::IsRootDescendant(nsAccessible* aNode) {

braces get there own line here.

>+  nsAccessible* ancestor = aNode;

no local variable needed

>+nsAccessiblePivot::SetPosition(nsIAccessible* aPosition)

>+  if (aPosition) {
>+    nsRefPtr<nsAccessible> position(do_QueryObject(aPosition));

might fail.

>+
>+    if (!IsRootDescendant(position))
>+      return NS_ERROR_INVALID_ARG; //Accessible is not a descendant of pivot root

comment isn't really useful.

>+  mStartOffset = -1;
>+  mEndOffset = -1;

m
StartOffset = mEndOffset = -1;

also, this means the positions you fire the event with will be wrong.

>+  NotifyPivotChanged(oldPosition, 

possible use after free if mPosition was the last reference holder to oldPosition which is possible.

I think I'd do something like

nsRefPtr<nsAccessible> newPosition;
if (aPosition)
  newPosition = do_QueryObject(aPosition);

if (aPosition && (!newPosition || !IsDecendentOfRoot(newPosition))
  return NS_ERROR_INVALID_ARG;

nsRefPtr<nsAccessible> oldPosition = mPosition.forget();
mPosition = newPosition.forget();
PRInt32 oldStart = mStartOffset, oldEnd = mOldeEndOffset;
Notify(oldPosition, oldStart, oldEnd);

you might be able to replace the two forget()s with a nsRefPtr::swap but I don't have a great idea how to name variables that way.

>+nsAccessiblePivot::GetStartOffset(PRInt32 *aStartOffset)

nit, type* not type *

>+  nsAccessible *oldPosition = mPosition;

same, type*

>+
>+  nsRefPtr<nsAccessible> aPosition(do_QueryObject(aTextAccessible));

might fail, also foo = do_QueryObject() not foo(do_QueryObject())

>+  if (!IsRootDescendant(aPosition))
>+    return NS_ERROR_INVALID_ARG; //Accessible is not a descendant of pivot root
>+
>+  // Make sure the given offsets don't exceed the character count.
>+  PRInt32 charCount = 0;
>+  nsresult rv = aTextAccessible->GetCharacterCount(&charCount);
>+  NS_ENSURE_SUCCESS(rv, rv);

it might make sense to do_QueryObject() to nsHyperTextAccessible so you can use the inline version.

>+
>+  if (aStartOffset > charCount || aEndOffset > charCount)
>+    return NS_ERROR_FAILURE;
>+
>+  mStartOffset = (aStartOffset <= aEndOffset) ? aStartOffset : aEndOffset;
>+  mEndOffset = (aEndOffset >= aStartOffset) ? aEndOffset : aStartOffset;

I tend to think we should be less tolerant here and require 0 <= aStart <= aEnd < length

>+  mPosition = aPosition;
>+
>+  NotifyPivotChanged(oldPosition, oldStart, oldEnd);

same possible use after free.

>+nsAccessiblePivot::MoveNext(nsIAccessibleTraversalRule* aRule, bool* aResult)
>+{
>+  NS_ENSURE_ARG(aResult);

check aRule too

>+  nsresult rv = NS_OK;
>+  nsAccessible* node = SearchForward(mPosition, aRule, &rv);
>+
>+  *aResult = (node && rv == NS_OK);

that's not the only ok status, use NS_FAILED, there might be an NS_SUCCEEDED() too.

>+
>+  if (!(*aResult)) {
>+    return rv;
>+  }

no braces.

>+nsAccessiblePivot::MovePrevious(nsIAccessibleTraversalRule* aRule, bool* aResult)

all of the same applies here too

>+nsAccessiblePivot::MoveFirst(nsIAccessibleTraversalRule* aRule, bool* aResult)

and here

>+nsAccessiblePivot::MoveLast(nsIAccessibleTraversalRule* aRule, bool* aResult)

and here

>+  while (lastNode->HasChildren()) {
>+    lastNode = lastNode->LastChild();
>+  }

no braces

>+nsAccessiblePivot::MoveNextByText(nsAccessiblePivotBoundary aBoundary,
>+                                  bool* aResult)
>+{
>+  NS_ENSURE_ARG(aResult);
>+
>+  *aResult = false;
>+
>+  return NS_OK;

I'd throw NS_ERROR_NOT_IMPLEMENTED

>+nsAccessiblePivot::MovePreviousByText(nsAccessiblePivotBoundary aBoundary,
>+                                      bool* aResult)

same

>+nsAccessible *
>+nsAccessiblePivot::SearchBackward(nsAccessible *aNode,
>+                                  nsIAccessibleTraversalRule* aRule,
>+                                  nsresult *rv)

type* not type *

>+  if (!node)

you don't need a local var yet.

>+    nsAccUtils::To32States(aNode->State(), &state, &extState);

you could just use states::FOO here.

>+nsAccessiblePivot::AddObserver(nsIAccessiblePivotObserver *aObserver)

>+  if (!mObservers.AppendObject(aObserver))
>+    return NS_ERROR_OUT_OF_MEMORY;

I would expect allocations here are infalible.

>+++ b/accessible/src/base/nsAccessiblePivot.h
>@@ -0,0 +1,87 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK 

add vim line.

>+#include "nsIAccessibleTraversalRule.h"

do you use this one at all?

>+#include "nsAccessible.h"

I don't think you need that, I think the forward decl should be enough.

>+private:
>+  nsAccessiblePivot();
>+  nsAccessiblePivot(const nsAccessiblePivot&);
>+  nsAccessiblePivot& operator = (const nsAccessiblePivot&);

If you're trying to prevent copying you shouldn't provide a impl of nsAccessiblePivit() and apparently there is a macro for this somewhere in mfbt/

>+  long           mStartOffset;
>+  long           mEndOffset;

it probably makes sense to use PRInt32 here too.

>+  nsCOMArray<nsIAccessiblePivotObserver> mObservers;

I think you want nsTObserverArray here

Comment 60

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581826 [details] [diff] [review]
Add nsIAccessibleCursorable and implement.


>       nsIAccessiblePivot.idl \
>+      nsIAccessibleCursorable.idl \

keep in order

>diff --git a/accessible/public/nsIAccessibleCursorable.idl b/accessible/public/nsIAccessibleCursorable.idl
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+/* ***** BEGIN LICENSE BLOCK *****

vim line

>+  if (parentDoc) {
>+    // Check if content element for this embedded document is a <browser/> tag.
>+    nsIContent *ownerContent = parentDoc->FindContentForSubDocument(mDocument);
>+    mNeedsVirtualCursor = ownerContent->Tag() == nsGkAtoms::browser;
>+  } else {
>+    // This is a top-level document, so we provide a virtual cursor.
>+    mNeedsVirtualCursor = true;
>+  }
>+
>+  // Initialize virtual cursor if this document manages it.
>+  if (mNeedsVirtualCursor) {
>+    mVirtualCursor = new nsAccessiblePivot(this);
>+    NS_ASSERTION(mVirtualCursor, "Failed to initialize virtual cursor.");
>+  }

can't fail, so  no need to assert.  It would appear it makes more sense just to do the construction on both paths than to have a boolean.

>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIAccessibleCursorable,
>+                                     mNeedsVirtualCursor)

just use mVirtualCurser

>     foundInterface = 0;
> 
>   nsresult status;
>   if (!foundInterface) {
>     // HTML document accessible must inherit from nsHyperTextAccessible to get
>     // support text interfaces. XUL document accessible doesn't need this.
>     // However at some point we may push <body> to implement the interfaces and
>     // return nsDocAccessible to inherit from nsAccessibleWrap.
>@@ -510,16 +530,32 @@ nsDocAccessible::GetChildDocumentAt(PRUint32 aIndex,
> 
>   if (IsDefunct())
>     return NS_OK;
> 
>   NS_IF_ADDREF(*aDocument = GetChildDocumentAt(aIndex));
>   return *aDocument ? NS_OK : NS_ERROR_INVALID_ARG;
> }
> 
>+// nsIAccessibleVirtualCursor method

usually sectons are marked by a full line comment then the interface something like

////////////////////////////////////////////////////////
// nsFooAccessible: nsIFoo

>+  if (IsDefunct())
>+    return NS_OK;

we usually do NS_ERROR_FAILURE, but atleast you should be sure the out ptr is null before returning.

>+
>+  if (!mVirtualCursor)
>+    mVirtualCursor = new nsAccessiblePivot(this);

shouldn't ever be needed

>+  NS_IF_ADDREF(*aVirtualCursor = mVirtualCursor);

NS_AddRef should be fine

>+#include "nsAccessiblePivot.h"

shouldn't be needed, forward decl should do

> class nsDocAccessible : public nsHyperTextAccessibleWrap,
>                         public nsIAccessibleDocument,
>                         public nsIDocumentObserver,
>                         public nsIObserver,
>                         public nsIScrollPositionListener,
>-                        public nsSupportsWeakReference
>+                        public nsSupportsWeakReference,
>+                        public nsIAccessibleCursorable

I can think of all sorts of reason snot to add something to the list of class FooAccessible inherits from.

Do you have a reason for not just adding to nsIAccessibleDocument?  Failing that Why not do the worlds silliest tearoff?  The tearoff idea is also somewhat apealing since it would mean we don't increase the size of nsDocAccessible at all.

>+  nsCOMPtr<nsAccessiblePivot> mVirtualCursor;

use nsRefPtr for concrete  objects

>+  bool mNeedsVirtualCursor;

shouldn't be needed

Comment 61

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581827 [details] [diff] [review]
Add EVENT_VIRTUALCURSOR_CHANGED and implement.

again, I'd rather not add interfaces for nsFooAccessible to inherit, but otherwise I think I need the other patches before I can review this.

Comment 62

[Trevor Saunders (:tbsaunde)]

Comment on attachment 582894 [details] [diff] [review]
Mochitests

a few general comments, first these tests only cover some portions of what you add, they don't get either type of event, and probably some other things.

utility js files usually go at the mochitest/ level.

usually the test js and the html it is run on live in the same file, ut I gues that might not be possible here.

usually we use is() not simpleutils.is(), and in general fel free to pull in other of our utility js files.

Comment 63

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Comment 64

[Eitan Isaacson [:eeejay]]

Some clarifications needed below. The rest of your review is incorporated into the patch above.

(In reply to Trevor Saunders (:tbsaunde) from comment #59)
> Comment on attachment 582889 [details] [diff] [review]
> Add nsIAccessiblePivot interface and implement.
> 
> >+  const nsAccessiblePivotBoundary BOUNDARY_CHAR            = 0;
> >+  const nsAccessiblePivotBoundary BOUNDARY_WORD            = 1;
> >+  const nsAccessiblePivotBoundary BOUNDARY_LINE            = 2;
> >+  const nsAccessiblePivotBoundary BOUNDARY_ATTRIBUTE_RANGE = 3;
>
> I think I'd rather just use the boundary constants we already have for nsIAccessibleText.

The boundary constants in the text interface have START/END specifiers that are not revelant to our use here. We could decide
to use the START constants, but that just looks confusing.

> >+  /**
> >+   * Set the pivot's text range in a text accessible.
> >+   *
> >+   * @param aStartOffset the start offset to set.
> >+   * @param aEndOffset the end offset to set.
> >+   * @throws NS_ERROR_NO_INTERFACE when the pivot's accessible does not have the
> >+   *   nsIAccessibleText inteface.
> >+   * @throws NS_ERROR_FAILURE when the offset exceeds the accessible's
> >+   *   character count.
> >+   */
> >+  void setTextRange(in nsIAccessibleText aTextAccessible,
> >+                    in long aStartOffset, in long aEndOffset);
> 
> it seems a little odd this changes the pivit as well as the selection.
> 

The reasons for this:
1. The pivot can be on a non-text accessible, so explicitly requiring to pass an nsIAccessibleText keeps the behavior clear and legality of setting an offset clearer.
2. The alternative would require a two-step operation of changing the position and then the offsets, this would result in two change notifications which could be confusing.

> >+   * Move pivot to next text range.
> >+   *
> >+   * @param aBoundary type of boundary for next text range, character, word, etc.
> >+   * @param aLast traverse to the last occurance in the document.
> 
> the document? I assume you mean this accessible?

I guess "the last occurrence in the root's subtree. Corrected.

> 
> >+   * Move pivot to previous text range.
> >+   *
> >+   * @param aBoundary type of boundary for previous text range, character, word,
> >+   *   etc.
> >+   * @param aFirst traverse to the first occurance in the document.
> 
> this part makes no sense, the argument doesn't appear to exist.
> 
> >+   * @return true on success, false if there are is no more text.
> 
> whatdo these methods do when the pivit isn't a text accessible?
> 

Traverse to first text accessible parent, I think. Or maybe first child, and if none exists, first parent. This is yet to be implemented. I think once it is, there should be a clearer explanation here.

> >+  /* Ignore this accessible object */
> >+  const short FILTER_IGNORE = 0x0;
> 
> but not its children right?
> 
> >+  /* Accept this accessible object */
> >+  const short FILTER_MATCH = 0x1;
> >+  /* Don't traverse accessibles children */
> >+  const short FILTER_IGNORE_SUBTREE = 0x2;
> 
> the accessible itself should also be ignored right?

This is a bitfield, the first bit indicates whether or not the current node matches. The second bit indicates whether the node's subtree should be traversed.

> 
> >+nsAccessiblePivot::GetRootNode(nsIAccessible** aRootNode)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aRootNode);
> >+  NS_IF_ADDREF(*aRootNode = mRootNode);
> 
> is there actually a case in which there isn't a root?  If not I'd say you
> should just use NS_ADDREF()
> 

You could construct a pivot with a root of null. Just being safe here, is the null-check too much?

> 
> >+  nsAccessible* ancestor = aNode;
> 
> no local variable needed

You mean just use aNode? Done, but it looks weird to me. I don't like changing argument variables, actually they should really be const.

> 
> >+nsAccessiblePivot::SetPosition(nsIAccessible* aPosition)
> 
> >+  if (aPosition) {
> >+    nsRefPtr<nsAccessible> position(do_QueryObject(aPosition));
> 
> might fail.
> 
> >+
> >+    if (!IsRootDescendant(position))
> >+      return NS_ERROR_INVALID_ARG; //Accessible is not a descendant of pivot root
> 
> comment isn't really useful.
> 
> >+  mStartOffset = -1;
> >+  mEndOffset = -1;
> 
> m
> StartOffset = mEndOffset = -1;
> 
> also, this means the positions you fire the event with will be wrong.
> 
> >+  NotifyPivotChanged(oldPosition, 
> 
> possible use after free if mPosition was the last reference holder to
> oldPosition which is possible.
> 
> I think I'd do something like
> 
> nsRefPtr<nsAccessible> newPosition;
> if (aPosition)
>   newPosition = do_QueryObject(aPosition);
> 
> if (aPosition && (!newPosition || !IsDecendentOfRoot(newPosition))
>   return NS_ERROR_INVALID_ARG;
> 
> nsRefPtr<nsAccessible> oldPosition = mPosition.forget();
> mPosition = newPosition.forget();
> PRInt32 oldStart = mStartOffset, oldEnd = mOldeEndOffset;
> Notify(oldPosition, oldStart, oldEnd);
> 
> you might be able to replace the two forget()s with a nsRefPtr::swap but I
> don't have a great idea how to name variables that way.
> 

I am a newb to C++ and xpc ref counting, I think I remedied it in the next version of the patch.

> >+
> >+  if (aStartOffset > charCount || aEndOffset > charCount)
> >+    return NS_ERROR_FAILURE;
> >+
> >+  mStartOffset = (aStartOffset <= aEndOffset) ? aStartOffset : aEndOffset;
> >+  mEndOffset = (aEndOffset >= aStartOffset) ? aEndOffset : aStartOffset;
> 
> I tend to think we should be less tolerant here and require 0 <= aStart <=
> aEnd < length

Agreed, aside from being larger than 0. A negative offset signifies no text selected in the pivot. I guess the argument could be made that instead of the the start and end offset should just be 0. But that might be confusing if someone finds a use for collapsed selections in the future.

> >+nsAccessiblePivot::AddObserver(nsIAccessiblePivotObserver *aObserver)
> 
> >+  if (!mObservers.AppendObject(aObserver))
> >+    return NS_ERROR_OUT_OF_MEMORY;
> 
> I would expect allocations here are infalible.
> 

I am emulating every other AddObserver implementation I found. I could remove it if it really is an eye-sore for you.

> >+++ b/accessible/src/base/nsAccessiblePivot.h
> >@@ -0,0 +1,87 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* ***** BEGIN LICENSE BLOCK 
> 
> add vim line.
> 
> >+#include "nsIAccessibleTraversalRule.h"
> 
> do you use this one at all?

Yes, the class is used in some private method prototypes as well as in the cpp file.

> 
> >+private:
> >+  nsAccessiblePivot();
> >+  nsAccessiblePivot(const nsAccessiblePivot&);
> >+  nsAccessiblePivot& operator = (const nsAccessiblePivot&);
> 
> If you're trying to prevent copying you shouldn't provide a impl of
> nsAccessiblePivit() and apparently there is a macro for this somewhere in
> mfbt/
> 

Hopefully remedied this. I am new to copy constructors and the like.

Comment 65

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

removes non-existant files from Makefile.in

Comment 66

[Trevor Saunders (:tbsaunde)]

(In reply to Eitan Isaacson [:eeejay] from comment #64)
> Some clarifications needed below. The rest of your review is incorporated
> into the patch above.
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #59)
> > Comment on attachment 582889 [details] [diff] [review]
> > Add nsIAccessiblePivot interface and implement.
> > 
> > >+  const nsAccessiblePivotBoundary BOUNDARY_CHAR            = 0;
> > >+  const nsAccessiblePivotBoundary BOUNDARY_WORD            = 1;
> > >+  const nsAccessiblePivotBoundary BOUNDARY_LINE            = 2;
> > >+  const nsAccessiblePivotBoundary BOUNDARY_ATTRIBUTE_RANGE = 3;
> >
> > I think I'd rather just use the boundary constants we already have for nsIAccessibleText.
> 
> The boundary constants in the text interface have START/END specifiers that
> are not revelant to our use here. We could decide
> to use the START constants, but that just looks confusing.

true, could we name the type TextBoundaryType? I think it makes it clearer what it is, and WORD_BOUNDARY seems betterthan BOUNDARY_WORD to me though it goes against the general pattern of nsIAccessibleFoo::FOO_SOMETHING, but I think that pattern is a bit silly so I think the clarity / readability is worth it.

> > >+   * Move pivot to previous text range.
> > >+   *
> > >+   * @param aBoundary type of boundary for previous text range, character, word,
> > >+   *   etc.
> > >+   * @param aFirst traverse to the first occurance in the document.
> > 
> > this part makes no sense, the argument doesn't appear to exist.
> > 
> > >+   * @return true on success, false if there are is no more text.
> > 
> > whatdo these methods do when the pivit isn't a text accessible?
> > 
> 
> Traverse to first text accessible parent, I think. Or maybe first child, and
> if none exists, first parent. This is yet to be implemented. I think once it
> is, there should be a clearer explanation here.

then why can't we add these methods to the interface when we want to implement them, instead of now?

> > >+nsAccessiblePivot::GetRootNode(nsIAccessible** aRootNode)
> > >+{
> > >+  NS_ENSURE_ARG_POINTER(aRootNode);
> > >+  NS_IF_ADDREF(*aRootNode = mRootNode);
> > 
> > is there actually a case in which there isn't a root?  If not I'd say you
> > should just use NS_ADDREF()
> > 
> 
> You could construct a pivot with a root of null. Just being safe here, is
> the null-check too much?

I'd think we should return an error in the case of constructing a pivit for null since its useless, and makes us check a lot more.  I don't terribly mind the check, but I don't see  a good reason to add it, and it makes it less clear that there should always be a root.

> > >+  nsAccessible* ancestor = aNode;
> > 
> > no local variable needed
> 
> You mean just use aNode? Done, but it looks weird to me. I don't like
> changing argument variables, actually they should really be const.

I don't really have an objection to changing arguments, but I don't see that is related to constness of what it points to.

btw I would agree you should make it const nsAccessible* aNode instead of nsAccessible* aNode

> > >+
> > >+  if (aStartOffset > charCount || aEndOffset > charCount)
> > >+    return NS_ERROR_FAILURE;
> > >+
> > >+  mStartOffset = (aStartOffset <= aEndOffset) ? aStartOffset : aEndOffset;
> > >+  mEndOffset = (aEndOffset >= aStartOffset) ? aEndOffset : aStartOffset;
> > 
> > I tend to think we should be less tolerant here and require 0 <= aStart <=
> > aEnd < length
> 
> Agreed, aside from being larger than 0. A negative offset signifies no text
> selected in the pivot. I guess the argument could be made that instead of
> the the start and end offset should just be 0. But that might be confusing
> if someone finds a use for collapsed selections in the future.

I thought we were using exactly -1?  I think making both -1 makes sense in that case.  So I would ammend my original assertion to what it was || (mStart == -1  && mEnd == -1)

> 
> > >+nsAccessiblePivot::AddObserver(nsIAccessiblePivotObserver *aObserver)
> > 
> > >+  if (!mObservers.AppendObject(aObserver))
> > >+    return NS_ERROR_OUT_OF_MEMORY;
> > 
> > I would expect allocations here are infalible.
> > 
> 
> I am emulating every other AddObserver implementation I found. I could
> remove it if it really is an eye-sore for you.

a quick look through content/ found me atleast one spot we append to a nsTObserverArray without checking.  I looked, and nsTObserverArray appears to store the observers in a nsTArray which certainly is infalable now, so if you use a nsTObserverArray then the check isn't necessary.  However I'm not familiar with nsCOMArray, and perhaps checking an append to it is needed.

> 
> > >+++ b/accessible/src/base/nsAccessiblePivot.h
> > >@@ -0,0 +1,87 @@
> > >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > >+/* ***** BEGIN LICENSE BLOCK 
> > 
> > add vim line.
> > 
> > >+#include "nsIAccessibleTraversalRule.h"
> > 
> > do you use this one at all?
> 
> Yes, the class is used in some private method prototypes as well as in the
> cpp file.

please forward declare instead of including headers in headers when possible.

Comment 67

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

I think this answers everthing.

Comment 68

[Eitan Isaacson [:eeejay]]

Aside from comments in line, everything is incorporated in the next patch.

(In reply to Trevor Saunders (:tbsaunde) from comment #60)
> Comment on attachment 581826 [details] [diff] [review]
> Add nsIAccessibleCursorable and implement.
> 
> >+// nsIAccessibleVirtualCursor method
> 
> usually sectons are marked by a full line comment then the interface
> something like
> 
> ////////////////////////////////////////////////////////
> // nsFooAccessible: nsIFoo
> 

I was conforming to the style of other interface methods in the file. I could fix this if you think we should just start doing it for new code.

> 
> > class nsDocAccessible : public nsHyperTextAccessibleWrap,
> >                         public nsIAccessibleDocument,
> >                         public nsIDocumentObserver,
> >                         public nsIObserver,
> >                         public nsIScrollPositionListener,
> >-                        public nsSupportsWeakReference
> >+                        public nsSupportsWeakReference,
> >+                        public nsIAccessibleCursorable
> 
> I can think of all sorts of reason snot to add something to the list of
> class FooAccessible inherits from.
> 
> Do you have a reason for not just adding to nsIAccessibleDocument?  Failing
> that Why not do the worlds silliest tearoff?  The tearoff idea is also
> somewhat apealing since it would mean we don't increase the size of
> nsDocAccessible at all.
> 

Don't know what a tearoff is. In any case, the extra interface was proposed by Alex somewhere in the comments here above in his review of the API proposal. I am fine either way.

Comment 69

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Comment 70

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #61)
> Comment on attachment 581827 [details] [diff] [review]
> Add EVENT_VIRTUALCURSOR_CHANGED and implement.
> 
> again, I'd rather not add interfaces for nsFooAccessible to inherit, but
> otherwise I think I need the other patches before I can review this.

This patch does not introduce a new interface and does not depend on any from previous patches. In any case, I saw a small glitch, and I will replace it with a new patch right now.

Comment 71

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Missed one place in your review that I forgot to fix. Sorry.

Comment 72

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

Forgot to add event string to AccessibilityService.

Comment 73

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add pivot factory function to nsIAccessibleRetrieval and implement.

Fixed as per review.

Comment 74

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Updated tests, event driven. Does a simple setTextRange smoke test as well.

Comment 75

[Trevor Saunders (:tbsaunde)]

Comment on attachment 585060 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.


>+  const nsAccessiblePivotBoundary CHAR_BOUNDARY            = 0;
>+  const nsAccessiblePivotBoundary WORD_BOUNDARY            = 1;
>+  const nsAccessiblePivotBoundary LINE_BOUNDARY            = 2;
>+  const nsAccessiblePivotBoundary ATTRIBUTE_RANGE_BOUNDARY = 3;

update the types here

>+   * The root past which the pivot cannot traverse.

the root accessible outside of whose subtree the position can't move?

>+   * The start offset in the accessible's text. Only supported when the accessible
>+   * implments the nsIAccessibleText interface. If no explicit offset was set or

implements

>+   * Set the pivot's text range in a text accessible.
>+   *
>+   * @param aStartOffset the start offset to set.
>+   * @param aEndOffset the end offset to set.

what about the pivit arg?  (I'm not sure this sort of comment is always that useful myself, but I don't mind if its consistant)

>+   * @throws NS_ERROR_NO_INTERFACE when the pivot's accessible does not have the
>+   *   nsIAccessibleText inteface.

the typesystem doesn't allow this to happen so I'm not sure what you mean here.  Unless xpconnect throws that if you try to call from js with something that isn't a nsIAccessibleText, but I would say if that is the case you shouldn't mention it here.

>+   * @throws NS_ERROR_FAILURE when the offset exceeds the accessible's
>+   *   character count.

shouldn't that be invalid arg?

>+nsAccessiblePivot::nsAccessiblePivot(nsAccessible* aRootNode) :
>+  mRootNode(aRootNode), mPosition(nsnull), mStartOffset(-1), mEndOffset(-1)
>+{
>+  NS_ABORT_IF_FALSE(aRootNode, "A root node is required");

use NS_ASSERTION, there's no reason to crash until we need to use mRoot for something.

>+nsAccessiblePivot::SetPosition(nsIAccessible* aPosition)
>+{
>+  nsRefPtr<nsAccessible> aSecondPosition = nsnull;

secondPosition, don't assign nsnull, default constructor is called which is fine.

>+  mPosition.swap(aSecondPosition);
>+
>+  NotifyPivotChanged(aSecondPosition, mStartOffset, mEndOffset);
>+
>+  mStartOffset = mEndOffset = -1;

current offsets are wrong when observers are called.

btw all the blank lines are a little much

>+  // Start offset must be before end offset.

obvious no?

>+  if (aStartOffset > aEndOffset)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  // The only legal negative value is -1, and both start and end must be equal.
>+  if (aStartOffset < 0 && (aStartOffset != -1 || aEndOffset != -1))
>+    return NS_ERROR_INVALID_ARG;

why not use the macro?

this also allows the special case of aStart == aEnd == 0 which I suspect you don't want.

>+  nsRefPtr<nsHyperTextAccessible> aPosition = do_QueryObject(aTextAccessible);

position, or maybe newPosition

>+  if (aStartOffset > charCount || aEndOffset > charCount)
>+    return NS_ERROR_FAILURE;

you should be able to only check aEndOffset

>+
>+  PRInt32 oldStart = mStartOffset;
>+  PRInt32 oldEnd = mEndOffset;

PRInt32 oldStart = mStart, oldEnd = mEnd;

>+  *aResult = (node && !(NS_FAILED(rv)));

just !NS_FAILED()

>+  return SetPosition(node);

that's a litle heavy weight since you don't really need the virtual call for the function it self and then QI, or the IsRootDescendant() check, but I gues open coding what you need is a little ugly to so if you can't think of a better way I can live with it.

>+  NS_ENSURE_ARG(aResult);
>+  NS_ENSURE_ARG(aRule);
>+  nsresult rv = NS_OK;

usually we put a blank line after the arg checks, and before the main body if the method isn't absolutely trivial.

>+  short filtered = nsIAccessibleTraversalRule::FILTER_MATCH;

please declare the first place you need it instead of here.

>+  if (!aNode)
>+    return nsnull;

can it ever be null?

>+  while (aNode != mRootNode) {

it would be nice to have a comment explaining this algorithm since it isn't completely trivial.

>+      if (*rv != NS_OK)

that's not the only success return code, always use the macros.

>+      while ((filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE) == 0 

!(filter & blah) &&

>+        if (*rv != NS_OK)

again, use the macros

>+      if (filtered & nsIAccessibleTraversalRule::FILTER_MATCH) {
>+        return aNode;
>+      }

get rid of braces
>+nsAccessiblePivot::SearchForward(nsAccessible* aNode,
>+                                 nsIAccessibleTraversalRule* aRule,
>+                                 nsresult* rv)
>+{
>+  *rv = NS_OK;
>+
>+  short filtered;

don't declare before you use.

>+  *rv = ApplyFilter(aNode, aRule, &filtered);
>+  if (*rv != NS_OK)

same

>+  while (true) {
>+    nsAccessible* firstChild;
>+    while ((filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE) == 0 &&

!(filter & blah)

>+           (firstChild = aNode->FirstChild())) {
>+      aNode = firstChild;
>+      *rv = ApplyFilter(aNode, aRule, &filtered);
>+      if (*rv != NS_OK)
>+        return nsnull;
>+
>+      if (filtered & nsIAccessibleTraversalRule::FILTER_MATCH)
>+        return aNode;
>+
>+      firstChild = aNode->FirstChild();

doing this at the top and bottum of the loop seems fishy
>+      temp = temp->Parent();
>+    } while (temp);

you could do while (temp = temp->Parent()); no?
>+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) != 0 &&
>+        state & states::DEFUNCT)

(preFilter & blah) && (state & states::foo)

>+    if ((nsIAccessibleTraversalRule::PREFILTER_INVISIBLE & preFilter) != 0 &&
>+        state & states::INVISIBLE)

same

>+      return NS_OK;
>+
>+    if ((nsIAccessibleTraversalRule::PREFILTER_OFFSCREEN & preFilter) != 0 &&
>+        state & states::OFFSCREEN)

same

>+  PRUint32 length;

probably should initialize it

>+  acceptRoles->GetLength(&length);

its a bit  silly, but in theory could fail

>+  if (length > 0) {
>+    PRUint32 nodeRole = aNode->Role();
>+    bool matchesRole = false;
>+    rv = acceptRoles->GetLength(&length);

you already have the length, also don't set something to the return value, and then ignore it.

>+    for (PRUint32 itemIndex = 0; itemIndex < length; itemIndex++) {

just idx

>+      nsCOMPtr<nsISupportsPRUint32> role = do_QueryElementAt(acceptRoles,
>+                                                             itemIndex);
>+      if (!role)
>+        continue;
>+
>+      PRUint32 roleData;
>+      role->GetData(&roleData);
>+      matchesRole = roleData == nodeRole;
>+      if (matchesRole)
>+        break;

why not just return?

>+nsAccessiblePivot::AddObserver(nsIAccessiblePivotObserver* aObserver)
>+{
>+  NS_ENSURE_ARG(aObserver);
>+
>+  mObservers.AppendElementUnlessExists(aObserver);

wanting to be notified twice seems sort of reasonable but silly.  If you want to make sure observers are unique in the array you should throw if its already there.

 also I'm concerned by the fact that  you aren't holding owning references to the observers.  The easy answer is I believe nsTObserverArray<nsCOMPtr<nsIAccessiblePivitObserver> >.  You could manually AddRef / Release in add observer / remove observer too, but that seems  harder.

Alex do you know if we need more cycle collector macros for this? it seems like we might, but I'll admitt to not really understanding them.

>+nsAccessiblePivot::NotifyPivotChanged(nsAccessible* aOldPosition,
>+                                      PRInt32 aOldStart, PRInt32 aOldEnd)
>+{
>+  nsTObserverArray<nsIAccessiblePivotObserver*>::ForwardIterator iter(mObservers);
>+  while (iter.HasMore()) {
>+    iter.GetNext()->OnPivotChanged(this, aOldPosition, aOldStart, aOldEnd);
>+  }

I think you can use the macro at the bottum of nsTObserverArray.h here

>+#include "nsIAccessibleTraversalRule.h"

isn't forward declaring the class enough?

>+#include "nsAccessible.h"

same
>+  nsAccessible *Position() { return mPosition; }

is this used anywhere? I'd suggest just getting rid of it unless you need it for something I'm missing. Atleas don't put it right next to the constructor, that's confusing.
>+  nsAccessiblePivot() MOZ_DELETE;
>+  void operator = (const nsAccessiblePivot&) MOZ_DELETE;

you forgot the copy constructor one nsAccessiblePivit(const nsAccessiblePivit&)

>+  nsresult ApplyFilter(nsAccessible *aNode, nsIAccessibleTraversalRule* aRule,
>+                       short *aResult);
>+  nsRefPtr<nsAccessible> mRootNode;

usually we put a blank line atleast between methods and data

Comment 76

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #75)
> Comment on attachment 585060 [details] [diff] [review]
> Bug 698823 - Add nsIAccessiblePivot interface and implement.
> 
> 
> >+  if (aStartOffset > aEndOffset)
> >+    return NS_ERROR_INVALID_ARG;
> >+
> >+  // The only legal negative value is -1, and both start and end must be equal.
> >+  if (aStartOffset < 0 && (aStartOffset != -1 || aEndOffset != -1))
> >+    return NS_ERROR_INVALID_ARG;
> 
> why not use the macro?

I used a macro and described the conditional since it is a bit involved.

> 
> this also allows the special case of aStart == aEnd == 0 which I suspect you
> don't want.
> 

That is fine. Collapsed ranges are allowed.

> 
> >+  return SetPosition(node);
> 
> that's a litle heavy weight since you don't really need the virtual call for
> the function it self and then QI, or the IsRootDescendant() check, but I
> gues open coding what you need is a little ugly to so if you can't think of
> a better way I can live with it.
> 

Made a convenience function for this since it is called internally times.

> >+  NS_ENSURE_ARG(aResult);
> >+  NS_ENSURE_ARG(aRule);
> >+  nsresult rv = NS_OK;
> 
> usually we put a blank line after the arg checks, and before the main body
> if the method isn't absolutely trivial.
> 
> >+  short filtered = nsIAccessibleTraversalRule::FILTER_MATCH;
> 
> please declare the first place you need it instead of here.
> 

Initializing to 0 instead. It needs to be declared outside of the loop's scope since it should retain the value from the last iteration. 

> >+  if (!aNode)
> >+    return nsnull;
> 
> can it ever be null?

Yes, it could be passed mPosition which could legally be null. In the search backwards context it means we are implicitly on the root node, and the root node is the first node in the search order, so there are no matches behind it.

> 
> >+      nsCOMPtr<nsISupportsPRUint32> role = do_QueryElementAt(acceptRoles,
> >+                                                             itemIndex);
> >+      if (!role)
> >+        continue;
> >+
> >+      PRUint32 roleData;
> >+      role->GetData(&roleData);
> >+      matchesRole = roleData == nodeRole;
> >+      if (matchesRole)
> >+        break;
> 
> why not just return?

The role matching is just a pre-filter, still need to apply the actual filter.

> 
> >+nsAccessiblePivot::AddObserver(nsIAccessiblePivotObserver* aObserver)
> >+{
> >+  NS_ENSURE_ARG(aObserver);
> >+
> >+  mObservers.AppendElementUnlessExists(aObserver);
> 
> wanting to be notified twice seems sort of reasonable but silly.  If you
> want to make sure observers are unique in the array you should throw if its
> already there.
> 

If this were the first observer pattern in the mozilla code base, I would say adding it once and failing silently other times is the right behavior, but it seems that other AddObserver methods don't check if the observer is in the list. So I guess that is the expected behavior. Although I think it opens the door for some ugly bugs in the future since adding the same observer more than once is almost never desired. Bottom line: I changed it not to check for a preexisting observer.

>  also I'm concerned by the fact that  you aren't holding owning references
> to the observers.  The easy answer is I believe
> nsTObserverArray<nsCOMPtr<nsIAccessiblePivitObserver> >.  You could manually
> AddRef / Release in add observer / remove observer too, but that seems 
> harder.

I'll try that.

> 
> Alex do you know if we need more cycle collector macros for this? it seems
> like we might, but I'll admitt to not really understanding them.
> 

I'll admit that too :)

> >+nsAccessiblePivot::NotifyPivotChanged(nsAccessible* aOldPosition,
> >+                                      PRInt32 aOldStart, PRInt32 aOldEnd)
> >+{
> >+  nsTObserverArray<nsIAccessiblePivotObserver*>::ForwardIterator iter(mObservers);
> >+  while (iter.HasMore()) {
> >+    iter.GetNext()->OnPivotChanged(this, aOldPosition, aOldStart, aOldEnd);
> >+  }
> 
> I think you can use the macro at the bottum of nsTObserverArray.h here

Not after changing the type to nsTObserverArray<nsCOMPtr<nsIAccessiblePivitObserver> >

> >+  nsAccessible *Position() { return mPosition; }
> 
> is this used anywhere? I'd suggest just getting rid of it unless you need it
> for something I'm missing. Atleas don't put it right next to the
> constructor, that's confusing.

I'll put a blank line in between, it comes in handy for the Android stuff I am implementing.

Comment 77

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Comment 78

[Trevor Saunders (:tbsaunde)]

Comment on attachment 585092 [details] [diff] [review]
Bug 698823 - Add nsIAccessibleCursorable and implement.


>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>index f4d8bcf..fe9dcd3 10064
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -39,16 +39,17 @@
> #include "AccIterator.h"
> #include "States.h"
> #include "nsAccCache.h"
> #include "nsAccessibilityService.h"
> #include "nsAccTreeWalker.h"
> #include "nsAccUtils.h"
> #include "nsRootAccessible.h"
> #include "nsTextEquivUtils.h"
>+#include "nsAccessiblePivot.h"

try to add headers in order

>+  // We provide a virtual cursor if this is a parent doc or if it's parent content
>+  // element is <browser/>
>+  nsIDocument *parent = mDocument->GetParentDocument();
>+  if (!parent ||
>+      parent->FindContentForSubDocument(mDocument)->Tag() == nsGkAtoms::browser) {
>+    // Initialize virtual cursor if this document manages it.

not really needed, comment above if should be fine

>+nsDocAccessible::GetVirtualCursor(nsIAccessiblePivot **aVirtualCursor) {
>+  NS_ENSURE_ARG_POINTER(aVirtualCursor);
>+
>+  *aVirtualCursor = nsnull;

no blank line

>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+

only one blank line here

>+  NS_ADDREF(*aVirtualCursor = mVirtualCursor);
>+
>+  return NS_OK;

you don't really need that one either

> #include "nsIAccessibleDocument.h"
>+#include "nsIAccessibleCursorable.h"

keep these in order too

r=me with that fixed

Comment 79

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Here is the patch with the small nits. It depends on the other patches so it will live here until it is committed to mc.

Comment 80

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

Added event to MSAA mapping

Comment 81

[Trevor Saunders (:tbsaunde)]

Comment on attachment 585559 [details] [diff] [review]
Bug 698823 - Add nsIAccessibleCursorable and implement.


>   NS_INTERFACE_MAP_ENTRY(nsIObserver)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleDocument)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIAccessibleCursorable,
>+                                     mVirtualCursor)

doesn't that fit on one line?

>+nsDocAccessible::GetVirtualCursor(nsIAccessiblePivot **aVirtualCursor) {
>+  NS_ENSURE_ARG_POINTER(aVirtualCursor);
>+  *aVirtualCursor = nsnull;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;

if you do this which I think you should you might as well make mVirtualCurser null in ::Shutdown

btw we don't usually bother setting r+ on final patches it seems more confusing than useful.

Comment 82

[[e-mail]]

Comment on attachment 585550 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.

>+  } while ((aNode = aNode->Parent()));
Personally I prefer the nsContentUtils approach:
  aNode = aNode->Parent();
} while (aNode);

>+  nsresult rv = NS_OK;
>+  nsAccessible* node = SearchForward(mPosition, aRule, &rv);
>+
>+  NS_ENSURE_TRUE(*aResult = (node && !NS_FAILED(rv)), rv);
>+
>+  MovePivotInternal(node);
This looks very wrong, especially the NS_ENSURE_TRUE bit. I think you would be better off if SearchForward called MovePivotInternal.

>+  if (!aNode)
>+    return nsnull;
You never pass in a null node. You could assert that if you want though.

>+    if (aNode == mRootNode)
>+      break;
I don't see how that can happen, but nsTreeWalker.cpp does it to, so...

>+  if (!aNode)
>+    aNode = mRootNode;
Again, you never pass in a null node.

>+  PRUint32 preFilter = nsnull;
nsnull isn't a PRUint32

>+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) &&
>+        (state & states::DEFUNCT))
Can we not arrange for PREFILTER_DEFUNCT to equal DEFUNCT (etc.), so that we can test for state & preFilter directly?

>+  nsCOMPtr<nsIArray> acceptRoles;
>+  rv = aRule->GetMatchRoles(getter_AddRefs(acceptRoles));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  PRUint32 length = 0;
>+  rv = acceptRoles->GetLength(&length);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (length > 0) {
>+    PRUint32 nodeRole = aNode->Role();
>+    bool matchesRole = false;
>+    for (PRUint32 idx = 0; idx < length; idx++) {
>+      nsCOMPtr<nsISupportsPRUint32> role = do_QueryElementAt(acceptRoles, idx);
It really sucks to have to do this all the time. Although, returning an integer array though JavaScript sucks anyway*. Is it possible to cache the PRUint32 array in advance? (And possibly the preFilter too.)

*Instead of an attribute, you would have to use a getter, something like this:
void getMatchRoles([array, size_is(length)]out unsigned long roles,
                   [retval]out unsigned long length);

getMatchRoles: function(roles) {
  roles.value = [1, 2, 3, 4]; // or whatever
  return roles.value.length;
}

PRUint32 length, *roles;
rv = acceptRules->GetMatchRoles(&roles, &length);
...
NS_Free(roles);

Comment 83

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #82)
> Comment on attachment 585550 [details] [diff] [review]
> Bug 698823 - Add nsIAccessiblePivot interface and implement.
> 
> >+  } while ((aNode = aNode->Parent()));
> Personally I prefer the nsContentUtils approach:
>   aNode = aNode->Parent();
> } while (aNode);

while (foo = foo->Something()); is pretty much the rule in accessible/ fwiw

> >+  nsresult rv = NS_OK;
> >+  nsAccessible* node = SearchForward(mPosition, aRule, &rv);
> >+
> >+  NS_ENSURE_TRUE(*aResult = (node && !NS_FAILED(rv)), rv);
> >+
> >+  MovePivotInternal(node);
> This looks very wrong, especially the NS_ENSURE_TRUE bit. I think you would
> be better off if SearchForward called MovePivotInternal.

that seems like a reasonable improvement on its own  though I would probably rename to MoveForward / Backward

btw what seems wrong? its certainly weird, but I believe it is technically correct

> >+  if (!aNode)
> >+    return nsnull;
> You never pass in a null node. You could assert that if you want though.

I'm pretty sure you can, by doing

SetPosition(nul) // legal though perhaps doesn't make sense
MoveForward() // perhaps an  an error? but technically legal now

> >+    if (aNode == mRootNode)
> >+      break;
> I don't see how that can happen, but nsTreeWalker.cpp does it to, so...

I was wondering that too, unfortunately  surkov won't be around for a week or so to tell us why nsAccTreeWalker does it.

> >+  if (!aNode)
> >+    aNode = mRootNode;
> Again, you never pass in a null node.

again I'm pretty sure you can though maybe we should outlaw what lets you make that happen

> >+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) &&
> >+        (state & states::DEFUNCT))
> Can we not arrange for PREFILTER_DEFUNCT to equal DEFUNCT (etc.), so that we
> can test for state & preFilter directly?

I think that would end being hard, the state bit mask is aligned such that states::FOO  maps directly to nsIAccessileStates::STATE_FOO module the bit for extra states since we can't pass a 64 bit object through idl.  Then note in States.h DEFUNCT is 1 << 32, so not representable in a 32 bit int.  We also have the nsIAccessibleStates  constants arranged to line up with the bit mask for msaa for the lower 32 bit word.

> >+  nsCOMPtr<nsIArray> acceptRoles;
> >+  rv = aRule->GetMatchRoles(getter_AddRefs(acceptRoles));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  PRUint32 length = 0;
> >+  rv = acceptRoles->GetLength(&length);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  if (length > 0) {
> >+    PRUint32 nodeRole = aNode->Role();
> >+    bool matchesRole = false;
> >+    for (PRUint32 idx = 0; idx < length; idx++) {
> >+      nsCOMPtr<nsISupportsPRUint32> role = do_QueryElementAt(acceptRoles, idx);
> It really sucks to have to do this all the time. Although, returning an
> integer array though JavaScript sucks anyway*. Is it possible to cache the
> PRUint32 array in advance? (And possibly the preFilter too.)
> 
> *Instead of an attribute, you would have to use a getter, something like
> this:
> void getMatchRoles([array, size_is(length)]out unsigned long roles,
>                    [retval]out unsigned long length);

yeah, I asked about that in an earlier comment too, it sort of sucks you can't have an attribute of that form, is there a reason for that?

It probably would be good for you to take a look at the rest of the API added in the follow on patches if you don't mind, I can sr? you on them  too if you like or you can just look.  

The basic idea is adding something sort of like focus to the accessibility APIs if you hadn't looked to see that, which  I'm not convinced is a great idea, but  I seem to be the only one to feel that way so I'l go along with it.

Comment 84

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #83)
> (In reply to [e-mail] from comment #82)
> > Comment on attachment 585550 [details] [diff] [review]
> > Bug 698823 - Add nsIAccessiblePivot interface and implement.
> > 
> > >+  } while ((aNode = aNode->Parent()));
> > Personally I prefer the nsContentUtils approach:
> >   aNode = aNode->Parent();
> > } while (aNode);
> 
> while (foo = foo->Something()); is pretty much the rule in accessible/ fwiw
> 

Uh oh.. do I follow the reviewer or super reviewer? :)

> > >+  nsresult rv = NS_OK;
> > >+  nsAccessible* node = SearchForward(mPosition, aRule, &rv);
> > >+
> > >+  NS_ENSURE_TRUE(*aResult = (node && !NS_FAILED(rv)), rv);
> > >+
> > >+  MovePivotInternal(node);
> > This looks very wrong, especially the NS_ENSURE_TRUE bit. I think you would
> > be better off if SearchForward called MovePivotInternal.
> 
> that seems like a reasonable improvement on its own  though I would probably
> rename to MoveForward / Backward
> 
> btw what seems wrong? its certainly weird, but I believe it is technically
> correct

I would like to keep the search functions separate from the moving functions. I think they might come in handy in the future when we implement text nav, or when we have a need to count matches of a given rule without actually traversing. I agree that the way I used NS_ENSURE_TRUE is a horrible. I could fix all that.

> 
> > >+  if (!aNode)
> > >+    return nsnull;
> > You never pass in a null node. You could assert that if you want though.
> 
> I'm pretty sure you can, by doing
> 
> SetPosition(nul) // legal though perhaps doesn't make sense
> MoveForward() // perhaps an  an error? but technically legal now
> 

Yep, it could be null. If it is, than searching backwards would always fail.

> > >+    if (aNode == mRootNode)
> > >+      break;
> > I don't see how that can happen, but nsTreeWalker.cpp does it to, so...
> 
> I was wondering that too, unfortunately  surkov won't be around for a week
> or so to tell us why nsAccTreeWalker does it.
> 

Yeah, I copied some of that from the tree walker, and yeah. I don't see the point for it. I bet Alex copied it too. I could remove it, it passes the tests I set up without it.

> > >+  if (!aNode)
> > >+    aNode = mRootNode;
> > Again, you never pass in a null node.
> 
> again I'm pretty sure you can though maybe we should outlaw what lets you
> make that happen

Yeah, mPosition could be null, so SearchForward could get a null value. This is the second or third time this has come up, so maybe I should do:
SearchForward(mPosition || mRootNode, aRule, &rv);

> 
> > >+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) &&
> > >+        (state & states::DEFUNCT))
> > Can we not arrange for PREFILTER_DEFUNCT to equal DEFUNCT (etc.), so that we
> > can test for state & preFilter directly?
> 
> I think that would end being hard, the state bit mask is aligned such that
> states::FOO  maps directly to nsIAccessileStates::STATE_FOO module the bit
> for extra states since we can't pass a 64 bit object through idl.  Then note
> in States.h DEFUNCT is 1 << 32, so not representable in a 32 bit int.  We
> also have the nsIAccessibleStates  constants arranged to line up with the
> bit mask for msaa for the lower 32 bit word.
> 

Yes, previous iterations of the API did this with states directly. The problem is in the nuances between a "whitelist" mask and a "blacklist" mask, and the two distinct 32 bit bitfields. It was getting confusing and cumbersome, so we opted for a simpler API since we already know the handful of states we want to prefilter for.

> > >+  nsCOMPtr<nsIArray> acceptRoles;
> > >+  rv = aRule->GetMatchRoles(getter_AddRefs(acceptRoles));
> > >+  NS_ENSURE_SUCCESS(rv, rv);
> > >+  PRUint32 length = 0;
> > >+  rv = acceptRoles->GetLength(&length);
> > >+  NS_ENSURE_SUCCESS(rv, rv);
> > >+  if (length > 0) {
> > >+    PRUint32 nodeRole = aNode->Role();
> > >+    bool matchesRole = false;
> > >+    for (PRUint32 idx = 0; idx < length; idx++) {
> > >+      nsCOMPtr<nsISupportsPRUint32> role = do_QueryElementAt(acceptRoles, idx);
> > It really sucks to have to do this all the time. Although, returning an
> > integer array though JavaScript sucks anyway*. Is it possible to cache the
> > PRUint32 array in advance? (And possibly the preFilter too.)
> > 
> > *Instead of an attribute, you would have to use a getter, something like
> > this:
> > void getMatchRoles([array, size_is(length)]out unsigned long roles,
> >                    [retval]out unsigned long length);
> 
> yeah, I asked about that in an earlier comment too, it sort of sucks you
> can't have an attribute of that form, is there a reason for that?

This is a very good call. Both the method as opposed to attribute, and the caching. I'll implement that. Trevor, when you first suggested this, I couldn't find how to do it.

Comment 85

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Changes after super review. Added comments and clarified code where it was
ambiguous. Changed the matched roles attribute to a method, and cache it during traversal to avoid roundtripping to JS when not needed.

Comment 86

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Updated test to use new match roles method.

Comment 87

[[e-mail]]

(In reply to Trevor Saunders from comment #83)
> (In reply to comment #82)
> > (From update of attachment 585550 [details] [diff] [review])
> while (foo = foo->Something()); is pretty much the rule in accessible/ fwiw
Fair enough.

> > >+  NS_ENSURE_TRUE(*aResult = (node && !NS_FAILED(rv)), rv);
> > This looks very wrong, especially the NS_ENSURE_TRUE bit.
> btw what seems wrong?
&& returns a boolean in C++

> > >+  if (!aNode)
> > >+    return nsnull;
> > You never pass in a null node. You could assert that if you want though.
> I'm pretty sure you can, by doing
> SetPosition(nul) // legal though perhaps doesn't make sense
Ah, I didn't notice that it was legal.

> > >+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) &&
> > >+        (state & states::DEFUNCT))
> > Can we not arrange for PREFILTER_DEFUNCT to equal DEFUNCT (etc.), so that we
> > can test for state & preFilter directly?
> I think that would end being hard, the state bit mask is aligned such that
> states::FOO  maps directly to nsIAccessileStates::STATE_FOO module the bit
> for extra states since we can't pass a 64 bit object through idl.  Then note
> in States.h DEFUNCT is 1 << 32, so not representable in a 32 bit int.  We
> also have the nsIAccessibleStates  constants arranged to line up with the
> bit mask for msaa for the lower 32 bit word.
Maybe we can bitshift state to make it easier to mask with the preFilter?

> > *Instead of an attribute, you would have to use a getter, something like
> > this:
> > void getMatchRoles([array, size_is(length)]out unsigned long roles,
> >                    [retval]out unsigned long length);
> yeah, I asked about that in an earlier comment too, it sort of sucks you
> can't have an attribute of that form, is there a reason for that?
Nowhere to put the length.

> It probably would be good for you to take a look at the rest of the API
> added in the follow on patches if you don't mind, I can sr? you on them  too
> if you like or you can just look.
I won't remember if you don't sr? me.

(In reply to Eitan Isaacson from comment #84)
> (In reply to Trevor Saunders from comment #83)
> > (In reply to comment #82)
> > > (From update of attachment 585550 [details] [diff] [review])
> Uh oh.. do I follow the reviewer or super reviewer? :)
The module owner, of course.

> > > >+    if (aNode == mRootNode)
> > > >+      break;
> > > I don't see how that can happen, but nsTreeWalker.cpp does it to, so...
> > I was wondering that too, unfortunately  surkov won't be around for a week
> > or so to tell us why nsAccTreeWalker does it.
> Yeah, I copied some of that from the tree walker, and yeah. I don't see the
> point for it. I bet Alex copied it too.
Probably from nsTreeWalker.cpp as I suggested in the first place...

Comment 88

[[e-mail]]

(In reply to Trevor Saunders from comment #83)
> (In reply to comment #82)
> > (From update of attachment 585550 [details] [diff] [review])
> > >+    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & preFilter) &&
> > >+        (state & states::DEFUNCT))
> > Can we not arrange for PREFILTER_DEFUNCT to equal DEFUNCT (etc.), so that we
> > can test for state & preFilter directly?
> I think that would end being hard, the state bit mask is aligned such that
> states::FOO  maps directly to nsIAccessileStates::STATE_FOO module the bit
> for extra states since we can't pass a 64 bit object through idl.  Then note
> in States.h DEFUNCT is 1 << 32, so not representable in a 32 bit int.  We
> also have the nsIAccessibleStates  constants arranged to line up with the
> bit mask for msaa for the lower 32 bit word.
Potential idea: query the prefilter once, convert it from the ATR flags into the state flags, then cache the result, like you do with the match roles.

Comment 89

[[e-mail]]

(In reply to [e-mail] from comment #87)
> (In reply to Trevor Saunders from comment #83)
> > (In reply to comment #82)
> > > (From update of attachment 585550 [details] [diff] [review])
> > > >+  NS_ENSURE_TRUE(*aResult = (node && !NS_FAILED(rv)), rv);
> > > This looks very wrong, especially the NS_ENSURE_TRUE bit.
> > btw what seems wrong?
> && returns a boolean in C++
D'oh, I hadn't realised that the result is a boolean... I was still thinking of nsIDOMTreeWalker, whose result is the actual next node (same as currentNode).

Comment 90

[[e-mail]]

Comment on attachment 587808 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.

>+  *aResult = node;
[It took me some time to realise what this was doing...]

>+  // Search backwards from last node and find the last occurance in the doc.
occurrence

>+  *aResult = matchedNode;
Hmm, inconsistent variable naming ;-)

>+  // Initial position could be not set, in that case return null.
s/not set/unset/ ?

>+  PRUint32 *acceptRoles;
>+  PRUint32 acceptRolesLength;
>+  *rv = aRule->GetMatchRoles(&acceptRoles, &acceptRolesLength);
>+  NS_ENSURE_SUCCESS(*rv, nsnull);
So, after this point, you have to be very careful not to leak acceptRoles. You're currently doing this using some gotos, but unfortunately you forgot about your NS_ENSURE_SUCCESSes. Offhand I can't actually find a class that will NS_Free your pointer for you automatically though. One idea I did have was a class that encapsulates the retrieval of the preFilter and the matchRoles, then you can have one instance in SearchForward and one in SearchBackward, e.g.
FilterCache cache(aRule, rv);
NS_ENSURE_SUCCESS(*rv, nsnull);
...
cache.ApplyFilter(aNode);
NS_ENSURE_SUCCESS(*rv, nsnull);
if (cache.filtered & nsIAccessibleTraversalRule::FILTER_MATCH)
  return aNode;
etc.

>+    ApplyFilter(aNode, aRule, acceptRoles, acceptRolesLength, &filtered);
Missing *rv check here (I didn't look for others).

>+    if ((nsIAccessibleTraversalRule::PREFILTER_NOT_FOCUSABLE & preFilter) &&
>+        !(state & states::FOCUSABLE))
(In regards to my prefilter state flag caching idea, you would need to XOR the state with states::FOCUSABLE before ANDing it with the cache.)

Comment 91

[Eitan Isaacson [:eeejay]]

(In reply to [e-mail] from comment #90)
> Comment on attachment 587808 [details] [diff] [review]
> >+    if ((nsIAccessibleTraversalRule::PREFILTER_NOT_FOCUSABLE & preFilter) &&
> >+        !(state & states::FOCUSABLE))
> (In regards to my prefilter state flag caching idea, you would need to XOR
> the state with states::FOCUSABLE before ANDing it with the cache.)

All great input that I will include in an updated patch soon. I was thinking of having an encapsulating class, but thought it might be overkill, not anymore!

As for the prefilter-to-state flags idea, I think it sacrifices readability for just a few less simple operations. Especially if we are going to be XORing. In the future the flags might get a bit more complicated than just checking for a single state bit, so I would prefer to keep it in it's current style.

Comment 92

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

This does some of the nits from Neil's last review and the encapsulating filter cache.

Comment 93

[Trevor Saunders (:tbsaunde)]

> 
> >+  PRUint32 *acceptRoles;
> >+  PRUint32 acceptRolesLength;
> >+  *rv = aRule->GetMatchRoles(&acceptRoles, &acceptRolesLength);
> >+  NS_ENSURE_SUCCESS(*rv, nsnull);
> So, after this point, you have to be very careful not to leak acceptRoles.
> You're currently doing this using some gotos, but unfortunately you forgot
> about your NS_ENSURE_SUCCESSes. Offhand I can't actually find a class that
> will NS_Free your pointer for you automatically though. One idea I did have

I think we could make nsAutoPtr do this if we add a template arg of what function to use to free no?  We'd probably want the arg to default to delete

Comment 94

[Trevor Saunders (:tbsaunde)]

> 
> > > *Instead of an attribute, you would have to use a getter, something like
> > > this:
> > > void getMatchRoles([array, size_is(length)]out unsigned long roles,
> > >                    [retval]out unsigned long length);
> > yeah, I asked about that in an earlier comment too, it sort of sucks you
> > can't have an attribute of that form, is there a reason for that?
> Nowhere to put the length.

so,  "we haven't thought about it / haven't spent time on it" is imho  a reasonable explanation, but I'm not sure I understand the issue with an attribute being a tuple of too things which are really conceptually one thing since I'd tend to think ideally xpconnect would deal with the length for you in js, and in C++ I don't really see why you can't have a attribute setter that takes two arguments.

Comment 95

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #93)
> > 
> > >+  PRUint32 *acceptRoles;
> > >+  PRUint32 acceptRolesLength;
> > >+  *rv = aRule->GetMatchRoles(&acceptRoles, &acceptRolesLength);
> > >+  NS_ENSURE_SUCCESS(*rv, nsnull);
> > So, after this point, you have to be very careful not to leak acceptRoles.
> > You're currently doing this using some gotos, but unfortunately you forgot
> > about your NS_ENSURE_SUCCESSes. Offhand I can't actually find a class that
> > will NS_Free your pointer for you automatically though. One idea I did have
> 
> I think we could make nsAutoPtr do this if we add a template arg of what
> function to use to free no?  We'd probably want the arg to default to delete

My latest patch introduces a new cache object that I think encapsulates the rules bit nicely and lives on the heap for extra cleanup bonus.

Comment 96

[Trevor Saunders (:tbsaunde)]

(In reply to Eitan Isaacson [:eeejay] from comment #95)
> (In reply to Trevor Saunders (:tbsaunde) from comment #93)
> > > 
> > > >+  PRUint32 *acceptRoles;
> > > >+  PRUint32 acceptRolesLength;
> > > >+  *rv = aRule->GetMatchRoles(&acceptRoles, &acceptRolesLength);
> > > >+  NS_ENSURE_SUCCESS(*rv, nsnull);
> > > So, after this point, you have to be very careful not to leak acceptRoles.
> > > You're currently doing this using some gotos, but unfortunately you forgot
> > > about your NS_ENSURE_SUCCESSes. Offhand I can't actually find a class that
> > > will NS_Free your pointer for you automatically though. One idea I did have
> > 
> > I think we could make nsAutoPtr do this if we add a template arg of what
> > function to use to free no?  We'd probably want the arg to default to delete
> 
> My latest patch introduces a new cache object that I think encapsulates the
> rules bit nicely and lives on the heap for extra cleanup bonus.

I  pointed that out since it might be a useful idea even if we don't need it here.

btw I assume you meant stack since that's what you did.

Comment 97

[Trevor Saunders (:tbsaunde)]

Comment on attachment 588980 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.

>+
>+#include "nsAccessiblePivot.h"
>+
>+#include "nsAccUtils.h"
>+#include "nsHyperTextAccessible.h"
>+#include "States.h"
>+#include "nsIAccessibleTraversalRule.h"

is an interface header, we sometimes keep those seperate from non-autogened headers.

>+#include "nsAccessible.h"

sort of isn't needed since nsHypertextAccessible inherits from it so the header has to be brought in by it, but I'm fine with keeping it on the grounds that its still bootlegging and we might want to change the inheritance setup.

>+  ~FilterCache();

define it inline, I can't think of a reason not to when you have a class in a cpp file.

>+  
>+  nsresult ApplyFilter(nsAccessible* aNode, PRUint16* aResult);

same

>+
>+private:

add copy protection.


>+  if (aPosition) {
>+    aSecondPosition = do_QueryObject(aPosition);
>+    if (!aSecondPosition || !IsRootDescendant(aSecondPosition))
>+      return NS_ERROR_INVALID_ARG;

check it isn't defunct too, setting your position to a defunct accessible will do you no good and will slow down us getting rid of it.

btw, no need for a on aSecond position.

>+  nsRefPtr<nsHyperTextAccessible> newPosition = do_QueryObject(aTextAccessible);
>+  if (!newPosition || !IsRootDescendant(newPosition))
>+    return NS_ERROR_INVALID_ARG;
>+
>+  // Make sure the given offsets don't exceed the character count.
>+  PRInt32 charCount = newPosition->CharacterCount();
>+
>+  if (aEndOffset > charCount)
>+    return NS_ERROR_FAILURE;

isn't that an invalid arg?

>+{
>+  NS_ENSURE_ARG(aResult);

common  nit, NS_ENSURE_ARG_POINTER?


>+nsAccessiblePivot::MoveLast(nsIAccessibleTraversalRule* aRule, bool* aResult)
>+{
>+  NS_ENSURE_ARG(aResult);
>+  NS_ENSURE_ARG(aRule);
>+
>+  *aResult = false;
>+  nsresult rv = NS_OK;
>+  nsAccessible* lastNode = mRootNode;
>+  nsAccessible* node = nsnull;

declare it at first use?

>+    nsCOMPtr<nsIAccessiblePivotObserver> obs = iter.GetNext();
>+    obs->OnPivotChanged(this, aOldPosition, aOldStart, aOldEnd);

since the array already has a ref you don't really need the nsCOMPtr for anything, and it'll cause some extra AddRef / Release traffic, I'd suggest itr.Next()->Notify()

>+  }
>+}
>+
>+nsresult
>+FilterCache::ApplyFilter(nsAccessible* aNode, PRUint16* aResult)
>+{
>+  *aResult = nsIAccessibleTraversalRule::FILTER_IGNORE;
>+
>+  if (!mAcceptRoles) {

Neil, what happens in the case of an empty array, does xpconnect hand us a null pointer? or something else?

>+    PRUint32 nodeRole = aNode->Role();

just role

>+    bool matchesRole = false;
>+    for (PRUint32 idx = 0; idx < mAcceptRolesLength; idx++) {
>+      matchesRole = mAcceptRoles[idx] == nodeRole;
>+      if (matchesRole)
>+        break;
>+    }
>+    if (!matchesRole)
>+      return NS_OK;

I'd be sort of tempted to use a goto here, but I wouldn't be suprised to hear everyone else on the planet disagrees with me.

>+
>+FilterCache::~FilterCache () {

{ on its own line

another general comment, when you go down the tree you need to check that the accessible you just got isn't defunct, I don't *think they should be still attached to the tree in that case, but I believe we have at least one case in mochitest-a11y asserts in such a way that it happens.

Comment 98

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #96)
> (In reply to Eitan Isaacson [:eeejay] from comment #95)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #93)
> > > > 
> > > > >+  PRUint32 *acceptRoles;
> > > > >+  PRUint32 acceptRolesLength;
> > > > >+  *rv = aRule->GetMatchRoles(&acceptRoles, &acceptRolesLength);
> > > > >+  NS_ENSURE_SUCCESS(*rv, nsnull);
> > > > So, after this point, you have to be very careful not to leak acceptRoles.
> > > > You're currently doing this using some gotos, but unfortunately you forgot
> > > > about your NS_ENSURE_SUCCESSes. Offhand I can't actually find a class that
> > > > will NS_Free your pointer for you automatically though. One idea I did have
> > > 
> > > I think we could make nsAutoPtr do this if we add a template arg of what
> > > function to use to free no?  We'd probably want the arg to default to delete
> > 
> > My latest patch introduces a new cache object that I think encapsulates the
> > rules bit nicely and lives on the heap for extra cleanup bonus.
> 
> I  pointed that out since it might be a useful idea even if we don't need it
> here.
> 
> btw I assume you meant stack since that's what you did.

I meant heap, sorry.

Comment 99

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #97)
> Comment on attachment 588980 [details] [diff] [review]
> Bug 698823 - Add nsIAccessiblePivot interface and implement.
> 
> 
> >+  ~FilterCache();
> 
> define it inline, I can't think of a reason not to when you have a class in
> a cpp file.
> 
> >+  
> >+  nsresult ApplyFilter(nsAccessible* aNode, PRUint16* aResult);
> 
> same
> 

Are you certain we can't define it later? It looks really ugly to have all of that on the top of the file. It is completely normal to function prototypes and definitions within the same file.

Comment 100

[alexander :surkov (:asurkov)]

Comment on attachment 585841 [details] [diff] [review]
Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

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

::: accessible/src/base/nsDocAccessible.h
@@ +78,5 @@
>                          public nsIObserver,
>                          public nsIScrollPositionListener,
>                          public nsSupportsWeakReference,
> +                        public nsIAccessibleCursorable,
> +                        public nsIAccessiblePivotObserver

where nsIAccessibleCursorable is defined, I don't see it in your patches

Comment 101

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Here it is. It somehow got obsoleted by mistake instead of a different patch.

Comment 102

[alexander :surkov (:asurkov)]

Comment on attachment 589549 [details] [diff] [review]
Bug 698823 - Add nsIAccessibleCursorable and implement.

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

::: accessible/public/nsIAccessibleCursorable.idl
@@ +41,5 @@
> +
> +interface nsIAccessiblePivot;
> +
> +[scriptable, uuid(5452dea5-d235-496f-8757-3ca016ff49ff)]
> +interface nsIAccessibleCursorable : nsISupports

document it

::: accessible/src/base/nsDocAccessible.cpp
@@ +131,5 @@
> +  // We provide a virtual cursor if this is a parent doc or if it's parent content
> +  // element is <browser/>
> +  nsIDocument *parent = mDocument->GetParentDocument();
> +  if (!parent ||
> +      parent->FindContentForSubDocument(mDocument)->Tag() == nsGkAtoms::browser) {

it'd be nice to unify tab document detection logic instead of introducing new code paths (see nsDocAccessible::IsLoadEventTarget, nsCoreUtils::IsTabDocument, nsAccessNodeWrap::QueryService)

@@ +132,5 @@
> +  // element is <browser/>
> +  nsIDocument *parent = mDocument->GetParentDocument();
> +  if (!parent ||
> +      parent->FindContentForSubDocument(mDocument)->Tag() == nsGkAtoms::browser) {
> +    mVirtualCursor = new nsAccessiblePivot(this);

it's worth to create it on demand since it's going to be used on one platform only

@@ +530,5 @@
>  
> +// nsIAccessibleVirtualCursor method
> +NS_IMETHODIMP
> +nsDocAccessible::GetVirtualCursor(nsIAccessiblePivot **aVirtualCursor) {
> +  NS_ENSURE_ARG_POINTER(aVirtualCursor);

nit: do proper styling

::: accessible/src/base/nsDocAccessible.h
@@ +599,5 @@
>    nsIAtom* mARIAAttrOldValue;
>  
>    nsTArray<nsRefPtr<nsDocAccessible> > mChildDocuments;
>  
> +  nsRefPtr<nsAccessiblePivot> mVirtualCursor;

nit: document it

Comment 103

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Answers Alex's feedback

Comment 104

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

Previous nsIAccessibleCursorable patch affects this one, so updating. I am also slowly moving these to Alex's review plate :)

Comment 105

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Added documentation to interface.

Comment 106

[alexander :surkov (:asurkov)]

Comment on attachment 590277 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.

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

mostly style nits, some questions, I'd like to get new patch to not get digressed by nits.

::: accessible/public/Makefile.in
@@ +70,3 @@
>        nsIAccessibleTable.idl \
>        nsIAccessibleText.idl \
> +      nsIAccessibleTraversalRule.idl \

it appears traversal rule is something that belongs to pivot, it makes sense to keep all things in one file.

::: accessible/public/nsIAccessiblePivot.idl
@@ +49,5 @@
> +/**
> + * An observer interface for pivot changes.
> + */
> +[scriptable, uuid(b6508c5e-c081-467d-835c-613eedf9ee9b)]
> +interface nsIAccessiblePivotObserver : nsISupports

I'd say it should be defined after pivot interface since pivot is central interface here

@@ +63,5 @@
> +/**
> + * The pivot interface encapsulates a reference to a single place in an accessible
> + *  subtree. This could either be an accessible node, or a text range withing a
> + *  text accessible. This interface also provides traversal methods and change
> + *  notifications via an observer.

nit: extra space between * and text starting from the second line
withing -> within
maybe something like this?: the pivot is a point or a range in the accessible tree. This interface provides traversal methods to move the pivot to next/prev state that complies to a filter.

@@ +71,5 @@
> +{
> +  const TextBoundaryType CHAR_BOUNDARY            = 0;
> +  const TextBoundaryType WORD_BOUNDARY            = 1;
> +  const TextBoundaryType LINE_BOUNDARY            = 2;
> +  const TextBoundaryType ATTRIBUTE_RANGE_BOUNDARY = 3;

nit: you don't need to indent initializations

@@ +74,5 @@
> +  const TextBoundaryType LINE_BOUNDARY            = 2;
> +  const TextBoundaryType ATTRIBUTE_RANGE_BOUNDARY = 3;
> +
> +  /*
> +   * The accessible the pivot is currently pointed at.

nit: /* -> /** and below in this file

@@ +86,5 @@
> +
> +  /*
> +   * The start offset in the accessible's text. Only supported when the accessible
> +   * implements the nsIAccessibleText interface. If no explicit offset was set or
> +   * if it is not supported in the pivot's accessible this is -1.

maybe: the start offset of the range the pivot points at, otherwise -1?

@@ +100,5 @@
> +
> +  /**
> +   * Set the pivot's text range in a text accessible.
> +   *
> +   * @param aTextAccessible the text accessible that contains the desired range.

nit: don't forget [in, out]

::: accessible/public/nsIAccessibleTraversalRule.idl
@@ +82,5 @@
> +   *
> +   * @param aAccessible accessible to examine.
> +   * @return either FILTER_ACCEPT, FILTER_REJECT, or FILTER_SKIP.
> +   */
> +  unsigned short filterAccessible(in nsIAccessible aAccessible);

it's not really filter actions, it's sort of test that accessible complies or not. Maybe like isMatched()?

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +42,5 @@
> +#include "nsAccUtils.h"
> +#include "nsHyperTextAccessible.h"
> +#include "States.h"
> +#include "nsIAccessibleTraversalRule.h"
> +#include "nsAccessible.h"

nit: put them in alphabetical order please

@@ +54,5 @@
> +class FilterCache
> +{
> +public:
> +  FilterCache(nsIAccessibleTraversalRule* aRule) : mRule(aRule),
> +                                                   mAcceptRoles(nsnull) { }

it appears you use 'filter' and 'traversal rule' terms for the same concept

@@ +72,5 @@
> +{
> +  NS_ASSERTION(aRootNode, "A root node is required");
> +}
> +
> +// nsISupports

please use style
/////// (80 chars)
// nsISupports

@@ +80,5 @@
> +NS_IMETHODIMP
> +nsAccessiblePivot::GetRootNode(nsIAccessible** aRootNode)
> +{
> +  NS_ENSURE_ARG_POINTER(aRootNode);
> +  NS_ADDREF(*aRootNode = mRootNode);

NS_IF_ADDREF is safer but depends who can use this object

@@ +93,5 @@
> +  return NS_OK;
> +}
> +
> +bool
> +nsAccessiblePivot::IsRootDescendant(nsAccessible* aNode)

keep things together, i.e methods of nsIAccessiblePivot go first then implementation of nsAccessiblePivot

@@ +106,5 @@
> +
> +NS_IMETHODIMP
> +nsAccessiblePivot::SetPosition(nsIAccessible* aPosition)
> +{
> +  nsRefPtr<nsAccessible> aSecondPosition;

nit: local variables shouldn't start from 'a' prefix

@@ +176,5 @@
> +
> +// Traversal functions
> +
> +void
> +nsAccessiblePivot::MovePivotInternal(nsAccessible* aPosition) {

bit: { on new line

@@ +263,5 @@
> +}
> +
> +// TODO: Implement
> +NS_IMETHODIMP
> +nsAccessiblePivot::MoveNextByText(TextBoundaryType aBoundary, bool* aResult)

do you have any plans to implement that in nearest future? otherwise we could not introduce these methods at all if there's no consumer

@@ +283,5 @@
> +
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +nsAccessible *

nit: nsAccessible*

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

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

@@ +338,5 @@
> +
> +  return nsnull;
> +}
> +
> +nsAccessible *

nit: *, please fix them through whole patch

@@ +352,5 @@
> +    aNode = mRootNode;
> +
> +  FilterCache cache(aRule);
> +
> +  PRUint16 filtered;

I don't like not initialized variables :)

@@ +359,5 @@
> +  if (searchCurrent && (filtered & nsIAccessibleTraversalRule::FILTER_MATCH))
> +    return aNode;
> +
> +  while (true) {
> +    nsAccessible* firstChild;

initialzie it

@@ +362,5 @@
> +  while (true) {
> +    nsAccessible* firstChild;
> +    while (!(filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE) &&
> +           (firstChild = aNode->FirstChild())) {
> +      aNode = firstChild;

bad practice to change in argument

@@ +405,5 @@
> +  NS_ENSURE_ARG(aObserver);
> +
> +  mObservers.AppendElement(aObserver);
> +
> +  return NS_OK;

you're doing different style, sometimes you put lines like these together, sometimes you split them by new lines

@@ +422,5 @@
> +                                      PRInt32 aOldStart, PRInt32 aOldEnd)
> +{
> +  nsTObserverArray<nsCOMPtr<nsIAccessiblePivotObserver> >::ForwardIterator iter(mObservers);
> +  while (iter.HasMore()) {
> +    nsCOMPtr<nsIAccessiblePivotObserver> obs = iter.GetNext();

no way to get a weak pointer? It doesn't make sense to addref/release it here since you keep a strong ref on it already

@@ +428,5 @@
> +  }
> +}
> +
> +nsresult
> +FilterCache::ApplyFilter(nsAccessible* aNode, PRUint16* aResult)

I don't see why you'd need to be to XPOMy, i.e. do you really need nsresult for something?

@@ +433,5 @@
> +{
> +  *aResult = nsIAccessibleTraversalRule::FILTER_IGNORE;
> +
> +  if (!mAcceptRoles) {
> +    nsresult rv = mRule->GetMatchRoles(&mAcceptRoles, &mAcceptRolesLength);

why don't you do that in constructor instead? Do you really want to cache them and ignore any potential changes in traversal rule object?

@@ +444,5 @@
> +    PRUint64 state = aNode->State();
> +
> +    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & mPreFilter) &&
> +        (state & states::DEFUNCT))
> +      return NS_OK;

that filter doesn't make much sense because if you're not at defunct accessible (which is could be since you keep strong ref on it) then you can get defunct object. Anyway if you're at defunct object then it shouldn't have any parents/children

@@ +460,5 @@
> +      return NS_OK;
> +  }
> +
> +  if (mAcceptRolesLength > 0) {
> +    PRUint32 nodeRole = aNode->Role();

btw, I wouldn't use node term for accessible object since it's much reserved for DOM nodes. At least we don't do that anywhere in a11y code

@@ +476,5 @@
> +}
> +
> +FilterCache::~FilterCache () {
> +  if (mAcceptRoles)
> +    nsMemory::Free(mAcceptRoles);

this is sort of dangerous assumption because you don't control implementation of nsIAccessibleTraversalRule and it's not clear who should do ownership. Would it better to use nsIArray or something and copy it?

::: accessible/src/base/nsAccessiblePivot.h
@@ +53,5 @@
> + */
> +class nsAccessiblePivot: public nsIAccessiblePivot
> +{
> +public:
> +  nsAccessiblePivot(nsAccessible *aRootNode);

nsAccessible*

@@ +55,5 @@
> +{
> +public:
> +  nsAccessiblePivot(nsAccessible *aRootNode);
> +
> +  nsAccessible *Position() { return mPosition; }

nit: nsAccessible*

@@ +72,5 @@
> +  nsAccessible* SearchForward(nsAccessible *aNode,
> +                              nsIAccessibleTraversalRule* aRule,
> +                              bool searchCurrent,
> +                              nsresult* rv);
> +  nsAccessible* SearchBackward(nsAccessible *aNode,

nit: *

@@ +76,5 @@
> +  nsAccessible* SearchBackward(nsAccessible *aNode,
> +                               nsIAccessibleTraversalRule* aRule,
> +                               bool searchCurrent,
> +                               nsresult* rv);
> +  void MovePivotInternal(nsAccessible* aPosition);

nit: would be nice to have them documented

@@ +79,5 @@
> +                               nsresult* rv);
> +  void MovePivotInternal(nsAccessible* aPosition);
> +
> +  nsRefPtr<nsAccessible> mRootNode;
> +  nsRefPtr<nsAccessible> mPosition;

nit: new line

@@ +81,5 @@
> +
> +  nsRefPtr<nsAccessible> mRootNode;
> +  nsRefPtr<nsAccessible> mPosition;
> +  PRInt32           mStartOffset;
> +  PRInt32           mEndOffset;

nit: new line, two spaces indentation

Comment 107

[alexander :surkov (:asurkov)]

Comment on attachment 590272 [details] [diff] [review]
Bug 698823 - Add nsIAccessibleCursorable and implement.

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

::: accessible/public/nsIAccessibleCursorable.idl
@@ +16,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2011

you might want to fix the year

@@ +41,5 @@
> +
> +interface nsIAccessiblePivot;
> +
> +/**
> + * An interface for a parent node accessible that controls a virtual cursor.

An interface for a parent node accessible that contrlols ... -> An interface is implemented by accessible object having associated virtual cursor?

::: accessible/src/base/nsDocAccessible.cpp
@@ +129,5 @@
>    AddScrollListener();
> +
> +  // We provide a virtual cursor if this is a root doc or if it's a tab doc.
> +  mIsCursorable = (!(mDocument->GetParentDocument()) ||
> +                   nsCoreUtils::IsTabDocument(mDocument));

doesn't sound like complex check to keep its result as a member?

for root accessible check I'd use nsAccessible::IsRoot() I think

@@ +525,5 @@
>  }
>  
> +// nsIAccessibleVirtualCursor method
> +NS_IMETHODIMP
> +nsDocAccessible::GetVirtualCursor(nsIAccessiblePivot **aVirtualCursor)

nit: **

@@ +533,5 @@
> +
> +  NS_ENSURE_TRUE(mIsCursorable, NS_ERROR_NOT_IMPLEMENTED);
> +
> +  if (IsDefunct())
> +    return NS_ERROR_FAILURE;

usually IsDefunct check comes first, up to you

::: accessible/src/base/nsDocAccessible.h
@@ +606,5 @@
> +  bool mIsCursorable;
> +
> +  /**
> +   * The virtual cursor of the document when it supports nsIAccessibleCursorable.
> +   * It is lazily instantiated when it is first retrieved.

last phrase sort of implementation details and until you're going to use mVirtualCursor internally, it doesn't make too much sense to say about this.

Comment 108

[alexander :surkov (:asurkov)]

Comment on attachment 590273 [details] [diff] [review]
Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

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

::: accessible/public/nsIAccessibleEvent.idl
@@ +442,5 @@
>    const unsigned long EVENT_OBJECT_ATTRIBUTE_CHANGED = 0x0055;
>  
>    /**
> +   * An object's virtual cursor changed. The target accessible is the virtual
> +   * cursor's manager, typically a document.

maybe just: A virtual cursor of document is changed?

::: accessible/src/base/nsDocAccessible.cpp
@@ +540,2 @@
>      mVirtualCursor = new nsAccessiblePivot(this);
> +    mVirtualCursor->AddObserver(this);

so pivot keeps document, document keeps pivot. don't we need cycle collection magic?

@@ +544,5 @@
>    NS_ADDREF(*aVirtualCursor = mVirtualCursor);
>    return NS_OK;
>  }
>  
> +// nsIAccessiblePivotObserver method

usually we do:
///// (80 chars)
// nsIAccessiblePivotObserver

comments like
// nsIAccessiblePivotObserver method
to say this method is in wrong section

@@ +547,5 @@
>  
> +// nsIAccessiblePivotObserver method
> +NS_IMETHODIMP nsDocAccessible::OnPivotChanged(nsIAccessiblePivot* aPivot,
> +                                              nsIAccessible* aOldAccessible,
> +                                              PRInt32 aOldStart, PRInt32 aOldEnd)

NS_IMETHODIMP on new line please

@@ +550,5 @@
> +                                              nsIAccessible* aOldAccessible,
> +                                              PRInt32 aOldStart, PRInt32 aOldEnd)
> +{
> +  nsRefPtr<AccEvent> event = new AccEvent(nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, this);
> +  nsEventShell::FireEvent(event);

sort of unuseful information about previous pivot state because there's no consumers

::: accessible/src/base/nsDocAccessible.h
@@ +78,5 @@
>                          public nsIObserver,
>                          public nsIScrollPositionListener,
>                          public nsSupportsWeakReference,
> +                        public nsIAccessibleCursorable,
> +                        public nsIAccessiblePivotObserver

I don't see where you put pivotobserver into queryInterface, is it on demand? I wonder if observers arrays needs queryIterface for something

Comment 109

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/nsDocAccessible.cpp
> @@ +540,2 @@
> >      mVirtualCursor = new nsAccessiblePivot(this);
> > +    mVirtualCursor->AddObserver(this);
> 
> so pivot keeps document, document keeps pivot. don't we need cycle
> collection magic?

is there a reason the document can't drop its reference to the pivot when it's shut down?

> @@ +550,5 @@
> > +                                              nsIAccessible* aOldAccessible,
> > +                                              PRInt32 aOldStart, PRInt32 aOldEnd)
> > +{
> > +  nsRefPtr<AccEvent> event = new AccEvent(nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, this);
> > +  nsEventShell::FireEvent(event);
> 
> sort of unuseful information about previous pivot state because there's no
> consumers

true, but if you implement that  interface your vfunc needs to take those args.

An alternative way of accomplishing this would be to template nsAccessiblePivot on a bool for if it should fire the event on the root, but I'm not sure I like that more.

> 
> ::: accessible/src/base/nsDocAccessible.h
> @@ +78,5 @@
> >                          public nsIObserver,
> >                          public nsIScrollPositionListener,
> >                          public nsSupportsWeakReference,
> > +                        public nsIAccessibleCursorable,
> > +                        public nsIAccessiblePivotObserver
> 
> I don't see where you put pivotobserver into queryInterface, is it on
> demand? I wonder if observers arrays needs queryIterface for something

I don't think it needs it right now, but if it implements it I tend to think you should be able to QI to it.  On the other hand it does slow down QI slightly.

Comment 110

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #109)

> > so pivot keeps document, document keeps pivot. don't we need cycle
> > collection magic?
> 
> is there a reason the document can't drop its reference to the pivot when
> it's shut down?

it should, but say something goes wrong and document doesn't get shutdown then cycle collector is supposed to take care about this case.

> > sort of unuseful information about previous pivot state because there's no
> > consumers
> 
> true, but if you implement that  interface your vfunc needs to take those
> args.

up to you, just curious

> An alternative way of accomplishing this would be to template
> nsAccessiblePivot on a bool for if it should fire the event on the root, but
> I'm not sure I like that more.

me neither
 
> > I don't see where you put pivotobserver into queryInterface, is it on
> > demand? I wonder if observers arrays needs queryIterface for something
> 
> I don't think it needs it right now, but if it implements it I tend to think
> you should be able to QI to it.  On the other hand it does slow down QI
> slightly.

this object is going to be kept by nsCOMPtr which assumes it can query the object. So if you wouldn't use this class that way then it would be ok. But since this is XPCOM interface then I think you should be able to query it.

Comment 111

[Eitan Isaacson [:eeejay]]

Anything I did not reply to here should be in the next patch.

(In reply to alexander :surkov from comment #106)
> Comment on attachment 590277 [details] [diff] [review]
> Bug 698823 - Add nsIAccessiblePivot interface and implement.
> 
> Review of attachment 590277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/public/nsIAccessibleTraversalRule.idl
> @@ +82,5 @@
> > +   *
> > +   * @param aAccessible accessible to examine.
> > +   * @return either FILTER_ACCEPT, FILTER_REJECT, or FILTER_SKIP.
> > +   */
> > +  unsigned short filterAccessible(in nsIAccessible aAccessible);
> 
> it's not really filter actions, it's sort of test that accessible complies
> or not. Maybe like isMatched()?
> 

To me, a is*() function means it returns boolean. The W3C tree walker uses the term filter, and it serves the same purpose. But if this is a real deal-breaker for you, we could fix it.

> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +80,5 @@
> > +NS_IMETHODIMP
> > +nsAccessiblePivot::GetRootNode(nsIAccessible** aRootNode)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aRootNode);
> > +  NS_ADDREF(*aRootNode = mRootNode);
> 
> NS_IF_ADDREF is safer but depends who can use this object
> 

See comment #59. Trevor and I went back and fourth on this, I guess that is what changing reviewers mid-review entails :p

> 
> @@ +263,5 @@
> > +}
> > +
> > +// TODO: Implement
> > +NS_IMETHODIMP
> > +nsAccessiblePivot::MoveNextByText(TextBoundaryType aBoundary, bool* aResult)
> 
> do you have any plans to implement that in nearest future? otherwise we
> could not introduce these methods at all if there's no consumer
> 

See discussion above, I would really like to introduce this into the API now. Just for the sake of speeding the process. The implementation depends on reworking the text interface, like you outline here[1].

1. https://wiki.mozilla.org/Accessibility/TextImplementation

> @@ +352,5 @@
> > +    aNode = mRootNode;
> > +
> > +  FilterCache cache(aRule);
> > +
> > +  PRUint16 filtered;
> 
> I don't like not initialized variables :)
> 

They became uninitialized in one of the review rounds above :P I'll fix.

> @@ +362,5 @@
> > +  while (true) {
> > +    nsAccessible* firstChild;
> > +    while (!(filtered & nsIAccessibleTraversalRule::FILTER_IGNORE_SUBTREE) &&
> > +           (firstChild = aNode->FirstChild())) {
> > +      aNode = firstChild;
> 
> bad practice to change in argument

I agree, see review process above. I'll change it back.

> 
> @@ +405,5 @@
> > +  NS_ENSURE_ARG(aObserver);
> > +
> > +  mObservers.AppendElement(aObserver);
> > +
> > +  return NS_OK;
> 
> you're doing different style, sometimes you put lines like these together,
> sometimes you split them by new lines

I split all of them. I like new lines :)

> @@ +428,5 @@
> > +  }
> > +}
> > +
> > +nsresult
> > +FilterCache::ApplyFilter(nsAccessible* aNode, PRUint16* aResult)
> 
> I don't see why you'd need to be to XPOMy, i.e. do you really need nsresult
> for something?

Yes, bubbling up errors thrown by the rule's implementation.

> 
> @@ +433,5 @@
> > +{
> > +  *aResult = nsIAccessibleTraversalRule::FILTER_IGNORE;
> > +
> > +  if (!mAcceptRoles) {
> > +    nsresult rv = mRule->GetMatchRoles(&mAcceptRoles, &mAcceptRolesLength);
> 
> why don't you do that in constructor instead? Do you really want to cache
> them and ignore any potential changes in traversal rule object?
> 

Yes, we discussed this. It is worth caching (and ignoring future changes). The point of the matched roles array is to speed things up and not to have to go to js on every node. I also put a warning in the idl documentation.

I don't want to do it in the constructor because I want to handle errors correctly, and it would be a pain to get an nsresult from the constructor (i guess not impossible with a reference argument, but still).

> @@ +444,5 @@
> > +    PRUint64 state = aNode->State();
> > +
> > +    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & mPreFilter) &&
> > +        (state & states::DEFUNCT))
> > +      return NS_OK;
> 
> that filter doesn't make much sense because if you're not at defunct
> accessible (which is could be since you keep strong ref on it) then you can
> get defunct object. Anyway if you're at defunct object then it shouldn't
> have any parents/children
> 

But could one of the children be defunct?

> 
> @@ +476,5 @@
> > +}
> > +
> > +FilterCache::~FilterCache () {
> > +  if (mAcceptRoles)
> > +    nsMemory::Free(mAcceptRoles);
> 
> this is sort of dangerous assumption because you don't control
> implementation of nsIAccessibleTraversalRule and it's not clear who should
> do ownership. Would it better to use nsIArray or something and copy it?
> 

If the callee does not allocate it is a bug :) This seems to be how it is done across the whole mozilla codebase. XPConnect, or whatever, does this correctly for javascript too.

Comment 112

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Comment 113

[Eitan Isaacson [:eeejay]]

Anything I don't reply to inline should be in the next patch.

(In reply to alexander :surkov from comment #107)
> Comment on attachment 590272 [details] [diff] [review]
> Bug 698823 - Add nsIAccessibleCursorable and implement.
> 
> Review of attachment 590272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsDocAccessible.cpp
> @@ +129,5 @@
> >    AddScrollListener();
> > +
> > +  // We provide a virtual cursor if this is a root doc or if it's a tab doc.
> > +  mIsCursorable = (!(mDocument->GetParentDocument()) ||
> > +                   nsCoreUtils::IsTabDocument(mDocument));
> 
> doesn't sound like complex check to keep its result as a member?
> 
> for root accessible check I'd use nsAccessible::IsRoot() I think
> 

Apparently it gets messy because that code could potentially be run during cycle collection. I kept on running into a crasher when I tried to do what you said. Granted, I am not 100% familiar with the object's life cycle.

Comment 114

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Comment 115

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #111)
> > > +  unsigned short filterAccessible(in nsIAccessible aAccessible);
> > 
> > it's not really filter actions, it's sort of test that accessible complies
> > or not. Maybe like isMatched()?
> > 
> 
> To me, a is*() function means it returns boolean. The W3C tree walker uses
> the term filter, and it serves the same purpose. But if this is a real
> deal-breaker for you, we could fix it.

DOM walker use 'filter' word as noun, you do as verb and that's not exactly what method does. As long as you and Trevor is fine with that then it's fine with me.

> > ::: accessible/src/base/nsAccessiblePivot.cpp
> > > +  NS_ENSURE_ARG_POINTER(aRootNode);
> > > +  NS_ADDREF(*aRootNode = mRootNode);
> > 
> > NS_IF_ADDREF is safer but depends who can use this object
> > 
> 
> See comment #59. Trevor and I went back and fourth on this, I guess that is
> what changing reviewers mid-review entails :p

Trevor said in comment #59 that "if it can't be null" but since it sounds you don't control nsAccessiblePivot (anyone can create it) then it's safer to have protection from null value.

> > > +nsAccessiblePivot::MoveNextByText(TextBoundaryType aBoundary, bool* aResult)
> > 
> > do you have any plans to implement that in nearest future? otherwise we
> > could not introduce these methods at all if there's no consumer
> > 
> 
> See discussion above, I would really like to introduce this into the API
> now. Just for the sake of speeding the process. The implementation depends
> on reworking the text interface, like you outline here[1].

fine with me

> > > +nsresult
> > > +FilterCache::ApplyFilter(nsAccessible* aNode, PRUint16* aResult)
> > 
> > I don't see why you'd need to be to XPOMy, i.e. do you really need nsresult
> > for something?
> 
> Yes, bubbling up errors thrown by the rule's implementation.

ok, though I'm not still sure it's useful

> > > +  if (!mAcceptRoles) {
> > > +    nsresult rv = mRule->GetMatchRoles(&mAcceptRoles, &mAcceptRolesLength);
> > 
> > why don't you do that in constructor instead? Do you really want to cache
> > them and ignore any potential changes in traversal rule object?
> > 
> 
> Yes, we discussed this. It is worth caching (and ignoring future changes).
> The point of the matched roles array is to speed things up and not to have
> to go to js on every node. I also put a warning in the idl documentation.
> 
> I don't want to do it in the constructor because I want to handle errors
> correctly, and it would be a pain to get an nsresult from the constructor (i
> guess not impossible with a reference argument, but still).

ok, but still not useful I think, if you propagate the error then it usually means nothing like general failure error that says you something failed what's not very useful.

> > > +    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & mPreFilter) &&
> > > +        (state & states::DEFUNCT))
> > > +      return NS_OK;
> > 
> > that filter doesn't make much sense because if you're not at defunct
> > accessible (which is could be since you keep strong ref on it) then you can
> > get defunct object. Anyway if you're at defunct object then it shouldn't
> > have any parents/children
> > 
> 
> But could one of the children be defunct?

I'd say no, defunct accessible doesn't have parent/child relations (at least it shouldn't).

> > > +FilterCache::~FilterCache () {
> > > +  if (mAcceptRoles)
> > > +    nsMemory::Free(mAcceptRoles);
> > 
> > this is sort of dangerous assumption because you don't control
> > implementation of nsIAccessibleTraversalRule and it's not clear who should
> > do ownership. Would it better to use nsIArray or something and copy it?
> > 
> 
> If the callee does not allocate it is a bug :) This seems to be how it is
> done across the whole mozilla codebase. XPConnect, or whatever, does this
> correctly for javascript too.

ok

Comment 116

[alexander :surkov (:asurkov)]

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

> > > +  // We provide a virtual cursor if this is a root doc or if it's a tab doc.
> > > +  mIsCursorable = (!(mDocument->GetParentDocument()) ||
> > > +                   nsCoreUtils::IsTabDocument(mDocument));
> > 
> > doesn't sound like complex check to keep its result as a member?
> > 
> > for root accessible check I'd use nsAccessible::IsRoot() I think
> > 
> 
> Apparently it gets messy because that code could potentially be run during
> cycle collection. I kept on running into a crasher when I tried to do what
> you said. Granted, I am not 100% familiar with the object's life cycle.

Ok. Move that to Init(), there you can use IsRoot().

Comment 117

[alexander :surkov (:asurkov)]

Comment on attachment 590273 [details] [diff] [review]
Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

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

could you please upload new version of the patch with comments addressed please?

Comment 118

[alexander :surkov (:asurkov)]

Comment on attachment 591630 [details] [diff] [review]
Bug 698823 - Add nsIAccessiblePivot interface and implement.

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

::: accessible/public/nsIAccessiblePivot.idl
@@ +67,5 @@
> +
> +  /**
> +   * The root of the subtree in which the pivot traverses.
> +   */
> +  readonly attribute nsIAccessible rootAccessible;

if you have 'position' then I'd say you need to have 'root' (not 'rootAccessible')

@@ +70,5 @@
> +   */
> +  readonly attribute nsIAccessible rootAccessible;
> +
> +  /**
> +   * The start text offset of the range the pivot points at, otherwise -1.

text range?

@@ +75,5 @@
> +   */
> +  readonly attribute long startOffset;
> +
> +  /**
> +   * The end text offset of the range the pivot points at, otherwise -1.

same

@@ +94,5 @@
> +                    in long aStartOffset, in long aEndOffset);
> +
> +  /**
> +   * Move pivot to next object via given traversal rule, which determines the
> +   *  nodes that will be traversed.

nit: extra indent
maybe: move pivot to next point complying the given traversal rule?

@@ +103,5 @@
> +  boolean moveNext(in nsIAccessibleTraversalRule aRule);
> +
> +  /**
> +   * Move pivot to previous object via given traversal rule, which determines the
> +   *  nodes that will be traversed.

same here (extra space and comment)

@@ +111,5 @@
> +   */
> +  boolean movePrevious(in nsIAccessibleTraversalRule aRule);
> +
> +  /**
> +   * Move pivot to first occurence in root's subtree of the given traversal rule,

subtree of traversal rule?

@@ +121,5 @@
> +  boolean moveFirst(in nsIAccessibleTraversalRule aRule);
> +
> +  /**
> +   * Move pivot to last occurence in document of the given traversal rule, which
> +   *  determines the nodes that will be traversed.

same, extra space

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +49,5 @@
> +#include "nsISupportsPrimitives.h"
> +
> +using namespace mozilla::a11y;
> +
> +class RuleCache

please short comment for the class

@@ +67,5 @@
> +  PRUint32* mAcceptRoles;
> +  PRUint32 mAcceptRolesLength;
> +  PRUint32 mPreFilter;
> +};
> +

please add here
/// (80)
// nsAccessiblePivot

@@ +76,5 @@
> +  NS_ASSERTION(aRootAccessible, "A root accessible is required");
> +}
> +
> +////////////////////////////////////////////////////////////////////////////////
> +// nsISupports

empty line?

@@ +80,5 @@
> +// nsISupports
> +NS_IMPL_ISUPPORTS1(nsAccessiblePivot, nsIAccessiblePivot)
> +
> +////////////////////////////////////////////////////////////////////////////////
> +// nsIAccessiblePivot

empty line

Comment 119

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #116)
> (In reply to Eitan Isaacson [:eeejay] from comment #113)
> 
> > > > +  // We provide a virtual cursor if this is a root doc or if it's a tab doc.
> > > > +  mIsCursorable = (!(mDocument->GetParentDocument()) ||
> > > > +                   nsCoreUtils::IsTabDocument(mDocument));
> > > 
> > > doesn't sound like complex check to keep its result as a member?
> > > 
> > > for root accessible check I'd use nsAccessible::IsRoot() I think
> > > 
> > 
> > Apparently it gets messy because that code could potentially be run during
> > cycle collection. I kept on running into a crasher when I tried to do what
> > you said. Granted, I am not 100% familiar with the object's life cycle.
> 
> Ok. Move that to Init(), there you can use IsRoot().

Sorry, I was answering the question above that :)
> > > doesn't sound like complex check to keep its result as a member?

IsRoot() works fine in the constructor, I think.

Comment 120

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #115)
> (In reply to Eitan Isaacson [:eeejay] from comment #111)
> > > > +  unsigned short filterAccessible(in nsIAccessible aAccessible);
> > > 
> > > it's not really filter actions, it's sort of test that accessible complies
> > > or not. Maybe like isMatched()?
> > > 
> > 
> > To me, a is*() function means it returns boolean. The W3C tree walker uses
> > the term filter, and it serves the same purpose. But if this is a real
> > deal-breaker for you, we could fix it.
> 
> DOM walker use 'filter' word as noun, you do as verb and that's not exactly
> what method does. As long as you and Trevor is fine with that then it's fine
> with me.
> 

You are right, the name of the method is actually matchNode. So I'll change ours to matchAccessible.

> > > ::: accessible/src/base/nsAccessiblePivot.cpp
> > > > +  NS_ENSURE_ARG_POINTER(aRootNode);
> > > > +  NS_ADDREF(*aRootNode = mRootNode);
> > > 
> > > NS_IF_ADDREF is safer but depends who can use this object
> > > 
> > 
> > See comment #59. Trevor and I went back and fourth on this, I guess that is
> > what changing reviewers mid-review entails :p
> 
> Trevor said in comment #59 that "if it can't be null" but since it sounds
> you don't control nsAccessiblePivot (anyone can create it) then it's safer
> to have protection from null value.
> 

Later we changed to asserting non-null during construction. But this so trivial, I'll add "_IF" :)

> > > > +nsresult
> > > > +FilterCache::ApplyFilter(nsAccessible* aNode, PRUint16* aResult)
> > > 
> > > I don't see why you'd need to be to XPOMy, i.e. do you really need nsresult
> > > for something?
> > 
> > Yes, bubbling up errors thrown by the rule's implementation.
> 
> ok, though I'm not still sure it's useful
> 

In the case of a js implementation you get a stack trace with a file and line number, so I think it is useful. The actual error is not but that, imho, is a general mozilla problem where the errors are way too vague. So yes, it could also just be a boolean for success/error, but since we are speaking in nsresult why not just keep it.

> > > > +  if (!mAcceptRoles) {
> > > > +    nsresult rv = mRule->GetMatchRoles(&mAcceptRoles, &mAcceptRolesLength);
> > > 
> > > why don't you do that in constructor instead? Do you really want to cache
> > > them and ignore any potential changes in traversal rule object?
> > > 
> > 
> > Yes, we discussed this. It is worth caching (and ignoring future changes).
> > The point of the matched roles array is to speed things up and not to have
> > to go to js on every node. I also put a warning in the idl documentation.
> > 
> > I don't want to do it in the constructor because I want to handle errors
> > correctly, and it would be a pain to get an nsresult from the constructor (i
> > guess not impossible with a reference argument, but still).
> 
> ok, but still not useful I think, if you propagate the error then it usually
> means nothing like general failure error that says you something failed
> what's not very useful.
> 

Right, see above. Also, I have gotten useful errors when implementing rules in js, for example NS_ERROR_NOT_IMPLEMENTED.

> > > > +    if ((nsIAccessibleTraversalRule::PREFILTER_DEFUNCT & mPreFilter) &&
> > > > +        (state & states::DEFUNCT))
> > > > +      return NS_OK;
> > > 
> > > that filter doesn't make much sense because if you're not at defunct
> > > accessible (which is could be since you keep strong ref on it) then you can
> > > get defunct object. Anyway if you're at defunct object then it shouldn't
> > > have any parents/children
> > > 
> > 
> > But could one of the children be defunct?
> 
> I'd say no, defunct accessible doesn't have parent/child relations (at least
> it shouldn't).
> 

So I should simply remove this from the idl altogether?

Comment 121

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Some trivial changes, already r+ed by Alex.

Comment 122

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Updated to new API changes. Added Alex as reviewer.

Comment 123

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Remove defunct prefilter as discussed above.

Comment 124

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Removed defunct prefilter.

Comment 125

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #117)
> Comment on attachment 590273 [details] [diff] [review]
> Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.
> 
> Review of attachment 590273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> could you please upload new version of the patch with comments addressed
> please?

Cycle collection is new terrain for me. Wish there was more documentation. I am a bit slow on that. Should have something up on Monday.

Comment 126

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #119)
> IsRoot() works fine in the constructor, I think.

I wounder how it's supposed to work because eRootAccessible flag is set in nsRootAccessible constructor which is called after nsDocAccessible constructor where flags are used

(In reply to Eitan Isaacson [:eeejay] from comment #120)
> > I'd say no, defunct accessible doesn't have parent/child relations (at least
> > it shouldn't).
> > 
> 
> So I should simply remove this from the idl altogether?

this makes sense

Comment 127

[alexander :surkov (:asurkov)]

Comment on attachment 592185 [details] [diff] [review]
Bug 698823 - Add mochitest for virtual cursor functionality.

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

please fix style issues and rerequest review

::: accessible/tests/mochitest/pivot.js
@@ +2,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +

//// (80)
// Traversal rules

@@ +3,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +var HeadersTraversalRule = {

nit: { on new line and everywhere below please
it'd be nice to have a comment for this object

@@ +5,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +var HeadersTraversalRule = {
> +  getMatchRoles: function (aRules) {
> +    aRules.value = [Ci.nsIAccessibleRole.ROLE_HEADING];

use ROLE_HEADING, it's ok to be dependent on role.js

@@ +9,5 @@
> +    aRules.value = [Ci.nsIAccessibleRole.ROLE_HEADING];
> +    return aRules.value.length;
> +  },
> +
> +  preFilter: Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE,

please add PREFILTER_INVISIBLE constants into beginning of the file

@@ +11,5 @@
> +  },
> +
> +  preFilter: Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE,
> +
> +  matchAccessible: function (aAccessible) {

don't keep space between 'function' and '(' here and below

@@ +12,5 @@
> +
> +  preFilter: Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE,
> +
> +  matchAccessible: function (aAccessible) {
> +    return Ci.nsIAccessibleTraversalRule.FILTER_MATCH;

same

@@ +46,5 @@
> +  _isFocusable: function (aAccessible) {
> +    var state = {};
> +    var extState = {};
> +    aAccessible.getState(state, extState);
> +    return (state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE) != 0;

use hasState() from states.js, if you do that then you don't need to have _isFocusable method or consider to add into states.js

@@ +51,5 @@
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAccessibleTraversalRule])
> +}
> +

add
/// (80)
// Virtual cursor invokers and checkers

@@ +62,5 @@
> +    var aId = null;
> +    var aAccessible = aDocAcc.virtualCursor.position;
> +
> +    try {
> +      aId = aAccessible.attributes.getStringProperty('id');

I'd do aAccessible.DOMNode.id

@@ +86,5 @@
> +    }
> +  };
> +}
> +
> +function virtualCursorOffsetInvoker(aDocAcc, aTextAccessible, aTextOffsets)

maybe setVirutalCursorRange and setVirtualCursorPos ?

@@ +143,5 @@
> +    new invokerChecker(EVENT_VIRTUALCURSOR_CHANGED, aDocAcc)
> +  ];
> +}
> +
> +function addRuleToQueue(aQueue, aDocAcc, aRule, aSequence) {

method name is not very good because you don't add a rule to the queue, you generate queue from aSequence

@@ +146,5 @@
> +
> +function addRuleToQueue(aQueue, aDocAcc, aRule, aSequence) {
> +  aDocAcc.virtualCursor.position = null;
> +
> +  for (var i=0;i<aSequence.length;i++) {

space between operator and operand

::: accessible/tests/mochitest/pivot/test_virtualcursor.html
@@ +5,5 @@
> +  <meta charset="utf-8" />
> +  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
> +
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>

new line

@@ +8,5 @@
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="../common.js"></script>
> +  <script type="application/javascript" src="../events.js"></script>
> +  <script type="application/javascript" src="../pivot.js"></script>

new line

@@ +26,5 @@
> +      var docAcc = getAccessible(doc, [nsIAccessibleDocument,
> +                                       nsIAccessibleCursorable]);
> +
> +      // Test that embedded documents don't have their own virtual cursor.
> +      SimpleTest.is(docAcc.childDocumentCount, 1, "Expecting one child document");

SimpleTest.is() -> is()

@@ +41,5 @@
> +
> +      gQueue = new eventQueue();
> +
> +      gQueue.onFinish = function onFinish () {
> +        SimpleTest.info("GOOO!");

debugging?

@@ +55,5 @@
> +         'dolor', ' sit amet. Integer vitae urna leo, id ',
> +         'semper', ' nulla. ', 'Second Section Title',
> +         'Sed accumsan luctus lacus, vitae mollis arcu tristique vulputate.',
> +         'An ', 'embedded', ' document.', 'Link 1', 'Link 2', 'Link 3']);
> +

maybe you should use DOM node lists that allows you to switch to querySelector

@@ +74,5 @@
> +      gBrowserWnd = window.openDialog(
> +        "chrome://browser/content/", "_blank", "chrome,all,dialog=no",
> +        "chrome://mochitests/content/a11y/accessible/pivot/doc_virtualcursor.html");
> +
> +      addA11yLoadEvent(startTests, gBrowserWnd);

consider to use browser.js

Comment 128

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

This adds the cycle collecting magic. I don't know if I did this correctly...

Comment 129

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #127)
> Comment on attachment 592185 [details] [diff] [review]
> Bug 698823 - Add mochitest for virtual cursor functionality.
> 
> Review of attachment 592185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/pivot/test_virtualcursor.html
> 
> @@ +55,5 @@
> > +         'dolor', ' sit amet. Integer vitae urna leo, id ',
> > +         'semper', ' nulla. ', 'Second Section Title',
> > +         'Sed accumsan luctus lacus, vitae mollis arcu tristique vulputate.',
> > +         'An ', 'embedded', ' document.', 'Link 1', 'Link 2', 'Link 3']);
> > +
> 
> maybe you should use DOM node lists that allows you to switch to
> querySelector

Some of those strings represent DOM ids and some accessible names, so querySelector would not be all that useful. Unless I missed your point...

> @@ +74,5 @@
> > +      gBrowserWnd = window.openDialog(
> > +        "chrome://browser/content/", "_blank", "chrome,all,dialog=no",
> > +        "chrome://mochitests/content/a11y/accessible/pivot/doc_virtualcursor.html");
> > +
> > +      addA11yLoadEvent(startTests, gBrowserWnd);
> 
> consider to use browser.js

Don't understand what you mean here.

Comment 130

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Fixed your nits. Except for the two issues above.

Comment 131

[alexander :surkov (:asurkov)]

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

> Some of those strings represent DOM ids and some accessible names, so
> querySelector would not be all that useful. Unless I missed your point...

I meant don't use id attributes and accessible name for accessible identification but use DOM node instead. If you do that then you can use querySelector method.

> > @@ +74,5 @@
> > > +      gBrowserWnd = window.openDialog(
> > > +        "chrome://browser/content/", "_blank", "chrome,all,dialog=no",
> > > +        "chrome://mochitests/content/a11y/accessible/pivot/doc_virtualcursor.html");
> > > +
> > > +      addA11yLoadEvent(startTests, gBrowserWnd);
> > 
> > consider to use browser.js
> 
> Don't understand what you mean here.

browser.js (a11y mochitest folder) adds helper functions to open/close browser window and functions to get tab document and etc. So you don't need to invent the wheel.

Comment 132

[alexander :surkov (:asurkov)]

Comment on attachment 592783 [details] [diff] [review]
Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

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

::: accessible/src/base/nsAccessibilityService.h
@@ +533,5 @@
>    "hyperlink start index changed",           // EVENT_HYPERLINK_START_INDEX_CHANGED
>    "hypertext changed",                       // EVENT_HYPERTEXT_CHANGED
>    "hypertext links count changed",           // EVENT_HYPERTEXT_NLINKS_CHANGED
>    "object attribute changed",                // EVENT_OBJECT_ATTRIBUTE_CHANGED
> +  "virtual cursor changed",                  // EVENT_VIRTUALCURSOR_CHANGED

you don't need trailing comma

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +93,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mPosition, nsIAccessible)
> +  PRUint32 i, length = tmp->mObservers.Length();                               \
> +  for (i = 0; i < length; ++i) {
> +    cb.NoteXPCOMChild(tmp->mObservers.ElementAt(i).get());
> +  }

use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR?

::: accessible/src/base/nsAccessiblePivot.h
@@ +59,5 @@
>  
>    /*
>     * A simple getter for the pivot's position.
>     */
>    nsAccessible* Position() { return mPosition; }

usually we keep all public methods after XPCOM stuffs

::: accessible/src/base/nsDocAccessible.cpp
@@ +547,2 @@
>      mVirtualCursor = new nsAccessiblePivot(this);
> +    mVirtualCursor->AddObserver(this);

btw, I don't see where you remove observer and don't see that you release mVirtalCursor in Shutdown()

@@ +928,5 @@
> +// nsIAccessiblePivotObserver
> +
> +NS_IMETHODIMP nsDocAccessible::OnPivotChanged(nsIAccessiblePivot* aPivot,
> +                                              nsIAccessible* aOldAccessible,
> +                                              PRInt32 aOldStart, PRInt32 aOldEnd)

NS_IMETHODOMP on new line, iirc I mentioned that already

Comment 133

[alexander :surkov (:asurkov)]

Comment on attachment 592934 [details] [diff] [review]
Bug 698823 - Add mochitest for virtual cursor functionality.

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

::: accessible/tests/mochitest/pivot.js
@@ +1,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

new line please

@@ +5,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +////////////////////////////////////////////////////////////////////////////////
> +// Constants
> +
> +const PREFILTER_INVISIBLE = Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE;

btw, I think prefilter is not very good name because it makes me think that it's not predefined filter but sort of filter that's used prior to main filter (maybe that's my language issue though)

@@ +8,5 @@
> +
> +const PREFILTER_INVISIBLE = Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE;
> +const FILTER_MATCH = Ci.nsIAccessibleTraversalRule.FILTER_MATCH;
> +const FILTER_IGNORE = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> +const FILTER_IGNORE_SUBTREE = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE_SUBTREE;

I'd define nsIAccessibleTraversalRule constant probably in common.js

@@ +22,5 @@
> +  getMatchRoles: function(aRules)
> +  {
> +    aRules.value = [ROLE_HEADING];
> +    return aRules.value.length;
> +  },

it sounds usage something like nsIMutableArray should be more friednly to js (I don't like putting an array as out argument and not forgetting to returns its length as retvalue).
interface nsIBla
{
  void getMatchedRoles(in nsIMutableArray roles);
}

getMatchedRoles: function(aRoles)
{
  aRoles.AppendElement(ROLE_HEADING);
}
that's similar to nsITreeView

@@ +26,5 @@
> +  },
> +
> +  preFilter: PREFILTER_INVISIBLE,
> +
> +  matchAccessible: function(aAccessible)

I'm still thinking if we need 'Accessible' in the end of method and wouldn't it be better to have simple 'match'

@@ +36,5 @@
> +}
> +
> +/**
> + * Traversal rules for all focusable nodes or leafs.
> + */

rules -> rule
maybe: Rule object to traverse all focusable nodes and text nodes.

@@ +43,5 @@
> +  getMatchRoles: function(aRules)
> +  {
> +    aRules.value = [];
> +    return 0;
> +  },

for JS it's friendly to leave it unimplemented but I guess those nsresults will be propogated and it will fail

@@ +56,5 @@
> +        (role != ROLE_DOCUMENT && role != ROLE_INTERNAL_FRAME))
> +      rv = FILTER_IGNORE_SUBTREE | FILTER_MATCH;
> +    else if (aAccessible.childCount == 0 &&
> +             role != ROLE_STATICTEXT && aAccessible.name.trim())
> +      rv = FILTER_MATCH;

hm that works for something like disabled controls? why you don't check text_leaf role?

@@ +74,5 @@
> +
> +  this.check = function virtualCursorChangedChecker_check(aEvent)
> +  {
> +    var aAccessible = aDocAcc.virtualCursor.position;
> +    aAccessible.QueryInterface(Ci.nsIAccessNode);

aAccessible -> position
Ci.nsIAccessNode -> nsIAccessNode

@@ +83,5 @@
> +
> +    SimpleTest.ok(idMatches || nameMatches || accMatches, "id or name matches",
> +                  "expecting " + aIdOrNameOrAcc + ", got '" +
> +                  aAccessible.DOMNode.id + "'/'" + aAccessible.name + "' " +
> +                  prettyName(aAccessible));

prettyName prints id and accessible name
id or name matches -> id, name or accessible matches

@@ +88,5 @@
> +
> +    if (aTextOffsets) {
> +      SimpleTest.is(aDocAcc.virtualCursor.startOffset,
> +                    aTextOffsets[0],
> +                    "startOffset");

startOffset -> wrong start offset

@@ +91,5 @@
> +                    aTextOffsets[0],
> +                    "startOffset");
> +      SimpleTest.is(aDocAcc.virtualCursor.endOffset,
> +                    aTextOffsets[1],
> +                    "endOffset");

SimpleTest.ok/is -> ok/is

@@ +96,5 @@
> +    }
> +  };
> +}
> +
> +function setVirtualCursorOffsetInvoker(aDocAcc, aTextAccessible, aTextOffsets)

setVirutalCursorRangeInvoker?

comment it please

@@ +100,5 @@
> +function setVirtualCursorOffsetInvoker(aDocAcc, aTextAccessible, aTextOffsets)
> +{
> +  this.invoke = function virtualCursorChangedInvoker_invoke()
> +  {
> +    SimpleTest.info(aTextAccessible + " " + aTextOffsets);

SimpleTest.info -> info?
aTextAccessible -> prettyName?
something nice for aTextOffsets?

@@ +118,5 @@
> +  ];
> +}
> +
> +function setVirtualCursorPosInvoker(aDocAcc, aPivotMoveMethod, aRule,
> +                                        aIdOrNameOrAcc)

comment it, wrong indentation
to keep it shorter you can omit 'Invoker', we often do that for example, synthFocus in events.js

@@ +130,5 @@
> +  {
> +    return "Do " + aPivotMoveMethod;
> +  }
> +
> +  this.eventSeq = [new virtualCursorChangedChecker(aDocAcc, aIdOrNameOrAcc)];

spaces inside of []

@@ +133,5 @@
> +
> +  this.eventSeq = [new virtualCursorChangedChecker(aDocAcc, aIdOrNameOrAcc)];
> +}
> +
> +function virtualCursorNoChangedInvoker(aDocAcc, aPivotMoveMethod, aRule)

can you reuse setVirtualCursorPosInvoker treating absent aIdOrNameOrAcc?

@@ +153,5 @@
> +    new invokerChecker(EVENT_VIRTUALCURSOR_CHANGED, aDocAcc)
> +  ];
> +}
> +
> +function pushToSequenceFromRule(aQueue, aDocAcc, aRule, aSequence) {

description please

maybe: queueTraversalSequence?

@@ +158,5 @@
> +  aDocAcc.virtualCursor.position = null;
> +
> +  for (var i = 0; i < aSequence.length; i++) {
> +    aQueue.push(new setVirtualCursorPosInvoker(
> +      aDocAcc, "moveNext", aRule, aSequence[i]));

to make it styled properly:
var invoker = new setVirtualCursorPosInvoker(aDocAcc,
                                            bla);
aQueue.push(invoker);

@@ +175,5 @@
> +  aQueue.push(new setVirtualCursorPosInvoker(
> +    aDocAcc, "moveFirst", aRule, aSequence[0]));
> +}
> +
> +/* A utility for writing tests for testTraversal */

/**
 * comment
 */

@@ +176,5 @@
> +    aDocAcc, "moveFirst", aRule, aSequence[0]));
> +}
> +
> +/* A utility for writing tests for testTraversal */
> +function dumpTraversalSequence(aPivot, aRule) {

{ on new line
but it's not used, right? debuggin stuffs? if so add to comment

::: accessible/tests/mochitest/pivot/test_virtualcursor.html
@@ +31,5 @@
> +      var docAcc = getAccessible(doc, [nsIAccessibleDocument,
> +                                       nsIAccessibleCursorable]);
> +
> +      // Test that embedded documents don't have their own virtual cursor.
> +      is(docAcc.childDocumentCount, 1, "Expecting one child document");

what's it for? you test that it doesn't have vc

@@ +35,5 @@
> +      is(docAcc.childDocumentCount, 1, "Expecting one child document");
> +      var childDoc = docAcc.getChildDocumentAt(0);
> +      var supportsVC = true;
> +      try {
> +        childDoc.QueryInterface(Ci.nsIAccessibleCursorable);

no Ci is needed here

Comment 134

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #131)
> (In reply to Eitan Isaacson [:eeejay] from comment #129)
> 
> > Some of those strings represent DOM ids and some accessible names, so
> > querySelector would not be all that useful. Unless I missed your point...
> 
> I meant don't use id attributes and accessible name for accessible
> identification but use DOM node instead. If you do that then you can use
> querySelector method.
> 

Sorry, still don't understand how that is useful. Many of the expected accessibles are text nodes which are hard to get via CSS selectors.

> > > @@ +74,5 @@
> > > > +      gBrowserWnd = window.openDialog(
> > > > +        "chrome://browser/content/", "_blank", "chrome,all,dialog=no",
> > > > +        "chrome://mochitests/content/a11y/accessible/pivot/doc_virtualcursor.html");
> > > > +
> > > > +      addA11yLoadEvent(startTests, gBrowserWnd);
> > > 
> > > consider to use browser.js
> > 
> > Don't understand what you mean here.
> 
> browser.js (a11y mochitest folder) adds helper functions to open/close
> browser window and functions to get tab document and etc. So you don't need
> to invent the wheel.

I still can't find browser.js :(

Comment 135

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #132)
> Comment on attachment 592783 [details] [diff] [review]
> Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.
> 
> Review of attachment 592783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +93,5 @@
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mPosition, nsIAccessible)
> > +  PRUint32 i, length = tmp->mObservers.Length();                               \
> > +  for (i = 0; i < length; ++i) {
> > +    cb.NoteXPCOMChild(tmp->mObservers.ElementAt(i).get());
> > +  }
> 
> use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR?
> 

Nope, that macro uses array[i], and for some reason nsTObserverArray is unsubscriptable.

Comment 136

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

This was r+ed by Alex, did some last minor changes.

Comment 137

[Eitan Isaacson [:eeejay]]

You are bringing up issues in this review that are relevant to the first patch that you already r+ed. In any case, they are mostly valid points, so expect a patch update for the pivot interface as well.

(In reply to alexander :surkov from comment #133)
> Comment on attachment 592934 [details] [diff] [review]
> Bug 698823 - Add mochitest for virtual cursor functionality.
> 
> Review of attachment 592934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/pivot.js
> @@ +5,5 @@
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +////////////////////////////////////////////////////////////////////////////////
> > +// Constants
> > +
> > +const PREFILTER_INVISIBLE = Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE;
> 
> btw, I think prefilter is not very good name because it makes me think that
> it's not predefined filter but sort of filter that's used prior to main
> filter (maybe that's my language issue though)
> 

Yes, it was originally meant to mean a prior filter. This is when the main method was still called filterAccessible. the prefilter is called before the main filter method is used. I'm keeping it that way unless you have another idea.

> @@ +22,5 @@
> > +  getMatchRoles: function(aRules)
> > +  {
> > +    aRules.value = [ROLE_HEADING];
> > +    return aRules.value.length;
> > +  },
> 
> it sounds usage something like nsIMutableArray should be more friednly to js
> (I don't like putting an array as out argument and not forgetting to returns
> its length as retvalue).
> interface nsIBla
> {
>   void getMatchedRoles(in nsIMutableArray roles);
> }
> 
> getMatchedRoles: function(aRoles)
> {
>   aRoles.AppendElement(ROLE_HEADING);
> }
> that's similar to nsITreeView
> 

This is how it is done in 99% of other interfaces, and it is what Neil said I should do. JS does not integrate perfectly with XPCOM. Your approach definitely has advantages (a concrete  container class that could be put on the heap instead of an allocated buffer). I'll try to pull it off.

> @@ +26,5 @@
> > +  },
> > +
> > +  preFilter: PREFILTER_INVISIBLE,
> > +
> > +  matchAccessible: function(aAccessible)
> 
> I'm still thinking if we need 'Accessible' in the end of method and wouldn't
> it be better to have simple 'match'
> 

Sure. I wish you mentioned this in the previous patch you approved.

> @@ +43,5 @@
> > +  getMatchRoles: function(aRules)
> > +  {
> > +    aRules.value = [];
> > +    return 0;
> > +  },
> 
> for JS it's friendly to leave it unimplemented but I guess those nsresults
> will be propogated and it will fail
> 

If we go with your approach above, we could leave this empty.

> @@ +56,5 @@
> > +        (role != ROLE_DOCUMENT && role != ROLE_INTERNAL_FRAME))
> > +      rv = FILTER_IGNORE_SUBTREE | FILTER_MATCH;
> > +    else if (aAccessible.childCount == 0 &&
> > +             role != ROLE_STATICTEXT && aAccessible.name.trim())
> > +      rv = FILTER_MATCH;
> 
> hm that works for something like disabled controls? why you don't check
> text_leaf role?
> 

i am not only interested in text leafs, disabled controls should match too. The reason not to traverse down focusable items is so we don't get every option in a combobox, or the text leaf descendant of a link. This is not meant to be a perfect or useful rule, just adding some complexity to the match function.

> @@ +91,5 @@
> > +                    aTextOffsets[0],
> > +                    "startOffset");
> > +      SimpleTest.is(aDocAcc.virtualCursor.endOffset,
> > +                    aTextOffsets[1],
> > +                    "endOffset");
> 
> SimpleTest.ok/is -> ok/is
> 

For some odd reason SimpleTest.* is not in the global scope.


> @@ +100,5 @@
> > +function setVirtualCursorOffsetInvoker(aDocAcc, aTextAccessible, aTextOffsets)
> > +{
> > +  this.invoke = function virtualCursorChangedInvoker_invoke()
> > +  {
> > +    SimpleTest.info(aTextAccessible + " " + aTextOffsets);
> 
> SimpleTest.info -> info?
> aTextAccessible -> prettyName?
> something nice for aTextOffsets?

aTextOffsets turns into "[1, 5]", that is pretty!

> 
> @@ +118,5 @@
> > +  ];
> > +}
> > +
> > +function setVirtualCursorPosInvoker(aDocAcc, aPivotMoveMethod, aRule,
> > +                                        aIdOrNameOrAcc)
> 
> comment it, wrong indentation
> to keep it shorter you can omit 'Invoker', we often do that for example,
> synthFocus in events.js
> 

I don't think the length really hurts it.

> ::: accessible/tests/mochitest/pivot/test_virtualcursor.html
> @@ +31,5 @@
> > +      var docAcc = getAccessible(doc, [nsIAccessibleDocument,
> > +                                       nsIAccessibleCursorable]);
> > +
> > +      // Test that embedded documents don't have their own virtual cursor.
> > +      is(docAcc.childDocumentCount, 1, "Expecting one child document");
> 
> what's it for? you test that it doesn't have vc
> 

This is just an assertion before we retrieve the child document at index 0.

Comment 138

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #137)
> > @@ +22,5 @@
> > > +  getMatchRoles: function(aRules)
> > > +  {
> > > +    aRules.value = [ROLE_HEADING];
> > > +    return aRules.value.length;
> > > +  },
> > 
> > it sounds usage something like nsIMutableArray should be more friednly to js
> > (I don't like putting an array as out argument and not forgetting to returns
> > its length as retvalue).
> > interface nsIBla
> > {
> >   void getMatchedRoles(in nsIMutableArray roles);
> > }
> > 
> > getMatchedRoles: function(aRoles)
> > {
> >   aRoles.AppendElement(ROLE_HEADING);
> > }
> > that's similar to nsITreeView
> > 
> 
> This is how it is done in 99% of other interfaces, and it is what Neil said
> I should do. JS does not integrate perfectly with XPCOM. Your approach
> definitely has advantages (a concrete  container class that could be put on
> the heap instead of an allocated buffer). I'll try to pull it off.
> 

I don't think this could work.. an nsIMutableArray would expect XPCOM objects, so we would need to wrap each role integer in nsISupportsPRUint32, this would be ugly on the js side, and ugly on the implementation side when we need to unwrap each role.

The reason it works in the nsITreeView is because the values are nsIAtoms.

Comment 139

[alexander :surkov (:asurkov)]

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

> Sorry, still don't understand how that is useful. Many of the expected
> accessibles are text nodes which are hard to get via CSS selectors.

ok

> I still can't find browser.js :(

http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/browser.js

(In reply to Eitan Isaacson [:eeejay] from comment #135)
> (In reply to alexander :surkov from comment #132)

> > ::: accessible/src/base/nsAccessiblePivot.cpp
> > @@ +93,5 @@
> > > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mPosition, nsIAccessible)
> > > +  PRUint32 i, length = tmp->mObservers.Length();                               \
> > > +  for (i = 0; i < length; ++i) {
> > > +    cb.NoteXPCOMChild(tmp->mObservers.ElementAt(i).get());
> > > +  }
> > 
> > use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR?
> > 
> 
> Nope, that macro uses array[i], and for some reason nsTObserverArray is
> unsubscriptable.

ok, but then you should use NS_CYCLE_COLLECTION_NOTE_EDGE_NAME for each traversed item

about terms, that's called doesn't have overloaded operator []

(In reply to Eitan Isaacson [:eeejay] from comment #137)
> You are bringing up issues in this review that are relevant to the first
> patch that you already r+ed. In any case, they are mostly valid points, so
> expect a patch update for the pivot interface as well.

review is iterative process ;) during review of next part you can get questions/concerns to previous part

> > btw, I think prefilter is not very good name because it makes me think that
> > it's not predefined filter but sort of filter that's used prior to main
> > filter (maybe that's my language issue though)
> > 
> 
> Yes, it was originally meant to mean a prior filter. This is when the main
> method was still called filterAccessible. the prefilter is called before the
> main filter method is used. I'm keeping it that way unless you have another
> idea.

maybe simple 'filter'?

> > @@ +22,5 @@
> > > +  getMatchRoles: function(aRules)
> > > +  {
> > > +    aRules.value = [ROLE_HEADING];
> > > +    return aRules.value.length;
> > > +  },
> > 
> > it sounds usage something like nsIMutableArray should be more friednly to js
> > (I don't like putting an array as out argument and not forgetting to returns
> > its length as retvalue).
> > interface nsIBla
> > {
> >   void getMatchedRoles(in nsIMutableArray roles);
> > }
> > 
> > getMatchedRoles: function(aRoles)
> > {
> >   aRoles.AppendElement(ROLE_HEADING);
> > }
> > that's similar to nsITreeView
> > 
> 
> This is how it is done in 99% of other interfaces, and it is what Neil said
> I should do. JS does not integrate perfectly with XPCOM. Your approach
> definitely has advantages (a concrete  container class that could be put on
> the heap instead of an allocated buffer). I'll try to pull it off.

ok, if you follow best practice (I assume 99% shouldn't be wrong :) ) then do that.

> > @@ +26,5 @@
> > > +  },
> > > +
> > > +  preFilter: PREFILTER_INVISIBLE,
> > > +
> > > +  matchAccessible: function(aAccessible)
> > 
> > I'm still thinking if we need 'Accessible' in the end of method and wouldn't
> > it be better to have simple 'match'
> > 
> 
> Sure. I wish you mentioned this in the previous patch you approved.

Sorry, I wasn't sure at that point. It really helps to understand the theory when you see it in practice.

> > @@ +43,5 @@
> > > +  getMatchRoles: function(aRules)
> > > +  {
> > > +    aRules.value = [];
> > > +    return 0;
> > > +  },
> > 
> > for JS it's friendly to leave it unimplemented but I guess those nsresults
> > will be propogated and it will fail
> > 
> 
> If we go with your approach above, we could leave this empty.

right, but I meant what if JS omits this function at all. I think cursor won't work in this case. Anyway, up to you.

> 
> > @@ +56,5 @@
> > > +        (role != ROLE_DOCUMENT && role != ROLE_INTERNAL_FRAME))
> > > +      rv = FILTER_IGNORE_SUBTREE | FILTER_MATCH;
> > > +    else if (aAccessible.childCount == 0 &&
> > > +             role != ROLE_STATICTEXT && aAccessible.name.trim())
> > > +      rv = FILTER_MATCH;
> > 
> > hm that works for something like disabled controls? why you don't check
> > text_leaf role?
> > 
> 
> i am not only interested in text leafs, disabled controls should match too.

disabled controls having children like comboboxes won't match here.

> > SimpleTest.ok/is -> ok/is
> > 
> 
> For some odd reason SimpleTest.* is not in the global scope.

strange, ok

> > > +    SimpleTest.info(aTextAccessible + " " + aTextOffsets);
> > 
> > SimpleTest.info -> info?
> > aTextAccessible -> prettyName?
> > something nice for aTextOffsets?
> 
> aTextOffsets turns into "[1, 5]", that is pretty!

ok

> > > +function setVirtualCursorPosInvoker(aDocAcc, aPivotMoveMethod, aRule,
> > > +                                        aIdOrNameOrAcc)
> > to keep it shorter you can omit 'Invoker', we often do that for example,
> > synthFocus in events.js
> > 
> 
> I don't think the length really hurts it.

up to you, probably I would omit 'invoker' postfix

> > > +      // Test that embedded documents don't have their own virtual cursor.
> > > +      is(docAcc.childDocumentCount, 1, "Expecting one child document");
> > 
> > what's it for? you test that it doesn't have vc
> > 
> 
> This is just an assertion before we retrieve the child document at index 0.

it's not just an assertion, this is a test. If you want to have assertion then you should do something like
if (docAcc.childDocument != 1) {
  ok(false, "");
}

(In reply to Eitan Isaacson [:eeejay] from comment #138)
> > > getMatchedRoles: function(aRoles)
> > > {
> > >   aRoles.AppendElement(ROLE_HEADING);
> > > }
> > > that's similar to nsITreeView
> > > 
> > 
> > This is how it is done in 99% of other interfaces, and it is what Neil said
> > I should do. JS does not integrate perfectly with XPCOM. Your approach
> > definitely has advantages (a concrete  container class that could be put on
> > the heap instead of an allocated buffer). I'll try to pull it off.
> > 
> 
> I don't think this could work.. an nsIMutableArray would expect XPCOM
> objects, so we would need to wrap each role integer in nsISupportsPRUint32,
> this would be ugly on the js side, and ugly on the implementation side when
> we need to unwrap each role.

fair enough

Comment 140

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

renamed matchAccessible to match.

Comment 141

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Comment 142

[alexander :surkov (:asurkov)]

Comment on attachment 593284 [details] [diff] [review]
Bug 698823 - Add mochitest for virtual cursor functionality.

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

r=me

::: accessible/tests/mochitest/pivot.js
@@ +1,3 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

you don't use them expect single instance of Cu, so you can do Components.utils.import instead

@@ +57,5 @@
> +        (role != ROLE_DOCUMENT && role != ROLE_INTERNAL_FRAME))
> +      rv = FILTER_IGNORE_SUBTREE | FILTER_MATCH;
> +    else if (aAccessible.childCount == 0 &&
> +             role != ROLE_STATICTEXT && aAccessible.name.trim())
> +      rv = FILTER_MATCH;

impl notice: you don't need to ask to go into subtree because there is no subtree so returning FILTER_IGNORE_SUBTREE additionally make sense

@@ +68,5 @@
> +
> +////////////////////////////////////////////////////////////////////////////////
> +// Virtual state invokers and checkers
> +
> +function virtualCursorChangedChecker(aDocAcc, aIdOrNameOrAcc, aTextOffsets)

comment should be nice like virtual_cursor_changed event checker, used by setVirtualCursor invokers

@@ +94,5 @@
> +    }
> +  };
> +}
> +
> +/*

/**

@@ +95,5 @@
> +  };
> +}
> +
> +/*
> + * Set a text offset in the pivot and wait for right virtual cursor change event.

nit: Set range maybe or at least set text offsets
nit: omit "right" here, it doesn't make lot of sense here

@@ +118,5 @@
> +    new virtualCursorChangedChecker(aDocAcc, aTextAccessible, aTextOffsets)
> +  ];
> +}
> +
> +/*

/**

@@ +119,5 @@
> +  ];
> +}
> +
> +/*
> + * Move the pivot and wait for right virtual cursor change event.

please describe arguments

@@ +146,5 @@
> +    ];
> +  }
> +}
> +
> +/*

/**

@@ +149,5 @@
> +
> +/*
> + * Add invokers to a queue to test a rule and an expected sequence of element ids
> + * or accessible names for that rule in the given document.
> + */

please describe arguments

@@ +150,5 @@
> +/*
> + * Add invokers to a queue to test a rule and an expected sequence of element ids
> + * or accessible names for that rule in the given document.
> + */
> +function queueTraversalSequence(aQueue, aDocAcc, aRule, aSequence) {

{ on new line

@@ +159,5 @@
> +                                                 aRule, aSequence[i]);
> +    aQueue.push(invoker);
> +  }
> +
> +  aQueue.push(new setVirtualCursorPosInvoker(aDocAcc, "moveNext", aRule));

please comment it, it makes sense to pass null explicitly

@@ +161,5 @@
> +  }
> +
> +  aQueue.push(new setVirtualCursorPosInvoker(aDocAcc, "moveNext", aRule));
> +
> +  for (var i=aSequence.length-2;i>=0;i--) {

spaces between operand and operator

@@ +165,5 @@
> +  for (var i=aSequence.length-2;i>=0;i--) {
> +    var invoker = new setVirtualCursorPosInvoker(aDocAcc, "movePrevious",
> +                                                 aRule, aSequence[i])
> +    aQueue.push(invoker);
> +  }

no false move check for movePrevious, moveLast, moveFirst?

@@ +175,5 @@
> +    aDocAcc, "moveFirst", aRule, aSequence[0]));
> +}
> +
> +/**
> + * A debug utility for writing proper sequences for queueTraversalSequence

dot in the end

@@ +179,5 @@
> + * A debug utility for writing proper sequences for queueTraversalSequence
> + */
> +function dumpTraversalSequence(aPivot, aRule)
> +{
> +  aPivot.position = null;

I think I'd prefer to aPivot.moveFirst(aRule)/moveNext(aRule)

@@ +184,5 @@
> +  var sequence = []
> +  while (aPivot.moveNext(aRule)) {
> +    var aId;
> +    try {
> +      aId = aPivot.position.attributes.getStringProperty('id');

I'd prefer aPivot.position.QueryInterface(nsIAccessNode).DOMNode.id. We don't need to test object attributes here

@@ +188,5 @@
> +      aId = aPivot.position.attributes.getStringProperty('id');
> +    } catch (e) {
> +      aId = null;
> +    }
> +    sequence.push("'" + (aId || aPivot.position.name) + "'");

maybe prettyName of accessible?

::: accessible/tests/mochitest/pivot/test_virtualcursor.html
@@ +24,5 @@
> +      var rootAcc = getRootAccessible(gBrowserWnd.gBrowser.document);
> +      try {
> +        rootAcc.QueryInterface(nsIAccessibleCursorable);
> +      } catch (e) {
> +        ok(false, "Root accessible does not support nsIAccessibleCursorable");

SimpleTest.finish(); reutrn; ?

@@ +26,5 @@
> +        rootAcc.QueryInterface(nsIAccessibleCursorable);
> +      } catch (e) {
> +        ok(false, "Root accessible does not support nsIAccessibleCursorable");
> +      }
> +      var doc = gBrowserWnd.gBrowser.selectedBrowser.contentDocument;

please use browser.js

@@ +44,5 @@
> +      ok(!supportsVC, "no nsIAccessibleCursorable support in child document");
> +
> +      gQueue = new eventQueue();
> +
> +      gQueue.onFinish = function onFinish ()

no space between onFinish and ()

Comment 143

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #142)
> Comment on attachment 593284 [details] [diff] [review]
> Bug 698823 - Add mochitest for virtual cursor functionality.
> 
> Review of attachment 593284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: accessible/tests/mochitest/pivot.js
> @@ +57,5 @@
> > +        (role != ROLE_DOCUMENT && role != ROLE_INTERNAL_FRAME))
> > +      rv = FILTER_IGNORE_SUBTREE | FILTER_MATCH;
> > +    else if (aAccessible.childCount == 0 &&
> > +             role != ROLE_STATICTEXT && aAccessible.name.trim())
> > +      rv = FILTER_MATCH;
> 
> impl notice: you don't need to ask to go into subtree because there is no
> subtree so returning FILTER_IGNORE_SUBTREE additionally make sense
> 

I am hesitant of optimizing tests, that isn't really what they are meant for. Having more than one kind of rv value makes sense to me because we want to test as many code-paths as possible. So if this were a real user-facing feature I would do what you said. But, imho, since this is test code it should exercise the code and not make short-cuts.

> ::: accessible/tests/mochitest/pivot/test_virtualcursor.html
> @@ +24,5 @@
> > +      var rootAcc = getRootAccessible(gBrowserWnd.gBrowser.document);
> > +      try {
> > +        rootAcc.QueryInterface(nsIAccessibleCursorable);
> > +      } catch (e) {
> > +        ok(false, "Root accessible does not support nsIAccessibleCursorable");
> 
> SimpleTest.finish(); reutrn; ?
> 

It is worth proceeding with the test since it is not the root accessible we are testing with.

> @@ +26,5 @@
> > +        rootAcc.QueryInterface(nsIAccessibleCursorable);
> > +      } catch (e) {
> > +        ok(false, "Root accessible does not support nsIAccessibleCursorable");
> > +      }
> > +      var doc = gBrowserWnd.gBrowser.selectedBrowser.contentDocument;
> 
> please use browser.js
> 

I searched high and low for this browser.js and have not found it.

Comment 144

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

R-plussed by Alex already. This answers the nits.

P.S.
I found browser.js, needed to update my working copy :)

Comment 145

[alexander :surkov (:asurkov)]

Comment on attachment 585095 [details] [diff] [review]
Bug 698823 - Add pivot factory function to nsIAccessibleRetrieval and implement.

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

r=me

::: accessible/public/nsIAccessibleRetrieval.idl
@@ +117,5 @@
> +   * Create a new pivot for tracking a position and traversing a subtree.
> +   *
> +   * @param aAccessibleRoot - the root node for this pivot, cannot traverse
> +   *   above this node.
> +   * @return - a new pivot

nit: @param aRoot [in] the accessible root for the pivot
@return a new pivot

@@ +119,5 @@
> +   * @param aAccessibleRoot - the root node for this pivot, cannot traverse
> +   *   above this node.
> +   * @return - a new pivot
> +   */
> +  nsIAccessiblePivot createAccessiblePivot(in nsIAccessible aAccessibleRoot);

aRoot

::: accessible/src/base/nsAccessibilityService.cpp
@@ +875,5 @@
> +  nsRefPtr<nsAccessible> accessibleRoot(do_QueryObject(aAccessibleRoot));
> +  NS_ENSURE_TRUE(accessibleRoot, NS_ERROR_INVALID_ARG);
> +
> +  nsRefPtr<nsAccessiblePivot> pivot = new nsAccessiblePivot(accessibleRoot);
> +  *aPivot = pivot.forget().get();

nsAccessiblePivot* pivot = new nsAccessiblePivot(root);
NS_ADDREF(pivot);

Comment 146

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add pivot factory function to nsIAccessibleRetrieval and implement.

Formally approved by Alex.

Comment 147

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessiblePivot interface and implement.

Rebased patch set

Comment 148

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add pivot factory function to nsIAccessibleRetrieval and implement.

Comment 149

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add nsIAccessibleCursorable and implement.

Comment 150

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.

Comment 151

[Eitan Isaacson [:eeejay]]

Attachment: Bug 698823 - Add mochitest for virtual cursor functionality.

Comment 152

[Eitan Isaacson [:eeejay]]

Who wants to check in a 1,453 line patch?? The order is:

Bug 698823 - Add nsIAccessiblePivot interface and implement.
Bug 698823 - Add pivot factory function to nsIAccessibleRetrieval and implement.
Bug 698823 - Add nsIAccessibleCursorable and implement.
Bug 698823 - Add EVENT_VIRTUALCURSOR_CHANGED and implement.
Bug 698823 - Add mochitest for virtual cursor functionality.

Comment 153

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/7707a591896f

Comment 154

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7707a591896f

Comment 156

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

Folks, do you think we should add the dev-doc-needed keyword?

Comment 157

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #156)
> Folks, do you think we should add the dev-doc-needed keyword?

we should, at least we need to ensure that interfaces are documented. They might be autogenerated but anyway it's worth if doc team take a look.