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

Jump to bottom

[css-flexbox] Add tests for intrinsic sizing behavior #5281

Closed
wants to merge 1 commit into from
Closed

[css-flexbox] Add tests for intrinsic sizing behavior #5281

wants to merge 1 commit into from

Conversation

gsnedders
Copy link
Member

@gsnedders gsnedders commented Mar 31, 2017 •
edited by wpt-pr-bot
Loading

Originally posted as w3c/csswg-test#1022 by @cbiesinger on 26 Jan 2016, 01:44 UTC:

@wpt-pr-bot
Copy link
Collaborator

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

@wpt-issue-mover
Copy link

Originally posted as w3c/csswg-test#1022 (comment) by @syncbot on 26 Jan 2016, 01:44 UTC:

Automatic validation checks of commit f0d3a1f passed.

@wpt-issue-mover
Copy link

Originally posted as w3c/csswg-test#1022 (comment) by @syncbot on 09 Feb 2016, 19:40 UTC:

Automatic validation checks of commit a6416d9 passed.

@wpt-issue-mover
Copy link

Originally posted as w3c/csswg-test#1022 (comment) by @cbiesinger on 09 Feb 2016, 19:41 UTC:

@tabatkins @fantasai - can one of you review this?

@wpt-issue-mover
Copy link

Originally posted as w3c/csswg-test#1022 (comment) by @dholbert on 07 Mar 2016, 22:36 UTC:

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

css-flexbox-1/intrinsic-height-000-ref.html, line 3 [r1] (raw file):
This title is probably too vague; it doesn't really say what the test is testing.

I think you mean to say, "Test that flex container's intrinsic size is influenced by its items' flex-grow values" or something like that?

css-flexbox-1/intrinsic-height-000-ref.html, line 30 [r1] (raw file):
The phrase "next to" is wrong here, for the height-000 testcase & its reference (which use vertical flex containers whose items are stacked vertically).

I think you mean to say "above" instead of "next to".

css-flexbox-1/intrinsic-width-000.html, line 18 [r1] (raw file):
It seems here you're assuming that a flex item with "width: 200px; flex: 1 1 0" has a max-content contribution of 200px. I don't think that's correct. The spec defines flex items' min-content and max-content contributions as being determined by their intrinsic sizes, flex-basis, and min/max-width|height properties, here: https://drafts.csswg.org/css-flexbox-1/#intrinsic-item-contributions . I don't think that spec-text has any way for "width" or "height" to influence the item's max-content contribution, in your example here (with flex-basis:0)...

Comments from the review on Reviewable.io

@wpt-issue-mover
Copy link

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

Automatic validation checks of commit 6039f5a passed.

@wpt-issue-mover
Copy link

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

All comments fixed, please take a look! @fantasai @dholbert

@wpt-issue-mover
Copy link

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

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

css-flexbox-1/intrinsic-height-000-ref.html, line 3 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

This title is probably too vague; it doesn't really say what the test is testing.

I think you mean to say, "Test that flex container's intrinsic size is influenced by its items' flex-grow values" or something like that?

Done.

css-flexbox-1/intrinsic-height-000-ref.html, line 30 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

The phrase "next to" is wrong here, for the height-000 testcase & its reference (which use vertical flex containers whose items are stacked vertically).

I think you mean to say "above" instead of "next to".

Done.

css-flexbox-1/intrinsic-width-000.html, line 18 at r1 (raw file):

Previously, dholbert (Daniel Holbert) wrote…

It seems here you're assuming that a flex item with "width: 200px; flex: 1 1 0" has a max-content contribution of 200px. I don't think that's correct. The spec defines flex items' min-content and max-content contributions as being determined by their intrinsic sizes, flex-basis, and min/max-width|height properties, here: https://drafts.csswg.org/css-flexbox-1/#intrinsic-item-contributions . I don't think that spec-text has any way for "width" or "height" to influence the item's max-content contribution, in your example here (with flex-basis:0)...

Done.

Comments from Reviewable

@wpt-issue-mover wpt-issue-mover mentioned this pull request Mar 31, 2017
Closed
@ghost
Copy link

ghost commented Mar 31, 2017

View the complete job log.

Firefox (nightly channel)

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

All results

3 tests ran
/css/css-flexbox-1/intrinsic-height-000.html
Subtest Results Messages
FAIL
/css/css-flexbox-1/intrinsic-width-000.html
Subtest Results Messages
FAIL
/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
Copy link

ghost commented Mar 31, 2017

View the complete job log.

Chrome (unstable channel)

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

All results

3 tests ran
/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"
/css/css-flexbox-1/intrinsic-height-000.html
Subtest Results Messages
FAIL
/css/css-flexbox-1/intrinsic-width-000.html
Subtest Results Messages
FAIL

@dholbert
Copy link
Contributor

dholbert commented Apr 28, 2017 •
edited
Loading

