Fix multi line render text elision with newline characters.
This fixes multi line elision for text that contains newline
characters. This had the following two issues:
1) RenderText::Elide renders the given text on a single line.
This is fixed by only passing in a single line when eliding the text in
RenderText::UpdateDisplayText.
2) The HarfBuzzLineBreaker keeps track of the min and max ascent.
This needs to ignore newline characters in multiline modes as they are
not drawn.
(cherry picked from commit 78785c2d677e46a7dcde8a0e8708c5c67f5e3890)
Bug: 972997
Change-Id: Ie7f6d84d61000595eaa53a660caff7b98b0fb17c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1649778
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#668440}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1662557
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#371}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index 87fd631..bad91f31 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -1073,8 +1073,36 @@
}
bool RenderText::IsNewlineSegment(const internal::LineSegment& segment) const {
- DCHECK_LT(segment.char_range.start(), text_.length());
- return text_[segment.char_range.start()] == '\n';
+ return IsNewlineSegment(text_, segment);
+}
+
+bool RenderText::IsNewlineSegment(const base::string16& text,
+ const internal::LineSegment& segment) const {
+ DCHECK_LT(segment.char_range.start(), text.length());
+ return text[segment.char_range.start()] == '\n';
+}
+
+Range RenderText::GetLineRange(const base::string16& text,
+ const internal::Line& line) const {
+ // This will find the logical start and end indices of the given line.
+ size_t max_index = 0;
+ size_t min_index = text.length();
+ for (const auto& segment : line.segments) {
+ min_index = std::min<size_t>(min_index, segment.char_range.GetMin());
+ max_index = std::max<size_t>(max_index, segment.char_range.GetMax());
+ }
+
+ // Do not include the newline character, as that could be considered leading
+ // into the next line. Note that the newline character is always the last
+ // character of the line regardless of the text direction, so decrease the
+ // |max_index|.
+ if (!line.segments.empty() &&
+ (IsNewlineSegment(text, line.segments.back()) ||
+ IsNewlineSegment(text, line.segments.front()))) {
+ --max_index;
+ }
+
+ return Range(min_index, max_index);
}
RenderText::RenderText()
@@ -1162,27 +1190,14 @@
}
DCHECK_GT(GetNumLines(), 1U);
- size_t max_index = 0;
- size_t min_index = text().length();
- for (const auto& segment : line.segments) {
- min_index = std::min<size_t>(min_index, segment.char_range.GetMin());
- max_index = std::max<size_t>(max_index, segment.char_range.GetMax());
- }
-
- // Do not select the newline character to preserve the line number of the
- // cursor. Note that the newline character is always the last character of the
- // line regardless of the text direction, so decrease the |max_index|.
- if (!line.segments.empty() && (IsNewlineSegment(line.segments.back()) ||
- IsNewlineSegment(line.segments.front()))) {
- --max_index;
- }
+ Range line_range = GetLineRange(text(), line);
// Cursor affinity should be the opposite of visual direction to preserve the
// line number of the cursor in multiline text.
return direction == GetVisualDirectionOfLogicalEnd()
- ? SelectionModel(DisplayIndexToTextIndex(max_index),
+ ? SelectionModel(DisplayIndexToTextIndex(line_range.end()),
CURSOR_BACKWARD)
- : SelectionModel(DisplayIndexToTextIndex(min_index),
+ : SelectionModel(DisplayIndexToTextIndex(line_range.start()),
CURSOR_FORWARD);
}
@@ -1227,14 +1242,15 @@
render_text->EnsureLayout();
if (render_text->lines_.size() > max_lines_) {
- // Find the start index of the line to be elided.
- size_t start_of_elision = layout_text_.length();
- for (const auto& segment : render_text->lines_[max_lines_ - 1].segments) {
- uint32_t segment_start = segment.char_range.GetMin();
- start_of_elision = std::min<size_t>(start_of_elision, segment_start);
- }
- base::string16 text_to_elide = layout_text_.substr(start_of_elision);
- display_text_.assign(layout_text_.substr(0, start_of_elision) +
+ // Find the start and end index of the line to be elided.
+ Range line_range =
+ GetLineRange(layout_text_, render_text->lines_[max_lines_ - 1]);
+ // Add an ellipsis character in case the last line is short enough to fit
+ // on a single line. Otherwise that character will be elided anyway.
+ base::string16 text_to_elide =
+ layout_text_.substr(line_range.start(), line_range.length()) +
+ base::string16(kEllipsisUTF16);
+ display_text_.assign(layout_text_.substr(0, line_range.start()) +
Elide(text_to_elide, 0,
static_cast<float>(display_rect_.width()),
ELIDE_TAIL));
diff --git a/ui/gfx/render_text.h b/ui/gfx/render_text.h
index 5c39851..5a422c0 100644
--- a/ui/gfx/render_text.h
+++ b/ui/gfx/render_text.h
@@ -566,9 +566,6 @@
void set_strike_thickness_factor(SkScalar f) { strike_thickness_factor_ = f; }
- // Whether |segment| corresponds to the newline character.
- bool IsNewlineSegment(const internal::LineSegment& segment) const;
-
// Return the line index that contains the argument; or the index of the last
// line if the |caret| exceeds the text length.
virtual size_t GetLineContainingCaret(const SelectionModel& caret) = 0;
@@ -576,6 +573,19 @@
protected:
RenderText();
+ // Whether |segment| corresponds to the newline character. This uses |text_|
+ // to look up the corresponding character.
+ bool IsNewlineSegment(const internal::LineSegment& segment) const;
+
+ // Whether |segment| corresponds to the newline character inside |text|.
+ bool IsNewlineSegment(const base::string16& text,
+ const internal::LineSegment& segment) const;
+
+ // Returns the character range of segments in |line| excluding the trailing
+ // newline segment.
+ Range GetLineRange(const base::string16& text,
+ const internal::Line& line) const;
+
// NOTE: The value of these accessors may be stale. Please make sure
// that these fields are up to date before accessing them.
const base::string16& layout_text() const { return layout_text_; }
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 720668e..a9e3666 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -348,7 +348,7 @@
segment.char_range = run.range;
segment.x_range = RangeF(SkScalarToFloat(text_x_),
SkScalarToFloat(text_x_) + run.shape.width);
- AddLineSegment(segment);
+ AddLineSegment(segment, false);
}
}
@@ -451,7 +451,7 @@
if (IsNewlineSegment(text_, segment) ||
segment.width() <= available_width_ ||
word_wrap_behavior_ == IGNORE_LONG_WORDS) {
- AddLineSegment(segment);
+ AddLineSegment(segment, true);
} else {
DCHECK(word_wrap_behavior_ == TRUNCATE_LONG_WORDS ||
word_wrap_behavior_ == WRAP_LONG_WORDS);
@@ -470,7 +470,7 @@
Range(remaining_segment.char_range.start(), cutoff_pos);
cut_segment.x_range = RangeF(SkScalarToFloat(text_x_),
SkScalarToFloat(text_x_ + width));
- AddLineSegment(cut_segment);
+ AddLineSegment(cut_segment, true);
// Updates old segment range.
remaining_segment.char_range.set_start(cutoff_pos);
remaining_segment.x_range.set_start(SkScalarToFloat(text_x_));
@@ -487,7 +487,7 @@
// Add a line segment to the current line. Note that, in order to keep the
// visual order correct for ltr and rtl language, we need to merge segments
// that belong to the same run.
- void AddLineSegment(const internal::LineSegment& segment) {
+ void AddLineSegment(const internal::LineSegment& segment, bool multiline) {
DCHECK(!lines_.empty());
internal::Line* line = &lines_.back();
const internal::TextRunHarfBuzz& run = *(run_list_.runs()[segment.run]);
@@ -515,19 +515,22 @@
line->segments.push_back(segment);
line->size.set_width(line->size.width() + segment.width());
- SkFont font(run.font_params.skia_face, run.font_params.font_size);
- font.setEdging(run.font_params.render_params.antialiasing
- ? SkFont::Edging::kAntiAlias
- : SkFont::Edging::kAlias);
- SkFontMetrics metrics;
- font.getMetrics(&metrics);
+ // Newline characters are not drawn for multi-line, ignore their metrics.
+ if (!multiline || !IsNewlineSegment(text_, segment)) {
+ SkFont font(run.font_params.skia_face, run.font_params.font_size);
+ font.setEdging(run.font_params.render_params.antialiasing
+ ? SkFont::Edging::kAntiAlias
+ : SkFont::Edging::kAlias);
+ SkFontMetrics metrics;
+ font.getMetrics(&metrics);
- // max_descent_ is y-down, fDescent is y-down, baseline_offset is y-down
- max_descent_ = std::max(max_descent_,
- metrics.fDescent + run.font_params.baseline_offset);
- // max_ascent_ is y-up, fAscent is y-down, baseline_offset is y-down
- max_ascent_ = std::max(
- max_ascent_, -(metrics.fAscent + run.font_params.baseline_offset));
+ // max_descent_ is y-down, fDescent is y-down, baseline_offset is y-down
+ max_descent_ = std::max(
+ max_descent_, metrics.fDescent + run.font_params.baseline_offset);
+ // max_ascent_ is y-up, fAscent is y-down, baseline_offset is y-down
+ max_ascent_ = std::max(
+ max_ascent_, -(metrics.fAscent + run.font_params.baseline_offset));
+ }
if (run.font_params.is_rtl) {
rtl_segments_.push_back(
@@ -1700,13 +1703,14 @@
ApplyCompositionAndSelectionStyles();
internal::TextRunList* run_list = GetRunList();
+ const base::string16& display_text = GetDisplayText();
for (size_t i = 0; i < lines().size(); ++i) {
const internal::Line& line = lines()[i];
const Vector2d origin = GetLineOffset(i) + Vector2d(0, line.baseline);
SkScalar preceding_segment_widths = 0;
for (const internal::LineSegment& segment : line.segments) {
// Don't draw the newline glyph (crbug.com/680430).
- if (IsNewlineSegment(segment))
+ if (IsNewlineSegment(display_text, segment))
continue;
const internal::TextRunHarfBuzz& run = *run_list->runs()[segment.run];
diff --git a/ui/gfx/render_text_unittest.cc b/ui/gfx/render_text_unittest.cc
index dc3b0ad0..6ea37e6 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -1148,6 +1148,42 @@
EXPECT_EQ(render_text->GetNumLines(), 1U);
}
+TEST_F(RenderTextTest, MultilineElideBiDi) {
+ RenderText* render_text = GetRenderText();
+ SetGlyphWidth(5);
+
+ base::string16 input_text(UTF8ToUTF16("אa\nbcdבגדהefg\nhו"));
+ render_text->SetText(input_text);
+ render_text->SetCursorEnabled(false);
+ render_text->SetMultiline(true);
+ render_text->SetMaxLines(2);
+ render_text->SetElideBehavior(ELIDE_TAIL);
+ render_text->SetDisplayRect(Rect(30, 0));
+ render_text->GetStringSize();
+
+ EXPECT_EQ(render_text->GetDisplayText(),
+ UTF8ToUTF16("אa\nbcdבג") + base::string16(kEllipsisUTF16));
+ EXPECT_EQ(render_text->GetNumLines(), 2U);
+}
+
+TEST_F(RenderTextTest, MultilineElideLinebreak) {
+ RenderText* render_text = GetRenderText();
+ SetGlyphWidth(5);
+
+ base::string16 input_text(UTF8ToUTF16("hello\nworld"));
+ render_text->SetText(input_text);
+ render_text->SetCursorEnabled(false);
+ render_text->SetMultiline(true);
+ render_text->SetMaxLines(1);
+ render_text->SetElideBehavior(ELIDE_TAIL);
+ render_text->SetDisplayRect(Rect(100, 0));
+ render_text->GetStringSize();
+
+ EXPECT_EQ(render_text->GetDisplayText(),
+ input_text.substr(0, 5) + base::string16(kEllipsisUTF16));
+ EXPECT_EQ(render_text->GetNumLines(), 1U);
+}
+
TEST_F(RenderTextTest, ElidedStyledTextRtl) {
static const char* kInputTexts[] = {
"http://ar.wikipedia.com/فحص",
@@ -2958,6 +2994,10 @@
25, render_text->GetLineSize(SelectionModel(6, CURSOR_FORWARD)).width());
// |GetStringSize()| of multi-line text does not include newline character.
EXPECT_EQ(25, render_text->GetStringSize().width());
+ // Expect height to be 2 times the font height. This assumes simple strings
+ // that do not have special metrics.
+ int font_height = render_text->font_list().GetHeight();
+ EXPECT_EQ(font_height * 2, render_text->GetStringSize().height());
}
TEST_F(RenderTextTest, MinLineHeight) {