Make ChromeVox Next a setting in options page.

chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
index 5c5b4dfcd0a3c978021d6a122f9aa5eaa68e0f03..ed1199b8698102efd8a5b59fc950515a1bf94fe8 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
@@ -70,13 +70,6 @@ Background = function() {
    */
   this.savedRange_ = null;
 
-  /**
-   * Which variant of ChromeVox is active.
-   * @type {ChromeVoxMode}
-   * @private
-   */
-  this.mode_ = ChromeVoxMode.COMPAT;
-
   // Manually bind all functions to |this|.
   for (var func in this) {
     if (typeof(this[func]) == 'function')
@@ -93,8 +86,8 @@ Background = function() {
   // Next earcons or the Classic earcons depending on the current mode.
   Object.defineProperty(cvox.ChromeVox, 'earcons', {
     get: (function() {
-      if (this.mode_ === ChromeVoxMode.FORCE_NEXT ||
-          this.mode_ === ChromeVoxMode.NEXT) {
+      if (this.mode === ChromeVoxMode.FORCE_NEXT ||
+          this.mode === ChromeVoxMode.NEXT) {
         return this.nextEarcons_;
       } else {
         return this.classicEarcons_;
@@ -105,7 +98,7 @@ Background = function() {
   if (cvox.ChromeVox.isChromeOS) {
     Object.defineProperty(cvox.ChromeVox, 'modKeyStr', {
       get: function() {
-        return (this.mode_ == ChromeVoxMode.CLASSIC || this.mode_ ==
+        return (this.mode == ChromeVoxMode.CLASSIC || this.mode ==
             ChromeVoxMode.COMPAT) ?
                 'Search+Shift' : 'Search';
       }.bind(this)
@@ -139,6 +132,11 @@ Background = function() {
   /** @type {boolean} @private */
   this.inExcursion_ = false;
 
+  /**
+   * Stores the mode as computed the last time a current range was set.
+   */
+  this.mode_ = ChromeVoxMode.COMPAT;

[dmazzoni, 2016/06/21 21:21:02]

Can you use getMode() here?

[David Tseng, 2016/06/21 22:41:32]

No, because we always need to perform the basic setup required of a mode
transition on startup especially if we're in force next or compat mode.

I made this nullable and set it to null.

+
   if (!chrome.accessibilityPrivate.setKeyboardListener)
     chrome.accessibilityPrivate.setKeyboardListener = function() {};
 
@@ -146,6 +144,11 @@ Background = function() {
     chrome.accessibilityPrivate.onAccessibilityGesture.addListener(
         this.onAccessibilityGesture_);
 
+    if (localStorage['useNext'] == 'true')
+      this.forceChromeVoxNextActive();
+    else
+      this.onModeChanged_(ChromeVoxMode.COMPAT, ChromeVoxMode.CLASSIC);

[dmazzoni, 2016/06/21 21:21:02]

It'd be nice if you could just call refreshMode() here and have it compute the
new mode and call onModeChanged automatically based on that - see comment below

[David Tseng, 2016/06/21 22:41:32]

 new mode and call onModeChanged automatically based on that - see comment below

But, that's doing basically the equivalent. You're just hiding the transition
occuring.
this.mode_ initially gets set to ChromeVoxMode.COMPAT, so if you're unlucky,
you'll transition from compat to compat and not add any listeners.

The right way to think about this I think is setting the range which then
determines the mode (transitions if needed). classic -> force next means setting
the range to the focused node. Moving from force next to classic means setting
the current range to null.

+
     Notifications.onStartup();
   }};
 
@@ -196,28 +199,61 @@ Background.prototype = {
    * @override
    */
   getMode: function() {
-    return this.mode_;
+    if (localStorage['useNext'] == 'true')
+      return ChromeVoxMode.FORCE_NEXT;
+
+    var target;
+    if (!this.getCurrentRange()) {
+      chrome.automation.getFocus(function(focus) {
+        target = focus;
+      });
+    } else {
+      target = this.getCurrentRange().start.node;
+    }
+
+    if (!target)
+      return ChromeVoxMode.CLASSIC;
+
+    // Closure complains, but clearly, |target| is not null.
+    var root =
+        AutomationUtil.getTopLevelRoot(/** @type {!AutomationNode} */(target));
+    if (!root)
+        return ChromeVoxMode.COMPAT;

[dmazzoni, 2016/06/21 21:21:02]

nit: wrong indent

[David Tseng, 2016/06/21 22:41:32]

Done.

+    if (this.isWhitelistedForCompat_(root.docUrl))
+      return ChromeVoxMode.COMPAT;
+    else if (this.isWhitelistedForNext_(root.docUrl))
+      return ChromeVoxMode.NEXT;
+    else
+      return ChromeVoxMode.CLASSIC;
   },
 
   /**
-   * @override
+   * Handles a mode change.
+   * @param {ChromeVoxMode} newMode
+   * @param {ChromeVoxMode} oldMode
+   * @private
    */
-  setMode: function(mode, opt_injectClassic) {
+  onModeChanged_: function(newMode, oldMode) {
     // Switching key maps potentially affects the key codes that involve
     // sequencing. Without resetting this list, potentially stale key codes
     // remain. The key codes themselves get pushed in
     // cvox.KeySequence.deserialize which gets called by cvox.KeyMap.
     cvox.ChromeVox.sequenceSwitchKeyCodes = [];
-    if (mode === ChromeVoxMode.CLASSIC || mode === ChromeVoxMode.COMPAT)
-      window['prefs'].switchToKeyMap('keymap_classic');
-    else
-      window['prefs'].switchToKeyMap('keymap_next');
+    if (window['prefs']) {
+      if (newMode === ChromeVoxMode.CLASSIC || newMode === ChromeVoxMode.COMPAT)
+        window['prefs'].switchToKeyMap('keymap_classic');
+      else
+        window['prefs'].switchToKeyMap('keymap_next');
+    }
 
-    if (mode == ChromeVoxMode.CLASSIC) {
+    if (newMode == ChromeVoxMode.CLASSIC) {
       if (chrome.commands &&
           chrome.commands.onCommand.hasListener(this.onGotCommand))
         chrome.commands.onCommand.removeListener(this.onGotCommand);
       chrome.accessibilityPrivate.setKeyboardListener(false, false);
+
+      if (cvox.ChromeVox.isChromeOS)
+        chrome.accessibilityPrivate.setFocusRing([]);
     } else {
       if (chrome.commands &&
           !chrome.commands.onCommand.hasListener(this.onGotCommand))
@@ -231,11 +267,11 @@ Background.prototype = {
     // in time.
     var cur = this.currentRange_;
     chrome.tabs.query({active: true}, function(tabs) {
-      if (mode === ChromeVoxMode.CLASSIC) {
+      if (newMode === ChromeVoxMode.CLASSIC) {
         // Generally, we don't want to inject classic content scripts as it is
         // done by the extension system at document load. The exception is when
         // we toggle classic on manually as part of a user command.
-        if (opt_injectClassic)
+        if (oldMode == ChromeVoxMode.FORCE_NEXT)
           cvox.ChromeVox.injectChromeVoxIntoTabs(tabs);
       } else {
         // When in compat mode, if the focus is within the desktop tree proper,
@@ -249,14 +285,12 @@ Background.prototype = {
 
     // If switching out of a ChromeVox Next mode, make sure we cancel
     // the progress loading sound just in case.
-    if ((this.mode_ === ChromeVoxMode.NEXT ||
-         this.mode_ === ChromeVoxMode.FORCE_NEXT) &&
-        this.mode_ != mode) {
+    if (oldMode === ChromeVoxMode.NEXT ||
+        oldMode === ChromeVoxMode.FORCE_NEXT)
       cvox.ChromeVox.earcons.cancelEarcon(cvox.Earcon.PAGE_START_LOADING);
-    }
 
-    if (mode === ChromeVoxMode.NEXT ||
-        mode === ChromeVoxMode.FORCE_NEXT) {
+    if (newMode === ChromeVoxMode.NEXT ||
+        newMode === ChromeVoxMode.FORCE_NEXT) {
       (new PanelCommand(PanelCommandType.ENABLE_MENUS)).send();
       if (cvox.TabsApiHandler)
         cvox.TabsApiHandler.shouldOutputSpeechAndBraille = false;
@@ -265,61 +299,38 @@ Background.prototype = {
       if (cvox.TabsApiHandler)
         cvox.TabsApiHandler.shouldOutputSpeechAndBraille = true;
     }
-
-    // If switching to Classic from any automation-API-based mode,
-    // clear the focus ring.
-    if (mode === ChromeVoxMode.CLASSIC && mode != this.mode_) {
-      if (cvox.ChromeVox.isChromeOS)
-        chrome.accessibilityPrivate.setFocusRing([]);
-    }
-
-    // If switching away from Classic to any automation-API-based mode,
-    // update the range based on what's focused.
-    if (this.mode_ === ChromeVoxMode.CLASSIC && mode != this.mode_) {
-      chrome.automation.getFocus((function(focus) {
-        if (focus)
-          this.setCurrentRange(cursors.Range.fromNode(focus));
-      }).bind(this));
-    }
-
-    this.mode_ = mode;
   },
 
   /**
-   * Mode refreshes takes into account both |url| and the current ChromeVox
-   *    range. The latter gets used to decide if the user is or isn't in web
-   *    content. The focused state also needs to be set for this info to be
-   *    reliable.
-   * @override
+   * Toggles between force next and classic/compat modes.
+   * This toggle automatically handles deciding between classic/compat based on
+   * the start of the current range.
+   * @param {boolean=} opt_setValue Directly set to force next (true) or
+   *                                classic/compat (false).
+   * @return {boolean} True to announce current position.
    */
-  refreshMode: function(node) {
-    // Mode changes are based upon the top level web root.
-    var root = node.root;
-    while (root &&
-        root.parent &&
-        root.parent.root &&
-        root.parent.root.role != RoleType.desktop) {
-      root = root.parent.root;
-    }
+  toggleNext: function(opt_setValue) {
+    var useNext;
+    if (opt_setValue !== undefined)
+      useNext = opt_setValue;
+    else
+      useNext = localStorage['useNext'] != 'true';
 
-    var url = '';
-    if (root && root.role == RoleType.rootWebArea)
-      url = root.docUrl;
-
-    var mode = this.mode_;
-    if (mode != ChromeVoxMode.FORCE_NEXT) {
-      if (this.isWhitelistedForNext_(url)) {
-        mode = ChromeVoxMode.NEXT;
-      } else if (this.isBlacklistedForClassic_(url) || (this.currentRange_ &&
-          !this.currentRange_.isWebRange() &&
-          this.currentRange_.start.node.state.focused)) {
-        mode = ChromeVoxMode.COMPAT;
-      } else {
-        mode = ChromeVoxMode.CLASSIC;
-      }
-    }
+    localStorage['useNext'] = useNext;
+    if (useNext)
+      this.forceChromeVoxNextActive();
+    else
+      this.setCurrentRange(null);
+
+    var announce = Msgs.getMsg(useNext ?
+        'switch_to_next' : 'switch_to_classic');
+    cvox.ChromeVox.tts.speak(
+        announce, cvox.QueueMode.FLUSH, {doNotInterrupt: true});
+    Notifications.onModeChange();
 
-    this.setMode(mode);
+    // If the new mode is Classic, return false now so we don't announce
+    // anything more.
+    return useNext;
   },
 
   /**
@@ -335,29 +346,22 @@ Background.prototype = {
    * @override
    */
   setCurrentRange: function(newRange) {
-    if (!newRange)
-      return;
-
-    // Is the range invalid?
-    if (newRange.start.node.role === undefined ||
-        newRange.end.node.role === undefined) {
-      // Restore range to the focused location.
-      chrome.automation.getFocus(function(f) {
-        newRange = cursors.Range.fromNode(f);
-      });
-    }
-
-    if (!this.inExcursion_)
+    if (!this.inExcursion_ && newRange)
       this.savedRange_ = new cursors.Range(newRange.start, newRange.end);
 
     this.currentRange_ = newRange;
+    var oldMode = this.mode_;

[dmazzoni, 2016/06/21 21:21:02]

Could these 5 lines be refreshMode()?

Then any call to forceChromeVoxNextActive can be replaced with refreshMode().

[David Tseng, 2016/06/21 22:41:32]

I'd like to limit the number of methods exposed by background especially if
there are no callers.

It's also not clear from this block that force next would go through the right
transitions since this.mode_ might have been set to force next already (e.g. on
startup) since it's getting persisted.

+    var newMode = this.getMode();
+    if (oldMode != newMode) {
+      this.onModeChanged_(newMode, oldMode);
+      this.mode_ = newMode;
+    }
 
     if (this.currentRange_) {
       var start = this.currentRange_.start.node;
       start.makeVisible();
 
       var root = start.root;
-
       if (!root || root.role == RoleType.desktop)
         return;
 
@@ -373,7 +377,7 @@ Background.prototype = {
 
   /** Forces ChromeVox Next to be active for all tabs. */
   forceChromeVoxNextActive: function() {

[dmazzoni, 2016/06/21 21:21:02]

This doesn't actually force it active, it assumes that localStorage and mode_
have already been set.

[David Tseng, 2016/06/21 22:41:32]

Yup; removed and in-lined since there are no external callers anyway.

-    this.setMode(ChromeVoxMode.FORCE_NEXT);
+    this.onModeChanged_(ChromeVoxMode.FORCE_NEXT, ChromeVoxMode.CLASSIC);
   },
 
   /**
@@ -396,7 +400,7 @@ Background.prototype = {
     if (!this.currentRange_)
       return true;
 
-    if (this.mode_ == ChromeVoxMode.CLASSIC && !opt_bypassModeCheck)
+    if (this.mode == ChromeVoxMode.CLASSIC && !opt_bypassModeCheck)
       return true;
 
     var current = this.currentRange_;
@@ -607,26 +611,8 @@ Background.prototype = {
         }
         break;
       case 'toggleChromeVoxVersion':
-        var newMode;
-        if (this.mode_ == ChromeVoxMode.FORCE_NEXT) {
-          var inWeb = current.isWebRange();
-          newMode = inWeb ? ChromeVoxMode.CLASSIC : ChromeVoxMode.COMPAT;
-        } else {
-          newMode = ChromeVoxMode.FORCE_NEXT;
-        }
-        this.setMode(newMode, true);
-
-        var isClassic =
-            newMode == ChromeVoxMode.CLASSIC || newMode == ChromeVoxMode.COMPAT;
-
-        // Leaving unlocalized as 'next' isn't an official name.
-        cvox.ChromeVox.tts.speak(isClassic ?
-            'classic' : 'next', cvox.QueueMode.FLUSH, {doNotInterrupt: true});
-
-        // If the new mode is Classic, return now so we don't announce
-        // anything more.
-        if (newMode == ChromeVoxMode.CLASSIC)
-          return false;
+      if (!this.toggleNext())
+        return false;
         break;
       case 'toggleStickyMode':
         cvox.ChromeVoxBackground.setPref('sticky',
@@ -787,7 +773,7 @@ Background.prototype = {
       case 'reportIssue':
         var url = Background.ISSUE_URL;
         var description = {};
-        description['Mode'] = this.mode_;
+        description['Mode'] = this.mode;
         description['Version'] = chrome.app.getDetails().version;
         description['Reproduction Steps'] = '%0a1.%0a2.%0a3.';
         for (var key in description)
@@ -908,7 +894,7 @@ Background.prototype = {
     if (cvox.ChromeVox.passThroughMode)
       return false;
 
-    if (this.mode_ != ChromeVoxMode.CLASSIC &&
+    if (this.mode != ChromeVoxMode.CLASSIC &&
         !cvox.ChromeVoxKbHandler.basicKeyDownActionsListener(evt)) {
       evt.preventDefault();
       evt.stopPropagation();
@@ -951,7 +937,7 @@ Background.prototype = {
    * @return {boolean} True if evt was processed.
    */
   onBrailleKeyEvent: function(evt, content) {
-    if (this.mode_ === ChromeVoxMode.CLASSIC)
+    if (this.mode === ChromeVoxMode.CLASSIC)
       return false;
 
     switch (evt.command) {
@@ -991,12 +977,24 @@ Background.prototype = {
    * @private
    */
   shouldEnableClassicForUrl_: function(url) {
-    return this.mode_ != ChromeVoxMode.FORCE_NEXT &&
+    return this.mode != ChromeVoxMode.FORCE_NEXT &&
         !this.isBlacklistedForClassic_(url) &&
         !this.isWhitelistedForNext_(url);
   },
 
   /**
+   * Compat mode is on if any of the following are true:
+   * 1. a url is blacklisted for Classic.
+   * 2. the current range is not within web content.
+   * @param {string} url
+   */
+  isWhitelistedForCompat_: function(url) {
+    return this.isBlacklistedForClassic_(url) || (this.getCurrentRange() &&
+        !this.getCurrentRange().isWebRange() &&
+        this.getCurrentRange().start.node.state.focused);
+  },
+
+  /**
    * @param {string} url
    * @return {boolean}
    * @private
@@ -1150,7 +1148,7 @@ Background.prototype = {
     // If we're in classic mode, some gestures need to be handled by the
     // content script. Other gestures are universal and will be handled in
     // this function.
-    if (this.mode_ == ChromeVoxMode.CLASSIC) {
+    if (this.mode == ChromeVoxMode.CLASSIC) {
       if (this.handleClassicGesture_(gesture))
         return;
     }