RESOLVED FIXED 225339
[css-flexbox] Wrong height of an empty table inside an orthogonal flex parent 
https://bugs.webkit.org/show_bug.cgi?id=225339
Summary [css-flexbox] Wrong height of an empty table inside an orthogonal flex parent
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
DetailsFormatted DiffDiff
Patch (4.75 KB, patch)
2021-05-05 19:54 PDT, Felipe Erias 		no flags
DetailsFormatted DiffDiff
Patch (7.76 KB, patch)
2021-05-07 03:13 PDT, Felipe Erias 		no flags
DetailsFormatted DiffDiff
Patch (7.83 KB, patch)
2021-05-09 21:19 PDT, Felipe Erias 		no flags
DetailsFormatted DiffDiff
Patch (7.86 KB, patch)
2021-05-14 01:35 PDT, Felipe Erias 		no flags
DetailsFormatted DiffDiff
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.