Fixing some round problems with metrics

Bug: skia:10996
Change-Id: I003086d3625bb713f4f6c682346ab190dddd7426
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339856
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>

modules/skparagraph/src/ParagraphImpl.cpp

@@ -447,27 +447,6 @@ void ParagraphImpl::breakShapedTextIntoLines(SkScalar maxWidth) {
     fAlphabeticBaseline = fLines.empty() ? fEmptyMetrics.alphabeticBaseline() : fLines.front().alphabeticBaseline();
     fIdeographicBaseline = fLines.empty() ? fEmptyMetrics.ideographicBaseline() : fLines.front().ideographicBaseline();
     fExceededMaxLines = textWrapper.exceededMaxLines();
-
-    // Correct the first and the last line ascents/descents if required
-    if ((fParagraphStyle.getTextHeightBehavior() & TextHeightBehavior::kDisableFirstAscent) != 0) {
-        auto& firstLine = fLines.front();
-        auto delta = firstLine.metricsWithoutMultiplier(TextHeightBehavior::kDisableFirstAscent);
-        if (!SkScalarNearlyZero(delta)) {
-            fHeight += delta;
-            // Shift all the lines up
-            for (auto& line : fLines) {
-                if (line.isFirstLine()) continue;
-                line.shiftVertically(delta);
-            }
-        }
-    }
-
-    if ((fParagraphStyle.getTextHeightBehavior() & TextHeightBehavior::kDisableLastDescent) != 0) {
-        auto& lastLine = fLines.back();
-        auto delta = lastLine.metricsWithoutMultiplier(TextHeightBehavior::kDisableLastDescent);
-        // It's the last line. There is nothing below to shift
-        fHeight += delta;
-    }
 }
 
 void ParagraphImpl::formatLines(SkScalar maxWidth) {

modules/skparagraph/src/Run.h

@@ -315,6 +315,9 @@ public:
         fAscent = a;
         fDescent = d;
         fLeading = l;
+        fRawAscent = a;
+        fRawDescent = d;
+        fRawLeading = l;
         fForceStrut = false;
     }
 
@@ -324,6 +327,9 @@ public:
         fAscent = metrics.fAscent;
         fDescent = metrics.fDescent;
         fLeading = metrics.fLeading;
+        fRawAscent = metrics.fAscent;
+        fRawDescent = metrics.fDescent;
+        fRawLeading = metrics.fLeading;
         fForceStrut = forceStrut;
     }
 
@@ -336,17 +342,28 @@ public:
         fAscent = std::min(fAscent, run->correctAscent());
         fDescent = std::max(fDescent, run->correctDescent());
         fLeading = std::max(fLeading, run->correctLeading());
+
+        fRawAscent = std::min(fRawAscent, run->ascent());
+        fRawDescent = std::max(fRawDescent, run->descent());
+        fRawLeading = std::max(fRawLeading, run->leading());
     }
 
     void add(InternalLineMetrics other) {
         fAscent = std::min(fAscent, other.fAscent);
         fDescent = std::max(fDescent, other.fDescent);
         fLeading = std::max(fLeading, other.fLeading);
+        fRawAscent = std::min(fRawAscent, other.fRawAscent);
+        fRawDescent = std::max(fRawDescent, other.fRawDescent);
+        fRawLeading = std::max(fRawLeading, other.fRawLeading);
     }
+
     void clean() {
         fAscent = 0;
         fDescent = 0;
         fLeading = 0;
+        fRawAscent = 0;
+        fRawDescent = 0;
+        fRawLeading = 0;
     }
 
     SkScalar delta() const { return height() - ideographicBaseline(); }
@@ -356,10 +373,15 @@ public:
             metrics.fAscent = fAscent;
             metrics.fDescent = fDescent;
             metrics.fLeading = fLeading;
+            metrics.fRawAscent = fRawAscent;
+            metrics.fRawDescent = fRawDescent;
+            metrics.fRawLeading = fRawLeading;
         } else {
             // This is another of those flutter changes. To be removed...
             metrics.fAscent = std::min(metrics.fAscent, fAscent - fLeading / 2.0f);
             metrics.fDescent = std::max(metrics.fDescent, fDescent + fLeading / 2.0f);
+            metrics.fRawAscent = std::min(metrics.fRawAscent, fRawAscent - fRawLeading / 2.0f);
+            metrics.fRawDescent = std::max(metrics.fRawDescent, fRawDescent + fRawLeading / 2.0f);
         }
     }
 
@@ -379,6 +401,8 @@ public:
     SkScalar ascent() const { return fAscent; }
     SkScalar descent() const { return fDescent; }
     SkScalar leading() const { return fLeading; }
+    SkScalar rawAscent() const { return fRawAscent; }
+    SkScalar rawDescent() const { return fRawDescent; }
     void setForceStrut(bool value) { fForceStrut = value; }
     bool getForceStrut() const { return fForceStrut; }
 
@@ -390,6 +414,11 @@ private:
     SkScalar fAscent;
     SkScalar fDescent;
     SkScalar fLeading;
+
+    SkScalar fRawAscent;
+    SkScalar fRawDescent;
+    SkScalar fRawLeading;
+
     bool fForceStrut;
 };
 }  // namespace textlayout

modules/skparagraph/src/TextLine.cpp

@@ -166,12 +166,6 @@ SkRect TextLine::paint(SkCanvas* textCanvas, SkScalar x, SkScalar y) {
     }
 
     if (fHasBackground) {
-        // TODO: Flutter only change
-        // Background boxes should ignore line height and height multiplier
-        auto ascentStyle = this->fAscentStyle;
-        auto descentStyle = this->fDescentStyle;
-        this->fAscentStyle = LineMetricStyle::Typographic;
-        this->fDescentStyle = LineMetricStyle::Typographic;
         this->iterateThroughVisualRuns(false,
             [textCanvas, x, y, this]
             (const Run* run, SkScalar runOffsetInLine, TextRange textRange, SkScalar* runWidthInLine) {
@@ -182,8 +176,6 @@ SkRect TextLine::paint(SkCanvas* textCanvas, SkScalar x, SkScalar y) {
                 });
             return true;
             });
