[css-flexbox] incorrect layout with max-height and flex-direction col…#49219
Conversation
wpt-pr-bot
requested review from
cbiesinger,
fantasai,
kojiishi,
plinss,
rachelandrew and
tabatkins
| <script src="/resources/testharnessreport.js"></script> | ||
| <script src="/resources/check-layout-th.js"></script> | ||
|
|
||
| Test passes if only the red square is visible. |
There was a problem hiding this comment.
Red is usually "wrong" on tests. As the expected result is a 100x100 square, maybe we could convert this in a reference test and check that it has the same output than ref-filled-green-100px-square.xht. Look for other tests in the same folder with:
<link rel="match" href="../reference/ref-filled-green-100px-square.xht" />There was a problem hiding this comment.
I have changed that the test passes only if there is a green square visible. I don't need to use ref-filled-green-100px-square.xht (I think) because validation is done by data-expected-height. The test I verified on local wpt with Chrome and FF (where it passes) and Epiphany/WebKitGTK/WPE (where it fails).
There was a problem hiding this comment.
I think I'd still make this a reftest and not use checkLayout() as it's not needed for this test.
About testing on the different engines, you can see the results for the different engines at: https://github.com/web-platform-tests/wpt/pull/49219/checks
@Loirooriol could you review this?
…umn and justify-content center Safari/WebKitGTK currently makes this flex item 200px tall. Firefox and Chrome give it the correct height of 100px. WebKit does not adjust the inner flex size to the max size specified in the outer flex container. After calculation of the flex item size and adjusting it to min/max size of the flex item WebKit should also check how the calculated size corresponds to specified max size of the whole container. In case the adjusted flex item size is bigger than the max size of the whole container WebKit should use the flex item size calculated before adjusting to flex item min/max size.
pgorszkowski-igalia
force-pushed
the
pgorszkowski/incorrect_layout_with_max-height_and_flex-direction_column_and_justify-content_center
branch
from
670fe75 to
7245189
Compare
|
|
||
| Test passes if only the green square is visible. | ||
|
|
||
| <div id="container" class="flexbox column" style="max-height: 100px; height: 100px; width: 100px;"> |
There was a problem hiding this comment.
Using both height: 100px and max-height: 100px seems unnecessary.
| <link href="support/flexbox.css" rel="stylesheet"> | ||
| <script src="/resources/testharness.js"></script> | ||
| <script src="/resources/testharnessreport.js"></script> | ||
| <script src="/resources/check-layout-th.js"></script> |
There was a problem hiding this comment.
Not a big deal, but I would also go with a reftest.
| Test passes if only the green square is visible. | ||
|
|
||
| <div id="container" class="flexbox column" style="max-height: 100px; height: 100px; width: 100px;"> | ||
| <div class="flexbox column justify-content-center" style="height: 400px; background: green" data-expected-height="100"><div></div></div> |
There was a problem hiding this comment.
It's fine as is, but maybe a bit weird to use justify-content: center when the centered item is 0px tall and invisible. It makes the test a bit larger, but to make justify-content: center more relevant you could do something like
<style>
#container {
height: 100px;
width: 100px;
}
#container > div {
height: 400px;
background: linear-gradient(to bottom, green 25px, red 25px 75px, green 75px 100px, red 100px);
}
#container > div > div {
height: 50px;
background: green;
}
</style>
<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
<div id="container" class="flexbox column">
<div class="flexbox column justify-content-center">
<div></div>
</div>
</div>In WebKit:
Up to you.
| @@ -0,0 +1,17 @@ | |||
| <!DOCTYPE html> | |||
| <link rel="author" title="Przemyslaw Gorszkowski" href="mailto:pgorszkowski@igalia.com"> | |||
| <link rel="help" href="https://bugs.webkit.org/show_bug.cgi?id=282036"> | |||
There was a problem hiding this comment.
You can add a <meta name="assert" saying that the nested flex container needs to shrink to be 100px tall, (and its item needs to be centered within these 100px, if you go with the suggestion above that exposes its position).
5 participants
…umn and justify-content center
Safarii/WebKitGTK currently makes this flex item 200px tall. Firefox and Chrome give it the correct height of 100px.
WebKit does not adjust the inner flex size to the max size specified in the outer flex container.
After calculation of the flex item size and adjusting it to min/max size of the flex item WebKit should also check how the calculated size corresponds to specified max size of the whole container. In case the adjusted flex item size is bigger than the max size of the whole container WebKit should use the flex item size calculated before adjusting to flex item min/max size.