Paint disclosure-* list markers as images even in counter()

Counters with disclosure-open and disclosure-closed list markers
should be sensitive to text direction and writing-mode. So we should
paint them as images.

https://w3c.github.io/csswg-drafts/css-counter-styles/#simple-symbolic
> When used in list-style-type, a UA may instead render these styles
> using a UA-generated image or a UA-chosen font instead of rendering
> the specified character in the element’s own font. If using an image,
> it must look similar to the character, and must be sized to
> attractively fill a 1em by 1em square.
>
> For the disclosure-open and disclosure-closed counter styles, the
> marker must be an image or character suitable for indicating the open
> and closed states of a disclosure widget, such as HTML’s details
> element.

* NGInlineNode:
  - CollectInlinesInternal(): Call SetIsSymbolMarker() for
    disclosure-* counters
  - ShapeText(): Support for IsSymbolMaker() with LayoutCounter

* ListMarkerPainter and NGTextFragmentPainter:
  Support for symbol markers with LayoutCounter

* ListMarker::RelativeSymbolMarkerRect() and WidthOfSymbol():
  Add list-style argument to support LayoutCounter

The new behavior matches to Firefox.

Bug: 1176195
Change-Id: Ic24e0bd50166a2a50c6970ecd63d6ed0aa0fb916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3709517
Reviewed-by: Koji Ishii <kojii@chromium.org>
Auto-Submit: Kent Tamura <tkent@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085290}

third_party/blink/renderer/core/layout/layout_counter.cc

@@ -606,15 +606,7 @@ scoped_refptr<StringImpl> LayoutCounter::OriginalText() const {
   DCHECK(child);
 
   int value = ValueForText(child);
-  const CounterStyle* counter_style = nullptr;
-  // Note: CSS3 spec doesn't allow 'none' but CSS2.1 allows it. We currently
-  // allow it for backward compatibility.
-  // See https://github.com/w3c/csswg-drafts/issues/5795 for details.
-  if (counter_->ListStyle() != "none") {
-    counter_style =
-        &GetDocument().GetStyleEngine().FindCounterStyleAcrossScopes(
-            counter_->ListStyle(), counter_->GetTreeScope());
-  }
+  const CounterStyle* counter_style = NullableCounterStyle();
   String text = GenerateCounterText(counter_style, value);
   // If the separator exists, we need to append all of the parent values as well,
   // including the ones that cross the style containment boundary.
@@ -871,6 +863,39 @@ CounterMap* LayoutCounter::GetCounterMap(LayoutObject* object) {
   return nullptr;
 }
 
+const CounterStyle* LayoutCounter::NullableCounterStyle() const {
+  // Note: CSS3 spec doesn't allow 'none' but CSS2.1 allows it. We currently
+  // allow it for backward compatibility.
+  // See https://github.com/w3c/csswg-drafts/issues/5795 for details.
+  if (counter_->ListStyle() == "none") {
+    return nullptr;
+  }
+  return &GetDocument().GetStyleEngine().FindCounterStyleAcrossScopes(
+      counter_->ListStyle(), counter_->GetTreeScope());
+}
+
+bool LayoutCounter::IsDirectionalSymbolMarker() const {
+  const auto* counter_style = NullableCounterStyle();
+  if (!counter_style || !counter_style->IsPredefinedSymbolMarker()) {
+    return false;
+  }
+  const AtomicString& list_style = counter_->ListStyle();
+  return list_style == "disclosure-open" || list_style == "disclosure-closed";
+}
+
+const AtomicString& LayoutCounter::Separator() const {
+  return counter_->Separator();
+}
+
+// static
+const AtomicString& LayoutCounter::ListStyle(const LayoutObject* object,
+                                             const ComputedStyle& style) {
+  if (const auto* counter = DynamicTo<LayoutCounter>(object)) {
+    return counter->counter_->ListStyle();
+  }
+  return style.ListStyleType()->GetCounterStyleName();
+}
+
 }  // namespace blink
 
 #if DCHECK_IS_ON()

third_party/blink/renderer/core/layout/layout_counter.h

