Split layout of RenderMathMLRow into smaller steps
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 16:34:43 +0000 (16:34 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 16:34:43 +0000 (16:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180348

Patch by Frederic Wang <fwang@igalia.com> on 2017-12-20
Reviewed by Manuel Rego Casasnovas.

Source/WebCore:

Currently, RenderMathMLRow mixes too many steps in the same layout functions: layout children,
calculate stretch size, stretch vertical operators, calculate final ascent/descent, handle
out-of-flow positioned children, set logical height, set logical width for non-display
<math> tag, center display <math> tag etc This situation is inherited from the old flexbox
implementation but it makes difficult to read the code and to re-use layout & metrics
calculation for follow-up work on <mrow>-like elements (<menclose>, <mapdded>, <msqrt> or
<math>). See for example bug 160547 for <math> or bug 161126 for <menclose>.
This patch rewrites RenderMathMLRow into smaller steps:
- stretchVerticalOperatorsAndLayoutChildren() which calls layoutIfNeeded() or
insertPositionedObject() on children and stretch vertical operators.
- getContentBoundingBox() to determine the metrics of the mrow-like element without calling
layout on children or positioning them, so that we can improve mrow-like element in the
future.
- layoutRowItems() which sets the position of children.

Setting the logical width/height or centering children is now moved into layoutBlock() since
derived class overriding layoutBlock() will do their own adjustment for width, height and
positions.

Test: mathml/mrow-preferred-width-with-out-of-flow-child.html
The rest of the behavior is unchanged and already covered by existing tests.

* rendering/mathml/RenderMathMLMenclose.cpp:
(WebCore::RenderMathMLMenclose::layoutBlock): Use the new function and get contentWidth
directly from getContentBoundingBox().
* rendering/mathml/RenderMathMLPadded.cpp:
(WebCore::RenderMathMLPadded::layoutBlock): Ditto.
* rendering/mathml/RenderMathMLRoot.cpp:
(WebCore::RenderMathMLRoot::layoutBlock): Ditto, also remove useless statement
baseAscent = baseDescent.
* rendering/mathml/RenderMathMLRow.cpp:
(WebCore::toVerticalStretchyOperator): New helper function to cast to a vertical stretchy
operator.
(WebCore::RenderMathMLRow::stretchVerticalOperatorsAndLayoutChildren): New helper function
to ensure layoutIfNeeded()/insertPositionedObject() is called on children and that the
vertical operators are stretched.
(WebCore::RenderMathMLRow::getContentBoundingBox const): New helper function to determine
the width/ascent/descent to use for the mrow content.
(WebCore::RenderMathMLRow::computePreferredLogicalWidths): Skip out-of-flow children in the
preferred width calculation. This is verified by the new test.
(WebCore::RenderMathMLRow::layoutRowItems): Only keep the positioning of children with the
specified width and ascent.
(WebCore::RenderMathMLRow::layoutBlock): Center children for <math display="block"> tag and
set the logical width in other cases. Also set the logical height here.
(WebCore::RenderMathMLRow::computeLineVerticalStretch): Deleted. This work is included in
stretchVerticalOperatorsAndLayoutChildren() now.
* rendering/mathml/RenderMathMLRow.h: Update declaration of functions.

LayoutTests:

New test to verify that out-of-flow positioned children are not taken into account in the
calculation of the preferred width of the mrow element.

* mathml/mrow-preferred-width-with-out-of-flow-child-expected.html: Added.
* mathml/mrow-preferred-width-with-out-of-flow-child.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226180 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child-expected.html [new file with mode: 0644]
LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp
Source/WebCore/rendering/mathml/RenderMathMLPadded.cpp
Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp
Source/WebCore/rendering/mathml/RenderMathMLRow.cpp
Source/WebCore/rendering/mathml/RenderMathMLRow.h

index 455c061..d646435 100644 (file)
@@ -1,3 +1,16 @@
+2017-12-20  Frederic Wang  <fwang@igalia.com>
+
+        Split layout of RenderMathMLRow into smaller steps
+        https://bugs.webkit.org/show_bug.cgi?id=180348
+
+        Reviewed by Manuel Rego Casasnovas.
+
+        New test to verify that out-of-flow positioned children are not taken into account in the
+        calculation of the preferred width of the mrow element.
+
+        * mathml/mrow-preferred-width-with-out-of-flow-child-expected.html: Added.
+        * mathml/mrow-preferred-width-with-out-of-flow-child.html: Added.
+
 2017-12-20  Ms2ger  <Ms2ger@igalia.com>
 
         Make fast/css-generated-content/quotes-lang.html pass on GTK
diff --git a/LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child-expected.html b/LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child-expected.html
new file mode 100644 (file)
index 0000000..61f8a26
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <title>mrow preferred width with out-of-flow child</title>
+  </head>
+  <body>
+
+    <p>This test passes if the absolute positioned child is not taken in account in the preferred width computation. You'll see a blue rectangle of about 100px width.</p>
+
+    <table>
+      <tr>
+        <td style="background: blue">
+          <math>
+            <mspace width="100px"></mspace>
+          </math>
+        </td>
+      </tr>
+    </table>
+
+  </body>
+</html>
diff --git a/LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child.html b/LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child.html
new file mode 100644 (file)
index 0000000..f7d9598
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <title>mrow preferred width with out-of-flow child</title>
+  </head>
+  <body>
+
+    <p>This test passes if the absolute positioned child is not taken in account in the preferred width computation. You'll see a blue rectangle of about 100px width.</p>
+
+    <table>
+      <tr>
+        <td style="background: blue">
+          <math>
+            <mspace style="position: absolute;" width="100px"></mspace>
+            <mspace width="100px"></mspace>
+          </math>
+        </td>
+      </tr>
+    </table>
+
+  </body>
+</html>
index 89d4462..4bf4722 100644 (file)
@@ -1,3 +1,58 @@
+2017-12-20  Frederic Wang  <fwang@igalia.com>
+
+        Split layout of RenderMathMLRow into smaller steps
+        https://bugs.webkit.org/show_bug.cgi?id=180348
+
+        Reviewed by Manuel Rego Casasnovas.
+
+        Currently, RenderMathMLRow mixes too many steps in the same layout functions: layout children,
+        calculate stretch size, stretch vertical operators, calculate final ascent/descent, handle
+        out-of-flow positioned children, set logical height, set logical width for non-display
+        <math> tag, center display <math> tag etc This situation is inherited from the old flexbox
+        implementation but it makes difficult to read the code and to re-use layout & metrics
+        calculation for follow-up work on <mrow>-like elements (<menclose>, <mapdded>, <msqrt> or
+        <math>). See for example bug 160547 for <math> or bug 161126 for <menclose>.
+        This patch rewrites RenderMathMLRow into smaller steps:
+        - stretchVerticalOperatorsAndLayoutChildren() which calls layoutIfNeeded() or
+        insertPositionedObject() on children and stretch vertical operators.
+        - getContentBoundingBox() to determine the metrics of the mrow-like element without calling
+        layout on children or positioning them, so that we can improve mrow-like element in the
+        future.
+        - layoutRowItems() which sets the position of children.
+
+        Setting the logical width/height or centering children is now moved into layoutBlock() since
+        derived class overriding layoutBlock() will do their own adjustment for width, height and
+        positions.
+
+        Test: mathml/mrow-preferred-width-with-out-of-flow-child.html
+        The rest of the behavior is unchanged and already covered by existing tests.
+
+        * rendering/mathml/RenderMathMLMenclose.cpp:
+        (WebCore::RenderMathMLMenclose::layoutBlock): Use the new function and get contentWidth
+        directly from getContentBoundingBox().
+        * rendering/mathml/RenderMathMLPadded.cpp:
+        (WebCore::RenderMathMLPadded::layoutBlock): Ditto.
+        * rendering/mathml/RenderMathMLRoot.cpp:
+        (WebCore::RenderMathMLRoot::layoutBlock): Ditto, also remove useless statement
+        baseAscent = baseDescent.
+        * rendering/mathml/RenderMathMLRow.cpp:
+        (WebCore::toVerticalStretchyOperator): New helper function to cast to a vertical stretchy
+        operator.
+        (WebCore::RenderMathMLRow::stretchVerticalOperatorsAndLayoutChildren): New helper function
+        to ensure layoutIfNeeded()/insertPositionedObject() is called on children and that the
+        vertical operators are stretched.
+        (WebCore::RenderMathMLRow::getContentBoundingBox const): New helper function to determine
+        the width/ascent/descent to use for the mrow content.
+        (WebCore::RenderMathMLRow::computePreferredLogicalWidths): Skip out-of-flow children in the
+        preferred width calculation. This is verified by the new test.
+        (WebCore::RenderMathMLRow::layoutRowItems): Only keep the positioning of children with the
+        specified width and ascent.
+        (WebCore::RenderMathMLRow::layoutBlock): Center children for <math display="block"> tag and
+        set the logical width in other cases. Also set the logical height here.
+        (WebCore::RenderMathMLRow::computeLineVerticalStretch): Deleted. This work is included in
+        stretchVerticalOperatorsAndLayoutChildren() now.
+        * rendering/mathml/RenderMathMLRow.h: Update declaration of functions.
+
 2017-12-20  Antti Koivisto  <antti@apple.com>
 
         Move list and multicolumn building code from RenderTreeUpdater to RenderTreeBuilder
index 3755031..908cfe3 100644 (file)
@@ -173,11 +173,10 @@ void RenderMathMLMenclose::layoutBlock(bool relayoutChildren, LayoutUnit)
     if (!relayoutChildren && simplifiedLayout())
         return;
 
-    LayoutUnit contentAscent = 0;
-    LayoutUnit contentDescent = 0;
-    RenderMathMLRow::computeLineVerticalStretch(contentAscent, contentDescent);
-    RenderMathMLRow::layoutRowItems(contentAscent, contentDescent);
-    LayoutUnit contentWidth = logicalWidth();
+    LayoutUnit contentWidth, contentAscent, contentDescent;
+    stretchVerticalOperatorsAndLayoutChildren();
+    getContentBoundingBox(contentWidth, contentAscent, contentDescent);
+    layoutRowItems(contentWidth, contentAscent);
 
     SpaceAroundContent space = spaceAroundContent(contentWidth, contentAscent + contentDescent);
     setLogicalWidth(space.left + contentWidth + space.right);
index 329c5e1..94f8eb2 100644 (file)
@@ -90,11 +90,10 @@ void RenderMathMLPadded::layoutBlock(bool relayoutChildren, LayoutUnit)
         return;
 
     // We first layout our children as a normal <mrow> element.
-    LayoutUnit contentAscent, contentDescent, contentWidth;
-    contentAscent = contentDescent = 0;
-    RenderMathMLRow::computeLineVerticalStretch(contentAscent, contentDescent);
-    RenderMathMLRow::layoutRowItems(contentAscent, contentDescent);
-    contentWidth = logicalWidth();
+    LayoutUnit contentWidth, contentAscent, contentDescent;
+    stretchVerticalOperatorsAndLayoutChildren();
+    getContentBoundingBox(contentWidth, contentAscent, contentDescent);
+    layoutRowItems(contentWidth, contentAscent);
 
     // We parse the mpadded attributes using the content metrics as the default value.
     LayoutUnit width = mpaddedWidth(contentWidth);
index d7f6e99..e94fa2b 100644 (file)
@@ -201,10 +201,9 @@ void RenderMathMLRoot::layoutBlock(bool relayoutChildren, LayoutUnit)
     LayoutUnit baseAscent, baseDescent;
     recomputeLogicalWidth();
     if (rootType() == RootType::SquareRoot) {
-        baseAscent = baseDescent;
-        RenderMathMLRow::computeLineVerticalStretch(baseAscent, baseDescent);
-        RenderMathMLRow::layoutRowItems(baseAscent, baseDescent);
-        m_baseWidth = logicalWidth();
+        stretchVerticalOperatorsAndLayoutChildren();
+        getContentBoundingBox(m_baseWidth, baseAscent, baseDescent);
+        layoutRowItems(m_baseWidth, baseAscent);
     } else {
         getBase().layoutIfNeeded();
         m_baseWidth = getBase().logicalWidth();
index 7b1f893..b33fd2c 100644 (file)
@@ -61,29 +61,66 @@ std::optional<int> RenderMathMLRow::firstLineBaseline() const
     return std::optional<int>(static_cast<int>(lroundf(ascentForChild(*baselineChild) + baselineChild->logicalTop())));
 }
 
-void RenderMathMLRow::computeLineVerticalStretch(LayoutUnit& ascent, LayoutUnit& descent)
+static RenderMathMLOperator* toVerticalStretchyOperator(RenderBox* box)
 {
+    if (is<RenderMathMLBlock>(box)) {
+        auto* renderOperator = downcast<RenderMathMLBlock>(*box).unembellishedOperator();
+        if (renderOperator && renderOperator->isStretchy() && renderOperator->isVertical())
+            return renderOperator;
+    }
+    return nullptr;
+}
+
+void RenderMathMLRow::stretchVerticalOperatorsAndLayoutChildren()
+{
+    // First calculate stretch ascent and descent.
+    LayoutUnit stretchAscent, stretchDescent;
     for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        if (is<RenderMathMLBlock>(child)) {
-            auto* renderOperator = downcast<RenderMathMLBlock>(child)->unembellishedOperator();
-            if (renderOperator && renderOperator->isStretchy())
-                continue;
+        if (child->isOutOfFlowPositioned()) {
+            child->containingBlock()->insertPositionedObject(*child);
+            continue;
         }
-
+        if (toVerticalStretchyOperator(child))
+            continue;
         child->layoutIfNeeded();
+        LayoutUnit childAscent = ascentForChild(*child);
+        LayoutUnit childDescent = child->logicalHeight() - childAscent;
+        stretchAscent = std::max(stretchAscent, childAscent);
+        stretchDescent = std::max(stretchDescent, childDescent);
+    }
+    if (stretchAscent + stretchDescent <= 0) {
+        // We ensure a minimal stretch size.
+        stretchAscent = style().computedFontPixelSize();
+        stretchDescent = 0;
+    }
 
-        LayoutUnit childHeightAboveBaseline = ascentForChild(*child);
-        LayoutUnit childDepthBelowBaseline = child->logicalHeight() - childHeightAboveBaseline;
-
-        ascent = std::max(ascent, childHeightAboveBaseline);
-        descent = std::max(descent, childDepthBelowBaseline);
+    // Next, we stretch the vertical operators.
+    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        if (child->isOutOfFlowPositioned())
+            continue;
+        if (auto renderOperator = toVerticalStretchyOperator(child)) {
+            renderOperator->stretchTo(stretchAscent, stretchDescent);
+            renderOperator->layoutIfNeeded();
+        }
     }
+}
+
+void RenderMathMLRow::getContentBoundingBox(LayoutUnit& width, LayoutUnit& ascent, LayoutUnit& descent) const
+{
+    ascent = 0;
+    descent = 0;
+    width = borderAndPaddingStart();
+    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        if (child->isOutOfFlowPositioned())
+            continue;
 
-    // We ensure a minimal stretch size.
-    if (ascent + descent <= 0) {
-        ascent = style().computedFontPixelSize();
-        descent = 0;
+        width += child->marginStart() + child->logicalWidth() + child->marginEnd();
+        LayoutUnit childAscent = ascentForChild(*child);
+        LayoutUnit childDescent = child->logicalHeight() - childAscent;
+        ascent = std::max(ascent, childAscent + child->marginTop());
+        descent = std::max(descent, childDescent + child->marginBottom());
     }
+    width += borderEnd() + paddingEnd();
 }
 
 void RenderMathMLRow::computePreferredLogicalWidths()
@@ -93,79 +130,31 @@ void RenderMathMLRow::computePreferredLogicalWidths()
     m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = 0;
 
     LayoutUnit preferredWidth = 0;
-    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox())
+    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        if (child->isOutOfFlowPositioned())
+            continue;
         preferredWidth += child->maxPreferredLogicalWidth() + child->marginLogicalWidth();
