Correctly follows focus and moves to valid node when current becomes invalid

Correctly follows focus and moves to valid node when current becomes invalid

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

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

[elichtenberg, 2017-05-11 01:54:22]

Description was changed from

==========
Handling focus change and being stuck at invalid nodes. Need to finish dealing
with loading new pages after clicking link

BUG=
==========

to

==========
Handling focus change and being stuck at invalid nodes. Need to finish dealing
with loading new pages after clicking link

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

[elichtenberg, 2017-05-18 00:37:47]

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

[elichtenberg, 2017-05-18 00:38:41]

Description was changed from

==========
Handling focus change and being stuck at invalid nodes. Need to finish dealing
with loading new pages after clicking link

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

to

==========
Handling focus change and being stuck at invalid nodes. Need to finish dealing
with loading new pages after clicking link

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

[elichtenberg, 2017-05-18 00:39:17]

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

[commit-bot: I haz the power, 2017-05-18 00:39:40]

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-18 06:08:22]

Description was changed from

==========
Handling focus change and being stuck at invalid nodes. Need to finish dealing
with loading new pages after clicking link

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

to

==========
Correctly follows focus and moves to valid node when current becomes invalid

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

[commit-bot: I haz the power, 2017-05-18 06:25:57]

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

[commit-bot: I haz the power, 2017-05-18 06:25:58]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-05-18 16:25:53]

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/automation_manager.js
(right):

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:109: //
Rebuild scope stack.
Might be simpler to just do this in one loop rather than
building a list and then popping from it.

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:141: if
(treeChange.type === chrome.automation.TreeChangeType.NODE_REMOVED
Please split this up into a few separate checks, and
add a TODO comment next to the one that's wrong.

I'm thinking:

// TODO: only listen to NODE_REMOVED callbacks, we don't
// need any others
if (treeChange.type !== NODE_REMOVED)
  return;

// TODO: get rid of this, we should be getting NODE_REMOVED
// when a whole tree is deleted
if (treeChange.target.role === 'rootWebArea' && !this.node_.role)

...and so on

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:142: &&
((treeChange.target.role === 'rootWebArea' && !this.node_.role)
Use RoleType.ROOT_WEB_AREA here and elsewhere

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:202:
chrome.accessibilityPrivate.setFocusRing(
Can you call this.setFocusRing_() here?

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:228:
setFocusRing_: function() {
How about updateFocusRing(), since it sets the focus
ring based on the current state, rather than setting it
to some particular argument.

[elichtenberg, 2017-05-18 18:30:44]

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

[elichtenberg, 2017-05-18 18:31:01]

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/switch_access/automation_manager.js
(right):

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:109: //
Rebuild scope stack.
On 2017/05/18 16:25:52, dmazzoni wrote:
> Might be simpler to just do this in one loop rather than
> building a list and then popping from it.

Need separate loops because need the node's scope in order to tell if that node
is a group. So need to start at desktop and walk down the ancestry chain.

Moved this to a helper function though.

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:141: if
(treeChange.type === chrome.automation.TreeChangeType.NODE_REMOVED
On 2017/05/18 16:25:52, dmazzoni wrote:
> Please split this up into a few separate checks, and
> add a TODO comment next to the one that's wrong.
> 
> I'm thinking:
> 
> // TODO: only listen to NODE_REMOVED callbacks, we don't
> // need any others
> if (treeChange.type !== NODE_REMOVED)
>   return;
> 
> // TODO: get rid of this, we should be getting NODE_REMOVED
> // when a whole tree is deleted
> if (treeChange.target.role === 'rootWebArea' && !this.node_.role)
> 
> ...and so on

Split it up and added some TODOs. Do you think this looks clean enough?

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:142: &&
((treeChange.target.role === 'rootWebArea' && !this.node_.role)
On 2017/05/18 16:25:52, dmazzoni wrote:
> Use RoleType.ROOT_WEB_AREA here and elsewhere

Done.

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:202:
chrome.accessibilityPrivate.setFocusRing(
On 2017/05/18 16:25:52, dmazzoni wrote:
> Can you call this.setFocusRing_() here?

Yep. Just changed it.

https://codereview.chromium.org/2872023005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/switch_access/automation_manager.js:228:
setFocusRing_: function() {
On 2017/05/18 16:25:52, dmazzoni wrote:
> How about updateFocusRing(), since it sets the focus
> ring based on the current state, rather than setting it
> to some particular argument.
> 

Done.

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

Dry run: 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-18 20:27:22]

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

[commit-bot: I haz the power, 2017-05-18 20:27:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elichtenberg, 2017-05-18 20:57:14]

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

[commit-bot: I haz the power, 2017-05-18 20:58:29]

Dry run: 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-19 00:22:26]

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

[commit-bot: I haz the power, 2017-05-19 00:22:29]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-05-19 19:38:17]

lgtm

[elichtenberg, 2017-05-19 20:14:38]

The CQ bit was checked by elichtenberg@google.com

[commit-bot: I haz the power, 2017-05-19 20:15:52]

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-19 22:36:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495224878838910,
"parent_rev": "a781fb649239a915431b9ab98a8e6e0f4155f2fa", "commit_rev":
"d00385ad4d2d12bf0aede04cbe08ae4122148ed7"}

[commit-bot: I haz the power, 2017-05-19 22:36:52]

Description was changed from

==========
Correctly follows focus and moves to valid node when current becomes invalid

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

to

==========
Correctly follows focus and moves to valid node when current becomes invalid

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

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

[commit-bot: I haz the power, 2017-05-19 22:36:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d00385ad4d2d12bf0aede04cbe08...