[css-pseudo] Don't update non-normal markers if list-style-type changes

If a ::marker has the 'content' property set to something different than
'normal', then the contents of the marker should be determined according
to that value, not from 'list-style-type'.

Before this patch, the behavior was already correct when laying out the
marker for the first time. But if 'list-style-type' changed afterwards,
the marker was updated to reflect the new value, ignoring 'content'.

BUG=457718

TEST=external/wpt/css/css-pseudo/marker-content-021.html

The test fails in legacy since 'content' is not supported yet.

Change-Id: Id845774739927c9c574a66e9d1cc94e6ea59d5e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2020957Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#736105}

third_party/blink/renderer/core/layout/ng/list/list_marker.cc

@@ -12,8 +12,7 @@
 
 namespace blink {
 
-ListMarker::ListMarker()
-    : marker_type_(kStatic), is_marker_text_updated_(false) {}
+ListMarker::ListMarker() : marker_text_type_(kNotText) {}
 
 const ListMarker* ListMarker::Get(const LayoutObject* object) {
   if (!object)
@@ -33,17 +32,17 @@ ListMarker* ListMarker::Get(LayoutObject* object) {
 // If the value of ListStyleType changed, we need to the marker text has been
 // updated.
 void ListMarker::ListStyleTypeChanged(LayoutObject& marker) {
-  if (!is_marker_text_updated_)
+  if (marker_text_type_ == kNotText || marker_text_type_ == kUnresolved)
     return;
 
-  is_marker_text_updated_ = false;
+  marker_text_type_ = kUnresolved;
   marker.SetNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(
       layout_invalidation_reason::kListStyleTypeChange);
 }
 
 void ListMarker::OrdinalValueChanged(LayoutObject& marker) {
-  if (marker_type_ == kOrdinalValue && is_marker_text_updated_) {
-    is_marker_text_updated_ = false;
+  if (marker_text_type_ == kOrdinalValue) {
+    marker_text_type_ = kUnresolved;
     marker.SetNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(
         layout_invalidation_reason::kListValueChange);
   }
@@ -51,10 +50,12 @@ void ListMarker::OrdinalValueChanged(LayoutObject& marker) {
 
 void ListMarker::UpdateMarkerText(LayoutObject& marker, LayoutText* text) {
   DCHECK(text);
+  DCHECK_EQ(marker_text_type_, kUnresolved);
   StringBuilder marker_text_builder;
-  marker_type_ = MarkerText(marker, &marker_text_builder, kWithSuffix);
+  marker_text_type_ = MarkerText(marker, &marker_text_builder, kWithSuffix);
   text->SetTextIfNeeded(marker_text_builder.ToString().ReleaseImpl());
-  is_marker_text_updated_ = true;
+  DCHECK_NE(marker_text_type_, kNotText);
+  DCHECK_NE(marker_text_type_, kUnresolved);
 }
 
 void ListMarker::UpdateMarkerText(LayoutObject& marker) {
@@ -65,20 +66,21 @@ LayoutNGListItem* ListMarker::ListItem(const LayoutObject& marker) {
   return ToLayoutNGListItem(marker.GetNode()->parentNode()->GetLayoutObject());
 }
 
-ListMarker::MarkerType ListMarker::MarkerText(const LayoutObject& marker,
-                                              StringBuilder* text,
-                                              MarkerTextFormat format) const {
+ListMarker::MarkerTextType ListMarker::MarkerText(
+    const LayoutObject& marker,
+    StringBuilder* text,
+    MarkerTextFormat format) const {
   if (IsMarkerImage(marker)) {
     if (format == kWithSuffix)
       text->Append(' ');
-    return kStatic;
+    return kNotText;
   }
 
   LayoutNGListItem* list_item = ListItem(marker);
   const ComputedStyle& style = list_item->StyleRef();
   switch (style.ListStyleType()) {
     case EListStyleType::kNone:
-      return kStatic;
+      return kNotText;
     case EListStyleType::kString: {
       text->Append(style.ListStyleStringValue());
       return kStatic;
@@ -181,8 +183,7 @@ void ListMarker::UpdateMarkerContentIfNeeded(LayoutObject& marker) {
   // There should be at most one child.
   DCHECK(!child || !child->SlowFirstChild());
   if (!marker.StyleRef().ContentBehavesAsNormal()) {
-    marker_type_ = kStatic;
-    is_marker_text_updated_ = true;
+    marker_text_type_ = kNotText;
   } else if (IsMarkerImage(marker)) {
     StyleImage* list_style_image = list_item->StyleRef().ListStyleImage();
     if (child) {
@@ -207,9 +208,9 @@ void ListMarker::UpdateMarkerContentIfNeeded(LayoutObject& marker) {
       image->SetIsGeneratedContent();
       marker.AddChild(image);
     }
+    marker_text_type_ = kNotText;
   } else if (list_item->StyleRef().ListStyleType() == EListStyleType::kNone) {
-    marker_type_ = kStatic;
-    is_marker_text_updated_ = true;
+    marker_text_type_ = kNotText;
   } else {
     // Create a LayoutText in it.
     LayoutText* text = nullptr;
@@ -232,14 +233,14 @@ void ListMarker::UpdateMarkerContentIfNeeded(LayoutObject& marker) {
       text = LayoutText::CreateEmptyAnonymous(marker.GetDocument(), text_style,
                                               LegacyLayout::kAuto);
       marker.AddChild(text);
-      is_marker_text_updated_ = false;
+      marker_text_type_ = kUnresolved;
     }
   }
 }
 
 LayoutObject* ListMarker::SymbolMarkerLayoutText(
     const LayoutObject& marker) const {
-  if (marker_type_ != kSymbolValue)
+  if (marker_text_type_ != kSymbolValue)
     return nullptr;
   return marker.SlowFirstChild();
 }

third_party/blink/renderer/core/layout/ng/list/list_marker.h

@@ -34,7 +34,7 @@ class CORE_EXPORT ListMarker {
   }
 
   void UpdateMarkerTextIfNeeded(LayoutObject& marker) {
-    if (!is_marker_text_updated_ && !IsMarkerImage(marker))
+    if (marker_text_type_ == kUnresolved)
       UpdateMarkerText(marker);
   }
   void UpdateMarkerContentIfNeeded(LayoutObject&);
@@ -45,17 +45,25 @@ class CORE_EXPORT ListMarker {
 
  private:
   enum MarkerTextFormat { kWithSuffix, kWithoutSuffix };
-  enum MarkerType { kStatic, kOrdinalValue, kSymbolValue };
-  MarkerType MarkerText(const LayoutObject&,
-                        StringBuilder*,
-                        MarkerTextFormat) const;
+  enum MarkerTextType {
+    kNotText,  // The marker doesn't have a LayoutText, either because it has
+               // not been created yet or because 'list-style-type' is 'none',
+               // 'list-style-image' is not 'none', or 'content' is not
+               // 'normal'.
+    kUnresolved,    // The marker has a LayoutText that needs to be updated.
+    kOrdinalValue,  // The marker text depends on the ordinal.
+    kStatic,        // The marker text doesn't depend on the ordinal.
+    kSymbolValue,   // Like kStatic, but the marker is painted as a symbol.
+  };
+  MarkerTextType MarkerText(const LayoutObject&,
+                            StringBuilder*,
+                            MarkerTextFormat) const;
   void UpdateMarkerText(LayoutObject&);
   void UpdateMarkerText(LayoutObject&, LayoutText*);
 
   void ListStyleTypeChanged(LayoutObject&);
 
-  unsigned marker_type_ : 2;  // Type
-  unsigned is_marker_text_updated_ : 1;
+  unsigned marker_text_type_ : 3;  // MarkerTextType
 };
 
 }  // namespace blink

third_party/blink/web_tests/FlagExpectations/disable-layout-ng

@@ -84,6 +84,7 @@ crbug.com/457718 external/wpt/css/css-pseudo/marker-content-017.html [ Failure ]
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-018.html [ Failure ]
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-019.html [ Failure ]
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-020.html [ Failure ]
+crbug.com/457718 external/wpt/css/css-pseudo/marker-content-021.html [ Failure ]
 crbug.com/1012289 external/wpt/css/css-pseudo/marker-unicode-bidi-default.html [ Failure ]
 crbug.com/1012289 external/wpt/css/css-pseudo/marker-unicode-bidi-normal.html [ Failure ]
 

third_party/blink/web_tests/external/wpt/css/css-pseudo/marker-content-021-ref.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Reference: ::marker pseudo elements styled with 'content' property</title>
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<style>
+::marker {
+  content: "[m]";
+}
+.inside {
+  list-style-position: inside;
+}
+</style>
+<ol class="inside">
+  <li>inside none→symbol</li>
+  <li>inside symbol→decimal</li>
+  <li>inside decimal→roman</li>
+  <li>inside roman→string</li>
+  <li>inside string→none</li>
+</ol>
+<ol class="outside">
+  <li>outside none→symbol</li>
+  <li>outside symbol→decimal</li>
+  <li>outside decimal→roman</li>
+  <li>outside roman→string</li>
+  <li>outside string→none</li>
+</ol>

third_party/blink/web_tests/external/wpt/css/css-pseudo/marker-content-021.html

+<!DOCTYPE html>
+<html class="reftest-wait">
+<meta charset="utf-8">
+<title>CSS Test: ::marker pseudo elements styled with 'content' property</title>
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<link rel="match" href="marker-content-021-ref.html">
+<link rel="help" href="https://drafts.csswg.org/css-pseudo-4/#marker-pseudo">
+<link rel="help" href="https://drafts.csswg.org/css-lists/#content-property">
+<meta name="assert" content="Checks that non-normal ::marker is not updated when 'list-style-type' changes dynamically.">
+<style>
+::marker {
+  content: "[m]";
+}
+.inside {
+  list-style-position: inside;
+}
+.none {
+  list-style-type: none;
+}
+.symbol {
+  list-style-type: disc;
+}
+.decimal {
+  list-style-type: decimal;
+}
+.roman {
+  list-style-type: lower-roman;
+}
+.string {
+  list-style-type: "string";
+}
+</style>
+<ol class="inside">
+  <li class="none">inside none→symbol</li>
+  <li class="symbol">inside symbol→decimal</li>
+  <li class="decimal">inside decimal→roman</li>
+  <li class="roman">inside roman→string</li>
+  <li class="string">inside string→none</li>
+</ol>
+<ol class="outside">
+  <li class="none">outside none→symbol</li>
+  <li class="symbol">outside symbol→decimal</li>
+  <li class="decimal">outside decimal→roman</li>
+  <li class="roman">outside roman→string</li>
+  <li class="string">outside string→none</li>
+</ol>
+<script src="/common/reftest-wait.js"></script>
+<script>
+"use strict";
+// Use a "load" event listener and requestAnimationFrame to ensure that
+// the markers will have been laid out.
+addEventListener("load", function() {
+  requestAnimationFrame(() => {
+    for (let list of document.querySelectorAll("ol")) {
+      // Rotate class names among list items
+      const firstClass = list.firstElementChild.className;
+      for (let item of list.children) {
+        const next = item.nextElementSibling;
+        item.className = next ? next.className : firstClass;
+      }
+    }
+    takeScreenshot();
+  });
+}, {once: true});
+</script>
+</html>