Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-lists] automatic start value of reversed list is affected by 'counter-increment: <counter> 0' nodes #6738

Open
MatsPalmgren opened this issue Oct 18, 2021 · 15 comments

Comments

@MatsPalmgren
Copy link

MatsPalmgren commented Oct 18, 2021

I just implemented the algorithm for instantiating reversed() counters described in:
https://drafts.csswg.org/css-lists/#instantiating-counters

I like how we now get symmetrical numbering, for example:

<style>
li { counter-increment: list-item 2; }
[reversed] > li { counter-increment: list-item -2; }
</style>

<ol>
<li>a</li>
<li>b</li>
<li>c</li>
</ol>

<ol reversed>
<li>a</li>
<li>b</li>
<li>c</li>
</ol>

now results in 2, 4, 6 followed by 6, 4, 2. But, I also note that the reversed result isn't backwards compatible: both Firefox and Chrome currently shows 2, 0, -2. However, I'm willing to try to ship it if other UAs agree with that...

Anyway, this issue is about this case:

<ol reversed>
<div><details><summary>x</summary></details></div>
<li>a</li>
<li>b</li>
<li>c</li>
</ol>

This example does not result in the same numbering as the first reversed case above, and is asymmetrical with the non-reversed case with the same content.

The problem is that <summary> has a counter-increment: list-item 0 (display: list-item would otherwise create a non-zero increment):
https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements

That affects step 3.2 in the algorithm: "If first is true, then add incrementNegated to num and set first to false.".

I think we should change this step to exclude counter-increment values that are zero, so the step would begin:
"If first is true and incrementNegated is non-zero, then ..."

(Arguably, the bug is that <summary> shouldn't have display: list-item to begin with. It's semantically wrong and just an ugly hack to display its disclosure arrow... Hopefully we can fix that at some point by making it display: block, remove its counter-increment, and generalize ::marker pseudos to apply to all display values in some way, but that's a bigger change that I don't want to tackle right now to fix this minor issue...)

I would argue this is a good change for three reasons:

  1. a counter-increment declaration with a zero value indicates an intent from the author that this element shouldn't affect the counter numbering
  2. it seems more practical/useful if it doesn't affect the counter start value
  3. it preserves the fact that counter-increment with a zero value is idempotent w.r.t. the counter value, which enables possible implementation optimizations

