Make cvox2 output feedback more robust when focusing a text field.

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

 // Copyright 2014 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.
 
 /**
  * @fileoverview The entry point for all ChromeVox2 related code for the
  * background page.
  */
 
 goog.provide('Background');
@@ skipping 47 matching lines @@
   /**
    * Which variant of ChromeVox is active.
    * @type {ChromeVoxMode}
    * @private
    */
   this.mode_ = ChromeVoxMode.CLASSIC;
 
   /** @type {!ClassicCompatibility} @private */
   this.compat_ = new ClassicCompatibility();
 
+  /** @type {Array<Object>} A queue of incoming events. @private */
+  this.eventQueue_ = [];

[Peter Lundblad, 2015/08/20 08:30:05]

I suggest putting the event queue into its own class outside of background
that can be unit tested separately.

+
+  /**
+   * @type {?number}
+   * The window.setTimeout timer ID of the event queue timer.
+   * @private
+   */
+  this.eventQueueTimerId_ = null;

[Peter Lundblad, 2015/08/20 08:30:05]

Since your code already relies on timer ids being non-zero, perhaps slightly
simplify and use 0 for no id?

+
+  /**
+   * @type {number}
+   * How long to hold events in the event queue before executing them.
+   * @const
+   */
+  this.EVENT_QUEUE_DELAY_MS = 10;

[David Tseng, 2015/08/19 17:19:25]

nit: make private.

[Peter Lundblad, 2015/08/20 08:30:05]

Put constants outside of the constructor, directly on the object.

+
   // Manually bind all functions to |this|.
   for (var func in this) {
     if (typeof(this[func]) == 'function')
       this[func] = this[func].bind(this);
   }
 
   /**
    * Maps an automation event to its listener.
    * @type {!Object<EventType, function(Object) : void>}
    */
   this.listeners_ = {
     alert: this.onEventDefault,
     focus: this.onFocus,
     hover: this.onEventDefault,
     loadComplete: this.onLoadComplete,
     menuStart: this.onEventDefault,
     menuEnd: this.onEventDefault,
     menuListValueChanged: this.onEventDefault,
     textChanged: this.onTextOrTextSelectionChanged,
     textSelectionChanged: this.onTextOrTextSelectionChanged,
     valueChanged: this.onValueChanged
   };
 
+  /**
+   * The object that speaks changes to an editable text field.
+   * @type {?cvox.ChromeVoxEditableTextBase}

[Peter Lundblad, 2015/08/20 08:30:05]

Document when this is null.

+   */
+  this.editableTextHandler_ = null;
+
   chrome.automation.getDesktop(this.onGotDesktop);
 
   // Handle messages directed to the Next background page.
   cvox.ExtensionBridge.addMessageListener(function(msg, port) {
     var target = msg['target'];
     var action = msg['action'];
 
     switch (target) {
       case 'next':
         if (action == 'getIsClassicEnabled') {
@@ skipping 15 matching lines @@
     this.setChromeVoxMode(ChromeVoxMode.FORCE_NEXT);
   },
 
   /**
    * Handles all setup once a new automation tree appears.
    * @param {chrome.automation.AutomationNode} desktop
    */
   onGotDesktop: function(desktop) {
     // Register all automation event listeners.
     for (var eventType in this.listeners_)
-      desktop.addEventListener(eventType, this.listeners_[eventType], true);
+      desktop.addEventListener(eventType, this.addToEventQueue_, true);
 
     // Register a tree change observer.
     chrome.automation.addTreeChangeObserver(this.onTreeChange);
 
     // The focused state gets set on the containing webView node.
     var webView = desktop.find({role: chrome.automation.RoleType.webView,
                                 state: {focused: true}});
     if (webView) {
       var root = webView.find({role: chrome.automation.RoleType.rootWebArea});
       if (root) {
@@ skipping 257 matching lines @@
       case cvox.BrailleKeyCommand.BOTTOM:
         this.onGotCommand('goToEnd', true);
         break;
       default:
         return false;
     }
     return true;
   },
 
   /**
+   * Add an event to the event queue so that we can avoid ignore

[David Tseng, 2015/08/19 17:19:25]

avoid or ignore

+   * transient states - for example if focus moves to one node and
+   * then immediately another, or if a node gains focus and
+   * immediately changes its selection.
+   *
+   * @param {Object} evt
+   */

[David Tseng, 2015/08/19 17:19:25]

nit: @private

+  addToEventQueue_: function(evt) {
+    evt.enqueueTime = new Date();

[Peter Lundblad, 2015/08/20 08:30:05]

Should we extend the automation api with a time field?  The closer to the
source, the closer to reality the time will be.  In any case, I suggest wrapping
the event and time in a new object instead of extending the AutomationEvent,
allowing the type of the queue elements to be correct.

+    this.eventQueue_.push(evt);
+    if (!this.eventQueueTimerId_) {
+      this.eventQueueTimerId_ = window.setTimeout(
+          this.executeNextEventFromQueue_, this.EVENT_QUEUE_DELAY_MS);
+    }
+  },
+
+  /**
+   * Filter duplicates from the event queue, then pop the first item
+   * off and if it's time to execute it, do so, otherwise set the timer
+   * to wake up again.

[David Tseng, 2015/08/19 17:19:25]

nit: @private

+   */
+  executeNextEventFromQueue_: function() {

[Peter Lundblad, 2015/08/20 08:30:05]

nit: s/execute/handle/ ?

+    this.eventQueueTimerId_ = null;
+
+    if (this.eventQueue_.length == 0) {
+      return;
+    }

[David Tseng, 2015/08/19 17:19:25]

nit: remove braces.

+
+    // Assuming the queue is in increasing order of enqueue time, remove all except the
+    // last item of each type.
+    var lastTimeForEventType = {};

[Peter Lundblad, 2015/08/20 08:30:05]

Why not just store the object reference instead of the the time in this
object. I think it has the same result, but is easier to understand.

+    this.eventQueue_.forEach(function(evt) {
+      lastTimeForEventType[evt.type] = evt.enqueueTime;
+    });
+    this.eventQueue_ = this.eventQueue_.filter(function(evt) {
+      return evt.enqueueTime == lastTimeForEventType[evt.type];

[David Tseng, 2015/08/19 17:19:25]

Any issues doing this generically? For example, doesn't editable text base
require processing all text changed / selected text changed events to generate
proper output?

+    });
+
+    while (this.eventQueue_.length) {
+      var now = new Date();
+      var delta = now - this.eventQueue_[0].enqueueTime;
+      if (delta < this.EVENT_QUEUE_DELAY_MS) {
+        this.eventQueueTimerId_ = window.setTimeout(
+            this.executeNextEventFromQueue_, this.EVENT_QUEUE_DELAY_MS - delta);
+        return;
+      }
+
+      var evt = this.eventQueue_.shift();
+      this.listeners_[evt.type](evt);
+    }
+  },
+
+  /**
    * Provides all feedback once ChromeVox's focus changes.
    * @param {Object} evt
    */
   onEventDefault: function(evt) {
     var node = evt.target;
 
     if (!node)
       return;
 
     var prevRange = this.currentRange_;
@@ skipping 33 matching lines @@
   onFocus: function(evt) {
     var node = evt.target;
 
     // It doesn't make sense to focus the containing web area if a descendant
     // has focused state.
     if (node.role == 'rootWebArea') {
       node = AutomationUtil.findNodePost(node,
                                          Dir.FORWARD,
                                          AutomationPredicate.focused) || node;
     }
+
+    if (evt.target.role == 'textField' || evt.target.role == 'textBox') {

[David Tseng, 2015/08/19 17:19:25]

nit: please use RoleType.textField and RoleType.textBox

+      this.createEditableTextHandlerIfNeeded_(evt.target);
+    }
+
     this.onEventDefault({target: node, type: 'focus'});
   },
 
   /**
    * Provides all feedback once a load complete event fires.
    * @param {Object} evt
    */
   onLoadComplete: function(evt) {
     this.setupChromeVoxVariants_(evt.target.docUrl);
 
@@ skipping 37 matching lines @@
       return;
 
     if (!evt.target.state.focused)
       return;
 
     if (!this.currentRange_) {
       this.onEventDefault(evt);
       this.currentRange_ = cursors.Range.fromNode(evt.target);
     }
 
+    this.createEditableTextHandlerIfNeeded_(evt.target);
     var textChangeEvent = new cvox.TextChangeEvent(
         evt.target.value,
         evt.target.textSelStart,
         evt.target.textSelEnd,
         true);  // triggered by user
-    if (!this.editableTextHandler ||
-        evt.target != this.currentRange_.start.node) {
-      this.editableTextHandler =
-          new cvox.ChromeVoxEditableTextBase(
-              textChangeEvent.value,
-              textChangeEvent.start,
-              textChangeEvent.end,
-              evt.target.state['protected'],
-              cvox.ChromeVox.tts);
-    }
+    this.editableTextHandler_.changed(textChangeEvent);
 
-    this.editableTextHandler.changed(textChangeEvent);
     new Output().withBraille(
             this.currentRange_, null, evt.type)
         .go();
   },
 
   /**
    * Provides all feedback once a value changed event fires.
    * @param {Object} evt
    */
   onValueChanged: function(evt) {
@@ skipping 141 matching lines @@
         // When in compat mode, if the focus is within the desktop tree proper,
         // then do not disable content scripts.
         if (this.currentRange_.start.node.root.role == 'desktop')
           return;
 
         this.disableClassicChromeVox_();
       }
     }.bind(this));
 
     this.mode_ = mode;
-  }
+  },
+
+  /**
+   * Create an editable text handler for the given node if needed.
+   * @param {Object} node

[David Tseng, 2015/08/19 17:19:25]

s/Object/AutomationNode

+   */

[David Tseng, 2015/08/19 17:19:25]

@private

+  createEditableTextHandlerIfNeeded_: function(node) {
+    if (!this.editableTextHandler_ || node != this.currentRange_.start.node) {
+      this.editableTextHandler_ =
+          new cvox.ChromeVoxEditableTextBase(
+              node.value,
+              node.textSelStart,
+              node.textSelEnd,
+              node.state['protected'],
+              cvox.ChromeVox.tts);
+    }
+  },
 };
 
 /** @type {Background} */
 global.backgroundObj = new Background();
 
 });  // goog.scope