Set keys to traverse accessibility tree. Using focus ring to highlight selected node.

Set keys to traverse accessibility tree. Using focus ring to highlight selected node.

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

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

[elichtenberg, 2017-02-24 21:05:20]

Description was changed from

==========
Set keys to traverse accessibility tree. Using focus ring to highlight selected
node.

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

to

==========
Set keys to traverse accessibility tree. Using focus ring to highlight selected
node.

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

[elichtenberg, 2017-02-24 21:08:19]

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

[elichtenberg, 2017-02-24 21:09:39]

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

[commit-bot: I haz the power, 2017-02-24 21:11:10]

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-02-24 21:36:43]

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

[commit-bot: I haz the power, 2017-02-24 21:36:44]

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

[dmazzoni, 2017-02-24 23:06:54]

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc
(right):

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:36: state_
= ONE_KEY_DOWN;
The state machine made a lot of sense for select-to-speak because it needed to
detect
a combination of Search key plus mouse events, but do nothing otherwise.

I think you can do something simpler here - just "eat" (cancel) all events
involving
the set of keys you care about, and forward them all to the extension. No state
machine required.

At least, I can't think of a reason to have one. There's no reason that Ctrl+1
needs to
do anything different than 1.

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:107: bool
SwitchAccessEventHandler::DispatchKeyEventToSwitchAccess(
Please refactor this into a common utility function like
DispatchKeyEventToExtension() and have it take an extension id
as a parameter.

You can put it in:

chrome/browser/chromeos/accessibility/accessibility_util.h
chrome/browser/chromeos/accessibility/accessibility_util.cc

Then use that from select-to_speak_event_handler.cc, too.

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/switch_access_event_handler.h
(right):

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.h:13: //
Intercepts key events.
Maybe expand on this a little bit?

When Switch Access is enabled, intercepts a few keys before
the rest of the OS has a chance to handle them, as a method
for controlling the entire OS with a small number of switches.

This class just intercepts the keys and forwards them to
the component extension.

[elichtenberg, 2017-02-28 00:28:42]

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/switch_access_event_handler.cc
(right):

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:36: state_
= ONE_KEY_DOWN;
On 2017/02/24 23:06:54, dmazzoni wrote:
> The state machine made a lot of sense for select-to-speak because it needed to
> detect
> a combination of Search key plus mouse events, but do nothing otherwise.
> 
> I think you can do something simpler here - just "eat" (cancel) all events
> involving
> the set of keys you care about, and forward them all to the extension. No
state
> machine required.
> 
> At least, I can't think of a reason to have one. There's no reason that Ctrl+1
> needs to
> do anything different than 1.

Done.

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.cc:107: bool
SwitchAccessEventHandler::DispatchKeyEventToSwitchAccess(
On 2017/02/24 23:06:54, dmazzoni wrote:
> Please refactor this into a common utility function like
> DispatchKeyEventToExtension() and have it take an extension id
> as a parameter.
> 
> You can put it in:
> 
> chrome/browser/chromeos/accessibility/accessibility_util.h
> chrome/browser/chromeos/accessibility/accessibility_util.cc
> 
> Then use that from select-to_speak_event_handler.cc, too.
> 

Done. Did event_handler_common.h and event_handler_common.cc

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/switch_access_event_handler.h
(right):

https://codereview.chromium.org/2711343005/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/switch_access_event_handler.h:13: //
Intercepts key events.
On 2017/02/24 23:06:54, dmazzoni wrote:
> Maybe expand on this a little bit?
> 
> When Switch Access is enabled, intercepts a few keys before
> the rest of the OS has a chance to handle them, as a method
> for controlling the entire OS with a small number of switches.
> 
> This class just intercepts the keys and forwards them to
> the component extension.

Done.

[elichtenberg, 2017-02-28 00:29:33]

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

[commit-bot: I haz the power, 2017-02-28 00:30:11]

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-02-28 00:41:27]

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

[commit-bot: I haz the power, 2017-02-28 00:41:28]

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

[dmazzoni, 2017-02-28 05:38:52]

lgtm

Looks like one closure compiler error and otherwise ready to go.

Nice work!

https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/event_handler_common.cc:54: }
nit: we usually put a comment here on the final brace: // namespace chromeos

