RenderText: always break runs at whitespace

ui/gfx/render_text_unittest.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 <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 287 matching lines @@
   std::vector<base::char16> arr(length, RenderText::kPasswordReplacementChar);
   arr[reveal_index] = reveal_char;
   return base::string16(arr.begin(), arr.end());
 }
 
 // Helper method to return an obscured string of the given |length|.
 base::string16 GetObscuredString(size_t length) {
   return base::string16(length, RenderText::kPasswordReplacementChar);
 }
 
+std::vector<base::string16> ToString16Vec(

[Alexei Svitkine (slow), 2017/06/16 14:45:33]

Nit: Add 1-line comments for the new helper methods you're adding.

[tapted, 2017/06/19 04:10:50]

Done.

+    const std::vector<const char*>& utf8_literals) {
+  std::vector<base::string16> vec;
+  for (auto* const literal : utf8_literals)
+    vec.push_back(UTF8ToUTF16(literal));
+  return vec;
+}
+
+Range LineCharRange(const internal::Line& line) {
+  if (line.segments.empty())
+    return Range();
+  Range ltr(line.segments.front().char_range.start(),
+            line.segments.back().char_range.end());
+  if (ltr.end() > ltr.start())
+    return ltr;
+
+  // For RTL, the order of segments is reversed, but the ranges are not.
+  return Range(line.segments.back().char_range.start(),
+               line.segments.front().char_range.end());
+}
+
 // The class which records the drawing operations so that the test case can
 // verify where exactly the glyphs are drawn.
 class TestSkiaTextRenderer : public internal::SkiaTextRenderer {
  public:
   struct TextLog {
     TextLog() : glyph_count(0u), color(SK_ColorTRANSPARENT) {}
     PointF origin;
     size_t glyph_count;
     SkColor color;
   };
@@ skipping 137 matching lines @@
     return test::RenderTextTestApi::GetRendererPaint(renderer());
   }
 
   void DrawVisualText() { test_api_->DrawVisualText(renderer()); }
 
   internal::TextRunList* GetHarfBuzzRunList() {
     DCHECK_EQ(RENDER_TEXT_HARFBUZZ, GetParam());
     return test_api_->GetHarfBuzzRunList();
   }
 
+  std::vector<base::string16> GetRuns() {
+    std::vector<base::string16> runs_as_text;
+    const std::vector<RenderText::FontSpan> spans =
+        render_text_->GetFontSpansForTesting();
+    for (const auto& span : spans) {
+      runs_as_text.push_back(render_text_->text().substr(span.second.GetMin(),
+                                                         span.second.length()));
+    }
+    return runs_as_text;
+  }
+
+  std::vector<base::string16> RunsFor(const base::string16& text) {
+    render_text_->SetText(text);
+    test_api()->EnsureLayout();
+    return GetRuns();
+  }
+
   void ResetRenderTextInstance() {
     render_text_ = CreateRenderTextInstance();
     test_api_.reset(new test::RenderTextTestApi(GetRenderText()));
   }
 
   RenderText* GetRenderText() { return render_text_.get(); };
 
   RenderTextHarfBuzz* GetRenderTextHarfBuzz() {
     DCHECK_EQ(RENDER_TEXT_HARFBUZZ, GetParam());
     return static_cast<RenderTextHarfBuzz*>(GetRenderText());
@@ skipping 2440 matching lines @@
     render_text->SetText(WideToUTF16(kTestStrings[i]));
     const int expected_width = render_text->GetStringSize().width();
     render_text->MoveCursorTo(SelectionModel(Range(0, 1), CURSOR_FORWARD));
     EXPECT_EQ(expected_width, render_text->GetStringSize().width());
     // Drawing the text should not DCHECK or crash; see http://crbug.com/262119
     render_text->Draw(canvas());
     render_text->MoveCursorTo(SelectionModel(0, CURSOR_FORWARD));
   }
 }
 
