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

content/browser/accessibility/browser_accessibility.cc

 // Copyright (c) 2012 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.
 
 #include "content/browser/accessibility/browser_accessibility.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 293 matching lines @@
 
   // Adjust the bounds by the top left corner of the containing view's bounds
   // in screen coordinates.
   bounds.Offset(manager_->GetViewBounds().OffsetFromOrigin());
 
   return bounds;
 }
 
 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)
+  DCHECK_GE(start, -1);
+  int unknown_offset = GetStaticTextLenRecursive();
+  int word_start = unknown_offset;
+
+  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;
+        }
+
+        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;
-      start -= child_len;
-    }
-    return word_start;
-  }
+        }
 
-  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.
-
-    const std::vector<int32>& word_starts = child->GetIntListAttribute(
-        ui::AX_ATTR_WORD_STARTS);
-    if (word_starts.empty()) {
-      word_start = child_end;
-      continue;
+        // 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 {
+          NOTREACHED();
+        }
+      }
+      return word_start;
     }
 
-    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);
+    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;
         }
-      } else {
-        NOTREACHED();
+
+        // 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;
       }
-      break;
-    }
-
-    // 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();
-    }
+      return word_start;
   }
-
-  return word_start;
 }
 
 BrowserAccessibility* BrowserAccessibility::BrowserAccessibilityForPoint(
     const gfx::Point& point) {
   // The best result found that's a child of this object.
   BrowserAccessibility* child_result = NULL;
   // The best result that's an indirect descendant like grandchild, etc.
   BrowserAccessibility* descendant_result = NULL;
 
   // Walk the children recursively looking for the BrowserAccessibility that
@@ skipping 326 matching lines @@
     return false;
 
   BrowserAccessibility* grandparent = parent->GetParent();
   if (!grandparent)
     return false;
 
   return grandparent->GetRole() == ui::AX_ROLE_IFRAME_PRESENTATIONAL;
 }
 
 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)
     len += InternalGetChild(i)->GetStaticTextLenRecursive();
   return len;
 }
 
 BrowserAccessibility* BrowserAccessibility::GetParentForBoundsCalculation()
     const {
   if (!node_ || !manager_)
@@ skipping 42 matching lines @@
       }
       need_to_offset_web_area = true;
     }
     parent = parent->GetParentForBoundsCalculation();
   }
 
   return bounds;
 }
 
 }  // namespace content