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

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 6abdb980da3fb97047fd01d0c1fa8e98aa49325b..0c552d39b1d90fae1c76746e1204bfc5dfd2a04f 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
@@ -65,6 +65,23 @@ Background = function() {
   /** @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')
@@ -88,6 +105,12 @@ Background = function() {
     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.
@@ -123,7 +146,7 @@ Background.prototype = {
   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);
@@ -401,6 +424,59 @@ Background.prototype = {
   },
 
   /**
+   * 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
    */
@@ -454,6 +530,11 @@ Background.prototype = {
                                          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'});
   },
 
@@ -511,23 +592,14 @@ Background.prototype = {
       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();
@@ -689,7 +761,23 @@ Background.prototype = {
     }.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} */