Implemented scanning by group

Implemented scanning by group.

A group is a node that has more than one interesting descendant split up among its child subtrees. An interesting node is one that a user can perform an action on, such as clicking or focusing on it.

In this form of scanning, rather than stepping through every item on the screen, a user steps across groups of items on the screen and selects the group containing the item they're interested in. This changes the current scope, letting the user step across items and sub-groups within that group. This lets the user navigate the desktop hierarchically and reach items more efficiently.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2863613003
Cr-Commit-Position: refs/heads/master@{#469875}
Committed: https://chromium.googlesource.com/chromium/src/+/fc330ee6e339230c34b73c54cb65f9ff6a4a20f1

[elichtenberg, 2017-05-05 01:02:23]

Description was changed from

==========
Implemented grouping algorithm

BUG=593885
==========

to

==========
Implemented grouping algorithm

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[elichtenberg, 2017-05-05 01:02:48]

The CQ bit was checked by elichtenberg@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 01:03:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[elichtenberg, 2017-05-05 01:04:01]

elichtenberg@google.com changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:59:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:59:34]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-05-05 18:23:19]

lgtm

Just a few suggestions, mostly for readability.
Code looks great, and I'm excited to see this.

Please update the change description with a
concise summary of the grouping algorithm.
Your one-sentence explanation of what a group is
would be great, then maybe one more sentence
explaining the behavior of navigating hierarchically
by selecting groups and changing scope.

