288704 - [css-lists] Implement list numbering using a built-in 'list-item' counter

Status: RESOLVED FIXED

(Core :: Layout: Generated Content, Lists, and Counters, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Now that we implement counters (bug 3247), using them for list numbering would
fix a significant number of list numbering bugs that we have.

A patch containing some of the necessary code removal is in bug 3247 comment 57
(removed from all later patches).

The code that would need to be added would probably involve implementing
considerable parts of css3-content's proposals for list handling, using -moz-
prefixes.  Note that we probably don't want to use counters for
disc/square/circle/none types since we probably want to keep the customized
drawing code that we use for them.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

This is the old patch merged to the trunk, plus a few tweaks.

Comment 2

[Anne (:annevk)]

Why isn't it called mozMarker internally? Similar to other Mozilla
implementations of CSS3 proposals/extensions?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> Why isn't it called mozMarker internally? Similar to other Mozilla
> implementations of CSS3 proposals/extensions?

Because that just creates more work when we want to drop the -moz- (for things
that are in a spec).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #0)
> Note that we probably don't want to use counters for
> disc/square/circle/none types since we probably want to keep the customized
> drawing code that we use for them.

Well, we do need to use counters so that 'list-style-type' on an individual item
still works.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

Nothing significant here, just some comments reflecting current thoughts.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

For a second I was thinking this would be easy, and then I ran into problems
with the spec; see comments.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I realized that this bug doesn't require implementing ::marker, and given the
dependency on bug 309217 if I want to do it using ::marker, there's probably
good reason to do it without.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, FWIW, I wasn't thinking straight about the spec problems in those comments;
it's actually fine.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

Just a little more work that I had lying around in a tree.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, that last patch is smaller -- I think it had the stuff that I thought was step 2 taken out, and maybe a little other work added.

Comment 11

[Gordon P. Hemsley [:GPHemsley]]

Is there any chance of this (and related counter stuff) being resurrected any time soon?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think it's a priority for me right now, though I'd be willing to help mentor someone.  It involves a good bit of discussion with the CSS working group, figuring out the current state of spec proposals/drafts, etc.

Comment 13

[Alex]

Hi I'm new to the Mozilla developers network as well as OSS contribution and want to work on this bug. Any chance I could get some mentored help and a basic how to start?

Thanks!

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You probably don't want to try working on this bug until you've successfully worked on a number of other patches first, since this is relatively involved.

Comment 15

[Ilya]

::marker pseudo-element returned to the CSS Pseudo Elements module level 4 spec: https://www.w3.org/TR/css-pseudo-4/#marker-pseudo. Would it help fixing this bug now?

It's a pity that we still have the 17-year-old https://bugzilla.mozilla.org/show_bug.cgi?id=4522 blocked by this. Other browsers have no problems with numeric list-style values for any element.

Comment 16

[Mats Palmgren (inactive)]

I have some patches that implements this...

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-lists] Add an inherited internal UA sheet property (-moz-list-reversed:true|false) to propagate <ol reversed> to its relevant decendants.

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter.

While I'm here, I'm removing the support for <ul start> which isn't
supported by Chrome, nor is it in the HTML spec. There's an open
issue about <ul> attributes here: https://github.com/whatwg/html/issues/3979
but until that is resolved I think compat with Chrome is better.

I'm disabling the WPT css/css-lists/inheritance.html for now, since
it now triggers bug 1405176. I'm guessing because of
the list-style-image: url("https://example.com/"). It seems to me
this is an existing issue that's just triggered by coincidence.

The change to WPT css/CSS2/lists/counter-reset-increment-002.xht is
simply a bug in the test (both <li> and <li::before> increments
the counter). Removing it on ::before makes the test pass in
both Chrome and Firefox and I think it's in line with the intention
of the test, which is to test incrementing from a negative value to
a positive).

Comment 19

[Mats Palmgren (inactive)]

Attachment: part 3 - Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation.

Comment 20

[Mats Palmgren (inactive)]

... and as usual there were some HTML tags suppressed in those comments.
Please read the raw text if you're interested :-)
(I really f*cking hate this newfangled Markdown-by-default thing.)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9035117 [details] [diff] [review]
part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter.

Review of attachment 9035117 [details] [diff] [review]:

::: servo/components/style/style_adjuster.rs
@@ +752,5 @@

