UserActionMonitor can stop propagation of an action, even when matched.

This change adds the ability to stop the propagation of an action, even
if it's matched by UserActionMonitor.

The motivating example for this change is
the interactive tutorial lesson that requests users to press Control.
After successfully pressing Control, the tutorial automatically moves
the user to the next lesson and reads the text. However, there are
cases where the text is never read because ChromeVox cancels speech
when Control is pressed.

The solution is to add a boolean parameter that can specify whether
or not the action should propagate, even if matched.
This ensures that speech for the next lesson is not cancelled
after completing the interactive Control key lesson.

Fixed: 1132944
AX-Relnotes: N/A
Change-Id: I6e63f7612b6fdf9aa398d46ff7746d5273aee683
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2469436Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Akihiro Ota <akihiroota@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817264}

chrome/browser/resources/chromeos/accessibility/chromevox/background/user_action_monitor.js

@@ -55,15 +55,16 @@ UserActionMonitor = class {
       this.actions_.push(
           UserActionMonitor.Action.fromActionInfo(actionInfos[i]));
     }
-    if (this.actions_[0].opt_beforeActionCallback) {
-      this.actions_[0].opt_beforeActionCallback();
+    if (this.actions_[0].beforeActionCallback) {
+      this.actions_[0].beforeActionCallback();
     }
   }
 
   // Public methods.
 
   /**
-   * Returns true if the key sequence was matched. Returns false otherwise.
+   * Returns true if the key sequence should be allowed to propagate to other
+   * handlers. Returns false otherwise.
    * @param {!KeySequence} actualSequence
    * @return {boolean}
    */
@@ -85,7 +86,7 @@ UserActionMonitor = class {
     }
 
     this.expectedActionMatched_();
-    return true;
+    return expectedAction.shouldPropagate;
   }
 
   // Private methods.
