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.
Created attachment 427629 [details] Patch
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?
Created attachment 427842 [details] Patch
Created attachment 427986 [details] Patch
Created attachment 428152 [details] Patch
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
<rdar://problem/77798383>
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.
(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.
Created attachment 428611 [details] Patch
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 ;)
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].