+    }
 
     m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = preferredWidth + borderAndPaddingLogicalWidth();
 
     setPreferredLogicalWidthsDirty(false);
 }
 
-void RenderMathMLRow::layoutRowItems(LayoutUnit& ascent, LayoutUnit& descent)
+void RenderMathMLRow::layoutRowItems(LayoutUnit width, LayoutUnit ascent)
 {
-    // We first stretch the vertical operators.
-    // For inline formulas, we can then calculate the logical width.
-    LayoutUnit width = borderAndPaddingStart();
-    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        if (child->isOutOfFlowPositioned())
-            continue;
-
-        if (is<RenderMathMLBlock>(child)) {
-            auto renderOperator = downcast<RenderMathMLBlock>(child)->unembellishedOperator();
-            if (renderOperator && renderOperator->isStretchy() && renderOperator->isVertical())
-                renderOperator->stretchTo(ascent, descent);
-        }
-
-        child->layoutIfNeeded();
-
-        width += child->marginStart() + child->logicalWidth() + child->marginEnd();
-    }
-
-    width += borderEnd() + paddingEnd();
-    if ((!isRenderMathMLMath() || style().display() == INLINE))
-        setLogicalWidth(width);
-
-    LayoutUnit verticalOffset = borderTop() + paddingTop();
-    LayoutUnit maxAscent = 0, maxDescent = 0; // Used baseline alignment.
     LayoutUnit horizontalOffset = borderAndPaddingStart();
