Fixed word and line navigation in multi-line text fields.

content/browser/accessibility/browser_accessibility.cc

Index: content/browser/accessibility/browser_accessibility.cc
diff --git a/content/browser/accessibility/browser_accessibility.cc b/content/browser/accessibility/browser_accessibility.cc
index 16a8ed2147b8f909f96620eee26d39a6d421c293..3c5dc1d2742881c0d586d438771e5f7f6b263dce 100644
--- a/content/browser/accessibility/browser_accessibility.cc
+++ b/content/browser/accessibility/browser_accessibility.cc
@@ -311,71 +311,103 @@ gfx::Rect BrowserAccessibility::GetGlobalBoundsForRange(int start, int len)
 
 int BrowserAccessibility::GetWordStartBoundary(
     int start, ui::TextBoundaryDirection direction) const {
-  int word_start = 0;
-  int prev_word_start = 0;
-  if (GetRole() != ui::AX_ROLE_STATIC_TEXT) {
-    for (size_t i = 0; i < InternalChildCount(); ++i) {
-      BrowserAccessibility* child = InternalGetChild(i);
-      int child_len = child->GetStaticTextLenRecursive();
-      int child_word_start = child->GetWordStartBoundary(start, direction);
-      word_start += child_word_start;
-      if (child_word_start != child_len)
-          break;
-      start -= child_len;
-    }
-    return word_start;
-  }
+  DCHECK_GE(start, -1);
+  int unknown_offset = GetStaticTextLenRecursive();
+  int word_start = unknown_offset;
 
-  int child_start = 0;
-  int child_end = 0;
-  for (size_t i = 0; i < InternalChildCount(); ++i) {
-    // The next child starts where the previous one ended.
-    child_start = child_end;
-    BrowserAccessibility* child = InternalGetChild(i);
-    DCHECK_EQ(child->GetRole(), ui::AX_ROLE_INLINE_TEXT_BOX);
-    const std::string& child_text = child->GetStringAttribute(
-        ui::AX_ATTR_VALUE);
-    int child_len = static_cast<int>(child_text.size());
-    child_end += child_len; // End is one past the last character.
+  switch (GetRole()) {
+    case ui::AX_ROLE_STATIC_TEXT: {
+      int prev_word_start = unknown_offset;
+      int child_start = 0;
+      int child_end = 0;
+
+      // Go through the inline text boxes.
+      for (size_t i = 0; i < InternalChildCount(); ++i) {
+        // The next child starts where the previous one ended.
+        child_start = child_end;
+        BrowserAccessibility* child = InternalGetChild(i);
+        DCHECK_EQ(child->GetRole(), ui::AX_ROLE_INLINE_TEXT_BOX);

[dmazzoni, 2015/04/29 21:49:49]

I think there's a bug that could trip this: I just stumbled upon a place in
Blink where we return a role of static text for something other than an actual
text node, so then it's possible it has a child that's not an inline text box.
Consider doing a real test for this role with a warning, not a debug check:

if (child->GetRole() != ui::AX_ROLE_INLINE_TEXT_BOX) {
  LOG(WARNING) << "..."
  continue;
}

+        const std::string& child_text = child->GetStringAttribute(
+            ui::AX_ATTR_VALUE);
+        int child_len = static_cast<int>(child_text.size());
+        child_end += child_len; // End is one past the last character.
+
+        const std::vector<int32>& word_starts = child->GetIntListAttribute(
+            ui::AX_ATTR_WORD_STARTS);
+        if (word_starts.empty()) {
+          word_start = child_end;
+          continue;
+        }
 
-    const std::vector<int32>& word_starts = child->GetIntListAttribute(
-        ui::AX_ATTR_WORD_STARTS);
-    if (word_starts.empty()) {
-      word_start = child_end;
-      continue;
-    }
+        int local_start = start - child_start;
+        std::vector<int32>::const_iterator iter = std::upper_bound(
+            word_starts.begin(), word_starts.end(), local_start);
+        if (iter != word_starts.end()) {
+          if (direction == ui::FORWARDS_DIRECTION) {
+            word_start = child_start + *iter;
+          } else if (direction == ui::BACKWARDS_DIRECTION) {
+            if (iter == word_starts.begin()) {
+              // Return the position of the last word in the previous child.
+              word_start = prev_word_start;
+            } else {
+              word_start = child_start + *(iter - 1);

[dmazzoni, 2015/04/29 21:49:50]

Is this safe if iter points to the first element in word_starts?

+            }
+          } else {
+            NOTREACHED();
+          }
+          break;
+        }
 
-    int local_start = start - child_start;
-    std::vector<int32>::const_iterator iter = std::upper_bound(
-        word_starts.begin(), word_starts.end(), local_start);
-    if (iter != word_starts.end()) {
-      if (direction == ui::FORWARDS_DIRECTION) {
-        word_start = child_start + *iter;
-      } else if (direction == ui::BACKWARDS_DIRECTION) {
-        if (iter == word_starts.begin()) {
-          // Return the position of the last word in the previous child.
+        // No word start that is greater than the requested offset has been
+        // found.
+        prev_word_start = child_start + *(iter - 1);
+        if (direction == ui::FORWARDS_DIRECTION) {
+          word_start = child_end;
+        } else if (direction == ui::BACKWARDS_DIRECTION) {
           word_start = prev_word_start;
         } else {
-          word_start = child_start + *(iter - 1);
+          NOTREACHED();
         }
-      } else {
-        NOTREACHED();
       }
-      break;
+      return word_start;
     }
 
-    // No word start that is >= to the requested offset has been found.
-    prev_word_start = child_start + *(iter - 1);
-    if (direction == ui::FORWARDS_DIRECTION) {
-      word_start = child_end;
-    } else if (direction == ui::BACKWARDS_DIRECTION) {
-      word_start = prev_word_start;
-    } else {
-      NOTREACHED();
-    }
-  }
+    case ui::AX_ROLE_LINE_BREAK:
+      // Words never start at a line break.
+      return unknown_offset;

[dmazzoni, 2015/04/29 21:49:49]

I think it'd be more clear to use something like "end_of_text_length" here
rather than unknown_offset. You could define two variables at the top of the
function since they have somewhat different purposes even though they happen to
be the same value.

+
+    default:
+      // If there are no children, the word start boundary is still unknown or
+      // found previously depending on the direction.
+      if (!InternalChildCount())
+        return unknown_offset;

[dmazzoni, 2015/04/29 21:49:49]

What happens if you have an element with a lot of static text children but no
words? Does it make sense to return the end offset like this?

+
+      int child_start = 0;
+      for (size_t i = 0; i < InternalChildCount(); ++i) {
+        BrowserAccessibility* child = InternalGetChild(i);
+        int child_len = child->GetStaticTextLenRecursive();
+        int child_word_start = child->GetWordStartBoundary(start, direction);
+        if (child_word_start < child_len) {
+          // We have found a possible word boundary.
+          word_start = child_start + child_word_start;
+        }
 
-  return word_start;
+        // Decide when to stop searching.
+         if ((word_start != unknown_offset &&

[dmazzoni, 2015/04/29 21:49:50]

nit: indentation

+             direction == ui::FORWARDS_DIRECTION) ||
+             (start < child_len &&
+             direction == ui::BACKWARDS_DIRECTION)) {
+           break;
+         }
+
+        child_start += child_len;
+        if (start >= child_len)
+          start -= child_len;
+        else
+          start = -1;
+      }
+      return word_start;
+  }
 }
 
 BrowserAccessibility* BrowserAccessibility::BrowserAccessibilityForPoint(
@@ -722,8 +754,10 @@ bool BrowserAccessibility::IsWebAreaForPresentationalIframe() const {
 }
 
 int BrowserAccessibility::GetStaticTextLenRecursive() const {
-  if (GetRole() == ui::AX_ROLE_STATIC_TEXT)
+  if (GetRole() == ui::AX_ROLE_STATIC_TEXT ||
+      GetRole() == ui::AX_ROLE_LINE_BREAK) {
     return static_cast<int>(GetStringAttribute(ui::AX_ATTR_VALUE).size());
+  }
 
   int len = 0;
   for (size_t i = 0; i < InternalChildCount(); ++i)