Provide complete, smart, spoken feedback for editable selection

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

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
index 199e4065051218e14740693862f0c68e14530339..f69e70fdc42124976b7b6fe3cfea6baca0ec4138 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
@@ -253,10 +253,18 @@ AutomationRichEditableText.prototype = {
     // extended from anchor or focus. The default behavior is to compute lines
     // via focus.
     var baseLineOnStart = prevFocusLine.isSameLineAndSelection(focusLine);
+    var isSameSelection =
+        baseLineOnStart && prevAnchorLine.isSameLineAndSelection(anchorLine);
 
-    var cur = new editing.EditableLine(
-        root.anchorObject, root.anchorOffset, root.focusObject,
-        root.focusOffset, baseLineOnStart);
+    var cur;
+    if (isSameSelection && this.line_) {
+      // Nothing changed, return.
+      return;
+    } else {
+      cur = new editing.EditableLine(
+          root.anchorObject, root.anchorOffset, root.focusObject,
+          root.focusOffset, baseLineOnStart);
+    }
     var prev = this.line_;
     this.line_ = cur;
 
@@ -317,8 +325,65 @@ AutomationRichEditableText.prototype = {
           .go();
     } else if (!cur.hasCollapsedSelection()) {
       // This is a selection.
-      cvox.ChromeVox.tts.speak(cur.selectedText, cvox.QueueMode.CATEGORY_FLUSH);
-      cvox.ChromeVox.tts.speak(Msgs.getMsg('selected'), cvox.QueueMode.QUEUE);
+
+      // Speech requires many more states than braille.
+      // Note that base and extend are used here rather than anchor

[dmazzoni, 2017/07/06 21:47:01]

We should use terminology correctly. Currently Blink
is exposing the selection start and end, but that's a bug.
This change fixes it so that it really exposes the focus
and anchor, as advertised:

https://chromium-review.googlesource.com/c/562645/

Inside Blink code, base and extent (not extend) are
the same as focus and anchor. I don't think we need to
invent different names here, we should just fix the
underlying bug and then use focus and anchor consistently.

[David Tseng, 2017/07/06 22:01:36]

I agree but would also like to land this and the dependent Docs change. I've
renamed the extend -> extent and added a TODO.

Note that this class already tries to detect the anchor (as you've correctly
defined) above so it requires another change at least anyway to clean up.

+      // and focus as concepts because anchor and focus are absolute
+      // while base and extend are relative.
+      var curBase = baseLineOnStart ? focusLine : anchorLine;
+      var curExtend = baseLineOnStart ? anchorLine : focusLine;
+      var text = '';
+      var suffixMsg = '';
+      if (curBase.isBeforeLine(curExtend)) {
+        // Forward selection.
+        if (prev.isBeforeLine(curBase)) {
+          // Wrapped across the baseline. Read out the new selection.
+          suffixMsg = 'selected';
+          text = this.getTextSelection_(
+              curBase.startContainer_, curBase.localStartOffset,
+              curExtend.endContainer_, curExtend.localEndOffset);
+        } else {
+          if (prev.isBeforeLine(curExtend)) {
+            // Grew.
+            suffixMsg = 'selected';
+            text = this.getTextSelection_(
+                prev.endContainer_, prev.localEndOffset,
+                curExtend.endContainer_, curExtend.localEndOffset);
+          } else {
+            // Shrank.
+            suffixMsg = 'unselected';
+            text = this.getTextSelection_(
+                curExtend.endContainer_, curExtend.localEndOffset,
+                prev.endContainer_, prev.localEndOffset);
+          }
+        }
+      } else {
+        // Backward selection.
+        if (curBase.isBeforeLine(prev)) {
+          // Wrapped across the baseline. Read out the new selection.
+          suffixMsg = 'selected';
+          text = this.getTextSelection_(
+              curExtend.startContainer_, curExtend.localStartOffset,
+              curBase.endContainer_, curBase.localEndOffset);
+        } else {
+          if (curExtend.isBeforeLine(prev)) {
+            // Grew.
+            suffixMsg = 'selected';
+            text = this.getTextSelection_(
+                curExtend.startContainer_, curExtend.localStartOffset,
+                prev.startContainer_, prev.localStartOffset);
+          } else {
+            // Shrank.
+            suffixMsg = 'unselected';
+            text = this.getTextSelection_(
+                prev.startContainer_, prev.localStartOffset,
+                curExtend.startContainer_, curExtend.localStartOffset);
+          }
+        }
+      }
+
+      cvox.ChromeVox.tts.speak(text, cvox.QueueMode.CATEGORY_FLUSH);
+      cvox.ChromeVox.tts.speak(Msgs.getMsg(suffixMsg), cvox.QueueMode.QUEUE);
       this.brailleCurrentRichLine_();
     } else {
       // Describe the current line. This accounts for previous/current
@@ -336,6 +401,40 @@ AutomationRichEditableText.prototype = {
     this.end = cur.endOffset;
   },
 
+  /**
+   * @param {AutomationNode|undefined} startNode
+   * @param {number} startOffset
+   * @param {AutomationNode|undefined} endNode
+   * @param {number} endOffset
+   * @return {string}
+   */
+  getTextSelection_: function(startNode, startOffset, endNode, endOffset) {
+    if (!startNode || !endNode)
+      return '';
+
+    if (startNode == endNode) {
+      return startNode.name ? startNode.name.substring(startOffset, endOffset) :
+                              '';
+    }
+
+    var text = '';
+    if (startNode.name)
+      text = startNode.name.substring(startOffset);
+
+    for (var node = startNode;
+         (node = AutomationUtil.findNextNode(
+              node, Dir.FORWARD, AutomationPredicate.leafOrStaticText)) &&
+         node != endNode;) {
+      // Padding needs to get added to break up speech utterances.
+      if (node.name)
+        text += ' ' + node.name;
+    }
+
+    if (endNode.name)
+      text += ' ' + endNode.name.substring(0, endOffset);
+    return text;
+  },
+
   /**
    * @param {number} markerType
    * @param {boolean=} opt_end
@@ -543,6 +642,8 @@ editing.EditableLine = function(
   this.end_ = this.end_.deepEquivalent || this.end_;
   /** @private {number} */
   this.localContainerStartOffset_ = startIndex;
+  /** @private {number} */
+  this.localContainerEndOffset_ = endIndex;
 
   // Computed members.
   /** @private {Spannable} */
@@ -583,10 +684,16 @@ editing.EditableLine.prototype = {
     if (lineBase.node == lineExtend.node)
       this.value_.setSpan(lineExtend, 0, nameLen);
 
+    this.startContainer_ = this.start_.node;
+    if (this.startContainer_.role == RoleType.INLINE_TEXT_BOX)
+      this.startContainer_ = this.startContainer_.parent;
+    this.endContainer_ = this.end_.node;
+    if (this.endContainer_.role == RoleType.INLINE_TEXT_BOX)
+      this.endContainer_ = this.endContainer_.parent;
+
     // Initialize defaults.
     this.lineStart_ = lineBase.node;
     this.lineEnd_ = this.lineStart_;
-    this.startContainer_ = this.lineStart_.parent;
     this.lineStartContainer_ = this.lineStart_.parent;
     this.lineEndContainer_ = this.lineStart_.parent;
 
@@ -739,7 +846,7 @@ editing.EditableLine.prototype = {
    * @return {number}
    */
   get localStartOffset() {
-    return this.startOffset - this.containerStartOffset;
+    return this.localContainerStartOffset_;
   },
 
   /**
@@ -748,7 +855,7 @@ editing.EditableLine.prototype = {
    * @return {number}
    */
   get localEndOffset() {
-    return this.endOffset - this.containerStartOffset;
+    return this.localContainerEndOffset_;
   },
 
   /**
@@ -815,6 +922,19 @@ editing.EditableLine.prototype = {
     return this.isSameLine(otherLine) &&
         this.startOffset == otherLine.startOffset &&
         this.endOffset == otherLine.endOffset;
+  },
+
+  /**
+   * Returns whether this line comes before |otherLine| in document order.
+   * @return {boolean}
+   */
+  isBeforeLine: function(otherLine) {
+    if (this.isSameLine(otherLine) || !this.lineStartContainer_ ||
+        !otherLine.lineStartContainer_)
+      return false;
+    return AutomationUtil.getDirection(
+               this.lineStartContainer_, otherLine.lineStartContainer_) ==
+        Dir.FORWARD;
   }
 };