[LayoutNG] Ensure self-collapsing block relayout when ancestor resolves.
This bug was a combination of older code before we had "forced" BFC
block-offsets.
We didn't correctly detect when we had a self-collapsing block (which
had floats within it) that needed relayout if the initial estimate
was wrong.
This makes sure we give ourselves enough information to detect these
cases.
Bug: 980803
Change-Id: Id1ea5e10d819cb4509fd7664564b75b876f0f7cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1690720
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675729}
diff --git a/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
index 161685c..c918813 100644
--- a/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
+++ b/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
@@ -1280,15 +1280,6 @@
base::Optional<LayoutUnit> child_bfc_block_offset =
layout_result->BfcBlockOffset();
- // TODO(layout-dev): A more optimal version of this is to set
- // relayout_child_when_bfc_resolved only if the child tree itself _added_ any
- // floats that it failed to position. Currently, we risk relaying out the
- // parent block for no reason, because we're not able to make this
- // distinction.
- bool relayout_child_when_bfc_resolved =
- layout_result->AdjoiningFloatTypes() && !child_bfc_block_offset &&
- !child_space.ForcedBfcBlockOffset();
-
bool is_self_collapsing = layout_result->IsSelfCollapsing();
// Only non self-collapsing children (e.g. "normal children") can be pushed
@@ -1327,9 +1318,9 @@
// present, but we shouldn't apply it (instead preferring the child's new
// BFC block-offset).
DCHECK(!ConstraintSpace().AncestorHasClearancePastAdjoiningFloats());
- ResolveBfcBlockOffset(previous_inflow_position, bfc_block_offset,
- /* forced_bfc_block_offset */ base::nullopt);
- return false;
+ if (!ResolveBfcBlockOffset(previous_inflow_position, bfc_block_offset,
+ /* forced_bfc_block_offset */ base::nullopt))
+ return false;
}
// We have special behaviour for a self-collapsing child which gets pushed
@@ -1366,16 +1357,28 @@
return false;
}
+ bool self_collapsing_child_needs_relayout = false;
if (!child_bfc_block_offset) {
// Layout wasn't able to determine the BFC block-offset of the child. This
// has to mean that the child is self-collapsing.
DCHECK(is_self_collapsing);
if (container_builder_.BfcBlockOffset()) {
- // Since we know our own BFC block offset, though, we can calculate that
+ // Since we know our own BFC block-offset, though, we can calculate that
// of the child as well.
child_bfc_block_offset = PositionSelfCollapsingChildWithParentBfc(
child, child_space, *child_data, *layout_result);
+
+ // We may need to relayout this child if it had any objects which were
+ // positioned in the incorrect position.
+ //
+ // TODO(layout-dev): A more optimal version of this is to set this flag
+ // only if the child tree *added* any floats which it failed to position.
+ // Currently, we risk relaying out the sub-tree for no reason, because
+ // we're not able to make this distinction.
+ if (layout_result->AdjoiningFloatTypes() &&
+ !child_space.ForcedBfcBlockOffset())
+ self_collapsing_child_needs_relayout = true;
}
} else if (!child_had_clearance) {
// We shouldn't have any pending floats here, since an in-flow child found
@@ -1411,7 +1414,6 @@
//
// The resulting margin strut in the above example will be {40, -30}. See
// |ComputeInflowPosition| for how this end margin strut is used.
- bool self_collapsing_child_had_clearance_needs_relayout = false;
if (self_collapsing_child_had_clearance) {
NGMarginStrut margin_strut;
margin_strut.Append(child_data->margins.block_start,
@@ -1421,7 +1423,7 @@
// previous one.
if (child_data->margin_strut != margin_strut) {
child_data->margin_strut = margin_strut;
- self_collapsing_child_had_clearance_needs_relayout = true;
+ self_collapsing_child_needs_relayout = true;
}
}
@@ -1430,8 +1432,7 @@
// - It has some unpositioned floats.
// - It was affected by clearance.
if ((layout_result->Status() == NGLayoutResult::kBfcBlockOffsetResolved ||
- relayout_child_when_bfc_resolved ||
- self_collapsing_child_had_clearance_needs_relayout) &&
+ self_collapsing_child_needs_relayout) &&
child_bfc_block_offset) {
NGConstraintSpace new_child_space = CreateConstraintSpaceForChild(
child, *child_data, child_available_size_, /* is_new_fc */ false,
@@ -1455,7 +1456,6 @@
}
DCHECK_EQ(layout_result->Status(), NGLayoutResult::kSuccess);
- relayout_child_when_bfc_resolved = false;
}
// It is now safe to update our version of the exclusion space, and any
@@ -1482,7 +1482,8 @@
// If we are a new formatting context, the child will get re-laid out once it
// has been positioned.
if (!container_builder_.BfcBlockOffset()) {
- abort_when_bfc_block_offset_updated_ |= relayout_child_when_bfc_resolved;
+ abort_when_bfc_block_offset_updated_ |=
+ layout_result->AdjoiningFloatTypes();
// If our BFC block offset is unknown, and the child got pushed down by
// floats, so will we.
if (layout_result->IsPushedByFloats())
@@ -2328,14 +2329,11 @@
// If no previous BFC block offset was set, we need to abort.
if (!ConstraintSpace().ForcedBfcBlockOffset())
return true;
+
// If the previous BFC block offset matches the new one, we can continue.
// Otherwise, we need to abort.
- LayoutUnit old_bfc_block_offset = *ConstraintSpace().ForcedBfcBlockOffset();
-
- // In order to determine if the two offsets are equal, we also need to adjust
- // floats offset by the clearance offset.
- ApplyClearance(ConstraintSpace(), &old_bfc_block_offset);
- return *container_builder_.BfcBlockOffset() != old_bfc_block_offset;
+ return *container_builder_.BfcBlockOffset() !=
+ *ConstraintSpace().ForcedBfcBlockOffset();
}
void NGBlockLayoutAlgorithm::PositionPendingFloats(
diff --git a/third_party/blink/web_tests/external/wpt/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-004.html b/third_party/blink/web_tests/external/wpt/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-004.html
new file mode 100644
index 0000000..bda89a9
--- /dev/null
+++ b/third_party/blink/web_tests/external/wpt/css/CSS2/floats-clear/adjoining-float-nested-forced-clearance-004.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<link rel="match" href="../../reference/ref-filled-green-100px-square-only.html" />
+<link rel="help" href="https://www.w3.org/TR/CSS22/visuren.html#flow-control" title="9.5.2 Controlling flow next to floats: the 'clear' property">
+<p>Test passes if there is a filled green square.</p>
+<div style="overflow: hidden;"></div>
+<div style="float: left; width: 100px; height: 50px; background: green;"></div>
+<span>
+ <div style="clear: both;">
+ <div style="height: 10px;">
+ <div style="float: left; width: 100px; height: 50px; background: green;">
+ </div>
+ </div>
+</span>
