From 879af72850c24c00b4e03be5693b2e94a7a7f084 Mon Sep 17 00:00:00 2001
From: Christoffer Sandberg <christoffer.todderod@gmail.com>
Date: Fri, 22 Nov 2024 10:54:00 +0100
Subject: [PATCH] LibWeb: Improve painting of nested inline elements

PaintableWithLines created from inline nodes need to examine their
children to fully compute their size and offset as nested children
might have textnodes that are placed before their parent.

Fixes #1286
---
 Libraries/LibWeb/Layout/LayoutState.cpp       | 47 ++++++++++++++-----
 .../Ref/expected/inline-nested-node-ref.html  | 20 ++++++++
 .../LibWeb/Ref/input/inline-nested-node.html  | 21 +++++++++
 .../expected/HTML/dimension-attributes.txt    |  6 +--
 4 files changed, 80 insertions(+), 14 deletions(-)
 create mode 100644 Tests/LibWeb/Ref/expected/inline-nested-node-ref.html
 create mode 100644 Tests/LibWeb/Ref/input/inline-nested-node.html

diff --git a/Libraries/LibWeb/Layout/LayoutState.cpp b/Libraries/LibWeb/Layout/LayoutState.cpp
index bc5cc21e530..7a0dae675a8 100644
--- a/Libraries/LibWeb/Layout/LayoutState.cpp
+++ b/Libraries/LibWeb/Layout/LayoutState.cpp
@@ -312,8 +312,9 @@ void LayoutState::commit(Box& root)
         auto* paintable = inline_node->first_paintable();
         if (paintable)
             continue;
-        paintable = inline_node->create_paintable_for_line_with_index(0);
-        inline_node->add_paintable(paintable);
+        auto line_paintable = inline_node->create_paintable_for_line_with_index(0);
+        inline_node->add_paintable(line_paintable);
+        inline_node_paintables.set(line_paintable.ptr());
     }
 
     // Resolve relative positions for regular boxes (not line box fragments):
@@ -377,17 +378,42 @@ void LayoutState::commit(Box& root)
             continue;
         }
 
-        auto const& fragments = paintable_with_lines->fragments();
-        if (fragments.is_empty()) {
-            continue;
+        Optional<CSSPixelPoint> offset;
+        CSSPixelSize size;
+        auto line_index = paintable_with_lines->line_index();
+        paintable_with_lines->for_each_in_inclusive_subtree_of_type<Painting::PaintableWithLines>([&offset, &size, &line_index](auto& paintable) {
+            if (paintable.line_index() == line_index) {
+                auto const& fragments = paintable.fragments();
+                if (!fragments.is_empty()) {
+                    if (!offset.has_value() || (fragments.first().offset().x() < offset.value().x())) {
+                        offset = fragments.first().offset();
+                    }
+                }
+                for (auto const& fragment : fragments) {
+                    // FIXME: Padding and margin of nested inlines not included in fragment width
+                    size.set_width(size.width() + fragment.width());
+                }
+            }
+            return TraversalDecision::Continue;
+        });
+
+        if (offset.has_value()) {
+            if (!paintable_with_lines->fragments().is_empty()) {
+                offset.value().set_y(paintable_with_lines->fragments().first().offset().y());
+            }
+            // FIXME: If this paintable does not have any fragment we do no know the y offset. It should be where text should
+            // start if there had been any for this node. Pick y offset of the leftmost fragment in the inclusive subtree in the meantime.
+            paintable_with_lines->set_offset(offset.value());
         }
 
-        paintable_with_lines->set_offset(fragments.first().offset());
-        CSSPixelSize size;
-        for (auto const& fragment : fragments) {
-            size.set_width(size.width() + fragment.width());
-            size.set_height(max(size.height(), fragment.height()));
+        if (!paintable_with_lines->fragments().is_empty()) {
+            for (auto const& fragment : paintable_with_lines->fragments()) {
+                size.set_height(max(size.height(), fragment.height()));
+            }
+        } else {
+            size.set_height(paintable_with_lines->layout_node().computed_values().line_height());
         }
+
         paintable_with_lines->set_content_size(size.width(), size.height());
     }
 
@@ -686,5 +712,4 @@ void LayoutState::UsedValues::set_indefinite_content_height()
 {
     m_has_definite_height = false;
 }
-
 }
diff --git a/Tests/LibWeb/Ref/expected/inline-nested-node-ref.html b/Tests/LibWeb/Ref/expected/inline-nested-node-ref.html
new file mode 100644
index 00000000000..495e6895c84
--- /dev/null
+++ b/Tests/LibWeb/Ref/expected/inline-nested-node-ref.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<style>
+  .bluebg {
+    background-color: #0000FF7F;
+  }
+</style>
+<body>
+    <div>
+        <span class="bluebg">12</span>
+    </div>
+    <div>
+        <span class="bluebg">12</span>
+    </div>
+    <div>
+        <span class="bluebg">1234</span><span>consecutive inline item</span>
+    </div>
+    <div>
+        <span class="bluebg">24</span>
+    </div>
+</body>
diff --git a/Tests/LibWeb/Ref/input/inline-nested-node.html b/Tests/LibWeb/Ref/input/inline-nested-node.html
new file mode 100644
index 00000000000..a1d00ca7934
--- /dev/null
+++ b/Tests/LibWeb/Ref/input/inline-nested-node.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<link rel="match" href="../expected/inline-nested-node-ref.html">
+<style>
+  .bluebg {
+    background-color: #0000FF7F;
+  }
+</style>
+<body>
+    <div>
+        <span class="bluebg">1<span>2</span></span>
+    </div>
+    <div>
+        <span class="bluebg"><span>1</span>2</span>
+    </div>
+    <div>
+        <span class="bluebg">1<span>2</span>3<span>4</span></span><span>consecutive inline item</span>
+    </div>
+    <div>
+        <span class="bluebg"><span>2</span><span>4</span></span>
+    </div>
+</body>
diff --git a/Tests/LibWeb/Text/expected/HTML/dimension-attributes.txt b/Tests/LibWeb/Text/expected/HTML/dimension-attributes.txt
index f147fa7fda0..2c8999717bf 100644
--- a/Tests/LibWeb/Text/expected/HTML/dimension-attributes.txt
+++ b/Tests/LibWeb/Text/expected/HTML/dimension-attributes.txt
@@ -52,9 +52,9 @@ Test embed.vspace = "120." maps to marginBottom: 120px
 Test embed.width = "100" maps to width: 0px
 Test embed.width = " 00110 " maps to width: 0px
 Test embed.width = "120." maps to width: 0px
-Test embed.height = "100" maps to height: 0px
-Test embed.height = " 00110 " maps to height: 0px
-Test embed.height = "120." maps to height: 0px
+Test embed.height = "100" maps to height: 17px
+Test embed.height = " 00110 " maps to height: 17px
+Test embed.height = "120." maps to height: 17px
 Test tr.height = "100" maps to height: 100px
 Test tr.height = " 00110 " maps to height: 110px
 Test tr.height = "120." maps to height: 120px