Refactor Select-to-speak so that mouse events are forwarded to the extension.

chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js

Index: chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
diff --git a/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js b/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
index 21d3c20143460cc10d370b89c63822d445b6715f..07baa123e7e8646a5dc090851cef3a9554af4978 100644
--- a/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
+++ b/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
@@ -58,15 +58,25 @@ var SelectToSpeak = function() {
   /** @private {{x: number, y: number}} */
   this.mouseStart_ = {x: 0, y: 0};
 
+  /** @private {{x: number, y: number}} */
+  this.mouseEnd_ = {x: 0, y: 0};
+
+  /** @private {AutomationRootNode} */
   chrome.automation.getDesktop(function(desktop) {
+    this.desktop_ = desktop;
+
+    // After the user selects a region of the screen, we do a hit test at
+    // the center of that box using the automation API. The result of the
+    // hit test is a MOUSE_RELEASED accessibility event.
     desktop.addEventListener(
-        EventType.MOUSE_PRESSED, this.onMousePressed_.bind(this), true);
-    desktop.addEventListener(
-        EventType.MOUSE_DRAGGED, this.onMouseDragged_.bind(this), true);
-    desktop.addEventListener(
-        EventType.MOUSE_RELEASED, this.onMouseReleased_.bind(this), true);
+        EventType.MOUSE_RELEASED, this.onMouseReleasedHitTest_.bind(this),
+        true);
+
+    // A MOUSE_CANCELED accessibility event is fired if the user releases

[David Tseng, 2017/05/07 06:10:27]

Would be great to also handle key events in this extension; it's still looking
to be split in ways that a new reader would find confusing.

[dmazzoni, 2017/05/10 19:43:18]

OK, done. The event handler now forwards all keyboard
events too, and the extension now handles canceling
via releasing the Search key after clicking, or
tapping and releasing Search or Control.

+    // the Search key while the mouse is still down.
     desktop.addEventListener(
-        EventType.MOUSE_CANCELED, this.onMouseCanceled_.bind(this), true);
+        EventType.MOUSE_CANCELED, this.onMouseCanceled_.bind(this),
+        true);
   }.bind(this));
 
   /** @private { ?string } */
