Bug 181084 - Special list-item counter starts from an incorrect number for ::before and ::after
Summary: Special list-item counter starts from an incorrect number for ::before and ::...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-21 09:35 PST by Roman Komarov
Modified: 2018-01-08 22:20 PST (History)
6 users (show)

See Also:


Attachments
Patch (33.35 KB, patch)
2018-01-07 21:59 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roman Komarov 2017-12-21 09:35:00 PST
## Steps to reproduce the problem:

1. Go to https://codepen.io/kizu/pen/QaKjmJ
2. Look at the custom counters

Or

1. Create an ordered list, optionally with a `start` attribute.
2. Add custom styles for the list items by using `counter(list-item)`.

## What is the expected behavior?

This counter should start from either the `1` or from the number set by `start` attribute.

## What went wrong?

The number the counters start from are wrong by one.

However, when the `value` attribute is used, the value is correct.

Edge browser renders everything in a correct way.

There is a hacky fix/workaround for this issue:

    ol.fix:not(*:root) > li:first-child:not([value]) {
      counter-increment: list-item 0;
    }
Comment 1 Darin Adler 2018-01-07 18:01:25 PST
Specification language for this is quite unclear. Nothing seems to indicate that the "list-item" counter should be affected by the start attribute on the <ul> element or the value attribute on the <li> element. Nothing clearly defines what the behavior should be if someone specifies counter-increment or counter-reset on the list item.

The "fix/workaround" is not compatible with the fix I made for WebKit. I could probably make a better fix if I had a more extensive test suite, demonstrating how counter-increment and counter-reset would affect things.

Has the Edge team contributed a test case for this to the CSS working group?
Comment 2 Darin Adler 2018-01-07 18:23:31 PST
There is some "sample HTML style sheet" that helps demonstrate the behavior they want.
Comment 3 Darin Adler 2018-01-07 21:59:18 PST
Created attachment 330668 [details]
Patch
Comment 4 zalan 2018-01-08 20:06:40 PST
Comment on attachment 330668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review

> Source/WebCore/rendering/RenderListItem.cpp:203
> +    auto* list = enclosingList(*this);
> +    auto* orderedList = is<HTMLOListElement>(list) ? downcast<HTMLOListElement>(list) : nullptr;
>  
>      // FIXME: This recurses to a possible depth of the length of the list.
> -    // That's not good -- we need to change this to an iterative algorithm.
> -    if (RenderListItem* previousItem = previousListItem(list, *this))
> -        return previousItem->value() + valueStep;
> -
> -    if (oListElement)
> -        return oListElement->start();
> +    // That's not good, and we can and should change this to an iterative algorithm.
> +    if (auto* previousItem = previousListItem(list, *this)) {
> +        m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1);

Couldn't you just use isInReversedOrderedList() here?

> Source/WebCore/rendering/style/CounterDirectives.h:37
> +struct CounterDirectives {
> +    std::optional<int> resetValue;
> +    std::optional<int> incrementValue;

This is a really nice cleanup (class -> struct).
Comment 5 Darin Adler 2018-01-08 21:58:24 PST
Comment on attachment 330668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review

>> Source/WebCore/rendering/RenderListItem.cpp:203
>> +        m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1);
> 
> Couldn't you just use isInReversedOrderedList() here?

I’ll do that, or something like it, in my next patch, which fixes the "iterative algorithm" comment here too.
Comment 6 Darin Adler 2018-01-08 22:04:55 PST
Comment on attachment 330668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review

>>> Source/WebCore/rendering/RenderListItem.cpp:203
>>> +        m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1);
>> 
>> Couldn't you just use isInReversedOrderedList() here?
> 
> I’ll do that, or something like it, in my next patch, which fixes the "iterative algorithm" comment here too.

I could do that, but it would re-do the work of calling enclosingList, which climbs the parent tree, and I don’t want to do that.
Comment 7 WebKit Commit Bot 2018-01-08 22:19:24 PST
Comment on attachment 330668 [details]
Patch

Clearing flags on attachment: 330668

Committed r226613: <https://trac.webkit.org/changeset/226613>
Comment 8 WebKit Commit Bot 2018-01-08 22:19:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-01-08 22:20:23 PST
<rdar://problem/36369157>