Changeset 271481 in webkit

Timestamp:

Jan 14, 2021, 6:30:19 AM (6 years ago)

Author:

Alan Bujtas

Message:

[LFC][IFC][Quirk] Let's adopt a less quirky behavior for line breaks on non-empty lines
​https://bugs.webkit.org/show_bug.cgi?id=220252

Reviewed by Antti Koivisto.

WebKit has this quirk behavior where <br> stretches the line box vertically only when the line is empty.
e.g.

<div><span><br></span></div>
We make the line as tall as the <br> is but

<div>some content<span style="font-size: 200px;"><br></span></div>
does not produce a ~200px tall line.

However when horizontal padding/border added, the line suddenly becomes ~200px tall:
<div>some content<span style="font-size: 200px; padding: 1px;"><br></span></div>

Removing this quirk makes the inlineLevelBoxAffectsLineBox logic simpler since we don't have to check against
whether the line is considered empty and the result is aligned with Chrome's behavior.
(This is in preparation for getting rid of the is-considered-empty concept for line/line box.)

• layout/inlineformatting/InlineFormattingContextGeometry.cpp:

(WebCore::Layout::LineBoxBuilder::constructInlineLevelBoxes):

• layout/inlineformatting/InlineFormattingContextQuirks.cpp:

(WebCore::Layout::InlineFormattingContext::Quirks::inlineLevelBoxAffectsLineBox const):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• layout/inlineformatting/InlineFormattingContextQuirks.cpp (modified) (2 diffs)
• rendering/InlineFlowBox.cpp (modified) (2 diffs)
• rendering/InlineFlowBox.h (modified) (3 diffs)
• rendering/RenderInline.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-01-14  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC][IFC][Quirk] Let's adopt a less quirky behavior for line breaks on non-empty lines
+        https://bugs.webkit.org/show_bug.cgi?id=220252
+
+        Reviewed by Antti Koivisto.
+
+        WebKit has this quirk behavior where <br> stretches the line box vertically only when the line is empty.
+        e.g.
+
+        <div><span><br></span></div>
+        We make the line as tall as the <br> is but
+
+        <div>some content<span style="font-size: 200px;"><br></span></div>
+        does not produce a ~200px tall line.
+
+        However when horizontal padding/border added, the line suddenly becomes ~200px tall:
+        <div>some content<span style="font-size: 200px; padding: 1px;"><br></span></div>
+
+        Removing this quirk makes the inlineLevelBoxAffectsLineBox logic simpler since we don't have to check against
+        whether the line is considered empty and the result is aligned with Chrome's behavior.
+        (This is in preparation for getting rid of the is-considered-empty concept for line/line box.)
+
+        * layout/inlineformatting/InlineFormattingContextGeometry.cpp:
+        (WebCore::Layout::LineBoxBuilder::constructInlineLevelBoxes):
+        * layout/inlineformatting/InlineFormattingContextQuirks.cpp:
+        (WebCore::Layout::InlineFormattingContext::Quirks::inlineLevelBoxAffectsLineBox const):
+
 2021-01-14  Martin Robinson  <mrobinson@igalia.com>
 

trunk/Source/WebCore/layout/inlineformatting/InlineFormattingContextQuirks.cpp

