[css-pseudo] Let 'content' ::markers be forced to be inside

Even if a list item has 'list-style-position: outside', Chromium may
force its marker to be inside when it's a <li> outside a list element.

This information was stored in layout, but this created an undesirable
dependency since the computed 'display' value of the marker depends on
whether it's inside or outside. And it wasn't possible to force a marker
with non-normal 'content' to be inside.

This patch fixes it by adding an internal 'IsInsideListElement' flag in
ComputedStyle. This design also avoids traversing all the ancestors of
every <li> element.

BUG=457718

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

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

Change-Id: I2df70841e8e9d729fd4aa47ec9b9973c09a82dcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1973914Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#726310}

third_party/blink/renderer/core/css/resolver/style_adjuster.cc

@@ -198,7 +198,8 @@ static void AdjustStyleForFirstLetter(ComputedStyle& style) {
 }
 
 static void AdjustStyleForMarker(ComputedStyle& style,
-                                 const ComputedStyle& parent_style) {
+                                 const ComputedStyle& parent_style,
+                                 Element* element) {
   if (style.StyleType() != kPseudoIdMarker)
     return;
 
@@ -206,8 +207,13 @@ static void AdjustStyleForMarker(ComputedStyle& style,
   if (!style.GetContentData())
     return;
 
+  bool is_inside =
+      parent_style.ListStylePosition() == EListStylePosition::kInside ||
+      (element && IsA<HTMLLIElement>(element->parentNode()) &&
+       !parent_style.IsInsideListElement());
+
   // Outside list markers should generate a block container.
-  if (parent_style.ListStylePosition() == EListStylePosition::kOutside) {
+  if (!is_inside) {
     DCHECK_EQ(style.Display(), EDisplay::kInline);
     style.SetDisplay(EDisplay::kInlineBlock);
   }
@@ -320,6 +326,11 @@ static void AdjustStyleForHTMLElement(ComputedStyle& style,
     return;
   }
 
+  if (IsA<HTMLUListElement>(element) || IsA<HTMLOListElement>(element)) {
+    style.SetIsInsideListElement();
+    return;
+  }
+
   if (style.Display() == EDisplay::kContents) {
     // See https://drafts.csswg.org/css-display/#unbox-html
     // Some of these elements are handled with other adjustments above.
@@ -624,7 +635,7 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
     // We don't adjust the first letter style earlier because we may change the
     // display setting in adjustStyeForTagName() above.
     AdjustStyleForFirstLetter(style);
-    AdjustStyleForMarker(style, parent_style);
+    AdjustStyleForMarker(style, parent_style, element);
 
     AdjustStyleForDisplay(style, layout_parent_style,
                           element ? &element->GetDocument() : nullptr);

third_party/blink/renderer/core/html/html_li_element.cc

@@ -92,24 +92,6 @@ void HTMLLIElement::AttachLayoutTree(AttachContext& context) {
 
   if (ListItemOrdinal* ordinal = ListItemOrdinal::Get(*this)) {
     DCHECK(!GetDocument().ChildNeedsDistributionRecalc());
-
-    // Find the enclosing list node.
-    Element* list_node = nullptr;
-    Element* current = this;
-    while (!list_node) {
-      current = LayoutTreeBuilderTraversal::ParentElement(*current);
-      if (!current)
-        break;
-      if (IsA<HTMLUListElement>(*current) || IsA<HTMLOListElement>(*current))
-        list_node = current;
-    }
-
-    // If we are not in a list, tell the layoutObject so it can position us
-    // inside.  We don't want to change our style to say "inside" since that
-    // would affect nested nodes.
-    if (!list_node)
-      ordinal->SetNotInList(true, *this);
-
     ParseValue(FastGetAttribute(html_names::kValueAttr), ordinal);
   }
 }

third_party/blink/renderer/core/html/list_item_ordinal.cc

@@ -35,8 +35,7 @@
 
 namespace blink {
 
-ListItemOrdinal::ListItemOrdinal()
-    : type_(kNeedsUpdate), not_in_list_(false), not_in_list_changed_(false) {}
+ListItemOrdinal::ListItemOrdinal() : type_(kNeedsUpdate) {}
 
 bool ListItemOrdinal::IsList(const Node& node) {
   return IsA<HTMLUListElement>(node) || IsA<HTMLOListElement>(node);
@@ -242,21 +241,6 @@ void ListItemOrdinal::ClearExplicitValue(const Node& item_node) {
   InvalidateAfter(EnclosingList(&item_node), &item_node);
 }
 
-void ListItemOrdinal::SetNotInList(bool not_in_list, const Node& item_node) {
-  if (not_in_list_ == not_in_list)
-    return;
-
-  not_in_list_ = not_in_list;
-  SetNotInListChanged(true);
-  LayoutObject* layout_object = item_node.GetLayoutObject();
-  if (layout_object->IsLayoutNGListItem())
-    layout_object->NotifyOfSubtreeChange();
-}
-
-void ListItemOrdinal::SetNotInListChanged(bool changed) {
-  not_in_list_changed_ = changed;
-}
-
 unsigned ListItemOrdinal::ItemCountForOrderedList(
     const HTMLOListElement* list_node) {
   DCHECK(list_node);

third_party/blink/renderer/core/html/list_item_ordinal.h

@@ -61,12 +61,6 @@ class CORE_EXPORT ListItemOrdinal {
   void SetExplicitValue(int, const Node&);
   void ClearExplicitValue(const Node&);
 
-  // Get/set whether this item is in a list or not.
-  bool NotInList() const { return not_in_list_; }
-  void SetNotInList(bool, const Node&);
-  bool NotInListChanged() const { return not_in_list_changed_; }
-  void SetNotInListChanged(bool);
-
   static bool IsList(const Node&);
   static bool IsListItem(const Node&);
   static bool IsListItem(const LayoutObject*);
@@ -113,8 +107,6 @@ class CORE_EXPORT ListItemOrdinal {
 
   mutable int value_ = 0;
   mutable unsigned type_ : 2;  // ValueType
-  unsigned not_in_list_ : 1;
-  unsigned not_in_list_changed_ : 1;
 };
 
 }  // namespace blink

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

@@ -459,8 +459,10 @@ LayoutListMarker::ListStyleCategory LayoutListMarker::GetListStyleCategory(
 }
 
 bool LayoutListMarker::IsInside() const {
-  return list_item_->Ordinal().NotInList() ||
-         StyleRef().ListStylePosition() == EListStylePosition::kInside;
+  const ComputedStyle& parent_style = list_item_->StyleRef();
+  return parent_style.ListStylePosition() == EListStylePosition::kInside ||
+         (IsA<HTMLLIElement>(list_item_->GetNode()) &&
+          !parent_style.IsInsideListElement());
 }
 
 LayoutRect LayoutListMarker::GetRelativeMarkerRect() const {

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

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/layout/layout_object_factory.h"
 
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/layout/layout_block_flow.h"
 #include "third_party/blink/renderer/core/layout/layout_fieldset.h"
 #include "third_party/blink/renderer/core/layout/layout_flexible_box.h"
@@ -99,9 +100,12 @@ LayoutObject* LayoutObjectFactory::CreateListMarker(Node& node,
   if (!RuntimeEnabledFeatures::LayoutNGEnabled() ||
       legacy == LegacyLayout::kForce)
     return nullptr;
-  // TODO(obrufau): markers may be forced to be inside despite having
-  // `list-style-position: outside`.
-  if (style.ListStylePosition() == EListStylePosition::kInside) {
+  const Node* parent = node.parentNode();
+  const ComputedStyle* parent_style = parent->GetComputedStyle();
+  bool is_inside =
+      parent_style->ListStylePosition() == EListStylePosition::kInside ||
+      (IsA<HTMLLIElement>(parent) && !parent_style->IsInsideListElement());
+  if (is_inside) {
     return CreateObject<LayoutObject, LayoutNGInsideListMarker,
                         LayoutNGInsideListMarker>(node, style, legacy);
   }

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

@@ -90,12 +90,6 @@ void LayoutNGListItem::SubtreeDidChange() {
   if (!marker_)
     return;
 
-  if (ordinal_.NotInListChanged()) {
-    UpdateMarker();
-    ordinal_.SetNotInListChanged(false);
-    return;
-  }
-
   // Make sure outside marker is the direct child of ListItem.
   if (!IsInside() && marker_->Parent() != this) {
     marker_->Remove();
@@ -112,8 +106,8 @@ void LayoutNGListItem::WillCollectInlines() {
 // Returns true if this is 'list-style-position: inside', or should be laid out
 // as 'inside'.
 bool LayoutNGListItem::IsInside() const {
-  return ordinal_.NotInList() ||
-         StyleRef().ListStylePosition() == EListStylePosition::kInside;
+  return StyleRef().ListStylePosition() == EListStylePosition::kInside ||
+         (IsA<HTMLLIElement>(GetNode()) && !StyleRef().IsInsideListElement());
 }
 
 // Destroy the list marker objects if exists.

third_party/blink/renderer/core/style/computed_style_extra_fields.json5

@@ -1036,5 +1036,12 @@
       custom_compare: true,
       inherited: true,
     },
+    {
+      name: "IsInsideListElement",
+      field_template: "monotonic_flag",
+      default_value: "false",
+      custom_compare: true,
+      inherited: true,
+    },
   ],
 }

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

@@ -76,6 +76,7 @@ crbug.com/457718 external/wpt/css/css-pseudo/marker-content-003b.html [ Failure
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-004.html [ Failure ]
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-006.html [ Failure ]
 crbug.com/457718 external/wpt/css/css-pseudo/marker-content-012.html [ Failure ]
+crbug.com/457718 external/wpt/css/css-pseudo/marker-content-015.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-015-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>
+li {
+  margin-left: 100px;
+}
+.outside {
+  list-style-position: outside;
+}
+.inside {
+  list-style-position: inside;
+}
+.decimal {
+  list-style-type: decimal;
+}
+.string {
+  list-style-type: "[marker]";
+}
+</style>
+<!-- Note: Chromium and WebKit force all these markers to be inside -->
+<li class="outside decimal">outside</li>
+<li class="outside string">outside</li>
+<li class="inside decimal">inside</li>
+<li class="inside string">inside</li>

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

+<!DOCTYPE html>
+<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-015-ref.html">
+<link rel="help" href="https://drafts.csswg.org/css-pseudo-4/#marker-pseudo">
+<meta name="assert" content="Checks that the position of a ::marker originated by a <li> which is not in a list is not affected by the 'content' property.">
+<style>
+li {
+  margin-left: 100px;
+}
+.outside {
+  list-style-position: outside;
+}
+.inside {
+  list-style-position: inside;
+}
+.decimal {
+  list-style-type: decimal;
+}
+.marker::marker {
+  content: "[marker]";
+}
+</style>
+<!-- Note: Chromium and WebKit force all these markers to be inside -->
+<li class="outside decimal">outside</li>
+<li class="outside marker">outside</li>
+<li class="inside decimal">inside</li>
+<li class="inside marker">inside</li>