Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight().

ui/gfx/render_text_unittest.cc

Index: ui/gfx/render_text_unittest.cc
diff --git a/ui/gfx/render_text_unittest.cc b/ui/gfx/render_text_unittest.cc
index dfd2111875670aa5eab66ab8ce8a42a10630db6c..bb9028f71b184c42cf45dd4e9d6ef74d1365da7e 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -4405,6 +4405,101 @@ TEST_P(RenderTextTest, InvalidFont) {
   DrawVisualText();
 }
 
+// Ensures that text is centered vertically and consistently when either the
+// display rectangle height changes, or when the minimum line height changes.
+// The difference between the two is the selection rectangle, which should match
+// the line height.
+TEST_P(RenderTextHarfBuzzTest, BaselineWithLineHeight) {
+  RenderText* render_text = GetRenderText();
+  const int font_height = render_text->font_list().GetHeight();
+  render_text->SetDisplayRect(Rect(500, font_height));
+  render_text->SetText(ASCIIToUTF16("abc"));
+
+  // Select everything so the test can use GetSelectionBoundsUnion().
+  render_text->SelectAll(false);
+
+  test_api()->EnsureLayout();
+  ASSERT_EQ(1u, test_api()->lines().size());
+  EXPECT_EQ(font_height, test_api()->lines()[0].size.height());
+
+  // Note this is equivalent to RenderText::GetDisplayTextBaseline().

[msw, 2017/03/31 17:19:27]

q: why not just use that here and on 4457, 4482, and 4496?
(that might even make some EnsureLayout calls here unnecessary)

[tapted, 2017/04/02 23:52:42]

Done (added to TestApi + replaced the EnsureLayout calls).

+  const int normal_baseline = test_api()->lines()[0].baseline;
+
+  // With a matching display height, the baseline calculated using font metrics
+  // and the baseline from the layout engine should agree. This works because
+  // the text string is simple (exotic glyphs may use fonts with different
+  // metrics).
+  EXPECT_EQ(normal_baseline, render_text->GetBaseline());
+  EXPECT_EQ(0, render_text->GetLineOffset(0).y());
+
+  const gfx::Rect normal_selection_bounds = GetSelectionBoundsUnion();
+
+  // Sanity check: selection should start from (0,0).
+  EXPECT_EQ(gfx::Vector2d(), normal_selection_bounds.OffsetFromOrigin());
+
+  constexpr int kDelta = 16;
+
+  // Grow just the display rectangle.
+  render_text->SetDisplayRect(Rect(500, font_height + kDelta));
+  test_api()->EnsureLayout();
+  ASSERT_EQ(1u, test_api()->lines().size());
+  EXPECT_EQ(font_height, test_api()->lines()[0].size.height());
+
+  // Save the baseline calculated using the display rectangle before enabling
+  // multi-line or SetMinLineHeight().
+  const int reported_baseline = render_text->GetBaseline();
+  const int baseline_shift = reported_baseline - normal_baseline;
+
+  // When line height matches font height, this should match the line offset.
+  EXPECT_EQ(baseline_shift, render_text->GetLineOffset(0).y());
+
+  // The display text baseline does not move: GetLineOffset() moves it instead.
+  EXPECT_EQ(normal_baseline, test_api()->lines()[0].baseline);
+
+  // The actual shift depends on font metrics, but it should be within a pixel

[msw, 2017/03/31 17:19:27]

Can we use the actual font metrics to make this a bit more resilient? 

[tapted, 2017/04/02 23:52:42]

We'd have to repeat all the calculations done in
DetermineBaselineCenteringText(..), but this step is mainly to ensure that the
"centering" part of those calculations is actually centering (within some
tolerance). Updated the comment: 


  // The actual shift depends on font metrics, and the calculations done in
  // RenderText::DetermineBaselineCenteringText(). Do a sanity check that the
  // "centering" part is happening within some tolerance by ensuring the shift
  // is within a pixel of (kDelta / 2). That is, 7, 8 or 9 pixels (for a delta
  // of 16). An unusual font in future may need more leeway.

+  // of (kDelta / 2). That is, 7, 8 or 9 pixels (for a delta of 16). An unusual
+  // font in future may need more leeway.
+  constexpr int kFuzz = 1;  // If the next EXPECT fails, try increasing this.
+  EXPECT_LE(abs(baseline_shift - kDelta / 2), kFuzz);
+
+  // Increasing display height (but not line height) should shift the selection
+  // bounds down by |baseline_shift|, but leave a matching size.
+  gfx::Rect current_selection_bounds = GetSelectionBoundsUnion();
+  EXPECT_EQ(baseline_shift, current_selection_bounds.y());
+  EXPECT_EQ(0, current_selection_bounds.x());
+  EXPECT_EQ(normal_selection_bounds.size(), current_selection_bounds.size());
+
+  // Now increase the line height, but remain single-line. Note the line height
+  // now matches the display rect.
+  render_text->SetMinLineHeight(font_height + kDelta);
+  test_api()->EnsureLayout();
+  ASSERT_EQ(1u, test_api()->lines().size());
+  EXPECT_EQ(font_height + kDelta, test_api()->lines()[0].size.height());
+
+  // The line offset should go back to zero, but now the display text baseline
+  // should shift down to compensate, and the shift amount should match.
+  EXPECT_EQ(0, render_text->GetLineOffset(0).y());
+  EXPECT_EQ(normal_baseline + baseline_shift, test_api()->lines()[0].baseline);

[tapted, 2017/03/31 10:31:14]

(without the fix, these are the first two lines that fail, since the offset
moves)

+
+  // Now selection bounds should grow in height, but not shift its origin.
+  current_selection_bounds = GetSelectionBoundsUnion();
+  EXPECT_EQ(font_height + kDelta, current_selection_bounds.height());
+  EXPECT_EQ(normal_selection_bounds.width(), current_selection_bounds.width());
+  EXPECT_EQ(gfx::Vector2d(), current_selection_bounds.OffsetFromOrigin());

[tapted, 2017/03/31 10:31:14]

(which means this also fails - the selection bounds shifts down ~7 pixels, so no
longer aligns with the display rectangle)

+
+  // Flipping the multiline flag should change nothing.
+  render_text->SetMultiline(true);
+  test_api()->EnsureLayout();
+  ASSERT_EQ(1u, test_api()->lines().size());
+  EXPECT_EQ(font_height + kDelta, test_api()->lines()[0].size.height());
+  EXPECT_EQ(0, render_text->GetLineOffset(0).y());
+  EXPECT_EQ(normal_baseline + baseline_shift, test_api()->lines()[0].baseline);

[tapted, 2017/03/31 10:31:14]

(for the multiline case, this is the only case that would fail - the selection
bounds is "right", but only because the text is aligned to the top)

+  current_selection_bounds = GetSelectionBoundsUnion();
+  EXPECT_EQ(font_height + kDelta, current_selection_bounds.height());
+  EXPECT_EQ(normal_selection_bounds.width(), current_selection_bounds.width());
+  EXPECT_EQ(gfx::Vector2d(), current_selection_bounds.OffsetFromOrigin());
+}
+
 // Prefix for test instantiations intentionally left blank since each test
 // fixture class has a single parameterization.
 #if defined(OS_MACOSX)