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.

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

flex container's intrinsic width and stretched images #28805

Open
davidsgrogan opened this issue May 4, 2021 · 7 comments
Open

flex container's intrinsic width and stretched images #28805

davidsgrogan opened this issue May 4, 2021 · 7 comments

Comments

@davidsgrogan
Copy link
Member

@davidsgrogan davidsgrogan commented May 4, 2021

Hi flex-interested people (@svillar @aethanyc @dholbert @bfgeek),

Blink is going to change how stretched aspect-ratio flex items ("images") contribute to a flex container's intrinsic width. When an image has a stretched height such that it qualifies as definite via flex 9.8.1, we'll now pass the height through the aspect ratio to find the image's intrinsic contribution.

E.g. https://jsfiddle.net/dgrogan/mjptyxe5/ :

<div style="display: flex; height: 100px; float: left">
  <img src="support/60x60-green.png" style="min-width: 0;"/>
</div>

All engines currently show a 60x100 green rectangle. Blink will soon (hopefully Chrome 92, which goes to stable mid-July) show 100x100.

This affects css/css-flexbox/flexbox-min-width-auto-00{5,6}.html
I think we are going to leave those tests unchanged and mark them failing in our tree until we know this change is web compatible.

Thoughts? Concerns?

@dholbert
Copy link
Contributor

@dholbert dholbert commented May 4, 2021

My initial reaction is that this seems reasonable, if it's web compatible.

Thinking a bit more... I wonder if there'll be some weird interplay, in cases where the stretch height isn't immediately available until after we've done some layout. (i.e. if we don't have a definite height up-front, but we do eventually have one). This could end up making this element have two different intrinsic width contributions at two different points in the flex algorithm.

Consider e.g. this example, where I've basically stuck your jsfiddle inside of a column-oriented flex container:
https://jsfiddle.net/dholbert/3c2vbgdy/

.colflex {
  display: flex;
  flex-direction: column;
  height: 200px;
  width: max-content;
  border: 2px solid fuchsia;
}
.text {
  border: 2px solid gray;
}
.rowflex {
  display: flex;
  height: 50px;
  flex: 1 0 50px;
  
  border: 2px solid teal;
  width: max-content;
}

* {
  /* Turn off automatic minimum sizes, to reduce the amount of potential
     magic behavior that's in play: */
  min-width: 0;
  min-height: 0;
}
<div class="colflex">
  <div class="text">
  Text
  </div>
  <div class="rowflex">
    <img src="//placekitten.com/60" style="min-width: 0;"/>
  </div>
</div>

In this scenario, we first have to compute the intrinsic width of the outer flex container, which is based on the intrinsic width contributions of its descendants, which ends up being the intrinsic width of the image (or the text, if it's wider).

Then we can do layout in the outer flex container, which ends up stretching rowflex vertically (in the outer flex container's main axis) and giving it a definite height (of around 180px), per https://drafts.csswg.org/css-flexbox/#definite-sizes part 2.

Then, when we do layout inside the inner flex container, we may start out by asking for its intrinsic width. With your change, we would find that it now has a significantly larger intrinsic width than what we initially thought, because its image flex item would suddenly have a larger intrinsic width contribution (due to being stretched to ~180px and transferring that through to its intrinsic size contribution, per your new rule).

I'm not sure what the outcome of this would be, but I think rowflex and/or its image would end up overflowing and being wider than the outer flex container (colflex), despite that outer container being sized to its contents. Whereas with current implemenetations, this ends up being well-defined and things stay contained in their intrinsically-sized ancestors.

What do you think about this sort of scenario and what this change means for it, @davidsgrogan ?

@davidsgrogan
Copy link
Member Author

@davidsgrogan davidsgrogan commented May 6, 2021

Yeah, there is weird interplay in this case. Your guess on the outcome is right: rowflex overflows colflex in the inline direction. Screenshot from a local build:
rowflex-overflows-colflex

The new behavior for this case is weird. I think it's consistent with the rest of CSS though*, and the funkiness comes from flex's 2-pass-edness. I'm not sure what alternate behavior we'd want to specify. Like, suppose we remove the stretching from your example above by adding these two lines:

.rowflex {
  align-items: flex-start;
}
img {
  height: 100%;
}
  1. The rendering of this modified example is more straightforward to me: it should render like the screenshot because block constraints are supposed to affect the img's intrinsic width.
  2. Then, I think this modified example should render the same as your original example. The height: 100% resolved against definite heights gives the same behavior as stretching here.
  3. If taken together, (1) and (2) imply that the screenshot is right for your original example.

So, in sum, this case is weird to me too, but after I went through (1) and (2), I think it's just an unfortunate side effect of making block sizes affect intrinsic inline sizes, which seems like a positive change overall.

* We're making replaced-with-aspect-ratio elements' intrinsic sizes in one dimension obey the transferred constraints from the other dimension (First note in https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes). This new flex behavior falls out of that change.

@aethanyc
Copy link
Contributor

@aethanyc aethanyc commented May 7, 2021

When an image has a stretched height such that it qualifies as definite via flex 9.8.1, we'll now pass the height through the aspect ratio to find the image's intrinsic contribution.

This means the stretched cross-size transfers to the main-size via aspect-ratio, doesn't it? I think this behavior may violate the spec. In flex algo step 11: "Determine the used cross size of each flex item", it says:

If the flex item has align-self: stretch, redo layout for its contents, treating this used size as its
definite cross size so that percentage-sized children can be resolved.

Note that this step does not affect the main size of the flex item, even if it has a preferred aspect ratio.

I blamed the flexbox spec, and found the edit came from https://lists.w3.org/Archives/Public/www-style/2012Oct/0781.html

@davidsgrogan
Copy link
Member Author

@davidsgrogan davidsgrogan commented May 7, 2021

This means the stretched cross-size transfers to the main-size via aspect-ratio, doesn't it? I think this behavior may violate the spec. In flex algo step 11 [snip]

Yes, the stretched cross-size is transferring to the main-size via aspect ratio. But not because of step 11 (not even in layout at all). Just via 9.8.1, which takes effect before step 11.

I take the note at step 11 to mean that when you stretch at step 11, you don't stretch the main axis through the aspect ratio.

It's the same reason that both Chrome and Firefox make this image 100x100 instead of 16x100. And what fantasai talks about in w3c/csswg-drafts#5060 (comment) , emphasis mine:

flex item having a definite cross size immediately (because it's stretched, and the flex container is single-line and has a definite cross size), so the aspect-ratio should activate

So, assuming 9.8.1 is in effect during intrinsic width calculation, just like it's in effect in layout before step 11, then the new behavior is just another consequence of 9.8.1.

I blamed the flexbox spec, and found the edit came from https://lists.w3.org/Archives/Public/www-style/2012Oct/0781.html

Good find. The case there is similar to dholbert's, and argues for the existing rendering. But 9.8.1 effectively does what that thread said the spec shouldn't do:

The obvious solution here is to move 'stretch' resolution up, to before the current line-breaking and flexing steps

, which is in contrast to the current spec and fantasai's linked comment.

@bfgeek
Copy link
Contributor

@bfgeek bfgeek commented May 7, 2021

Consider e.g. this example, where I've basically stuck your jsfiddle inside of a column-oriented flex container:
https://jsfiddle.net/dholbert/3c2vbgdy/
What do you think about this sort of scenario and what this change means for it, @davidsgrogan ?

So I think for this specific case we might need to update our implementation a little. What happens here (in layout when determining sizes) is that the column flexbox tells the row flexbox that children shouldn't resolve against its height (we call this concept the "initial block-size is indefinite").
We should also do this in the min/max sizes pass (which we are currently missing).

I quickly made this change in a local build and for this specific case it renders the same as before.

(@davidsgrogan this is the local change: https://chromium-review.googlesource.com/c/chromium/src/+/2878647 )

One thing to note is that when working through these scenarios with stretching we often start with "what would a height: 100% do in the same situation?"

@aethanyc
Copy link
Contributor

@aethanyc aethanyc commented May 8, 2021

Yes, the stretched cross-size is transferring to the main-size via aspect ratio. But not because of step 11 (not even in layout at all). Just via 9.8.1, which takes effect before step 11.

Ah, now I see the point. An aspect-ratio flex item's width can be different depending whether its height is stretched or not. This is all about whether the stretched item's width can contribute to the flex container's intrinsic width or not. A single-line row flex container's width surely can affect the flexible item's final width. Thanks for the explanation.

@davidsgrogan
Copy link
Member Author

@davidsgrogan davidsgrogan commented May 12, 2021

@bfgeek and I talked offline. We plan to continue with the overall change I described in comment 0 above. But Ian is going to commit the change referenced two comments up that forces items in column flexboxes to treat their block size as indefinite when calculating the flexbox's intrinsic inline sizes.

I don't think there's spec support for this forced indefiniteness but it seems to be good because (a) it makes us ignore the height:50px that is almost-wholly unrelated to the final height of .rowflex in @dholbert 's original example, and (b) we aren't breaking backwards compatibility for this case.

We'll add a .tentative test for the forced indefiniteness.

davidsgrogan added a commit to davidsgrogan/wpt-results-analysis that referenced this issue May 13, 2021
The new tests seem reasonable to include, with two hesitations:

 * table-as-item* -- We explicitly decided to exclude tables as a focus area from compat2021. Making Blink pass these `flexbox/table-as-item` tests required a bunch of work in the tables code. Maybe we shouldn't penalize Webkit for an area that we agreed to deprioritize? Maybe we should exclude any test that any engine fails?
 * flexbox-min-width-auto-00{5,6} -- Currently all engines pass, but we think the tests are wrong and Blink plans to change them as long as the behavior change is web compatible (see web-platform-tests/wpt#28805)

@dholbert, thoughts on the updated test list? Any we should remove from the original list because they are invalid?
foolip pushed a commit to Ecosystem-Infra/wpt-results-analysis that referenced this issue May 18, 2021
 * table-as-item* -- We explicitly decided to exclude tables as a focus area from compat2021. Making Blink pass these `flexbox/table-as-item` tests required a bunch of work in the tables code. Maybe we shouldn't penalize Webkit for an area that we agreed to deprioritize? Maybe we should exclude any test that any engine fails?
 * flexbox-min-width-auto-00{5,6} -- Currently all engines pass, but we think the tests are wrong and Blink plans to change them as long as the behavior change is web compatible (see web-platform-tests/wpt#28805)
 * Remove table*item tests that >=1 engines fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants