Refine braille output

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview Provides output services for ChromeVox.
  */
 
 goog.provide('Output');
 goog.provide('Output.EventType');
@@ skipping 632 matching lines @@
   /** @override */
   run: function() {
     cvox.ChromeVox.earcons.playEarcon(cvox.Earcon[this.earconId]);
   }
 };
 
 /**
  * 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;
 };
 
 /**
  * Wrapper for automation nodes as annotations.  Since the
  * {@code AutomationNode} constructor isn't exposed in the API, this class is
  * used to allow instanceof checks on these annotations.
  * @param {!AutomationNode} node
  * @constructor
  */
 Output.NodeSpan = function(node) {
@@ skipping 194 matching lines @@
    * @param {string} formatStr
    * @param {!AutomationNode=} opt_node An optional node to apply the
    *     formatting to.
    * @return {!Output} |this| for chaining
    */
   formatForBraille: function(formatStr, opt_node) {
     var node = opt_node || null;
 
     this.formatOptions_ = {speech: false, braille: true, auralStyle: false};
     this.format_(node, formatStr, this.brailleBuffer_);
-
     return this;
   },
 
   /**
    * Triggers callback for a speech event.
    * @param {function()} callback
    * @return {Output}
    */
   onSpeechEnd: function(callback) {
     this.speechEndCallback_ = function(opt_cleanupOnly) {
@@ skipping 39 matching lines @@
       if (i == this.speechBuffer_.length - 1)
         speechProps['endCallback'] = this.speechEndCallback_;
 
       cvox.ChromeVox.tts.speak(
           buff.toString(), queueMode, speechProps);
       queueMode = cvox.QueueMode.QUEUE;
     }
 
     // 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;
       if (selSpan) {
         var valueStart = buff.getSpanStart(selSpan);
         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);
       }
 
       var output = new cvox.NavBraille({
         text: buff,
         startIndex: startIndex,
         endIndex: endIndex
       });
 
       cvox.ChromeVox.braille.write(output);
@@ skipping 76 matching lines @@
       // Obtain the operator token.
       token = tree.value;
 
       // Set suffix options.
       var options = {};
       options.annotation = [];
       options.isUnique = token[token.length - 1] == '=';
       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);
 
       // All possible tokens based on prefix.
       if (prefix == '$') {
         if (token == 'value') {
           var text = node.value;
           if (!node.state.editable && node.name == text)
             return;
 
           var selectedText = '';
           if (text !== undefined) {
             if (node.textSelStart !== undefined) {
               options.annotation.push(new Output.SelectionSpan(
                   node.textSelStart,
                   node.textSelEnd));
 
               selectedText =
                   node.value.substring(node.textSelStart, node.textSelEnd);
             }
           }
           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);
           }
         } else if (token == 'name') {
           options.annotation.push(token);
           var earcon = node ? this.findEarcon_(node, opt_prevNode) : null;
           if (earcon)
             options.annotation.push(earcon);
           this.append_(buff, node.name, options);
         } else if (token == 'urlFilename') {
@@ skipping 409 matching lines @@
       if (enteredRoleSet[formatPrevNode.role] ||
           node.role == formatPrevNode.role ||
           localStorage['useVerboseMode'] == 'false')
         continue;
 
       var roleBlock = getMergedRoleBlock(formatPrevNode.role);
       if (roleBlock.leave && localStorage['useVerboseMode'] == 'true')
         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]);
          j--) {
       var roleBlock = getMergedRoleBlock(formatNode.role);
       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);
+        }
       }
     }
   },
 
   /**
    * @param {!AutomationNode} node
    * @param {!AutomationNode} prevNode
    * @param {EventType|Output.EventType} type
    * @param {!Array<Spannable>} buff
    * @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] || {};
     var parentRole = (Output.ROLE_INFO_[node.role] || {}).inherits;
     var parentRoleBlock = eventBlock[parentRole || ''] || {};
     var speakFormat = roleBlock.speak ||
         parentRoleBlock.speak ||
         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);
+    }
   },
 
   /**
    * @param {!cursors.Range} range
    * @param {cursors.Range} prevRange
    * @param {EventType|Output.EventType} type
    * @param {!Array<Spannable>} buff
    * @private
    */
   subNode_: function(range, prevRange, type, buff) {
     if (!prevRange)
       prevRange = range;
     var dir = cursors.Range.getDirection(prevRange, range);
     var node = range.start.node;
     var prevNode = prevRange.getBound(dir).node;
     if (!node || !prevNode)
       return;
 
     var options = {annotation: ['name'], isUnique: true};
     var startIndex = range.start.index;
     var endIndex = 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 >= 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));
       }
     }
 
     if (this.outputContextFirst_)
       this.ancestry_(node, prevNode, type, buff);
     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);
 
     var loc =
         range.start.node.boundsForRange(startIndex, endIndex);
     if (loc)
       this.locations_.push(loc);
   },
 
   /**
@@ skipping 81 matching lines @@
       index++;
     }
 
     if (currentNode != root)
       throw 'Unbalanced parenthesis: ' + inputStr;
 
     return root;
   },
 
   /**
-   * 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());
   },
 
   /**
    * Find the earcon for a given node (including ancestry).
    * @param {!AutomationNode} node
    * @param {!AutomationNode=} opt_prevNode
    * @return {Output.Action}
    */
   findEarcon_: function(node, opt_prevNode) {
     if (node === opt_prevNode)
@@ skipping 44 matching lines @@
       prev.append(cur);
       return prev;
     }, null);
   },
 
   /**
    * Gets the output buffer for braille.
    * @return {!Spannable}
    */
   get brailleOutputForTest() {
-    return this.createBrailleOutput_();
+    return this.mergeBraille_(this.brailleBuffer_);
   }
 };
 
 });  // goog.scope