From e7498bb918a6fc64717473c6109fb57a34d09288 Mon Sep 17 00:00:00 2001
From: Tobias Christiansen <tobi@tobyase.de>
Date: Tue, 11 May 2021 22:32:54 +0200
Subject: [PATCH] LibWeb: Position the ListItemMarkers according to their width

Previously they were positioned with a fixed offset. However this lead
to wider markers with more than one character to collide with the
element itself.
Now the ListItemMarkerBox generates and stores the appropriate String
in its constructor and sets its own width according to that.
The ListItemBox then lays out the Marker taking this width into
account.
This also made the painting a lot easier since we don't generate the
needed Strings every time we repaint, just once.
---
 .../Libraries/LibWeb/Layout/ListItemBox.cpp   |  4 +-
 .../LibWeb/Layout/ListItemMarkerBox.cpp       | 50 ++++++++++++++-----
 .../LibWeb/Layout/ListItemMarkerBox.h         |  2 +
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/Userland/Libraries/LibWeb/Layout/ListItemBox.cpp b/Userland/Libraries/LibWeb/Layout/ListItemBox.cpp
index 38c27373c6..1fbc2e11ce 100644
--- a/Userland/Libraries/LibWeb/Layout/ListItemBox.cpp
+++ b/Userland/Libraries/LibWeb/Layout/ListItemBox.cpp
@@ -37,8 +37,8 @@ void ListItemBox::layout_marker()
         append_child(*m_marker);
     }
 
-    m_marker->set_offset(-8, 0);
-    m_marker->set_size(4, line_height());
+    m_marker->set_offset(-(m_marker->width() + 4), 0);
+    m_marker->set_height(line_height());
 }
 
 }
diff --git a/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.cpp b/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.cpp
index 0142a4f8de..e04e5a7b9f 100644
--- a/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.cpp
+++ b/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.cpp
@@ -16,6 +16,40 @@ ListItemMarkerBox::ListItemMarkerBox(DOM::Document& document, CSS::ListStyleType
     , m_list_style_type(style_type)
     , m_index(index)
 {
+    switch (m_list_style_type) {
+    case CSS::ListStyleType::Square:
+    case CSS::ListStyleType::Circle:
+    case CSS::ListStyleType::Disc:
+        break;
+    case CSS::ListStyleType::Decimal:
+        m_text = String::formatted("{}.", m_index);
+        break;
+    case CSS::ListStyleType::DecimalLeadingZero:
+        // This is weird, but in accordance to spec.
+        m_text = m_index < 10 ? String::formatted("0{}.", m_index) : String::formatted("{}.", m_index);
+        break;
+    case CSS::ListStyleType::LowerAlpha:
+    case CSS::ListStyleType::LowerLatin:
+        m_text = String::bijective_base_from(m_index).to_lowercase();
+        break;
+    case CSS::ListStyleType::UpperAlpha:
+    case CSS::ListStyleType::UpperLatin:
+        m_text = String::bijective_base_from(m_index);
+        break;
+    case CSS::ListStyleType::None:
+        break;
+
+    default:
+        VERIFY_NOT_REACHED();
+    }
+
+    if (m_text.is_null()) {
+        set_width(4);
+        return;
+    }
+
+    auto text_width = font().width(m_text);
+    set_width(text_width);
 }
 
 ListItemMarkerBox::~ListItemMarkerBox()
@@ -47,26 +81,18 @@ void ListItemMarkerBox::paint(PaintContext& context, PaintPhase phase)
         marker_rect.center_within(enclosing);
         context.painter().draw_ellipse_intersecting(marker_rect, color);
         break;
-    case CSS::ListStyleType::Decimal:
-        context.painter().draw_text(enclosing, String::formatted("{}.", m_index), Gfx::TextAlignment::Center);
-        break;
     case CSS::ListStyleType::Disc:
         context.painter().fill_ellipse(marker_rect, color);
         break;
+    case CSS::ListStyleType::Decimal:
     case CSS::ListStyleType::DecimalLeadingZero:
-        // This is weird, but in accordance to spec.
-        context.painter().draw_text(
-            enclosing,
-            m_index < 10 ? String::formatted("0{}.", m_index) : String::formatted("{}.", m_index),
-            Gfx::TextAlignment::Center);
-        break;
     case CSS::ListStyleType::LowerAlpha:
     case CSS::ListStyleType::LowerLatin:
-        context.painter().draw_text(enclosing, String::bijective_base_from(m_index).to_lowercase(), Gfx::TextAlignment::Center);
-        break;
     case CSS::ListStyleType::UpperAlpha:
     case CSS::ListStyleType::UpperLatin:
-        context.painter().draw_text(enclosing, String::bijective_base_from(m_index), Gfx::TextAlignment::Center);
+        if (m_text.is_null())
+            break;
+        context.painter().draw_text(enclosing, m_text, Gfx::TextAlignment::Center);
         break;
     case CSS::ListStyleType::None:
         return;
diff --git a/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.h b/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.h
index c32165df71..0c6c60cc73 100644
--- a/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.h
+++ b/Userland/Libraries/LibWeb/Layout/ListItemMarkerBox.h
@@ -21,6 +21,8 @@ public:
 private:
     CSS::ListStyleType m_list_style_type { CSS::ListStyleType::None };
     size_t m_index;
+
+    String m_text {};
 };
 
 }