-    bool shouldFlipHorizontal = !style().isLeftToRightDirection();
     for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        if (child->isOutOfFlowPositioned()) {
-            child->containingBlock()->insertPositionedObject(*child);
+        if (child->isOutOfFlowPositioned())
             continue;
-        }
-        LayoutUnit childHorizontalExtent = child->logicalWidth();
-        LayoutUnit ascent = ascentForChild(*child);
-        LayoutUnit descent = child->verticalMarginExtent() + child->logicalHeight() - ascent;
-        maxAscent = std::max(maxAscent, ascent);
-        maxDescent = std::max(maxDescent, descent);
-        LayoutUnit childVerticalMarginBoxExtent = maxAscent + maxDescent;
-
         horizontalOffset += child->marginStart();
-
-        setLogicalHeight(std::max(logicalHeight(), verticalOffset + borderBottom() + paddingBottom() + childVerticalMarginBoxExtent + horizontalScrollbarHeight()));
-
-        LayoutPoint childLocation(shouldFlipHorizontal ? logicalWidth() - horizontalOffset - childHorizontalExtent : horizontalOffset, verticalOffset + child->marginTop());
-        child->setLocation(childLocation);
-
-        horizontalOffset += childHorizontalExtent + child->marginEnd();
+        LayoutUnit childAscent = ascentForChild(*child);
+        LayoutUnit childVerticalOffset = borderTop() + paddingTop() + child->marginTop() + ascent - childAscent;
+        LayoutUnit childWidth = child->logicalWidth();
+        LayoutUnit childHorizontalOffset = style().isLeftToRightDirection() ? horizontalOffset : width - horizontalOffset - childWidth;
+        child->setLocation(LayoutPoint(childHorizontalOffset, childVerticalOffset));
+        horizontalOffset += childWidth + child->marginEnd();
     }