@@ -93,8 +94,8 @@ UserActionMonitor = class {
   /** @private */
   expectedActionMatched_() {
     const action = this.getExpectedAction_();
-    if (action.opt_afterActionCallback) {
-      action.opt_afterActionCallback();
+    if (action.afterActionCallback) {
+      action.afterActionCallback();
     }
 
     this.nextAction_();
@@ -114,8 +115,8 @@ UserActionMonitor = class {
     }
 
     const action = this.getExpectedAction_();
-    if (action.opt_beforeActionCallback) {
-      action.opt_beforeActionCallback();
+    if (action.beforeActionCallback) {
+      action.beforeActionCallback();
     }
   }
 
@@ -155,8 +156,9 @@ UserActionMonitor.CLOSE_CHROMEVOX_KEY_SEQUENCE_ = KeySequence.deserialize(
  * @typedef {{
  *    type: ActionType,
  *    value: (string|Object),
+ *    shouldPropagate: (boolean|undefined),
  *    beforeActionMsg: (string|undefined),
- *    afterActionMsg: (string|undefined)
+ *    afterActionMsg: (string|undefined),
  * }}
  */
 UserActionMonitor.ActionInfo;
@@ -164,38 +166,49 @@ UserActionMonitor.ActionInfo;
 // Represents an expected action.
 UserActionMonitor.Action = class {
   /**
-   * @param {ActionType} type
-   * @param {string|!KeySequence} value
-   * @param {(function():void)=} opt_beforeActionCallback Runs once before this
-   *     action is seen.
-   * @param {(function():void)=} opt_afterActionCallback Runs once after this
-   *     action is seen.
+   * Please see below for more information on arguments:
+   * type: The type of action.
+   * value: The action value.
+   * shouldPropagate: Whether or not this action should propagate to other
+   *  handlers e.g. CommandHandler.
+   * beforeActionCallback: A callback that runs once before this action is seen.
+   * afterActionCallback: A callback that runs once after this action is seen.
+   * @param {!{
+   *  type: ActionType,
+   *  value: (string|!KeySequence),
+   *  shouldPropagate: (boolean|undefined),
+   *  beforeActionCallback: (function(): void|undefined),
+   *  afterActionCallback: (function(): void|undefined)
+   * }} params
    */
-  constructor(type, value, opt_beforeActionCallback, opt_afterActionCallback) {
-    switch (type) {
+  constructor(params) {
+    /** @type {ActionType} */
+    this.type = params.type;
+    /** @type {string|!KeySequence} */
+    this.value = params.value;
+    /** @type {boolean} */
+    this.shouldPropagate =
+        (params.shouldPropagate !== undefined) ? params.shouldPropagate : true;
+    /** @type {(function():void)|undefined} */
+    this.beforeActionCallback = params.beforeActionCallback;
+    /** @type {(function():void)|undefined} */
+    this.afterActionCallback = params.afterActionCallback;
+
+    switch (this.type) {
       case ActionType.KEY_SEQUENCE:
-        if (!(value instanceof KeySequence)) {
+        if (!(this.value instanceof KeySequence)) {
           throw new Error(
               'UserActionMonitor: Must provide a KeySequence value for ' +
               'Actions of type ActionType.KEY_SEQUENCE');
         }
         break;
       default:
-        if (typeof value !== 'string') {
+        if (typeof this.value !== 'string') {
           throw new Error(
               'UserActionMonitor: Must provide a string value for Actions ' +
               'if type is other than ActionType.KEY_SEQUENCE');
         }
     }
-
-    /** @type {ActionType} */
-    this.type = type;
-    /** @type {string|!KeySequence} */
-    this.value = value;
-    /** @type {(function():void)|undefined} */
-    this.opt_beforeActionCallback = opt_beforeActionCallback;
-    /** @type {(function():void)|undefined} */
-    this.opt_afterActionCallback = opt_afterActionCallback;
   }
 
   /**
@@ -225,6 +238,7 @@ UserActionMonitor.Action = class {
     const value = (typeof info.value === 'object') ?
         KeySequence.deserialize(info.value) :
         info.value;
+    const shouldPropagate = info.shouldPropagate;
     const beforeActionMsg = info.beforeActionMsg;
     const afterActionMsg = info.afterActionMsg;
 
@@ -244,8 +258,13 @@ UserActionMonitor.Action = class {
       UserActionMonitor.Action.output_(afterActionMsg);
     };
 
-    return new UserActionMonitor.Action(
-        type, value, beforeActionCallback, afterActionCallback);
+    return new UserActionMonitor.Action({
+      type,
+      value,
+      shouldPropagate,
+      beforeActionCallback,
+      afterActionCallback
+    });
   }
 
   /**

chrome/browser/resources/chromeos/accessibility/chromevox/background/user_action_monitor_test.js

@@ -68,13 +68,14 @@ TEST_F('ChromeVoxUserActionMonitorTest', 'ActionUnitTest', function() {
   this.runWithLoadedTree(this.simpleDoc, function() {
     const keySequenceActionOne = UserActionMonitor.Action.fromActionInfo(
         {type: 'key_sequence', value: {keys: {keyCode: [KeyCode.SPACE]}}});
-    const keySequenceActionTwo = new UserActionMonitor.Action(
-        'key_sequence',
-        new KeySequence(TestUtils.createMockKeyEvent(KeyCode.A)));
+    const keySequenceActionTwo = new UserActionMonitor.Action({
+      type: 'key_sequence',
+      value: new KeySequence(TestUtils.createMockKeyEvent(KeyCode.A))
+    });
     const gestureActionOne = UserActionMonitor.Action.fromActionInfo(
         {type: 'gesture', value: 'swipeUp1'});
     const gestureActionTwo =
-        new UserActionMonitor.Action('gesture', 'swipeUp2');
+        new UserActionMonitor.Action({type: 'gesture', value: 'swipeUp2'});
 
     assertFalse(keySequenceActionOne.equals(keySequenceActionTwo));
     assertFalse(keySequenceActionOne.equals(gestureActionOne));
@@ -86,7 +87,7 @@ TEST_F('ChromeVoxUserActionMonitorTest', 'ActionUnitTest', function() {
     const cloneKeySequenceActionOne = UserActionMonitor.Action.fromActionInfo(
         {type: 'key_sequence', value: {keys: {keyCode: [KeyCode.SPACE]}}});
     const cloneGestureActionOne =
-        new UserActionMonitor.Action('gesture', 'swipeUp1');
+        new UserActionMonitor.Action({type: 'gesture', value: 'swipeUp1'});
     assertTrue(keySequenceActionOne.equals(cloneKeySequenceActionOne));
     assertTrue(gestureActionOne.equals(cloneGestureActionOne));
   });
@@ -119,7 +120,7 @@ TEST_F('ChromeVoxUserActionMonitorTest', 'Errors', function() {
     }
     assertCaughtAndReset();
     try {
-      new UserActionMonitor.Action('key_sequence', 'invalid');
+      new UserActionMonitor.Action({type: 'key_sequence', value: 'invalid'});
       assertTrue(false);  // Shouldn't execute
     } catch (error) {
       assertEquals(
@@ -417,3 +418,30 @@ TEST_F('ChromeVoxUserActionMonitorTest', 'CloseChromeVox', function() {
     assertFalse(finished);
   });
 });
+
+// Tests that we can stop propagation of an action, even if it is matched.
+// In this test, we stop propagation of the Control key to avoid executing the
+// stopSpeech command.
+TEST_F('ChromeVoxUserActionMonitorTest', 'StopPropagation', function() {
+  this.runWithLoadedTree(this.simpleDoc, function() {
+    const keyboardHandler = ChromeVoxState.instance.keyboardHandler_;
+    let finished = false;
+    let executedCommand = false;
+    const actions = [{
+      type: 'key_sequence',
+      value: {keys: {keyCode: [KeyCode.CONTROL]}},
+      shouldPropagate: false
+    }];
+    const onFinished = () => finished = true;
+    ChromeVoxState.instance.createUserActionMonitor(actions, onFinished);
+    ChromeVoxKbHandler.commandHandler = function(command) {
+      executedCommand = true;
+    };
+    assertFalse(finished);
+    assertFalse(executedCommand);
+    keyboardHandler.onKeyDown(TestUtils.createMockKeyEvent(KeyCode.CONTROL));
+    keyboardHandler.onKeyUp(TestUtils.createMockKeyEvent(KeyCode.CONTROL));
+    assertFalse(executedCommand);
+    assertTrue(finished);
+  });
+});

chrome/browser/resources/chromeos/accessibility/chromevox/common/keyboard_handler.js

@@ -89,9 +89,8 @@ ChromeVoxKbHandler.basicKeyDownActionsListener = function(evt) {
   const chromeVoxState = ChromeVoxState.instance;
   const monitor = chromeVoxState ? chromeVoxState.getUserActionMonitor() : null;
   if (monitor && !monitor.onKeySequence(keySequence)) {
-    // UserActionMonitor returns true if this key sequence was matched. If a
-    // key sequence is matched by the UserActionMonitor, allow it to process.
-    // Otherwise, prevent the default action.
+    // UserActionMonitor returns true if this key sequence should propagate.
+    // Prevent the default action if it returns false.
     return false;
   }
 

chrome/browser/resources/chromeos/accessibility/chromevox/i_tutorial/components/i_tutorial.js

@@ -170,9 +170,11 @@ Polymer({
             key.`],
           medium: InteractionMedium.KEYBOARD,
           curriculums: [Curriculum.QUICK_ORIENTATION],
-          actions: [
-            {type: 'key_sequence', value: {keys: {keyCode: [17 /* Ctrl */]}}}
-          ],
+          actions: [{
+            type: 'key_sequence',
+            value: {keys: {keyCode: [17 /* Ctrl */]}},
+            shouldPropagate: false
+          }],
           autoInteractive: true,
         },
 

chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js

@@ -537,6 +537,13 @@ TEST_F('ChromeVoxTutorialTest', 'QuickOrientationLessonTest', function() {
           assertNotEquals(firstLessonNode, getRangeStart());
           assertEquals(1, tutorial.activeLessonNum);
         })
+        // Pressing control, which is the desired key sequence, should move us
+        // to the next lesson.
+        .call(simulateKeyPress.bind(this, KeyCode.CONTROL, {}))
+        .expectSpeech('Essential Keys: Shift')
+        .call(() => {
+          assertEquals(2, tutorial.activeLessonNum);
+        })
         .replay();
   });
 });