Skip to content

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

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbiesinger
Copy link
Contributor

@cbiesinger cbiesinger commented May 4, 2017

Taking over #5281 and fixing the issue pointed out by dholbert

@ghost
Copy link

ghost commented May 4, 2017

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision ab968bc
Using browser at version BuildID 20170503100422; SourceStamp 82c2d17e74ef9cdf38a5d5ac4eb3ae846ec30ba4
Starting 10 test iterations
All results were stable

All results

2 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

@ghost
Copy link

ghost commented May 4, 2017

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision ab968bc
Using browser at version 60.0.3088.3 dev
Starting 10 test iterations
All results were stable

All results

2 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

@codecov-io
Copy link

codecov-io commented May 4, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@845a5fe). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5791   +/-   ##
=========================================
  Coverage          ?   87.22%           
=========================================
  Files             ?       24           
  Lines             ?     2418           
  Branches          ?      406           
=========================================
  Hits              ?     2109           
  Misses            ?      240           
  Partials          ?       69

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 845a5fe...b6dd91a. Read the comment docs.

@wpt-pr-bot
Copy link
Collaborator

Notifying owners who are not repo collaborators: @atanassov and @chenxix.

@jgraham
Copy link
Contributor

jgraham commented May 5, 2017

@dholbert - Can you look at this?

It seems that no one is marked as an owner for this directory; it would be useful if people who can review flexbox tests were added to an OWNERS file in css/flexbox/.

@jgraham
Copy link
Contributor

jgraham commented May 5, 2017

Erm, sorry, I got that wrong.

There are no OWNERs here who are able to merge PRs. If someone is going to take responsibility for landing PRs here we should set you up with push access; please join #testing on irc.w3.org and we can sort something out.

@cbiesinger
Copy link
Contributor Author

@dholbert ping for review? this is the continuation of #5281 with your comment fixed.

@dholbert
Copy link
Contributor

dholbert commented May 20, 2017

@cbiesinger , sorry for the delay - I've been largely AFK for the past couple weeks. (Just got married, and then was on honeymoon. \o/ )

Looking at intrinsic-height-000.html and its reference: it intuitively makes sense, but I'm not sure how it corresponds to the spec text. (This might be an indication that the spec needs an adjustment, not the testcase -- but that probably needs to happen [or I need to correct my spec understanding] before this lands.)

In particular: you're testing section 9.9.1, but really that relies on 9.9.3 ("Flex Item Intrinsic Size Contributions") to define what the max-content contribution is for each flex item.

And in this case, I believe 9.9.3 says the max-content height contribution is ZERO for each flex item. Specifically: it says the contribution is the max-content size of the item (which is zero since there's no content and no specified height), and clamped by the flex-basis in some cases, but not in this case because the item has nonzero flex-grow and flex-shrink.

So when we apply 9.9.1 "subtract its outer flex base size from its max-content contribution size", I think we're subtracting the flex base sizes from zero, and then we end up using the flex-shrink factor below that, because we end up with a negative value. (0 - 200px and 0 - 100px)

With that, I think the spec ends up with an intrinsic height of zero for the flex container (with zero being contributed by each flex items), after I follow all the steps in 9.9.1 -- NOT 600px/200px/400px as your testcase expects.

Let me know if I'm just misunderstanding the spec somehow, though, and how you see it arriving at the values in your testcase. (In particular, is there any spec text that says the "max-content size" of each item is nonzero here? If there was [or if 9.9.1 had each item contributing something nonzero one way or another], then your testcase's expectations would make more sense.)

@dholbert
Copy link
Contributor

dholbert commented May 21, 2017

BTW: I filed w3c/csswg-drafts#1435 to clarify whether I'm correctly interpreting the spec & the spec authors' intentions here.

@cbiesinger
Copy link
Contributor Author

Yeah, hm. I think you're right. Let's wait for the outcome of that spec issue.

@gsnedders
Copy link
Member

w3c-test:mirror

@cbiesinger
Copy link
Contributor Author

(By the way, @dholbert, congratulations on getting married! Hope you had a great honeymoon!)

@gsnedders
Copy link
Member

w3c-test:mirror

@gregwhitworth
Copy link
Contributor

@gsnedders is there a status that states it's waiting on the spec changes to be completed, the decision was reached and width/height should be taken into account but not the flex-basis when determining the item's max-content contribution.

@gsnedders
Copy link
Member

If there's an agreement then we should just test for that and treat the spec being wrong as a bug, because we don't expect anyone to implement the spec without the resolution applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants