Make touch accessibility gestures work with ChromeVox Next

Make touch accessibility gestures work with ChromeVox Next

When spoken feedback is enabled, Chrome OS enables a touch exploration
controller that lets the user explore items on the screen without accidentally
clicking on them. It also captures swipe gestures, and previously it simply
rewrote the swipe gestures into key events to control ChromeVox, but that was
a hacky solution, and didn't work with ChromeVox Next (which uses different
key events).

Fix this by defining an enum for all of the possible accessibility gestures
(allowing for future non-swipe gestures), pass them through to ChromeVox via
a accessibilityPrivate API, and then map them directly to ChromeVox commands.

Note that this change temporarily loses a few of the gesture bindings that
worked previously, but those weren't meant to be set in stone and we will
probably rethink many of them.

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

Committed: https://crrev.com/82ef29a0d6470bf6d83f00bba15ca4d4c8835930
Cr-Commit-Position: refs/heads/master@{#392772}

[dmazzoni, 2016-05-04 23:06:13]

Description was changed from

==========
Make touch accessibility gestures work with ChromeVox Next

When spoken feedback is enabled, Chrome OS enables a touch exploration
controller that lets the user explore items on the screen without accidentally
clicking on them. It also captures swipe gestures, and previously it simply
rewrote the swipe gestures into key events to control ChromeVox, but that was
a hacky solution, and didn't work with ChromeVox Next (which uses different
key events).

Fix this by defining an enum for all of the possible accessibility gestures
(allowing for future non-swipe gestures), pass them through to ChromeVox via
a accessibilityPrivate API, and then map them directly to ChromeVox commands.

Note that this change temporarily loses a few of the gesture bindings that
worked previously, but those weren't meant to be set in stone and we will
probably rethink many of them.

BUG=513713
==========

to

==========
Make touch accessibility gestures work with ChromeVox Next

When spoken feedback is enabled, Chrome OS enables a touch exploration
controller that lets the user explore items on the screen without accidentally
clicking on them. It also captures swipe gestures, and previously it simply
rewrote the swipe gestures into key events to control ChromeVox, but that was
a hacky solution, and didn't work with ChromeVox Next (which uses different
key events).

Fix this by defining an enum for all of the possible accessibility gestures
(allowing for future non-swipe gestures), pass them through to ChromeVox via
a accessibilityPrivate API, and then map them directly to ChromeVox commands.

Note that this change temporarily loses a few of the gesture bindings that
worked previously, but those weren't meant to be set in stone and we will
probably rethink many of them.

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

[dmazzoni, 2016-05-04 23:19:53]

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

[dmazzoni, 2016-05-04 23:19:54]

David, please do an initial review of everything here, then I'll need to get
several owners to approve.

I will tackle the double-tap to click issue in a follow-up patch, already in
progress.

[David Tseng, 2016-05-05 22:30:39]

Great to see this change. We should continue to talk about double tap.

https://codereview.chromium.org/1953613002/diff/1/ash/accessibility_delegate.h
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1953613002/diff/1/ash/accessibility_delegate....
ash/accessibility_delegate.h:130: // to ChromeVox.
nit: 'to' fits above.

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

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/externs.js:74:
chrome.accessibilityPrivate.onChromeVoxLoadStateChanged.destroy_;
Since you're here, this isn't used and can be removed.

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

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1008:
console.error('Gesture: ' + gesture);
nit: remove

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1012:
return;
Are you going to allow for unhandled gestures?

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1013:
}
Is this meant to fall through?

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1022:
case 'swipeDown2': command = 'jumpToBottom'; break;
Maybe just put these into an object dictionary?

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1022:
case 'swipeDown2': command = 'jumpToBottom'; break;
This should be readFromHere

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1023:
}
default?

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1025:
console.error('Gesture command: ' + command);
nit: remove

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1047:
case 'swipeDown2': command = 'jumpToBottom'; break;
Ditto; readFromHere.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1053:

nit: extra line

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:121: /**
nit: remove

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:142: **/
nit: remove?

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:797: LOG(ERROR) <<
"ProcessGestureEvents";
nit: remove

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.h:62: // touch accessibility mode, that
should be forwarded to ChromeVox.
nit: room for touch

