Bug 221473 - [css-flexbox] Tables as flex items issues
Summary: [css-flexbox] Tables as flex items issues
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 225343 224665 224934
Blocks: 136754
  Show dependency treegraph
 
Reported: 2021-02-05 09:20 PST by Sergio Villar Senin
Modified: 2021-05-04 00:34 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2021-04-16 04:51 PDT, Felipe Erias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2021-02-05 09:20:17 PST
Several failures in newly imported WPT tests:

imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-fixed-min-width-3.html
imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-flex-cross-size.html
imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-percent-width-cell-001.html
imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-specified-width.html
imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-stretch-cross-size.html
imported/w3c/web-platform-tests/css/css-flexbox/table-item-flex-percentage-width.html
Comment 1 Radar WebKit Bug Importer 2021-02-12 09:21:14 PST
<rdar://problem/74278994>
Comment 2 Felipe Erias 2021-04-13 05:27:05 PDT
A few notes about the failing tests:

— table-as-item-fixed-min-width-3

This test creates a flex layout with a table and a div. The table claims to have a min-width of 50px, but after layout it becomes wider as its only cell contains a div with a width of 100px.
  
The flex algorithm uses the min-width of the table to allocate the available space. After layout, the table takes up more space than expected, causing the second flex child to overflow the container.

— table-as-item-flex-cross-size

The flex container (row direction) sets an overriding logical height on its table child (cross axis from the point of view of the container). However, the table does not actually grow because it ignores that overriding value.

— table-as-item-percent-width-cell-001

The table ignores the overriding logical width placed by its flex container and instead takes all the available space and gives it to its first cell (which has width:100%).

Furthermore, the different values of flex-basis (content, min-content, max-content) are ignored, so the outcome is the same in all cases.

— table-as-item-specified-width

The element has flex-basis:100px with flex-grow and flex-shrink both set to 0. This means that its width should stay at 100px and don't change.

However, RenderTable::updateLogicalWidth() erroneously discards the 100px overriding width set by the flex container and uses the width value from the style instead (500px).

— table-as-item-stretch-cross-size

The flex container (column direction) sets an overriding logical height on its table child (main axis for the container). Like before, the table does not actually grow because it ignores that overriding value.

— table-item-flex-percentage-width

This test creates a series of flex containers with display:table children.

Several of those do not have the expected outcome, mainly because the content's width (set as a percentage) takes precedence over its flex-basis and does not change as a result of flex-shrink or flex-grow.
Comment 3 Felipe Erias 2021-04-16 04:51:59 PDT
Created attachment 426208 [details]
Patch
Comment 4 Sergio Villar Senin 2021-04-16 05:04:41 PDT
Comment on attachment 426208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426208&action=review

Thanks for the patch!

Every patch to WebKit needs a test. If this is covered by WPT tests then you need to remove them from TestExpectations file.

> Source/WebCore/rendering/RenderTable.cpp:513
> +            for (unsigned i = 0; i < m_captions.size(); ++i) {

You can write this like

for (auto& caption : m_captions)

> Source/WebCore/rendering/RenderTable.cpp:517
> +            LayoutUnit contentOverridingLogicalHeight = overridingLogicalHeight() - captionLogicalHeight;

Not sure why you label this as contentOverridingLogicalHeight. overridingLogicalHeight() refers to the border box, meaning that it includes border and padding so if you really want the content height you have to use overridingContentLogicalHeight()
Comment 5 Sergio Villar Senin 2021-04-16 08:14:13 PDT
Comment on attachment 426208 [details]
Patch

Moved to bug 224665