Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-flexbox-1] Add a test for definite cross sizes #5282

Merged
merged 4 commits into from
Jun 21, 2017
Merged

[css-flexbox-1] Add a test for definite cross sizes #5282

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e19868
[css-flexbox-1] Add a test for definite cross sizes
cbiesinger Feb 27, 2016
ae87cb2
fixup! [css-flexbox-1] Add a test for definite cross sizes
gsnedders May 12, 2017
db3c3b3
fixup! [css-flexbox-1] Add a test for definite cross sizes
gsnedders May 12, 2017
01f4d10
fixup! [css-flexbox-1] Add a test for definite cross sizes
cbiesinger Jun 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions css/css-flexbox-1/percentage-heights-001.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
<!DOCTYPE html>

<title>CSS Flexbox: Definite cross sizes</title>

<link rel="stylesheet" href="support/flexbox.css">
<link rel="help" href="https://drafts.csswg.org/css-flexbox-1/#definite-sizes">
<link rel="author" title="Google Inc." href="https://www.google.com/">
<meta name="flags" content="dom" />

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="support/check-layout-th.js"></script>

<style>
.rect {
width: 50px;
height: 50px;
background-color: blue;
}

.flexbox {
width: 50px;
outline: 3px solid black;
}


.flexbox > * {
min-height: 0;
min-width: 0;
}

.flexbox > div > div {
overflow: hidden;
}
</style>

<body onload="checkLayout('.flexbox')" style="height: 800px;">
<div id=log></div>


<p>This test verifies that we consider flex items' cross sizes to be definite
if the align value is stretch (the default)</p>

<p>Tests that we get a definite size in the simple case:</p>
<div class="flexbox" data-expected-height="50">
<div data-expected-height="50">
<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that we get a definite size in a wrapping flexbox:</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have "style="height: 50px;" here? The descriptive text implies that the only difference between this one & the previous one is the presence of flex-wrap:wrap (via the wrap class). But you've also got an explicit height here for some reason, not mentioned in the description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this a while ago but I think I wanted to make sure that even though https://drafts.csswg.org/css-flexbox/#definite-sizes #1 is limited to non-wrapping containers, that we do consider this definite due to #3. However, I think you're right that I should also have a test without an explicit height and a better description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, maybe the explicit height case isn't worth it, I'll delete it for now.

<div class="flexbox wrap" style="height: 50px;" data-expected-height="50">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have "style="height: 50px;" here? The descriptive text implies that the only difference between this one & the previous one is the presence of flex-wrap:wrap (via the wrap class). But you've also got an explicit height here for some reason, not mentioned in the description.

If you do need the explicit height for some reason (and I'm not sure you do), then you probably want the descriptive text to mention it -- e.g. "wrapping definite-height flexbox". (But I think the descriptive text only starts mentioning definite-height flexboxes a few examples down from this one -- so I'm guessing this one's just a mistake?)

<div data-expected-height="50">
<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that we get an indefinite size when not stretch-aligning:</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The descriptive text here probably wants to say "despite definite size on container". (Or maybe you want to just remove the definite "height: 50px" size on the container?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the first part.

<div class="flexbox wrap" style="height: 50px;" data-expected-height="50">
<div class="align-self-flex-start" data-expected-height="50">
<div style="height: 50%" data-expected-height="50">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that we get a definite size in a definite-height flexbox:</p>
<div class="flexbox" style="height: 50px;" data-expected-height="50">
<div data-expected-height="50">
<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that we get a definite size in a nested flexbox where only the outer
one has an explicit height:</p>
<div class="flexbox" style="height: 50px;" data-expected-height="50">
<div class="flexbox" data-expected-height="50">
<div data-expected-height="50">
<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>
</div>

<p>Tests that we get a definite size in a nested flexbox where only the outer
one has an explicit height and has an opposite direction:</p>
<div class="flexbox" style="height: 50px;" data-expected-height="50">
<div class="flexbox column" data-expected-height="50">
<div class="flex-one" data-expected-height="50">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description implies that the only difference between this example & the previous one is the "flex-direction" of the nested flexbox. But you've also got "flex-one" here for some reason (which isn't there in the previous example) -- that's one additional difference. Might be worth hinting at the this, in the descriptive text? (And/or adding an HTML comment to hint at the reason for it.) I don't immediately understand the significance of that difference.

<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>
</div>

<p>Tests that we respect min-height:</p>
Copy link
Contributor

@dholbert dholbert May 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps expand descriptive text to be a bit more specific, e.g.:
"Tests that we respect min-height, on child of a flex item with percent height that's treated as definite:"
Or:
"Tests that we respect min-height as a clamp for definite percentage height:"

ALSO: this makes me think -- it might be worth testing a percent min/max-height in this scenario (probably max-height since we're generally testing percent resolution by letting it produce a smaller size). e.g. we should have something like the very first example in this file, but with "max-height: 50%" instead of "height: 50%" on the child of the flex item. That should end up with a height of 25, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

<div class="flexbox" style="height: 50px;" data-expected-height="50">
<div data-expected-height="50">
<div style="height: 50%; min-height: 30px;" data-expected-height="30">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that percentage sizes can also be definite:</p>
<div class="flexbox" style="height: 10%;" data-expected-height="80">
<div data-expected-height="80">
<div style="height: 50%" data-expected-height="40">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>

<p>Tests that we use a definite size even when a percentage size is not definite</p>
<div>
<div class="flexbox" style="height: 10%;" data-expected-height="50">
<div data-expected-height="50">
<div style="height: 50%" data-expected-height="25">
<div class="rect" data-expected-height="50"></div>
</div>
</div>
</div>
</div>

<p>Tests that we don't mix up widths and heights</p>
<div class="flexbox" style="height: 50px; width: 100px;" data-expected-height="50">
<div style="width: 100px;" data-expected-height="50" data-expected-width="100">
<div style="width: 50%" data-expected-width="50">
<div class="rect" data-expected-height="50" data-expected-width="50"></div>
</div>
</div>
</div>