Add a user selection for number reading style

Also:
- fixes the handling of menuItemSelected to only be respected from a focused node (Chrome now fires it more noisely)
- add a full test suite for options page testing which drives the UI end to end and verifies local ChromeVox state
(this motivated the above fix)
- noticed a regression in the way <select> nodes work (TODO'ed and not handled fixing in this change)
- adds a unit test for verifying number style reading behavior

Test: see above
Change-Id: I4dce435c6de54399128c1a21f805671b54ac3904
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003895
Commit-Queue: David Tseng <dtseng@chromium.org>
Reviewed-by: Akihiro Ota <akihiroota@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741209}

chrome/browser/resources/chromeos/accessibility/chromevox/BUILD.gn

@@ -449,6 +449,7 @@ if (is_chromeos) {
       "background/language_switching_test.js",
       "background/live_regions_test.js",
       "background/logging/log_store_test.js",
+      "background/options/options_test.js",
       "background/output_test.js",
       "background/panel/i_search_test.js",
       "background/panel/panel_test.js",

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

@@ -87,5 +87,42 @@ BaseAutomationHandler = class {
    * @protected
    */
   didHandleEvent_(evt) {}
+
+  /**
+   * Provides all feedback once ChromeVox's focus changes.
+   * @param {!ChromeVoxEvent} evt
+   */
+  onEventDefault(evt) {
+    const node = evt.target;
+    if (!node) {
+      return;
+    }
+
+    // Decide whether to announce and sync this event.
+    const isFocusOnRoot = evt.type == 'focus' && evt.target == evt.target.root;
+    if (!DesktopAutomationHandler.announceActions &&
+        evt.eventFrom == 'action' &&
+        (EventSourceState.get() != EventSourceType.TOUCH_GESTURE ||
+         isFocusOnRoot)) {
+      return;
+    }
+
+    const prevRange = ChromeVoxState.instance.getCurrentRangeWithoutRecovery();
+
+    ChromeVoxState.instance.setCurrentRange(cursors.Range.fromNode(node));
+
+    // Don't output if focused node hasn't changed. Allow focus announcements
+    // when interacting via touch. Touch never sets focus without a double tap.
+    if (prevRange && evt.type == 'focus' &&
+        ChromeVoxState.instance.currentRange.equalsWithoutRecovery(prevRange) &&
+        EventSourceState.get() != EventSourceType.TOUCH_GESTURE) {
+      return;
+    }
+
+    const output = new Output();
+    output.withRichSpeechAndBraille(
+        ChromeVoxState.instance.currentRange, prevRange, evt.type);
+    output.go();
+  }
 };
 });  // goog.scope

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

@@ -70,8 +70,6 @@ DesktopAutomationHandler = class extends BaseAutomationHandler {
 
     this.addListener_(EventType.LOAD_COMPLETE, this.onLoadComplete);
     this.addListener_(EventType.MENU_END, this.onMenuEnd);
-    this.addListener_(
-        EventType.MENU_LIST_ITEM_SELECTED, this.onEventIfSelected);
     this.addListener_(EventType.MENU_START, this.onMenuStart);
     this.addListener_(
         EventType.SCROLL_POSITION_CHANGED, this.onScrollPositionChanged);
@@ -102,43 +100,6 @@ DesktopAutomationHandler = class extends BaseAutomationHandler {
     return false;
   }
 
-  /**
-   * Provides all feedback once ChromeVox's focus changes.
-   * @param {!ChromeVoxEvent} evt
-   */
-  onEventDefault(evt) {
-    const node = evt.target;
-    if (!node) {
-      return;
-    }
-
-    // Decide whether to announce and sync this event.
-    const isFocusOnRoot = evt.type == 'focus' && evt.target == evt.target.root;
-    if (!DesktopAutomationHandler.announceActions &&
-        evt.eventFrom == 'action' &&
-        (EventSourceState.get() != EventSourceType.TOUCH_GESTURE ||
-         isFocusOnRoot)) {
-      return;
-    }
-
-    const prevRange = ChromeVoxState.instance.getCurrentRangeWithoutRecovery();
-
-    ChromeVoxState.instance.setCurrentRange(cursors.Range.fromNode(node));
-
-    // Don't output if focused node hasn't changed. Allow focus announcements
-    // when interacting via touch. Touch never sets focus without a double tap.
-    if (prevRange && evt.type == 'focus' &&
-        ChromeVoxState.instance.currentRange.equalsWithoutRecovery(prevRange) &&
-        EventSourceState.get() != EventSourceType.TOUCH_GESTURE) {
-      return;
-    }
-
-    const output = new Output();
-    output.withRichSpeechAndBraille(
-        ChromeVoxState.instance.currentRange, prevRange, evt.type);
-    output.go();
-  }
-
   /**
    * @param {!ChromeVoxEvent} evt
    */
@@ -148,15 +109,6 @@ DesktopAutomationHandler = class extends BaseAutomationHandler {
     }
   }
 
