[css-flex] Obey stretch size in replaced items' content size suggestion #30900
Conversation
|
@dholbert @aethanyc, can you take a look at the difference in expected behavior here? Each test difference boils down to the example from the commit message quoted below. I want to make sure you have the same interpretation of the spec, which is not consistent with current Gecko behavior. With this interpretation, a flex item's transferred size suggestion never has any effect, and we could remove it from the spec.
<div style="display: flex; height: 50px;">
<img src="support/300x150-green.png">
</div>/cc @bfgeek |
|
Specifically this would roughly act the same as if you placed "height: 100%" on the image (which we agree on today). We found this while unifying all our replaced element sizing behaviour. |
|
I think the proposed change makes a lot of sense and we should definitely try shipping it. (If, however, we end up not shipping it we should clarify whether replaced items or items with an aspect ratio are the exception) |
Just to clarify -- the actual final layout of your comment's 3-line example isn't changing, correct? The image ends up 100px wide, regardless of this proposed change, I think? (It's the first image in this testcase: https://jsfiddle.net/dholbert/febgszay/ , and it's already 100px wide in Firefox as well as Chrome.)
From a quick analysis, I think Gecko matches the current spec in its rendering of all three of the modified testcases here... Are you saying that our behavior disagrees with the current spec? |
|
Though to be clear, we are not accounting for the preferred size in the opposite axis when computing our content size suggestion. Given the existence of "transferred size suggestion" as a thing that the spec requires us to sometimes-ignore, I think that our behavior on that is correct & reasonable. Having said that: if in fact the min-content size does implicitly include the transferred size due to how min-content sizes are defined, then the spec definitely needs some rewording one way or another to account for that. |
|
Piecemeal responses:
Oh, I didn't check if the final layout would change, I just made a quick example that would have a different content-size suggestion.
Yes, that's what I'm saying. I think the modified css/css-flexbox/flex-minimum-height-flex-items-023.html matches the current spec. Current Blink and current Gecko both fail it.
Not accounting for that is the part that disagrees with the spec. A brief explanation of why is in the comments of css/css-flexbox/flex-minimum-height-flex-items-023.html.
I think min-content size does include transferred size, but I don't know what implicitly really means with regard to the specs, so I don't know if I agree with the implicitly part.
I think that is the first Note in https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes ? Thanks a bunch for the thoughtful response. We're making progress! |
|
When computing flex item's automatic minimum size, current spec prioritizes content size suggestion over transferred size suggestion. Gecko current behavior is that content size suggestion ignores aspect-ratio [1], it implies a replaced element's nature size in the main axis has a higher priority over the main size transferred via aspect-ratio and cross-size (either the item's specified cross-size or stretched cross-size imposed by flex container.) Philosophically, if we obey stretched cross size when computing content size suggestion, it seems to me we rule out replaced element's nature size from the equation. I'm not sure this is intended from spec authors. If blink thinks this is the way to go, we should file a spec issue and ask feedback from spec authors. [1] Per @fantasai's issue w3c/csswg-drafts#5305, does content size suggestion a scenario that aspect ratio should be ignored? |
|
Practically speaking the case that this would change is this example: Importantly we all agree on the non-replaced case here - specifically: There is also For the first example (9648) the In the second example (9649) we are all (correctly) using the block-constraints (percentage or stretch) to determine the min-content size. I think this all falls out naturally from the spec? |
|
|
||
| 6. specified size suggestion = 100px because that's what's specified. | ||
|
|
||
| 7. So content-based min size = min(150, 100) = 100px. That becomes its flexed | ||
| 7. So content-based min size = min(50, 100) = 50px. That becomes its flexed | ||
| height (see #1). | ||
|
|
||
| 8. Then, the item stretches its width to 100px. The item has an aspect ratio, | ||
| so does the new width change the height? Not according to | ||
| https://drafts.csswg.org/css-flexbox/#algo-stretch, which says "Note that | ||
| this step does not affect the main size of the flex item, even if it has an | ||
| intrinsic aspect ratio." The height remains 100px. |
There was a problem hiding this comment.
Nit: If we were to take this patch, 100px needs to change to 50px, and line 13, too.
There was a problem hiding this comment.
Thanks for catching. I put your comments in the Chrome issue tracker and won't submit until I've resolved them.
(If I update the files now, what would happen to our existing thread in this PR? I don't use github PRs very much and don't know how to use them ergonomically)
| <!-- should use min(specified, content height) = 10px as minimum height, | ||
| which the image will shrink to due to default flex-shrink. --> |
There was a problem hiding this comment.
Nit: this comment needs an update.
|
My goal in this PR conversation is to ensure we're on the same page about what behavior the current specs dictate for this behavior. Then I'll file a csswg-drafts issue to discuss what we want the behavior to be, which if we decide to change, will most likely involve changing the definition of flex's content size suggestion. But if we fundamentally disagree about the current specs dictate, well, then we'll file an issue for that and loop in the spec authors :) |
|
Oh, and, potentially changing the definition of content-size suggestion is where @aethanyc and I also ended up in 21a7c47#commitcomment-56282089 . Just some links for future reference: @bfgeek brought up changing content-size suggestion to ignore aspect-ratio in w3c/csswg-drafts#5032 @fantasai floated the idea of changing content-size suggestion to be equivalent to natural size for replaced elements in w3c/csswg-drafts#6069 (comment) |
Yes, filing spec issue sounds good because we didn't reach an agreement on what the current spec behavior should be. |
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-3173922
branch
3 times, most recently
from
73b73f7 to
2a28295
Compare
Content size suggestion is just min-content size. The min-content size of a replaced element is supposed to account for preferred size in the opposite axis. We were not doing that for content size suggestion when the opposize axis preferred size came from a flexbox's fixed cross size. In the example below, Blink now gives the image content size suggestion 100px because 100 = 300*150/50. Before this patch Blink gave it a content size suggestion equal to its natural width 300px. <div style="display: flex; height: 50px;"> <img src="support/300x150-green.png"> </div> Firefox matches Blink's behavior before this patch. We may have compat issues. See w3c/csswg-drafts#6693 for discussion of this new behavior. Bug: 1246126 Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922 Commit-Queue: David Grogan <[email protected]> Reviewed-by: Ian Kilpatrick <[email protected]> Cr-Commit-Position: refs/heads/main@{#931259}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-3173922
branch
from
2a28295 to
234b85c
Compare
Content size suggestion is just min-content size. The min-content size
of a replaced element is supposed to account for preferred size in the
opposite axis. We were not doing that for content size suggestion when
the opposize axis preferred size came from a flexbox's fixed cross size.
In the example below, Blink now gives the image content size suggestion
100px because 100 = 300*150/50. Before this patch Blink gave it a
content size suggestion equal to its natural width 300px.
<div style="display: flex; height: 50px;">
<img src="support/300x150-green.png">
</div>
Firefox matches Blink's behavior before this patch. We may have compat
issues.
See w3c/csswg-drafts#6693 for discussion of
this new behavior.
Bug: 1246126
Change-Id: I1d42c7151b9a0da7e001ca89ea1d7fb9187ae235
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173922
Commit-Queue: David Grogan <[email protected]>
Reviewed-by: Ian Kilpatrick <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931259}