Skip to content

GitLab

T
tangled
Commit d97346eb authored by Morten Stenshorne's avatar Morten Stenshorne Committed by Commit Bot
Browse files
Options

[LayoutNG] Don't mark floats as "placed". 

Being "placed" means that they should be added to the
FloatingObjectTree, which is really only used consistently by the legacy
engine. NG only messes with it by accident. Have LayoutNG stay out of
that business more than before, by introducing a "has geometry" flag.

I really doubt that it's a splendid idea to populate the
FloatingObjectTree at all in LayoutNG formatting contexts, but it does
happen sometimes when laying out legacy objects in an NG container that
also has floats. See bug report for further analysis.

Bug: 906077
Change-Id: Iacebf5b8ca64e16c954a25745c8c2b01a18e1440
Reviewed-on: https://chromium-review.googlesource.com/c/1341915Reviewed-by: default avatarIan Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: default avatarAleks Totic <atotic@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609671}
parent 0554cbe7
Hide whitespace changes
Inline Side-by-side
Showing with 69 additions and 13 deletions
third_party/WebKit/LayoutTests/fast/block/float/add-remove-float-after-table-relayout-crash.html 0 → 100644
View file @ d97346eb
<!DOCTYPE html>
<style>body { overflow:scroll; }</style>
<p>PASS if no crash or DCHECK failure</p>
<div id="container" style="width:300px;">
<div style="display:table;"></div>
<div id="firstFloat" style="float:left;"></div>
<div id="secondFloat" style="display:none; float:left;"></div>
</div>
<script src="../../../resources/testharness.js"></script>
<script src="../../../resources/testharnessreport.js"></script>
<script>
test(() => {
document.body.offsetTop;
container.style.width = "301px";
document.body.offsetTop;
secondFloat.style.display = "block";
document.body.offsetTop;
secondFloat.style.display = "none";
}, "PASS if no crash or DCHECK failure");
</script>
third_party/blink/renderer/core/layout/floating_objects.cc
View file @ d97346eb
......@@ -57,7 +57,8 @@ FloatingObject::FloatingObject(LayoutBox* layout_object, Type type)
is_lowest_non_overhanging_float_in_child_(false)
#if DCHECK_IS_ON()
,
is_in_placed_tree_(false)
is_in_placed_tree_(false),
has_geometry_(false)
#endif
{
}
......@@ -79,7 +80,8 @@ FloatingObject::FloatingObject(LayoutBox* layout_object,
is_lowest_non_overhanging_float_in_child)
#if DCHECK_IS_ON()
,
is_in_placed_tree_(false)
is_in_placed_tree_(false),
has_geometry_(false)
#endif
{
}
......@@ -122,6 +124,9 @@ std::unique_ptr<FloatingObject> FloatingObject::UnsafeClone() const {
new FloatingObject(GetLayoutObject(), GetType(), frame_rect_,
should_paint_, is_descendant_, false));
clone_object->is_placed_ = is_placed_;
#if DCHECK_IS_ON()
clone_object->has_geometry_ = has_geometry_;
#endif
return clone_object;
}
......@@ -511,6 +516,7 @@ inline FloatingObjectInterval FloatingObjects::IntervalForFloatingObject(
}
void FloatingObjects::AddPlacedObject(FloatingObject& floating_object) {
DCHECK(!layout_object_->IsLayoutNGMixin());
DCHECK(!floating_object.IsInPlacedTree());
floating_object.SetIsPlaced(true);
......@@ -524,6 +530,7 @@ void FloatingObjects::AddPlacedObject(FloatingObject& floating_object) {
}
void FloatingObjects::RemovePlacedObject(FloatingObject& floating_object) {
DCHECK(!layout_object_->IsLayoutNGMixin());
DCHECK(floating_object.IsPlaced());
DCHECK(floating_object.IsInPlacedTree());
......
third_party/blink/renderer/core/layout/floating_objects.h
View file @ d97346eb
......@@ -66,22 +66,39 @@ class FloatingObject {
LayoutBox* GetLayoutObject() const { return layout_object_; }
bool IsPlaced() const { return is_placed_; }
void SetIsPlaced(bool placed = true) { is_placed_ = placed; }
void SetIsPlaced(bool placed = true) {
is_placed_ = placed;
#if DCHECK_IS_ON()
has_geometry_ = placed;
#endif
}
#if DCHECK_IS_ON()
void SetHasGeometry() { has_geometry_ = true; }
#endif
bool HasGeometry() const {
#if DCHECK_IS_ON()
return has_geometry_;
#else
return true;
#endif
}
LayoutUnit X() const {
DCHECK(IsPlaced());
DCHECK(HasGeometry());
return frame_rect_.X();
}
LayoutUnit MaxX() const {
DCHECK(IsPlaced());
DCHECK(HasGeometry());
return frame_rect_.MaxX();
}
LayoutUnit Y() const {
DCHECK(IsPlaced());
DCHECK(HasGeometry());
return frame_rect_.Y();
}
LayoutUnit MaxY() const {
DCHECK(IsPlaced());
DCHECK(HasGeometry());
return frame_rect_.MaxY();
}
LayoutUnit Width() const { return frame_rect_.Width(); }
......@@ -105,7 +122,7 @@ class FloatingObject {
}
const LayoutRect& FrameRect() const {
DCHECK(IsPlaced());
DCHECK(HasGeometry());
return frame_rect_;
}
......@@ -149,6 +166,14 @@ class FloatingObject {
unsigned is_placed_ : 1;
unsigned is_lowest_non_overhanging_float_in_child_ : 1;
unsigned is_in_placed_tree_ : 1;
#if DCHECK_IS_ON()
// If set, it's safe to read out position data for this float. For LayoutNG
// this will always be true, while for legacy layout, it depends on whether
// the float IsPlaced() or not.
unsigned has_geometry_ : 1;
#endif
DISALLOW_COPY_AND_ASSIGN(FloatingObject);
};
......
third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm_test.cc
View file @ d97346eb
......@@ -1108,14 +1108,12 @@ TEST_F(NGBlockLayoutAlgorithmTest, PositionFloatInsideEmptyBlocks) {
->MutableSet();
ASSERT_EQ(2UL, floating_objects.size());
auto left_floating_object = floating_objects.TakeFirst();
ASSERT_TRUE(left_floating_object->IsPlaced());
// 80 = float_inline_offset(25) + accumulative offset of empty blocks(35 + 20)
EXPECT_THAT(left_floating_object->X(), LayoutUnit(15));
// 10 = left float's margin
EXPECT_THAT(left_floating_object->Y(), LayoutUnit());
auto right_floating_object = floating_objects.TakeFirst();
ASSERT_TRUE(right_floating_object->IsPlaced());
// 150 = float_inline_offset(25) +
// right float offset(125)
EXPECT_THAT(right_floating_object->X(), LayoutUnit(140));
......
third_party/blink/renderer/core/layout/ng/ng_block_node.cc
View file @ d97346eb
......@@ -710,7 +710,6 @@ void NGBlockNode::CopyChildFragmentPosition(
if (IsFloatFragment(fragment) && containing_block->IsLayoutBlockFlow()) {
FloatingObject* floating_object =
ToLayoutBlockFlow(containing_block)->InsertFloatingObject(*layout_box);
floating_object->SetIsInPlacedTree(false);
floating_object->SetShouldPaint(!layout_box->HasSelfPaintingLayer());
LayoutUnit horizontal_margin_edge_offset = horizontal_offset;
if (has_flipped_x_axis)
......@@ -720,8 +719,15 @@ void NGBlockNode::CopyChildFragmentPosition(
floating_object->SetX(horizontal_margin_edge_offset);
floating_object->SetY(fragment_offset.top + additional_offset.top -
layout_box->MarginTop());
floating_object->SetIsPlaced(true);
floating_object->SetIsInPlacedTree(true);
#if DCHECK_IS_ON()
// Being "placed" is a legacy thing. Make sure the flags remain unset in NG.
DCHECK(!floating_object->IsPlaced());
DCHECK(!floating_object->IsInPlacedTree());
// Set this flag to tell the float machinery that it's safe to read out
// position data.
floating_object->SetHasGeometry();
#endif
}
}
......
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