[LayoutNG] Improve handling of adjoining floats.

A float always needs to be positioned by its block parent, so passing them around to children, parents or siblings really shouldn't be necessary (and it *was* somewhat confusing, since nobody but the direct float parent is allowed to touch them anyway, apart from placing them into temporary exclusion spaces). The main reason for passing them around like that, was for other blocks to determine, based on the list of floats being empty or not, the need for relayout once the BFC offset was resolved. Instead, confine the list of unpositioned floats to the block parent of those floats, and introduce the concept of adjoining float types (none, left, right, both). Adjoining floats occur when the BFC offset is unknown, meaning that their position may be affected by the current layout algorithm. Adjoining float types will now be the thing that's both input to and output from the layout algorithms. Having something other than "none" means that a block's BFC offset is unknown, but that doesn't automatically mean that we have to abort and re-layout if the BFC offset gets resolved. If the "floats BFC offset" is known, for instance, those adjoining floats may be positioned right away. Still we need to know about them (positioned or not), to get clearance correct. We're going to need to treat adjoining floats specially when applying clearance. Will deal with that in a later CL. For now, we just keep track of the adjoining float types, so that the clearance machinery can tell that there are floats there that may not yet be positioned. That used to be taken care of the list of unpositioned floats, but, as previously stated, adjoining floats are special, and we need to know about them, whether they are positioned or not. This is a preparatory CL for that. Each time we add an unpositioned float, we need to update the types of adjoining floats, so that these can be returned from the algorithm if necessary. Whether they end up being positioned right away or not isn't relevant. Adjoining is adjoining. Note that we don't have to #include the header file for unpositioned floats as much as before now, but I'll clean that up in a follow-up CL, because it turned out that there were quite a few translation units that got stuff for free via that header file, instead of explicitly including what they need. Had to rewrite how we deal with floats in HandleNewFormattingContext() and LayoutNewFormattingContext(), since those depended on a list of all preceding unpositioned floats to place them into a temporary exclusion space, to figure out whether to let the child's margin be adjoining with the current margin strut or not. Instead of using a temporary exclusion space, we now position floats via the regular mechanisms, and initially assume that the child's margin is going to be adjoining. This means that we have to abort and roll back if there are preceding unpositioned floats. This is no different from how we lay out regular blocks, though. What *is* different is that if it turns out that the child's margin has to be separated from the strut, we'll have to abort and roll back once *again* (but only once). The algorithms now need to keep track of whether they need to abort if the BFC offset *changes*, rather than if it is *resolved*. We only allow the offset to resolve and optionally change *once* afterwards, though. Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng Change-Id: Ie527d659213049f180ebedc764e1d7f4926a5876 Reviewed-on: https://chromium-review.googlesource.com/1030191 Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#555314}
Denger-Network · May 2, 2018 · dea199a · dea199a
1 parent e28bf02
commit dea199a
Show file tree

Hide file tree

