Bug 1539171 - Make the list-item increment not visible from the computed style. r=mats

This is per https://drafts.csswg.org/css-lists/#declaring-a-list-item.

I intentionally kept <li value> defined using attribute mapping, I think that's
saner than special-casing it in layout.

Differential Revision: https://phabricator.services.mozilla.com/D24935

--HG--
extra : moz-landing-system : lando

layout/base/nsCounterManager.cpp

@@ -9,6 +9,7 @@
 #include "nsCounterManager.h"
 
 #include "mozilla/Likely.h"
+#include "mozilla/IntegerRange.h"
 #include "mozilla/PresShell.h"
 #include "mozilla/WritingModes.h"
 #include "nsBulletFrame.h"  // legacy location for list style type to text code
@@ -197,30 +198,63 @@ void nsCounterList::RecalcAll() {
   }
 }
 
+static bool HasCounters(const nsStyleContent& aStyle) {
+  return aStyle.CounterIncrementCount() || aStyle.CounterResetCount() ||
+         aStyle.CounterSetCount();
+}
+
+// For elements with 'display:list-item' we add a default
+// 'counter-increment:list-item' unless 'counter-increment' already has a value
+// for 'list-item'.
+//
+// https://drafts.csswg.org/css-lists-3/#declaring-a-list-item
+static bool GeneratesListItemIncrement(const nsIFrame* aFrame) {
+  if (aFrame->StyleDisplay()->mDisplay != StyleDisplay::ListItem) {
+    return false;
+  }
+  // FIXME(emilio): Per https://github.com/w3c/csswg-drafts/issues/3766,
+  // this condition should be removed.
+  if (aFrame->Style()->GetPseudoType() != PseudoStyleType::NotPseudo) {
+    return false;
+  }
+  return true;
+}
+
 bool nsCounterManager::AddCounterChanges(nsIFrame* aFrame) {
+  const bool requiresListItemIncrement = GeneratesListItemIncrement(aFrame);
   const nsStyleContent* styleContent = aFrame->StyleContent();
-  if (!styleContent->CounterIncrementCount() &&
-      !styleContent->CounterResetCount() && !styleContent->CounterSetCount()) {
+
+  if (!requiresListItemIncrement && !HasCounters(*styleContent)) {
     MOZ_ASSERT(!aFrame->HasAnyStateBits(NS_FRAME_HAS_CSS_COUNTER_STYLE));
     return false;
   }
 
   aFrame->AddStateBits(NS_FRAME_HAS_CSS_COUNTER_STYLE);
 
+  bool dirty = false;
   // Add in order, resets first, so all the comparisons will be optimized
   // for addition at the end of the list.
-  int32_t i, i_end;
-  bool dirty = false;
-  for (i = 0, i_end = styleContent->CounterResetCount(); i != i_end; ++i) {
+  for (int32_t i : IntegerRange(styleContent->CounterResetCount())) {
     dirty |= AddCounterChangeNode(aFrame, i, styleContent->CounterResetAt(i),
                                   nsCounterChangeNode::RESET);
   }
-  for (i = 0, i_end = styleContent->CounterIncrementCount(); i != i_end; ++i) {
-    dirty |=
-        AddCounterChangeNode(aFrame, i, styleContent->CounterIncrementAt(i),
-                             nsCounterChangeNode::INCREMENT);
+  bool hasListItemIncrement = false;
+  for (int32_t i : IntegerRange(styleContent->CounterIncrementCount())) {
+    const nsStyleCounterData& increment = styleContent->CounterIncrementAt(i);
+    hasListItemIncrement |= increment.mCounter.EqualsLiteral("list-item");
+    dirty |= AddCounterChangeNode(aFrame, i, increment,
+                                  nsCounterChangeNode::INCREMENT);
   }
-  for (i = 0, i_end = styleContent->CounterSetCount(); i != i_end; ++i) {
+  if (requiresListItemIncrement && !hasListItemIncrement) {
+    bool reversed =
+        aFrame->StyleList()->mMozListReversed == StyleMozListReversed::True;
+    nsStyleCounterData listItemIncrement{NS_LITERAL_STRING("list-item"),
+                                         reversed ? -1 : 1};
+    dirty |=
+        AddCounterChangeNode(aFrame, styleContent->CounterIncrementCount() + 1,
+                             listItemIncrement, nsCounterChangeNode::INCREMENT);
+  }
+  for (int32_t i : IntegerRange(styleContent->CounterSetCount())) {
     dirty |= AddCounterChangeNode(aFrame, i, styleContent->CounterSetAt(i),
                                   nsCounterChangeNode::SET);
   }

servo/components/style/style_adjuster.rs

@@ -723,44 +723,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         }
     }
 
-    /// For HTML elements with 'display:list-item' we add a default 'counter-increment:list-item'
-    /// unless 'counter-increment' already has a value for 'list-item'.
-    ///
-    /// https://drafts.csswg.org/css-lists-3/#declaring-a-list-item
-    #[cfg(feature = "gecko")]
-    fn adjust_for_list_item(&mut self) {
-        use crate::properties::longhands::counter_increment::computed_value::T as ComputedIncrement;
-        use crate::values::generics::counters::CounterPair;
-        use crate::values::specified::list::MozListReversed;
-        use crate::values::CustomIdent;
-
-        if self.style.get_box().clone_display() != Display::ListItem {
-            return;
-        }
-        if self.style.pseudo.is_some() {
-            return;
-        }
-
-        let increments = self.style.get_counters().clone_counter_increment();
-        if increments.iter().any(|i| i.name.0 == atom!("list-item")) {
-            return;
-        }
-
-        let reversed = self.style.get_list().clone__moz_list_reversed() == MozListReversed::True;
-        let increment = if reversed { -1 } else { 1 };
-        let list_increment = CounterPair {
-            name: CustomIdent(atom!("list-item")),
-            value: increment,
-        };
-        let increments = increments
-            .iter()
-            .cloned()
-            .chain(std::iter::once(list_increment));
-        self.style
-            .mutate_counters()
-            .set_counter_increment(ComputedIncrement::new(increments.collect()));
-    }
-
     /// Adjusts the style to account for various fixups that don't fit naturally
     /// into the cascade.
     ///
@@ -825,7 +787,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         #[cfg(feature = "gecko")]
         {
             self.adjust_for_appearance(element);
-            self.adjust_for_list_item();
         }
         self.set_bits();
     }

testing/web-platform/tests/css/css-lists/li-counter-increment-computed-style.html

@@ -0,0 +1,20 @@
+<!doctype html>
+<title>Magic list-item counter-increment shouldn't be visible from computed style</title>
+<link rel="help" href="https://drafts.csswg.org/css-lists/#declaring-a-list-item">
+<link rel="help" href="https://drafts.csswg.org/css-lists/#list-item-counter">
+<link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">
+<link rel="author" href="https://mozilla.org" title="Mozilla">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<li data-expected="none">No explicit counter.
+<li><span style="counter-increment:inherit" data-expected="none">Inherited
+<li value="10" data-expected="none">Value attribute.
+<li style="counter-increment: list-item 10" data-expected="list-item 10">Explicit list-item counter.
+<li style="counter-increment: list-item 1" data-expected="list-item 1">Explicit and redundant list-item counter.
+<li style="counter-increment: foo 10" data-expected="foo 10">Other counter.
+<script>
+test(function() {
+  for (const element of document.querySelectorAll("[data-expected]"))
+    assert_equals(getComputedStyle(element).counterIncrement, element.getAttribute("data-expected"), element.innerText);
+}, "list-item counter-increment shouldn't be visible from computed style");
+</script>