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

[gsnedders]

Originally posted as w3c/csswg-test#1057 by @cbiesinger on 27 Feb 2016, 00:28 UTC:

[wpt-pr-bot]

Notifying @atanassov, @cbiesinger, @chenxix, @fantasai, @kojiishi, @mrego, @plinss, and @tabatkins. (Learn how reviewing works.)

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @syncbot on 27 Feb 2016, 00:28 UTC:

Automatic validation checks of commit 7fa4e32 discovered the following problem:

In css-flexbox-1/definite-cross-sizes.html:

• Linked to unversioned specification URL https://drafts.csswg.org/css-flexbox/#definite-sizes. Use https://www.w3.org/TR/css-flexbox-1/... or https://drafts.csswg.org/css-flexbox-1/... instead.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @syncbot on 27 Feb 2016, 00:30 UTC:

Automatic validation checks of commit 9dbf389 passed.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @dholbert on 07 Mar 2016, 23:19 UTC:

This testcase doesn't seem to set display:flex on anything, so I think it might not be testing what you want it to test... Right now it has:

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

I expect you want to have display:flex in there?

Also, might be worth giving all of the flex items all min-height:0, so that there's no chance of spuriously "passing" a piece of the test simply because we're stopping a flex item from shrinking below its intrinsic minimum size.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 07 Mar 2016, 23:21 UTC:

Oh that's a little subtle. The display: flex is set by the included stylesheet:
+<link rel="stylesheet" href="support/flexbox.css">

but the min-height is a good point. Will change.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @dholbert on 07 Mar 2016, 23:37 UTC:

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

css-flexbox-1/definite-cross-sizes.html, line 38 [r1] (raw file):
Nit: This sentence ("Tests that we ...") ends in a period, but all the ones after it do not.

Really, they might all want to end in a colon, since they're all referring to the content directly after them...?

Regardless, probably best to make these consistent.

---

css-flexbox-1/definite-cross-sizes.html, line 65 [r1] (raw file):
Nit: s/an definite/a definite/ (drop the "n" from "an")

---

Comments from the review on Reviewable.io

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @dholbert on 07 Mar 2016, 23:43 UTC:

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.

---

css-flexbox-1/definite-cross-sizes.html, line 1 [r1] (raw file):
This test's filename ("definite-cross-sizes") probably needs to include e.g. "001", right? Really, it might want to be named percentage-heights-001.html, since it's about percentage heights, and it's using the same check-layout-th.js library that the existing percentage-heights-000.html testcase uses.

---

css-flexbox-1/definite-cross-sizes.html, line 5 [r1] (raw file):
For consistency, I think this <style> block should come after all of the other boilerplate (the help/author tags in particular).

---

Comments from the review on Reviewable.io

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @dholbert on 07 Mar 2016, 23:46 UTC:

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.

---

css-flexbox-1/definite-cross-sizes.html, line 5 [r1] (raw file):
(er, I typed "this <style> block", but I forgot to include backticks, so reviewable ate the <style>.)

---

Comments from the review on Reviewable.io

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @syncbot on 27 Mar 2017, 20:26 UTC:

Automatic validation checks of commit 17af0dc passed.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:27 UTC:

css-flexbox-1/percentage-heights-001.html, line 38 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

Nit: This sentence ("Tests that we ...") ends in a period, but all the ones after it do not.

Really, they might all want to end in a colon, since they're all referring to the content directly after them...?

Regardless, probably best to make these consistent.

Done.

---

Comments from Reviewable

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:27 UTC:

css-flexbox-1/percentage-heights-001.html, line 5 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

(er, I typed "this <style> block", but I forgot to include backticks, so reviewable ate the <style>.)

Done.

---

Comments from Reviewable

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:27 UTC:

css-flexbox-1/percentage-heights-001.html, line 65 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

Nit: s/an definite/a definite/ (drop the "n" from "an")

Done.

---

Comments from Reviewable

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:28 UTC:

css-flexbox-1/percentage-heights-001.html, line 1 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

This test's filename ("definite-cross-sizes") probably needs to include e.g. "001", right? Really, it might want to be named percentage-heights-001.html, since it's about percentage heights, and it's using the same check-layout-th.js library that the existing percentage-heights-000.html testcase uses.

Done.

---

Comments from Reviewable

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:32 UTC:

All changes made, but please hold off while I verify this is all still correct.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @syncbot on 27 Mar 2017, 20:39 UTC:

Automatic validation checks of commit cc1f578 passed.

[wpt-issue-mover]

Originally posted as w3c/csswg-test#1057 (comment) by @cbiesinger on 27 Mar 2017, 20:40 UTC:

OK, this should be ready for merging -- updated to the current version of the spec (I think), and updated with a couple more tests from the current version of this testcase in blink.

r? @fantasai @dholbert

[ghost]

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 20e1787
Using browser at version BuildID 20170331102157; SourceStamp 8df9fabf2587b7020889755acb9e75b664fe13cf
Starting 10 test iterations
All results were stable

All results

2 tests ran

/css/css-flexbox-1/percentage-heights-001.html

Subtest	Results	Messages
	OK
.flexbox 1	PASS
.flexbox 2	PASS
.flexbox 3	PASS
.flexbox 4	PASS
.flexbox 5	FAIL	assert_equals: \n<div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 6	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 7	FAIL	assert_equals: \n<div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 8	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 9	PASS
.flexbox 10	PASS
.flexbox 11	PASS
.flexbox 12	PASS

/innerText/setter.html

Subtest	Results	Messages
	OK
Simplest possible test	PASS
Simplest possible test, detached	PASS
Newlines convert to <br> in non-white-space:pre elements	PASS
Newlines convert to <br> in non-white-space:pre elements, detached	PASS
Newlines convert to <br> in <pre> element	PASS
Newlines convert to <br> in <pre> element, detached	PASS
Newlines convert to <br> in <textarea> element	PASS
Newlines convert to <br> in <textarea> element, detached	PASS
Newlines convert to <br> in white-space:pre element	PASS
Newlines convert to <br> in white-space:pre element, detached	PASS
CRs convert to <br> in non-white-space:pre elements	PASS
CRs convert to <br> in non-white-space:pre elements, detached	PASS
CRs convert to <br> in <pre> element	PASS
CRs convert to <br> in <pre> element, detached	PASS
Newline/CR pair converts to <br> in non-white-space:pre element	PASS
Newline/CR pair converts to <br> in non-white-space:pre element, detached	PASS
Newline/newline pair converts to two <br>s in non-white-space:pre element	PASS
Newline/newline pair converts to two <br>s in non-white-space:pre element, detached	PASS
CR/CR pair converts to two <br>s in non-white-space:pre element	PASS
CR/CR pair converts to two <br>s in non-white-space:pre element, detached	PASS
CRs convert to <br> in white-space:pre element	PASS
CRs convert to <br> in white-space:pre element, detached	PASS
< preserved	PASS
< preserved, detached	PASS
> preserved	PASS
> preserved, detached	PASS
& preserved	PASS
& preserved, detached	PASS
" preserved	PASS
" preserved, detached	PASS
' preserved	PASS
' preserved, detached	PASS
innerText not supported on SVG elements	PASS
innerText not supported on SVG elements, detached	PASS
innerText not supported on MathML elements	PASS
innerText not supported on MathML elements, detached	PASS
Null characters preserved	PASS
Null characters preserved, detached	PASS
Tabs preserved	PASS
Tabs preserved, detached	PASS
Leading whitespace preserved	PASS
Leading whitespace preserved, detached	PASS
Trailing whitespace preserved	PASS
Trailing whitespace preserved, detached	PASS
Whitespace not compressed	PASS
Whitespace not compressed, detached	PASS
Existing text deleted	PASS
Existing text deleted, detached	PASS
Existing <br> deleted	PASS
Existing <br> deleted, detached	PASS
Assigning the empty string	PASS
Assigning the empty string, detached	PASS
Assigning null	PASS
Assigning null, detached	PASS
Assigning undefined	PASS
Assigning undefined, detached	PASS
Start with CR	PASS
Start with CR, detached	PASS
Start with LF	PASS
Start with LF, detached	PASS
Start with CRLF	PASS
Start with CRLF, detached	PASS
End with CR	PASS
End with CR, detached	PASS
End with LF	PASS
End with LF, detached	PASS
End with CRLF	PASS
End with CRLF, detached	PASS
innerText on <area> element	PASS
innerText on <area> element, detached	PASS
innerText on <base> element	PASS
innerText on <base> element, detached	PASS
innerText on <basefont> element	PASS
innerText on <basefont> element, detached	PASS
innerText on <bgsound> element	PASS
innerText on <bgsound> element, detached	PASS
innerText on <br> element	PASS
innerText on <br> element, detached	PASS
innerText on <col> element	PASS
innerText on <col> element, detached	PASS
innerText on <embed> element	PASS
innerText on <embed> element, detached	PASS
innerText on <frame> element	PASS
innerText on <frame> element, detached	PASS
innerText on <hr> element	PASS
innerText on <hr> element, detached	PASS
innerText on <image> element	PASS
innerText on <image> element, detached	PASS
innerText on <img> element	PASS
innerText on <img> element, detached	PASS
innerText on <input> element	PASS
innerText on <input> element, detached	PASS
innerText on <keygen> element	PASS
innerText on <keygen> element, detached	PASS
innerText on <link> element	PASS
innerText on <link> element, detached	PASS
innerText on <menuitem> element	PASS
innerText on <menuitem> element, detached	PASS
innerText on <meta> element	PASS
innerText on <meta> element, detached	PASS
innerText on <param> element	PASS
innerText on <param> element, detached	PASS
innerText on <source> element	PASS
innerText on <source> element, detached	PASS
innerText on <track> element	PASS
innerText on <track> element, detached	PASS
innerText on <wbr> element	PASS
innerText on <wbr> element, detached	PASS
innerText on <colgroup> element	PASS
innerText on <colgroup> element, detached	PASS
innerText on <frameset> element	PASS
innerText on <frameset> element, detached	PASS
innerText on <head> element	PASS
innerText on <head> element, detached	PASS
innerText on <html> element	PASS
innerText on <html> element, detached	PASS
innerText on <table> element	PASS
innerText on <table> element, detached	PASS
innerText on <tbody> element	PASS
innerText on <tbody> element, detached	PASS
innerText on <tfoot> element	PASS
innerText on <tfoot> element, detached	PASS
innerText on <thead> element	PASS
innerText on <thead> element, detached	PASS
innerText on <tr> element	PASS
innerText on <tr> element, detached	PASS

[ghost]

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 20e1787
Using browser at version 59.0.3053.3 dev
Starting 10 test iterations
All results were stable

All results

2 tests ran

/css/css-flexbox-1/percentage-heights-001.html

Subtest	Results	Messages
	OK
.flexbox 1	PASS
.flexbox 2	PASS
.flexbox 3	PASS
.flexbox 4	PASS
.flexbox 5	PASS
.flexbox 6	PASS
.flexbox 7	PASS
.flexbox 8	PASS
.flexbox 9	PASS
.flexbox 10	PASS
.flexbox 11	PASS
.flexbox 12	PASS

/innerText/setter.html

Subtest	Results	Messages
	OK
Simplest possible test	PASS
Simplest possible test, detached	PASS
Newlines convert to <br> in non-white-space:pre elements	PASS
Newlines convert to <br> in non-white-space:pre elements, detached	PASS
Newlines convert to <br> in <pre> element	FAIL	assert_equals: expected "abc<br>def" but got "abc\ndef"
Newlines convert to <br> in <pre> element, detached	PASS
Newlines convert to <br> in <textarea> element	FAIL	assert_equals: expected "abc<br>def" but got "abc\ndef"
Newlines convert to <br> in <textarea> element, detached	PASS
Newlines convert to <br> in white-space:pre element	FAIL	assert_equals: expected "abc<br>def" but got "abc\ndef"
Newlines convert to <br> in white-space:pre element, detached	PASS
CRs convert to <br> in non-white-space:pre elements	PASS
CRs convert to <br> in non-white-space:pre elements, detached	PASS
CRs convert to <br> in <pre> element	FAIL	assert_equals: expected "abc<br>def" but got "abc\ndef"
CRs convert to <br> in <pre> element, detached	PASS
Newline/CR pair converts to <br> in non-white-space:pre element	PASS
Newline/CR pair converts to <br> in non-white-space:pre element, detached	PASS
Newline/newline pair converts to two <br>s in non-white-space:pre element	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Newline/newline pair converts to two <br>s in non-white-space:pre element, detached	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
CR/CR pair converts to two <br>s in non-white-space:pre element	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
CR/CR pair converts to two <br>s in non-white-space:pre element, detached	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
CRs convert to <br> in white-space:pre element	FAIL	assert_equals: expected "abc<br>def" but got "abc\ndef"
CRs convert to <br> in white-space:pre element, detached	PASS
< preserved	PASS
< preserved, detached	PASS
> preserved	PASS
> preserved, detached	PASS
& preserved	PASS
& preserved, detached	PASS
" preserved	PASS
" preserved, detached	PASS
' preserved	PASS
' preserved, detached	PASS
innerText not supported on SVG elements	PASS
innerText not supported on SVG elements, detached	PASS
innerText not supported on MathML elements	PASS
innerText not supported on MathML elements, detached	PASS
Null characters preserved	PASS
Null characters preserved, detached	PASS
Tabs preserved	PASS
Tabs preserved, detached	PASS
Leading whitespace preserved	PASS
Leading whitespace preserved, detached	PASS
Trailing whitespace preserved	PASS
Trailing whitespace preserved, detached	PASS
Whitespace not compressed	PASS
Whitespace not compressed, detached	PASS
Existing text deleted	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
Existing text deleted, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
Existing <br> deleted	PASS
Existing <br> deleted, detached	PASS
Assigning the empty string	PASS
Assigning the empty string, detached	PASS
Assigning null	PASS
Assigning null, detached	PASS
Assigning undefined	PASS
Assigning undefined, detached	PASS
Start with CR	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Start with CR, detached	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Start with LF	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Start with LF, detached	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Start with CRLF	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
Start with CRLF, detached	FAIL	assert_not_equals: Should not have empty text nodes got disallowed value ""
End with CR	PASS
End with CR, detached	PASS
End with LF	PASS
End with LF, detached	PASS
End with CRLF	PASS
End with CRLF, detached	PASS
innerText on <area> element	PASS
innerText on <area> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <base> element	PASS
innerText on <base> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <basefont> element	PASS
innerText on <basefont> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <bgsound> element	PASS
innerText on <bgsound> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <br> element	PASS
innerText on <br> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <col> element	PASS
innerText on <col> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <embed> element	PASS
innerText on <embed> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <frame> element	PASS
innerText on <frame> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <hr> element	PASS
innerText on <hr> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <image> element	PASS
innerText on <image> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <img> element	PASS
innerText on <img> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <input> element	PASS
innerText on <input> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <keygen> element	PASS
innerText on <keygen> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <link> element	PASS
innerText on <link> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <menuitem> element	PASS
innerText on <menuitem> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <meta> element	PASS
innerText on <meta> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <param> element	PASS
innerText on <param> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <source> element	PASS
innerText on <source> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <track> element	PASS
innerText on <track> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <wbr> element	PASS
innerText on <wbr> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <colgroup> element	PASS
innerText on <colgroup> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <frameset> element	PASS
innerText on <frameset> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <head> element	PASS
innerText on <head> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <html> element	PASS
innerText on <html> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <table> element	PASS
innerText on <table> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <tbody> element	PASS
innerText on <tbody> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <tfoot> element	PASS
innerText on <tfoot> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <thead> element	PASS
innerText on <thead> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"
innerText on <tr> element	PASS
innerText on <tr> element, detached	FAIL	assert_not_equals: Child should be a *new* text node got disallowed value Text node "abc"