+// Test that characters commonly used in the context of several scripts do not
+// cause text runs to break. For example the Japanese "long sound symbol" --
+// normally only used in a Katakana script, is also used on occasion when in
+// Hiragana scripts. It shouldn't cause a Hiragana text run break since that
+// could upset kerning.
+TEST_P(RenderTextTest, ScriptExtensionsDoNotBreak) {
+  // Apparently ramen restaurants prefer "らーめん" over "らあめん". The "dash"
+  // is the long sound symbol and usually just appears in Katakana writing.
+  const base::string16 ramen_hiragana = UTF8ToUTF16("らーめん");
+  const base::string16 ramen_katakana = UTF8ToUTF16("ラーメン");
+  const base::string16 ramen_mixed = UTF8ToUTF16("らあメン");
+
+  EXPECT_EQ(std::vector<base::string16>({ramen_hiragana}),
+            RunsFor(ramen_hiragana));
+  EXPECT_EQ(std::vector<base::string16>({ramen_katakana}),
+            RunsFor(ramen_katakana));
+
+  // Currently Harfbuzz breaks this, but not Mac.
+  if (GetParam() == RENDER_TEXT_HARFBUZZ)
+    EXPECT_EQ(ToString16Vec({"らあ", "メン"}), RunsFor(ramen_mixed));
+  else
+    EXPECT_EQ(std::vector<base::string16>({ramen_mixed}), RunsFor(ramen_mixed));
+}
+
+// Test that whitespace breaks runs of text. E.g. this can permit better fonts
+// to be chosen by the fallback mechanism when a font does not provide
+// whitespace glyphs for all scripts. See http://crbug.com/731563.
+TEST_P(RenderTextTest, WhitespaceDoesBreak) {
+  // Title of the Wikipedia page for "bit". ASCII spaces. In Hebrew and English.
+  // Note that the hyphens that Wikipedia uses are different. English uses
+  // ASCII (U+002D) "hyphen minus", Hebrew uses the U+2013 "EN Dash".
+  const base::string16 ascii_space_he = UTF8ToUTF16("סיבית – ויקיפדיה");
+  const base::string16 ascii_space_en = ASCIIToUTF16("Bit - Wikipedia");
+
+  // This says "thank you very much" with a full-width non-ascii space (U+3000).
+  const base::string16 full_width_space = UTF8ToUTF16("ども　ありがと");
+
+  if (GetParam() == RENDER_TEXT_HARFBUZZ) {
+    // Old behavior:
+    // { "סיבית ", "–", " ", "ויקיפדיה"" }
+    // { "Bit ", "- ", "Wikipedia" }
+    // { "ども　ありがと"" }.
+    EXPECT_EQ(ToString16Vec({"סיבית", " ", "–", " ", "ויקיפדיה"}),
+              RunsFor(ascii_space_he));
+    EXPECT_EQ(ToString16Vec({"Bit", " - ", "Wikipedia"}),
+              RunsFor(ascii_space_en));
+    EXPECT_EQ(ToString16Vec({"ども", "　", "ありがと"}),
+              RunsFor(full_width_space));
+  } else {
+    // Mac is a black box. From this it seems to draw RTL runs right to left and
+    // is able to use typeface characteristics when breaking runs. On 10.9, the
+    // Hebrew string is given a single run, and on 10.12 it's given 3 runs. It
+    // doesn't really matter as far as the rest of the RenderText machinery is
+    // concerned.
+    auto actual = RunsFor(ascii_space_he);
+    if (actual.size() == 3) {
+      EXPECT_EQ(ToString16Vec({"ויקיפדיה", " – ", "סיבית"}), actual);
+    } else {
+      EXPECT_EQ(ToString16Vec({"סיבית – ויקיפדיה"}), actual);
+    }
+    EXPECT_EQ(ToString16Vec({"Bit - Wikipedia"}), RunsFor(ascii_space_en));
+    EXPECT_EQ(ToString16Vec({"ども　ありがと"}), RunsFor(full_width_space));
+  }
+}
+
 // Ensure strings wrap onto multiple lines for a small available width.
 TEST_P(RenderTextHarfBuzzTest, Multiline_MinWidth) {
   const wchar_t* kTestStrings[] = { kWeak, kLtr, kLtrRtl, kLtrRtlLtr, kRtl,
                                     kRtlLtr, kRtlLtrRtl };
 
   RenderText* render_text = GetRenderText();
   render_text->SetDisplayRect(Rect(1, 1000));
   render_text->SetMultiline(true);
   render_text->SetWordWrapBehavior(WRAP_LONG_WORDS);
 
   for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
     SCOPED_TRACE(base::StringPrintf("kTestStrings[%" PRIuS "]", i));
     render_text->SetText(WideToUTF16(kTestStrings[i]));
     render_text->Draw(canvas());
     EXPECT_GT(test_api()->lines().size(), 1U);
   }
 }
 
 // Ensure strings wrap onto multiple lines for a normal available width.
 TEST_P(RenderTextHarfBuzzTest, Multiline_NormalWidth) {
+  // Should RenderText suppress drawing whitespace at the end of a line?
+  // Currently it does not.
   const struct {
     const wchar_t* const text;
     const Range first_line_char_range;
     const Range second_line_char_range;
+
+    // Lengths of each text run. Runs break at whitespace.
+    std::vector<size_t> run_lengths;
+
+    // The index of the text run that should start the second line.
+    int second_line_run_index;
+
     bool is_ltr;
   } kTestStrings[] = {
-    { L"abc defg hijkl", Range(0, 9), Range(9, 14), true },
-    { L"qwertyzxcvbn", Range(0, 10), Range(10, 12), true },
-    { L"\x0627\x0644\x0644\x063A\x0629 "
-      L"\x0627\x0644\x0639\x0631\x0628\x064A\x0629",
-      Range(0, 6), Range(6, 13), false },
-    { L"\x062A\x0641\x0627\x062D \x05EA\x05E4\x05D5\x05D6\x05D9"
-      L"\x05DA\x05DB\x05DD", Range(0, 5), Range(5, 13), false }
-  };
+      {L"abc defg hijkl", Range(0, 9), Range(9, 14), {3, 1, 4, 1, 5}, 4, true},
+      {L"qwertyzxcvbn", Range(0, 10), Range(10, 12), {10, 2}, 1, true},
+      // RTL: should render left-to-right as "<space>43210 \n cba9876".
+      {L"\x0627\x0644\x0644\x063A\x0629 "
+       L"\x0627\x0644\x0639\x0631\x0628\x064A\x0629",
+       Range(0, 6),
+       Range(6, 13),
+       {1 /* space first */, 5, 7},
+       2,
+       false},
+      // RTL: should render left-to-right as "<space>3210 \n cba98765".
+      {L"\x062A\x0641\x0627\x062D \x05EA\x05E4\x05D5\x05D6\x05D9"
+       L"\x05DA\x05DB\x05DD",
+       Range(0, 5),
+       Range(5, 13),
+       {1 /* space first */, 5, 8},
+       2,
+       false}};
 
   RenderTextHarfBuzz* render_text = GetRenderTextHarfBuzz();
 
   // Specify the fixed width for characters to suppress the possible variations
   // of linebreak results.
   SetGlyphWidth(5);
   render_text->SetDisplayRect(Rect(50, 1000));
   render_text->SetMultiline(true);
   render_text->SetWordWrapBehavior(WRAP_LONG_WORDS);
   render_text->SetHorizontalAlignment(ALIGN_TO_HEAD);
 
   for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
     SCOPED_TRACE(base::StringPrintf("kTestStrings[%" PRIuS "]", i));
     render_text->SetText(WideToUTF16(kTestStrings[i].text));
     DrawVisualText();
 
     ASSERT_EQ(2U, test_api()->lines().size());