-        this->fAscentStyle = ascentStyle;
-        this->fDescentStyle = descentStyle;
     }
 
     if (fHasShadows) {
@@ -291,11 +283,11 @@ SkScalar TextLine::metricsWithoutMultiplier(TextHeightBehavior correction) {
     SkScalar delta = 0;
     if (correction  == TextHeightBehavior::kDisableFirstAscent) {
         delta += (this->fSizes.fAscent - result.fAscent);
-        this->fSizes.fAscent -= delta;
+        this->fSizes.fAscent = result.fAscent;
         this->fAscentStyle = LineMetricStyle::Typographic;
     } else if (correction  == TextHeightBehavior::kDisableLastDescent) {
         delta -= (this->fSizes.fDescent - result.fDescent);
-        this->fSizes.fDescent -= delta;
+        this->fSizes.fDescent = result.fDescent;
         this->fDescentStyle = LineMetricStyle::Typographic;
     }
     fAdvance.fY += delta;

modules/skparagraph/src/TextWrapper.cpp

@@ -228,6 +228,10 @@ void TextWrapper::breakTextIntoLines(ParagraphImpl* parent,
     auto endlessLine = !SkScalarIsFinite(maxWidth);
     auto hasEllipsis = parent->paragraphStyle().ellipsized();
 
+    auto disableFirstAscent = parent->paragraphStyle().getTextHeightBehavior() & TextHeightBehavior::kDisableFirstAscent;
+    auto disableLastDescent = parent->paragraphStyle().getTextHeightBehavior() & TextHeightBehavior::kDisableLastDescent;
+    bool firstLine = true; // We only interested in fist line if we have to disable the first ascent
+
     SkScalar softLineMaxIntrinsicWidth = 0;
     fEndLine = TextStretch(span.begin(), span.begin(), parent->strutForceHeight());
     auto end = span.end() - 1;
@@ -297,11 +301,22 @@ void TextWrapper::breakTextIntoLines(ParagraphImpl* parent,
         }
         ClusterRange clusters(fEndLine.startCluster() - start, fEndLine.endCluster() - start + 1);
         ClusterRange clustersWithGhosts(fEndLine.startCluster() - start, startLine - start);
+
+        if (disableFirstAscent && firstLine) {
+            fEndLine.metrics().fAscent = fEndLine.metrics().fRawAscent;
+        }
+        if (disableLastDescent && (lastLine || (startLine == end && !fHardLineBreak ))) {
+            fEndLine.metrics().fDescent = fEndLine.metrics().fRawDescent;
+        }
+
+        SkScalar lineHeight = fEndLine.metrics().height();
+        firstLine = false;
+
         addLine(text, textWithSpaces, clusters, clustersWithGhosts, widthWithSpaces,
                 fEndLine.startPos(),
                 fEndLine.endPos(),
                 SkVector::Make(0, fHeight),
-                SkVector::Make(fEndLine.width(), fEndLine.metrics().height()),
+                SkVector::Make(fEndLine.width(), lineHeight),
                 fEndLine.metrics(),
                 needEllipsis && !fHardLineBreak);
 
@@ -312,7 +327,7 @@ void TextWrapper::breakTextIntoLines(ParagraphImpl* parent,
             softLineMaxIntrinsicWidth = 0;
         }
         // Start a new line
-        fHeight += fEndLine.metrics().height();
+        fHeight += lineHeight;
         if (!fHardLineBreak || startLine != end) {
             fEndLine.clean();
         }

modules/skparagraph/tests/SkParagraphTest.cpp

@@ -5642,3 +5642,35 @@ DEF_TEST(SkParagraph_NoCache1, reporter) {
     // Make sure that different strings are not flagged as editing
     test("different strings", "0123456789 0123456789 0123456789 0123456789 0123456789", false);
 }
+
+DEF_TEST(SkParagraph_HeightCalculations, reporter) {
+    sk_sp<ResourceFontCollection> fontCollection = sk_make_sp<ResourceFontCollection>();
+    if (!fontCollection->fontsFound()) return;
+
+    TestCanvas canvas("SkParagraph_HeightCalculations.png");
+
+    auto draw = [&](TextHeightBehavior hb, const char* text, SkScalar height) {
+        ParagraphStyle paragraph_style;
+        paragraph_style.setTextHeightBehavior(hb);
+
+        ParagraphBuilderImpl builder(paragraph_style, fontCollection);
+        TextStyle text_style;
+        text_style.setFontFamilies({SkString("Roboto")});
+        text_style.setFontSize(14.0f);
+        text_style.setHeight(5.0f);
+        text_style.setHeightOverride(true);
+        text_style.setColor(SK_ColorBLACK);
+        builder.pushStyle(text_style);
+        builder.addText(text);
+
+        auto paragraph = builder.Build();
+        paragraph->layout(500);
+        paragraph->paint(canvas.get(), 0, 0);
+        canvas.get()->translate(0, paragraph->getHeight());
+        REPORTER_ASSERT(reporter, SkScalarNearlyEqual(paragraph->getHeight(), height));
+    };
+
+    draw(TextHeightBehavior::kAll, "Hello\nLine 2\nLine 3", 210);
+    draw(TextHeightBehavior::kDisableAll, "Hello\nLine 2\nLine 3", 157);
+    draw(TextHeightBehavior::kDisableFirstAscent, "Hello", 28);
+}