Changeset 286593 in webkit

Timestamp:

Dec 7, 2021 2:20:24 AM (8 months ago)

Author:

svillar@igalia.com

Message:

[css-flexbox] Account for captions when flexing tables with specified sizes
​https://bugs.webkit.org/show_bug.cgi?id=233814

Reviewed by Darin Adler.

Source/WebCore:

Flexing tables is complex because of the specifics of the table layout algorithms. There are
two interrelated issues that were causing some failures in flexbox WPT tests.

The first one is that tables interpret overriding sizes (flexed sizes) as the sizes of rows + captions.
However the specified height of a table only accounts for the heights of the rows, not the captions. That's
why when setting the flexed height of a table we must add up the specified height of the table and the height
of the captions. The table algorithm will properly substract the captions' size in order to compute the
available height for rows.

The second issue is that the table layout algorithm adds the size of the bottom border at the very end
of its execution after performing all the computations and it does it unconditionally. The thing is that
the flexbox code already takes into account the borders and paddings so we basically have to substract that
border when setting the height of the table, because the table layout will add it later.

• rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::computeMainAxisExtentForChild):

• rendering/RenderTable.cpp:

(WebCore::RenderTable::computeCaptionsLogicalHeight const): New method refactored from current code.
(WebCore::RenderTable::layout):

• rendering/RenderTable.h: Expose computeCaptionsLogicalHeight.

LayoutTests:

• TestExpectations: Unskipped two tests that are now passing

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderFlexibleBox.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderTable.cpp (modified) (3 diffs)
• Source/WebCore/rendering/RenderTable.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-12-03  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] Account for captions when flexing tables with specified sizes
+        https://bugs.webkit.org/show_bug.cgi?id=233814
+
+        Reviewed by Darin Adler.
+
+        * TestExpectations: Unskipped two tests that are now passing
+
 2021-12-07  Martin Robinson  <mrobinson@webkit.org>
 

trunk/LayoutTests/TestExpectations

@@ -4225 +4225 @@
 
 # Tables as flex items.
-webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-inflexible-in-column-1.html [ ImageOnlyFailure ]
-webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-inflexible-in-column-2.html [ ImageOnlyFailure ]
 webkit.org/b/221473 imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-min-height-1.html [ ImageOnlyFailure ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-12-03  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] Account for captions when flexing tables with specified sizes
+        https://bugs.webkit.org/show_bug.cgi?id=233814
+
+        Reviewed by Darin Adler.
+
+        Flexing tables is complex because of the specifics of the table layout algorithms. There are
+        two interrelated issues that were causing some failures in flexbox WPT tests.
+
+        The first one is that tables interpret overriding sizes (flexed sizes) as the sizes of rows + captions.
+        However the specified height of a table only accounts for the heights of the rows, not the captions. That's
+        why when setting the flexed height of a table we must add up the specified height of the table and the height
+        of the captions. The table algorithm will properly substract the captions' size in order to compute the
+        available height for rows.
+
+        The second issue is that the table layout algorithm adds the size of the bottom border at the very end
+        of its execution after performing all the computations and it does it unconditionally. The thing is that
+        the flexbox code already takes into account the borders and paddings so we basically have to substract that
+        border when setting the height of the table, because the table layout will add it later.
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::computeMainAxisExtentForChild):
+        * rendering/RenderTable.cpp:
+        (WebCore::RenderTable::computeCaptionsLogicalHeight const): New method refactored from current code.
+        (WebCore::RenderTable::layout):
+        * rendering/RenderTable.h: Expose computeCaptionsLogicalHeight.
+
 2021-12-07  Martin Robinson  <mrobinson@webkit.org>
 

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -41 +41 @@
 #include "RenderReplaced.h"
 #include "RenderStyleConstants.h"
+#include "RenderTable.h"
 #include "RenderView.h"
 #include "WritingMode.h"
@@ -653 +654 @@
         if (!height)
             return height;
-        return height.value() + child.scrollbarLogicalHeight();
+        // Tables interpret overriding sizes as the size of captions + rows. However the specified height of a table
+        // only includes the size of the rows. That's why we need to add the size of the captions here so that the table
+        // layout algorithm behaves appropiately.
+        LayoutUnit captionsHeight;
+        if (is<RenderTable>(child) && childMainSizeIsDefinite(child, size))
+            captionsHeight = downcast<RenderTable>(child).sumCaptionsLogicalHeight();
+        return *height + child.scrollbarLogicalHeight() + captionsHeight;
     }
 

trunk/Source/WebCore/rendering/RenderTable.cpp

@@ -420 +420 @@
 }
 
+LayoutUnit RenderTable::sumCaptionsLogicalHeight() const
+{
+    LayoutUnit height;
+    for (auto& caption : m_captions)
+        height += caption->logicalHeight() + caption->marginBefore() + caption->marginAfter();
+    return height;
+}
+
 void RenderTable::layout()
 {
@@ -506 +514 @@
             computedLogicalHeight = convertStyleLogicalHeightToComputedHeight(logicalHeightLength);
 
-        if (hasOverridingLogicalHeight()) {
-            LayoutUnit captionLogicalHeight;
-            for (auto& caption : m_captions)
-                captionLogicalHeight += caption->logicalHeight() + caption->marginBefore() + caption->marginAfter();
-            computedLogicalHeight = std::max(computedLogicalHeight, overridingLogicalHeight() - captionLogicalHeight);
-        }
+        if (hasOverridingLogicalHeight())
+            computedLogicalHeight = std::max(computedLogicalHeight, overridingLogicalHeight() - borderAndPaddingAfter - sumCaptionsLogicalHeight());
 
         Length logicalMaxHeightLength = style().logicalMaxHeight();
@@ -534 +538 @@
             // overriding or specified height in strict mode, but this value will not be cached.
             shouldCacheIntrinsicContentLogicalHeightForFlexItem = false;
-            setLogicalHeight(hasOverridingLogicalHeight() ? overridingLogicalHeight() : logicalHeight() + computedLogicalHeight);
+            setLogicalHeight(hasOverridingLogicalHeight() ? overridingLogicalHeight() - borderAndPaddingAfter : logicalHeight() + computedLogicalHeight);
         }
 

trunk/Source/WebCore/rendering/RenderTable.h

@@ -272 +272 @@
     void willInsertTableSection(RenderTableSection& child, RenderObject* beforeChild);
 
+    LayoutUnit sumCaptionsLogicalHeight() const;
+
 protected:
     void styleDidChange(StyleDifference, const RenderStyle* oldStyle) final;