Fix Views inline autocomplete with multi-char graphemes.

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index 645573865c3d075a7389a54d3c0dc5277658fdb7..0b7d9fc39deec0e56a5fc21931d0412472248c50 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -501,26 +501,28 @@ void RenderText::SetCursorPosition(size_t position) {
 void RenderText::MoveCursor(BreakType break_type,
                             VisualCursorDirection direction,
                             bool select) {
-  SelectionModel position(cursor_position(), selection_model_.caret_affinity());
+  SelectionModel cursor(cursor_position(), selection_model_.caret_affinity());
   // Cancelling a selection moves to the edge of the selection.
   if (break_type != LINE_BREAK && !selection().is_empty() && !select) {
     SelectionModel selection_start = GetSelectionModelForSelectionStart();
     int start_x = GetCursorBounds(selection_start, true).x();
-    int cursor_x = GetCursorBounds(position, true).x();
+    int cursor_x = GetCursorBounds(cursor, true).x();
     // Use the selection start if it is left (when |direction| is CURSOR_LEFT)
     // or right (when |direction| is CURSOR_RIGHT) of the selection end.
     if (direction == CURSOR_RIGHT ? start_x > cursor_x : start_x < cursor_x)
-      position = selection_start;
-    // For word breaks, use the nearest word boundary in the appropriate
-    // |direction|.
+      cursor = selection_start;
+    // Use the nearest word boundary in the proper |direction| for word breaks.
     if (break_type == WORD_BREAK)
-      position = GetAdjacentSelectionModel(position, break_type, direction);
+      cursor = GetAdjacentSelectionModel(cursor, break_type, direction);
+    // Use an adjacent selection model if the cursor is not at a valid position.
+    if (!IsValidCursorIndex(cursor.caret_pos()))
+      cursor = GetAdjacentSelectionModel(cursor, CHARACTER_BREAK, direction);
   } else {
-    position = GetAdjacentSelectionModel(position, break_type, direction);
+    cursor = GetAdjacentSelectionModel(cursor, break_type, direction);
   }
   if (select)
-    position.set_selection_start(selection().start());
-  MoveCursorTo(position);
+    cursor.set_selection_start(selection().start());
+  MoveCursorTo(cursor);
 }
 
 bool RenderText::MoveCursorTo(const SelectionModel& model) {
@@ -528,9 +530,8 @@ bool RenderText::MoveCursorTo(const SelectionModel& model) {
   size_t text_length = text().length();
   Range range(std::min(model.selection().start(), text_length),
               std::min(model.caret_pos(), text_length));
-  // The current model only supports caret positions at valid character indices.
-  if (!IsCursorablePosition(range.start()) ||
-      !IsCursorablePosition(range.end()))
+  // The current model only supports caret positions at valid cursor indices.
+  if (!IsValidCursorIndex(range.start()) || !IsValidCursorIndex(range.end()))
     return false;
   SelectionModel sel(range, model.caret_affinity());
   bool changed = sel != selection_model_;
@@ -548,7 +549,8 @@ bool RenderText::MoveCursorTo(const Point& point, bool select) {
 bool RenderText::SelectRange(const Range& range) {
   Range sel(std::min(range.start(), text().length()),
             std::min(range.end(), text().length()));
-  if (!IsCursorablePosition(sel.start()) || !IsCursorablePosition(sel.end()))
+  // Allow selection bounds at valid indicies amid multi-character graphemes.
+  if (!IsValidLogicalIndex(sel.start()) || !IsValidLogicalIndex(sel.end()))
     return false;
   LogicalCursorDirection affinity =
       (sel.is_reversed() || sel.is_empty()) ? CURSOR_FORWARD : CURSOR_BACKWARD;
@@ -767,6 +769,15 @@ void RenderText::DrawCursor(Canvas* canvas, const SelectionModel& position) {
   canvas->FillRect(GetCursorBounds(position, true), cursor_color_);
 }
 
+bool RenderText::IsValidLogicalIndex(size_t index) {
+  // Check that the index is at a valid code point (not mid-surrgate-pair) and
+  // that it's not truncated from the layout text (its glyph may be shown).
+  return index == 0 || index == text().length() ||
+      (index <= text().length() &&

[Alexei Svitkine (slow), 2014/05/01 20:48:09]

Nit: Since you check for == above, change <= to < to make it clearer.

[msw, 2014/05/01 22:43:22]

Done.

+       (truncate_length_ == 0 || index < truncate_length_) &&

[Alexei Svitkine (slow), 2014/05/01 20:48:09]

It seems wrong to me to accept text().length() but reject e.g. text().length() -
1 if text().length() is > truncate_length_. Should the truncate check be done
before the index == text().length() check?

[msw, 2014/05/01 22:43:22]

That case isn't actually wrong. We disallow indices between the truncation
length and the underlying string length so that users can easily interact with
the underlying truncated text using the ellipsis as a proxy. So, users can
select all text, select the truncated text, and cursor from the last rendered
cursor position before truncation to the end of the string, all without setting
cursor positions to invisible indices or requiring unbounded arrow key presses
to traverse the ellipsis. (i.e. cursoring over the ellipsis that indicates
truncation transitions from the last visible position to the last logical
position). I tested this manually a good deal when I landed r209286.

[Alexei Svitkine (slow), 2014/05/01 22:53:39]

That makes a lot of sense. Can you incorporate this explanation into the
comment?

[msw, 2014/05/01 23:04:35]

Done.

+       IsValidCodePointIndex(text(), index));
+}
+
 Rect RenderText::GetCursorBounds(const SelectionModel& caret,
                                  bool insert_mode) {
   // TODO(ckocagil): Support multiline. This function should return the height
@@ -774,9 +785,8 @@ Rect RenderText::GetCursorBounds(const SelectionModel& caret,
   //                 the multiline size, eliminate its use here.
 
   EnsureLayout();
-
   size_t caret_pos = caret.caret_pos();
-  DCHECK(IsCursorablePosition(caret_pos));
+  DCHECK(IsValidLogicalIndex(caret_pos));
   // In overtype mode, ignore the affinity and always indicate that we will
   // overtype the next character.
   LogicalCursorDirection caret_affinity =
@@ -817,7 +827,7 @@ size_t RenderText::IndexOfAdjacentGrapheme(size_t index,
   if (direction == CURSOR_FORWARD) {
     while (index < text().length()) {
       index++;
-      if (IsCursorablePosition(index))
+      if (IsValidCursorIndex(index))
         return index;
     }
     return text().length();
@@ -825,7 +835,7 @@ size_t RenderText::IndexOfAdjacentGrapheme(size_t index,
 
   while (index > 0) {
     index--;
-    if (IsCursorablePosition(index))
+    if (IsValidCursorIndex(index))
       return index;
   }
   return 0;
@@ -1108,7 +1118,7 @@ bool RenderText::RangeContainsCaret(const Range& range,
 
 void RenderText::MoveCursorTo(size_t position, bool select) {
   size_t cursor = std::min(position, text().length());
-  if (IsCursorablePosition(cursor))
+  if (IsValidCursorIndex(cursor))
     SetSelectionModel(SelectionModel(
         Range(select ? selection().start() : cursor, cursor),
         (cursor == 0) ? CURSOR_FORWARD : CURSOR_BACKWARD));