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..e538ea071e456eaeaa621ed0fc843ad4b74b4523 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);
+    }
   },
 
   /**
@@ -1550,18 +1565,30 @@ Output.prototype = {
       return;
 
     var options = {annotation: ['name'], isUnique: true};
-    var startIndex = range.start.index;
-    var endIndex = range.end.index;
+    var rangeStart = range.start.index;
+    var rangeEnd = range.end.index;
     if (this.formatOptions_.braille) {
       options.annotation.push(new Output.NodeSpan(node));
       var selStart = node.textSelStart;
       var selEnd = node.textSelEnd;
+
       if (selStart !== undefined &&
-          (selEnd >= startIndex && selStart <= endIndex)) {
+          selEnd >= rangeStart && selStart <= rangeEnd) {
+        // Editable text selection.
+
+        // |rangeStart| and |rangeEnd| are indices 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.
+
+        // |selStart| and |selEnd| reflect the editable selection. The relative
+        // selStart and relative selEnd for the current line are then just the
+        // difference between |selStart|, |selEnd| with |rangeStart|.
+        // See editing_test.js for examples.
         options.annotation.push(new Output.SelectionSpan(
-            selStart - startIndex,
-            selEnd - startIndex,
-            startIndex));
+            selStart - rangeStart, selEnd - rangeStart));
+      } else {
+        // Non-editable text selection.
+        options.annotation.push(new Output.SelectionSpan(rangeStart, rangeEnd));
       }
     }
 
@@ -1570,13 +1597,24 @@ 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 = '';
+
+    if (this.formatOptions_.braille && !node.state.editable) {
+      // In braille, we almost always want to show the entire contents and
+      // simply place the cursor under the SelectionSpan we set above.
+      text = range.start.getText();
+    } else {
+      // This is output for speech or non-editable braille.

[dmazzoni, 2016/11/10 23:31:29]

This appears backwards from your logic. Above, you have if
(this.formatOptions_.braille && !node.state.editable), but your comment says
that the else case is for non-editable braille

[David Tseng, 2016/11/10 23:54:59]

Changed comment.
The code is right. In editable braille, we want to restrict to the start/end
bounds of the range which is always a line. The if clause covers when we're in
non-editable braille and moving by characters and words not showing just the
spoken word.

[David Tseng, 2016/11/11 02:36:27]

Alright, to clarify things, I added a test and constrained more of this
SelectionSpan. We are now only setting the SelectionSpan on non-editable content
when that selection is less than the full contents of the range (it's really
distracting to read braille when everything's underlined).

Also, I filed a bug to clean up output.js in general in phase4. We can split a
lot of these things up (e.g. BaseOutput, OutputParser, BrailleOutput,
SpeechOutput, OutputRule, OutputRuleGroup, etc); should look a lot prettier
afterwards..

+      text = range.start.getText().substring(rangeStart, rangeEnd);
+    }
+
+    this.append_(buff, text, options);
+
     if (!this.outputContextFirst_)
       this.ancestry_(node, prevNode, type, buff);
 
     var loc =
-        range.start.node.boundsForRange(startIndex, endIndex);
+        range.start.node.boundsForRange(rangeStart, rangeEnd);
     if (loc)
       this.locations_.push(loc);
   },
@@ -1673,65 +1711,49 @@ 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 prevHasInlineNode = false;
+    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;
+      }
+
+      // Keep track of if there's an inline node associated with
+      // |cur|.
+      var hasInlineNode = cur.getSpansInstanceOf(Output.NodeSpan)
+          .some(function(s) {
+            return s.node && s.node.display == 'inline';
+          });
+
+      // 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 || hasInlineNode || prevHasInlineNode)

[dmazzoni, 2016/11/10 23:31:29]

I'm not sure we want no separator if either of them has an inline
node anywhere, that might be too aggressively sticking things
together with no whitespace. But I'm fine with landing this
and improving as we find such cases.

[David Tseng, 2016/11/11 02:36:27]

I found good examples of block <-> inline and inline <-> block elements that
look bad with separators. Namely, there's double spaces between them without
this code.

For example,
google chrome lnk  some more text


is what I see for an inline link and block static text.

+        separator = '';
+      else
+        separator = Output.SPACE;
+
+      prevHasInlineNode = hasInlineNode;
       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;
+      return result;
+    }, new Spannable());
   },
 
   /**
@@ -1796,7 +1818,7 @@ Output.prototype = {
    * @return {!Spannable}
    */
   get brailleOutputForTest() {
-    return this.createBrailleOutput_();
+    return this.mergeBraille_(this.brailleBuffer_);
   }
 };