Bug 225339

Summary:

[css-flexbox] Wrong height of an empty table inside an orthogonal flex parent

Product:

WebKit

Reporter:

Felipe Erias <felipeerias>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, changseok, clopez, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer, youennf, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/web-platform-tests/wpt/pull/28918

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Felipe Erias

Reported 2021-05-03 17:54:09 PDT

Failing test: imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-specified-height.html The flex box has vertical flow and the child table has horizontal writing direction, which means that the logical width for the flex box is the logical height for the child. But since the table is empty, the logical height calculated by RenderTable::layout() is just the "height" property. This value is stored as the child's intrinsic content logical height. Later on, RenderFlexibleBox::adjustChildSizeForMinAndMax() will try to find the child's min height through computeMainAxisExtentForChild(). Since the child's direction is orthogonal to the flexbox's direction, this will be done with a simple call to computeContentLogicalHeight() passing the previously cached value as the child's intrinsic content height. Because that cached value is assumed to be correct (even though the table is actually empty and could be shorter) the final flexbox layout is wrong.

Attachments
Patch (4.82 KB, patch) 2021-05-03 18:50 PDT, Felipe Erias	no flags	Details Formatted Diff Diff
Patch (4.75 KB, patch) 2021-05-05 19:54 PDT, Felipe Erias	no flags	Details Formatted Diff Diff
Patch (7.76 KB, patch) 2021-05-07 03:13 PDT, Felipe Erias	no flags	Details Formatted Diff Diff
Patch (7.83 KB, patch) 2021-05-09 21:19 PDT, Felipe Erias	no flags	Details Formatted Diff Diff
Patch (7.86 KB, patch) 2021-05-14 01:35 PDT, Felipe Erias	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Felipe Erias

Comment 1 2021-05-03 18:50:37 PDT

Created attachment 427629 [details] Patch

Sergio Villar Senin

Comment 2 2021-05-05 03:08:28 PDT

Comment on attachment 427629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427629&action=review > Source/WebCore/ChangeLog:15 > + layout problems (the table is actually empty and may be assigned a different height by its parent). This paragraph should go in between the "Reviewed by" and the "Test:" > Source/WebCore/rendering/RenderTable.cpp:538 > + setLogicalHeight(logicalHeight() + computedLogicalHeight); What about setLogicalHeight(hasOverridingLogicalHeight() ? overridingLogicalHeight() : logicalHeight() + computedLogicalHeight); we don't fear long lines in WebKit :) > Source/WebCore/rendering/RenderTable.cpp:609 > + This add a subtle change for the case of having an empty table with no overriding logical height. We used to cache that value anyway but now you are not doing it. Do we have test coverage for that?

Felipe Erias

Comment 3 2021-05-05 19:54:30 PDT

Created attachment 427842 [details] Patch

Felipe Erias

Comment 4 2021-05-07 03:13:33 PDT

Created attachment 427986 [details] Patch

Felipe Erias

Comment 5 2021-05-09 21:19:27 PDT

Created attachment 428152 [details] Patch

EWS Watchlist

Comment 6 2021-05-09 21:20:23 PDT

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Radar WebKit Bug Importer

Comment 7 2021-05-10 17:55:17 PDT

<rdar://problem/77798383>

Manuel Rego Casasnovas

Comment 8 2021-05-14 00:56:14 PDT

Comment on attachment 428152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428152&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-specified-width-vertical.html:1 > +<!DOCTYPE html> You have to add a -expected.txt file with the same name than this test. So adding table-as-item-specified-width-vertical-expected.html that is a copy of ref-filled-green-100px-square-only.html. That's what the importer would do on that case.

Manuel Rego Casasnovas

Comment 9 2021-05-14 01:03:02 PDT

(In reply to Manuel Rego Casasnovas from comment #8) > Comment on attachment 428152 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428152&action=review > > > LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-specified-width-vertical.html:1 > > +<!DOCTYPE html> > > You have to add a -expected.txt file with the same name than this test. So > adding table-as-item-specified-width-vertical-expected.html that is a copy > of ref-filled-green-100px-square-only.html. That's what the importer would > do on that case. Oops sorry, it's there already.

Felipe Erias

Comment 10 2021-05-14 01:35:44 PDT

Created attachment 428611 [details] Patch

Sergio Villar Senin

Comment 11 2021-05-14 02:46:17 PDT

Comment on attachment 428611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428611&action=review Looking good but let's try to land the patch in WPT first so that there is no disagreement with other vendors. Table stuff is really hard WRT interoperability. > LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/table-as-item-specified-width-vertical-expected.html:2 > +<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org"> I guess you're the author ;)

EWS

Comment 12 2021-05-20 01:00:28 PDT

Committed r277777 (237938@main): <https://commits.webkit.org/237938@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428611 [details].

Note You need to log in before you can comment on or make changes to this bug.