-
-    LayoutUnit centerBlockOffset = 0;
-    if (style().display() == BLOCK)
-        centerBlockOffset = std::max<LayoutUnit>(0, (logicalWidth() - (horizontalOffset + borderEnd() + paddingEnd())) / 2);
-
-    if (shouldFlipHorizontal && centerBlockOffset > 0)
-        centerBlockOffset = -centerBlockOffset;
-
-    for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        LayoutUnit ascent = ascentForChild(*child);
-        LayoutUnit startOffset = maxAscent - ascent;
-        child->setLocation(child->location() + LayoutPoint(centerBlockOffset, startOffset));
-    }
-
-    ascent = maxAscent;
-    descent = maxDescent;
 }
 
 void RenderMathMLRow::layoutBlock(bool relayoutChildren, LayoutUnit)
@@ -175,16 +164,30 @@ void RenderMathMLRow::layoutBlock(bool relayoutChildren, LayoutUnit)
     if (!relayoutChildren && simplifiedLayout())
         return;
 
-    LayoutUnit ascent = 0;
-    LayoutUnit descent = 0;
-    computeLineVerticalStretch(ascent, descent);
-
     recomputeLogicalWidth();
 
     setLogicalHeight(borderAndPaddingLogicalHeight() + scrollbarLogicalHeight());
 