-  /**
-   * @param {!ChromeVoxEvent} evt
-   */
-  onEventIfSelected(evt) {
-    if (evt.target.selected) {
-      this.onEventDefault(evt);
-    }
-  }
-
   /**
    * Handles the result of a hit test.
    * @param {!AutomationNode} node The hit result.

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

@@ -40,6 +40,8 @@ FocusAutomationHandler = class extends BaseAutomationHandler {
     this.node_ = evt.target;
     this.addListener_(
         EventType.ACTIVEDESCENDANTCHANGED, this.onActiveDescendantChanged);
+    this.addListener_(
+        EventType.MENU_LIST_ITEM_SELECTED, this.onEventIfSelected);
   }
 
   /**
@@ -65,6 +67,15 @@ FocusAutomationHandler = class extends BaseAutomationHandler {
         .go();
     this.previousActiveDescendant_ = evt.target.activeDescendant;
   }
+
+  /**
+   * @param {!ChromeVoxEvent} evt
+   */
+  onEventIfSelected(evt) {
+    if (evt.target.selected) {
+      this.onEventDefault(evt);
+    }
+  }
 };
 
 new FocusAutomationHandler();

chrome/browser/resources/chromeos/accessibility/chromevox/background/options/options.html

@@ -72,6 +72,24 @@
     </select>
   </div>
 
+  <div class="option" id="numberReadingStyleOption">
+    <label id="numberReadingStyleLabel" class="i18n"
+          msgid="options_number_reading_style_select_label">
+      Read numbers as:
+    </label>
+    <select id="numberReadingStyle" class="pref"
+          aria-labelledby="numberReadingStyleLabel">
+      <option id="asWords" class="i18n"
+            msgid="options_number_reading_style_words">
+        Words
+      </option>
+      <option id="asDigits" class="i18n"
+              msgid="options_number_reading_style_digits">
+        Digits
+      </option>
+    </select>
+  </div>
+
   <div class="option">
     <label id="announceDownloadsLabel" class="i18n"
           msgid="options_announce_download">

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

@@ -93,6 +93,14 @@ OptionsPage = class {
       }
     }
 
