Skip to content

Commit 634d150

Browse files
lsalzmanSkia Commit-Bot
authored andcommitted
fix SkTextBlob::getIntercepts regression no longer considering glyph vertical offset
As of revision 7924d9a (https://skia-review.googlesource.com/c/skia/+/222277), SkTextBlob::getIntercepts no longer considers each glyph's vertical offset. So getIntercepts will only work properly if each glyph has a vertical offset of 0. This patch restores the original behavior of offsetting the bounds from each glyph's position. Without this fix, Firefox has no way to implement proper intercepts for underlining decorations when glyphs are offset... Change-Id: I06fc4b63bd57c9d70e3b07a95ead69f3caa8b33a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248724 Reviewed-by: Herb Derby <herb@google.com> Commit-Queue: Herb Derby <herb@google.com> Auto-Submit: Lee Salzman <lsalzman@mozilla.com>
1 parent 20c626a commit 634d150

File tree

2 files changed

+36
-3
lines changed

2 files changed

+36
-3
lines changed

src/core/SkTextBlob.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -881,9 +881,6 @@ int get_glyph_run_intercepts(const SkGlyphRun& glyphRun,
881881
SkScalar xPos = xOffset;
882882
SkScalar prevAdvance = 0;
883883

884-
// The typeface is scaled, so un-scale the bounds to be in the space of the typeface.
885-
SkScalar scaledBounds[2] = {bounds[0] / scale, bounds[1] / scale};
886-
887884
const SkPoint* posCursor = glyphRun.positions().begin();
888885
for (auto glyphID : glyphRun.glyphsIDs()) {
889886
SkPoint pos = *posCursor++;
@@ -892,6 +889,12 @@ int get_glyph_run_intercepts(const SkGlyphRun& glyphRun,
892889
xPos += prevAdvance * scale;
893890
prevAdvance = glyph->advanceX();
894891
if (cache->preparePath(glyph) != nullptr) {
892+
// The typeface is scaled, so un-scale the bounds to be in the space of the typeface.
893+
// Also ensure the bounds are properly offset by the vertical positioning of the glyph.
894+
SkScalar scaledBounds[2] = {
895+
(bounds[0] - pos.y()) / scale,
896+
(bounds[1] - pos.y()) / scale
897+
};
895898
cache->findIntercepts(scaledBounds, scale, pos.x(), glyph, intervals, intervalCount);
896899
}
897900
}

tests/TextBlobTest.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,3 +480,33 @@ DEF_TEST(TextBlob_iter, reporter) {
480480
// Hello should have the same glyph repeated for the 'l'
481481
REPORTER_ASSERT(reporter, run.fGlyphIndices[2] == run.fGlyphIndices[3]);
482482
}
483+
484+
DEF_TEST(TextBlob_getIntercepts, reporter) {
485+
SkFont font;
486+
font.setSize(16);
487+
488+
SkPoint lowPos[1] = { SkPoint::Make(0, 5) };
489+
SkPoint highPos[1] = { SkPoint::Make(0, -8) };
490+
SkPoint zeroPos[1] = { SkPoint::Make(0, 0) };
491+
492+
// 'x' sitting on baseline
493+
auto blobZeroX = SkTextBlob::MakeFromPosText("x", 1, zeroPos, font);
494+
// 'x' lowered to intersect baseline
495+
auto blobLowX = SkTextBlob::MakeFromPosText("x", 1, lowPos, font);
496+
// 'y' sitting on baseline
497+
auto blobZeroY = SkTextBlob::MakeFromPosText("y", 1, zeroPos, font);
498+
// 'y' raised to not intersect baseline
499+
auto blobHighY = SkTextBlob::MakeFromPosText("y", 1, highPos, font);
500+
501+
// bounds right below baseline
502+
SkScalar bounds[2] = { 1, 2 };
503+
504+
// 'x' on baseline should not intersect
505+
REPORTER_ASSERT(reporter, blobZeroX->getIntercepts(bounds, nullptr) == 0);
506+
// lowered 'x' should intersect
507+
REPORTER_ASSERT(reporter, blobLowX->getIntercepts(bounds, nullptr) == 2);
508+
// 'y' on baseline should intersect
509+
REPORTER_ASSERT(reporter, blobZeroY->getIntercepts(bounds, nullptr) == 2);
510+
// raised 'y' should not intersect
511+
REPORTER_ASSERT(reporter, blobHighY->getIntercepts(bounds, nullptr) == 0);
512+
}

0 commit comments

Comments
 (0)