[dholbert]

At first glance, I have two superficial nits:

1. this still has 3 "an definite" typos in this file (percentage-heights-001.html on https://github.com/w3c/web-platform-tests/pull/5282/files ), it looks like. Those all need s/an definite/a definite/.
2. In many/most of the subtests here, the "expected" results are a half-filled green box, whereas the "failure" scenario would be an entirely-filled green box. That's a little problematic, because a fully-filled green box looks quite a lot like "everything is OK!". Consider using a non-green color for the backgrounds here, so that humans inspecting this test know to look beyond an entirely-filled-with-green box to see whether that entirely-filled-ness is actually expected or not.

Beyond that, I'll hold off until I'm clearer about best-practices for reviewing, per my questions just now over in #5281.

[gsnedders]

@dholbert fixed both of those

[ghost]

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 06b3a7c
Using browser at version BuildID 20170612100241; SourceStamp 27cad9749cddf68e11fdd4e5d73dad84a8f8cf23
Starting 10 test iterations
All results were stable

All results

1 test ran

/css/css-flexbox-1/percentage-heights-001.html

Subtest	Results	Messages
	OK
.flexbox 1	PASS
.flexbox 2	PASS
.flexbox 3	PASS
.flexbox 4	PASS
.flexbox 5	PASS
.flexbox 6	FAIL	assert_equals: \n<div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 7	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 8	FAIL	assert_equals: \n<div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 9	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 10	PASS
.flexbox 11	PASS
.flexbox 12	PASS
.flexbox 13	PASS

[ghost]

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 06b3a7c
Using browser at version 61.0.3128.3 dev
Starting 10 test iterations

[cbiesinger]

@dholbert ping? gsnedders fixed your comments here.

[dholbert]

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.

[cbiesinger]

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.

[cbiesinger]

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

[dholbert]

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?)

