Commit 546e3610 authored by Koji Ishii's avatar Koji Ishii Committed by Commit Bot

Fix ShapeResult::Bounds() when runs are split

HarfBuzzShaper calls ShapeResult::InsertRun() for each run RunSegmenter
segmented.

This patch fixes ShapeResult::InsertRun() to compute
ShapeResult::glyph_bounding_box_ when there are more than one run.

Before this fix, it computes glyph bounding box of each run from (0,0)
then unite all bounding boxes. So if input text is "englishARABIC",
RunSegmenter creates 2 runs and each InsertRun computes:
Run 1: "english", width 70, bounding box (0,0,70,10) (x,y,w,h)
Run 2: "ARABIC", width 80, bounding box (0,0,80,10)
then the united result would be (0,0,80,0) while the total width is 150.

This patch fixes InsertRun() to compute bounding box from the current
point, so that:
Run 1: "english", width 70, bounding box (0,0,70,10)
Run 2: "ARABIC", width 80, bounding box (70,0,80,10)
and the united result would be (0,0,150,0).

This patch also removes incorrect fix made to ShapeResult::CopyRange().
This was done to pass tests in a previous CL[1], but turned out that
the original glyph bounds is incorrect and the fix was wrong.

[1] r487410, https://chromium-review.googlesource.com/c/574508/

BUG=746904, 636993

