Skip to content

GitLab

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

[FlexNG] Fix min-width: auto for flex item tables 

This patch only modifies the min-width: auto case for tables by skipping
the flex-specific behavior and just using the table's preferred minimum
width as calculated by legacy. We now match legacy when tables are flex
items.

But I think when the spec was updated in
https://github.com/w3c/csswg-drafts/commit/66241e4896d3e6300d8559080a0de4ee9a11fdcd,
legacy became incorrect in cases where min-width is specified. Compare
css/css-flexbox/table-as-item-fixed-min-width.html with
http://wpt.live/css/css-flexbox/table-as-item-auto-min-width.html . The
only difference is an additional min-width: 5px that should have no
effect but does, see https://i.imgur.com/yMxUdeI.png for how the new
test renders in legacy.

An older patchset (3) does what I think is the newly-correct behavior,
which also says that
http://wpt.live/css/css-flexbox/table-as-item-wide-content.html is
invalid -- the green square should be 500px wide.

Bug: 845235
Change-Id: Ica870fe3aa12fc1fbeb2388c0f2c69263c5e8860
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1995617
Commit-Queue: David Grogan <dgrogan@chromium.org>
Reviewed-by: default avatarIan Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: default avatarMorten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731785}
parent d83c769e
Hide whitespace changes
Inline Side-by-side
Showing with 99 additions and 57 deletions
third_party/blink/renderer/core/layout/ng/ng_flex_layout_algorithm.cc
View file @ 704e6460
......@@ -480,60 +480,73 @@ void NGFlexLayoutAlgorithm::ConstructAndAppendFlexItems() {
: child.Style().MinHeight();
if (min.IsAuto()) {
if (algorithm_->ShouldApplyMinSizeAutoForChild(*child.GetLayoutBox())) {
// TODO(dgrogan): Do the aspect ratio parts of
// https://www.w3.org/TR/css-flexbox-1/#min-size-auto
LayoutUnit content_size_suggestion =
MainAxisIsInlineAxis(child) ? intrinsic_sizes_border_box.min_size
: layout_result->IntrinsicBlockSize();
content_size_suggestion =
std::min(content_size_suggestion,
min_max_sizes_in_main_axis_direction.max_size);
if (child.MayHaveAspectRatio()) {
// TODO(dgrogan): We're including borders/padding in both
// content_size_suggestion and min_max_sizes_in_cross_axis_direction.
// Maybe we need to multiply the content size by the aspect ratio and
// then apply the border/padding from the other axis inside the
// Adjust* function. Test legacy/firefox. Start with
// https://jsfiddle.net/dgrogan/9uyg3aro/
// TODO(dgrogan): This should probably apply to column flexboxes also,
// but that's not what legacy does.
if (child.IsTable() && !is_column_) {
MinMaxSize table_preferred_widths =
ComputeMinAndMaxContentContribution(
Style(), child,
MinMaxSizeInput(child_percentage_size_.block_size));
min_max_sizes_in_main_axis_direction.min_size =
table_preferred_widths.min_size;
} else {
// TODO(dgrogan): Do the aspect ratio parts of
// https://www.w3.org/TR/css-flexbox-1/#min-size-auto
LayoutUnit content_size_suggestion =
MainAxisIsInlineAxis(child) ? intrinsic_sizes_border_box.min_size
: layout_result->IntrinsicBlockSize();
content_size_suggestion =
AdjustChildSizeForAspectRatioCrossAxisMinAndMax(
child, content_size_suggestion,
min_max_sizes_in_cross_axis_direction.min_size,
min_max_sizes_in_cross_axis_direction.max_size);
}
std::min(content_size_suggestion,
min_max_sizes_in_main_axis_direction.max_size);
if (child.MayHaveAspectRatio()) {
// TODO(dgrogan): We're including borders/padding in both
// content_size_suggestion and
// min_max_sizes_in_cross_axis_direction. Maybe we need to multiply
// the content size by the aspect ratio and then apply the
// border/padding from the other axis inside the Adjust* function.
// Test legacy/firefox. Start with
// https://jsfiddle.net/dgrogan/9uyg3aro/
content_size_suggestion =
AdjustChildSizeForAspectRatioCrossAxisMinAndMax(
child, content_size_suggestion,
min_max_sizes_in_cross_axis_direction.min_size,
min_max_sizes_in_cross_axis_direction.max_size);
}
LayoutUnit specified_size_suggestion(LayoutUnit::Max());
// If the item’s computed main size property is definite, then the
// specified size suggestion is that size.
if (MainAxisIsInlineAxis(child)) {
if (!specified_length_in_main_axis.IsAuto()) {
// TODO(dgrogan): Optimization opportunity: we may have already
// resolved specified_length_in_main_axis in the flex basis
// calculation. Reuse that if possible.
specified_size_suggestion = ResolveMainInlineLength(
flex_basis_space, child_style,
border_padding_in_child_writing_mode,
intrinsic_sizes_border_box, specified_length_in_main_axis);
LayoutUnit specified_size_suggestion(LayoutUnit::Max());
// If the item’s computed main size property is definite, then the
// specified size suggestion is that size.
if (MainAxisIsInlineAxis(child)) {
if (!specified_length_in_main_axis.IsAuto()) {
// TODO(dgrogan): Optimization opportunity: we may have already
// resolved specified_length_in_main_axis in the flex basis
// calculation. Reuse that if possible.
specified_size_suggestion = ResolveMainInlineLength(
flex_basis_space, child_style,
border_padding_in_child_writing_mode,
intrinsic_sizes_border_box, specified_length_in_main_axis);
}
} else if (!BlockLengthUnresolvable(flex_basis_space,
specified_length_in_main_axis,
LengthResolvePhase::kLayout)) {
specified_size_suggestion =
ResolveMainBlockLength(flex_basis_space, child_style,
border_padding_in_child_writing_mode,
specified_length_in_main_axis,
layout_result->IntrinsicBlockSize(),
LengthResolvePhase::kLayout);
DCHECK_NE(specified_size_suggestion, kIndefiniteSize);
}
} else if (!BlockLengthUnresolvable(flex_basis_space,
specified_length_in_main_axis,
LengthResolvePhase::kLayout)) {
specified_size_suggestion = ResolveMainBlockLength(
flex_basis_space, child_style,
border_padding_in_child_writing_mode,
specified_length_in_main_axis,
layout_result->IntrinsicBlockSize(), LengthResolvePhase::kLayout);
DCHECK_NE(specified_size_suggestion, kIndefiniteSize);
}
// Spec says to clamp specified_size_suggestion by max size but because
// content_size_suggestion already is, and we take the min of those
// two, we don't need to clamp specified_size_suggestion.
// https://github.com/w3c/csswg-drafts/issues/3669
// Spec says to clamp specified_size_suggestion by max size but
// because content_size_suggestion already is, and we take the min of
// those two, we don't need to clamp specified_size_suggestion.
// https://github.com/w3c/csswg-drafts/issues/3669
min_max_sizes_in_main_axis_direction.min_size =
std::min(specified_size_suggestion, content_size_suggestion);
min_max_sizes_in_main_axis_direction.min_size =
std::min(specified_size_suggestion, content_size_suggestion);
}
}
} else if (MainAxisIsInlineAxis(child)) {
min_max_sizes_in_main_axis_direction.min_size = ResolveMinInlineLength(
......
third_party/blink/renderer/core/layout/ng/ng_layout_input_node.h
View file @ 704e6460
......@@ -123,6 +123,7 @@ class CORE_EXPORT NGLayoutInputNode {
bool IsRenderedLegend() const {
return IsBlock() && box_->IsRenderedLegend();
}
bool IsTable() const { return IsBlock() && box_->IsTable(); }
bool IsMathRoot() const { return box_->IsMathMLRoot(); }
......
third_party/blink/web_tests/TestExpectations
View file @ 704e6460
......@@ -1333,7 +1333,6 @@ crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/flex-as
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/hittest-overlapping-margin.html [ Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/hittest-overlapping-order.html [ Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/quirks-auto-block-size-with-percentage-item.html [ Pass Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/table-as-item-auto-min-width.html [ Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-003.html [ Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-004.html [ Failure ]
crbug.com/845235 virtual/layout_ng_flex_box/external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-intrinsic-ratio-004v.html [ Failure ]
......@@ -1421,6 +1420,8 @@ crbug.com/953534 [ Mac ] virtual/layout_ng_flex_box/external/wpt/css/css-flexbox
crbug.com/1003506 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/percentage-heights-007.html [ Failure ]
crbug.com/1025630 [ Mac ] virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/select-element-zero-height-001.html [ Failure ]
crbug.com/1025630 [ Mac ] virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/select-element-zero-height-002.html [ Failure ]
crbug.com/782948 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/table-as-item-fixed-min-width.html [ Failure ]
crbug.com/782948 virtual/layout_ng_flex_box/external/wpt/css/css-flexbox/table-as-item-wide-content.html [ Failure ]
# Fieldset in NG
#
......@@ -2396,6 +2397,9 @@ crbug.com/249112 external/wpt/css/css-flexbox/flex-minimum-width-flex-items-007.
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/782948 external/wpt/css/css-flexbox/table-as-item-fixed-min-width.html [ Failure ]
crbug.com/782948 external/wpt/css/css-flexbox/table-as-item-wide-content.html [ Failure ]
# Requires support for hit-testing based on flex order.
crbug.com/844505 external/wpt/css/css-flexbox/hittest-overlapping-order.html [ Failure ]
......
third_party/blink/web_tests/external/wpt/css/css-flexbox/table-as-item-fixed-min-width.html 0 → 100644
View file @ 704e6460
<!DOCTYPE html>
<title>CSS Flexbox Test: Flex item as table, specified width and min-width less than minimum intrinsic width</title>
<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
<link rel="help" href="https://www.w3.org/TR/css-flexbox-1/#layout-algorithm" title="9. Flex Layout Algorithm">
<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
<div style="display:flex; width:100px; background:red;">
<div style="display:table; min-width: 5px; width: 10px; max-width:10px; height:100px; background:green;">
<div style="width:100px; height:10px; background:green;"></div>
</div>
</div>
third_party/blink/web_tests/external/wpt/css/css-flexbox/table-as-item-wide-content.html
View file @ 704e6460
<!DOCTYPE html>
<title>CSS Flexbox Test: Flex item as table with wide content</title>
<link rel="author" title="Morten Stenshorne" href="mailto:mstensho@chromium.org">
<link rel="help" href="https://www.w3.org/TR/css-flexbox-1/#layout-algorithm" title="9. Flex Layout Algorithm">
<meta name="assert" content="A flex item as a table uses the sizing algorithm of the flexbox">
<link rel="match" href="../reference/ref-filled-green-100px-square-only.html">
<p>Test passes if there is a filled green square.</p>
<div style="display:flex; width:100px;">
<link rel="author" title="David Grogan" href="mailto:dgrogan@chromium.org">
<link rel="help" href="https://drafts.csswg.org/css-flexbox/#algo-main-item" title="Sentence beginning with 'The hypothetical main size is...'">
<meta name="assert" content="A flex item respects the _used_ min size of an item, which tables define specially.">
<link rel="bookmark" href="https://github.com/w3c/csswg-drafts/issues/2442" />
<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
<style>
#reference-overlapped-red {
position: absolute;
background-color: red;
width: 100px;
height: 100px;
z-index: -1;
}
</style>
<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
<div id=reference-overlapped-red></div>
<div style="display:flex; width:50px;">
<div style="min-width:0; flex:1 1; display:table; background:green;">
<div style="width:500px; height:100px;"></div>
<div style="width:100px; height:100px;"></div>
</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