[css-lists] Implement presentational hint for the `counter-reset` [chromium/src : main]
Minseong Kim (Gerrit)
Minseong Kim added 5 comments![]( "Open in Gerrit")
Thanks for the reviews. Would you review again, please?
if (RuntimeEnabledFeatures::CSSCounterResetReversedEnabled() &&Rune LillesveenSo, IIUC, we cannot represent this modification as presentation style because it's combined with declarations from author style in a way that is not expressible using current CSS primitives?
It looks like the counter-reset part could be represented as presentation style?
Or actually, looking at the code below, it does indeed like this could be purely presentation style. What does the spec say?
Rune LillesveenIn fact, it's specified as presentation style here:
Minseong KimHTML element specific presentation styles are implemented in overrides of CollectStyleForPresentationAttribute()
Ah, yes, that's the correct way! Thank you so much for the information. Done.
builder.AccessCounterDirectives().insert(list_item_identifier,
new_directives);Minseong Kimbtw, I don't think we should override "list-item" counter directive, if the element has one defined from style?
let's add a test for it, which has some non-default "list-item" counter sets and reversed on <ol>?
This didn't override the "list-item" counter because this logic was guarded by `if (!directives.IsReset())`.
> let's add a test for it, which has some non-default "list-item" counter sets and reversed on <ol>?
Can I ask `third_party/blink/web_tests/external/wpt/css/css-lists/li-value-reversed-025.html` is right one?
kSubtreeStyleChange,Minseong KimkSubtreeStyleChange can be super-expensive. Why do we need to recalc all descendants?
Removed. `SetNeedsStyleRecalc` is not need after moving the logic into `CollectStyleForPresentationAttribute`.
if (reversed == IsReversed()) {Minseong KimHmm, I need to check if changing the reversed attribute value from true to false and changing to true immediately causes any issues.
Done
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. |
Minseong Kim (Gerrit)
Message from Minseong Kim![]( "Open in Gerrit")
- Daniil Sakhapov
- 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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 3 comments![]( "Open in Gerrit")
int64_t counter_value = counter_data_value->ComputeNumber(state.CssToLengthConversionData());ComputeInteger maybe?
// This logic depends on both attributes. To avoid duplicate processing,
// if both are present, skip when processing the 'reversed' attribute.
if (name == html_names::kReversedAttr && has_explicit_start_) {
return;
}what if you already have an explicit start attribute, but then with JS you set reversed = true, then due to this `if` it would early return, and 'start' is cached, so we don't see a change?
maybe just remove it at let the latest pass override to produce the final correct value?
list3.setAttribute("reversed", "");why this? maybe it's due to the early return problem I've mentioned above?
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. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 2 comments![]( "Open in Gerrit")
builder.AccessCounterDirectives().insert(list_item_identifier,
new_directives);Minseong Kimbtw, I don't think we should override "list-item" counter directive, if the element has one defined from style?
let's add a test for it, which has some non-default "list-item" counter sets and reversed on <ol>?
This didn't override the "list-item" counter because this logic was guarded by `if (!directives.IsReset())`.
> let's add a test for it, which has some non-default "list-item" counter sets and reversed on <ol>?
Can I ask `third_party/blink/web_tests/external/wpt/css/css-lists/li-value-reversed-025.html` is right one?
I see now, thanks, looks good!
bool IsReversed() const {
if (RuntimeEnabledFeatures::CSSCounterResetReversedEnabled()) {
return ListItemCounterDirectives().IsResetReversed();
}
return is_reversed_;
}Daniil SakhapovJust a side note - I was a bit worried about this, since - https://developer.mozilla.org/en-US/docs/Web/API/HTMLOListElement/reversed says `It reflects the reversed attribute of the <ol> element.`
But in IDL (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_olist_element.idl;l=25;drc=939a03bab4837456f0924dbc2502f6f557dc0801) we have - `[CEReactions, Reflect] attribute boolean reversed;`, which probably generates something like `bool HTMLOListElement::reversed() { return hasAttribute("reversed"); }`.
Meaning that we don't use IsReversed() to interact with JS `revesed` prop, so it should be fine.But it raises another question. This IsReversed() function now requires style to be clean, right? Could you, please, double check that it doesn't conflict with the call sites that use it?
Maybe we need to think through some test that changes attribute from JS and checks result before the style is triggered? (or maybe it's not even possible)
@fut...@chromium.org, could you check my thoughts, please?
Minseong KimI gave it more thought though and found that it is a more complex indeed.
Check HTMLOListElement::ParseAttribute, when we change `reversed`, we end up immediatly calling InvalidateItemValues().
THat sounds dangerous? Or it will just mark list items as layout and paint dirty, but then RecalcOwnStyle changes will override those changes anyway, so that the final update is still correct?
Rune, could you check the logic, please?
Daniil SakhapovYes, `IsReversed()` requires the style to be clean. I have checked the call sites: `CountersAttachmentContext::MaybeCreateListItemCounter`, `ListItemOrdinal::IsInReversedOrderedList` and `ListItemOrdinal::CalcValue` are invoked during the Layout phase.
On the other hand, `HTMLOListElement::ParseAttribute` is triggered during the DOM/Attribute Parsing phase. However, it only calls `InvalidateItemValues()` to mark the ordinals as dirty. It does not perform any immediate calculations that require clean style. So, there should be no conflict.. right?
sounds good, I'll just let Rune have a second look