Initial support for Ranges over automation nodes (used to track ChromeVox focus).

Initial support for Ranges used to track ChromeVox focus.

Committed: https://crrev.com/0b414646653172bf97ab93328261747a750681e1
Cr-Commit-Position: refs/heads/master@{#301137}

[David Tseng, 2014-10-16 22:07:16]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org, plundblad@chromium.org

[David Tseng, 2014-10-16 22:09:03]

Ready for an initial look; still working on adding e2e tests (BackgroundTest).

Lines now work. ChromeVox Next, now with the fix in for automation API lag, is
already usable on many static pages and pretty responsive.

[dmazzoni, 2014-10-17 16:46:08]

I'd like more low-level unit tests of individual functions and classes, so we
can debug any potential issues with tree computations or range objects in
isolation, independent of the rest of the code.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:38:
* @param {chrome.automation.AutomationNode} cur Node to begin the search from.
Please replace all chrome.automation.AutomationNode with AutomationNode now that
you're adding it to scope.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:160:
* Returns ancestors of the given node (inclusive).
Specify the order in which it returns the ancestors - from current node to root,
or from root to node?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:178:
AutomationUtil.isBefore = function(first, second) {
How about a unit test for this?

I think it's worth taking the time to write a test...we need this type of
function to be testable - maybe a concise way to create mock AutomationNode
trees for tests, maybe from JSON?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:194:
// TODO(dtseng): Expose parent in index (chrome.automation.AutomationNode).
How about writing an indexInParent helper, which could be eliminated once it's
exposed? That seems more foolproof, you don't have to think about this logic
again later.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
this.range_ = null;
Does this mean the current selected object? Maybe currentRange_ or activeRange_?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:323: *
Normalizes this range to point forward.
I'm confused - the description makes it sound like this modifies the range in
place, but it returns a bool.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:326: if
(this.start.node == this.end.node)
nit: ===?

[Peter Lundblad, 2014-10-23 13:35:08]

Hi,

I'd also like to see unit tests for the utilities and
other simple classes being added.
For cursors and ragnes, have you considered making these objects immutable?
In that way, methods would return new objects instead of mutating
'this'.  You can then pass these simple objects around without fearing
that they will be modified by some other code that also happens to have
a reference to the same object.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:160:
* Returns ancestors of the given node (inclusive).
If order is guaranteed, specify that.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:164:
AutomationUtil.getAncestors = function(node) {
nit: consider calling getAncestorsAndSelf or something that makes the
inclusive aspect clear (I know it's verbose).

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:173:
* Returns whether |first| comes before |second| in Dir.FORWARD traversal.
Should specify if that this is not strict.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:184:
var lcaIndex;
What does 'lca' mean?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:196:
firstCount++;
Looks like one increment too much here.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:201:
};
This function is unnecessarily cryptic because of the short names.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
this.range_ = null;
Either make the name more self-explanatory or add a description in the jsdoc.

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:245:
this.end = end;
Are these members meant to be public?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:326: if
(this.start.node == this.end.node)
Do we want to be consistent and use === when comparing nodes?

https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:329: }
Is this the right implementation for this function?

[chromium-reviews, 2014-10-23 23:59:28]

PTAL; addressed all comments (most were made obsolete after removing
most changes to automation_util.js).

On Thu 23 Oct 2014 06:35:07 AM PDT, plundblad@chromium.org wrote:

 > Hi,
 > 
 > I'd also like to see unit tests for the utilities and
 > other simple classes being added.
 > For cursors and ragnes, have you considered making these objects immutable?
 > In that way, methods would return new objects instead of mutating
 > 'this'.  You can then pass these simple objects around without fearing
 > that they will be modified by some other code that also happens to have
 > a reference to the same object.
 > 
 > 
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:160:
 > * Returns ancestors of the given node (inclusive).
 > If order is guaranteed, specify that.
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:164:
 > AutomationUtil.getAncestors = function(node) {
 > nit: consider calling getAncestorsAndSelf or something that makes the
 > inclusive aspect clear (I know it's verbose).
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:173:
 > * Returns whether |first| comes before |second| in Dir.FORWARD
 > traversal.
 > Should specify if that this is not strict.
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:184:
 > var lcaIndex;
 > What does 'lca' mean?
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:196:
 > firstCount++;
 > Looks like one increment too much here.
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:201:
 > };
 > This function is unnecessarily cryptic because of the short names.
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
 > this.range_ = null;
 > Either make the name more self-explanatory or add a description in the
 > jsdoc.
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:245:
 > this.end = end;
 > Are these members meant to be public?
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:326:
 > if (this.start.node == this.end.node)
 > Do we want to be consistent and use === when comparing nodes?
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:329:
 > }
 > Is this the right implementation for this function?
 > 
 > https://codereview.chromium.org/637223006/
 > 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-10-24 16:01:07]

On Fri 17 Oct 2014 09:46:08 AM PDT, dmazzoni@chromium.org wrote:

 > I'd like more low-level unit tests of individual functions and classes, so  
 > we
 > can debug any potential issues with tree computations or range objects in
 > isolation, independent of the rest of the code.
 > 
Done. Eliminated most additions to automation util.
 > 
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:38:
 > * @param {chrome.automation.AutomationNode} cur Node to begin the search
 > from.
 > Please replace all chrome.automation.AutomationNode with AutomationNode
 > now that you're adding it to scope.
 >
 Done.
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:160:
 > * Returns ancestors of the given node (inclusive).
 > Specify the order in which it returns the ancestors - from current node
 > to root, or from root to node?

 > Obsolete.
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:178:
 > AutomationUtil.isBefore = function(first, second) {
 > How about a unit test for this?


 > I think it's worth taking the time to write a test...we need this type
 > of function to be testable - maybe a concise way to create mock
 > AutomationNode trees for tests, maybe from JSON?

Ditto. > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:194:
 > // TODO(dtseng): Expose parent in index
 > (chrome.automation.AutomationNode).
 > How about writing an indexInParent helper, which could be eliminated
 > once it's exposed? That seems more foolproof, you don't have to think
 > about this logic again later.
 > 
Ditto.
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
 > this.range_ = null;
 > Does this mean the current selected object? Maybe currentRange_ or
 > activeRange_?
 > 
Done.
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:323:
 > * Normalizes this range to point forward.
 > I'm confused - the description makes it sound like this modifies the
 > range in place, but it returns a bool.
 > 
Obsolete.
 >
https://codereview.chromium.org/637223006/diff/20001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:326:
 > if (this.start.node == this.end.node)
 > nit: ===?
 > 
Done.
 > https://codereview.chromium.org/637223006/
 > 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-10-24 16:13:30]

lgtm

I love the tests!

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
(right):

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:33:
var AutomationNode = chrome.automation.AutomationNode;
Replace chrome.automation.AutomationNode with AutomationNode if you add this to
scope?

Or just revert changes to this file since I don't see any other changes.

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
this.activeRange_ = null;
Since we use "active" to mean whether ChromeVox is active at all, and since
you're using "current" as the local variable name, maybe this should be
currentRange_?

Alternate idea: virtualRange (like virtual cursor)

Or, we could rename active_ to running_?

Sorry to nitpick, but it seems like the best time to get these things right
while the ChromeVox Next codebase is small. It will make it so much more
readable in the future.

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:193:
current.getStart().getNode().focus();
Perhaps this should call a method on current that says to focus any focusable
elements contained within the range? For example, if the active range is a
single line containing a link, we should focus the link even though it's not the
start node.

OK to just mark as TODO, or write the helper function but not fully implement it
yet.

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:159: var
i = 0; i < this.node_.attributes.wordStarts.length; i++) {
funny indentation?

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs
(right):

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:21:
/** Test cursors.Range. @const {string} */
nit: indent

https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:60:
*     cursors.Unit|
funny indentation?

[David Tseng, 2014-10-24 16:32:10]

On Fri 24 Oct 2014 09:13:29 AM PDT, dmazzoni@chromium.org wrote:

 > lgtm
 > 
 > I love the tests!
 > 
 > 
 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 > File
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_util.js:33:
 > var AutomationNode = chrome.automation.AutomationNode;
 > Replace chrome.automation.AutomationNode with AutomationNode if you add
 > this to scope?
 > 
 > Or just revert changes to this file since I don't see any other changes.

Left as is since it's less verbose.
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:52:
 > this.activeRange_ = null;
 > Since we use "active" to mean whether ChromeVox is active at all, and
 > since you're using "current" as the local variable name, maybe this
 > should be currentRange_?


I like currentRange_ best out of the proposed names here.

> 

 > Alternate idea: virtualRange (like virtual cursor)

 > 
 > Or, we could rename active_ to running_?
 > 
 > Sorry to nitpick, but it seems like the best time to get these things
 > right while the ChromeVox Next codebase is small. It will make it so
 > much more readable in the future.
 >
 Agreed.


 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:193:
 > current.getStart().getNode().focus();
 > Perhaps this should call a method on current that says to focus any
 > focusable elements contained within the range? For example, if the
 > active range is a single line containing a link, we should focus the
 > link even though it's not the start node.
 > 
 > OK to just mark as TODO, or write the helper function but not fully
 > implement it yet.

I'll mark as TODO; I'm curious, what happens if we call focus on the
LCA; will Blink do anything smart (such as finding a focusable
descendant).


 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:159:
 > var i = 0; i < this.node_.attributes.wordStarts.length; i++) {
 > funny indentation?

Done.> 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 > File
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs
 > (right):
 > 
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:21:
 > /** Test cursors.Range. @const {string} */
 > nit: indent
 >
 Done.
 >
https://codereview.chromium.org/637223006/diff/40001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:60:
 > *     cursors.Unit|
 > funny indentation?
 >
 Done.
 > https://codereview.chromium.org/637223006/
 > 
 > To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
 > 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-10-24 16:37:42]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:38:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637223006/120001

[chromium-reviews, 2014-10-24 16:43:41]

>
> I'll mark as TODO; I'm curious, what happens if we call focus on the
> LCA; will Blink do anything smart (such as finding a focusable
> descendant).
>

No, we should assume it's dumb / literal and figure out what element to
focus specifically.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-10-24 17:36:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-24 17:37:11]

Patchset 7 (id:??) landed as
https://crrev.com/0b414646653172bf97ab93328261747a750681e1
Cr-Commit-Position: refs/heads/master@{#301137}