•    // Map <li value=INTEGER> to 'counter-set: list-item INTEGER'.
  

•    // https://drafts.csswg.org/css-lists-3/#ua-stylesheet
  

•    let e = element.as_ref().unwrap();
  

•    if e.local_name() == &*local_name!("li") && e.has_attr(&ns!(), &atom!("value")) {
  

hmm, so this makes the counter-set property depend on display, and the style of two elements depend on whether the value attribute is present...

I wonder if this violates some assumptions the style sharing code may be making...

::: testing/web-platform/meta/css/css-lists/inheritance.html.ini
@@ +1,2 @@

+[inheritance.html]

• disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1405176

Looks like this test is just testing syntax. Can we replace https://example.com by a local URI instead of disabling the test?

Comment 22

[Mats Palmgren (inactive)]

hmm, so this makes the counter-set property depend on display

Yes, this is what the spec says:
https://drafts.csswg.org/css-lists-3/#declaring-a-list-item

A list item is any element with its display property set to list-item.
[...] list items automatically increment the special list-item counter. Unless the counter-increment property manually specifies a different increment for the list-item counter, it must be incremented by 1 on every list item...

Can we replace https://example.com by a local URI instead of disabling the test?

Good idea, I'll try that...

Comment 23

[Mats Palmgren (inactive)]

Changing "https://example.com" to use http: instead gives the same error.
Changing it to url("#foo") makes the test fail:
assert_equals: expected "url("#foo")" but got "url("http://web-platform.test:8000/css/css-lists/inheritance.html#foo\")"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10dbf733e21aff72cc9aced98ae593426c88c3c4&selectedJob=220693190

Interestingly, even after disabling the test completely the same assertion occurs,
but somewhere else:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d31e7f89b92ac249da0145b251eb0925d700423&selectedJob=220623487

I'm not really sure what to make of this... perhaps this assertion is
a permafail on Try at the moment?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9035115 [details] [diff] [review]
part 1 - [css-lists] Add an inherited internal UA sheet property (-moz-list-reversed:true|false) to propagate <ol reversed> to its relevant decendants.

Review of attachment 9035115 [details] [diff] [review]:
-----------------------------------------------------------------

Could you add this property to layout/style/test/test_non_content_accessible_properties.html?

How does this interact with display: contents?

If I have:

<ol reversed>
  <ol style="display: contents">
    <li>First
    <li>Second
    <li>Third
  </ol>
</ol>

This would change behavior in that case, right? Maybe worth a spec clarification? I'm not quite sure what's supposed to happen here (though it's enough of an edge case I don't think anybody would rely on it).

::: servo/components/style/values/specified/list.rs
@@ +124,5 @@
>      }
>  }
> +
> +/// Specified and computed `-moz-list-reversed` property (for UA sheets only).
> +#[cfg(feature = "gecko")]

No need for the #[cfg] stuff. It's fine to compile it on Servo as long as it's not used.