Change-Id: Iec147e5e9b9853a60489e23355ede730ce918c36
Reviewed-on: https://chromium-review.googlesource.com/577970Reviewed-by: default avatarEmil A Eklund <eae@chromium.org>
Reviewed-by: default avatarDominik Röttsches <drott@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488626}
parent 2cf2db5a
...@@ -433,14 +433,14 @@ layer at (0,0) size 800x521 ...@@ -433,14 +433,14 @@ layer at (0,0) size 800x521
LayoutSVGText {text} at (390,355) size 4x10 contains 1 chunk(s) LayoutSVGText {text} at (390,355) size 4x10 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (390,355) size 4x10 LayoutSVGInlineText {#text} at (390,355) size 4x10
chunk 1 (middle anchor) text run 1 at (390.00,363.00) startOffset 0 endOffset 1 width 4.00: "4" chunk 1 (middle anchor) text run 1 at (390.00,363.00) startOffset 0 endOffset 1 width 4.00: "4"
LayoutSVGText {text} at (50,11) size 60x49 contains 1 chunk(s) LayoutSVGText {text} at (50,11) size 76x49 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (50,11) size 60x49 LayoutSVGInlineText {#text} at (50,11) size 76x49
chunk 1 text run 1 at (50.00,50.00) startOffset 0 endOffset 6 width 60.00: "ab\x{30C}c\x{30C}\x{30C}" chunk 1 text run 1 at (50.00,50.00) startOffset 0 endOffset 6 width 60.00: "ab\x{30C}c\x{30C}\x{30C}"
LayoutSVGText {text} at (200,18) size 56x40 contains 1 chunk(s) LayoutSVGText {text} at (200,18) size 72x40 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (200,18) size 56x40 LayoutSVGInlineText {#text} at (200,18) size 72x40
chunk 1 text run 1 at (200.00,50.00) startOffset 0 endOffset 6 width 50.00: "ab\x{30C}c\x{30C}\x{30C}" chunk 1 text run 1 at (200.00,50.00) startOffset 0 endOffset 6 width 50.00: "ab\x{30C}c\x{30C}\x{30C}"
LayoutSVGText {text} at (350,18) size 60x40 contains 1 chunk(s) LayoutSVGText {text} at (350,18) size 78x40 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (350,18) size 60x40 LayoutSVGInlineText {#text} at (350,18) size 78x40
chunk 1 text run 1 at (350.00,50.00) startOffset 0 endOffset 6 width 56.00: "ab\x{30C}c\x{30C}\x{30C}" chunk 1 text run 1 at (350.00,50.00) startOffset 0 endOffset 6 width 56.00: "ab\x{30C}c\x{30C}\x{30C}"
LayoutSVGText {text} at (40,86) size 60x49 contains 1 chunk(s) LayoutSVGText {text} at (40,86) size 60x49 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (40,86) size 60x49 LayoutSVGInlineText {#text} at (40,86) size 60x49
......
...@@ -433,8 +433,8 @@ layer at (0,0) size 800x521 ...@@ -433,8 +433,8 @@ layer at (0,0) size 800x521
LayoutSVGText {text} at (389.25,355) size 4x10 contains 1 chunk(s) LayoutSVGText {text} at (389.25,355) size 4x10 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (389.25,355) size 4x10 LayoutSVGInlineText {#text} at (389.25,355) size 4x10
chunk 1 (middle anchor) text run 1 at (389.25,363.00) startOffset 0 endOffset 1 width 4.00: "4" chunk 1 (middle anchor) text run 1 at (389.25,363.00) startOffset 0 endOffset 1 width 4.00: "4"
LayoutSVGText {text} at (50,11) size 49x49 contains 1 chunk(s) LayoutSVGText {text} at (50,11) size 54x49 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (50,11) size 49x49 LayoutSVGInlineText {#text} at (50,11) size 54x49
chunk 1 text run 1 at (50.00,50.00) startOffset 0 endOffset 6 width 49.00: "ab\x{30C}c\x{30C}\x{30C}" chunk 1 text run 1 at (50.00,50.00) startOffset 0 endOffset 6 width 49.00: "ab\x{30C}c\x{30C}\x{30C}"
LayoutSVGText {text} at (200,17.11) size 49x40.88 contains 1 chunk(s) LayoutSVGText {text} at (200,17.11) size 49x40.88 contains 1 chunk(s)
LayoutSVGInlineText {#text} at (200,17.11) size 49x40.88 LayoutSVGInlineText {#text} at (200,17.11) size 49x40.88
......
...@@ -425,6 +425,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeIntoArabicThaiHanLatin) { ...@@ -425,6 +425,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeIntoArabicThaiHanLatin) {
HarfBuzzShaper shaper(mixed_string, 8); HarfBuzzShaper shaper(mixed_string, 8);
RefPtr<ShapeResult> result = shaper.Shape(&font, direction); RefPtr<ShapeResult> result = shaper.Shape(&font, direction);
// Check width and bounds are not too much different. ".2" is heuristic.
EXPECT_NEAR(result->Width(), result->Bounds().Width(), result->Width() * .2);
RefPtr<ShapeResult> composite_result = RefPtr<ShapeResult> composite_result =
ShapeResult::Create(&font, 0, direction); ShapeResult::Create(&font, 0, direction);
result->CopyRange(0, 4, composite_result.Get()); result->CopyRange(0, 4, composite_result.Get());
...@@ -464,6 +467,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeAcrossRuns) { ...@@ -464,6 +467,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeAcrossRuns) {
HarfBuzzShaper shaper(mixed_string.Characters16(), mixed_string.length()); HarfBuzzShaper shaper(mixed_string.Characters16(), mixed_string.length());
RefPtr<ShapeResult> result = shaper.Shape(&font, direction); RefPtr<ShapeResult> result = shaper.Shape(&font, direction);
// Check width and bounds are not too much different. ".1" is heuristic.
EXPECT_NEAR(result->Width(), result->Bounds().Width(), result->Width() * .1);
// CopyRange(5, 7) should copy 1 character from [1] and 1 from [2]. // CopyRange(5, 7) should copy 1 character from [1] and 1 from [2].
RefPtr<ShapeResult> target = ShapeResult::Create(&font, 0, direction); RefPtr<ShapeResult> target = ShapeResult::Create(&font, 0, direction);
result->CopyRange(5, 7, target.Get()); result->CopyRange(5, 7, target.Get());
...@@ -486,6 +492,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeSegmentGlyphBoundingBox) { ...@@ -486,6 +492,9 @@ TEST_F(HarfBuzzShaperTest, ShapeResultCopyRangeSegmentGlyphBoundingBox) {
RefPtr<ShapeResult> result = shaper.Shape(&font, direction); RefPtr<ShapeResult> result = shaper.Shape(&font, direction);
EXPECT_EQ(result->Bounds(), composite_result->Bounds()); EXPECT_EQ(result->Bounds(), composite_result->Bounds());
// Check width and bounds are not too much different. ".1" is heuristic.
EXPECT_NEAR(result->Width(), result->Bounds().Width(), result->Width() * .1);
} }
} // namespace blink } // namespace blink
...@@ -389,7 +389,7 @@ void ShapeResult::InsertRun(std::unique_ptr<ShapeResult::RunInfo> run_to_insert, ...@@ -389,7 +389,7 @@ void ShapeResult::InsertRun(std::unique_ptr<ShapeResult::RunInfo> run_to_insert,
: glyph_infos[start_glyph + num_glyphs - 1].cluster; : glyph_infos[start_glyph + num_glyphs - 1].cluster;
float total_advance = 0.0f; float total_advance = 0.0f;
FloatPoint glyph_origin; FloatPoint glyph_origin(width_, 0.0f);
bool has_vertical_offsets = !HB_DIRECTION_IS_HORIZONTAL(run->direction_); bool has_vertical_offsets = !HB_DIRECTION_IS_HORIZONTAL(run->direction_);
// HarfBuzz returns result in visual order, no need to flip for RTL. // HarfBuzz returns result in visual order, no need to flip for RTL.
...@@ -488,16 +488,10 @@ void ShapeResult::CopyRange(unsigned start_offset, ...@@ -488,16 +488,10 @@ void ShapeResult::CopyRange(unsigned start_offset,
left += glyph_bounding_box_.X(); left += glyph_bounding_box_.X();
if (end_offset >= EndIndexForResult()) if (end_offset >= EndIndexForResult())
right += glyph_bounding_box_.MaxX() - width_; right += glyph_bounding_box_.MaxX() - width_;
if (right >= left) { FloatRect adjusted_box(left, glyph_bounding_box_.Y(),
FloatRect adjusted_box(left, glyph_bounding_box_.Y(), right - left, std::max(right - left, 0.0f),
glyph_bounding_box_.Height()); glyph_bounding_box_.Height());
target->glyph_bounding_box_.UniteIfNonZero(adjusted_box); target->glyph_bounding_box_.UniteIfNonZero(adjusted_box);
} else {
FloatRect adjusted_box(left, glyph_bounding_box_.Y(), 0,
glyph_bounding_box_.Height());
target->glyph_bounding_box_.UniteIfNonZero(adjusted_box);
target->glyph_bounding_box_.ShiftMaxXEdgeTo(right);
}
DCHECK_EQ(index - target->num_characters_, DCHECK_EQ(index - target->num_characters_,
std::min(end_offset, EndIndexForResult()) - std::min(end_offset, EndIndexForResult()) -
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment