Commit 69f9a43b authored by David Grogan's avatar David Grogan Committed by Commit Bot

[css-flex] Fix min-width: auto for replaced elements

This is the 'width' version of
https://chromium-review.googlesource.com/c/chromium/src/+/2462574

min-width:auto had been using the image's intrinsic widths as the
content size suggestion, as returned by image_node.ComputeMinMaxSizes().
But that function honors specified min/main/max widths and the content
size suggestion is supposed to ignore those.

Now content size suggestion ignores specified width properties and also
honors the transferred size -- if the specified height is definite, we
pass it through the aspect ratio to determine the content size suggested
width.

Change-Id: I74ed9d96e584605ad8154cc9cb5a94ef84f78544
Bug: 1132627
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2500448Reviewed-by: default avatarChristian Biesinger <cbiesinger@chromium.org>
Commit-Queue: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821309}
parent ca72cf0e
...@@ -415,6 +415,7 @@ namespace { ...@@ -415,6 +415,7 @@ namespace {
LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement( LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement(
const NGBlockNode& node, const NGBlockNode& node,
const NGConstraintSpace& space, const NGConstraintSpace& space,
const base::Optional<LayoutUnit> definite_block_size,
const MinMaxSizes& used_min_max_block_sizes) { const MinMaxSizes& used_min_max_block_sizes) {
DCHECK(node.HasAspectRatio()); DCHECK(node.HasAspectRatio());
LogicalSize aspect_ratio = node.GetAspectRatio(); LogicalSize aspect_ratio = node.GetAspectRatio();
...@@ -422,16 +423,27 @@ LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement( ...@@ -422,16 +423,27 @@ LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement(
NGBoxStrut border_padding = NGBoxStrut border_padding =
ComputeBorders(space, node) + ComputePadding(space, style); ComputeBorders(space, node) + ComputePadding(space, style);
DCHECK_NE(style.LogicalHeight().GetType(), Length::Type::kFixed)
<< "Flex will not use this function if the block size of the replaced "
"element is definite.";
base::Optional<LayoutUnit> intrinsic_inline; base::Optional<LayoutUnit> intrinsic_inline;
base::Optional<LayoutUnit> intrinsic_block; base::Optional<LayoutUnit> intrinsic_block;
node.IntrinsicSize(&intrinsic_inline, &intrinsic_block);
// intrinsic_inline and intrinsic_block can be empty independent of each base::Optional<LayoutUnit> block_size_border_box;
// other. if (definite_block_size.has_value()) {
block_size_border_box = definite_block_size;
} else {
node.IntrinsicSize(&intrinsic_inline, &intrinsic_block);
if (intrinsic_block) {
block_size_border_box = *intrinsic_block + border_padding.BlockSum();
}
}
if (block_size_border_box) {
LayoutUnit clamped_intrinsic_block_border_box =
used_min_max_block_sizes.ClampSizeToMinAndMax(*block_size_border_box);
return InlineSizeFromAspectRatio(border_padding, aspect_ratio,
EBoxSizing::kContentBox,
clamped_intrinsic_block_border_box);
}
if (intrinsic_inline) { if (intrinsic_inline) {
MinMaxSizes inline_min_max = ComputeTransferredMinMaxInlineSizes( MinMaxSizes inline_min_max = ComputeTransferredMinMaxInlineSizes(
aspect_ratio, used_min_max_block_sizes, border_padding, aspect_ratio, used_min_max_block_sizes, border_padding,
...@@ -441,14 +453,6 @@ LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement( ...@@ -441,14 +453,6 @@ LayoutUnit ComputeIntrinsicInlineSizeForAspectRatioElement(
return inline_min_max.ClampSizeToMinAndMax(intrinsic_inline_border_box); return inline_min_max.ClampSizeToMinAndMax(intrinsic_inline_border_box);
} }
if (intrinsic_block) {
intrinsic_block = *intrinsic_block + border_padding.BlockSum();
intrinsic_block =
used_min_max_block_sizes.ClampSizeToMinAndMax(*intrinsic_block);
return InlineSizeFromAspectRatio(border_padding, aspect_ratio,
EBoxSizing::kContentBox, *intrinsic_block);
}
// If control flow reaches here, the item has aspect ratio only, no natural // If control flow reaches here, the item has aspect ratio only, no natural
// sizes. Spec says: // sizes. Spec says:
// * If the available space is definite in the inline axis, use the stretch // * If the available space is definite in the inline axis, use the stretch
...@@ -706,10 +710,15 @@ void NGFlexLayoutAlgorithm::ConstructAndAppendFlexItems() { ...@@ -706,10 +710,15 @@ void NGFlexLayoutAlgorithm::ConstructAndAppendFlexItems() {
if (RuntimeEnabledFeatures::FlexAspectRatioEnabled()) { if (RuntimeEnabledFeatures::FlexAspectRatioEnabled()) {
// Legacy uses child.PreferredLogicalWidths() for this case, which // Legacy uses child.PreferredLogicalWidths() for this case, which
// is not exactly correct. // is not exactly correct.
// ComputeIntrinsicInlineSizeForAspectRatioElement would honor the
// definite block size parameter by multipying it by the aspect
// ratio, but if control flow reaches here, we know we don't have a
// definite inline size. If we did, we would have fallen into the
// "part B" section above, not this "part C, D, E" section.
flex_base_border_box = flex_base_border_box =
ComputeIntrinsicInlineSizeForAspectRatioElement( ComputeIntrinsicInlineSizeForAspectRatioElement(
child, flex_basis_space, child, flex_basis_space, /* definite_block_size */
min_max_sizes_in_cross_axis_direction); base::nullopt, min_max_sizes_in_cross_axis_direction);
} else { } else {
MinMaxSizesInput input(child_percentage_size_.block_size, MinMaxSizesInput input(child_percentage_size_.block_size,
MinMaxSizesType::kContent); MinMaxSizesType::kContent);
...@@ -768,8 +777,27 @@ void NGFlexLayoutAlgorithm::ConstructAndAppendFlexItems() { ...@@ -768,8 +777,27 @@ void NGFlexLayoutAlgorithm::ConstructAndAppendFlexItems() {
if (algorithm_->ShouldApplyMinSizeAutoForChild(*child.GetLayoutBox())) { if (algorithm_->ShouldApplyMinSizeAutoForChild(*child.GetLayoutBox())) {
LayoutUnit content_size_suggestion; LayoutUnit content_size_suggestion;
if (MainAxisIsInlineAxis(child)) { if (MainAxisIsInlineAxis(child)) {
content_size_suggestion = if (child.IsReplaced() && child.HasAspectRatio() &&
MinMaxSizesFunc(MinMaxSizesType::kContent).sizes.min_size; RuntimeEnabledFeatures::FlexAspectRatioEnabled()) {
base::Optional<LayoutUnit> definite_block_size;
if (!BlockLengthUnresolvable(flex_basis_space,
child_style.LogicalHeight(),
LengthResolvePhase::kLayout)) {
definite_block_size = ResolveMainBlockLength(
flex_basis_space, child_style,
border_padding_in_child_writing_mode,
child_style.LogicalHeight(), IntrinsicBlockSizeFunc,
LengthResolvePhase::kLayout);
}
content_size_suggestion =
ComputeIntrinsicInlineSizeForAspectRatioElement(
child, flex_basis_space, definite_block_size,
min_max_sizes_in_cross_axis_direction);
} else {
content_size_suggestion =
MinMaxSizesFunc(MinMaxSizesType::kContent).sizes.min_size;
}
} else { } else {
LayoutUnit intrinsic_block_size; LayoutUnit intrinsic_block_size;
if (child.IsReplaced()) { if (child.IsReplaced()) {
......
...@@ -58,6 +58,7 @@ crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-003.html [ ...@@ -58,6 +58,7 @@ crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-003.html [
crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-003v.html [ Failure ] crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-003v.html [ Failure ]
crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-004.html [ Failure ] crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-004.html [ Failure ]
crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-004v.html [ Failure ] crbug.com/591099 external/wpt/css/css-flexbox/image-as-flexitem-size-004v.html [ Failure ]
crbug.com/1132627 external/wpt/css/css-flexbox/flex-minimum-width-flex-items-007.xht [ Failure ]
### external/wpt/css/css-fonts/ ### external/wpt/css/css-fonts/
crbug.com/591099 external/wpt/css/css-fonts/font-features-across-space-1.html [ Failure ] crbug.com/591099 external/wpt/css/css-fonts/font-features-across-space-1.html [ Failure ]
...@@ -368,6 +369,8 @@ crbug.com/591099 external/wpt/css/css-content/quotes-033.html [ Failure ] ...@@ -368,6 +369,8 @@ crbug.com/591099 external/wpt/css/css-content/quotes-033.html [ Failure ]
### external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/ ### external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/
crbug.com/591099 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-self-horiz-002.xhtml [ Failure ] crbug.com/591099 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-self-horiz-002.xhtml [ Failure ]
crbug.com/886592 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/align3/flex-abspos-staticpos-margin-002.html [ Failure ] crbug.com/886592 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/align3/flex-abspos-staticpos-margin-002.html [ Failure ]
crbug.com/1132627 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-width-auto-002a.html [ Failure ]
crbug.com/1132627 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-width-auto-002c.html [ Failure ]
### external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/ib-split/ ### external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/ib-split/
crbug.com/591099 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/ib-split/emptyspan-1.html [ Failure ] crbug.com/591099 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/ib-split/emptyspan-1.html [ Failure ]
......
...@@ -762,8 +762,6 @@ crbug.com/1111708 external/wpt/css/css-flexbox/flexbox_justifycontent-center-ove ...@@ -762,8 +762,6 @@ crbug.com/1111708 external/wpt/css/css-flexbox/flexbox_justifycontent-center-ove
crbug.com/1111128 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-height-auto-002b.html [ Failure ] crbug.com/1111128 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-height-auto-002b.html [ Failure ]
crbug.com/1128262 external/wpt/css/css-flexbox/table-as-item-specified-width.html [ Failure ] crbug.com/1128262 external/wpt/css/css-flexbox/table-as-item-specified-width.html [ Failure ]
crbug.com/249112 external/wpt/css/css-flexbox/flex-minimum-width-flex-items-007.xht [ Failure ]
# These require css-align-3 positional keywords that Blink doesn't yet support for flexbox. # These require css-align-3 positional keywords that Blink doesn't yet support for flexbox.
crbug.com/1011718 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-content-horiz-001a.xhtml [ Failure ] crbug.com/1011718 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-content-horiz-001a.xhtml [ Failure ]
crbug.com/1011718 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-content-horiz-001b.xhtml [ Failure ] crbug.com/1011718 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-align-content-horiz-001b.xhtml [ Failure ]
...@@ -793,9 +791,6 @@ crbug.com/336604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftest ...@@ -793,9 +791,6 @@ crbug.com/336604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftest
crbug.com/336604 external/wpt/css/css-flexbox/flexbox_visibility-collapse-line-wrapping.html [ Failure ] crbug.com/336604 external/wpt/css/css-flexbox/flexbox_visibility-collapse-line-wrapping.html [ Failure ]
crbug.com/336604 external/wpt/css/css-flexbox/flexbox_visibility-collapse.html [ Failure ] crbug.com/336604 external/wpt/css/css-flexbox/flexbox_visibility-collapse.html [ Failure ]
crbug.com/553838 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-width-auto-002a.html [ Failure ]
crbug.com/553838 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-width-auto-002c.html [ Failure ]
# We paint in an incorrect order when layers are present. Blocked on composite-after-paint. # We paint in an incorrect order when layers are present. Blocked on composite-after-paint.
crbug.com/370604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-paint-ordering-002.xhtml [ Failure ] crbug.com/370604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-paint-ordering-002.xhtml [ Failure ]
......
...@@ -41,9 +41,9 @@ ...@@ -41,9 +41,9 @@
</div> </div>
<div class="flexbox" style="width: 10px;" data-expected-width="10"> <div class="flexbox" style="width: 10px;" data-expected-width="10">
<!-- should use min(transferred, content width) = 10px as minimum width, <!-- transferred and content suggestions are both 100px here, so minimum
which the image will shrink to due to default flex-shrink. --> width is min(transferred, content width) = 100px. -->
<img src="support/10x10-green.png" style="height: 100px;" data-expected-width="10"> <img src="support/10x10-green.png" style="height: 100px;" data-expected-width="100">
</div> </div>
<div class="flexbox column" style="height: 10px;" data-expected-height="10"> <div class="flexbox column" style="height: 10px;" data-expected-height="10">
......
...@@ -2,8 +2,9 @@ ...@@ -2,8 +2,9 @@
<title>Flex transferred minimum width</title> <title>Flex transferred minimum width</title>
<link rel="author" title="dgrogan@chromium.org" href="mailto:dgrogan@chromium.org" /> <link rel="author" title="dgrogan@chromium.org" href="mailto:dgrogan@chromium.org" />
<link rel="help" href="https://drafts.csswg.org/css-flexbox/#min-size-auto" /> <link rel="help" href="https://drafts.csswg.org/css-flexbox/#min-size-auto" />
<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/5663" />
<link rel="match" href="../reference/ref-filled-green-100px-square.xht" /> <link rel="match" href="../reference/ref-filled-green-100px-square.xht" />
<meta name="assert" content="min-width: auto ignores transferred size suggestion when item has a definite main size"> <meta name="assert" content="min-width: auto ignores transferred size suggestion when item has a definite specified main size">
<style> <style>
#reference-overlapped-red { #reference-overlapped-red {
...@@ -19,13 +20,16 @@ ...@@ -19,13 +20,16 @@
<div id="reference-overlapped-red"></div> <div id="reference-overlapped-red"></div>
<div style="width:100px; height: 75px; background: green;"></div> <div style="width:100px; height: 75px; background: green;"></div>
<div style="display: flex; width: 0px"> <div style="display: flex; width: 0px; height: 25px;">
<!-- <!--
content size suggestion is 300px. content size suggestion is 300px
specified size suggestion is 100px. specified size suggestion is 100px
transferred size suggestion is 50px, but that is ignored because there is a
specified size. The ignoring is from the spec text that says, Chrome 87 and Firefox 84a1 disagree about transferred size suggestion here
(capitalization added): (Chrome says undefined. Firefox says 300/150 * 25 = 50. See spec issue
above.) but it doesn't matter: this test asserts that the transferred size
suggestion is ignored, because of this from the spec (capitalization
added):
In general, the content-based minimum size of a flex item is the smaller of In general, the content-based minimum size of a flex item is the smaller of
its content size suggestion and its specified size suggestion. However, if its content size suggestion and its specified size suggestion. However, if
...@@ -35,5 +39,5 @@ ...@@ -35,5 +39,5 @@
So here the content-based minimum size is min(300, 100) = 100. So here the content-based minimum size is min(300, 100) = 100.
--> -->
<img src="support/300x150-green.png" style="height: 25px; width: 100px;"> <img src="support/300x150-green.png" style="width: 100px;">
</div> </div>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment