Removes any dependency on inlineTextBoxes state.

Previously, we tracked inlineTextBox nodes using their index in the
staticText parent. However, if the container holding the text was
resized, the staticText parent indices change as the flow of boxes
within them changes. This change removes all dependency on the
index of the inlineTextBox within the staticText node, and instead
relies purely on character indices to determine nodes to highlight.

Bug: 800912
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I0c0ecb2f78912b376040cc029f77a32fec6f5c85
Reviewed-on: https://chromium-review.googlesource.com/898206
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534194}

chrome/browser/chromeos/accessibility/select_to_speak_browsertest.cc

@@ -16,11 +16,15 @@
 #include "chrome/browser/chromeos/accessibility/speech_monitor.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/test/browser_test_utils.h"
+#include "extensions/browser/extension_host.h"
 #include "extensions/browser/notification_types.h"
+#include "extensions/browser/process_manager.h"
 #include "ui/events/test/event_generator.h"
 #include "url/url_constants.h"
 
@@ -84,6 +88,23 @@ class SelectToSpeakTest : public InProcessBrowserTest {
     return weak_ptr_factory_.GetWeakPtr();
   }
 
+  void RunJavaScriptInSelectToSpeakBackgroundPage(const std::string& script) {
+    extensions::ExtensionHost* host =
+        extensions::ProcessManager::Get(browser()->profile())
+            ->GetBackgroundHostForExtension(
+                extension_misc::kSelectToSpeakExtensionId);
+    CHECK(content::ExecuteScript(host->host_contents(), script));
+  }
+
+  content::WebContents* GetWebContents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  void ExecuteJavaScriptInForeground(const std::string& script) {
+    CHECK(
+        content::ExecuteScript(GetWebContents()->GetRenderViewHost(), script));
+  }
+
  private:
   scoped_refptr<content::MessageLoopRunner> loop_runner_;
   base::WeakPtrFactory<SelectToSpeakTest> weak_ptr_factory_;
@@ -108,7 +129,7 @@ IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, SpeakStatusTray) {
 IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, SmoothlyReadsAcrossInlineUrl) {
   // Make sure an inline URL is read smoothly.
   ActivateSelectToSpeakInWindowBounds(
-      "data:text/html;charset=utf-8,<p>This is some text <a href=>with a"
+      "data:text/html;charset=utf-8,<p>This is some text <a href=\"\">with a"
       " node</a> in the middle");
   // Should combine nodes in a paragraph into one utterance.
   // Includes some wildcards between words because there may be extra
@@ -147,6 +168,15 @@ IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, SmoothlyReadsAcrossFormattedText) {
                          "This is some text*with a node*in the middle*"));
 }
 
+IN_PROC_BROWSER_TEST_F(SelectToSpeakTest,
+                       ReadsStaticTextWithoutInlineTextChildren) {
+  // Bold or formatted text
+  ActivateSelectToSpeakInWindowBounds(
+      "data:text/html;charset=utf-8,<canvas>This is some text</canvas>");
+  EXPECT_TRUE(base::MatchPattern(speech_monitor_.GetNextUtterance(),
+                                 "This is some text*"));
+}
+
 IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, BreaksAtParagraphBounds) {
   ActivateSelectToSpeakInWindowBounds(
       "data:text/html;charset=utf-8,<div><p>First paragraph</p>"
@@ -226,4 +256,22 @@ IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, FocusRingMovesWithMouse) {
   EXPECT_EQ(focus_rings.size(), 0u);
 }
 
+IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, ContinuesReadingDuringResize) {
+  ActivateSelectToSpeakInWindowBounds(
+      "data:text/html;charset=utf-8,<p>First paragraph</p>"
+      "<div id='resize' style='width:300px; font-size: 10em'>"
+      "<p>Second paragraph is longer than 300 pixels and will wrap when "
+      "resized</p></div>");
+
+  EXPECT_TRUE(base::MatchPattern(speech_monitor_.GetNextUtterance(),
+                                 "First paragraph*"));
+
+  // Resize before second is spoken. If resizing caused errors finding the
+  // inlineTextBoxes in the node, speech would be stopped early.
+  ExecuteJavaScriptInForeground(
+      "document.getElementById('resize').style.width='100px'");
+  EXPECT_TRUE(
+      base::MatchPattern(speech_monitor_.GetNextUtterance(), "*when*resized*"));
+}
+
 }  // namespace chromeos

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

