[Feature] Support symbols() in list-style-type [chromium/src : main]
Javier Contreras (Gerrit)
Javier Contreras added 3 comments![]( "Open in Gerrit")
[Feature] Support symbols() in list-style-type[symbols] or [countery-styles] depending on what you decide in the other CL
// An anonymous symbols() counter style has no name, so there is no
// corresponding @counter-style at-rule to report.this comment could be simplified to be easier to understand. If its `symbols` then it by definition has no corresponding counter style at rule
// The anonymous counter style of a symbols() function is immutable and
// self-contained, so once built it never becomes stale.we should probably add a CHECK to enforce this, and just return true then.
so something like:
if (IsSymbols())
CHECK(cointer_style_)
return true
Related details
- Ananya Anand
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. |
Javier Contreras (Gerrit)
Javier Contreras added 1 comment![]( "Open in Gerrit")
Ananya Anand (Gerrit)
Ananya Anand added 1 comment![]( "Open in Gerrit")
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. |
Ananya Anand (Gerrit)
Ananya Anand added 5 comments![]( "Open in Gerrit")
fixed, thanks!:)
[symbols] or [countery-styles] depending on what you decide in the other CL
Done
// An anonymous symbols() counter style has no name, so there is no
// corresponding @counter-style at-rule to report.this comment could be simplified to be easier to understand. If its `symbols` then it by definition has no corresponding counter style at rule
Done
// The anonymous counter style of a symbols() function is immutable and
// self-contained, so once built it never becomes stale.we should probably add a CHECK to enforce this, and just return true then.
so something like:
if (IsSymbols())
CHECK(cointer_style_)
return true
Done
Related details
- Javier Contreras
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Contreras (Gerrit)
Javier Contreras voted and added 1 comment![]( "Open in Gerrit")
Votes added by Javier Contreras
| Code-Review | +1 |
1 comment
// The anonymous counter style of a symbols() function is immutable and
// self-contained, so once built it never becomes stale.Ananya Anandwe should probably add a CHECK to enforce this, and just return true then.
so something like:
if (IsSymbols())
CHECK(cointer_style_)
return true
Done
- Ananya Anand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 9 comments![]( "Open in Gerrit")
Member<const cssvalue::CSSSymbolsValue> symbols_value_;Would symbols_funciton_value_ make sense? Same with other namings
// A symbols() function is a nameless counter style; callers must resolve it
// via GetCounterStyle() (which builds the anonymous style) rather than by
// name.nit: this part is ok to remove
bool IsSymbols() const { return symbols_value_ != nullptr; }IsSymbolsFunction
static ListStyleTypeData* CreateSymbols(const cssvalue::CSSSymbolsValue&);CreateSymbolsFunciton
#include "base/memory/values_equivalent.h"I think this should be already included by /css_symbols_value.h and can be removed
Type::kCounterStyle, g_empty_atom, nullptr, &symbols_value);nit: missing /var_name=/ comment for both https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments
// and built on demand by GetCounterStyle(), so its reference never becomesnit: this part can be removed
// and built on demand by GetCounterStyle(), so its reference never becomes
// stale.I'm assuming if we change the list style to a different symbols function we invalidate this. I'm guessing this is just needed for the counter styles global map?
return true;Should we check here that the symbols was properly created?
Related details
- Ananya Anand
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. |
Ananya Anand (Gerrit)
Ananya Anand added 10 comments![]( "Open in Gerrit")
Member<const cssvalue::CSSSymbolsValue> symbols_value_;Would symbols_funciton_value_ make sense? Same with other namings
yeah I think it's good to be more specific
// A symbols() function is a nameless counter style; callers must resolve it
// via GetCounterStyle() (which builds the anonymous style) rather than by
// name.nit: this part is ok to remove
Done
bool IsSymbols() const { return symbols_value_ != nullptr; }Ananya AnandIsSymbolsFunction
Done
static ListStyleTypeData* CreateSymbols(const cssvalue::CSSSymbolsValue&);Ananya AnandCreateSymbolsFunciton
Done
I think this should be already included by /css_symbols_value.h and can be removed
Done
Type::kCounterStyle, g_empty_atom, nullptr, &symbols_value);nit: missing /var_name=/ comment for both https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments
Done
// and built on demand by GetCounterStyle(), so its reference never becomesnit: this part can be removed
Done
// and built on demand by GetCounterStyle(), so its reference never becomes
// stale.I'm assuming if we change the list style to a different symbols function we invalidate this. I'm guessing this is just needed for the counter styles global map?
yep! A different symbols() produces a whole new ListStyleTypeData, so the old value is replaced rather than left stale to detect. This validity check only matters for named counter styles in the global map, so an anonymous symbols() can never go stale and sp I made it return true early.
return true;Should we check here that the symbols was properly created?
I don't think its rlly needed here bc a symbols() value only exists if it passed parsing, and the parser accepts exactly what building the counter style requires (>=1 symbol, and >= 2 for alphabetic/numeric) so CreateAnonymousCounterStyle() can't fail, and the CHECK inside it already backstops that.
Related details
- Alison Maher
- Javier Contreras
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. |
Alison Maher (Gerrit)
Alison Maher added 6 comments![]( "Open in Gerrit")
context.Count(WebFeature::kCSSSymbolsFunction);Might be worth a test to make sure we are firing this
return symbols;What if an author sets this to something like "symbols(...) some-garbage" - where there is garbage listed after the symbols function. Do we return invalid in that case? Might be worth a test if we don't have that one yet
DCHECK(symbols_function_value_);nit: CHECK is preferred instead of DCHECK when the check is trivial
DCHECK(!IsSymbolsFunction());nit: CHECK is preferred instead of DCHECK when the check is trivial
/*tree_scope=*/nullptr, &symbols_function_value);Instead of storing this, could we just create the counterstyle here based on it, rather than doing it lazily. Then we can get rid of symbols_function_value_. Then IsSymbolsFunction would check if IsCounterStyle and the name is anonymous or something to tell if it is a symbols function
return true;Ananya AnandShould we check here that the symbols was properly created?
I don't think its rlly needed here bc a symbols() value only exists if it passed parsing, and the parser accepts exactly what building the counter style requires (>=1 symbol, and >= 2 for alphabetic/numeric) so CreateAnonymousCounterStyle() can't fail, and the CHECK inside it already backstops that.
- Ananya Anand
- Javier Contreras
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. |
Ananya Anand (Gerrit)
Ananya Anand voted and added 6 comments![]( "Open in Gerrit")
Votes added by Ananya Anand
| Commit-Queue | +1 |
6 comments
Might be worth a test to make sure we are firing this
yeah fs added CSSPropertyParserTest.ListStyleTypeSymbolsFunctionUseCounter in css_property_parser_test.cc
return symbols;What if an author sets this to something like "symbols(...) some-garbage" - where there is garbage listed after the symbols function. Do we return invalid in that case? Might be worth a test if we don't have that one yet
yea its invalid. ListStyleType::ParseSingleValue returns as soon as it matches the symbols func, and the generic property parser then requires the whole value to be consumed. CSSPropertyParser::ParseSingleValue returns null when !stream.AtEnd() so any trailing "junk" after symbols() reject the entire declaration. We didn't have a test for that specific case, so I added two to list-style-type-invalid.html (checking for both cases where symbols() is followed up by a valid keyword and junk).
nit: CHECK is preferred instead of DCHECK when the check is trivial
Done
nit: CHECK is preferred instead of DCHECK when the check is trivial
Done
/*tree_scope=*/nullptr, &symbols_function_value);Instead of storing this, could we just create the counterstyle here based on it, rather than doing it lazily. Then we can get rid of symbols_function_value_. Then IsSymbolsFunction would check if IsCounterStyle and the name is anonymous or something to tell if it is a symbols function
I kept symbols_function_value_ bc it is the computed value of list-style-type and it's what getComputedStyle serializes. A CounterStyle is just the internal object that renders the markers but it isn't a CSSValue, so it can't stand in for that. Building it eagerly would be fine, but dropping the parsed value isn't bc we'd have to rebuild it from the counter style's already-defaulted descriptors, which is messier than just keeping it.
Related details
- Alison Maher
- Javier Contreras
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. |
Alison Maher (Gerrit)
Alison Maher added 2 comments![]( "Open in Gerrit")
return symbols;Ananya AnandWhat if an author sets this to something like "symbols(...) some-garbage" - where there is garbage listed after the symbols function. Do we return invalid in that case? Might be worth a test if we don't have that one yet
yea its invalid. ListStyleType::ParseSingleValue returns as soon as it matches the symbols func, and the generic property parser then requires the whole value to be consumed. CSSPropertyParser::ParseSingleValue returns null when !stream.AtEnd() so any trailing "junk" after symbols() reject the entire declaration. We didn't have a test for that specific case, so I added two to list-style-type-invalid.html (checking for both cases where symbols() is followed up by a valid keyword and junk).
Acknowledged
/*tree_scope=*/nullptr, &symbols_function_value);Ananya AnandInstead of storing this, could we just create the counterstyle here based on it, rather than doing it lazily. Then we can get rid of symbols_function_value_. Then IsSymbolsFunction would check if IsCounterStyle and the name is anonymous or something to tell if it is a symbols function
I kept symbols_function_value_ bc it is the computed value of list-style-type and it's what getComputedStyle serializes. A CounterStyle is just the internal object that renders the markers but it isn't a CSSValue, so it can't stand in for that. Building it eagerly would be fine, but dropping the parsed value isn't bc we'd have to rebuild it from the counter style's already-defaulted descriptors, which is messier than just keeping it.
So there should be precedence to convert the style format to a value format (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc;l=5205?q=CSSGridTemplateAreasValue%20&ss=chromium)
If we don't have enough info in CounterStyle to convert back to the value, then we should consider adding that as opposed to keeping both around
Related details
- Ananya Anand
- Javier Contreras
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. |
Ananya Anand (Gerrit)
Ananya Anand added 2 comments![]( "Open in Gerrit")
added some thoughts! lmk what you think
/*tree_scope=*/nullptr, &symbols_function_value);Ananya AnandInstead of storing this, could we just create the counterstyle here based on it, rather than doing it lazily. Then we can get rid of symbols_function_value_. Then IsSymbolsFunction would check if IsCounterStyle and the name is anonymous or something to tell if it is a symbols function
Alison MaherI kept symbols_function_value_ bc it is the computed value of list-style-type and it's what getComputedStyle serializes. A CounterStyle is just the internal object that renders the markers but it isn't a CSSValue, so it can't stand in for that. Building it eagerly would be fine, but dropping the parsed value isn't bc we'd have to rebuild it from the counter style's already-defaulted descriptors, which is messier than just keeping it.
So there should be precedence to convert the style format to a value format (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc;l=5205?q=CSSGridTemplateAreasValue%20&ss=chromium)
If we don't have enough info in CounterStyle to convert back to the value, then we should consider adding that as opposed to keeping both around
Yeah, that grid pattern works, but symbols() isn't quite the same case. A couple reasons:
1.Grid can reconstruct its value because what it stores is a lightweight, normalized struct (the grid map). But here we're dealing wiht CounterStyle, which is heavyweight and only built lazily (carries a lotta state, it needs GetDecimal(), and fallback resolution). So making it the source of truth would be a step down, not up since we'd have to build it eagerly for every symbols(), even ones only read through getComputedStyle or never rendered.
2.There's precedent for keeping the value. Custom properties store the CSSValue as the computed value and return it directly for getComputedStyle, comparing with base::ValuesEquivalent (same as we do here): https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/computed_style.cc (see GetVariableValue). That fits symbols() because what you write comes back unchanged when you read it with getComputedStyle so there's nothing to rebuild. Grid is diff bc it already needs that cell map for layout, so keeping the value too would be redundant, which is why it regenerates the value from the map instead. That reasoning only kicks in when you're already holding the resolved form, and we aren't.
3.The named @counter-style path in this same class already stores its reference (the name) and lazily caches the resolved CounterStyle. symbols() is just the inline-value version of that, so keeping the value is the consistent choice.
So I'd lean toward keeping symbols_function_value_, but if you'd still rather reconstruct from the CounterStyle, I'm happy to do that.
Related details
- Alison Maher
- Javier Contreras
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. |