[css-lists] Implement the counter-set property

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
22 days ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Regressed 1 bug, {dev-doc-complete})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

Attachments

(2 attachments)

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.

Attachment #9034771 - Flags: review?(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.
Attachment #9034771 - Flags: review?(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:futhark@chromium.org">
```

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

No big deal if not though.
Attachment #9034772 - Flags: review?(emilio) → review+

(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?

(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.

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)

(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.

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.)

(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.

Apparently you cannot, see bug 1518304.

Yikes. :(

Pushed by mpalmgren@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16096 for changes under testing/web-platform/tests
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)
Regressions: 1548753

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.

Regressions: 1567773
Regressions: 1568449
You need to log in before you can comment on or make changes to this bug.