Bug 97337 – [css-lists] Implement `counter-set` property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andrei Onea]

The latest CSS3 lists editor's draft defines the new counter-set property. We need to also implement this in WebKit.

[Alexey Proskuryakov]

> We need to also implement this in WebKit.

Could you please elaborate? Why is it needed?

[Andrei Onea]

(In reply to comment #1)
> > We need to also implement this in WebKit.
> 
> Could you please elaborate? Why is it needed?

Yeah, sorry. s/need/might want/
I've been working on a patch for this feature during a WebKit hackathon, and thought it could be easily added to WebKit, since it's such a low hanging fruit.

Currently, only counter-reset and counter-increment are supported, and the current editor's draft also adds the counter-set property. http://dev.w3.org/csswg/css3-lists/#counter-set

[Andrei Onea]

Created attachment 165382 [details]
Patch

[Early Warning System Bot]

Comment on attachment 165382 [details]
Patch

Attachment 165382 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13992647

[Build Bot]

Comment on attachment 165382 [details]
Patch

Attachment 165382 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13989699

[WebKit Review Bot]

Comment on attachment 165382 [details]
Patch

Attachment 165382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14002411

[Peter Beverloo (cr-android ews)]

Comment on attachment 165382 [details]
Patch

Attachment 165382 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13985747

[Early Warning System Bot]

Comment on attachment 165382 [details]
Patch

Attachment 165382 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13984899

[Andrei Onea]

Created attachment 165563 [details]
Patch

[Darin Adler]

Comment on attachment 165563 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Test: fast/css/counters/counter-set.html

This seems like really light test coverage for this. We need a test that covers a bit more, at least exercising all the code that was added.

For example, this does not test getComputedStyle behavior for the new property, nor does it test the behavior of changes to the property rather than just an initial value set. It does not test parsing edge cases instead having just a single test case.

With all the code we have to write to support things like inheriting we need to make sure our test cases cover those cases as well. Removing any line of code should make a test fail, but I think that currently there are many lines of code I could remove without having any effect on this one test.

> Source/WebCore/css/StyleBuilder.cpp:1002
> -enum CounterBehavior {Increment = 0, Reset};
> +enum CounterBehavior {Increment = 0, Reset, Set};

Nor new, but the formatting here is not our standard. There should be spaces inside the braces { Increment, Reset, Set } and the = 0 should not be there since it adds no value.

> Source/WebCore/css/StyleBuilder.cpp:1028
> -            if (counterBehavior == Reset) {
> +            switch (counterBehavior) {
> +            case Reset:
>                  directives.inheritReset(it->second);
> -            } else {
> +                break;
> +            case Increment:
>                  directives.inheritIncrement(it->second);
> +                break;
> +            case Set:
> +                directives.inheritSet(it->second);
> +                break;
> +            default:
> +                ASSERT_NOT_REACHED();
>              }

We normally don’t include defaults in switch statements, because that turns off the compiler warnings for unhandled enum values. I understand the value of the ASSERT_NOT_REACHED, but the compile time warning is even more valuable.

There may also be a more elegant way to handle this. Passing in an enum and using a bunch of switch statements seems inferior to using a traits class that can supply the various functions needed inside the class.

> Source/WebCore/css/StyleBuilder.cpp:1055
> -            if (counterBehavior == Reset)
> +            switch (counterBehavior) {
> +            case Reset:
>                  it->second.clearReset();
> -            else
> +                break;
> +            case Increment:
>                  it->second.clearIncrement();
> +                break;
> +            case Set:
> +                it->second.clearSet();
> +                break;
> +            default:
> +                ASSERT_NOT_REACHED();
> +            }

Same comment applies here.

> Source/WebCore/css/StyleBuilder.cpp:1082
> -            if (counterBehavior == Reset) {
> +            switch (counterBehavior) {
> +            case Reset:
>                  directives.setResetValue(value);
> -            } else {
> +                break;
> +            case Increment:
>                  directives.addIncrementValue(value);
> +                break;
> +            case Set:
> +                directives.setSetValue(value);
> +                break;
> +            default:
> +                ASSERT_NOT_REACHED();
> +                break;

And here.

> Source/WebCore/rendering/CounterNode.h:45
> -    static PassRefPtr<CounterNode> create(RenderObject*, bool isReset, int value);
> +    static PassRefPtr<CounterNode> create(RenderObject*, bool isSet, bool isReset, int value);

The use of two booleans here is not good. We should replace the boolean with an enum. Rather than having two booleans that if they were both set would be incorrect.

> LayoutTests/fast/css/counters/counter-set.html:19
> +        newSpanElement.innerText = window.internals.counterValue(document.getElementById(spanList.item(i).getAttribute("id"))) + " ";

You can use “internals” here, no need to use “window.internals”.

But also, the test is carefully using “window.testRunner” to try to run outside the test runner, but that’s really pointless if the test relies on window.internals which is also present only inside the test runner.

What does this test do in the browser?

[Zihua Li]

Has there been any progress on this issue?

For further references, Chromium/Blink recently implemented this property and will release it in Chrome v85: https://www.chromestatus.com/feature/4688138070917120

[Mats Palmgren]

Any chance Safari could add 'counter-set' for better web-compat?
This is causing real issues for web developers.
Firefox and Chrome already support it for a few years now...

[Cameron McCormack (:heycam)]

The testing situation is improved a little now, with some counter-set tests in WPT.  The patch probably needs a bit of unbitrotting though.

[Radar WebKit Bug Importer]

<rdar://problem/88663348>

[Brent Fulgham]

One use case in the wild:
“Our product is a WYSIWYG editor library, and we need it to control list numbering.”

[Tim Nguyen (:ntim)]

Pull request: https://github.com/WebKit/WebKit/pull/16868

[EWS]

Committed 267137@main (35c672ab4264): <https://commits.webkit.org/267137@main>

Reviewed commits have been landed. Closing PR #16868 and removing active labels.