-    ASSERT_EQ(1U, test_api()->lines()[0].segments.size());
     EXPECT_EQ(kTestStrings[i].first_line_char_range,
-              test_api()->lines()[0].segments[0].char_range);
-    ASSERT_EQ(1U, test_api()->lines()[1].segments.size());
+              LineCharRange(test_api()->lines()[0]));
     EXPECT_EQ(kTestStrings[i].second_line_char_range,
-              test_api()->lines()[1].segments[0].char_range);
+              LineCharRange(test_api()->lines()[1]));
 
     std::vector<TestSkiaTextRenderer::TextLog> text_log;
     renderer()->GetTextLogAndReset(&text_log);
-    ASSERT_EQ(2U, text_log.size());
+
+    ASSERT_EQ(kTestStrings[i].run_lengths.size(), text_log.size());
+
     // NOTE: this expectation compares the character length and glyph counts,
     // which isn't always equal. This is okay only because all the test
     // strings are simple (like, no compound characters nor UTF16-surrogate
     // pairs). Be careful in case more complicated test strings are added.
-    EXPECT_EQ(kTestStrings[i].first_line_char_range.length(),
-              text_log[0].glyph_count);
-    EXPECT_EQ(kTestStrings[i].second_line_char_range.length(),
-              text_log[1].glyph_count);
-    EXPECT_LT(text_log[0].origin.y(), text_log[1].origin.y());
+    EXPECT_EQ(kTestStrings[i].run_lengths[0], text_log[0].glyph_count);
+    const int second_line_start = kTestStrings[i].second_line_run_index;
+    EXPECT_EQ(kTestStrings[i].run_lengths[second_line_start],
+              text_log[second_line_start].glyph_count);
+    EXPECT_LT(text_log[0].origin.y(), text_log[second_line_start].origin.y());
     if (kTestStrings[i].is_ltr) {
       EXPECT_EQ(0, text_log[0].origin.x());
-      EXPECT_EQ(0, text_log[1].origin.x());
+      EXPECT_EQ(0, text_log[second_line_start].origin.x());
     } else {
       EXPECT_LT(0, text_log[0].origin.x());
-      EXPECT_LT(0, text_log[1].origin.x());
+      EXPECT_LT(0, text_log[second_line_start].origin.x());
     }
   }
 }
 
 // Ensure strings don't wrap onto multiple lines for a sufficient available
 // width.
 TEST_P(RenderTextHarfBuzzTest, Multiline_SufficientWidth) {
   const wchar_t* kTestStrings[] = { L"", L" ", L".", L" . ", L"abc", L"a b c",
                                     L"\x62E\x628\x632", L"\x62E \x628 \x632" };
 
@@ skipping 166 matching lines @@
   for (size_t i = 0; i < arraysize(kTestScenarios); ++i) {
     SCOPED_TRACE(base::StringPrintf(
         "kTestScenarios[%" PRIuS "] %d", i, kTestScenarios[i].behavior));
     render_text->SetWordWrapBehavior(kTestScenarios[i].behavior);
     render_text->Draw(canvas());
 
     ASSERT_EQ(kTestScenarios[i].num_lines, test_api()->lines().size());
     for (size_t j = 0; j < test_api()->lines().size(); ++j) {
       SCOPED_TRACE(base::StringPrintf("%" PRIuS "-th line", j));
       EXPECT_EQ(kTestScenarios[i].char_ranges[j],
-                test_api()->lines()[j].segments[0].char_range);
+                LineCharRange(test_api()->lines()[j]));
       EXPECT_EQ(kTestScenarios[i].char_ranges[j].length() * kGlyphSize,
                 test_api()->lines()[j].size.width());
     }
   }
 }
 
 TEST_P(RenderTextHarfBuzzTest, Multiline_LineBreakerBehavior) {
   const int kGlyphSize = 5;
   const struct {
     const wchar_t* const text;
@@ skipping 388 matching lines @@
   EXPECT_EQ(0U, run_list->logical_to_visual(3));
 }
 
 TEST_P(RenderTextHarfBuzzTest, HarfBuzz_BreakRunsByUnicodeBlocks) {
   RenderTextHarfBuzz* render_text = GetRenderTextHarfBuzz();
 
   // The '\x25B6' "play character" should break runs. http://crbug.com/278913
   render_text->SetText(WideToUTF16(L"x\x25B6y"));
   test_api()->EnsureLayout();
   internal::TextRunList* run_list = GetHarfBuzzRunList();
+  EXPECT_EQ(ToString16Vec({"x", "▶", "y"}), GetRuns());
   ASSERT_EQ(3U, run_list->size());
   EXPECT_EQ(Range(0, 1), run_list->runs()[0]->range);
   EXPECT_EQ(Range(1, 2), run_list->runs()[1]->range);
   EXPECT_EQ(Range(2, 3), run_list->runs()[2]->range);
 
   render_text->SetText(WideToUTF16(L"x \x25B6 y"));
   test_api()->EnsureLayout();
   run_list = GetHarfBuzzRunList();
-  ASSERT_EQ(4U, run_list->size());
-  EXPECT_EQ(Range(0, 2), run_list->runs()[0]->range);
-  EXPECT_EQ(Range(2, 3), run_list->runs()[1]->range);
-  EXPECT_EQ(Range(3, 4), run_list->runs()[2]->range);
-  EXPECT_EQ(Range(4, 5), run_list->runs()[3]->range);
+  EXPECT_EQ(ToString16Vec({"x", " ", "▶", " ", "y"}), GetRuns());

[tapted, 2017/06/16 07:12:36]

this test actually captures the old inconsistency. This line would have been
written

EXPECT_EQ(ToString16Vec({"x ", "▶", " ", "y"}), GetRuns());

i.e. 4 breaks, with whitespace attached to the 'x' but not attached to the 'y'.

+  ASSERT_EQ(5U, run_list->size());
+  EXPECT_EQ(Range(0, 1), run_list->runs()[0]->range);
+  EXPECT_EQ(Range(1, 2), run_list->runs()[1]->range);
+  EXPECT_EQ(Range(2, 3), run_list->runs()[2]->range);
+  EXPECT_EQ(Range(3, 4), run_list->runs()[3]->range);
+  EXPECT_EQ(Range(4, 5), run_list->runs()[4]->range);
 }
 
 TEST_P(RenderTextHarfBuzzTest, HarfBuzz_BreakRunsByEmoji) {
   RenderTextHarfBuzz* render_text = GetRenderTextHarfBuzz();
 
   // \xF0\x9F\x98\x81 (U+1F601) is smile icon emoji. \xE2\x9C\xA8 (U+2728) is
   // a sparkle icon. Both can be drawn with color emoji fonts, so runs should be
   // separated. See crbug.com/448909
   render_text->SetText(UTF8ToUTF16("x\xF0\x9F\x98\x81y\xE2\x9C\xA8"));
   test_api()->EnsureLayout();
@@ skipping 930 matching lines @@
                         ::testing::Values(RENDER_TEXT_HARFBUZZ),
                         PrintRenderTextBackend());
 #endif
 
 INSTANTIATE_TEST_CASE_P(,
                         RenderTextHarfBuzzTest,
                         ::testing::Values(RENDER_TEXT_HARFBUZZ),
                         PrintRenderTextBackend());
 
 }  // namespace gfx