Finish implementing ChromeVox Next active indicator.

Finish implementing ChromeVox Next active indicator.

1. Scrolls the page when navigating to a node, and updates the active indicator
location when a scroll event is received.

2. Update the active indicator location on every Output, rather than only
for speech or only for braille. (It was sometimes getting set and then
overwritten with nothing.)

3. Handle subranges within a node with a new boundsForRange API.

4. Automation API: implement boundsForRange, take frame offsets and
scroll offsets into account when returning a node's bounding box,
and use heuristics to avoid returning an empty bounding box.

BUG=537248
TBR=aboxhall

Committed: https://crrev.com/05affbfa8212759600659294afb7f077a25b4b95
Cr-Commit-Position: refs/heads/master@{#361160}

[dmazzoni, 2015-11-18 00:01:12]

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

[dmazzoni, 2015-11-19 17:11:34]

Friendly ping

[Peter Lundblad, 2015-11-20 11:15:03]

You can drop 3. from the description, since that was already done separately.
(or rephase it if you decide to keep the extra guard)

Some kind of apitest for the new api would be nice.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:440:
* @return {{top: number, left: number, height: number, width: number}}
Return value should be {({top: number, left: number, height: number, width:
number}|undefined)}

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

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:149:
value.start.node.makeVisible();
I recommend setting the value first or ignoring exceptions fro makeVisible.
Otherwise, chromevox could get stuck if makeVisible would for some
reason throw and makeVisible is not critical for the operation of chromevox.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:255:
*/
@override

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:258:
global.backgroundObj.currentRange, null, evt.type)
currentRange can be null, but this parameter is not nullable. Not sure why the
compiler doesn't catch that.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:757: if
(chrome.accessibilityPrivate)
This guard wouldn't be enoguh on Linux, where the namespace seems to be
available. When do we anticipate it wouldn't be available and do we need to
update other places where we refer to it? At least we have another call to
setFocusRing in bacgkround.sj that should be maed consistent.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1199:
range.start.node.boundsForRange(startIndex, endIndex));
Cool, it is nice to not have to use document selection for this as I think we
had to do in classic!

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:340: // Computes the bounding box of
a subrange of this node within the frame.
Sorry for being ignorant about coordinate systems and stuff, but what does 'in
the frame' mean here? It looks to me like you're making the coordinate screen
absolute, but maybe I am missing something. Do you want to be clear that this
only works for inline text boxes?

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:342: static automation.Rect
boundsForRange(long startIndex, long endIndex);
Document what the start and end indices index into.

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:533:
location.set_x(location.x() + start_offset);
Could this need to be extended for top-to-bottom text? (I don't know if that's
at all supported and if so how it would be represented).

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/automation/automation_node.js:129: *
@return {?automation.Rect} The bounding box of the subrange of this node,
FWIW, the file is consistently wrong when it comes to these annotations.
The question mark means nullable, but the prose talks about undefined.
Feel free to ignore in this cl, but we should clean it up.
Also, the question mark would be redundant for an object.

[dmazzoni, 2015-11-20 17:05:17]

Description was changed from

==========
Finish implementing ChromeVox Next active indicator.

1. Scrolls the page when navigating to a node, and updates the active indicator
location when a scroll event is received.

2. Update the active indicator location on every Output, rather than only
for speech or only for braille. (It was sometimes getting set and then
overwritten with nothing.)

3. Only call accessibilityPrivate if the API exists, so this doesn't break
the webstore version.

4. Handle subranges within a node with a new boundsForRange API.

5. Automation API: implement boundsForRange, take frame offsets and
scroll offsets into account when returning a node's bounding box,
and use heuristics to avoid returning an empty bounding box.

BUG=537248
==========

to

==========
Finish implementing ChromeVox Next active indicator.

1. Scrolls the page when navigating to a node, and updates the active indicator
location when a scroll event is received.

2. Update the active indicator location on every Output, rather than only
for speech or only for braille. (It was sometimes getting set and then
overwritten with nothing.)

3. Handle subranges within a node with a new boundsForRange API.

4. Automation API: implement boundsForRange, take frame offsets and
scroll offsets into account when returning a node's bounding box,
and use heuristics to avoid returning an empty bounding box.

BUG=537248
==========

[dmazzoni, 2015-11-20 23:35:15]

Addressed feedback, modified comment, and added an automation api test for
boundsForRange.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/chrome_extension_externs.js:440:
* @return {{top: number, left: number, height: number, width: number}}
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> Return value should be {({top: number, left: number, height: number, width:
> number}|undefined)}

Done.

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

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:149:
value.start.node.makeVisible();
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> I recommend setting the value first or ignoring exceptions fro makeVisible.
> Otherwise, chromevox could get stuck if makeVisible would for some
> reason throw and makeVisible is not critical for the operation of chromevox.

Done.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:255:
*/
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> @override

Done.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:258:
global.backgroundObj.currentRange, null, evt.type)
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> currentRange can be null, but this parameter is not nullable. Not sure why the
> compiler doesn't catch that.

Huh. Added a null check.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:757: if
(chrome.accessibilityPrivate)
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> This guard wouldn't be enoguh on Linux, where the namespace seems to be
> available. When do we anticipate it wouldn't be available and do we need to
> update other places where we refer to it? At least we have another call to
> setFocusRing in bacgkround.sj that should be maed consistent.

