1518201 - [css-lists] Implement the counter-set property

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

https://drafts.csswg.org/css-lists/#propdef-counter-set

Comment 1

[Mats Palmgren (inactive)]

Attachment: [css-lists] Implement the counter-set property.

It's basically the same as counter-increment except that it assigns an absolute value instead of incrementing. It's needed to implement <li value>, which will happen in bug 288704.

Comment 2

[Mats Palmgren (inactive)]

Attachment: [css-lists] Add some basic tests for the counter-set property.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9034771 [details] [diff] [review]
[css-lists] Implement the counter-set property.

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

Looks good, but the Rust code duplication shouldn't be needed.

Just renaming all occurrences of CounterReset for CounterSetOrReset and using that in the new property as well as counter-reset should be enough.

Alternatively, you can alias them everywhere using `pub use self::CounterReset as CounterSet;` in the relevant counters.rs files, but that may inhibit some code-sharing optimizations we do for cloning declarations, animations, and other bits (see all the grouby usage in properties.mako.rs and animated_properties.mako.rs), so I'd rather not do that.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4602,5 @@
>      RestoreFrameStateFor(aNewFrame, aState.mFrameState);
>    }
>  
>    if (aAllowCounters &&
> +      mCounterManager.AddCounterChanges(aNewFrame)) {

I think this fits on one line now.

::: layout/style/GeckoBindings.cpp
@@ +1409,5 @@
>      aContent->SetCounterResetAt(i, data.mCounter, data.mValue);
>    }
>  }
>  
> +void Gecko_ClearAndResizeCounterSets(nsStyleContent* aContent,

If you fancy about it, you can refactor this as well so there's a single Clear / Copy function for all three properties that takes an nsTArray<nsStyleCounterData>*, and just call it with `&mut self.gecko.m${counter_property}s` from gecko.mako.rs to reduce the duplication on the C++ side as well (it should allow you to remove the Set / Allocate functions from nsStyleStruct.h.

Though if it gets annoying to touch the mako stuff to much (I know the feeling :/), I can do that myself in a followup bug :)

::: layout/style/nsStyleStruct.h
@@ +2518,5 @@
> +    mSets.SetLength(aCount);
> +  }
> +
> +  void SetCounterSetAt(uint32_t aIndex, const nsString& aCounter,
> +                         int32_t aValue) {
```
nit: Please clang-format this before landing, indentation seems broken.

::: servo/components/style/values/generics/counters.rs
@@ +71,5 @@
> +/// A generic value for the `counter-set` property.
> +#[derive(
> +    Clone, Debug, Default, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss,
> +)]
> +pub struct CounterSet<I>(Counters<I>);

This should not be needed, since it is exactly the same as `CounterReset` in all regards, including Parse implementation.

Instead, let's rename `CounterReset` to `CounterSetOrReset` everywhere, and just use it in both .mako.rs entries.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9034772 [details] [diff] [review]
[css-lists] Add some basic tests for the counter-set property.

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

This looks great, thanks! :)

It may be useful to have some tests for dynamic changes to the property if you want, though given the previous patch is straight-forward, and that we don't have pre-existing tests for the other properties in WPT, looks like, I don't think I'd feel good enforcing it.

```patch
::: testing/web-platform/tests/css/css-lists/counter-reset-increment-set-display-contents.html
@@ -4,1 @@
>  <link rel="author" title="Rune Lillesveen" href="mailto:[e-mail]">
```

Maybe we should rename the file to `counter-set-reset-increment-*`? Same for the other existing test.

No big deal if not though.

Comment 5

[Mats Palmgren (inactive)]

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

+  void SetCounterSetAt(uint32_t aIndex, const nsString& aCounter,
+                         int32_t aValue) {

nit: Please clang-format this before landing, indentation seems broken.

Thanks, but do reviewers still need to worry about code formatting?
Isn't it automated by a commit hook now?

Comment 6

[Mats Palmgren (inactive)]

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

+void Gecko_ClearAndResizeCounterSets(nsStyleContent* aContent,

If you fancy about it, you can refactor this as well so there's a single Clear / Copy function for all three properties that takes an nsTArray<nsStyleCounterData>*, and just call it with &mut self.gecko.m${counter_property}s from gecko.mako.rs to reduce the duplication on the C++ side as well (it should allow you to remove the Set / Allocate functions from nsStyleStruct.h.

That seems to be in generated code, so it's beyond my level of
understanding the internals of Stylo, sorry.

Instead, let's rename CounterReset to CounterSetOrReset everywhere, and just use it in both .mako.rs entries.

OK, I've merged Set/Reset everywhere.

Comment 7

[Mats Palmgren (inactive)]

So Bugzilla defaults to markdown now? Hmm, how do I turn that off?
(there is a "LI value" tag in the last comment that was hidden)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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

That seems to be in generated code, so it's beyond my level of
understanding the internals of Stylo, sorry.

No problem, I'm slowly trying to make it less magical... :)

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

Thanks, but do reviewers still need to worry about code formatting?
Isn't it automated by a commit hook now?

I think it's not (yet?), but icbw. There's a Phabricator hook somewhere that leaves a comment if your patch is misformatted.

Comment 9

[Daniel Holbert [:dholbert]]

RE formatting: the commit hook that mats mentioned has to be enabled manually in order for it to work -- see bug 1507007 comment 11.

(I believe some piece of the phabricator workflow -- maybe "moz-phab" itself? -- does a local clang-format as well. And maybe there's a bot that posts too if you bypasss moz-phab by using "arc" directly. But anyway, that moz-phab autoformatting clearly doesn't happen for patches posted directly to bugzilla and landed directly on inbound.)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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

So Bugzilla defaults to markdown now? Hmm, how do I turn that off?
(there is a "LI value" tag in the last comment that was hidden)

Apparently you cannot, see bug 1518304.

Comment 11

[Mats Palmgren (inactive)]

Apparently you cannot, see bug 1518304.

Yikes. :(

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/467284f16c74
[css-lists] Implement the counter-set property.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/9535b36adb21
[css-lists] Add some basic tests for the counter-set property.  r=emilio

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/467284f16c74
https://hg.mozilla.org/mozilla-central/rev/9535b36adb21

Comment 14

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

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

Comment 15

[Web Platform Test Sync Bot (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/16096
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/G3Pi1-yLRr-21WJFcozmzA)

Comment 16

[Rachel Andrew]

I've created a property page for counter-set it is still waiting for some data to be merged. Also added a line to the Firefox 68 release notes.