[dholbert]

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?)

[cbiesinger]

Yes, the first part.

[dholbert]

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.

[dholbert]

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.

[cbiesinger]

Will do

[cbiesinger]

@dholbert thanks for the review, can you take another look?

[ghost]

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 06b3a7c
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran

/css/css-flexbox-1/percentage-heights-001.html

Subtest	Results	Messages
	OK
.flexbox 1	PASS
.flexbox 2	FAIL	assert_equals: \n<div class="flexbox" style="height: 10%;" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 3	FAIL	assert_equals: \n<div class="flexbox" style="height: 10%;" data-expected-height="80">\n <div data-expected-height="80">\n <div style="height: 50%" data-expected-height="40">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 40 but got 50
.flexbox 4	FAIL	assert_equals: \n<div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="max-height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 5	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%; min-height: 30px;" data-expected-height="30">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 30 but got 50
.flexbox 6	FAIL	assert_equals: \n<div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 7	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox column" data-expected-height="50">\n <div class="flex-one" data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 8	FAIL	assert_equals: \n<div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\nheight expected 25 but got 50
.flexbox 9	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 10	FAIL	assert_equals: \n<div class="flexbox" style="height: 50px;" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 11	PASS
.flexbox 12	FAIL	assert_equals: \n<div class="flexbox wrap" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50
.flexbox 13	FAIL	assert_equals: \n<div class="flexbox" data-expected-height="50">\n <div data-expected-height="50">\n <div style="height: 50%" data-expected-height="25">\n <div class="rect" data-expected-height="50"></div>\n </div>\n </div>\n</div>\nheight expected 25 but got 50

[ghost]

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 06b3a7c
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran

/css/css-flexbox-1/percentage-heights-001.html

Subtest	Results	Messages
	OK
.flexbox 1	PASS
.flexbox 2	PASS
.flexbox 3	PASS
.flexbox 4	PASS
.flexbox 5	PASS
.flexbox 6	PASS
.flexbox 7	PASS
.flexbox 8	PASS
.flexbox 9	PASS
.flexbox 10	PASS
.flexbox 11	PASS
.flexbox 12	PASS
.flexbox 13	PASS

[dholbert]

@dholbert thanks for the review, can you take another look?

Sure -- I'll aim to get to this early next week.

[dholbert]

Thanks - looks like you addressed my comments, so I think this is good!

(I'm ticking "Approve" / "Submit review" which I think is what's needed here - let me know if there's anything else I should do.)

[cbiesinger]

Oh... I think you are not an official reviewer (?) for some reason, as far as Github is concerned. But I think I can do that? Let me try this...