Fix GetSubstringBounds() rounding to floor x coordinate

Before this CL, RenderText::GetSubstringBounds() rounded up both the
starting x and ending x that it computed. As a result, the left edge
of the substring could be slightly outside the returned bounds, and
the right edge of the returned bounds might contain some small portion
of the next characters.

When the output of GetSubstringBounds() is used as input for
set_display_rect(), this rounding behavior could show up as small
artifacts trailing the domain and/or clipping the first character
slightly (see screenshots in linked bug).

This CL makes GetSubstringBounds() round down the starting x
coordinate.  This provides the guarantee that the returned bounds
always fully contain the substring -- that is, the bounds will be
slightly larger rather than slightly smaller when rounding occurs. An
alternative I considered was to introduce GetSubstringBoundsF and let
consumers do their own rounding. However, as far as I can tell no
other caller cares about the rounding behavior so it seemed like
unnecessary complexity to add a float version of the method.

Bug: 1108045, 1111044
Change-Id: I733326ef0544c4ecbe3e39bb9bd8f7a410bc0047
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2317820
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793692}

ui/gfx/render_text.h

@@ -564,11 +564,13 @@ class GFX_EXPORT RenderText {
   const ShadowValues& shadows() const { return shadows_; }
 
   // Get the visual bounds containing the logical substring within the |range|.
-  // If |range| is empty, the result is empty. These bounds could be visually
-  // discontinuous if the substring is split by a LTR/RTL level change.
-  // These bounds are in local coordinates, but may be outside the visible
-  // region if the text is longer than the textfield. Subsequent text, cursor,
-  // or bounds changes may invalidate returned values.
+  // If |range| is empty, the result is empty. This method rounds internally so
+  // the returned bounds may be slightly larger than the |range|, but are
+  // guaranteed not to be smaller. These bounds could be visually discontinuous
+  // if the substring is split by a LTR/RTL level change. These bounds are in
+  // local coordinates, but may be outside the visible region if the text is
+  // longer than the textfield. Subsequent text, cursor, or bounds changes may
+  // invalidate returned values.
   virtual std::vector<Rect> GetSubstringBounds(const Range& range) = 0;
 
   // Gets the horizontal span (relative to the left of the text, not the view)

ui/gfx/render_text_harfbuzz.cc

@@ -1454,7 +1454,8 @@ std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
         const internal::TextRunHarfBuzz& run = *run_list->runs()[segment.run];
         RangeF selected_span =
             run.GetGraphemeSpanForCharRange(this, intersection);
-        int start_x = base::ClampCeil(selected_span.start() - line_start_x);
+        DCHECK(!selected_span.is_reversed());
+        int start_x = base::ClampFloor(selected_span.start() - line_start_x);
         int end_x = base::ClampCeil(selected_span.end() - line_start_x);
         Rect rect(start_x, 0, end_x - start_x,
                   base::ClampCeil(line.size.height()));

ui/gfx/render_text_unittest.cc

@@ -7016,6 +7016,12 @@ TEST_F(RenderTextTest, SkFontEdging) {
 // Verify GetWordLookupDataAtPoint returns the correct baseline point and
 // decorated word for an LTR string.
 TEST_F(RenderTextTest, GetWordLookupDataAtPoint_LTR) {
+  // Set an integer glyph width; GetCursorBounds() and
+  // GetWordLookupDataAtPoint() use different rounding internally.
+  //
+  // TODO(crbug.com/1111044): this shouldn't be necessary once RenderText keeps
+  // float precision through GetCursorBounds().
+  SetGlyphWidth(5);
   const base::string16 ltr = ASCIIToUTF16("  ab  c ");
   const int kWordOneStartIndex = 2;
   const int kWordTwoStartIndex = 6;
@@ -7095,6 +7101,12 @@ TEST_F(RenderTextTest, GetWordLookupDataAtPoint_LTR) {
 // Verify GetWordLookupDataAtPoint returns the correct baseline point and
 // decorated word for an RTL string.
 TEST_F(RenderTextTest, GetWordLookupDataAtPoint_RTL) {
+  // Set an integer glyph width; GetCursorBounds() and
+  // GetWordLookupDataAtPoint() use different rounding internally.
+  //
+  // TODO(crbug.com/1111044): this shouldn't be necessary once RenderText keeps
+  // float precision through GetCursorBounds().
+  SetGlyphWidth(5);
   const base::string16 rtl = UTF8ToUTF16(" \u0634\u0632  \u0634");
   const int kWordOneStartIndex = 1;
   const int kWordTwoStartIndex = 5;
@@ -7401,6 +7413,33 @@ TEST_F(RenderTextTest, LineEndSelections) {
   }
 }
 
+// Tests that GetSubstringBounds rounds outward when glyphs have floating-point
+// widths.
+TEST_F(RenderTextTest, GetSubstringBoundsFloatingPoint) {
+  const float kGlyphWidth = 5.8;
+  SetGlyphWidth(kGlyphWidth);
+  RenderText* render_text = GetRenderText();
+  render_text->SetDisplayRect(Rect(200, 1000));
+  render_text->SetText(UTF8ToUTF16("abcdef"));
+  gfx::Rect bounds = GetSubstringBoundsUnion(Range(1, 2));
+  // The bounds should be rounded outwards so that the full substring is always
+  // contained in them.
+  EXPECT_EQ(base::ClampFloor(kGlyphWidth), bounds.x());
+  EXPECT_EQ(base::ClampCeil(2 * kGlyphWidth), bounds.right());
+}
+
+// Tests that GetSubstringBounds handles integer glypth widths correctly.
+TEST_F(RenderTextTest, GetSubstringBoundsInt) {
+  const float kGlyphWidth = 5;
+  SetGlyphWidth(kGlyphWidth);
+  RenderText* render_text = GetRenderText();
+  render_text->SetDisplayRect(Rect(200, 1000));
+  render_text->SetText(UTF8ToUTF16("abcdef"));
+  gfx::Rect bounds = GetSubstringBoundsUnion(Range(1, 2));
+  EXPECT_EQ(kGlyphWidth, bounds.x());
+  EXPECT_EQ(2 * kGlyphWidth, bounds.right());
+}
+
 // Tests that GetSubstringBounds returns the correct bounds for multiline text.
 TEST_F(RenderTextTest, GetSubstringBoundsMultiline) {
   RenderText* render_text = GetRenderText();