Showing 26 changed files with 476 additions and 247 deletions.
diff --git a/...arty/WebKit/LayoutTests/external/wpt/css/CSS2/floats/new-fc-beside-adjoining-float-2.html b/...arty/WebKit/LayoutTests/external/wpt/css/CSS2/floats/new-fc-beside-adjoining-float-2.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<title>A new formatting context that fits beside an adjoining float, and thus pulls down the float with its top margin</title>
+<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
+<link rel="help" href="https://www.w3.org/TR/CSS22/visuren.html#bfc-next-to-float" title="9.5 Floats">
+<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">
+<meta name="assert" content="The float is adjoining with the box that establishes a new formatting context when it fits beside it, and will therefore be affected by its margin">
+<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="overflow:hidden; width:200px; height:200px; background:red;">
+  <div style="margin-top:190px;">
+    <div>
+      <div style="float:left; width:100px; height:200px; background:green;"></div>
+    </div>
+    <div style="margin-top:-190px; overflow:hidden; width:100px; height:200px; background:green;"></div>
+  </div>
+</div>
diff --git a/..._party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/new-fc-separates-from-float-2.html b/..._party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/new-fc-separates-from-float-2.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<title>A new formatting context that doesn't fit beside a float make the float non-adjoining</title>
+<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
+<link rel="help" href="https://www.w3.org/TR/CSS22/visuren.html#bfc-next-to-float" title="9.5 Floats">
+<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">
+<meta name="assert" content="Although the 'clear' property isn't specified in this test, a new formatting context that doesn't fit below a float that would otherwise be adjoining will need to separate its margin from the float, so that it doesn't affect the float. This is very similar to clearance.">
+<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div style="overflow:hidden; width:200px; height:200px; background:red;">
+  <div style="margin-top:-50px;">
+    <div>
+      <div style="float:left; width:200px; height:150px; background:green;"></div>
+    </div>
+    <div style="margin-top:12345px; overflow:hidden; width:200px; height:100px; background:green;"></div>
+  </div>
+</div>
diff --git a/third_party/blink/renderer/core/layout/BUILD.gn b/third_party/blink/renderer/core/layout/BUILD.gn
@@ -419,6 +419,8 @@ blink_core_sources("layout") {
     "ng/ng_layout_input_node.h",
     "ng/ng_layout_result.cc",
     "ng/ng_layout_result.h",
+    "ng/ng_layout_utils.cc",
+    "ng/ng_layout_utils.h",
     "ng/ng_length_utils.cc",
     "ng/ng_length_utils.h",
     "ng/ng_out_of_flow_layout_part.cc",

diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc b/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
@@ -84,7 +84,6 @@ NGInlineLayoutAlgorithm::NGInlineLayoutAlgorithm(
       is_horizontal_writing_mode_(
           blink::IsHorizontalWritingMode(space.GetWritingMode())) {
   quirks_mode_ = inline_node.InLineHeightQuirksMode();
-  unpositioned_floats_ = ConstraintSpace().UnpositionedFloats();
 
   if (!is_horizontal_writing_mode_)
     baseline_type_ = FontBaseline::kIdeographicBaseline;
@@ -552,10 +551,20 @@ scoped_refptr<NGLayoutResult> NGInlineLayoutAlgorithm::Layout() {
 
   bool is_empty_inline = Node().IsEmptyInline();
 
-  if (!is_empty_inline) {
-    DCHECK(ConstraintSpace().UnpositionedFloats().IsEmpty());
+  if (is_empty_inline) {
+    // We're just going to collapse through this one, so whatever went in on one
+    // side will go out on the other side. The position of the adjoining floats
+    // will be affected by any subsequent block, until the BFC offset is
+    // resolved.
+    container_builder_.AddAdjoiningFloatTypes(
+        ConstraintSpace().AdjoiningFloatTypes());
+  } else {
     DCHECK(ConstraintSpace().MarginStrut().IsEmpty());
     container_builder_.SetBfcOffset(ConstraintSpace().BfcOffset());
+
+    // The BFC offset was determined before entering this algorithm. This means
+    // that there should be no adjoining floats.
+    DCHECK(!ConstraintSpace().AdjoiningFloatTypes());
   }
 
   // In order to get the correct list of layout opportunities, we need to
@@ -569,7 +578,6 @@ scoped_refptr<NGLayoutResult> NGInlineLayoutAlgorithm::Layout() {
     DCHECK_EQ(handled_item_index, Node().Items().size());
 
     container_builder_.SwapPositionedFloats(&positioned_floats_);
-    container_builder_.SwapUnpositionedFloats(&unpositioned_floats_);
     container_builder_.SetEndMarginStrut(ConstraintSpace().MarginStrut());
     container_builder_.SetExclusionSpace(std::move(initial_exclusion_space));
 
@@ -606,10 +614,10 @@ scoped_refptr<NGLayoutResult> NGInlineLayoutAlgorithm::Layout() {
         std::make_unique<NGExclusionSpace>(*initial_exclusion_space);
 
     NGLineInfo line_info;
-    NGLineBreaker line_breaker(Node(), NGLineBreakerMode::kContent,
-                               constraint_space_, &positioned_floats,
-                               &unpositioned_floats_, exclusion_space.get(),
-                               handled_item_index, break_token);
+    NGLineBreaker line_breaker(
+        Node(), NGLineBreakerMode::kContent, constraint_space_,
+        &positioned_floats, &unpositioned_floats_, &container_builder_,
+        exclusion_space.get(), handled_item_index, break_token);
 
     // TODO(ikilpatrick): Does this always succeed when we aren't an empty
     // inline?
@@ -689,10 +697,12 @@ unsigned NGInlineLayoutAlgorithm::PositionLeadingItems(
       NGBoxStrut margins =
           ComputeMarginsForContainer(ConstraintSpace(), node.Style());
 
-      unpositioned_floats_.push_back(NGUnpositionedFloat::Create(
+      auto unpositioned_float = NGUnpositionedFloat::Create(
           ConstraintSpace().AvailableSize(),
           ConstraintSpace().PercentageResolutionSize(), bfc_line_offset,
-          bfc_line_offset, margins, node, /* break_token */ nullptr));
+          bfc_line_offset, margins, node, /* break_token */ nullptr);
+      AddUnpositionedFloat(&unpositioned_floats_, &container_builder_,
+                           std::move(unpositioned_float));
     } else if (is_empty_inline &&
                item.Type() == NGInlineItem::kOutOfFlowPositioned) {
       NGBlockNode node(ToLayoutBox(item.GetLayoutObject()));

diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc b/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc
@@ -463,8 +463,9 @@ static LayoutUnit ComputeContentSize(NGInlineNode node,
     unpositioned_floats.clear();
 
     NGLineBreaker line_breaker(node, mode, *space, &positioned_floats,
-                               &unpositioned_floats, &empty_exclusion_space, 0u,
-                               break_token.get());
+                               &unpositioned_floats,
+                               nullptr /* container_builder */,
+                               &empty_exclusion_space, 0u, break_token.get());
     if (!line_breaker.NextLine(opportunity, &line_info))
       break;
 

diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.cc b/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.cc
@@ -163,12 +163,12 @@ scoped_refptr<NGLayoutResult> NGLineBoxFragmentBuilder::ToLineBoxFragment() {
 
   return base::AdoptRef(new NGLayoutResult(
       std::move(fragment), oof_positioned_descendants_, positioned_floats_,
-      unpositioned_floats_, unpositioned_list_marker_,
-      std::move(exclusion_space_), bfc_offset_, end_margin_strut_,
+      unpositioned_list_marker_, std::move(exclusion_space_), bfc_offset_,
+      end_margin_strut_,
       /* intrinsic_block_size */ LayoutUnit(),
       /* minimal_space_shortage */ LayoutUnit::Max(), EBreakBetween::kAuto,
       EBreakBetween::kAuto, /* has_forced_break */ false, is_pushed_by_floats_,
-      NGLayoutResult::kSuccess));
+      adjoining_floats_, NGLayoutResult::kSuccess));
 }
 
 }  // namespace blink
diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc
@@ -41,6 +41,7 @@ NGLineBreaker::NGLineBreaker(
     const NGConstraintSpace& space,
     Vector<NGPositionedFloat>* positioned_floats,
     Vector<scoped_refptr<NGUnpositionedFloat>>* unpositioned_floats,
+    NGContainerFragmentBuilder* container_builder,
     NGExclusionSpace* exclusion_space,
     unsigned handled_float_index,
     const NGInlineBreakToken* break_token)
@@ -49,6 +50,7 @@ NGLineBreaker::NGLineBreaker(
       constraint_space_(space),
       positioned_floats_(positioned_floats),
       unpositioned_floats_(unpositioned_floats),
+      container_builder_(container_builder),
       exclusion_space_(exclusion_space),
       break_iterator_(node.Text()),
       shaper_(node.Text().Characters16(), node.Text().length()),
@@ -652,7 +654,8 @@ void NGLineBreaker::HandleFloat(const NGInlineItem& item,
   // Check if we already have a pending float. That's because a float cannot be
   // higher than any block or floated box generated before.
   if (!unpositioned_floats_->IsEmpty() || float_after_line) {
-    unpositioned_floats_->push_back(std::move(unpositioned_float));
+    AddUnpositionedFloat(unpositioned_floats_, container_builder_,
+                         std::move(unpositioned_float));
   } else {
     LayoutUnit origin_block_offset = bfc_block_offset_;
 

diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h
@@ -17,6 +17,7 @@
 namespace blink {
 
 class Hyphenation;
+class NGContainerFragmentBuilder;
 class NGInlineBreakToken;
 class NGInlineItem;
 class NGInlineLayoutStateStack;
@@ -38,6 +39,7 @@ class CORE_EXPORT NGLineBreaker {
                 const NGConstraintSpace&,
                 Vector<NGPositionedFloat>*,
                 Vector<scoped_refptr<NGUnpositionedFloat>>*,
+                NGContainerFragmentBuilder* container_builder,
                 NGExclusionSpace*,
                 unsigned handled_float_index,
                 const NGInlineBreakToken* = nullptr);
@@ -162,6 +164,7 @@ class CORE_EXPORT NGLineBreaker {
   const NGConstraintSpace& constraint_space_;
   Vector<NGPositionedFloat>* positioned_floats_;
   Vector<scoped_refptr<NGUnpositionedFloat>>* unpositioned_floats_;
+  NGContainerFragmentBuilder* container_builder_; /* May be nullptr */
   NGExclusionSpace* exclusion_space_;
   scoped_refptr<const ComputedStyle> current_style_;
 

diff --git a/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker_test.cc b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker_test.cc
@@ -53,6 +53,7 @@ class NGLineBreakerTest : public NGBaseLayoutAlgorithmTest {
     while (!break_token || !break_token->IsFinished()) {
       NGLineBreaker line_breaker(node, NGLineBreakerMode::kContent, *space,
                                  &positioned_floats, &unpositioned_floats,
+                                 /* container_builder */ nullptr,
                                  &exclusion_space, 0u, break_token.get());
       if (!line_breaker.NextLine(opportunity, &line_info))
         break;

diff --git a/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc b/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc
@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node_data.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_constraint_space.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_result.h"
+#include "third_party/blink/renderer/core/layout/ng/ng_layout_utils.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_length_utils.h"
 #include "third_party/blink/renderer/core/page/scrolling/root_scroller_util.h"
 #include "third_party/blink/renderer/core/paint/ng/ng_block_flow_painter.h"
@@ -147,9 +148,9 @@ scoped_refptr<NGLayoutResult> LayoutNGMixin<Base>::CachedLayoutResult(
     return nullptr;
   if (constraint_space != *cached_constraint_space_)
     return nullptr;
-  if (cached_constraint_space_->UnpositionedFloats().size() ||
-      cached_result_->UnpositionedFloats().size())
-    return nullptr;
+  // The checks above should be enough to bail if layout is incomplete, but
+  // let's verify:
+  DCHECK(IsBlockLayoutComplete(*cached_constraint_space_, *cached_result_));
   return cached_result_->CloneWithoutOffset();
 }