RenderTextHarfBuzz: Add support for multi line text selection.

ui/gfx/render_text_harfbuzz.cc

 // Copyright 2014 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_harfbuzz.h"
 
 #include <limits>
 #include <set>
 
 #include "base/i18n/bidi_line_iterator.h"
@@ skipping 273 matching lines @@
       const Range word_range = words_->GetRange(iter);
       std::vector<internal::LineSegment> word_segments;
       SkScalar word_width = GetWordWidth(word_range, &word_segments);
 
       // If the last word is '\n', we should advance a new line after adding
       // the word to the current line.
       bool new_line = false;
       if (!word_segments.empty() &&
           text_[word_segments.back().char_range.start()] == '\n') {
         new_line = true;
-        word_width -= word_segments.back().width();
-        word_segments.pop_back();
+
+        // Since the line should atleast contain some information regarding the
+        // text range it corresponds to, don't pop the newline segment, if it's
+        // the only segment in the line. This ensures that every line has a non-
+        // empty segments vector (except the last in some cases).
+        if (word_segments.size() != 1u || available_width_ != max_width_) {
+          word_width -= word_segments.back().width();
+          word_segments.pop_back();

[karandeepb, 2016/12/07 03:57:47]

Why do we remove newline segments? 

The change here would cause the \n glyph to be rendered as well when it's the
only character in the line. Is it ok? If you think it's necessary I can add code
to not render the newline segments. 

[msw, 2016/12/13 03:04:55]

I don't know if/why this was necessary; perhaps we should just always include
the newline character in the line segments? Hopefully most fonts don't render a
glyph or add width for newline characters. Maybe ping xdai@?

[karandeepb, 2016/12/16 02:58:38]

See my comment at
https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h#n....

+        }
       }
 
       // If the word is not the first word in the line and it can't fit into
       // the current line, advance a new line.
       if (word_width > available_width_ && available_width_ != max_width_)
         AdvanceLine();
       if (!word_segments.empty())
         AddWordToLine(word_segments);
       if (new_line)
         AdvanceLine();
@@ skipping 502 matching lines @@
 Size RenderTextHarfBuzz::GetStringSize() {
   const SizeF size_f = GetStringSizeF();
   return Size(std::ceil(size_f.width()), size_f.height());
 }
 
 SizeF RenderTextHarfBuzz::GetStringSizeF() {
   EnsureLayout();
   return total_size_;
 }
 