[dmazzoni, 2017-02-28 05:39:51]

dmazzoni@chromium.org changed reviewers:
+ dbeam@chromium.org

[dmazzoni, 2017-02-28 05:39:51]

+dbeam for third_party/closure_compiler/compiled_resources2.gyp

[elichtenberg, 2017-02-28 17:49:05]

On 2017/02/28 05:38:52, dmazzoni wrote:
> lgtm
> 
> Looks like one closure compiler error and otherwise ready to go.
> 
> Nice work!
> 
>
https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):
> 
>
https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/accessibility/event_handler_common.cc:54: }
> nit: we usually put a comment here on the final brace: // namespace chromeos

Thanks!

[elichtenberg, 2017-02-28 17:49:34]

https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2711343005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/event_handler_common.cc:54: }
On 2017/02/28 05:38:52, dmazzoni wrote:
> nit: we usually put a comment here on the final brace: // namespace chromeos

Done.

[Dan Beam, 2017-02-28 18:31:25]

lgtm

[elichtenberg, 2017-02-28 18:34:46]

The CQ bit was checked by elichtenberg@google.com

[elichtenberg, 2017-02-28 18:34:47]

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/2711343005/#ps40001
(title: "Added comment for closing namespace brace")

[commit-bot: I haz the power, 2017-02-28 18:35:15]

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-02-28 18:47:15]

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

[commit-bot: I haz the power, 2017-02-28 18:47:17]

Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dmazzoni, 2017-02-28 18:53:00]

I'm seeing this error in closure_compilation:

(ERROR) Error in: switch_access.js
##
/b/build/slave/linux/build/src/chrome/browser/resources/chromeos/switch_access/switch_access.js:122:
ERROR - actual parameter 1 of SwitchAccess.prototype.printNode_ does not match
formal parameter
## found   : (AutomationNode|null)
## required: chrome.automation.AutomationNode
##     this.printNode_(this.node_);
##                     ^^^^^^^^^^

It's basically saying that the parameter is required to be
an AutomationNode, but you passed it an argument that could
possibly be null.

You could fix it by checking if it's null before calling it,
the compiler is smart enough to understand that it won't be
null then.

[elichtenberg, 2017-02-28 19:11:33]

On 2017/02/28 18:53:00, dmazzoni wrote:
> I'm seeing this error in closure_compilation:
> 
> (ERROR) Error in: switch_access.js
> ##
>
/b/build/slave/linux/build/src/chrome/browser/resources/chromeos/switch_access/switch_access.js:122:
> ERROR - actual parameter 1 of SwitchAccess.prototype.printNode_ does not match
> formal parameter
> ## found   : (AutomationNode|null)
> ## required: chrome.automation.AutomationNode
> ##     this.printNode_(this.node_);
> ##                     ^^^^^^^^^^
> 
> It's basically saying that the parameter is required to be
> an AutomationNode, but you passed it an argument that could
> possibly be null.
> 
> You could fix it by checking if it's null before calling it,
> the compiler is smart enough to understand that it won't be
> null then.

Yeah just fixed it. I was assuming it was giving an error because we were
missing an OWNER, so I didn't actually check it until now. Should be good now
though!

[elichtenberg, 2017-02-28 19:12:29]

The CQ bit was checked by elichtenberg@google.com

[elichtenberg, 2017-02-28 19:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2711343005/#ps60001
(title: "Fixing closure compilation error")

[commit-bot: I haz the power, 2017-02-28 19:12:58]

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-02-28 20:48:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488309149520180,
"parent_rev": "81eed96866bf40ae48128b4551e0cad292bb2a7a", "commit_rev":
"c4b4bf46546ce1a66230edc596fa1afd7e05f3b5"}

[commit-bot: I haz the power, 2017-02-28 20:48:58]

Description was changed from

==========
Set keys to traverse accessibility tree. Using focus ring to highlight selected
node.

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

to

==========
Set keys to traverse accessibility tree. Using focus ring to highlight selected
node.

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

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

[commit-bot: I haz the power, 2017-02-28 20:48:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c4b4bf46546ce1a66230edc596fa...