@@ -4,7 +4,6 @@
 
 var AutomationNode = chrome.automation.AutomationNode;
 var RoleType = chrome.automation.RoleType;
-const NO_INLINE_TEXT_BOX = -1;
 
 /**
  * Gets the first ancestor of a node which is a paragraph or is not inline,
@@ -72,6 +71,50 @@ function isWhitespace(name) {
   return re.exec(name) != null;
 }
 
+/**
+ * Determines the index into the parent name at which the inlineTextBox
+ * node name begins.
+ * @param {AutomationNode} inlineTextNode An inlineTextBox type node.
+ * @return {number} The character index into the parent node at which
+ *     this node begins.
+ */
+function getStartCharIndexInParent(inlineTextNode) {
+  let result = 0;
+  for (let i = 0; i < inlineTextNode.indexInParent; i++) {
+    result += inlineTextNode.parent.children[i].name.length;
+  }
+  return result;
+}
+
+/**
+ * Determines the inlineTextBox child of a staticText node that appears
+ * at the given character index into the name of the staticText node. Uses
+ * the inlineTextBoxes name length to determine position. For example, if
+ * a staticText has name "abc 123" and two children with names "abc " and
+ * "123", indexes 0-3 would return the first child and indexes 4+ would
+ * return the second child.
+ * @param {AutomationNode} staticTextNode The staticText node to search.
+ * @param {number} index The index into the staticTextNode's name.
+ * @return {?AutomationNode} The inlineTextBox node within the staticText
+ *    node that appears at this index into the staticText node's name, or
+ *    the last inlineTextBox in the staticText node if the index is too
+ *    large.
+ */
+function findInlineTextNodeByCharacterIndex(staticTextNode, index) {
+  if (staticTextNode.children.length == 0) {
+    return null;
+  }
+  let textLength = 0;
+  for (var i = 0; i < staticTextNode.children.length; i++) {
+    let node = staticTextNode.children[i];
+    if (node.name.length + textLength > index) {
+      return node;
+    }
+    textLength += node.name.length;
+  }
+  return staticTextNode.children[staticTextNode.children.length - 1];
+}
+
 /**
  * Builds information about nodes in a group until it reaches the end of the
  * group. It may return a NodeGroup with a single node, or a large group
@@ -83,7 +126,8 @@ function isWhitespace(name) {
 function buildNodeGroup(nodes, index) {
   let node = nodes[index];
   let next = nodes[index + 1];
-  let result = new NodeGroup(index, getFirstBlockAncestor(nodes[index]));
+  let result = new NodeGroup(getFirstBlockAncestor(nodes[index]));
+  let staticTextParent = null;
   // TODO: Don't skip nodes. Instead, go through every node in
   // this paragraph from the first to the last in the nodes list.
   // This will catch nodes at the edges of the user's selection like
@@ -93,13 +137,33 @@ function buildNodeGroup(nodes, index) {
   // a text type node, continue building the paragraph.
   while (index < nodes.length) {
     if (node.name !== undefined && !isWhitespace(node.name)) {
+      let newNode;
       if (node.role == RoleType.INLINE_TEXT_BOX && node.parent !== undefined) {
-        result.nodes.push(new NodeGroupItem(
-            node.parent, result.text.length, node.indexInParent));
+        if (node.parent.role == RoleType.STATIC_TEXT) {
+          // This is an inlineTextBox node with a staticText parent. If that
+          // parent is already added to the result, we can skip. This adds
+          // each parent only exactly once.
+          if (staticTextParent && staticTextParent.node !== node.parent) {
+            // We are on a new staticText. Make a new parent to add to.
+            staticTextParent = null;
+          }
+          if (staticTextParent === null) {
+            staticTextParent =
+                new NodeGroupItem(node.parent, result.text.length, true);
+            newNode = staticTextParent;
+          }
+        } else {
+          // Not an staticText parent node. Add it directly.
+          newNode = new NodeGroupItem(node, result.text.length, false);
+        }
       } else {
-        result.nodes.push(new NodeGroupItem(node, result.text.length));
+        // Not an inlineTextBox node. Add it directly.
+        newNode = new NodeGroupItem(node, result.text.length, false);
+      }
+      if (newNode) {
+        result.text += newNode.node.name + ' ';
+        result.nodes.push(newNode);
       }
-      result.text += node.name + ' ';
     }
     if (!inSameParagraph(node, next)) {
       break;
@@ -116,12 +180,11 @@ function buildNodeGroup(nodes, index) {
  * Class representing a node group, which may be a single node or a
  * full paragraph of nodes.
  *
- * @param {number} startIndex The index of the first node within
  * @param {?AutomationNode} blockParent The first block ancestor of
  *     this group. This may be the paragraph parent, for example.
  * @constructor
  */