@@ -74,6 +74,18 @@ class LayoutCounter : public LayoutText {
 
   void UpdateCounter();
 
+  // Returns true if <counter-style> is "disclosure-open" or
+  // "disclosure-closed".
+  bool IsDirectionalSymbolMarker() const;
+  // Returns <string> in counters().
+  const AtomicString& Separator() const;
+
+  // Returns LayoutCounter::counter_->ListStyle() if `object` is a
+  // LayoutCounter.
+  // Returns style.ListStyleType()->GetCounterStyleName() otherwise.
+  static const AtomicString& ListStyle(const LayoutObject* object,
+                                       const ComputedStyle& style);
+
   const char* GetName() const override {
     NOT_DESTROYED();
     return "LayoutCounter";
@@ -94,6 +106,8 @@ class LayoutCounter : public LayoutText {
   // changes.
   void Invalidate();
 
+  const CounterStyle* NullableCounterStyle() const;
+
   Member<const CounterContentData> counter_;
   Member<CounterNode> counter_node_;
   Member<LayoutCounter> next_for_same_counter_;

third_party/blink/renderer/core/layout/layout_list_marker.cc

@@ -259,7 +259,8 @@ MinMaxSizes LayoutListMarker::ComputeIntrinsicLogicalWidths() const {
       case ListMarker::ListStyleCategory::kNone:
         break;
       case ListMarker::ListStyleCategory::kSymbol:
-        sizes = ListMarker::WidthOfSymbol(StyleRef());
+        sizes = ListMarker::WidthOfSymbol(
+            StyleRef(), StyleRef().ListStyleType()->GetCounterStyleName());
         break;
       case ListMarker::ListStyleCategory::kLanguage:
       case ListMarker::ListStyleCategory::kStaticString:
@@ -358,7 +359,9 @@ LayoutRect LayoutListMarker::GetRelativeMarkerRect() const {
     case ListMarker::ListStyleCategory::kNone:
       return LayoutRect();
     case ListMarker::ListStyleCategory::kSymbol:
-      return ListMarker::RelativeSymbolMarkerRect(StyleRef(), Size().Width());
+      return ListMarker::RelativeSymbolMarkerRect(
+          StyleRef(), StyleRef().ListStyleType()->GetCounterStyleName(),
+          Size().Width());
     case ListMarker::ListStyleCategory::kLanguage:
     case ListMarker::ListStyleCategory::kStaticString: {
       const SimpleFontData* font_data = StyleRef().GetFont().PrimaryFont();

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

@@ -329,7 +329,8 @@ bool ListMarker::IsMarkerImage(const LayoutObject& marker) const {
          ListItem(marker)->StyleRef().GeneratesMarkerImage();
 }
 
-LayoutUnit ListMarker::WidthOfSymbol(const ComputedStyle& style) {
+LayoutUnit ListMarker::WidthOfSymbol(const ComputedStyle& style,
+                                     const AtomicString& list_style) {
   const Font& font = style.GetFont();
   const SimpleFontData* font_data = font.PrimaryFont();
   DCHECK(font_data);
@@ -339,9 +340,9 @@ LayoutUnit ListMarker::WidthOfSymbol(const ComputedStyle& style) {
     // See http://crbug.com/1228157
     return LayoutUnit();
   }
-  const AtomicString& name = style.ListStyleType()->GetCounterStyleName();
-  if (name == "disclosure-open" || name == "disclosure-closed")
+  if (list_style == "disclosure-open" || list_style == "disclosure-closed") {
     return DisclosureSymbolSize(style);
+  }
   return LayoutUnit((font_data->GetFontMetrics().Ascent() * 2 / 3 + 1) / 2 + 2);
 }
 
@@ -411,6 +412,7 @@ std::pair<LayoutUnit, LayoutUnit> ListMarker::InlineMarginsForOutside(
 }
 
 LayoutRect ListMarker::RelativeSymbolMarkerRect(const ComputedStyle& style,
+                                                const AtomicString& list_style,
                                                 LayoutUnit width) {
   LayoutRect relative_rect;
   const SimpleFontData* font_data = style.GetFont().PrimaryFont();
@@ -422,8 +424,7 @@ LayoutRect ListMarker::RelativeSymbolMarkerRect(const ComputedStyle& style,
   // http://crbug.com/543193
   const FontMetrics& font_metrics = font_data->GetFontMetrics();
   const int ascent = font_metrics.Ascent();
-  const AtomicString& name = style.ListStyleType()->GetCounterStyleName();
-  if (name == "disclosure-open" || name == "disclosure-closed") {
+  if (list_style == "disclosure-open" || list_style == "disclosure-closed") {
     LayoutUnit marker_size = DisclosureSymbolSize(style);
     relative_rect = LayoutRect(LayoutUnit(), ascent - marker_size, marker_size,
                                marker_size);

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

@@ -61,8 +61,11 @@ class CORE_EXPORT ListMarker {
       const ComputedStyle& list_item_style,
       LayoutUnit marker_inline_size);
 
-  static LayoutRect RelativeSymbolMarkerRect(const ComputedStyle&, LayoutUnit);
-  static LayoutUnit WidthOfSymbol(const ComputedStyle&);
+  static LayoutRect RelativeSymbolMarkerRect(const ComputedStyle&,
+                                             const AtomicString& list_style,
+                                             LayoutUnit);
+  static LayoutUnit WidthOfSymbol(const ComputedStyle&,
+                                  const AtomicString& list_style);
 
   // A reduced set of list style categories allowing for more concise expression
   // of list style specific logic.

third_party/blink/renderer/core/layout/list_marker_test.cc

@@ -316,7 +316,10 @@ TEST_F(ListMarkerTest, WidthOfSymbolForFontSizeZero) {
   const auto& target_layout_object = *target.GetLayoutObject();
 
   EXPECT_EQ(LayoutUnit(),
-            ListMarker::WidthOfSymbol(target_layout_object.StyleRef()));
+            ListMarker::WidthOfSymbol(target_layout_object.StyleRef(),
+                                      target_layout_object.StyleRef()
+                                          .ListStyleType()
+                                          ->GetCounterStyleName()));
 }
 
 // crbug.com/1310599

third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc

@@ -15,6 +15,7 @@
 #include "third_party/blink/renderer/core/layout/deferred_shaping.h"
 #include "third_party/blink/renderer/core/layout/deferred_shaping_controller.h"
 #include "third_party/blink/renderer/core/layout/layout_block_flow.h"
+#include "third_party/blink/renderer/core/layout/layout_counter.h"
 #include "third_party/blink/renderer/core/layout/layout_inline.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/layout/layout_object_inlines.h"
@@ -235,7 +236,40 @@ void CollectInlinesInternal(ItemsBuilder* builder,
   const LayoutObject* symbol =
       LayoutNGListItem::FindSymbolMarkerLayoutText(block);
   while (node) {
-    if (auto* layout_text = DynamicTo<LayoutText>(node)) {
+    if (auto* counter = DynamicTo<LayoutCounter>(node)) {
+      // According to
+      // https://w3c.github.io/csswg-drafts/css-counter-styles/#simple-symbolic,
+      // disclosure-* should have special rendering paths.
+      if (counter->IsDirectionalSymbolMarker()) {
+        const String& text = counter->GetText();
+        // We assume the text representation length for a predefined symbol
+        // marker is always 1.
+        if (text.length() <= 1) {
+          builder->AppendText(counter, previous_data);
+          builder->SetIsSymbolMarker();
+        } else {
+          // The text must be in the following form:
+          // Symbol, separator, symbol, separator, symbol, ...
+          builder->AppendText(text.Substring(0, 1), counter);
+          builder->SetIsSymbolMarker();
+          const AtomicString& separator = counter->Separator();
+          for (wtf_size_t i = 1; i < text.length();) {
+            if (separator.length() > 0) {
+              DCHECK_EQ(separator, text.Substring(i, separator.length()));
+              builder->AppendText(separator, counter);
+              i += separator.length();
+              DCHECK_LT(i, text.length());
+            }
+            builder->AppendText(text.Substring(i, 1), counter);
+            builder->SetIsSymbolMarker();
+            ++i;
+          }
+        }
+      } else {
+        builder->AppendText(counter, previous_data);
+      }
+      builder->ClearNeedsLayout(counter);
+    } else if (auto* layout_text = DynamicTo<LayoutText>(node)) {
       builder->AppendText(layout_text, previous_data);
 
       if (symbol == layout_text)
@@ -1303,7 +1337,9 @@ void NGInlineNode::ShapeText(NGInlineItemsData* data,
     // Symbol marker is painted as graphics. Create a ShapeResult of space
     // glyphs with the desired size to make it less special for line breaker.
     if (UNLIKELY(start_item.IsSymbolMarker())) {
-      LayoutUnit symbol_width = ListMarker::WidthOfSymbol(start_style);
+      LayoutUnit symbol_width = ListMarker::WidthOfSymbol(
+          start_style,
+          LayoutCounter::ListStyle(start_item.GetLayoutObject(), start_style));
       DCHECK_GE(symbol_width, 0);
       start_item.shape_result_ = ShapeResult::CreateForSpaces(
           &font, direction, start_item.StartOffset(), start_item.Length(),
@@ -1327,6 +1363,9 @@ void NGInlineNode::ShapeText(NGInlineItemsData* data,
       if (item.Type() == NGInlineItem::kText) {
         if (!item.Length())
           continue;
+        if (item.TextType() == NGTextType::kSymbolMarker) {
+          break;
+        }
         if (ShouldBreakShapingBeforeText(item, start_item, start_style, font,
                                          direction)) {
           break;

third_party/blink/renderer/core/paint/list_marker_painter.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/paint/list_marker_painter.h"
 
 #include "third_party/blink/renderer/core/css/counter_style.h"
+#include "third_party/blink/renderer/core/layout/layout_counter.h"
 #include "third_party/blink/renderer/core/layout/layout_list_item.h"
 #include "third_party/blink/renderer/core/layout/layout_list_marker.h"
 #include "third_party/blink/renderer/core/layout/list_marker.h"
@@ -87,8 +88,14 @@ void ListMarkerPainter::PaintSymbol(const PaintInfo& paint_info,
                                     const ComputedStyle& style,
                                     const LayoutRect& marker) {
   DCHECK(object);
-  DCHECK(style.ListStyleType());
-  DCHECK(style.ListStyleType()->IsCounterStyle());
+#if DCHECK_IS_ON()
+  if (object->IsCounter()) {
+    DCHECK(To<LayoutCounter>(object)->IsDirectionalSymbolMarker());
+  } else {
+    DCHECK(style.ListStyleType());
+    DCHECK(style.ListStyleType()->IsCounterStyle());
+  }
+#endif
   GraphicsContext& context = paint_info.context;
   Color color(object->ResolveColor(GetCSSPropertyColor()));
   if (BoxModelObjectPainter::ShouldForceWhiteBackgroundForPrintEconomy(
@@ -100,7 +107,7 @@ void ListMarkerPainter::PaintSymbol(const PaintInfo& paint_info,
   context.SetStrokeStyle(kSolidStroke);
   context.SetStrokeThickness(1.0f);
   gfx::Rect snapped_rect = ToPixelSnappedRect(marker);
-  const AtomicString& type = style.ListStyleType()->GetCounterStyleName();
+  const AtomicString& type = LayoutCounter::ListStyle(object, style);
   AutoDarkMode auto_dark_mode(
       PaintAutoDarkMode(style, DarkModeFilter::ElementRole::kListSymbol));
   if (type == "disc") {

third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

@@ -11,6 +11,7 @@
 #include "third_party/blink/renderer/core/editing/markers/text_match_marker.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/layout/geometry/logical_rect.h"
+#include "third_party/blink/renderer/core/layout/layout_counter.h"
 #include "third_party/blink/renderer/core/layout/layout_ruby_run.h"
 #include "third_party/blink/renderer/core/layout/layout_ruby_text.h"
 #include "third_party/blink/renderer/core/layout/list_marker.h"
@@ -145,8 +146,8 @@ void NGTextFragmentPainter::PaintSymbol(const LayoutObject* layout_object,
                                         const PhysicalSize box_size,
                                         const PaintInfo& paint_info,
                                         const PhysicalOffset& paint_offset) {
-  PhysicalRect marker_rect(
-      ListMarker::RelativeSymbolMarkerRect(style, box_size.width));
+  PhysicalRect marker_rect(ListMarker::RelativeSymbolMarkerRect(
+      style, LayoutCounter::ListStyle(layout_object, style), box_size.width));
   marker_rect.Move(paint_offset);
   ListMarkerPainter::PaintSymbol(paint_info, layout_object, style,
                                  marker_rect.ToLayoutRect());
@@ -281,15 +282,20 @@ void NGTextFragmentPainter::Paint(const PaintInfo& paint_info,
   }
 
   if (UNLIKELY(text_item.IsSymbolMarker())) {
-    // The NGInlineItem of marker might be Split(). To avoid calling PaintSymbol
-    // multiple times, only call it the first time. For an outside marker, this
-    // is when StartOffset is 0. But for an inside marker, the first StartOffset
-    // can be greater due to leading bidi control characters like U+202A/U+202B,
-    // U+202D/U+202E, U+2066/U+2067 or U+2068.
-    DCHECK_LT(fragment_paint_info.from, fragment_paint_info.text.length());
-    for (unsigned i = 0; i < fragment_paint_info.from; ++i) {
-      if (!Character::IsBidiControl(fragment_paint_info.text.CodepointAt(i)))
-        return;
+    if (!IsA<LayoutCounter>(layout_object)) {
+      // The NGInlineItem of marker might be Split(). To avoid calling
+      // PaintSymbol multiple times, only call it the first time. For an
+      // outside marker, this is when StartOffset is 0. But for an inside
+      // marker, the first StartOffset can be greater due to leading bidi
+      // control characters like U+202A/U+202B, U+202D/U+202E, U+2066/U+2067
+      // or U+2068.
+      DCHECK_LT(fragment_paint_info.from, fragment_paint_info.text.length());
+      for (unsigned i = 0; i < fragment_paint_info.from; ++i) {
+        if (!Character::IsBidiControl(
+                fragment_paint_info.text.CodepointAt(i))) {
+          return;
+        }
+      }
     }
     PaintSymbol(layout_object, style, physical_box.size, paint_info,
                 physical_box.offset);