[dmazzoni, 2016-05-06 19:06:07]

dmazzoni@chromium.org changed reviewers:
+ asargent@chromium.org, asvitkine@chromium.org, oshima@chromium.org

[dmazzoni, 2016-05-06 19:06:09]

Ready for owners review:

oshima: ash/, ui/chromeos/
asargent: chrome/common/extensions/api/
asvitkine: *histograms*

https://codereview.chromium.org/1953613002/diff/1/ash/accessibility_delegate.h
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1953613002/diff/1/ash/accessibility_delegate....
ash/accessibility_delegate.h:130: // to ChromeVox.
On 2016/05/05 22:30:38, David Tseng wrote:
> nit: 'to' fits above.

Done.

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

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/externs.js:74:
chrome.accessibilityPrivate.onChromeVoxLoadStateChanged.destroy_;
On 2016/05/05 22:30:38, David Tseng wrote:
> Since you're here, this isn't used and can be removed.

Done.

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

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1008:
console.error('Gesture: ' + gesture);
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: remove

Done.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1012:
return;
On 2016/05/05 22:30:39, David Tseng wrote:
> Are you going to allow for unhandled gestures?

Yes, it should return true if it shouldn't continue processing just like an
event.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1013:
}
On 2016/05/05 22:30:39, David Tseng wrote:
> Is this meant to fall through?

Yes, my thought was that some gestures might be universal for both classic and
next, like if there was a gesture to go home or speak the date and time. I added
a comment.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1022:
case 'swipeDown2': command = 'jumpToBottom'; break;
On 2016/05/05 22:30:39, David Tseng wrote:
> This should be readFromHere

Done.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1022:
case 'swipeDown2': command = 'jumpToBottom'; break;
On 2016/05/05 22:30:39, David Tseng wrote:
> Maybe just put these into an object dictionary?

Done.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1023:
}
On 2016/05/05 22:30:39, David Tseng wrote:
> default?

What do you want the default handler to do? I think it's okay if not every
possible gesture maps to something, especially as we increase our gesture
vocabulary

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1025:
console.error('Gesture command: ' + command);
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: remove

Done.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1047:
case 'swipeDown2': command = 'jumpToBottom'; break;
On 2016/05/05 22:30:39, David Tseng wrote:
> Ditto; readFromHere.

Done.

https://codereview.chromium.org/1953613002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:1053:

On 2016/05/05 22:30:39, David Tseng wrote:
> nit: extra line

Done.

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:121: /**
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: remove

Done.

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:142: **/
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: remove?

Done.

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.cc:797: LOG(ERROR) <<
"ProcessGestureEvents";
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: remove

Done.

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/1953613002/diff/1/ui/chromeos/touch_explorati...
ui/chromeos/touch_exploration_controller.h:62: // touch accessibility mode, that
should be forwarded to ChromeVox.
On 2016/05/05 22:30:39, David Tseng wrote:
> nit: room for touch

Done.

[Alexei Svitkine (slow), 2016-05-06 19:15:35]

histograms lgtm, didn't look at anything else

[oshima, 2016-05-06 19:43:50]

lgtm with nits

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:888: default:
qq: this (5 fingers or more) can happen?

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:92: ui::AXGesture
GetLastGesture() { return last_gesture_; }
const

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:1322: int delta_time_ms =
floor(delta_time * 1000);
#include <math>

[David Tseng, 2016-05-09 17:35:20]

lgtm

[dmazzoni, 2016-05-09 18:07:08]

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:888: default:
On 2016/05/06 19:43:50, oshima wrote:
> qq: this (5 fingers or more) can happen?
> 

Yes, it can, we just don't intend to map 5 finger gestures at this time - too
hard to do.

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:92: ui::AXGesture
GetLastGesture() { return last_gesture_; }
On 2016/05/06 19:43:50, oshima wrote:
> const

Make all accessors const.

https://codereview.chromium.org/1953613002/diff/20001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:1322: int delta_time_ms =
floor(delta_time * 1000);
On 2016/05/06 19:43:50, oshima wrote:
> #include <math>

Done.

[asargent_no_longer_on_chrome, 2016-05-09 19:04:55]

lgtm

https://codereview.chromium.org/1953613002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/accessibility_private.json (right):

