Bug 1539171 - Make the list-item increment not visible from the computed style. r=mats
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 11 Apr 2019 15:21:17 +0000
changeset 469013 4ec02d2be99e933a9599075f4e45c25687744111
parent 469012 838848b7451d8247a23c73cd571dea9a837f087e
child 469014 c18ecd346791c201683180481173ea1af23c4594
push id35856
push usercsabou@mozilla.com
push dateFri, 12 Apr 2019 03:19:48 +0000
treeherdermozilla-central@940684cd1065 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1539171
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
layout/base/nsCounterManager.cpp
servo/components/style/style_adjuster.rs
testing/web-platform/tests/css/css-lists/li-counter-increment-computed-style.html
--- a/layout/base/nsCounterManager.cpp
+++ b/layout/base/nsCounterManager.cpp
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* implementation of CSS counters (for numbering things) */
 
 #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
 #include "nsContentUtils.h"
 #include "nsIContent.h"
 #include "nsTArray.h"
 #include "mozilla/dom/Text.h"
 
@@ -192,40 +193,73 @@ void nsCounterList::RecalcAll() {
             StyleDisplay::ListItem) {
       auto* shell = node->mPseudoFrame->PresShell();
       shell->FrameNeedsReflow(node->mPseudoFrame, nsIPresShell::eStyleChange,
                               NS_FRAME_IS_DIRTY);
     }
   }
 }
 
+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) {
+  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);
+  }
+  if (requiresListItemIncrement && !hasListItemIncrement) {
+    bool reversed =
+        aFrame->StyleList()->mMozListReversed == StyleMozListReversed::True;
+    nsStyleCounterData listItemIncrement{NS_LITERAL_STRING("list-item"),
+                                         reversed ? -1 : 1};
     dirty |=
-        AddCounterChangeNode(aFrame, i, styleContent->CounterIncrementAt(i),
-                             nsCounterChangeNode::INCREMENT);
+        AddCounterChangeNode(aFrame, styleContent->CounterIncrementCount() + 1,
+                             listItemIncrement, nsCounterChangeNode::INCREMENT);
   }
-  for (i = 0, i_end = styleContent->CounterSetCount(); i != i_end; ++i) {
+  for (int32_t i : IntegerRange(styleContent->CounterSetCount())) {
     dirty |= AddCounterChangeNode(aFrame, i, styleContent->CounterSetAt(i),
                                   nsCounterChangeNode::SET);
   }
   return dirty;
 }
 
 bool nsCounterManager::AddCounterChangeNode(
     nsIFrame* aFrame, int32_t aIndex, const nsStyleCounterData& aCounterData,
--- a/servo/components/style/style_adjuster.rs
+++ b/servo/components/style/style_adjuster.rs
@@ -718,54 +718,16 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
                 return;
             }
             self.style
                 .mutate_inherited_text()
                 .set_line_height(LineHeight::normal());
         }
     }
 
-    /// 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.
     ///
     /// When comparing to Gecko, this is similar to the work done by
     /// `ComputedStyle::ApplyStyleFixups`, plus some parts of
     /// `nsStyleSet::GetContext`.
     pub fn adjust<E>(&mut self, layout_parent_style: &ComputedValues, element: Option<E>)
     where
@@ -820,13 +782,12 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         }
         #[cfg(feature = "servo")]
         {
             self.adjust_for_text_decorations_in_effect();
         }
         #[cfg(feature = "gecko")]
         {
             self.adjust_for_appearance(element);
-            self.adjust_for_list_item();
         }
         self.set_bits();
     }
 }
new file mode 100644
--- /dev/null
+++ b/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>