(Discussion that lead up to the algorithm in question is in #6233.)

CC @zcorpan @tabatkins @emilio @Loirooriol

@Loirooriol
Copy link
Contributor

I'm not sure special-casing 0 is the way to go. What if there is a positive counter-increment, that could be confusing too.

In general,

<ol reversed>
  <li style="counter-increment: list-item k1"></li>
  <li style="counter-increment: list-item k2"></li>
  ...
  <li style="counter-increment: list-item kn"></li>
</ol>

Then it's clear that:

  • The counter should be initialized to some value v0
  • The 1st item should have value v1 := v0 + k1
  • The 2nd item should have value v2 := v0 + k1 + k2
  • ...
  • The n-th item should have value vn := v0 + k1 + k2 + ... + kn

But basically v0 is free in reversed() with no start value, we need an additional constraint to determine it. Examples:

  • vn + k1 = 0. This is what the spec says now. Adding an initial counter-increment: list-item 0 item changes values.
    <ol reversed><!-- 7 -->
      <li style="counter-increment: list-item -2"><!-- 5 --></li>
      <li style="counter-increment: list-item -3"><!-- 2 --></li>
    </ol>
    <ol reversed><!-- 5 -->
      <li style="counter-increment: list-item 0"><!-- 5 --></li>
      <li style="counter-increment: list-item -2"><!-- 3 --></li>
      <li style="counter-increment: list-item -3"><!-- 0 --></li>
    </ol>
  • vn + kn = 0. Adding a final counter-increment: list-item 0 item changes values.
    <ol reversed><!-- 8 -->
      <li style="counter-increment: list-item -2"><!-- 6 --></li>
      <li style="counter-increment: list-item -3"><!-- 3 --></li>
    </ol>
    <ol reversed><!-- 5 -->
      <li style="counter-increment: list-item -2"><!-- 3 --></li>
      <li style="counter-increment: list-item -3"><!-- 0 --></li>
      <li style="counter-increment: list-item 0"><!-- 0 --></li>
    </ol>
  • vn = 1. Adding counter-increment: list-item 0 items doesn't change values.
    <ol reversed><!-- 6 -->
      <li style="counter-increment: list-item -2"><!-- 4 --></li>
      <li style="counter-increment: list-item -3"><!-- 1 --></li>
    </ol>
    <ol reversed><!-- 6 -->
      <li style="counter-increment: list-item 0"><!-- 6 --></li>
      <li style="counter-increment: list-item -2"><!-- 4 --></li>
      <li style="counter-increment: list-item -3"><!-- 1 --></li>
    </ol>
    <ol reversed><!-- 6 -->
      <li style="counter-increment: list-item -2"><!-- 4 --></li>
      <li style="counter-increment: list-item -3"><!-- 1 --></li>
      <li style="counter-increment: list-item 0"><!-- 1 --></li>
    </ol>
    <ol reversed><!-- 6 -->
      <li style="counter-increment: list-item 0"><!-- 6 --></li>
      <li style="counter-increment: list-item -2"><!-- 4 --></li>
      <li style="counter-increment: list-item -3"><!-- 1 --></li>
      <li style="counter-increment: list-item 0"><!-- 1 --></li>
    </ol>

IMO vn + kn = 0 is more intuitive than vn + k1 = 0. And vn = 1 seems the way to go if we want to prevent list-item 0 from messing the others.

@Loirooriol Loirooriol added the css-lists-3 Current Work label Oct 18, 2021
@Loirooriol
Copy link
Contributor

Another possibility would be changing the normal order and say that, for reversed lists, counter increments are applied after using the values. Specifically, when the counter is inherited by a following element in tree order which is not a descendant, I guess. Then we would just initialize to the negated sum of increments:

<ol reversed><!-- 5 -->
  <li style="counter-increment: list-item -2"><!-- 5 --></li>
  <li style="counter-increment: list-item -3"><!-- 3 --></li>
</ol>
<ol reversed><!-- 5 -->
  <li style="counter-increment: list-item 0"><!-- 5 --></li>
  <li style="counter-increment: list-item -2"><!-- 5 --></li>
  <li style="counter-increment: list-item -3"><!-- 3 --></li>
</ol>
<ol reversed><!-- 5 -->
  <li style="counter-increment: list-item -2"><!-- 5 --></li>
  <li style="counter-increment: list-item -3"><!-- 3 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
</ol>
<ol reversed><!-- 5 -->
  <li style="counter-increment: list-item 0"><!-- 5 --></li>
  <li style="counter-increment: list-item -2"><!-- 5 --></li>
  <li style="counter-increment: list-item -3"><!-- 3 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
</ol>

The UA style would have

ol[reversed] {
  counter-reset: reversed(list-item);
}
ol[reversed][start] {
  counter-reset: reversed(list-item) calc(attr(start integer));
}

Looks nice, but that would change the model quite a bit, which probably is not a good idea for implementations.

@MatsPalmgren
Copy link
Author

Another possibility would be changing the normal order and say that, for reversed lists, counter increments are applied after using the values.

Wouldn't that break child list numbering? Extending your first example:

<style>::marker { content: counters(list-item,".") }</style>
<ol reversed><!-- 5 -->
  <li style="counter-increment: list-item -2"><!-- 5 --><ol><li>a</ol></li>
  <li style="counter-increment: list-item -3"><!-- 3 --><ol><li>b</ol></li>
</ol>

The child list items would show "3.1a" and "0.1b" respectively, no?

Anyway, I don't think invasive changes like that are acceptable to us. Firefox have been shipping <ol reversed> and honoring counter-increment/set/reset: list-item values for years now. The one and only case where I think we're open to breaking changes are reverse counters that doesn't have a start value. As I said in the OP, the result 2, 0, -2 doesn't really make much sense, so authors encountering that is likely to have shaken their head and added a start value to get the expected result.

So the options I see are:

  1. let the first non-zero increment determine the start value (what I propose)
  2. let the last non-zero increment determine the start value (your vn + kn = 0 alternative, with the added condition that zero-valued increments are ignored)
  3. set a start value that will make the last item have the value 1 (your vn = 1 alternative)

The third option seems like it would lead to undesirable results in the common case. The common case is that all the increments have the same value, e.g. li { counter-increment: list-item -2; } and I think it would be surprising if the list doesn't end with the value 2 in this case. I think that's the intent behind the current algorithm in the spec.

So that leaves options 1 or 2. I don't feel strongly either way, but I do feel rather strongly that zero-valued increments should be ignored, for the reasons I gave in the OP. I think option 2 would be slightly more complicated to implement - the current algorithm can be done as one pass over the counters, but I think option 2 would need a second pass. Option 2 also needs a way to gracefully deal with counter-set nodes, which option 1 handles quite nicely.

So, given that I don't see any obvious advantages from switching to option 2, I still think option 1 is the best one. (In the common case of all list items having the same increment, they will lead to the same result anyway.)

@Loirooriol
Copy link
Contributor

The child list items would show "3.1a" and "0.1b" respectively, no?

I guess the increments would be delayed until we leave the subtree. So they would show "5.1a" and "3.1b". But yeah, that would be like a different model for reversed counters, which probably is not worth it. Just matches my intuition better.

it would be surprising if the list doesn't end with the value 2 in this case.

The last value could be decided with ol > :last-child { counter-set: list-item 2 } and then generate the start value from there using increments.

I think option 2 would need a second pass

I don't see why.

Option 2 also needs a way to gracefully deal with counter-set nodes

That should be analogous to option 1:

  1. Let num be 0.
  2. Let incrementNegated be 0.
  3. For each element or pseudo-element el that increments or sets the same counter in the same scope:
    1. Set incrementNegated to el’s counter-increment integer value for this counter, multiplied by -1.
    2. If el sets this counter with counter-set, then add that integer value to num and break this loop.
    3. Add incrementNegated to num.
  4. Add incrementNegated to num.
  5. Return num.

I don't see any obvious advantages from switching to option 2

The advantage is that (if there is no counter-set) the value of the last item depends on its own counter-increment, not the one of the 1st item, which probably is totally unrelated. Maybe it's clearer in:

Option 1 Option 2 Option 3
<ol reversed> -(-2*2 -1 -1 -1) = 7 -(-2 -1 -1 -1*2) = 6 -(-2 -1 -1 -1) +1 = 6
<li style="counter-increment: list-item -2"> 5 4 4
<li style="counter-increment: list-item -1"> 4 3 3
<li style="counter-increment: list-item -1"> 3 2 2
<li style="counter-increment: list-item -1"> 2 1 1
</ol>

Basically, in a non-reversed list, if you add items at the end, the former ones don't change. So in a reversed list, if you add items at the beginning (with no counter-set), the the latter ones shouldn't change either. That's provided by options 2 and 3, not by option 1.

@MatsPalmgren
Copy link
Author

That should be analogous to option 1:

That's a bit tricky to implement though; when looping forward over a list of nodes you don't really know which one will turn out to be the last one and you go in and out of nested scopes as you iterate...

Anyway, I'm sure it's implementable. As I said, I don't feel strongly either way, as long as we don't let counter-increments with a zero value affect the result I'm fine with either option.

So in your algorithm in the last comment we should change "Set incrementNegated to el’s counter-increment..." to "If el’s counter-increment integer value is non-zero, then set incrementNegated to el’s counter-increment..." (so that in step 4 we add in the last increment with a non-zero value). Does that work for you?

Perhaps we're ready to let the CSSWG vote on those two options?

@Loirooriol
Copy link
Contributor

I guess that would work. Another possible approach, rather than skipping 0 increments, could be adding a way to disable the automatic list-item ±1, maybe list-item none or something. In practice that would be like list-item 0, but the element wouldn't be considered to have an increment for list-item. Then <summary> could use that, and for other counters the author can just not include them in counter-increment instead of incrementing them by 0.

@MatsPalmgren
Copy link
Author

MatsPalmgren commented Oct 20, 2021

That feels like different kind of wallpaper to workaround that <summary> shouldn't be a list item in the first place. I'd rather fix that properly instead some time in the future (making it display:block). I think in practice counter-increment: <counter-name> 0 is the way to say that this node doesn't have an increment. I don't see why we need to introduce a new keyword when the intent from the author is already clear.

So I still think ignoring zero-valued increments is appropriate for the reasons I gave in the OP.

@MatsPalmgren
Copy link
Author

We want to ship reversed() counter support in Gecko, so we'd like a resolution on this.

Question 1: should a zero-valued counter-increment affect the automatic start value of reversed lists?

I feel rather strongly that they should not affect the start value, for the following reasons:
1. a counter-increment declaration with a zero value indicates an intent from the author that this element shouldn't affect the counter numbering
2. it seems more practical/useful if it doesn't affect the counter start value (it's a feature and it also avoids <summary> unintentionally affecting the start value)
3. it preserves the invariant that a counter-increment with a zero value is idempotent w.r.t. the counter value, which enables possible implementation optimizations