https://codereview.chromium.org/1953613002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/accessibility_private.json:93: "type": "string"
optional suggestion: since the values of this come from an enum in C++, you can
add a "enum" key here mapped to a list of the possible strings, and then you'll
get validity checking from the bindings layer for free (also documentation
nicety, although I guess that doesn't matter for a private API since we don't
generate docs for it).

[dmazzoni, 2016-05-09 20:03:33]

https://codereview.chromium.org/1953613002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/accessibility_private.json (right):

https://codereview.chromium.org/1953613002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/accessibility_private.json:93: "type": "string"
On 2016/05/09 19:04:54, Antony Sargent wrote:
> optional suggestion: since the values of this come from an enum in C++, you
can
> add a "enum" key here mapped to a list of the possible strings, and then
you'll
> get validity checking from the bindings layer for free (also documentation
> nicety, although I guess that doesn't matter for a private API since we don't
> generate docs for it). 

Done.

[dmazzoni, 2016-05-09 20:04:03]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-09 20:04:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dtseng@chromium.org, oshima@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1953613002/#ps80001
(title: "Add enum to api")

[commit-bot: I haz the power, 2016-05-09 20:04:38]

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

[commit-bot: I haz the power, 2016-05-09 20:13:03]

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

[commit-bot: I haz the power, 2016-05-09 20:13:05]

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

[dmazzoni, 2016-05-10 19:59:21]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-10 19:59:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dtseng@chromium.org, oshima@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1953613002/#ps100001
(title: "Revert unrelated change")

[commit-bot: I haz the power, 2016-05-10 20:00:00]

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

[commit-bot: I haz the power, 2016-05-10 20:03:12]

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

[commit-bot: I haz the power, 2016-05-10 20:03:14]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmazzoni, 2016-05-10 22:02:17]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-10 22:02:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dtseng@chromium.org, oshima@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1953613002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-10 22:02:47]

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

[commit-bot: I haz the power, 2016-05-10 23:38:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-10 23:39:45]

Description was changed from

==========
Make touch accessibility gestures work with ChromeVox Next

When spoken feedback is enabled, Chrome OS enables a touch exploration
controller that lets the user explore items on the screen without accidentally
clicking on them. It also captures swipe gestures, and previously it simply
rewrote the swipe gestures into key events to control ChromeVox, but that was
a hacky solution, and didn't work with ChromeVox Next (which uses different
key events).

Fix this by defining an enum for all of the possible accessibility gestures
(allowing for future non-swipe gestures), pass them through to ChromeVox via
a accessibilityPrivate API, and then map them directly to ChromeVox commands.

Note that this change temporarily loses a few of the gesture bindings that
worked previously, but those weren't meant to be set in stone and we will
probably rethink many of them.

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

to

==========
Make touch accessibility gestures work with ChromeVox Next

When spoken feedback is enabled, Chrome OS enables a touch exploration
controller that lets the user explore items on the screen without accidentally
clicking on them. It also captures swipe gestures, and previously it simply
rewrote the swipe gestures into key events to control ChromeVox, but that was
a hacky solution, and didn't work with ChromeVox Next (which uses different
key events).

Fix this by defining an enum for all of the possible accessibility gestures
(allowing for future non-swipe gestures), pass them through to ChromeVox via
a accessibilityPrivate API, and then map them directly to ChromeVox commands.

Note that this change temporarily loses a few of the gesture bindings that
worked previously, but those weren't meant to be set in stone and we will
probably rethink many of them.

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

Committed: https://crrev.com/82ef29a0d6470bf6d83f00bba15ca4d4c8835930
Cr-Commit-Position: refs/heads/master@{#392772}
==========

[commit-bot: I haz the power, 2016-05-10 23:39:47]

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

[dcheng, 2016-05-11 01:24:57]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-05-11 01:24:59]

https://codereview.chromium.org/1953613002/diff/120001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1953613002/diff/120001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:493: swipe_down_4,
FWIW, something is complaining about trailing commas:
[100/10377] ACTION
//ui/accessibility:ax_gen_schema_generator(//build/toolchain/linux:clang_x64)
../../ui/accessibility/ax_enums.idl(494) : Error : Trailing comma in block.

Though it seems like the error might be advisory? My build hasn't failed yet...