Sorry, this is going to be a bit clunky -- I'm not familiar with the GitHub code-review experience.
Questions:

On reviewing in general:

  1. What's the recommended UI for leaving review comments on lines of code at this point? The "Reviewable" button at the top of this review request seems to take me to the wrong page, since it says "branch abandoned" at the top right.
  2. Is there a better way for me to actually get the files (and their resources) locally, aside from copypasting IDs into the "Modifying an inactive pull request locally" longform steps at this help page? (That page's initial "modifying an active pull request locally" section says I should see a "command line instructions" button on this pull-request page somewhere, but that's not there for me. I think maybe GitHub only shows that to people who have merge privileges? I don't want to have merge privileges, but I would like to be able to view files that I'm reviewing locally.)

On the pull request itself:

  1. Are these tests intended to actually pass right now? I tested in Chrome & Firefox, and we seem to agree on the rendering of each of the 4 files, though the testcases do not match their reference cases. In particular, in Firefox 55 Nightly and Chrome 59 dev:
  • intrinsic-width-000.html shows no colored boxes (they are 0 width).
  • intrinsic-height-000.html shows two colorful boxes which are each 200px tall, whereas the reference case (and the text) suggests that one is supposed to be 400px tall.
    I haven't dug into the details of why they fail yet because I'm not clear if they're expected to pass or fail at this point. I didn't test them on Edge because that would've required me to post these files publicly somewhere or copy the whole clunkily-cloned-repo between machines.
  1. One actual actionable nit -- the intrinsic-height-* files have a typo. They say "You should see a 200px wide green rectangle above a 400px wide blue rectangle" (copypasted from the "width" testcases). That description is incorrect -- they need s/wide/tall/, and s/above/alongside/

@dholbert dholbert mentioned this pull request Apr 28, 2017
Merged
@gsnedders
Copy link
Member Author

@dholbert

Questions:

On reviewing in general:

  1. What's the recommended UI for leaving review comments on lines of code at this point? The "Reviewable" button at the top of this review request seems to take me to the wrong page, since it says "branch abandoned" at the top right.

As I mentioned on IRC yesterday: the Reviewable link is that from the old csswg-test repo, so that's broken on the PRs imported from there.

  1. Is there a better way for me to actually get the files (and their resources) locally, aside from copypasting IDs into the "Modifying an inactive pull request locally" longform steps at this help page? (That page's initial "modifying an active pull request locally" section says I should see a "command line instructions" button on this pull-request page somewhere, but that's not there for me. I think maybe GitHub only shows that to people who have merge privileges? I don't want to have merge privileges, but I would like to be able to view files that I'm reviewing locally.)

Yeah, looking around, I think you need merge permissions… That's a nice bug in the docs. I always just go for git fetch origin pull/ID/head && git checkout FETCH_HEAD, which is the steps for inactive PRs.

There's also http://w3c-test.org/submissions/ which contains mirrored copies of PRs if you just want to run them.

@cbiesinger
Copy link
Contributor

@dholbert -- no, they're not expected to pass right now. They are testing the wording in https://drafts.csswg.org/css-flexbox/#intrinsic-main-sizes which I don't think any browser implements yet.

I will try to fix your other comment as soon as I figure out a good way to do that...

@cbiesinger
Copy link
Contributor

Hmm I think since @gsnedders created this pull request and I'm not a repository maintainer, there's no way for me to commit anything to this request, and I have to create a new one.

@cbiesinger cbiesinger mentioned this pull request May 4, 2017
Open
@cbiesinger
Copy link
Contributor

See #5791 for the height issue fixed (the "above" part should be correct as it is)

@dholbert
Copy link
Contributor

Quoting @gsnedders:

There's also http://w3c-test.org/submissions/ which contains
mirrored copies of PRs if you just want to run them.

Hmm, darn -- that page doesn't have an entry for this pull request (#5281) or for its continuation (#5791). Is that a bug?

In any case, I'll just use the longhand commands to check out the files locally, I suppose, so I can run/view them.

@wpt-pr-bot wpt-pr-bot requested review from atanassov and chenxix May 20, 2017 17:11
@gsnedders
Copy link
Member Author

This has all moved to #5791.

@gsnedders gsnedders closed this May 24, 2017
@gsnedders gsnedders deleted the csswg-test-pr-1022 branch May 24, 2017 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrego mrego Awaiting requested review from mrego

@tabatkins tabatkins Awaiting requested review from tabatkins

@fantasai fantasai Awaiting requested review from fantasai

@kojiishi kojiishi Awaiting requested review from kojiishi

@cbiesinger cbiesinger Awaiting requested review from cbiesinger

@plinss plinss Awaiting requested review from plinss

@atanassov atanassov Awaiting requested review from atanassov

@chenxix chenxix Awaiting requested review from chenxix

Assignees
No one assigned
Labels
css-flexbox wg-css
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gsnedders @wpt-pr-bot @wpt-issue-mover @dholbert @cbiesinger