From a4fdbc30ea27f47be4bee28b367b858b79861c77 Mon Sep 17 00:00:00 2001
From: Oriol Brufau <obrufau@igalia.com>
Date: Fri, 1 Sep 2023 22:18:19 +0200
Subject: [PATCH] Fix PlacementAmongFloats to avoid missing some bands (#30280)

PlacementAmongFloats would stop iterating when current_bands would be
empty, even if next_band wasn't at infinity.

Then the BFC root or replaced block was placed after all the floats,
even if it could fit next to some of them.

This patch moves the next_band into current_bands so that the loop
keeps considering bands.
---
 components/layout_2020/flow/float.rs          | 20 +++++++++++--
 .../CSS2/floats/floats-wrap-bfc-008.html.ini  |  2 ++
 tests/wpt/meta/MANIFEST.json                  | 13 ++++++++
 .../css/CSS2/css1/c414-flt-fit-001.xht.ini    |  2 --
 .../css/CSS2/floats/floats-wrap-bfc-008.html  | 30 +++++++++++++++++++
 5 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100644 tests/wpt/meta-legacy-layout/css/CSS2/floats/floats-wrap-bfc-008.html.ini
 delete mode 100644 tests/wpt/meta/css/CSS2/css1/c414-flt-fit-001.xht.ini
 create mode 100644 tests/wpt/tests/css/CSS2/floats/floats-wrap-bfc-008.html

diff --git a/components/layout_2020/flow/float.rs b/components/layout_2020/flow/float.rs
index f0230f2ac40..be275520690 100644
--- a/components/layout_2020/flow/float.rs
+++ b/components/layout_2020/flow/float.rs
@@ -200,14 +200,29 @@ impl<'a> PlacementAmongFloats<'a> {
         })
     }
 
+    /// Checks if we either have bands or we have gone past all of them.
+    /// This is an invariant that should hold, otherwise we are in a broken state.
+    fn has_bands_or_at_end(&self) -> bool {
+        !self.current_bands.is_empty() || self.next_band.top.px().is_infinite()
+    }
+
+    fn pop_front_band_ensuring_has_bands_or_at_end(&mut self) {
+        self.current_bands.pop_front();
+        if !self.has_bands_or_at_end() {
+            self.add_one_band();
+        }
+    }
+
     /// Run the placement algorithm for this [PlacementAmongFloats].
     pub(crate) fn place(&mut self) -> Rect<Length> {
+        debug_assert!(self.has_bands_or_at_end());
         while !self.current_bands.is_empty() {
             if let Some(result) = self.try_place_once() {
                 return result;
             }
-            self.current_bands.pop_front();
+            self.pop_front_band_ensuring_has_bands_or_at_end();
         }
+        debug_assert!(self.has_bands_or_at_end());
 
         // We could not fit the object in among the floats, so we place it as if it
         // cleared all floats.
@@ -237,6 +252,7 @@ impl<'a> PlacementAmongFloats<'a> {
         block_size_after_layout: Length,
         size_from_placement: &Vec2<Length>,
     ) -> bool {
+        debug_assert!(self.has_bands_or_at_end());
         debug_assert_eq!(size_from_placement.block, self.current_bands_height());
         debug_assert_eq!(
             size_from_placement.inline,
@@ -268,7 +284,7 @@ impl<'a> PlacementAmongFloats<'a> {
                 if available_inline_size < self.object_size.inline {
                     self.next_band = self.current_bands[old_num_bands];
                     self.current_bands.truncate(old_num_bands);
-                    self.current_bands.pop_front();
+                    self.pop_front_band_ensuring_has_bands_or_at_end();
                 }
                 return false;
             }
diff --git a/tests/wpt/meta-legacy-layout/css/CSS2/floats/floats-wrap-bfc-008.html.ini b/tests/wpt/meta-legacy-layout/css/CSS2/floats/floats-wrap-bfc-008.html.ini
new file mode 100644
index 00000000000..c097860913a
--- /dev/null
+++ b/tests/wpt/meta-legacy-layout/css/CSS2/floats/floats-wrap-bfc-008.html.ini
@@ -0,0 +1,2 @@
+[floats-wrap-bfc-008.html]
+  expected: FAIL
diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json
index 0f85b75ceaa..fa5e955080c 100644
--- a/tests/wpt/meta/MANIFEST.json
+++ b/tests/wpt/meta/MANIFEST.json
@@ -61829,6 +61829,19 @@
         {}
        ]
       ],
+      "floats-wrap-bfc-008.html": [
+       "5da80756d5f00f19afca1c05af462cac3af62a67",
+       [
+        null,
+        [
+         [
+          "/css/CSS2/reference/ref-filled-green-100px-square.xht",
+          "=="
+         ]
+        ],
+        {}
+       ]
+      ],
       "floats-wrap-bfc-outside-001.xht": [
        "5ba6a9750e7bafcb0a1fa1e5e420f337943a232a",
        [
diff --git a/tests/wpt/meta/css/CSS2/css1/c414-flt-fit-001.xht.ini b/tests/wpt/meta/css/CSS2/css1/c414-flt-fit-001.xht.ini
deleted file mode 100644
index 3fc224bb62d..00000000000
--- a/tests/wpt/meta/css/CSS2/css1/c414-flt-fit-001.xht.ini
+++ /dev/null
@@ -1,2 +0,0 @@
-[c414-flt-fit-001.xht]
-  expected: FAIL
diff --git a/tests/wpt/tests/css/CSS2/floats/floats-wrap-bfc-008.html b/tests/wpt/tests/css/CSS2/floats/floats-wrap-bfc-008.html
new file mode 100644
index 00000000000..5da80756d5f
--- /dev/null
+++ b/tests/wpt/tests/css/CSS2/floats/floats-wrap-bfc-008.html
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<title>CSS Test: BFC root after 2 floats</title>
+<link rel="author" title="Oriol Brufau" href="obrufau@igalia.com">
+<link rel="help" href="https://drafts.csswg.org/css2/#floats">
+<link rel="match" href="../reference/ref-filled-green-100px-square.xht">
+<meta name="assert" content="
+  The BFC root doesn't fit next to the 1st float, but fits next to the 2nd one,
+  so it should be placed next to the 2nd one.">
+<style>
+.wrapper {
+  width: 100px;
+}
+.float {
+  float: left;
+  height: 50px;
+  background: green;
+}
+.bfc {
+  overflow: hidden;
+  width: 50px;
+  height: 50px;
+  background: green;
+}
+</style>
+<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
+<div class="wrapper">
+  <div class="float" style="width: 100px"></div>
+  <div class="float" style="width: 50px"></div>
+  <div class="bfc" style=""></div>
+</div>