-    layoutRowItems(ascent, descent);
+    LayoutUnit width, ascent, descent;
+    stretchVerticalOperatorsAndLayoutChildren();
+    getContentBoundingBox(width, ascent, descent);
+
+    if (isRenderMathMLMath() && style().display() == BLOCK) {
+        // Display formulas must be centered horizontally.
+        layoutRowItems(logicalWidth(), ascent);
+        LayoutUnit centerBlockOffset = std::max<LayoutUnit>(0, logicalWidth() - width) / 2;
+        if (!style().isLeftToRightDirection())
+            centerBlockOffset = -centerBlockOffset;
+        for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+            if (!child->isOutOfFlowPositioned())
+                child->setLocation(child->location() + LayoutPoint(centerBlockOffset, 0));
+        }
+    } else {
+        layoutRowItems(width, ascent);
+        setLogicalWidth(width);
+    }
 
+    setLogicalHeight(borderTop() + paddingTop() + ascent + descent + borderBottom() + paddingBottom() + horizontalScrollbarHeight());
     updateLogicalHeight();
 
     layoutPositionedObjects(relayoutChildren);
index a32317a..d38b85f 100644 (file)
@@ -44,8 +44,9 @@ protected:
     void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) override;
     std::optional<int> firstLineBaseline() const override;
 
-    void layoutRowItems(LayoutUnit& ascent, LayoutUnit& descent);
-    void computeLineVerticalStretch(LayoutUnit& ascent, LayoutUnit& descent);
+    void stretchVerticalOperatorsAndLayoutChildren();
+    void getContentBoundingBox(LayoutUnit& width, LayoutUnit& ascent, LayoutUnit& descent) const;
+    void layoutRowItems(LayoutUnit width, LayoutUnit ascent);
     void computePreferredLogicalWidths() override;
 
 private: