Refine braille output

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

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
index 84e99215d7150a3dc046190cebad4af40f8d3f1d..886f4fad6579b67be9b77471d231fd1f72dba55c 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
@@ -650,14 +650,12 @@ Output.EarconAction.prototype = {
  * Annotation for text with a selection inside it.
  * @param {number} startIndex
  * @param {number} endIndex
- * @param {number=} opt_offset
  * @constructor
  */
-Output.SelectionSpan = function(startIndex, endIndex, opt_offset) {
+Output.SelectionSpan = function(startIndex, endIndex) {
   // TODO(dtseng): Direction lost below; should preserve for braille panning.
   this.startIndex = startIndex < endIndex ? startIndex : endIndex;
   this.endIndex = endIndex > startIndex ? endIndex : startIndex;
-  this.offset = opt_offset || 0;
 };
 
 /**
@@ -872,7 +870,6 @@ Output.prototype = {
 
     this.formatOptions_ = {speech: false, braille: true, auralStyle: false};
     this.format_(node, formatStr, this.brailleBuffer_);
-
     return this;
   },
 
@@ -932,7 +929,7 @@ Output.prototype = {
 
     // Braille.
     if (this.brailleBuffer_.length) {
-      var buff = this.createBrailleOutput_();
+      var buff = this.mergeBraille_(this.brailleBuffer_);
       var selSpan =
           buff.getSpanInstanceOf(Output.SelectionSpan);
       var startIndex = -1, endIndex = -1;
@@ -941,7 +938,7 @@ Output.prototype = {
         var valueEnd = buff.getSpanEnd(selSpan);
         startIndex = valueStart + selSpan.startIndex;
         endIndex = valueStart + selSpan.endIndex;
-        buff.setSpan(new cvox.ValueSpan(selSpan.offset), valueStart, valueEnd);
+        buff.setSpan(new cvox.ValueSpan(0), valueStart, valueEnd);
         buff.setSpan(new cvox.ValueSelectionSpan(), startIndex, endIndex);
       }
 
@@ -1038,11 +1035,6 @@ Output.prototype = {
       if (options.isUnique)
         token = token.substring(0, token.length - 1);
 
-      // Annotate braille output with the corresponding automation nodes
-      // to support acting on nodes based on location in the output.
-      if (node && this.formatOptions_.braille)
-        options.annotation.push(new Output.NodeSpan(node));
-
       // Process token based on prefix.
       var prefix = token[0];
       token = token.slice(1);
@@ -1068,7 +1060,7 @@ Output.prototype = {
           options.annotation.push(token);
           if (selectedText) {
             this.append_(buff, selectedText, options);
-            this.append_(buff, Msgs.getMsg('selected'), options);
+            this.append_(buff, Msgs.getMsg('selected'));
           } else {
             this.append_(buff, text, options);
           }
@@ -1498,7 +1490,8 @@ Output.prototype = {
         this.format_(formatPrevNode, roleBlock.leave, buff, prevNode);
     }
 
-    var enterOutputs = [];
+    // Customize for braille node annotations.
+    var originalBuff = buff;
     var enterRole = {};
     for (var j = uniqueAncestors.length - 1, formatNode;
          (formatNode = uniqueAncestors[j]);
@@ -1507,8 +1500,18 @@ Output.prototype = {
       if (roleBlock.enter) {
         if (enterRole[formatNode.role])
           continue;
+
+        if (this.formatOptions_.braille)
+          buff = [];
+
         enterRole[formatNode.role] = true;
         this.format_(formatNode, roleBlock.enter, buff, prevNode);
+
+        if (this.formatOptions_.braille && buff.length) {
+          var nodeSpan = this.mergeBraille_(buff);
+          nodeSpan.setSpan(new Output.NodeSpan(formatNode), 0, nodeSpan.length);
+          originalBuff.push(nodeSpan);
+        }
       }
     }
   },
@@ -1521,6 +1524,11 @@ Output.prototype = {
    * @private
    */
   node_: function(node, prevNode, type, buff) {
+    var originalBuff = buff;
+
+    if (this.formatOptions_.braille)
+      buff = [];
+
     // Navigate is the default event.
     var eventBlock = Output.RULES[type] || Output.RULES['navigate'];
     var roleBlock = eventBlock[node.role] || {};
@@ -1531,6 +1539,13 @@ Output.prototype = {
         eventBlock['default'].speak;
 
     this.format_(node, speakFormat, buff, prevNode);
+
+    // Restore braille and add an annotation for this node.
+    if (this.formatOptions_.braille) {
+      var nodeSpan = this.mergeBraille_(buff);
+      nodeSpan.setSpan(new Output.NodeSpan(node), 0, nodeSpan.length);
+      originalBuff.push(nodeSpan);
+    }
   },
 
   /**
@@ -1556,12 +1571,29 @@ Output.prototype = {
       options.annotation.push(new Output.NodeSpan(node));
       var selStart = node.textSelStart;
       var selEnd = node.textSelEnd;
+
       if (selStart !== undefined &&
-          (selEnd >= startIndex && selStart <= endIndex)) {
+          selEnd >= startIndex && selStart <= endIndex) {
+        // Editable text selection.
+        // Note that there are two sets of indicies here.

[dmazzoni, 2016/11/10 17:17:22]

nit: indicies -> indices (throughout)

[David Tseng, 2016/11/10 23:17:04]

Done.

+
+        // |startIndex| and |endIndex| are indicies set by the caller
+        // and are assumed to be inside of the range. In braille, we
+        // only ever expect to get ranges surrounding a line as
+        // anything smaller doesn't make sense.
+
+        // The second set of indicies is set by the browser and

[dmazzoni, 2016/11/10 17:17:22]

Nit: clarify that the second set of indices you're referring to
are |selStart| and |selEnd|, if I understand correctly, like
you referred to |startIndex| and |endIndex|, above.

How about renaming startIndex -> rangeStart, and
endIndex -> rangeEnd, or something along those lines?
"Index" is kind of redundant since they're all indices, but
if the type is useful then to be consistent you could have
rangeStartIndex, selStartIndex, and so on.

[David Tseng, 2016/11/10 23:17:04]

Done.

+        // reflects the editable selection. Since both sets of
+        // indicies are absolute indicies into the text content, we
+        // can safely take the difference between the selection
+        // indicies and the start index of the range to arrive at the
+        // relative index for the line output. See editing_test.js for
+        // examples.
         options.annotation.push(new Output.SelectionSpan(
-            selStart - startIndex,
-            selEnd - startIndex,
-            startIndex));
+            selStart - startIndex, selEnd - startIndex));
+      } else {
+        // Non-editable text selection.
+        options.annotation.push(new Output.SelectionSpan(startIndex, endIndex));
       }
     }
 
@@ -1570,8 +1602,16 @@ Output.prototype = {
     var earcon = this.findEarcon_(node, prevNode);
     if (earcon)
       options.annotation.push(earcon);
-    this.append_(buff, range.start.getText().substring(startIndex, endIndex),
-        options);
+    var text = '';
+    text = range.start.getText().substring(startIndex, endIndex);
+
+    // In braille, we almost always want to show the entire contents and simply
+    // place the cursor under the selection.
+    if (this.formatOptions_.braille && !node.state.editable)
+      text = range.start.getText();

[dmazzoni, 2016/11/10 17:17:22]

How about making this an if/else with a comment explaining both the
speech case and the braille case. For speech, the spoken text is
just the subrange, whereas for braille, it's the whole node with the
cursor (or selection, right) underlined.

[David Tseng, 2016/11/10 23:17:04]

Done (if/else). The else case is both speech and non-editable braille.
Non-editable bralile probably has no selection and we want to show what
ChromeVox selects.

We'll have to re-think this a little more because DOM selections (i.e.
anchorOffset, and friends) are not well represented here. With three ways to
represent selections, I guess at some point we could abstract away with a
utility class.

+
+    this.append_(buff, text, options);
+
     if (!this.outputContextFirst_)
       this.ancestry_(node, prevNode, type, buff);
 
@@ -1673,65 +1713,81 @@ Output.prototype = {
   },
 
   /**
-   * Converts the currently rendered braille buffers to a single spannable.
+   * Converts the braille |spans| buffer to a single spannable.
+   * @param {!Array<Spannable>} spans
    * @return {!Spannable}
    * @private
    */
-  createBrailleOutput_: function() {
-    var result = new Spannable();
+  mergeBraille_: function(spans) {
     var separator = '';  // Changes to space as appropriate.
-    this.brailleBuffer_.forEach(function(cur) {
-      // If this chunk is empty, don't add it since it won't result
-      // in any output on the braille display, but node spans would
-      // start before the separator in that case, which is not desired.
-      // The exception is if this chunk contains a selectionm, in which
-      // case it will result in a cursor which has to be preserved.
-      // In this case, having separators, potentially both before and after
-      // the empty string is correct.
-      if (cur.length == 0 && !cur.getSpanInstanceOf(Output.SelectionSpan))
-        return;
-      var spansToExtend = [];
-      var spansToRemove = [];
-      // Nodes that have node spans both on the character to the left
-      // of the separator and to the right should also cover the separator.
-      // We extend the left span to cover both the separator and what the
-      // right span used to cover, removing the right span, mostly for
-      // ease of writing tests and debug.
-      // Note that getSpan(position) never returns zero length spans
-      // (because they don't cover any position).  Still, we want to include
-      // these because they can be included (the selection span in an empty
-      // text field is an example), which is why we write the below code
-      // using getSpansInstanceOf and check the endpoints (isntead of doing
-      // the opposite).
-      result.getSpansInstanceOf(Output.NodeSpan).forEach(function(leftSpan) {
-        if (result.getSpanEnd(leftSpan) < result.length)
-          return;
-        var newEnd = result.length;
-        cur.getSpansInstanceOf(Output.NodeSpan).forEach(function(rightSpan) {
-          if (cur.getSpanStart(rightSpan) == 0 &&
-              leftSpan.node === rightSpan.node) {
-            newEnd = Math.max(
-                newEnd,
-                result.length + separator.length +
-                    cur.getSpanEnd(rightSpan));
-            spansToRemove.push(rightSpan);
-          }
-        });
-        if (newEnd > result.length)
-          spansToExtend.push({span: leftSpan, end: newEnd});
+    var prevInline = {};
+    var lookupInline = function(node) {
+      return prevInline[node];
+    };
+    var hashInlineSiblings = function(dict, node) {

[dmazzoni, 2016/11/10 17:17:22]

It seems you just want a set, right?

How about: addNodesOnSameLineToSet(set, node) that uses a Set()?

[David Tseng, 2016/11/10 23:17:04]

Good point; using WeakSet now.

+      var inlineSibling = node;
+      while (inlineSibling) {
+        dict[inlineSibling] = inlineSibling;
+        if (inlineSibling.role == RoleType.inlineTextBox)
+          dict[inlineSibling.parent] = inlineSibling.parent;

[dmazzoni, 2016/11/10 17:17:22]

Note that the parent of an inline text box might be partially on the same line
but not
completely. Consider this html:

<pre>One two\n
three <span>four</span></pre>

The static text object containing "One two\nthree " spans two lines. If |node|
is the inline text box for "four", do you want the static text "One two\nthree "
to be considered part of that line even if only part of it is on that line?

Either way, it'd be helpful to comment the exact purpose of this set so that
it's
clear both what you expect it to contain, semantically, and how you compute it
(you and I both know inline text boxes but a future hacker on this code might
not understand what assumptions it's making).

[David Tseng, 2016/11/10 23:17:04]

Alright, I simplified this more and it appears display inline is all we need.

+        inlineSibling = inlineSibling.nextOnLine;
+      }
+      inlineSibling = node.previousOnLine;
+      while (inlineSibling) {
+        dict[inlineSibling] = inlineSibling;
+        if (inlineSibling.role == RoleType.inlineTextBox)
+          dict[inlineSibling.parent] = inlineSibling.parent;
+        inlineSibling = inlineSibling.previousOnLine;
+      }
+    };
+
+    return spans.reduce(function(result, cur) {
+      // Ignore empty spans except when they contain a selection.
+      var hasSelection = cur.getSpanInstanceOf(Output.SelectionSpan);
+      if (cur.length == 0 && !hasSelection)
+        return result;
+
+      // For empty selections, we just add the space separator to account for
+      // showing the braille cursor.
+      if (cur.length == 0 && hasSelection) {
+        result.append(cur);
+        result.append(Output.SPACE);
+        separator = '';
+        return result;
+      }
+
+      // Grab all of the nodes associated with |cur|.
+      var curNodes = cur.getSpansInstanceOf(Output.NodeSpan).map(function(s) {
+        return s.node;
       });
+
+      // Now, decide whether we should include separators between the previous
+      // span and |cur|.
+      // Never separate chunks without something already there at this point.
+      if (result.length == 0 ||
+          curNodes.some(lookupInline))

[dmazzoni, 2016/11/10 17:17:22]

nit: fits on previous line

[David Tseng, 2016/11/10 23:17:04]

Acknowledged.

+        separator = '';
+      else
+      separator = Output.SPACE;

[dmazzoni, 2016/11/10 17:17:22]

nit: indent

[David Tseng, 2016/11/10 23:17:04]

Done.

+
       result.append(separator);
       result.append(cur);
-      spansToExtend.forEach(function(elem) {
-        result.setSpan(
-            elem.span,
-            result.getSpanStart(elem.span),
-            elem.end);
-      });
-      spansToRemove.forEach(result.removeSpan.bind(result));
-      separator = Output.SPACE;
-    });
-    return result;
+
+      // Refresh |prevInline| for the next span.  Any two spans who have
+      // associated nodes that fall on the same line and are themselves text
+      // nodes should not cause a separator.
+      prevInline = curNodes.reduce(function(dict, node) {
+        if (node.role == RoleType.inlineTextBox)
+          hashInlineSiblings(dict, node);

[dmazzoni, 2016/11/10 17:17:22]

Maybe hashInlineSiblings or whatever you end up calling it could handle whether
it's a static text node or inline text box. Right now the logic of dealing with
that subtle distinction is spread across two places, and it might be cleaner
to put it all in hashInlineSiblings and document that more clearly.

[David Tseng, 2016/11/10 23:17:04]

Acknowledged.

+
+        if (node.role == RoleType.staticText)
+          hashInlineSiblings(dict, node.firstChild);
+
+        return dict;
+      }, {});
+
+      return result;
+    }, new Spannable());
   },
 
   /**
@@ -1796,7 +1852,7 @@ Output.prototype = {
    * @return {!Spannable}
    */
   get brailleOutputForTest() {
-    return this.createBrailleOutput_();
+    return this.mergeBraille_(this.brailleBuffer_);
   }
 };