Skip to content

Commit

Permalink
Fix apply styles with range to support graphemes
Browse files Browse the repository at this point in the history
This CL is adding support for graphemes with the RenderText styles.

The range of a style needs to be enlarge to the whole grapheme. Otherwise,
we can get cases where an emoji or a ligature can be split apart
during the ItemizeText(...) phase.

Bug: 1020841
Change-Id: I8e6e49c5d6250a907d8d4ed5a13dc8b16421592e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903948
Commit-Queue: Etienne Bergeron <[email protected]>
Reviewed-by: Alexei Svitkine <[email protected]>
Reviewed-by: Robert Liao <[email protected]>
Cr-Commit-Position: refs/heads/master@{#714051}
  • Loading branch information
bergeret authored and Commit Bot committed Nov 9, 2019
1 parent 5f66d9b commit c7b6d3a
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 8 deletions.
30 changes: 22 additions & 8 deletions ui/gfx/render_text.cc
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,9 @@ void RenderText::SetColor(SkColor value) {
}

void RenderText::ApplyColor(SkColor value, const Range& range) {
colors_.ApplyValue(value, range);
// Do not change styles mid-grapheme to avoid breaking ligatures.
Range expanded_range = ExpandRangeToGraphemeBoundary(range);
colors_.ApplyValue(value, expanded_range);
OnTextColorChanged();
}

Expand All @@ -900,7 +902,9 @@ void RenderText::SetBaselineStyle(BaselineStyle value) {
}

void RenderText::ApplyBaselineStyle(BaselineStyle value, const Range& range) {
baselines_.ApplyValue(value, range);
// Do not change styles mid-grapheme to avoid breaking ligatures.
Range expanded_range = ExpandRangeToGraphemeBoundary(range);
baselines_.ApplyValue(value, expanded_range);
}

void RenderText::ApplyFontSizeOverride(int font_size_override,
Expand All @@ -919,11 +923,8 @@ void RenderText::SetStyle(TextStyle style, bool value) {

void RenderText::ApplyStyle(TextStyle style, bool value, const Range& range) {
// Do not change styles mid-grapheme to avoid breaking ligatures.
const size_t start = IsValidCursorIndex(range.start()) ? range.start() :
IndexOfAdjacentGrapheme(range.start(), CURSOR_BACKWARD);
const size_t end = IsValidCursorIndex(range.end()) ? range.end() :
IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD);
styles_[style].ApplyValue(value, Range(start, end));
Range expanded_range = ExpandRangeToGraphemeBoundary(range);
styles_[style].ApplyValue(value, expanded_range);

cached_bounds_and_offset_valid_ = false;
// TODO(oshima|msw): Not all style change requires layout changes.
Expand All @@ -939,7 +940,9 @@ void RenderText::SetWeight(Font::Weight weight) {
}

void RenderText::ApplyWeight(Font::Weight weight, const Range& range) {
weights_.ApplyValue(weight, range);
// Do not change styles mid-grapheme to avoid breaking ligatures.
Range expanded_range = ExpandRangeToGraphemeBoundary(range);
weights_.ApplyValue(weight, expanded_range);

cached_bounds_and_offset_valid_ = false;
OnLayoutTextAttributeChanged(false);
Expand Down Expand Up @@ -2101,6 +2104,17 @@ Range RenderText::ExpandRangeToWordBoundary(const Range& range) const {
: Range(range_min, range_max);
}

Range RenderText::ExpandRangeToGraphemeBoundary(const Range& range) {
const size_t start =
IsValidCursorIndex(range.start())
? range.start()
: IndexOfAdjacentGrapheme(range.start(), CURSOR_BACKWARD);
const size_t end = IsValidCursorIndex(range.end())
? range.end()
: IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD);
return Range(start, end);
}

internal::TextRunList* RenderText::GetRunList() {
NOTREACHED();
return nullptr;
Expand Down
4 changes: 4 additions & 0 deletions ui/gfx/render_text.h
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,10 @@ class GFX_EXPORT RenderText {
// range. Maintains directionality of |range|.
Range ExpandRangeToWordBoundary(const Range& range) const;

// Expands |range| to its nearest grapheme boundaries and returns the
// resulting range.
Range ExpandRangeToGraphemeBoundary(const Range& range);

// Returns an implementation-specific run list, if implemented.
virtual internal::TextRunList* GetRunList();
virtual const internal::TextRunList* GetRunList() const;
Expand Down
42 changes: 42 additions & 0 deletions ui/gfx/render_text_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,48 @@ TEST_F(RenderTextTest, ApplyStyles) {
{{0, false}, {1, true}, {6, false}}));
}

TEST_F(RenderTextTest, ApplyStyleSurrogatePair) {
RenderText* render_text = GetRenderText();
render_text->SetText(WideToUTF16(L"x\U0001F601x"));
// Apply the style in the middle of a surrogate pair. The style should be
// applied to the whole range of the codepoint.
gfx::Range range(2, 3);
render_text->ApplyWeight(gfx::Font::Weight::BOLD, range);
render_text->ApplyStyle(TEXT_STYLE_ITALIC, true, range);
render_text->ApplyColor(SK_ColorRED, range);
render_text->Draw(canvas());

EXPECT_TRUE(test_api()->styles()[TEXT_STYLE_ITALIC].EqualsForTesting(
{{0, false}, {1, true}, {3, false}}));
EXPECT_TRUE(test_api()->colors().EqualsForTesting(
{{0, SK_ColorBLACK}, {1, SK_ColorRED}, {3, SK_ColorBLACK}}));
EXPECT_TRUE(
test_api()->weights().EqualsForTesting({{0, Font::Weight::NORMAL},
{1, Font::Weight::BOLD},
{3, Font::Weight::NORMAL}}));
}

TEST_F(RenderTextTest, ApplyStyleGrapheme) {
RenderText* render_text = GetRenderText();
render_text->SetText(WideToUTF16(L"x\u0065\u0301x"));
// Apply the style in the middle of a grapheme. The style should be applied to
// the whole range of the grapheme.
gfx::Range range(2, 3);
render_text->ApplyWeight(gfx::Font::Weight::BOLD, range);
render_text->ApplyStyle(TEXT_STYLE_ITALIC, true, range);
render_text->ApplyColor(SK_ColorRED, range);
render_text->Draw(canvas());

EXPECT_TRUE(test_api()->styles()[TEXT_STYLE_ITALIC].EqualsForTesting(
{{0, false}, {1, true}, {3, false}}));
EXPECT_TRUE(test_api()->colors().EqualsForTesting(
{{0, SK_ColorBLACK}, {1, SK_ColorRED}, {3, SK_ColorBLACK}}));
EXPECT_TRUE(
test_api()->weights().EqualsForTesting({{0, Font::Weight::NORMAL},
{1, Font::Weight::BOLD},
{3, Font::Weight::NORMAL}}));
}

TEST_F(RenderTextTest, AppendTextKeepsStyles) {
RenderText* render_text = GetRenderText();
// Setup basic functionality.
Expand Down

0 comments on commit c7b6d3a

Please sign in to comment.