@@ -45 +45 @@
 bool InlineFormattingContext::Quirks::inlineLevelBoxAffectsLineBox(const LineBox::InlineLevelBox& inlineLevelBox, const LineBox& lineBox) const
 {
+    if (inlineLevelBox.isLineBreakBox()) {
+        if (layoutState().inNoQuirksMode())
+            return true;
+        // In quirks mode linebreak boxes (<br>) affect the line box when they are inside a non-root inline box (<span></span>) or when
+        // the line has no other inline level box/root inlinebox has no content.
+        auto& parentInlineBox = lineBox.inlineLevelBoxForLayoutBox(inlineLevelBox.layoutBox().parent());
+        if (!parentInlineBox.isRootInlineBox())
+            return true;
+        return !parentInlineBox.hasContent() && lineBox.nonRootInlineLevelBoxes().size() == 1;
+    }
     if (inlineLevelBox.isInlineBox()) {
         // Inline boxes (e.g. root inline box or <span>) affects line boxes either through the strut or actual content.
@@ -58 +68 @@
         auto inlineBoxHasImaginaryStrut = layoutState().inNoQuirksMode();
         return inlineBoxHasImaginaryStrut && !lineBox.isConsideredEmpty();
-    }
-    if (inlineLevelBox.isLineBreakBox()) {
-        // <br> in non-standard mode stretches the line box only when the line is empty.
-        // e.g. <div><span><br></span></div> will stretch but <div>this will not stretch to 200px<span style="font-size: 200px;"><br></span></div>
-        return layoutState().inNoQuirksMode() ? true : lineBox.isConsideredEmpty();
     }
     if (inlineLevelBox.isAtomicInlineLevelBox()) {

trunk/Source/WebCore/rendering/InlineFlowBox.cpp

@@ -138 +138 @@
                 // FIXME: This isn't ideal. We only turn off because current layout test results expect the <br> to be 0-height on the baseline.
                 // Other than making a zillion tests have to regenerate results, there's no reason to ditch the optimization here.
-                shouldClearDescendantsHaveSameLineHeightAndBaseline = child->renderer().isBR();
+                auto childIsHardLinebreak = child->renderer().isBR();
+                shouldClearDescendantsHaveSameLineHeightAndBaseline = childIsHardLinebreak;
+                m_hasHardLinebreak = m_hasHardLinebreak || childIsHardLinebreak;
             } else {
                 auto& childFlowBox = downcast<InlineFlowBox>(*child);
@@ -596 +598 @@
             if (maxPositionBottom < boxHeight)
                 maxPositionBottom = boxHeight;
-        } else if (!inlineFlowBox || strictMode || inlineFlowBox->hasTextChildren() || (inlineFlowBox->descendantsHaveSameLineHeightAndBaseline() && inlineFlowBox->hasTextDescendants())
-                   || inlineFlowBox->renderer().hasInlineDirectionBordersOrPadding()) {
+        } else if (strictMode
+            || !inlineFlowBox
+            || inlineFlowBox->hasTextChildren()
+            || (inlineFlowBox->descendantsHaveSameLineHeightAndBaseline() && inlineFlowBox->hasTextDescendants())
+            || inlineFlowBox->renderer().hasInlineDirectionBordersOrPadding()
+            || inlineFlowBox->hasHardLinebreak()) {
             // Note that these values can be negative.  Even though we only affect the maxAscent and maxDescent values
             // if our box (excluding line-height) was above (for ascent) or below (for descent) the root baseline, once you factor in line-height

trunk/Source/WebCore/rendering/InlineFlowBox.h

@@ -45 +45 @@
         , m_includeLogicalLeftEdge(false)
         , m_includeLogicalRightEdge(false)
+        , m_hasHardLinebreak(false)
         , m_descendantsHaveSameLineHeightAndBaseline(true)
         , m_baselineType(AlphabeticBaseline)
@@ -209 +210 @@
     bool hasTextChildren() const { return m_hasTextChildren; }
     bool hasTextDescendants() const { return m_hasTextDescendants; }
+    bool hasHardLinebreak() const { return m_hasHardLinebreak; }
     void setHasTextChildren() { m_hasTextChildren = true; setHasTextDescendants(); }
     void setHasTextDescendants() { m_hasTextDescendants = true; }
@@ -313 +315 @@
     unsigned m_hasTextChildren : 1;
     unsigned m_hasTextDescendants : 1;
+    unsigned m_hasHardLinebreak : 1;
     unsigned m_descendantsHaveSameLineHeightAndBaseline : 1;
 

trunk/Source/WebCore/rendering/RenderInline.cpp

@@ -213 +213 @@
     auto* parentStyle = &parent()->style();
     RenderInline* parentRenderInline = is<RenderInline>(*parent()) ? downcast<RenderInline>(parent()) : nullptr;
+    auto hasHardLineBreakChildOnly = firstChild() && firstChild() == lastChild() && firstChild()->isBR();
     bool checkFonts = document().inNoQuirksMode();
     bool alwaysCreateLineBoxes = (parentRenderInline && parentRenderInline->alwaysCreateLineBoxes())
@@ -219 +220 @@
         || style().textEmphasisMark() != TextEmphasisMark::None
         || (checkFonts && (!parentStyle->fontCascade().fontMetrics().hasIdenticalAscentDescentAndLineGap(style().fontCascade().fontMetrics())
-        || parentStyle->lineHeight() != style().lineHeight()));
+        || parentStyle->lineHeight() != style().lineHeight()))
+        || hasHardLineBreakChildOnly;
 
     if (!alwaysCreateLineBoxes && checkFonts && view().usesFirstLineRules()) {