Doesn't need to be as detailed as the design
doc, just enough to give a quick explanation of
what this changelist does if someone else reads
it - especially in the future, if someone's
debugging and looking back over this.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/automation_manager.js
(right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:34: * A
list of past subtree roots. Allows user to traverse back to previous
Would it be more clear to say this is a list of past
scopes?

Also, document whether the most recent is at the beginning
or end of the list.

Is this a stack? Maybe you could call it scopeStack so that
it's more intuitive that you add and pop stacks from the end
of the list.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:90: *
Select the currently highlighted node. If the node is a group, create a
Also document what happens if you're on the current scope.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:131: // If
no explicit start node, start walking the tree from |scope|.
Does it start at |scope|, or at the first child?

Intuitively when you click on a group, shouldn't it jump
to the first child within the group?

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/automation_predicate.js
(right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:32: *
its outer group, |scope|.
How about "it cannot have the same bounding box as its scope".

(Even though we use "location" as the api, intuitively some people
thing of location as just the x, y but here we mean the whole
bounding box.)

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:66:
AutomationPredicate.hasSameLocation_ = function(node1, node2) {
Don't have to do anything now, but I wonder if this should be
slightly "fuzzy" - i.e. if the difference between them is less
than some number of pixels, consider them the same.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:101: if
(state[chrome.automation.StateType.OFFSCREEN]
Consider defining shorthand for chrome.automation.StateType
and chrome.automation.RoleType at the top of the file kind of
like how ChromeVox does it. I know there were difficulties
doing this with the Closure Compiler and tests, so you don't
have to worry about it for this change if it's not easy.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/tree_walker.js (right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:62: * previous
node.
Maybe mention that it stays within scope_?

[dmazzoni, 2017-05-05 18:25:52]

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/automation_manager.js
(right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:103: if
(!oldScope)
One possible bug - it's possible that the old scope is no
longer valid.

For example, imagine someone navigates to the close box for
a window or dialog and clicks on it. The previous scope may
have been the window itself, which no longer exists. You should
keep walking up until you find a valid scope.

An invalid node will still be a valid JavaScript object, but
all of its attributes like role and state will return undefined.

[elichtenberg, 2017-05-06 00:07:54]

Description was changed from

==========
Implemented grouping algorithm

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implemented scanning by group.

A group is a node that has more than one interesting descendant split up among
its child subtrees. An interesting node is one that a user can perform an action
on, such as clicking or focusing on it.

In this form of scanning, rather than stepping through every item on the screen,
a user steps across groups of items on the screen and selects the group
containing the item they're interested in. This changes the current scope,
letting the user step across items and sub-groups within that group. This lets
the user navigate the desktop hierarchically and reach items more efficiently.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[elichtenberg, 2017-05-06 00:13:09]

The CQ bit was checked by elichtenberg@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 00:13:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[elichtenberg, 2017-05-06 00:13:53]

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/automation_manager.js
(right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:34: * A
list of past subtree roots. Allows user to traverse back to previous
On 2017/05/05 18:23:19, dmazzoni wrote:
> Would it be more clear to say this is a list of past
> scopes?
> 
> Also, document whether the most recent is at the beginning
> or end of the list.
> 
> Is this a stack? Maybe you could call it scopeStack so that
> it's more intuitive that you add and pop stacks from the end
> of the list.

Done.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:90: *
Select the currently highlighted node. If the node is a group, create a
On 2017/05/05 18:23:19, dmazzoni wrote:
> Also document what happens if you're on the current scope.

Done.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:103: if
(!oldScope)
On 2017/05/05 18:25:52, dmazzoni wrote:
> One possible bug - it's possible that the old scope is no
> longer valid.
> 
> For example, imagine someone navigates to the close box for
> a window or dialog and clicks on it. The previous scope may
> have been the window itself, which no longer exists. You should
> keep walking up until you find a valid scope.
> 
> An invalid node will still be a valid JavaScript object, but
> all of its attributes like role and state will return undefined.

Done.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:131: // If
no explicit start node, start walking the tree from |scope|.
On 2017/05/05 18:23:19, dmazzoni wrote:
> Does it start at |scope|, or at the first child?
> 
> Intuitively when you click on a group, shouldn't it jump
> to the first child within the group?

I agree. I changed selectCurrentNode so that when you select a group, you
automatically move to the next node. I didn't want to change createTreeWalker
though since we won't always want to move to the next node (e.g., when you
select the current scope).

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/automation_predicate.js
(right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:32: *
its outer group, |scope|.
On 2017/05/05 18:23:19, dmazzoni wrote:
> How about "it cannot have the same bounding box as its scope".
> 
> (Even though we use "location" as the api, intuitively some people
> thing of location as just the x, y but here we mean the whole
> bounding box.)

Done.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:66:
AutomationPredicate.hasSameLocation_ = function(node1, node2) {
On 2017/05/05 18:23:19, dmazzoni wrote:
> Don't have to do anything now, but I wonder if this should be
> slightly "fuzzy" - i.e. if the difference between them is less
> than some number of pixels, consider them the same.

Good idea. That's probably something we should look into.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/automation_predicate.js:101: if
(state[chrome.automation.StateType.OFFSCREEN]
On 2017/05/05 18:23:19, dmazzoni wrote:
> Consider defining shorthand for chrome.automation.StateType
> and chrome.automation.RoleType at the top of the file kind of
> like how ChromeVox does it. I know there were difficulties
> doing this with the Closure Compiler and tests, so you don't
> have to worry about it for this change if it's not easy.

Adding a TODO for this. Hopefully I can find an easier way to do this.

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/switch_access/tree_walker.js (right):

https://codereview.chromium.org/2863613003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/switch_access/tree_walker.js:62: * previous
node.
On 2017/05/05 18:23:19, dmazzoni wrote:
> Maybe mention that it stays within scope_?

Done.

[commit-bot: I haz the power, 2017-05-06 01:54:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-06 01:54:50]

Dry run: This issue passed the CQ dry run.

[elichtenberg, 2017-05-06 02:58:15]

The CQ bit was checked by elichtenberg@google.com

[elichtenberg, 2017-05-06 02:58:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2863613003/#ps20001
(title: "Responded to comments")

[commit-bot: I haz the power, 2017-05-06 02:58:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 19:15:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494039495226320,
"parent_rev": "9c9cc15d81514bb92abaa3252c7c7b01c3e47bc4", "commit_rev":
"fc330ee6e339230c34b73c54cb65f9ff6a4a20f1"}

[commit-bot: I haz the power, 2017-05-06 19:15:10]

Description was changed from

==========
Implemented scanning by group.

A group is a node that has more than one interesting descendant split up among
its child subtrees. An interesting node is one that a user can perform an action
on, such as clicking or focusing on it.

In this form of scanning, rather than stepping through every item on the screen,
a user steps across groups of items on the screen and selects the group
containing the item they're interested in. This changes the current scope,
letting the user step across items and sub-groups within that group. This lets
the user navigate the desktop hierarchically and reach items more efficiently.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implemented scanning by group.

A group is a node that has more than one interesting descendant split up among
its child subtrees. An interesting node is one that a user can perform an action
on, such as clicking or focusing on it.

In this form of scanning, rather than stepping through every item on the screen,
a user steps across groups of items on the screen and selects the group
containing the item they're interested in. This changes the current scope,
letting the user step across items and sub-groups within that group. This lets
the user navigate the desktop hierarchically and reach items more efficiently.

BUG=593885
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2863613003
Cr-Commit-Position: refs/heads/master@{#469875}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc330ee6e339230c34b73c54cb65...
==========

[commit-bot: I haz the power, 2017-05-06 19:15:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/fc330ee6e339230c34b73c54cb65...