[css-lists] Support for parsing counter-reset: reversed(counter-name) [chromium/src : main]
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 5 comments![]( "Open in Gerrit")
bool contains_reversed_function) {Seems this is just RuntimeEnabledFeatures::CSSCounterResetReversedEnabled(). Why do we need to pass that in?
void SetIsResetReversed() { is_reset_reversed_ = true; }I think it would be better to always set the is_reset_reversed_ flag along with the reset_value_. So instead, have a SetResetReversedValue() which sets both the reset_value_ and the revesed flag.
bool HasResetValue() const { return !!reset_value_; }reset_value_.has_value()
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }I think `reset_value_.has_value()` is clearer.
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }Won't is_reset_reversed_ only be true if `reset_value_.has_value()` is true?
If so, we don't need to add this check.
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 6 comments![]( "Open in Gerrit")
Thanks for your review! Would you take a look once again, please?
Seems this is just RuntimeEnabledFeatures::CSSCounterResetReversedEnabled(). Why do we need to pass that in?
This is because `counter-increment` and `counter-set` also use `ConsumeCounter()`. They should parse only `[ <counter-name> <integer>? ]+ | none` except `<reversed-counter-name> <integer>?`
void SetIsResetReversed() { is_reset_reversed_ = true; }I think it would be better to always set the is_reset_reversed_ flag along with the reset_value_. So instead, have a SetResetReversedValue() which sets both the reset_value_ and the revesed flag.
As I mentioned [above](https://chromium-review.googlesource.com/c/chromium/src/+/7150339/comment/079495f8_00dcfbd9/), `is_reset_reversed_` can be set without `reset_value_`.
bool HasResetValue() const { return !!reset_value_; }Minseong Kimreset_value_.has_value()
Done
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }Won't is_reset_reversed_ only be true if `reset_value_.has_value()` is true?
If so, we don't need to add this check.
No, it won't. If we specify `reversed(counter-name)` without a <integer> value in CSS, then only `is_reset_reversed_` is true and `reset_value_.has_value()` is false.
This is because the [spec](https://drafts.csswg.org/css-lists-3/#counter-reset) says,
> `<counter-name> <integer>?` instantiates a counter of the given <counter-name> with a starting value of the given <integer>, defaulting to 0.
> `<reversed-counter-name> <integer>?` instantiates a reversed counter of the given <counter-name> with a starting value of the given <integer>, **or no starting value if not given.**
So, I implemented the code in `style_builder_functions.tmpl`,
```
// ...
if (auto* reversed_function = DynamicTo<CSSFunctionValue>(*item)) {
AtomicString identifier(
To<CSSCustomIdentValue>(reversed_function->First()).Value());
CounterDirectives& directives =
map.insert(identifier, CounterDirectives()).stored_value->value;
directives.SetIsResetReversed();
continue;
}
// ...
```
Do I understand correctly? WDYT?
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }I think `reset_value_.has_value()` is clearer.
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 3 comments![]( "Open in Gerrit")
(a.value.ResetValue() != b.value.ResetValue() ||Won't this crash if reset_value_ is nullopt?
Maybe it would be better for ResetValue() to return an optional and handle via checking for nullopt instead?
bool contains_reversed_function) {Minseong KimSeems this is just RuntimeEnabledFeatures::CSSCounterResetReversedEnabled(). Why do we need to pass that in?
This is because `counter-increment` and `counter-set` also use `ConsumeCounter()`. They should parse only `[ <counter-name> <integer>? ]+ | none` except `<reversed-counter-name> <integer>?`
I think `accept_reversed_function` is a better name.
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }Minseong KimWon't is_reset_reversed_ only be true if `reset_value_.has_value()` is true?
If so, we don't need to add this check.
No, it won't. If we specify `reversed(counter-name)` without a <integer> value in CSS, then only `is_reset_reversed_` is true and `reset_value_.has_value()` is false.
This is because the [spec](https://drafts.csswg.org/css-lists-3/#counter-reset) says,
> `<counter-name> <integer>?` instantiates a counter of the given <counter-name> with a starting value of the given <integer>, defaulting to 0.
> `<reversed-counter-name> <integer>?` instantiates a reversed counter of the given <counter-name> with a starting value of the given <integer>, **or no starting value if not given.**
So, I implemented the code in `style_builder_functions.tmpl`,```
// ...
if (auto* reversed_function = DynamicTo<CSSFunctionValue>(*item)) {
AtomicString identifier(
To<CSSCustomIdentValue>(reversed_function->First()).Value());
CounterDirectives& directives =
map.insert(identifier, CounterDirectives()).stored_value->value;
directives.SetIsResetReversed();
continue;
}
// ...```
Do I understand correctly? WDYT?
I think this calls for some documentation in CounterDirectives.
Is the implicit 0 for non-resets stored as an actual value, or is that also a nullopt?
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 3 comments![]( "Open in Gerrit")
Won't this crash if reset_value_ is nullopt?
Maybe it would be better for ResetValue() to return an optional and handle via checking for nullopt instead?
Ah, you're right. Thank you. I changed it to return an optional.
bool contains_reversed_function) {Minseong KimSeems this is just RuntimeEnabledFeatures::CSSCounterResetReversedEnabled(). Why do we need to pass that in?
Rune LillesveenThis is because `counter-increment` and `counter-set` also use `ConsumeCounter()`. They should parse only `[ <counter-name> <integer>? ]+ | none` except `<reversed-counter-name> <integer>?`
I think `accept_reversed_function` is a better name.
Done. Thanks for the suggestion!
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }Minseong KimWon't is_reset_reversed_ only be true if `reset_value_.has_value()` is true?
If so, we don't need to add this check.
Rune LillesveenNo, it won't. If we specify `reversed(counter-name)` without a <integer> value in CSS, then only `is_reset_reversed_` is true and `reset_value_.has_value()` is false.
This is because the [spec](https://drafts.csswg.org/css-lists-3/#counter-reset) says,
> `<counter-name> <integer>?` instantiates a counter of the given <counter-name> with a starting value of the given <integer>, defaulting to 0.
> `<reversed-counter-name> <integer>?` instantiates a reversed counter of the given <counter-name> with a starting value of the given <integer>, **or no starting value if not given.**
So, I implemented the code in `style_builder_functions.tmpl`,```
// ...
if (auto* reversed_function = DynamicTo<CSSFunctionValue>(*item)) {
AtomicString identifier(
To<CSSCustomIdentValue>(reversed_function->First()).Value());
CounterDirectives& directives =
map.insert(identifier, CounterDirectives()).stored_value->value;
directives.SetIsResetReversed();
continue;
}
// ...```
Do I understand correctly? WDYT?
I think this calls for some documentation in CounterDirectives.
Is the implicit 0 for non-resets stored as an actual value, or is that also a nullopt?
Okay. I added some documentation in CounterDirectives.
Is the implicit 0 for non-resets stored as an actual value, or is that also a nullopt?
The implicit 0 for non‑resets is stored as an *actual value* because it has a default value. The spec says,
If the <integer> is omitted, it defaults to 1 (for counter-increment) or 0 (for counter-set).
If there is not currently a counter of the given name on the element, the element instantiates a new counter of the given name with a starting value of 0 setting or incrementing its value.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 4 comments![]( "Open in Gerrit")
return std::ranges::equal(*a_map, *b_map, [](const auto& a, const auto& b) {This is not correct. You have no guarantee about the iteration order for a HashMap, so this might fail even if the HashMaps are equal.
We need to iterate over one of them and do a lookup with a comparison against the other.
(a.value.ResetValue() != b.value.ResetValue() ||Minseong KimWon't this crash if reset_value_ is nullopt?
Maybe it would be better for ResetValue() to return an optional and handle via checking for nullopt instead?
Ah, you're right. Thank you. I changed it to return an optional.
Could you add a couple of `test_no_interpolation()` tests, where at least one of the values has a nullopt for the reset value to cover that case?
class CounterDirectives {I was a bit confused by the comparisons in css_property_equality.cc not simply using an operator== before I realized this stores all of counter-set, counter-increment, counter-reset in the same object.
Add a couple of lines of documentation for the class saying that.
IsIncrement()/IsSet() should probably be named HasIncrement()/HasSet() to lessen that confusion.
bool IsReset() const { return !!reset_value_ || is_reset_reversed_; }- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 3 comments![]( "Open in Gerrit")
return std::ranges::equal(*a_map, *b_map, [](const auto& a, const auto& b) {This is not correct. You have no guarantee about the iteration order for a HashMap, so this might fail even if the HashMaps are equal.
We need to iterate over one of them and do a lookup with a comparison against the other.
Oh, yes. You're right. I fixed it in this CL.
(a.value.ResetValue() != b.value.ResetValue() ||Minseong KimWon't this crash if reset_value_ is nullopt?
Maybe it would be better for ResetValue() to return an optional and handle via checking for nullopt instead?
Rune LillesveenAh, you're right. Thank you. I changed it to return an optional.
Could you add a couple of `test_no_interpolation()` tests, where at least one of the values has a nullopt for the reset value to cover that case?
Done
I was a bit confused by the comparisons in css_property_equality.cc not simply using an operator== before I realized this stores all of counter-set, counter-increment, counter-reset in the same object.
Add a couple of lines of documentation for the class saying that.
IsIncrement()/IsSet() should probably be named HasIncrement()/HasSet() to lessen that confusion.
Okay. I added the documentation and renamed those functions.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
Hi, @sakh...@chromium.org. Would you review this CL, please?
I was playing with this idea in https://chromium-review.googlesource.com/c/chromium/src/+/6708788, and there I ended up having a separate class value to store counter data (CSSCounterDataValue), comparing with this CL that idea looks a bit more clean in terms of parsing and changing existing code, could you, please, have a look and tell me what you think, Minseong?
@fut...@chromium.org, wdyt?
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
Daniil SakhapovHi, @sakh...@chromium.org. Would you review this CL, please?
I was playing with this idea in https://chromium-review.googlesource.com/c/chromium/src/+/6708788, and there I ended up having a separate class value to store counter data (CSSCounterDataValue), comparing with this CL that idea looks a bit more clean in terms of parsing and changing existing code, could you, please, have a look and tell me what you think, Minseong?
@fut...@chromium.org, wdyt?
Thank you for pointing me to your previous work on this. I've reviewed that CL, and I agree that using a separate `CSSCounterDataValue` class seems like a cleaner solution :)
Would you be okay with me updating this CL to incorporate that design?
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
Daniil SakhapovHi, @sakh...@chromium.org. Would you review this CL, please?
Minseong KimI was playing with this idea in https://chromium-review.googlesource.com/c/chromium/src/+/6708788, and there I ended up having a separate class value to store counter data (CSSCounterDataValue), comparing with this CL that idea looks a bit more clean in terms of parsing and changing existing code, could you, please, have a look and tell me what you think, Minseong?
@fut...@chromium.org, wdyt?
Thank you for pointing me to your previous work on this. I've reviewed that CL, and I agree that using a separate `CSSCounterDataValue` class seems like a cleaner solution :)
Would you be okay with me updating this CL to incorporate that design?
- Minseong Kim
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 1 comment![]( "Open in Gerrit")
I updated this CL by creating `CSSCounterDataValue`. Would you, please, review this CL?
Related details
- Daniil Sakhapov
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Code-Review | +1 |
Daniil SakhapovHi, @sakh...@chromium.org. Would you review this CL, please?
Minseong KimI was playing with this idea in https://chromium-review.googlesource.com/c/chromium/src/+/6708788, and there I ended up having a separate class value to store counter data (CSSCounterDataValue), comparing with this CL that idea looks a bit more clean in terms of parsing and changing existing code, could you, please, have a look and tell me what you think, Minseong?
@fut...@chromium.org, wdyt?
Daniil SakhapovThank you for pointing me to your previous work on this. I've reviewed that CL, and I agree that using a separate `CSSCounterDataValue` class seems like a cleaner solution :)
Would you be okay with me updating this CL to incorporate that design?
Sure, go ahead, please.
Done
bool operator==(const CounterDirectives& a, const CounterDirectives& b) {
return a.reset_value_ == b.reset_value_ &&
a.is_reset_reversed_ == b.is_reset_reversed_ &&
a.increment_value_ == b.increment_value_ &&
a.set_value_ == b.set_value_;
}super optional nit: if I'm not mistaken, we can just say `bool operator==(const CounterDirectives&) const = default;` and remove the C++ file at all?
Related details
- Minseong Kim
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 4 comments![]( "Open in Gerrit")
class CSSCounterDataValue final : public CSSValue {The naming here is really confusing, and there's no documentation that explains it. What is a CSSCounterDataValue vs a CSSCounterValue? (rhetorical question, but I had to check)
"Data" doesn't say anything. Everything is "data".
IIUC, CSSCounterValue is the value of a counter() in generated content?
Maybe CSSCounterValue should be CSSCounterFunctionValue or CSSCounterContentValue (probably the latter)?
Then, a more natural name for this would be CSSCounterValue?
But still, a couple of lines of documentation of what this represent is good.
CSSValue* ConsumeCounter(CSSParserTokenStream&,This name sounds like it's consuming a single counter, while it consumes a list.
ConsumeCounters() or ConsumeCounterList()?
cssvalue::CSSCounterDataValue* ConsumeCounterDataValue(ConsumeCounter()?
bool is_reversed) {I think it would be more readable if you passed on the accept parameter and did the Peek for reversed here instead.
Related details
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim added 6 comments![]( "Open in Gerrit")
The naming here is really confusing, and there's no documentation that explains it. What is a CSSCounterDataValue vs a CSSCounterValue? (rhetorical question, but I had to check)
"Data" doesn't say anything. Everything is "data".
IIUC, CSSCounterValue is the value of a counter() in generated content?
Maybe CSSCounterValue should be CSSCounterFunctionValue or CSSCounterContentValue (probably the latter)?
Then, a more natural name for this would be CSSCounterValue?
But still, a couple of lines of documentation of what this represent is good.
Yes, I agree that it's confusing. Thanks for pointing it.
I think renaming `CSSCounterValue` to `CSSCounterContentValue` is good because it's used for `ContentData`. So, I renamed it in [an another CL](https://chromium-review.googlesource.com/c/chromium/src/+/7210112).
And, I also renamed `CSSCounterDataValue` to `CSSCounterValue` and added some documentations in this CL.
This name sounds like it's consuming a single counter, while it consumes a list.
ConsumeCounters() or ConsumeCounterList()?
Done
cssvalue::CSSCounterDataValue* ConsumeCounterDataValue(Minseong KimConsumeCounter()?
Done
I think it would be more readable if you passed on the accept parameter and did the Peek for reversed here instead.
Done
bool operator==(const CounterDirectives& a, const CounterDirectives& b) {
return a.reset_value_ == b.reset_value_ &&
a.is_reset_reversed_ == b.is_reset_reversed_ &&
a.increment_value_ == b.increment_value_ &&
a.set_value_ == b.set_value_;
}super optional nit: if I'm not mistaken, we can just say `bool operator==(const CounterDirectives&) const = default;` and remove the C++ file at all?
Yeah, we can use `default` on this. I'll change this by creating an another CL. Thanks!
Related details
- Daniil Sakhapov
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
lgtm
- Daniil Sakhapov
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted Code-Review+1![]( "Open in Gerrit")
- Minseong Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minseong Kim (Gerrit)
Minseong Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Minseong Kim
| Commit-Queue | +2 |
1 comment
Thank you all for the reviews.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[css-lists] Support for parsing counter-reset: reversed(counter-name)
This CL introduces support for the `reversed()` function within the
`counter-reset` CSS property. This allows authors to specify that a
counter should be reset in a reversed order.
This CL includes:
- Adding `reversed` as a recognized CSS value keyword.
- Adding `CSSCounterValue` class for parsing.
- Modifying `CounterDirectives` to store a `is_reset_reversed_` flag.
- Updating CSS parsing utilities to correctly parse new syntax.
- Updating relevant WPTs to reflect the new parsing behaviour.
This CL also fixes `CounterRulesEqual` by iterating over one of
`CounterDirectiveMap` and do a lookup with a comparison against the
other.
The new behavior is behind a runtime flag, CSSCounterResetReversed.
- M third_party/blink/renderer/build/scripts/core/css/properties/templates/css_properties.cc.tmpl
- M third_party/blink/renderer/build/scripts/core/css/properties/templates/style_builder_functions.tmpl
- M third_party/blink/renderer/core/css/build.gni
- M third_party/blink/renderer/core/css/counters_attachment_context.cc
- A third_party/blink/renderer/core/css/css_counter_value.cc
- A third_party/blink/renderer/core/css/css_counter_value.h
- M third_party/blink/renderer/core/css/css_property_equality.cc
- M third_party/blink/renderer/core/css/css_value.cc
- M third_party/blink/renderer/core/css/css_value.h
- M third_party/blink/renderer/core/css/css_value_keywords.json5
- M third_party/blink/renderer/core/css/properties/computed_style_utils.cc
- M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
- M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
- M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
- M third_party/blink/renderer/core/html/list_item_ordinal.cc
- M third_party/blink/renderer/core/style/counter_directives.cc
- M third_party/blink/renderer/core/style/counter_directives.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/external/wpt/css/css-lists/css-lists-no-interpolation.html
- D third_party/blink/web_tests/external/wpt/css/css-lists/parsing/counter-reset-valid-expected.txt
- M third_party/blink/web_tests/external/wpt/html/rendering/non-replaced-elements/lists/lists-styles-expected.txt
- M third_party/blink/web_tests/external/wpt/html/rendering/non-replaced-elements/lists/lists-styles-quirks-expected.txt
Code-Review: +1 by Daniil Sakhapov, +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/56364
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |