Skip to content

GitLab

T
tangled
Commit a02c341b authored by David Grogan's avatar David Grogan Committed by Commit Bot
Browse files
Options

[css-flex] Respect aspect ratio when determining cross size 

Previously, if a 300x300 flex item got a 700 main size after flexing,
we'd assign a 300px cross size. LayoutReplaced doesn't pass the flexed
size (delivered in one of the Override sizes) through the aspect ratio,
so just add this logic directly in flex.

There's a regression, external/wpt/css/css-flexbox/align-items-007.html
that we'll have to fix before shipping, but I suspect the new behavior
might be right.

This CL adds a blink runtime flag, FlexAspectRatio, because this is the
first of a few aspect ratio fixes I want to ship at the same time. The
flag is set to experimental: enable-experimental-web-platform-features
users will see this fix in Canary right away. I will set the flag to
stable probably in time for M87.

Bug: 721123
Change-Id: Ie01f8da1a95eb940f20ba3e9bbf960bd4624c71b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333530
Commit-Queue: David Grogan <dgrogan@chromium.org>
Reviewed-by: default avatarChristian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795227}
parent c767d9df
Hide whitespace changes
Inline Side-by-side
Showing with 53 additions and 59 deletions
third_party/blink/renderer/core/layout/ng/flex/ng_flex_layout_algorithm.cc
View file @ a02c341b
......@@ -852,6 +852,33 @@ scoped_refptr<const NGLayoutResult> NGFlexLayoutAlgorithm::LayoutInternal() {
LogicalSize available_size;
NGBoxStrut margins = flex_item.physical_margins.ConvertToLogical(
ConstraintSpace().GetWritingMode(), Style().Direction());
LayoutUnit fixed_aspect_ratio_cross_size = kIndefiniteSize;
if (RuntimeEnabledFeatures::FlexAspectRatioEnabled() &&
flex_item.ng_input_node.HasAspectRatio() &&
flex_item.ng_input_node.IsReplaced()) {
// This code derives the cross axis size from the flexed main size and
// the aspect ratio. We can delete this code when
// NGReplacedLayoutAlgorithm exists, because it will do this for us.
NGConstraintSpace flex_basis_space =
BuildSpaceForFlexBasis(flex_item.ng_input_node);
const Length& cross_axis_length =
is_horizontal_flow_ ? child_style.Height() : child_style.Width();
// Only derive the cross axis size from the aspect ratio if the computed
// cross axis length might be indefinite. The item's cross axis length
// might still be definite if it is stretched, but that is checked in
// the |WillChildCrossSizeBeContainerCrossSize| calls below.
if (cross_axis_length.IsAuto() ||
(MainAxisIsInlineAxis(flex_item.ng_input_node) &&
BlockLengthUnresolvable(flex_basis_space, cross_axis_length,
LengthResolvePhase::kLayout))) {
fixed_aspect_ratio_cross_size =
flex_item.min_max_cross_sizes->ClampSizeToMinAndMax(
flex_item.cross_axis_border_padding +
LayoutUnit(
flex_item.flexed_content_size /
GetMainOverCrossAspectRatio(flex_item.ng_input_node)));
}
}
if (is_column_) {
available_size.inline_size = ChildAvailableSize().inline_size;
available_size.block_size =
......@@ -861,6 +888,9 @@ scoped_refptr<const NGLayoutResult> NGFlexLayoutAlgorithm::LayoutInternal() {
space_builder.SetIsFixedInlineSize(true);
available_size.inline_size = CalculateFixedCrossSize(
flex_item.min_max_cross_sizes.value(), margins);
} else if (fixed_aspect_ratio_cross_size != kIndefiniteSize) {
space_builder.SetIsFixedInlineSize(true);
available_size.inline_size = fixed_aspect_ratio_cross_size;
}
// https://drafts.csswg.org/css-flexbox/#definite-sizes
// If the flex container has a definite main size, a flex item's
......@@ -879,6 +909,9 @@ scoped_refptr<const NGLayoutResult> NGFlexLayoutAlgorithm::LayoutInternal() {
space_builder.SetIsFixedBlockSize(true);
available_size.block_size = CalculateFixedCrossSize(
flex_item.min_max_cross_sizes.value(), margins);
} else if (fixed_aspect_ratio_cross_size != kIndefiniteSize) {
space_builder.SetIsFixedBlockSize(true);
available_size.block_size = fixed_aspect_ratio_cross_size;
} else if (DoesItemStretch(flex_item.ng_input_node)) {
// If we are in a row flexbox, and we don't have a fixed block-size
// (yet), use the "measure" cache slot. This will be the first
......
third_party/blink/renderer/platform/runtime_enabled_features.json5
View file @ a02c341b
......@@ -786,6 +786,10 @@
name: "FileSystem",
status: "stable",
},
{
name: "FlexAspectRatio",
status: "experimental",
},
{
name: "FocuslessSpatialNavigation",
settable_from_internals: true,
......
third_party/blink/web_tests/FlagExpectations/disable-layout-ng
View file @ a02c341b
......@@ -494,6 +494,18 @@ crbug.com/591099 external/wpt/css/css-flexbox/flexbox_align-items-stretch-3.html
crbug.com/591099 external/wpt/css/css-flexbox/percentage-heights-006.html [ Failure ]
crbug.com/591099 external/wpt/css/css-flexbox/percentage-heights-014.html [ Failure ]
crbug.com/591099 external/wpt/css/css-flexbox/flex-minimum-height-flex-items-023.html [ Failure ]
crbug.com/721123 css3/flexbox/flexitem.html [ Failure ]
crbug.com/721123 external/wpt/css/css-flexbox/flexitem-stretch-image.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-001.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-001v.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-002.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-002v.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-005.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-005v.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-006.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-006v.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-007.html [ Failure ]
crbug.com/721123 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-007v.html [ Failure ]
# These would need a rebaseline back from LayoutNGBlockFlow to LayoutBlockFlow
crbug.com/864567 paint/float/float-under-inline-self-painting-change.html [ Failure ]
......
third_party/blink/web_tests/TestExpectations
View file @ a02c341b
......@@ -737,6 +737,8 @@ crbug.com/591099 external/wpt/css/css-ui/text-overflow-015.html [ Failure ]
# [css-flexbox]
crbug.com/957454 external/wpt/css/css-flexbox/align-items-007.html [ Failure ]
crbug.com/1003506 external/wpt/css/css-flexbox/percentage-heights-007.html [ Failure ]
crbug.com/1081802 external/wpt/css/css-flexbox/overflow-area-001.html [ Failure ]
crbug.com/1081802 external/wpt/css/css-flexbox/overflow-area-002.html [ Failure ]
......@@ -782,10 +784,6 @@ crbug.com/336604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftest
crbug.com/336604 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-collapsed-item-horiz-003.html [ Failure ]
# We don't correctly implement aspect ratios for images in Flexbox
crbug.com/531199 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-001.html [ Failure ]
crbug.com/531199 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-002.html [ Failure ]
crbug.com/531199 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-005.html [ Failure ]
crbug.com/531199 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-006.html [ Failure ]
crbug.com/1111128 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-min-height-auto-002b.html [ Failure ]
# These tests are incorrect, as Firefox has a bug in this area. https://bugzilla.mozilla.org/show_bug.cgi?id=1136312
......@@ -814,12 +812,6 @@ crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftest
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-flex-basis-content-002b.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-flex-basis-content-003a.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-flex-basis-content-004a.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-001v.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-002v.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-005v.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-006v.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-007.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-007v.html [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-justify-content-vert-006.xhtml [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-justify-content-wmvert-001.xhtml [ Failure ]
crbug.com/626703 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-safe-overflow-position-001.html [ Failure ]
......
third_party/blink/web_tests/css3/flexbox/flex-item-stretch-image.html deleted 100644 → 0
View file @ c767d9df
<!DOCTYPE html>
<title>CSS Flexbox: image stretch</title>
<meta name="assert" content="This test ensures that flexbox stretches a given image
to fit the size of flexitem according to flex values.">
<link rel="issue" href="https://bugs.chromium.org/p/chromium/issues/detail?id=721123">
<link rel="bookmark" href="http://wpt.live/css/css-flexbox/flexitem-stretch-image.html">
<html>
<style>
.flexbox {
width: 600px;
display: flex;
background-color: #aaa;
position: relative;
min-height: 10px;
}
.flexbox > * {
margin: 0;
border: 0;
padding: 0;
min-width: 0;
}
</style>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="../../resources/check-layout-th.js"></script>
<body onload="checkLayout('.flexbox')">
<div id=log></div>
<!-- Blink's behavior here is incorrect, see https://crbug.com/721123.
When that bug is fixed, please delete this test, as this is correctly
tested by external/wpt/css/css-flexbox/flexitem-stretch-image.html -->
<div class="flexbox">
<img data-expected-display="block" data-expected-width="345" style="flex: 1 0 auto;" src="../../images/resources/blue-100.png">
<img data-expected-display="block" data-expected-width="255" data-expected-height="100" style="flex: 1 0 auto;" src="../../images/resources/green-10.png">
</div>
</body>
third_party/blink/web_tests/css3/flexbox/flexitem.html
View file @ a02c341b
......@@ -47,8 +47,8 @@
<div class="flexbox">
<img data-expected-display="block" data-expected-width="200" style="flex: 1 0 auto;" src="../../images/resources/blue-100.png">
<img data-expected-display="block" data-expected-width="200" data-expected-height="100" style="flex: 2 0 0;" src="doesnotexist.png">
<img data-expected-display="block" data-expected-width="200" data-expected-height="100" style="flex: 2 0 0;" src="doesnotexist.png" alt="placeholder text">
<img data-expected-display="block" data-expected-width="200" data-expected-height="200" style="flex: 2 0 0;" src="doesnotexist.png">
<img data-expected-display="block" data-expected-width="200" data-expected-height="200" style="flex: 2 0 0;" src="doesnotexist.png" alt="placeholder text">
</div>
<div class="flexbox">
......
third_party/blink/web_tests/external/wpt/css/css-flexbox/flexitem-stretch-image-expected.txt deleted 100644 → 0
View file @ c767d9df
This is a testharness.js-based test.
FAIL .flexbox 1 assert_equals:
<div class="flexbox">
<img data-expected-display="block" data-expected-width="345" data-expected-height="345" style="flex: 1 0 auto;" src="support/100x100-blue.png">
<img data-expected-display="block" data-expected-width="255" data-expected-height="345" style="flex: 1 0 auto;" src="support/10x10-green.png">
</div>
height expected 345 but got 100
Harness: the test ran to completion.
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