-SelectionModel RenderTextHarfBuzz::FindCursorPosition(const Point& point) {
+SelectionModel RenderTextHarfBuzz::FindCursorPosition(const Point& view_point) {
   EnsureLayout();
+  DCHECK(!lines().empty());
 
-  int x = ToTextPoint(point).x();
-  float offset = 0;
-  size_t run_index = GetRunContainingXCoord(x, &offset);
+  int line_index = GetLineContainingYCoord((view_point - GetLineOffset(0)).y());
+  // Clip line index to a valid value in case kDragToEndIfOutsideVerticalBounds
+  // is false. Else, drag to end.
+  if (line_index < 0) {
+    if (RenderText::kDragToEndIfOutsideVerticalBounds)
+      return EdgeSelectionModel(GetVisualDirectionOfLogicalBeginning());
+    else
+      line_index++;
+  }
+  if (line_index >= (int)lines().size()) {
+    if (RenderText::kDragToEndIfOutsideVerticalBounds)
+      return EdgeSelectionModel(GetVisualDirectionOfLogicalEnd());
+    else
+      line_index--;
+  }
+  const internal::Line& line = lines()[line_index];
 
-  internal::TextRunList* run_list = GetRunList();
-  if (run_index >= run_list->size())
-    return EdgeSelectionModel((x < 0) ? CURSOR_LEFT : CURSOR_RIGHT);
-  const internal::TextRunHarfBuzz& run = *run_list->runs()[run_index];
+  float point_offset_relative_segment = 0;
+  const int segment_index = GetLineSegmentContainingXCoord(
+      line, (view_point - GetLineOffset(line_index)).x(),
+      &point_offset_relative_segment);
+  if (segment_index < 0)
+    return LineSelectionModel(line, CURSOR_LEFT);

[msw, 2016/12/13 03:04:55]

Perhaps we need to replace CURSOR_LEFT with
GetVisualDirectionOfLogicalBeginning() and CURSOR_RIGHT below with
GetVisualDirectionOfLogicalEnd()? That said, I didn't notice an issue here with
RTL text in manual testing.

[karandeepb, 2016/12/16 02:58:38]

Think this is correct, segment_index < 0, indicates, the point was before all
the segments and similarly for the other case, it is after all the segments. The
segments for a line are stored in display order (left to right). See
HarfBuzzLineBreaker::AdvanceLine which sorts the segments in display order and
DrawVisualText which draws the segments.

[msw, 2016/12/19 22:27:42]

Acknowledged.

+  if (segment_index >= (int)line.segments.size())
+    return LineSelectionModel(line, CURSOR_RIGHT);
+  const internal::LineSegment& segment = line.segments[segment_index];
+
+  const internal::TextRunHarfBuzz& run = *GetRunList()->runs()[segment.run];
+  const Range segment_glyph_range =
+      run.CharRangeToGlyphRange(segment.char_range);
+  const float segment_offset_relative_run =
+      segment_glyph_range.GetMin() != 0
+          ? run.positions[segment_glyph_range.GetMin()].x()
+          : 0;
+  const float point_offset_relative_run =
+      point_offset_relative_segment + segment_offset_relative_run;
+
   for (size_t i = 0; i < run.glyph_count; ++i) {
     const SkScalar end =
         i + 1 == run.glyph_count ? run.width : run.positions[i + 1].x();
     const SkScalar middle = (end + run.positions[i].x()) / 2;
 
-    if (offset < middle) {
+    if (point_offset_relative_run < middle) {
       return SelectionModel(DisplayIndexToTextIndex(
           run.glyph_to_char[i] + (run.is_rtl ? 1 : 0)),
           (run.is_rtl ? CURSOR_BACKWARD : CURSOR_FORWARD));
     }
-    if (offset < end) {
+    if (point_offset_relative_run < end) {
       return SelectionModel(DisplayIndexToTextIndex(
           run.glyph_to_char[i] + (run.is_rtl ? 0 : 1)),
           (run.is_rtl ? CURSOR_FORWARD : CURSOR_BACKWARD));
     }
   }
-  return EdgeSelectionModel(CURSOR_RIGHT);
+
+  return LineSelectionModel(line, CURSOR_RIGHT);
 }
 
 bool RenderTextHarfBuzz::IsSelectionSupported() const {
-  // TODO(karandeepb): Support multi-line text selection.
-  return !multiline();
+  return true;
 }
 
 std::vector<RenderText::FontSpan> RenderTextHarfBuzz::GetFontSpansForTesting() {
   EnsureLayout();
 
   internal::TextRunList* run_list = GetRunList();
   std::vector<RenderText::FontSpan> spans;
   for (auto* run : run_list->runs()) {
     spans.push_back(RenderText::FontSpan(
         run->font, Range(DisplayIndexToTextIndex(run->range.start()),
@@ skipping 144 matching lines @@
     if (is_forward ? iter.IsEndOfWord(cursor) : iter.IsStartOfWord(cursor))
       break;
   }
   return cur;
 #endif
 }
 
 std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
   DCHECK(!update_display_run_list_);
   DCHECK(Range(0, text().length()).Contains(range));
   Range layout_range(TextIndexToDisplayIndex(range.start()),

[msw, 2016/12/13 03:04:55]

optional nit: rename this |display_range|

[karandeepb, 2016/12/16 02:58:38]

Done.

                      TextIndexToDisplayIndex(range.end()));
   DCHECK(Range(0, GetDisplayText().length()).Contains(layout_range));
 
   std::vector<Rect> rects;
   if (layout_range.is_empty())
     return rects;
-  std::vector<Range> bounds;
 
   internal::TextRunList* run_list = GetRunList();
+  for (size_t line_index = 0; line_index < lines().size(); ++line_index) {
+    const internal::Line& line = lines()[line_index];
+    if (line.segments.empty()) {
+      DCHECK_EQ(lines().size() - 1, line_index);
+      continue;
+    }
 
-  // Add a Range for each run/selection intersection.
-  for (size_t i = 0; i < run_list->size(); ++i) {
-    internal::TextRunHarfBuzz* run =
-        run_list->runs()[run_list->visual_to_logical(i)];
-    Range intersection = run->range.Intersect(layout_range);
-    if (!intersection.IsValid())
-      continue;
-    DCHECK(!intersection.is_reversed());
-    const size_t left_index =
-        run->is_rtl ? intersection.end() - 1 : intersection.start();
-    const Range leftmost_character_x =
-        run->GetGraphemeBounds(this, left_index).Round();
-    const size_t right_index =
-        run->is_rtl ? intersection.start() : intersection.end() - 1;
-    const Range rightmost_character_x =
-        run->GetGraphemeBounds(this, right_index).Round();
-    Range range_x(leftmost_character_x.start(), rightmost_character_x.end());
-    DCHECK(!range_x.is_reversed());
-    if (range_x.is_empty())
-      continue;
-
-    // Union this with the last range if they're adjacent.
-    DCHECK(bounds.empty() || bounds.back().GetMax() <= range_x.GetMin());
-    if (!bounds.empty() && bounds.back().GetMax() == range_x.GetMin()) {
-      range_x = Range(bounds.back().GetMin(), range_x.GetMax());
-      bounds.pop_back();
+    float line_x = 0;
+    for (const internal::LineSegment& segment : line.segments) {
+      const internal::TextRunHarfBuzz& run = *run_list->runs()[segment.run];
+      const Range intersection = segment.char_range.Intersect(layout_range);
+      DCHECK(!intersection.is_reversed());
+      if (!intersection.is_empty()) {

[karandeepb, 2016/12/07 03:57:47]

The earlier code used GetGraphemeBounds so as to consider the whole grapheme.
However I am not sure if it's necessary. I guess only the graphemes at the end
points would matter, but then the |range| passed would have accounted for it.
Selection seems to be behaving correctly for multi character graphemes. Let me
know if you think this needs a closer look.

[msw, 2016/12/13 03:04:55]

Perhaps that was done to handle invalid ranges (that start/end amid multi-char
graphemes), I'm not sure... What you have seems correct, but I didn't get to
test multi-char graphemes manually today. Maybe look over the tests for this
function and add some extra test cases if needed?

[karandeepb, 2016/12/16 02:58:38]

The code was indeed incorrect for the cases when the range end points were not
valid grapheme boundaries or when the grapheme boundary was between a glyph. I
did not understand the concept of multi grapheme glyphs earlier. Have modified
the code, verified it behaves correctly with ligatures and when selection is not
at a valid grapheme boundary. Also have added tests for the 2 cases. 

+        SkScalar width = run.GetGlyphWidthForCharRange(intersection);
+        SkScalar x;
+        if (run.is_rtl) {
+          x = line_x + SkScalarToFloat(run.GetGlyphWidthForCharRange(gfx::Range(
+                           intersection.end(), segment.char_range.end())));
+        } else {
+          x = line_x + SkScalarToFloat(run.GetGlyphWidthForCharRange(gfx::Range(
+                           segment.char_range.start(), intersection.start())));
+        }
+        int end_x = std::ceil(x + width);
+        int start_x = std::ceil(x);
+        gfx::Rect rect(start_x, 0, end_x - start_x, line.size.height());
+        rects.push_back(rect + GetLineOffset(line_index));
+      }
+      line_x += segment.width();
     }
-    bounds.push_back(range_x);
-  }
-  for (Range& bound : bounds) {
-    std::vector<Rect> current_rects = TextBoundsToViewBounds(bound);
-    rects.insert(rects.end(), current_rects.begin(), current_rects.end());
   }
   return rects;
 }
 
 size_t RenderTextHarfBuzz::TextIndexToDisplayIndex(size_t index) {
   return TextIndexToGivenTextIndex(GetDisplayText(), index);
 }
 
 size_t RenderTextHarfBuzz::DisplayIndexToTextIndex(size_t index) {
   if (!obscured())
@@ skipping 154 matching lines @@
   LogicalCursorDirection affinity = caret.caret_affinity();
   internal::TextRunList* run_list = GetRunList();
   for (size_t i = 0; i < run_list->size(); ++i) {
     internal::TextRunHarfBuzz* run = run_list->runs()[i];
     if (RangeContainsCaret(run->range, layout_position, affinity))
       return i;
   }
   return run_list->size();
 }
 
-size_t RenderTextHarfBuzz::GetRunContainingXCoord(float x,
-                                                  float* offset) const {
-  DCHECK(!update_display_run_list_);
-  const internal::TextRunList* run_list = GetRunList();
-  if (x < 0)
-    return run_list->size();
-  // Find the text run containing the argument point (assumed already offset).
-  float current_x = 0;
-  for (size_t i = 0; i < run_list->size(); ++i) {
-    size_t run = run_list->visual_to_logical(i);
-    current_x += run_list->runs()[run]->width;
-    if (x < current_x) {
-      *offset = x - (current_x - run_list->runs()[run]->width);
-      return run;
+int RenderTextHarfBuzz::GetLineContainingYCoord(float text_y) {
+  if (text_y < 0)
+    return -1;
+
+  for (size_t i = 0; i < lines().size(); i++) {
+    const internal::Line& line = lines()[i];
+
+    if (text_y <= line.size.height())
+      return i;
+    text_y -= line.size.height();
+  }
+
+  return lines().size();
+}
+
+int RenderTextHarfBuzz::GetLineSegmentContainingXCoord(
+    const internal::Line& line,
+    float line_x,
+    float* offset_relative_segment) {
+  DCHECK(offset_relative_segment);
+
+  *offset_relative_segment = 0;
+  if (line_x < 0)
+    return -1;
+  for (size_t i = 0; i < line.segments.size(); i++) {
+    const internal::LineSegment& segment = line.segments[i];
+
+    // segment.x_range is not used because it is text space.
+    if (line_x <= segment.width()) {
+      *offset_relative_segment = line_x;
+      return i;
     }
+    line_x -= segment.width();
   }
-  return run_list->size();
+  return line.segments.size();
 }
 
 SelectionModel RenderTextHarfBuzz::FirstSelectionModelInsideRun(
     const internal::TextRunHarfBuzz* run) {
   size_t position = DisplayIndexToTextIndex(run->range.start());
   position = IndexOfAdjacentGrapheme(position, CURSOR_FORWARD);
   return SelectionModel(position, CURSOR_BACKWARD);
 }
 
 SelectionModel RenderTextHarfBuzz::LastSelectionModelInsideRun(
@@ skipping 367 matching lines @@
 
       attribute.strike = run.strike;
       attribute.diagonal_strike = run.diagonal_strike;
       decorated_text->attributes.push_back(attribute);
     }
   }
   return true;
 }
 
 }  // namespace gfx