-function NodeGroup(startIndex, blockParent) {
+function NodeGroup(blockParent) {
   /**
    * Full text of this paragraph.
    * @type {string}
@@ -140,16 +203,12 @@ function NodeGroup(startIndex, blockParent) {
    */
   this.blockParent = blockParent;
 
-  /**
-   * The index of the first node in this paragraph from the list of
-   * nodes originally selected by the user.
-   * @type {number}
-   */
-  this.startIndex = startIndex;
-
   /**
    * The index of the last node in this paragraph from the list of
    * nodes originally selected by the user.
+   * Note that this may not be stable over time, because nodes may
+   * come and go from the automation tree. This should not be used
+   * in any callbacks / asynchronously.
    * @type {number}
    */
   this.endIndex = -1;
@@ -164,12 +223,11 @@ function NodeGroup(startIndex, blockParent) {
  * @param {AutomationNode} node The AutomationNode associated with this item
  * @param {number} startChar The index into the NodeGroup's text string where
  *     this item begins.
- * @param {number=} opt_inlineTextBoxIndex If the node role is staticText,
- *     this represents the index into the inlineTextBox children of that
- *     node.
+ * @param {boolean=} opt_hasInlineText If this NodeGroupItem has inlineText
+ *     children.
  * @constructor
  */
-function NodeGroupItem(node, startChar, opt_inlineTextBoxIndex) {
+function NodeGroupItem(node, startChar, opt_hasInlineText) {
   /**
    * @type {AutomationNode}
    */
@@ -183,22 +241,11 @@ function NodeGroupItem(node, startChar, opt_inlineTextBoxIndex) {
   this.startChar = startChar;
 
   /**
-   * If this is a staticText node, this is ihe index into the inlineTextBox
-   * children which is represented by this node. We cannot store
-   * inlineTextBox children directly because they are not guarenteed to be
-   * stable. Instead, indexing into their children allows for stability.
-   * @type {number}
-   */
-  this.inlineTextBoxIndex = opt_inlineTextBoxIndex !== undefined ?
-      opt_inlineTextBoxIndex :
-      NO_INLINE_TEXT_BOX;
-
-  /**
-   * Whether this NodeGroupItem is better represented by an inlineTextBox
-   * at the index of inlineTextBoxIndex rather than the node directly.
-   * @return {boolean} True if there are inlineTextBox children.
+   * If this is a staticText node which has inlineTextBox children which should
+   * be selected. We cannot select the inlineTextBox children directly because
+   * they are not guarenteed to be stable.
+   * @type {boolean}
    */
-  this.hasInlineTextChildren = function() {
-    return this.inlineTextBoxIndex != NO_INLINE_TEXT_BOX;
-  };
+  this.hasInlineText =
+      opt_hasInlineText !== undefined ? opt_hasInlineText : false;
 }

chrome/browser/resources/chromeos/select_to_speak/paragraph_utils_unittest.gtestjs

@@ -72,6 +72,38 @@ TEST_F('SelectToSpeakParagraphUnitTest', 'IsWhitespace', function() {
   assertFalse(isWhitespace(' cats '));
 });
 
+TEST_F('SelectToSpeakParagraphUnitTest', 'GetStartCharIndexInParent',
+    function() {
+  let staticText = {role: 'staticText', name: 'My name is Bond, James Bond'};
+  let inline1 = {role: 'inlineTextBox', name: 'My name is ', indexInParent: 0,
+                 parent: staticText};
+  let inline2 = {role: 'inlineTextBox', name: 'Bond, ', indexInParent: 1,
+                 parent: staticText};
+  let inline3 = {role: 'inlineTextBox', name: 'James Bond', indexInParent: 2,
+                 parent: staticText};
+  staticText.children = [inline1, inline2, inline3];
+  assertEquals(getStartCharIndexInParent(inline1), 0);
+  assertEquals(getStartCharIndexInParent(inline2), 11);
+  assertEquals(getStartCharIndexInParent(inline3), 17);
+});
+
+TEST_F('SelectToSpeakParagraphUnitTest', 'FindInlineTextNodeByCharIndex',
+    function() {
+  let staticText = {role: 'staticText', name: 'My name is Bond, James Bond'};
+  let inline1 = {role: 'inlineTextBox', name: 'My name is '};
+  let inline2 = {role: 'inlineTextBox', name: 'Bond, '};
+  let inline3 = {role: 'inlineTextBox', name: 'James Bond'};
+  staticText.children = [inline1, inline2, inline3];
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 0), inline1);
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 10), inline1);
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 11), inline2);
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 16), inline2);
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 17), inline3);
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 50), inline3);
+  staticText.children = [];
+  assertEquals(findInlineTextNodeByCharacterIndex(staticText, 10), null);
+});
+
 TEST_F('SelectToSpeakParagraphUnitTest', 'BuildNodeGroupStopsAtNewParagraph',
     function() {
   let root = {role: 'rootWebArea'};

chrome/browser/resources/chromeos/select_to_speak/select_to_speak_keystroke_selection_test.extjs

@@ -176,7 +176,8 @@ TEST_F('SelectToSpeakKeystrokeSelectionTest',
     });
   };
   setFocusCallback = this.newCallback(setFocusCallback);