Agreed, testing for isChromeOS makes more sense. Done.

https://codereview.chromium.org/1458723002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1199:
range.start.node.boundsForRange(startIndex, endIndex));
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> Cool, it is nice to not have to use document selection for this as I think we
> had to do in classic!

Acknowledged.

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:340: // Computes the bounding box of
a subrange of this node within the frame.
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> Sorry for being ignorant about coordinate systems and stuff, but what does 'in
> the frame' mean here? It looks to me like you're making the coordinate screen
> absolute, but maybe I am missing something. Do you want to be clear that this
> only works for inline text boxes?

Good catch. Actually this returns global screen coordinates now, when I fixed it
a while ago I forgot to update the docs here. Fixed both.

I think doing the local to global conversion in C++ makes far more sense.

https://codereview.chromium.org/1458723002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:342: static automation.Rect
boundsForRange(long startIndex, long endIndex);
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> Document what the start and end indices index into.

Done.

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:533:
location.set_x(location.x() + start_offset);
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> Could this need to be extended for top-to-bottom text? (I don't know if that's
> at all supported and if so how it would be represented).

Yes! We were already exposing this info, wasn't hard to implement all four
directions.

Testing it ended up being a lot more work, but it will be good to have.

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/1458723002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/automation/automation_node.js:129: *
@return {?automation.Rect} The bounding box of the subrange of this node,
On 2015/11/20 11:15:03, Peter Lundblad wrote:
> FWIW, the file is consistently wrong when it comes to these annotations.
> The question mark means nullable, but the prose talks about undefined.
> Feel free to ignore in this cl, but we should clean it up.
> Also, the question mark would be redundant for an object.

Acknowledged.

[Peter Lundblad, 2015-11-23 13:39:08]

lgtm

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

https://codereview.chromium.org/1458723002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:256:
* @param {Object} evt
nit: I think this @param is redundant given override.

https://codereview.chromium.org/1458723002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1458723002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:342: // coordinates.  Returns the
same as |location| if range information
Uber-nit: the standard in this file is one space after period.

https://codereview.chromium.org/1458723002/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1458723002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:529: if (start
>= 0 && start <= len && end >= 0 && end <= len &&
I think 
          if (start >= 0 && start <= end && end <= len) {
would be enough.

[dmazzoni, 2015-11-23 17:22:32]

Description was changed from

==========
Finish implementing ChromeVox Next active indicator.

1. Scrolls the page when navigating to a node, and updates the active indicator
location when a scroll event is received.

2. Update the active indicator location on every Output, rather than only
for speech or only for braille. (It was sometimes getting set and then
overwritten with nothing.)

3. Handle subranges within a node with a new boundsForRange API.

4. Automation API: implement boundsForRange, take frame offsets and
scroll offsets into account when returning a node's bounding box,
and use heuristics to avoid returning an empty bounding box.

BUG=537248
==========

to

==========
Finish implementing ChromeVox Next active indicator.

1. Scrolls the page when navigating to a node, and updates the active indicator
location when a scroll event is received.

2. Update the active indicator location on every Output, rather than only
for speech or only for braille. (It was sometimes getting set and then
overwritten with nothing.)

3. Handle subranges within a node with a new boundsForRange API.

4. Automation API: implement boundsForRange, take frame offsets and
scroll offsets into account when returning a node's bounding box,
and use heuristics to avoid returning an empty bounding box.

BUG=537248
TBR=aboxhall
==========

[dmazzoni, 2015-11-23 17:22:49]

TBR=aboxhall for 	chrome/browser/extensions/api/automation/automation_apitest.cc

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

https://codereview.chromium.org/1458723002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:256:
* @param {Object} evt
On 2015/11/23 13:39:08, Peter Lundblad wrote:
> nit: I think this @param is redundant given override.

Done.

https://codereview.chromium.org/1458723002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/1458723002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:342: // coordinates.  Returns the
same as |location| if range information
On 2015/11/23 13:39:08, Peter Lundblad wrote:
> Uber-nit: the standard in this file is one space after period.

Done.

https://codereview.chromium.org/1458723002/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1458723002/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:529: if (start
>= 0 && start <= len && end >= 0 && end <= len &&
On 2015/11/23 13:39:08, Peter Lundblad wrote:
> I think 
>           if (start >= 0 && start <= end && end <= len) {
> would be enough.

Done.

[dmazzoni, 2015-11-23 17:23:02]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-23 17:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1458723002/#ps40001
(title: "Address final nits")

[commit-bot: I haz the power, 2015-11-23 17:24:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458723002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458723002/40001

[dmazzoni, 2015-11-23 18:26:00]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-23 18:26:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1458723002/#ps60001
(title: "Made test expectations approximate")

[commit-bot: I haz the power, 2015-11-23 18:27:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458723002/60001

[commit-bot: I haz the power, 2015-11-23 20:13:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-23 20:14:57]

Patchset 4 (id:??) landed as
https://crrev.com/05affbfa8212759600659294afb7f077a25b4b95
Cr-Commit-Position: refs/heads/master@{#361160}