+    if (localStorage['numberReadingStyle']) {
+      for (let i = 0, opt; opt = $('numberReadingStyle').options[i]; ++i) {
+        if (opt.id == localStorage['numberReadingStyle']) {
+          opt.setAttribute('selected', '');
+        }
+      }
+    }
+
     chrome.commandLinePrivate.hasSwitch(
         'enable-experimental-accessibility-chromevox-language-switching',
         function(enabled) {

chrome/browser/resources/chromeos/accessibility/chromevox/background/options/options_test.js

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Include test fixture.
+GEN_INCLUDE([
+  '../../testing/chromevox_next_e2e_test_base.js',
+  '//chrome/browser/resources/chromeos/accessibility/chromevox/testing/assert_additions.js',
+  '//chrome/browser/resources/chromeos/accessibility/chromevox/testing/mock_feedback.js'
+]);
+
+/**
+ * Test fixture for ChromeVox options page.
+ */
+ChromeVoxOptionsTest = class extends ChromeVoxNextE2ETest {
+  constructor() {
+    super();
+    window.press = this.press;
+  }
+
+  runOnOptionsPage(callback) {
+    const mockFeedback = this.createMockFeedback();
+    chrome.automation.getDesktop((desktop) => {
+      desktop.addEventListener(
+          chrome.automation.EventType.LOAD_COMPLETE, (evt) => {
+            if (evt.target.docUrl.indexOf('background/options/options.html') ==
+                    -1 ||
+                !evt.target.docLoaded) {
+              return;
+            }
+
+            mockFeedback.expectSpeech('ChromeVox Options');
+            callback(mockFeedback, evt);
+          });
+      CommandHandler.onCommand('showOptionsPage');
+    });
+  }
+
+  /**
+   * @return {!MockFeedback}
+   */
+  createMockFeedback() {
+    const mockFeedback =
+        new MockFeedback(this.newCallback(), this.newCallback.bind(this));
+    mockFeedback.install();
+    return mockFeedback;
+  }
+
+  press(keyCode, modifiers) {
+    return function() {
+      BackgroundKeyboardHandler.sendKeyPress(keyCode, modifiers);
+    };
+  }
+};
+
+TEST_F('ChromeVoxOptionsTest', 'NumberReadingStyleSelect', function() {
+  this.runOnOptionsPage((mockFeedback, evt) => {
+    const numberStyleSelect = evt.target.find({
+      role: chrome.automation.RoleType.POP_UP_BUTTON,
+      attributes: {name: 'Read numbers as:'}
+    });
+    assertNotNullNorUndefined(numberStyleSelect);
+    mockFeedback.call(numberStyleSelect.focus.bind(numberStyleSelect))
+        .expectSpeech('Read numbers as:', 'Words', 'Collapsed')
+        .call(numberStyleSelect.doDefault.bind(numberStyleSelect))
+        .expectSpeech('Expanded')
+
+        // Before selecting the menu option.
+        .call(() => {
+          assertEquals('asWords', localStorage['numberReadingStyle']);
+        })
+
+        .call(press(40 /* ArrowDown */))
+        .expectSpeech('Digits', 'Menu item', ' 2 of 2 ')
+        .call(press(13 /* enter */))
+
+        // TODO: The underlying select behavior here is unexpected because we
+        // never get a new focus event for the select (moving us away from the
+        // menu item). We simply repeat the menu item.
+        .expectSpeech('Digits', 'Menu item', ' 2 of 2 ')
+        .call(() => {
+          assertEquals('asDigits', localStorage['numberReadingStyle']);
+        })
+
+        .replay();
+  });
+});

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

@@ -202,9 +202,6 @@ ChromeVoxPrefs.DEFAULT_PREFS = {
   'brailleTableType': 'brailleTable8',
   'brailleTable6': 'en-UEB-g2',
   'brailleTable8': 'en-US-comp8',
-  // TODO(dtseng): Leaking state about multiple key maps here until we have a
-  // class to manage multiple key maps. Also, this doesn't belong as a pref;
-  // should just store in local storage.
   'capitalStrategy': 'increasePitch',
   'currentKeyMap': KeyMap.DEFAULT_KEYMAP,
   'cvoxKey': '',
@@ -216,6 +213,7 @@ ChromeVoxPrefs.DEFAULT_PREFS = {
   'focusFollowsMouse': false,
   'granularity': undefined,
   'languageSwitching': false,
+  'numberReadingStyle': 'asWords',
   'position': '{}',
   'siteSpecificEnhancements': true,
   'siteSpecificScriptBase':

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

@@ -196,9 +196,10 @@ TtsBackground = class extends ChromeTtsBase {
 
     textString = this.preprocess(textString, properties);
 
-    // TODO(dtseng): Google TTS has bad performance when speaking numbers. This
-    // pattern causes ChromeVox to read numbers as digits rather than words.
+    // This pref on localStorage gets set by the options page.
+    if (localStorage['numberReadingStyle'] == 'asDigits') {
       textString = this.getNumberAsDigits_(textString);
+    }
 
     // TODO(dtseng): some TTS engines don't handle strings that don't produce
     // any speech very well. Handle empty and whitespace only strings (including
@@ -613,9 +614,6 @@ TtsBackground = class extends ChromeTtsBase {
    */
   getNumberAsDigits_(text) {
     return text.replace(/\d+/g, function(num) {
-      if (num.length <= 4) {
-        return num;
-      }
       return num.split('').join(' ');
     });
   }

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

@@ -155,3 +155,29 @@ SYNC_TEST_F('ChromeVoxTtsBackgroundTest', 'AnnounceCapitalLetters', function() {
   assertEquals('BB', preprocess('BB'));
   assertEquals('A.', preprocess('A.'));
 });
+
+SYNC_TEST_F('ChromeVoxTtsBackgroundTest', 'NumberReadingStyle', function() {
+  const tts = new TtsBackground();
+  let lastSpokenTextString = '';
+  tts.speakUsingQueue_ = function(utterance, _) {
+    lastSpokenTextString = utterance.textString;
+  };
+
+  // Check the default preference.
+  assertEquals('asWords', localStorage['numberReadingStyle']);
+
+  tts.speak('100');
+  assertEquals('100', lastSpokenTextString);
+
+  tts.speak('An unanswered call lasts for 30 seconds.');
+  assertEquals(
+      'An unanswered call lasts for 30 seconds.', lastSpokenTextString);
+
+  localStorage['numberReadingStyle'] = 'asDigits';
+  tts.speak('100');
+  assertEquals('1 0 0', lastSpokenTextString);
+
+  tts.speak('An unanswered call lasts for 30 seconds.');
+  assertEquals(
+      'An unanswered call lasts for 3 0 seconds.', lastSpokenTextString);
+});

chrome/browser/resources/chromeos/accessibility/chromevox/strings/chromevox_strings.grd

@@ -3825,6 +3825,15 @@ If you're done with the tutorial, use ChromeVox to navigate to the Close button
       <message desc="A label for the button in the ChromeVox panel used to save annotations." name="IDS_CHROMEVOX_SAVE_ANNOTATION">
         Save label
       </message>
+      <message desc="Labels the select for choosing how ChromeVox reads numbers." name="IDS_CHROMEVOX_OPTIONS_NUMBER_READING_STYLE_SELECT_LABEL">
+        Read numbers as:
+      </message>
+      <message desc="Describes an option for ChromeVox to read numbers as words." name="IDS_CHROMEVOX_OPTIONS_NUMBER_READING_STYLE_WORDS">
+        Words
+      </message>
+      <message desc="Describes an option for ChromeVox to read numbers as digits." name="IDS_CHROMEVOX_OPTIONS_NUMBER_READING_STYLE_DIGITS">
+        Digits
+      </message>
     </messages>
   </release>
 </grit>