@@ -85,6 +95,8 @@ var SelectToSpeak = function() {
   this.color_ = "#f73a98";
 
   this.initPreferences_();
+
+  this.captureMouseEvents_();
 };
 
 SelectToSpeak.prototype = {
@@ -93,14 +105,17 @@ SelectToSpeak.prototype = {
    * select-to-speak is capturing mouse events (for example holding down
    * Search).
    *
-   * @param {!AutomationEvent} evt
+   * @param {!Event} evt The DOM event

[David Tseng, 2017/05/07 06:10:27]

Dom event listeners have a return value (i.e. add @return).

[dmazzoni, 2017/05/10 19:43:18]

Done.

    */
-  onMousePressed_: function(evt) {
+  onMouseDown_: function(evt) {
     this.down_ = true;

[David Tseng, 2017/05/07 06:59:40]

Now that you have the mouse event, I don't think you need to track down state.
Just use |evt.which|.

Also, evt.metaKey, evt.ctrlKey, ...
might clean things up dramatically as you can just check them in all of the
handlers and remove state from the native handler (and possibly here).

[dmazzoni, 2017/05/10 19:43:18]

Changed to this.trackingMouse_ - it's not whether or not
the button is down, it's whether or not we're currently
tracking it.
There's some state changes that need to happen on up and
down, so I'm not sure evt.metaKey or evt.ctrlKey would help us
much. The logic is basically all in the extension now, though,
so see what you think.

Some examples of corner cases that might fail if we just tried
to check evt.metaKey and not keep track of more state:

* The mouse is down when the extension starts so the
  first event we get is move or up
* The user clicks and starts dragging and then presses Search

-    this.mouseStart_ = {x: evt.mouseX, y: evt.mouseY};
-    this.startNode_ = evt.target;
+    this.mouseStart_ = {x: evt.screenX, y: evt.screenY};
     chrome.tts.stop();
-    this.onMouseDragged_(evt);
+
+    // Fire a hit test event on click to warm up the cache.
+    this.desktop_.hitTest(ctrX, ctrY, EventType.NONE);
+
+    this.onMouseMove_(evt);
   },
 
   /**
@@ -108,15 +123,15 @@ SelectToSpeak.prototype = {
    * mode where select-to-speak is capturing mouse events (for example
    * holding down Search).
    *
-   * @param {!AutomationEvent} evt
+   * @param {!Event} evt The DOM event

[David Tseng, 2017/05/07 06:10:27]

Ditto, @return

[dmazzoni, 2017/05/10 19:43:18]

Done.

    */
-  onMouseDragged_: function(evt) {
+  onMouseMove_: function(evt) {
     if (!this.down_)
       return;
 
     var rect = rectFromPoints(
         this.mouseStart_.x, this.mouseStart_.y,
-        evt.mouseX, evt.mouseY);
+        evt.screenX, evt.screenY);
     chrome.accessibilityPrivate.setFocusRing([rect], this.color_);
   },
 
@@ -127,17 +142,37 @@ SelectToSpeak.prototype = {
    *
    * @param {!AutomationEvent} evt

[David Tseng, 2017/05/07 06:10:27]

This is a DOM event listener, not an automation event listener

[dmazzoni, 2017/05/10 19:43:18]

Done.

    */

[David Tseng, 2017/05/07 06:10:27]

@return

[dmazzoni, 2017/05/10 19:43:18]

Done.

-  onMouseReleased_: function(evt) {
-    this.onMouseDragged_(evt);
+  onMouseUp_: function(evt) {
+    this.onMouseMove_(evt);
     this.down_ = false;
 
     chrome.accessibilityPrivate.setFocusRing([]);
 
+    this.mouseEnd_ = {x: evt.screenX, y: evt.screenY};
+    var ctrX = Math.floor((this.mouseStart_.x + this.mouseEnd_.x) / 2);
+    var ctrY = Math.floor((this.mouseStart_.y + this.mouseEnd_.y) / 2);
+
+    // Do a hit test at the center of the area the user dragged over.
+    // This will give us some context when searching the accessibility tree.
+    // The hit test will result in a EventType.MOUSE_RELEASED event being
+    // fired on the result of that hit test, which will trigger
+    // onMouseReleasedHitTest_.

[David Tseng, 2017/05/07 06:10:27]

Just wondering, but it seems confusing to have a mouse released event in the
accessibility enums. Can we use some other event here and remove the mouse
related events from accessibility?

[dmazzoni, 2017/05/10 19:43:18]

I agree, this is confusing.

I think we should get rid of the mouse events from
accessibility, but there's no other obvious event to
return. How about as a follow-up I delete all of the
mouse events and replace them with a HIT_TEST_RESPONSE
event?

+    this.desktop_.hitTest(ctrX, ctrY, EventType.MOUSE_RELEASED);
+  },
+
+  /**
+   * Called in response to our hit test afterthe mouse is released,

[David Tseng, 2017/05/07 06:10:27]

nit: after the

[dmazzoni, 2017/05/10 19:43:18]

Done.

+   * when the user is in a mode where select-to-speak is capturing
+   * mouse events (for example holding down Search).
+   *
+   * @param {!AutomationEvent} evt The automation event.
+   */
+  onMouseReleasedHitTest_: function(evt) {

[David Tseng, 2017/05/07 06:10:27]

nit: Maybe rename the listeners like
onAutomationFooEvent and
onMouseFooEvent

[dmazzoni, 2017/05/10 19:43:18]

Done.

     // Walk up to the nearest window, web area, or dialog that the

[David Tseng, 2017/05/07 06:10:28]

Might as well include toolbar as well.

[dmazzoni, 2017/05/10 19:43:18]

Done.

     // hit node is contained inside. Only speak objects within that
     // container. In the future we might include other container-like
     // roles here.
-    var root = this.startNode_;
+    var root = evt.target;
     while (root.parent &&
         root.role != RoleType.WINDOW &&
         root.role != RoleType.ROOT_WEB_AREA &&
@@ -148,13 +183,22 @@ SelectToSpeak.prototype = {
 
     var rect = rectFromPoints(
         this.mouseStart_.x, this.mouseStart_.y,
-        evt.mouseX, evt.mouseY);
+        this.mouseEnd_.x, this.mouseEnd_.y);
     var nodes = [];
     this.findAllMatching_(root, rect, nodes);
     this.startSpeechQueue_(nodes);
   },
 
   /**
+   * Set up event listeners for all mouse events.
+   */
+  captureMouseEvents_: function() {
+    document.addEventListener('mousedown', this.onMouseDown_.bind(this));

[David Tseng, 2017/05/07 06:10:27]

Do these listeners need to be removed at some point or does the reader need to
know that mouse events don't get sent when some things occur in the native event
handler?

[dmazzoni, 2017/05/10 19:43:19]

We're tracking keys in the extension now, as suggested,
so it doesn't matter whether the event handler sends all
keys or not. But either way I don't see any reason to
remove the event listeners. It's simpler to just return
if we don't care about some events, rather than constantly
adding and removing listeners.

+    document.addEventListener('mouseup', this.onMouseUp_.bind(this));
+    document.addEventListener('mousemove', this.onMouseMove_.bind(this));

[David Tseng, 2017/05/07 06:10:27]

nit: sort

[dmazzoni, 2017/05/10 19:43:19]

Done.

+  },
+
+  /**
    * Called when the user cancels select-to-speak's capturing of mouse
    * events (for example by releasing Search while the mouse is still down).
    *