Question 2: as @Loirooriol suggested above (Option 2), using the value of the last counter-increment might be preferable over what the spec currently says, which is to use the value of the first counter-increment. (Given that the current spec text is fairly new (June #6233) I'm pretty sure no one have implemented/shipped that yet.) So, to be clear, the options for question 2 are:

A. keep the spec as is: the first counter-increment value determines the counter value at the end
B. change the spec to count the last counter-increment value twice instead, so that the last counter-increment value determines the counter value at the end

I don't have a strong opinion on Question 2, but I think his suggestion makes sense so I'm slightly in favor of changing the spec to that.

@Loirooriol
Copy link
Contributor

My concern with skipping 0 is something like this:

<ol reversed><!-- 0 -->
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
</ol>

With the current spec, changing the increment of an item in the middle will only alter preceding items:

<ol reversed><!-- 1 -->
  <li style="counter-increment: list-item 0"><!-- 1 --></li>
  <li style="counter-increment: list-item 0"><!-- 1 --></li>
  <li style="counter-increment: list-item -1"><!-- 0 --></li>
  <li style="counter-increment: list-item 0"><!-- 0 --></li>
</ol>

By skipping zero, all items will be affected:

<ol reversed><!-- 2 -->
  <li style="counter-increment: list-item 0"><!-- 2 --></li>
  <li style="counter-increment: list-item 0"><!-- 2 --></li>
  <li style="counter-increment: list-item -1"><!-- 1 --></li>
  <li style="counter-increment: list-item 0"><!-- 1 --></li>
</ol>

This seems inconsistent with what would happen when modifying a counter-increment in a list with non-zero increments. And would require updating all the items, possibly defeating some optimization.

@MatsPalmgren
Copy link
Author

That seems like a completely artificial testcase. During my 20+ years of working on Gecko I have never once seen anyone dynamically changing a counter-increment value as you suggest. Yes, treating zero-valued counter-increment differently is "inconsistent", that's the whole point of my proposal in this issue. I'm saying that doing that has practical benefits on real world web pages. Your "possibly defeating some optimization" claim is nonsense. You suggest optimizing a completely theoretical case that will never happen in practice while at the same time de-optimizing content that we know happen quite often on real web pages. Here's the most recent example we encountered - it's a large legal document with many thousands of <summary> elements which are all in the same (implied) counter scope. Excluding those from our internal CSS counter data structures is a significant performance optimization in Gecko (as I was alluding to in my point 3 above).

@tabatkins
Copy link
Member

tabatkins commented Oct 28, 2021

Here's the most recent example we encountered - it's a large legal document with many thousands of <summary> elements which are all in the same (implied) counter scope. Excluding those from our internal CSS counter data structures is a significant performance optimization in Gecko (as I was alluding to in my point 3 above).

It would have been useful to lead with that, so we knew the "possible implementation optimizations" you allude to in the OP aren't theoretical but extremely real and immediately demonstrated! That changes the calculus a bit.

Okay, so, reversed counters are weird in the first place. In the trivial case, when all the increments are identical and there's no counter-set, we can get a reasonable result that looks symmetrical with a non-reversed counter. But in any non-trivial case we'll get a break in the symmetry: some intentional, like a counter-set causing the following items to count down from it (in the non-reversed case, the preceding items are causally disconnected from the counter-set value, instead); some unintentional, just because the order of value inheritance is always preceding->following, which breaks the symmetry.

If we just set all the non-trivial cases to the side, we could go with "use the first increment" and just deal with it. But we're already going to some effort to line things up in non-trivial cases (counting up the increments, stopping at the first counter-set, etc), and you already have to process an arbitrary number of elements in the general case to find the counter's starting value anyway, so we might as well do the more-likely-to-be-correct thing and use the final increment instead (or the final increment before a counter-set), pretending that there's a last+1 increment of the same size that finally reduces the counter to zero as it reaches the end of its scope.

While respecting a final increment of zero would be "more correct", I agree that a zero increment in this case can be safely ignored. Especially in this case, since it's required to avoid changing the list-item counter, since if you leave it off it auto-increments.

So yeah, let's go with:

  • collect all increments from elements preceding the first counter-set (or all of them, if there's no set)
  • pretend there's a last+1 increment of the same size as the last non-zero increment, which takes the counter to the counter-set value (or to 0)
  • then reverse the increments back to the beginning to find the starting value

@Loirooriol
Copy link
Contributor

Here's the most recent example we encountered - it's a large legal document with many thousands of <summary> elements which are all in the same (implied) counter scope. Excluding those from our internal CSS counter data structures is a significant performance optimization in Gecko

As Tab says, it would have been useful to lead with that. Then the inconsistency of skipping 0 can be worth it. But I must be doing something wrong because I don't see any <summary> in that page.

@MatsPalmgren
Copy link
Author

MatsPalmgren commented Oct 28, 2021

Sorry, it seems I forgot what happened there... The performance issue was reported to us from a user here, but the use case was viewing the source of that page, not the page itself. So it was more a case of Firefox UI code shooting us in the foot... Anyway, we've seen this issue with a large number of <summary>s in the wild too, which is probably why I misremembered it and thought this was one of those...

EDIT: here's one of those external cases, where we introduced this optimization.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed automatic start value of reversed list is affected by 'counter-increment: <counter> 0' nodes, and agreed to the following:

  • RESOLVED: Skip 0 increments when determining the starting value of a reverse counter
The full IRC log of that discussion <dael> Topic: automatic start value of reversed list is affected by 'counter-increment: <counter> 0' nodes
<dael> github: https://github.com//issues/6738
<dael> TabAtkins: This is...set up is weird
<dael> TabAtkins: Some time ago we told html it was okay to style the disclousure triangle on a details using list item. That way they can use marker pseudo
<dael> TabAtkins: The definition in html spec says it's a display list item. b/c it auto increments the counter it also says counter-incemenet list-item 0.
<dael> TabAtkins: This means a page with a whole lot of summeries they're incrementing an unnamed counter
<dael> TabAtkins: Came up here with a reversed item counter. Question was what value to start at and there was perf issue in FF with an element named summary and there was a hang
<dael> TabAtkins: The question was how we could make this work better. Only reason summary interacts with list-item counter is anything with display: list-item must interact
<dael> TabAtkins: The way algo was writter to calc the starting coutner value for reversed list counts the 0 increments.
<dael> TabAtkins: Mats suggests we skip those b/c it would avoid weird performance and if you are ever explicitly 0 increment a counter you must set as 0 so if you do it you are probably indicating the is not something that should interact
<dael> TabAtkins: He concludes best to have 0 counters not count. I suggest we adopt this. For purpose of list skip 0 incrememnt when calc starting increment
<dael> TabAtkins: oriol has some comments this is a little weird but once Mats pointed out perf issue we thought it wasn't a big deal
<dael> TabAtkins: So we're fine with the change. Unless anyone has an argument they should count we'll accept that
<dael> astearns: Opinions?
<dael> astearns: If Mats and oriol agree I'm inclined to agree too
<dael> astearns: Prop: Do what Mats says? Or summary?
<dael> TabAtkins: Skip 0 increments when determining the starting value of a reverse counter
<dael> astearns: Obj?
<dael> RESOLVED: Skip 0 increments when determining the starting value of a reverse counter
<fantasai> https://github.com//issues/6781
<dael> TabAtkins: Pointing out, Mats pointed out it's weird we told HTML list it's okay to use display list-item here. Her would have prefered to use marker without making it a list-item. Separate issue linked to be able to do that.
<dael> TabAtkins: If you have interest in that, reference that issue
<dael> oriol: Another note, when disussing this I found other issues in the algo. I can file them as different issues since they're orthogonal.
<dael> astearns: Yes, please do that

@Loirooriol
Copy link
Contributor

Filed #6797 for repeating the last (non-zero) increment instead of the 1st one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-lists-3 Current Work
Projects
No open projects
Development

No branches or pull requests

4 participants