@@ +134,5 @@
> +    /// exclusively used for <ol reversed> in our html.css UA sheet
> +    True,
> +}
> +
> +impl From<bool> for MozListReversed {

If you don't mind much, I'd rather add MozListReversed to:

  https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/servo/components/style/cbindgen.toml#41

and:

  https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/layout/style/ServoBindings.toml#386

(changing `bool mMozListReversed;` to `mozilla::StyleMozListReversed mMozListReversed;`)

But it's not a big deal I guess.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9035117 [details] [diff] [review]
part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter.

Review of attachment 9035117 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I took such a long time to take a look at this :(

I think the <li value> part, which is what concerned me, should be removed instead, using the general mapped attributes mechanism.

::: servo/components/style/style_adjuster.rs
@@ +750,5 @@
> +            return;
> +        }
> +
> +        // Map <li value=INTEGER> to 'counter-set: list-item INTEGER'.
> +        // https://drafts.csswg.org/css-lists-3/#ua-stylesheet

So this doesn't match the spec, and it seems to me matching the spec is much easier. We should do this independently of the display value, using the mapped attributes stuff.

That would make it a normal rule in the presentation hints level, and this block of code and get_li_value_attribute wouldn't be needed. Plus you could override counter-set: none too :)

Let me know if I missed something or I can help fixing this.

@@ +757,5 @@
> +            if self.style.get_counters().clone_counter_set().is_empty() {
> +                self.style
> +                    .mutate_counters()
> +                    .set_counter_set(ComputedSet::new(vec![CounterPair {
> +                        name : CustomIdent(Atom::from("list-item")),

nit: This should use the `atom!()` macro.

@@ +764,5 @@
> +            }
> +            return; // 'counter-increment:none' by default in this case
> +        }
> +
> +        if !self.style.get_counters().clone_counter_increment().is_empty() {

So the intention of this is to let the author override this, right? But this wouldn't let authors set counter-increment: none, which is unfortunate...

@@ +771,5 @@
> +
> +        // TODO(mats): <summary> has display:list-item in our UA sheet which we
> +        // need to workaround here.
> +        // (e.g breakage: layout/reftests/details-summary/details-in-ol.html)
> +        if e.local_name() == &*local_name!("summary") {

I'd remove this hack if possible, even if it means changing the rendering of that test...

::: servo/ports/geckolib/glue.rs
@@ +4463,5 @@
> +    use style::values::generics::counters::{CounterPair, CounterSetOrReset};
> +    use style::properties::{PropertyDeclaration};
> +
> +    let prop = PropertyDeclaration::CounterReset(CounterSetOrReset::new(vec![CounterPair {
> +        name: CustomIdent(Atom::from("list-item")),

atom!

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9035118 [details] [diff] [review]
part 3 - Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation.

Review of attachment 9035118 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great assuming we're fine with the behavior change I pointed out for the first patch, and the second patch is fixed-up, thanks for working on this!

::: layout/generic/nsBulletFrame.cpp
@@ +812,5 @@
> +  auto* fc = PresShell()->FrameConstructor();
> +  auto* cm = fc->CounterManager();
> +  auto* list = cm->CounterListFor(NS_LITERAL_STRING("list-item"));
> +  MOZ_ASSERT(list && !list->IsDirty());
> +  nsIFrame* listItem = GetParent()->GetContent()->GetPrimaryFrame();

I wonder if there's any inside-bullet + first-line + span case or such this wouldn't handle...

Comment 27

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

•    if !self.style.get_counters().clone_counter_increment().is_empty() {
  

So the intention of this is to let the author override this, right? But this
wouldn't let authors set counter-increment: none, which is unfortunate...

Yeah, ideally we should change the initial value for all the counter-*
properties so that we can easily detect an author-specified 'none' value
in this code, e.g. 'auto'. The root of the problem is that the whole
spec rests on the foundation that only 'display:list-item' element's are
"list items", which we can't express in a selector in the UA sheet.
https://drafts.csswg.org/css-lists-3/#declaring-a-list-item

Since you're at the F2F, could you check if the spec people would be OK
with that? (something like: 'auto' computes to 'none' when 'display'
isn't 'list-item', otherwise computes to the relevant 'list-item'
counter value for the respective property)

Comment 28

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

I think the <li value> part, which is what concerned me, should be removed
instead, using the general mapped attributes mechanism.

Mapping <li value=N> to "counter-set: list-item N; counter-increment: none"
means that adjust_for_list_item() will think that counter-increment isn't
set and add thus the default value (assuming it has display:list-item).
It would be implementable if the initial value for counter-increment
is changed to auto though.

I think I'll keep it as is for now... we can adjust it later if
the CSSWG agrees to change the initial value.

nit: This should use the atom!() macro.

 0:02.14 761 |                         name : CustomIdent(atom!("list-item")),
 0:02.14     |                                                  ^^^^^^^^^^^ no rules expected this token in macro call

¯_(ツ)_/¯

Comment 29

[Mats Palmgren (inactive)]

Attachment: Part 1b

I'll fold this into part 1, but it might be easier to review as
a standalone patch on top of that.

How does this interact with display: contents?

I think display:contents shouldn't matter, and that
we handle your example incorrectly currently (3-2-1).
These patches makes us compatible with Chrome (1-2-3).

(changing bool mMozListReversed; to mozilla::StyleMozListReversed mMozListReversed;)

I guess I could've used different values than True/False,
but it seems worth fixing this in any case to avoid name
collisions in the future.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9047090 [details] [diff] [review]
Part 1b

Review of attachment 9047090 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/X11UndefineNone.h
@@ +26,5 @@
>  #ifdef Always
>  #  undef Always
>  #endif
> +
> +// X11/Xlib.h also defines True and False, get rid of those too for

Sigh :(

Comment 31

[Emilio Cobos Álvarez (:emilio)]

I got locked out of Bugzilla this morning, so just some conversation that happened over mail then irc:

I mailed:

I got locked out of bugzilla due to too many requests looks like, so I'll comment when I get a different ip, but in case it's helpful before that:

I talked a bit with fantasai yesterday about it, but still didn't get to any conclusion about whether to change the default value or not, can try again today I guess.

Mapping <li value=N> to "counter-set: list-item N; counter-increment:
none"
means that adjust_for_list_item() will think that counter-increment isn't
set and add thus the default value (assuming it has display:list item).
It would be implementable if the initial value for counter-increment
is changed to auto though.

I see... Unfortunately adding style adjustments that depend on attribute values is a no-go and will cause incorrect style sharing, so it's not sound and we'd need to think harder about it.

^^^^^^^^^^^ no rules expected this token in macro call

You need to add it to StaticAtoms.py, the same way you'd add them if you wanted to use nsGkAtoms::list_item or such.

Mats replied:

I see... Unfortunately adding style adjustments that depend on attribute
values is a no-go and will cause incorrect style sharing, so it's not
sound and we'd need to think harder about it.

But the patch does add 'value' to IsAttributeMapped, even though it
also does a style adjustment with the value later... does that still
cause a style sharing problem?

Anyway, I think we have a strong case for changing the initial value.
Let me know if I should file a github issue about it...

You need to add it to StaticAtoms.py, the same way you'd add them if you
wanted to use nsGkAtoms::list_item or such.

Thanks, I'll try that.

/Mats

Then on IRC:

08:39 <@emilio> mats: sent a mail about list items since I cannot comment on bugzilla right now (will probably be able in about half an hour)
08:39 <@emilio> mats: would applying counter-set after counter-increment be an acceptable solution?
08:40 <mats> emilio: thx, I just replied
08:40 <@emilio> mats: then you just need to map <li value> to counter-set
08:42 <@emilio> mats: yeah, even with value in IsAttributeMapped it is still a problem, I think. Style sharing only cares about declarations and selectors. Mapped attributes usually get handled via having a different declaration
08:43 <@emilio> mats: but I can apply the patches and try to come up with a testcase
08:44 <@emilio> mats: we could fix by never inserting <li> elements with value attributes in the style sharing cache, but that's not great...
08:46 <mats> emilio: changing the way we apply counter-set/increment would break normal CSS counters though, right?
08:46 <mats> emilio: what we really need is a different initial value ;-)
08:49 <@emilio> mats: well we haven't shipped counter-set yet, but yes, if done for counter-reset it'd be a behavior change. And yeah, I agree. I can try to talk to tab or fantasai about it for a few minutes, but if you can file the issue that'd be great.
08:50 <mats> emilio: ok, I'll file one
08:51 <@emilio> ty!
08:52 <@emilio> mats: also, yesterday we happened to talk about how broken all list implementations are, and how you were fixing them in Gecko ;)

Comment 32

[Mats Palmgren (inactive)]

Attachment: part 2b

I'll fold this into part 2.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

•    // Map <li value=INTEGER> to 'counter-set: list-item INTEGER'.
  

•    // https://drafts.csswg.org/css-lists-3/#ua-stylesheet
  

So this doesn't match the spec, and it seems to me matching the spec is much
easier. We should do this independently of the display value, using the
mapped attributes stuff.

Yeah, but as we discussed on IRC, I don' think it's possible
to implement it any better ATM. We really need a different
initial value for the counter-* properties for this.
I'll file a github issue about it and hope the CSSWG agrees...

nit: This should use the atom!() macro.

fixed

•    // need to workaround here.
  

•    // (e.g breakage: layout/reftests/details-summary/details-in-ol.html)
  

•    if e.local_name() == &*local_name!("summary") {
  

I'd remove this hack if possible, even if it means changing the rendering of
that test...

Fair enough, I added "counter-increment: list-item 0" to the UA sheet
instead, which is what the HTML spec suggests:
https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
(and yeah, that UA rule kind of sucks too, I know)

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9047148 [details] [diff] [review]
part 2b

Review of attachment 9047148 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, though I think we still need to change the value attribute to use mapped attributes instead of style_adjuster.rs.

Comment 34

[Mats Palmgren (inactive)]

Attachment: part 2c

addendum for part 2, I'll fold it into that part.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #33)

This looks fine, though I think we still need to change the value attribute
to use mapped attributes instead of style_adjuster.rs.

OK, but we need a different initial value for 'counter-increment' for
that to work properly. This patch implements alternative (A) in
https://github.com/w3c/csswg-drafts/issues/3686#issuecomment-468098431
which seems to be the solution the CSSWG favors.
That is, keep 'none' as the initial value, but don't add the default
'counter-increment' for list items if the property is already set
and includes a value for 'list-item'. This patch makes <li value=N>
map to 'counter-set: list-item N; counter-increment: list-item 0;'
so it inhibits the default increment.

(sorry for abusing the feedback flag as a review flag;
I haven't figured out this archanist stuff yet...)

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9047770 [details] [diff] [review]
part 2c

Review of attachment 9047770 [details] [diff] [review]:
-----------------------------------------------------------------

I see, so the advantage of doing proposal (C) in that issue is that you don't need to explicitly map the value attribute to `counter-increment: list-item 0`. But I think this is simpler if web-compatible, so this looks good.

I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get a resolution on that issue. If you want to comment the details of the solution you ended up using (in particular the important detail of mapping <li value> to two declarations) that's great, otherwise let me know and I'll do that.

Thanks! r=me with the nits addressed.

::: dom/html/HTMLLIElement.cpp
@@ +70,5 @@
> +  // Map <li value=INTEGER> to 'counter-set: list-item INTEGER;
> +  // counter-increment: list-item 0;'.
> +  const nsAttrValue* attrVal = aAttributes->GetAttr(nsGkAtoms::value);
> +  if (attrVal && attrVal->Type() == nsAttrValue::eInteger) {
> +    if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) {

I think you could just assert that they're not set, but we should do that all over the place basically, so this is fine for now.

::: servo/components/style/style_adjuster.rs
@@ +743,5 @@
>              return;
>          }
> +        // Note that we map <li value=INTEGER> to 'counter-set: list-item INTEGER;
> +        // counter-increment: list-item 0;' so we'll return here unless the author
> +        // explicitly specified something else.

Maybe point to the CSSWG issue?

@@ +744,5 @@
>          }
> +        // Note that we map <li value=INTEGER> to 'counter-set: list-item INTEGER;
> +        // counter-increment: list-item 0;' so we'll return here unless the author
> +        // explicitly specified something else.
> +        use crate::values::CustomIdent;

nit: Maybe move all the `use` statements to the of the function? We don't have many mid-function, but not a big deal.

@@ +747,5 @@
> +        // explicitly specified something else.
> +        use crate::values::CustomIdent;
> +        use crate::values::generics::counters::{CounterPair};
> +        let mut counter_increments: Vec<CounterPair<i32>> = vec![];
> +        for i in self.style.get_counters().clone_counter_increment().iter() {

This reallocates a lot unnecessarily.

We could avoid the unnecessary allocation when not mutating the counter with something like a counter_increment_references_list_item() directly in gecko.mako.rs or something, but that's probably not too important since that's not the common case.

A trivial improvement that avoids a copy and would also make it slightly easier to follow IMO would be:

```
let mut increments = self.style.get_counters().clone_counter_increment();
if increments.iter().any(|i| i.name.as_atom() == atom!("list-item")) {
    return;
}

let reversed = ..;
increments.0.push(..);
self.style.mutate_counters().set_counter_increment(increments);
```

@@ +759,5 @@
>          use crate::values::specified::list::MozListReversed;
>          let reversed = self.style.get_list().clone__moz_list_reversed() == MozListReversed::True;
>          let increment = if reversed { -1 } else { 1 };
> +        counter_increments.push(CounterPair {
> +            name : CustomIdent(atom!("list-item")),

nit: no space to the right of `name`.

Comment 36

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #35)

I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get
a resolution on that issue. If you want to comment the details of the
solution you ended up using (in particular the important detail of mapping
<li value> to two declarations) that's great, otherwise let me know and I'll
do that.

I amended my last comment with that detail.

• if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) {

I think you could just assert that they're not set, but we should do that
all over the place basically, so this is fine for now.

Yeah, I noticed this too, but figured I should follow the established
pattern. But you're right, it's probably better to assert in general.
I'll file that as a follow-up bug.

•    use crate::values::CustomIdent;
  

nit: Maybe move all the use statements to the of the function? We don't
have many mid-function, but not a big deal.

OK, idk what the preferred style is for these, so I followed a "closest
to first use" rule of thumb.

A trivial improvement that avoids a copy and would also make it slightly
easier to follow IMO would be:

Sounds good. Fwiw, the part that I couldn't figure out how to do was:

increments.0.push(..);

I didn't realize I could access the internal vector like that...

Comment 37

[Mats Palmgren (inactive)]

 1:12.84 error[E0599]: no method named `as_atom` found for type `values::CustomIdent` in the current scope` 
 1:12.84    --> servo/components/style/style_adjuster.rs:754:45
 1:12.84     |
 1:12.84 754 |         if increments.iter().any(|i| i.name.as_atom() == atom!("list-item")) {
 1:12.84     |                                             ^^^^^^^
 1:12.84     | 
 1:12.84    ::: servo/components/style/values/mod.rs:153:1
 1:12.84     |
 1:12.84 153 | pub struct CustomIdent(pub Atom);
 1:12.84     | --------------------------------- method `as_atom` not found for this
 1:12.84 error[E0616]: field `0` of struct `values::generics::counters::CounterIncrement` is private
 1:12.84    --> servo/components/style/style_adjuster.rs:760:9
 1:12.84     |
 1:12.84 760 |         increments.0.push(CounterPair {
 1:12.84     |         ^^^^^^^^^^^^
 1:12.85 error[E0599]: no method named `push` found for type `values::generics::counters::Counters<i32>` in the current scope
 1:12.85    --> servo/components/style/style_adjuster.rs:760:22
 1:12.85     |
 1:12.85 760 |         increments.0.push(CounterPair {
 1:12.85     |                      ^^^^

It seems I can use i.name.0 to fix the first one (yay, I learned something :) ).
Not sure what to do about the second one though - maybe just make it public somehow?

Comment 38

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #37)

Not sure what to do about the second one though - maybe just make it public somehow?

Yeah, you can make it public with pub Box<..> in:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/servo/components/style/values/generics/counters.rs#75

But I misremembered how our counters stuff is stored, thought it was unconditionally a vector. It's a boxed slice, which is more compact, but not growable.

Given push will reallocate unconditionally, we could do something like:

let increments = self.style.get_counters().clone_counter_increment();
if increments.iter().any(|i| i.name.0 == atom!("list-item")) {
    return;
}
let list_increment = CounterPair {
    ...
};
let increments = increments.iter().cloned().chain(std::iter::once(list_increment));
set_counter_increment(CounterIncrement::new(increments.collect());

Or such. Sorry for the hassle.

Comment 39

[Mats Palmgren (inactive)]

No worries, always happy to pick up new Rust skills. :-)

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84c996219204
part 1 - [css-lists] Add an inherited internal UA sheet property (-moz-list-reversed:true|false) to propagate <ol reversed> to its relevant decendants.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4e4daebdc4
part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2e7cfa5484
part 3 - Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation.  r=emilio

Comment 41

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/84c996219204
https://hg.mozilla.org/mozilla-central/rev/ae4e4daebdc4
https://hg.mozilla.org/mozilla-central/rev/6c2e7cfa5484

Comment 42

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16095 for changes under testing/web-platform/tests

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I notice that testing/web-platform/meta/css/css-lists/inheritance.html.ini marks a test as disabled due to a bug that's already fixed... is that intentional?

Comment 44

[Mats Palmgren (inactive)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #43)

I notice that testing/web-platform/meta/css/css-lists/inheritance.html.ini marks a test as disabled due to a bug that's already fixed... is that intentional?

Good catch, I didn't notice that bug got fixed.
The test PASS now so I'll just remove that .ini file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2313d92d15911673321417dcd60c2acac3140aa6

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7970abe6388b
Follow-up: remove unnecessary .ini since the test pass.  r=me  DONTBUILD

Comment 46

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7970abe6388b

Comment 47

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/16095
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/cJoUW5k_TAy0JBDYXKs1aw)

Comment 48

[Rachel Andrew]

I looked at this for developer docs, and have added a note to the 68 release notes. I don't think there is anything else here that needs documenting but please let me know if I have missed something.