Simplify handling of BiDi cursor movement

ui/gfx/render_text.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 "ui/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/debug/trace_event.h"
 #include "base/i18n/break_iterator.h"
@@ skipping 314 matching lines @@
     // range ends beyond text_.length, it must be the last one.
     style_ranges_.back().range.set_end(text_.length());
   }
 #ifndef NDEBUG
   CheckStyleRanges(style_ranges_, text_.length());
 #endif
   cached_bounds_and_offset_valid_ = false;
 
   // Reset selection model. SetText should always followed by SetSelectionModel
   // or SetCursorPosition in upper layer.
-  SetSelectionModel(SelectionModel(0, 0, SelectionModel::LEADING));
+  SetSelectionModel(SelectionModel());
 
   UpdateLayout();
 }
 
 void RenderText::SetHorizontalAlignment(HorizontalAlignment alignment) {
   if (horizontal_alignment_ != alignment) {
     horizontal_alignment_ = alignment;
     display_offset_ = Point();
     cached_bounds_and_offset_valid_ = false;
   }
@@ skipping 24 matching lines @@
   insert_mode_ = !insert_mode_;
   cached_bounds_and_offset_valid_ = false;
 }
 
 void RenderText::SetDisplayRect(const Rect& r) {
   display_rect_ = r;
   cached_bounds_and_offset_valid_ = false;
   UpdateLayout();
 }
 
-size_t RenderText::GetCursorPosition() const {
-  return selection_model_.selection_end();
-}
-
 void RenderText::SetCursorPosition(size_t position) {
   MoveCursorTo(position, false);
 }
 
 void RenderText::MoveCursor(BreakType break_type,
                             VisualCursorDirection direction,
                             bool select) {
-  SelectionModel position(selection_model());
-  position.set_selection_start(GetCursorPosition());
+  SelectionModel position(cursor_position(), selection_model_.caret_affinity());
   // Cancelling a selection moves to the edge of the selection.
-  if (break_type != LINE_BREAK && !EmptySelection() && !select) {
+  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();
     // 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|.
     if (break_type == WORD_BREAK)
       position = GetAdjacentSelectionModel(position, break_type, direction);
   } else {
     position = GetAdjacentSelectionModel(position, break_type, direction);
   }
   if (select)
-    position.set_selection_start(GetSelectionStart());
+    position.set_selection_start(selection().start());
   MoveCursorTo(position);
 }
 
 bool RenderText::MoveCursorTo(const SelectionModel& model) {
-  SelectionModel sel(model);
+  // Enforce valid selection model components.
   size_t text_length = text().length();
-  // Enforce valid selection model components.
-  if (sel.selection_start() > text_length)
-    sel.set_selection_start(text_length);
-  if (sel.selection_end() > text_length)
-    sel.set_selection_end(text_length);
+  ui::Range range(std::min(model.selection().start(), text_length),

[msw, 2012/02/16 00:09:21]

nit: move range down to where it's used.

+                  std::min(model.caret_pos(), text_length));
+  LogicalCursorDirection affinity = model.caret_affinity();
+  if (model.caret_pos() > text_length)

[xji, 2012/02/16 02:38:29]

are "{" and "}" needed here for style correct-ness?

+    // Make the cursor appear at the edge of the text, not next to the final

[msw, 2012/02/16 00:09:21]

nit: Wrap multi-line if blocks in braces, even for comments.

+    // grapheme.
+    affinity = CURSOR_FORWARD;
+
   // The current model only supports caret positions at valid character indices.
-  if (text_length == 0) {
-    sel.set_caret_pos(0);
-    sel.set_caret_placement(SelectionModel::LEADING);
-  } else if (sel.caret_pos() >= text_length) {
-    SelectionModel end_selection =
-        EdgeSelectionModel(GetVisualDirectionOfLogicalEnd());
-    sel.set_caret_pos(end_selection.caret_pos());
-    sel.set_caret_placement(end_selection.caret_placement());
-  }
-
-  if (!IsCursorablePosition(sel.selection_start()) ||
-      !IsCursorablePosition(sel.selection_end()) ||
-      !IsCursorablePosition(sel.caret_pos()))
+  if (!IsCursorablePosition(range.start()) ||
+      !IsCursorablePosition(range.end()))
     return false;
 
-  bool changed = !sel.Equals(selection_model_);
+  SelectionModel sel(range, affinity);
+  bool changed = sel != selection_model_;
   SetSelectionModel(sel);
   return changed;
 }
 
 bool RenderText::MoveCursorTo(const Point& point, bool select) {
-  SelectionModel selection = FindCursorPosition(point);
+  SelectionModel position = FindCursorPosition(point);
   if (select)
-    selection.set_selection_start(GetSelectionStart());
-  return MoveCursorTo(selection);
+    position.set_selection_start(selection().start());
+  return MoveCursorTo(position);
 }
 
 bool RenderText::SelectRange(const ui::Range& range) {
-  size_t text_length = text().length();
-  size_t start = std::min(range.start(), text_length);
-  size_t end = std::min(range.end(), text_length);
-
-  if (!IsCursorablePosition(start) || !IsCursorablePosition(end))
+  ui::Range sel(std::min(range.start(), text().length()),
+                std::min(range.end(), text().length()));
+  if (!IsCursorablePosition(sel.start()) || !IsCursorablePosition(sel.end()))
     return false;
-
-  size_t pos = end;
-  SelectionModel::CaretPlacement placement = SelectionModel::LEADING;
-  if (start < end) {
-    pos = IndexOfAdjacentGrapheme(end, CURSOR_BACKWARD);
-    DCHECK_LT(pos, end);
-    placement = SelectionModel::TRAILING;
-  } else if (end == text_length) {
-    SelectionModel end_selection =
-        EdgeSelectionModel(GetVisualDirectionOfLogicalEnd());
-    pos = end_selection.caret_pos();
-    placement = end_selection.caret_placement();
-  }
-  SetSelectionModel(SelectionModel(start, end, pos, placement));
+  LogicalCursorDirection affinity =
+      (sel.is_reversed() || (sel.is_empty() && sel.start() != 0)) ?

[xji, 2012/02/16 02:38:29]

why sel.start() == 0 is handled differently from sel.start() != 0 when
sel.is_empty()?

[benrg, 2012/02/16 09:20:55]

The original code attached the cursor to the last character in the selection if
the selection was nonempty, to the visual end if the selection was
Range(text_length, text_length), and to the logically following character
otherwise. The new code replicates this behavior except that it attaches the
cursor to the visual beginning when the selection is Range(0, 0), because I
wanted to treat the two ends of the text symmetrically.

Fundamentally the problem is that the argument to this method doesn't provide
enough information, forcing us to guess the rest. The solution is probably to
replace it everywhere with MoveCursorTo or another method that takes a
SelectionModel. I started to do this, but it touched a lot of code and this CL
was complicated enough already. For now I changed the test to sel.is_reversed()
|| sel.is_empty(), which should match the old behavior.

[xji, 2012/02/16 19:52:55]

I see.
Yes, that is the problem. It should be replaced by a method that takes a pair of
SelectionModel as argument, instead of Range (a pair of only caret_pos). And it
needs to change upper layer to get/pass the correct caret_affinity.
I am fine with either way.

+          CURSOR_FORWARD : CURSOR_BACKWARD;
+  SetSelectionModel(SelectionModel(sel, affinity));
   return true;
 }
 
 bool RenderText::IsPointInSelection(const Point& point) {
-  if (EmptySelection())
+  if (selection().is_empty())
     return false;
-  // TODO(xji): should this check whether the point is inside the visual
-  // selection bounds? In case of "abcFED", if "ED" is selected, |point| points
-  // to the right half of 'c', is the point in selection?
-  size_t pos = FindCursorPosition(point).selection_end();
-  return (pos >= MinOfSelection() && pos < MaxOfSelection());
+  SelectionModel cursor = FindCursorPosition(point);
+  return RangeContainsCaret(
+      selection(), cursor.caret_pos(), cursor.caret_affinity());
 }
 
 void RenderText::ClearSelection() {
-  SelectionModel sel(selection_model());
-  sel.set_selection_start(GetCursorPosition());
-  SetSelectionModel(sel);
+  SetSelectionModel(SelectionModel(selection_model_.caret_pos(),
+                                   selection_model_.caret_affinity()));
 }
 
 void RenderText::SelectAll() {
-  SelectionModel sel = EdgeSelectionModel(CURSOR_RIGHT);
-  sel.set_selection_start(EdgeSelectionModel(CURSOR_LEFT).selection_start());
-  SetSelectionModel(sel);
+  SelectionModel all;
+  if (GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
+    all = SelectionModel(ui::Range(0, text().length()), CURSOR_FORWARD);
+  else
+    all = SelectionModel(ui::Range(text().length(), 0), CURSOR_BACKWARD);
+  SetSelectionModel(all);
 }
 
 void RenderText::SelectWord() {
-  size_t cursor_position = GetCursorPosition();
+  size_t cursor_position = selection_model_.caret_pos();
 
   base::i18n::BreakIterator iter(text(), base::i18n::BreakIterator::BREAK_WORD);
   bool success = iter.Init();
   DCHECK(success);
   if (!success)
     return;
 
   size_t selection_start = cursor_position;
   for (; selection_start != 0; --selection_start) {
     if (iter.IsStartOfWord(selection_start) ||
@@ skipping 70 matching lines @@
 
   DrawCursor(canvas);
 
   if (!text().empty()) {
     TRACE_EVENT0("gfx", "RenderText::Draw draw text");
     DrawVisualText(canvas);
   }
   canvas->Restore();
 }
 
+Rect RenderText::GetCursorBounds(const SelectionModel& selection,
+                                 bool insert_mode) {
+  EnsureLayout();
+
+  size_t caret_pos = selection.caret_pos();
+  LogicalCursorDirection caret_affinity = selection.caret_affinity();
+  int x, width, height;
+  if (caret_pos == (caret_affinity == CURSOR_BACKWARD ? 0 : text().length())) {
+    // The caret is attached to the boundary. Always return a zero-width caret,
+    // since there is nothing to overstrike.
+    Size size = GetStringSize();
+    if ((GetTextDirection() == base::i18n::LEFT_TO_RIGHT) == (caret_pos == 0))
+      x = 0;
+    else
+      x = size.width();
+    width = 0;
+    height = size.height();
+  } else {
+    if (caret_affinity == CURSOR_BACKWARD)
+      caret_pos = IndexOfAdjacentGrapheme(caret_pos, CURSOR_BACKWARD);
+    ui::Range xspan;
+    GetGlyphBounds(caret_pos, &xspan, &height);
+    if (insert_mode) {
+      x = (caret_affinity == CURSOR_BACKWARD) ? xspan.end() : xspan.start();
+      width = 0;
+    } else {  // overstrike mode
+      x = xspan.GetMin();
+      width = xspan.length();
+    }
+  }
+  height = std::min(height, display_rect().height());
+  int y = (display_rect().height() - height) / 2;
+  return Rect(ToViewPoint(Point(x, y)), Size(width, height));
+}
+
 const Rect& RenderText::GetUpdatedCursorBounds() {
   UpdateCachedBoundsAndOffset();
   return cursor_bounds_;
 }
 
 SelectionModel RenderText::GetSelectionModelForSelectionStart() {
-  size_t selection_start = GetSelectionStart();
-  size_t selection_end = GetCursorPosition();
-  if (selection_start < selection_end)
-    return SelectionModel(selection_start,
-                          selection_start,
-                          SelectionModel::LEADING);
-  else if (selection_start > selection_end)
+  const ui::Range& selection = selection_model_.selection();
+  if (selection.is_empty()) {
+    return selection_model_;
+  } else {
     return SelectionModel(

[msw, 2012/02/16 00:09:21]

nit: Move this return out of the else block.

-        selection_start,
-        IndexOfAdjacentGrapheme(selection_start, CURSOR_BACKWARD),
-        SelectionModel::TRAILING);
-  return selection_model_;
+        selection.start(),
+        selection.is_reversed() ? CURSOR_BACKWARD : CURSOR_FORWARD);
+  }
 }
 
 RenderText::RenderText()
     : horizontal_alignment_(base::i18n::IsRTL() ? ALIGN_RIGHT : ALIGN_LEFT),
       cursor_enabled_(true),
       cursor_visible_(false),
       insert_mode_(true),
       focused_(false),
       composition_range_(ui::Range::InvalidRange()),
       fade_head_(false),
@@ skipping 13 matching lines @@
   EnsureLayout();
 
   if (break_type == LINE_BREAK || text().empty())
     return EdgeSelectionModel(direction);
   if (break_type == CHARACTER_BREAK)
     return AdjacentCharSelectionModel(current, direction);
   DCHECK(break_type == WORD_BREAK);
   return AdjacentWordSelectionModel(current, direction);
 }
 
+SelectionModel RenderText::EdgeSelectionModel(
+    VisualCursorDirection direction) {
+  if (direction == GetVisualDirectionOfLogicalEnd())
+    return SelectionModel(text().length(), CURSOR_FORWARD);
+  else
+    return SelectionModel(0, CURSOR_BACKWARD);
+}
+
 void RenderText::SetSelectionModel(const SelectionModel& model) {
-  DCHECK_LE(model.selection_start(), text().length());
-  selection_model_.set_selection_start(model.selection_start());
-  DCHECK_LE(model.selection_end(), text().length());
-  selection_model_.set_selection_end(model.selection_end());
-  DCHECK_LT(model.caret_pos(), std::max<size_t>(text().length(), 1));
-  selection_model_.set_caret_pos(model.caret_pos());
-  selection_model_.set_caret_placement(model.caret_placement());
-
+  DCHECK_LE(model.selection().GetMax(), text().length());
+  selection_model_ = model;
   cached_bounds_and_offset_valid_ = false;
 }
 
 void RenderText::ApplyCompositionAndSelectionStyles(
     StyleRanges* style_ranges) {
   // TODO(msw): This pattern ought to be reconsidered; what about composition
   //            and selection overlaps, retain existing local style features?
   // Apply a composition style override to a copy of the style ranges.
   if (composition_range_.IsValid() && !composition_range_.is_empty()) {
     StyleRange composition_style(default_style_);
     composition_style.underline = true;
-    composition_style.range.set_start(composition_range_.start());
-    composition_style.range.set_end(composition_range_.end());
+    composition_style.range = composition_range_;
     ApplyStyleRangeImpl(style_ranges, composition_style);
   }
   // Apply a selection style override to a copy of the style ranges.
-  if (!EmptySelection()) {
+  if (!selection().is_empty()) {
     StyleRange selection_style(default_style_);
     selection_style.foreground = NativeTheme::instance()->GetSystemColor(
         NativeTheme::kColorId_TextfieldSelectionColor);
-    selection_style.range.set_start(MinOfSelection());
-    selection_style.range.set_end(MaxOfSelection());
+    selection_style.range = ui::Range(selection().GetMin(),
+                                      selection().GetMax());
     ApplyStyleRangeImpl(style_ranges, selection_style);
   }
   // Apply replacement-mode style override to a copy of the style ranges.
   //
   // TODO(xji): NEED TO FIX FOR WINDOWS ASAP. Windows call this function (to
   // apply styles) in ItemizeLogicalText(). In order for the cursor's underline
   // character to be drawn correctly, we will need to re-layout the text. It's
   // not practical to do layout on every cursor blink. We need to fix Windows
   // port to apply styles during drawing phase like Linux port does.
   // http://crbug.com/110109
   if (!insert_mode_ && cursor_visible() && focused()) {
     StyleRange replacement_mode_style(default_style_);
     replacement_mode_style.foreground = NativeTheme::instance()->GetSystemColor(
         NativeTheme::kColorId_TextfieldSelectionColor);
-    size_t cursor = GetCursorPosition();
+    size_t cursor = cursor_position();
     replacement_mode_style.range.set_start(cursor);
     replacement_mode_style.range.set_end(
         IndexOfAdjacentGrapheme(cursor, CURSOR_FORWARD));
     ApplyStyleRangeImpl(style_ranges, replacement_mode_style);
   }
 }
 
 Point RenderText::GetTextOrigin() {
   Point origin = display_rect().origin();
   origin = origin.Add(GetUpdatedDisplayOffset());
   origin = origin.Add(GetAlignmentOffset());
   return origin;
 }
 
 Point RenderText::ToTextPoint(const Point& point) {
   return point.Subtract(GetTextOrigin());
 }
 
 Point RenderText::ToViewPoint(const Point& point) {
   return point.Add(GetTextOrigin());
 }
 
 int RenderText::GetContentWidth() {
-  return GetStringWidth() + (cursor_enabled_ ? 1 : 0);
+  return GetStringSize().width() + (cursor_enabled_ ? 1 : 0);
 }
 
 Point RenderText::GetAlignmentOffset() {
   if (horizontal_alignment() != ALIGN_LEFT) {
     int x_offset = display_rect().width() - GetContentWidth();
     if (horizontal_alignment() == ALIGN_CENTER)
       x_offset /= 2;
     return Point(x_offset, 0);
   }
   return Point();
 }
 
 Point RenderText::GetOriginForSkiaDrawing() {
   Point origin(GetTextOrigin());
   // TODO(msw): Establish a vertical baseline for strings of mixed font heights.
   const Font& font = GetFont();
   int height = font.GetHeight();
   DCHECK_LE(height, display_rect().height());
   // Center the text vertically in the display area.
   origin.Offset(0, (display_rect().height() - height) / 2);
   // Offset by the font size to account for Skia expecting y to be the bottom.
   origin.Offset(0, font.GetFontSize());
   return origin;
 }
 
 void RenderText::ApplyFadeEffects(internal::SkiaTextRenderer* renderer) {
   if (!fade_head() && !fade_tail())
     return;
 
-  const int text_width = GetStringWidth();
+  const int text_width = GetStringSize().width();
   const int display_width = display_rect().width();
 
   // If the text fits as-is, no need to fade.
   if (text_width <= display_width)
     return;
 
   int gradient_width = CalculateFadeGradientWidth(GetFont(), display_width);
   if (gradient_width == 0)
     return;
 
@@ skipping 22 matching lines @@
 
   const SkColor color = default_style().foreground;
   SkAutoTUnref<SkShader> shader(
       CreateFadeShader(text_rect, left_part, right_part, color));
   if (shader.get()) {
     // |renderer| adds its own ref. So don't |release()| it from the ref ptr.
     renderer->SetShader(shader.get());
   }
 }
 
+// static
+bool RenderText::RangeContainsCaret(const ui::Range& range,
+                                    size_t caret_pos,
+                                    LogicalCursorDirection caret_affinity) {
+  // NB: exploits unsigned wraparound (WG14/N1124 section 6.2.5 paragraph 9).
+  size_t adjacent = caret_affinity == CURSOR_BACKWARD ?
+                        caret_pos - 1 : caret_pos + 1;

[xji, 2012/02/16 02:38:29]

wrong indentation.

theoretically, it should not be -/+1, should be the index of previous/next
grapheme boundary, but it probably works fine in practice.

so, (0, backward) and (text().length(), forward) will never be considered within
range.
Are (0, backward) and (text().length(), forward) reserved for visual boundary
only and will never be returned otherwise? Which does not seems true,
moving-cursor-left-right reaches those 2 models. Or is it because the passing in
parameters (caret_pos, caret_affinity) are returned from FindCursorPosition(),
and FindCursorPosition will never return (0, backward) and (text().length(),
forward)?

[benrg, 2012/02/16 09:20:55]

It works in practice. It is kind of hacky. I'm thinking about maintaining a list
of run boundaries in logical order and using std::{lower,upper}_bound to find
the proper run in O(log n) time. Then this code could go away.
Yes: in those cases the cursor is attached to the imaginary runs on either side
of the text, so it's not in any text run.
They refer to the visual-and-logical boundary. When you initially cursor to the
visual edge you're attached to the character you just cursored over, which might
not be the logically first/last character. If you press the arrow key once more,
the cursor doesn't move visually, but it moves logically to the edge. That's
represented by (0, backward) or (text().length(), forward). That's how the code
worked before this CL, except for the issue with the start of the text on
Windows.
FindCursorPosition returns those results when the given point is out of bounds
to the left or right.

[xji, 2012/02/16 19:52:55]

I guess you are talking about replacing GetRunContainingCaret().
But RangeContainsCaret() is used in IsPointInSelection() as well.

Thanks for the explanation.

+  return range.Contains(ui::Range(caret_pos, adjacent));
+}
+
 void RenderText::MoveCursorTo(size_t position, bool select) {
   size_t cursor = std::min(position, text().length());
-  size_t caret_pos = IndexOfAdjacentGrapheme(cursor, CURSOR_BACKWARD);
-  SelectionModel::CaretPlacement placement = (caret_pos == cursor) ?
-      SelectionModel::LEADING : SelectionModel::TRAILING;
-  size_t selection_start = select ? GetSelectionStart() : cursor;
   if (IsCursorablePosition(cursor)) {
-    SelectionModel sel(selection_start, cursor, caret_pos, placement);
-    SetSelectionModel(sel);
+    ui::Range pos(cursor);
+    if (select)
+      pos.set_start(selection().start());
+    LogicalCursorDirection affinity = cursor ? CURSOR_BACKWARD : CURSOR_FORWARD;
+    SetSelectionModel(SelectionModel(pos, affinity));
   }
 }
 
 void RenderText::UpdateCachedBoundsAndOffset() {
   if (cached_bounds_and_offset_valid_)
     return;
 
   // First, set the valid flag true to calculate the current cursor bounds using
   // the stale |display_offset_|. Applying |delta_offset| at the end of this
   // function will set |cursor_bounds_| and |display_offset_| to correct values.
@@ skipping 29 matching lines @@
     const int offset = negate_rtl * display_offset_.x();
     if (display_width > (content_width + offset))
       delta_offset = negate_rtl * (display_width - (content_width + offset));
   }
 
   display_offset_.Offset(delta_offset, 0);
   cursor_bounds_.Offset(delta_offset, 0);
 }
 
 void RenderText::DrawSelection(Canvas* canvas) {
-  std::vector<Rect> sel = GetSubstringBounds(
-      GetSelectionStart(), GetCursorPosition());
+  std::vector<Rect> sel = GetSubstringBounds(selection());
   NativeTheme::ColorId color_id = focused() ?
       NativeTheme::kColorId_TextfieldSelectionBackgroundFocused :
       NativeTheme::kColorId_TextfieldSelectionBackgroundUnfocused;
   SkColor color = NativeTheme::instance()->GetSystemColor(color_id);
   for (std::vector<Rect>::const_iterator i = sel.begin(); i < sel.end(); ++i)
     canvas->FillRect(*i, color);
 }
 
 void RenderText::DrawCursor(Canvas* canvas) {
   // Paint cursor. Replace cursor is drawn as rectangle for now.
   // TODO(msw): Draw a better cursor with a better indication of association.
   if (cursor_enabled() && cursor_visible() && focused()) {
     const Rect& bounds = GetUpdatedCursorBounds();
     if (bounds.width() != 0)
       canvas->FillRect(bounds, kCursorColor);
     else
       canvas->DrawRect(bounds, kCursorColor);
   }
 }
 
 }  // namespace gfx