-  this.runWithLoadedTree('data:text/html;charset=utf-8,<br/><p>Selected text</p></br>',
+  this.runWithLoadedTree('data:text/html;charset=utf-8,' +
+                         '<br/><p>Selected text</p><br/>',
     function(desktop) {
       // Add an event listener that will start the user interaction
       // of the test once the selection is completed.
@@ -207,4 +208,4 @@ TEST_F('SelectToSpeakKeystrokeSelectionTest',
             focusObject: lastNode, focusOffset: 5
         });
       }, 'This is some bold text');
-});
+});
\ No newline at end of file

chrome/browser/resources/chromeos/select_to_speak/select_to_speak_unittest.gtestjs

@@ -17,6 +17,7 @@ SelectToSpeakUnitTest.prototype = {
   /** @override */
   extraLibraries: [
     'test_support.js',
+    'paragraph_utils.js',
     'select_to_speak.js'
   ]
 };
@@ -124,7 +125,9 @@ TEST_F('SelectToSpeakUnitTest', 'getNextWordEndWithoutWordEnds', function() {
 });
 
 TEST_F('SelectToSpeakUnitTest', 'getNextWordStart', function() {
-  let node = {node: {wordStarts: [0, 6]}, startChar: 0};
+  let inlineText = {wordStarts: [0, 6], name: 'kitty cat'};
+  let staticText = {children: [inlineText], name: 'kitty cat'};
+  let node = {node: staticText, startChar: 0, hasInlineText: true};
   assertEquals(0, getNextWordStart('kitty cat', 0, node));
   assertEquals(6, getNextWordStart('kitty cat', 5, node));
   assertEquals(6, getNextWordStart('kitty cat', 6, node));
@@ -132,17 +135,28 @@ TEST_F('SelectToSpeakUnitTest', 'getNextWordStart', function() {
   node.startChar = 10;
   assertEquals(10, getNextWordStart('once upon kitty cat', 9, node));
   assertEquals(16, getNextWordStart('once upon kitty cat', 15, node));
+
+  // Should return the default if the inlineText children are missing.
+  staticText.children = [];
+  assertEquals(10, getNextWordStart('once upon a kitty cat', 10, node));
 });
 
 TEST_F('SelectToSpeakUnitTest', 'getNextWordEnd', function() {
-  let node = {node: {wordEnds: [5, 9]}, startChar: 0};
+  let inlineText = {wordEnds: [5, 9], name: 'kitty cat'};
+  let staticText = {children: [inlineText], name: 'kitty cat'};
+  let node = {node: staticText, startChar: 0, hasInlineText: true};
   assertEquals(5, getNextWordEnd('kitty cat', 0, node));
-  assertEquals(5, getNextWordEnd('kitty cat', 5, node));
+  assertEquals(5, getNextWordEnd('kitty cat', 4, node));
+  assertEquals(9, getNextWordEnd('kitty cat', 5, node));
   assertEquals(9, getNextWordEnd('kitty cat', 6, node));
 
   node.startChar = 10;
   assertEquals(15, getNextWordEnd('once upon kitty cat', 9, node));
   assertEquals(19, getNextWordEnd('once upon kitty cat', 17, node));
+
+  // Should return the default if the inlineText children are missing.
+  staticText.children = [];
+  assertEquals(5, getNextWordEnd('kitty cat', 4, node));
 });
 
 TEST_F('SelectToSpeakUnitTest', 'findAllMatching', function() {