[Feature]Support symbols() in counter() and counters() [chromium/src : main]
Javier Contreras (Gerrit)
Javier Contreras voted and added 6 comments![]( "Open in Gerrit")
Votes added by Javier Contreras
| Commit-Queue | +1 |
6 comments
[Feature]Support symbols() in counter() and counters()same comment as the other CLs
String CSSCounterContentValue::CustomCSSText() const {does symbols go through this path? If so, shouldnt the text be different rather than saying "counnters(...)?
If it doesn't go through this path on the other hand, why do we need the condition added below?
const CounterStyle* counter_style = GetSymbolsCounterStyle();
if (!counter_style) {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}this reads kind of confusingly. Previously we would call FindCounterStyle.. unconditionally, now this reads `if its not a counter style, call FindCounterStyle`. Which seems like the wrong way to read it considering what was there before.
Maybe reorder or rename variables here to make the intent clearer, or at the very least add a comment explaining.
// Regression test for the symbols() function in the counter() alt-text path
// (https://crbug.com/1176315). The CSS alt text uses the inline symbols()
// counter style, not the 'decimal' fallback.
TEST_F(AccessibilityTest, CSSAltTextCounterWithSymbolsFunction) {Thank you for adding accessibility tests. Could you corroborate that this is what we do for counter styles accessibility tests?
Found 2 FAIL, 0 TIMEOUT, 0 NOTRUN.why is this needed?
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 7 comments![]( "Open in Gerrit")
[Feature]Support symbols() in counter() and counters()same comment as the other CLs
Done
String CSSCounterContentValue::CustomCSSText() const {does symbols go through this path? If so, shouldnt the text be different rather than saying "counnters(...)?
If it doesn't go through this path on the other hand, why do we need the condition added below?
Done
const CounterStyle* counter_style = GetSymbolsCounterStyle();
if (!counter_style) {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}this reads kind of confusingly. Previously we would call FindCounterStyle.. unconditionally, now this reads `if its not a counter style, call FindCounterStyle`. Which seems like the wrong way to read it considering what was there before.
Maybe reorder or rename variables here to make the intent clearer, or at the very least add a comment explaining.
Done
// Regression test for the symbols() function in the counter() alt-text path
// (https://crbug.com/1176315). The CSS alt text uses the inline symbols()
// counter style, not the 'decimal' fallback.
TEST_F(AccessibilityTest, CSSAltTextCounterWithSymbolsFunction) {Thank you for adding accessibility tests. Could you corroborate that this is what we do for counter styles accessibility tests?
Done
Found 2 FAIL, 0 TIMEOUT, 0 NOTRUN.Ananya Anandwhy is this needed?
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 added 2 comments![]( "Open in Gerrit")
const CounterStyle* counter_style = GetSymbolsCounterStyle();
if (!counter_style) {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}Ananya Anandthis reads kind of confusingly. Previously we would call FindCounterStyle.. unconditionally, now this reads `if its not a counter style, call FindCounterStyle`. Which seems like the wrong way to read it considering what was there before.
Maybe reorder or rename variables here to make the intent clearer, or at the very least add a comment explaining.
Done
Hmm I still think the flow here is confusing, since the null check currently means the value uses a named counter style rather than symbols(), not that resolving a counter style failed.
I think it would be easier to understand and cleaner if it looked something like:
```
const CounterStyle* counter_style;
if (const CounterStyle* symbols_counter_style =
GetSymbolsCounterStyle()) {
counter_style = symbols_counter_style;
} else {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}
```
// (https://crbug.com/1176315). The CSS alt text uses the inline symbols()nit: this just links to the tracking item for the whole symbols function, so a little unecessary.
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 3 comments![]( "Open in Gerrit")
const CounterStyle* counter_style = GetSymbolsCounterStyle();
if (!counter_style) {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}Ananya Anandthis reads kind of confusingly. Previously we would call FindCounterStyle.. unconditionally, now this reads `if its not a counter style, call FindCounterStyle`. Which seems like the wrong way to read it considering what was there before.
Maybe reorder or rename variables here to make the intent clearer, or at the very least add a comment explaining.
Javier ContrerasDone
Hmm I still think the flow here is confusing, since the null check currently means the value uses a named counter style rather than symbols(), not that resolving a counter style failed.
I think it would be easier to understand and cleaner if it looked something like:
```
const CounterStyle* counter_style;
if (const CounterStyle* symbols_counter_style =
GetSymbolsCounterStyle()) {
counter_style = symbols_counter_style;
} else {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}
```
Done
// (https://crbug.com/1176315). The CSS alt text uses the inline symbols()nit: this just links to the tracking item for the whole symbols function, so a little unecessary.
- 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
| Commit-Queue | +1 |
1 comment
const CounterStyle* counter_style;
if (const CounterStyle* symbols_counter_style = GetSymbolsCounterStyle()) {
counter_style = symbols_counter_style;
} else {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}
String text = LayoutCounter::GenerateCounterText(std::move(counter_values),This logic is now repeated in two places, here and in `LayoutCounter::NullableCounterStyle`. We should extract it into a helper functino in content_data.h/.cc called something along the lines of `const CounterStyle& ResolveCounterStyle(style_engine)` that does the reusable logic:
```
if (const CounterStyle* symbols_style = GetSymbolsCounterStyle()) {
return *symbols_style;
}
return style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
```
And then we can call this same function on both `AltCounterContentData::UpdateText` and `LayoutCounter::NullableCounterStyle`
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")
const CounterStyle* counter_style;
if (const CounterStyle* symbols_counter_style = GetSymbolsCounterStyle()) {
counter_style = symbols_counter_style;
} else {
counter_style =
&style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
}
String text = LayoutCounter::GenerateCounterText(std::move(counter_values),This logic is now repeated in two places, here and in `LayoutCounter::NullableCounterStyle`. We should extract it into a helper functino in content_data.h/.cc called something along the lines of `const CounterStyle& ResolveCounterStyle(style_engine)` that does the reusable logic:
```
if (const CounterStyle* symbols_style = GetSymbolsCounterStyle()) {
return *symbols_style;
}
return style_engine.FindCounterStyleAcrossScopes(ListStyle(), GetTreeScope());
```And then we can call this same function on both `AltCounterContentData::UpdateText` and `LayoutCounter::NullableCounterStyle`
- 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 Code-Review+1![]( "Open in Gerrit")
- 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 15 comments![]( "Open in Gerrit")
nit: It might be helpful to more clearly state the relationship between some of these different classes just for context. i.e. What is ContentData etc and why we store things in these places
I'm having a hard time following all of the various relationships across the various files across between the various CLs
Member<const CSSValue> list_style_; // <counter-style-name> or symbols()Would it make things any simpler to store the name separate from the symbols function at this point
const CSSSymbolsValue* ListStyleSymbols() const {ListStyleSymbolsFunciton
return To<CSSCustomIdentValue>(*list_style_).Value();Should this also be dymnamicto (in the case that this is accidentally called from a list style symbols)
bool ListStyleIsSymbols() const { return list_style_->IsSymbolsValue(); }ListStyleIsSymbolsFunction
#include "base/memory/values_equivalent.h"Can this be removed?
// There's no way to define a counter() function value where the identifiers
// are associated with different tree scopes.Should this comment be added back but to the else statement? Also, out of curiosity, why is this different for the symbols function?
// allow it for backward compatibility. A symbols() value has no name, so it
// never matches 'none' and resolves to its inline style below.nit: I think this comment is ok to remove
// Resolves the counter style this value uses: the inline symbols() style ifBy this value do you mean this content data uses
// cached; nullptr when the list style is a name. Not part of the value.nit: not sure I follow what this means
const cssvalue::CSSSymbolsValue* ListStyleSymbols() const {nit: I'd maybe add an extra line above this to space things out
const cssvalue::CSSSymbolsValue* ListStyleSymbols() const {ListStyleSymbolsFunction. Same below
const cssvalue::CSSSymbolsValue* list_style_symbols = nullptr)list_style_symbols_function. Same elsewhere
#include "base/memory/values_equivalent.h"Is this needed
if (!symbols_counter_style_) {If we invalidate the symbols function with JS, does this get reset?
Probably worth testing that scenario
Also is it possible to add rendering WPT tests for this change and the previous parent CL? Do they already exist somewhere?
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 voted and added 17 comments![]( "Open in Gerrit")
Votes added by Ananya Anand
| Commit-Queue | +0 |
17 comments
nit: It might be helpful to more clearly state the relationship between some of these different classes just for context. i.e. What is ContentData etc and why we store things in these places
I'm having a hard time following all of the various relationships across the various files across between the various CLs
Done
Member<const CSSValue> list_style_; // <counter-style-name> or symbols()Would it make things any simpler to store the name separate from the symbols function at this point
I thought about this. Splitting it into two separate fields would let me drop the casts, but then the code could technically end up in states that should never happen (both fields empty, or both filled at once), so I'd have to add checks to guard against that. I'd also have to remember to update both fields in every place that touches it (ex.Equals, Trace). So it doesn't really make things simpler, it just moves the work around. Whereas the single field gives us "exactly one is always set" for free. That said, happy to split it if you'd prefer.
Member<const CSSValue> list_style_; // <counter-style-name> or symbols()Would it make things any simpler to store the name separate from the symbols function at this point
I thought about this. Splitting it into two separate fields would let me drop the casts, but then the code could technically end up in states that should never happen (both fields empty, or both filled at once), so I'd have to add checks to guard against that. I'd also have to remember to update both fields in every place that touches it (ex. Equals, Trace, etc). So it doesn't really make things simpler, it just moves the work around. The single field gives us "exactly one is always set" for free. That said, happy to split it if you'd prefer.
const CSSSymbolsValue* ListStyleSymbols() const {Ananya AnandListStyleSymbolsFunciton
Done
return To<CSSCustomIdentValue>(*list_style_).Value();Should this also be dymnamicto (in the case that this is accidentally called from a list style symbols)
I feel like dynamicto wouldn't help here since the method returns a reference (a null would just crash at .Value()), and all callers already guard with !ListStyleIsSymbolsFunction() first. Maybe I could add an explciit DCHECK instead?
bool ListStyleIsSymbols() const { return list_style_->IsSymbolsValue(); }Ananya AnandListStyleIsSymbolsFunction
Done
#include "base/memory/values_equivalent.h"Ananya AnandCan this be removed?
Done
// There's no way to define a counter() function value where the identifiers
// are associated with different tree scopes.Should this comment be added back but to the else statement? Also, out of curiosity, why is this different for the symbols function?
Done! Restored it on the else. It's different for symbols function because that's an inline value with no name/tree scope, so the shared-scope DCHECKs only apply to the named counter-style case
// allow it for backward compatibility. A symbols() value has no name, so it
// never matches 'none' and resolves to its inline style below.nit: I think this comment is ok to remove
Done
// Resolves the counter style this value uses: the inline symbols() style ifBy this value do you mean this content data uses
Done
// cached; nullptr when the list style is a name. Not part of the value.nit: not sure I follow what this means
Done
const cssvalue::CSSSymbolsValue* ListStyleSymbols() const {nit: I'd maybe add an extra line above this to space things out
Done
const cssvalue::CSSSymbolsValue* ListStyleSymbols() const {Ananya AnandListStyleSymbolsFunction. Same below
Done
const cssvalue::CSSSymbolsValue* list_style_symbols = nullptr)Ananya Anandlist_style_symbols_function. Same elsewhere
Done
#include "base/memory/values_equivalent.h"Ananya AnandIs this needed
Done
if (!symbols_counter_style_) {If we invalidate the symbols function with JS, does this get reset?
Probably worth testing that scenario
Also is it possible to add rendering WPT tests for this change and the previous parent CL? Do they already exist somewhere?
Yes, it does reset. The cached counter-style is attached to the computed content value, and that value never gets edited in place, so when you change the symbols function from JS the browser just builds a fresh value with an empty cache and throws the old one away. So there's no way for it to go stale. I added a test that proves this (symbols-function-dynamic.html)
btw for rendering tests, they already exist too: symbols-function.html is a reftest that draws both list-style-type: symbols(...) (the CL before this one) and counter()/counters() with symbols()(this CL), and there are invalid-input ones alongside 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 5 comments![]( "Open in Gerrit")
needs_tree_scope_population_ = !identifier->IsScopedValue();Can you add a comment on why we use the identifier in this case? (Also unrelated to your change, but might be worth adding to the comment before CSSCounterContentValue to note what identifier is for extra context)
Member<const CSSValue> list_style_; // <counter-style-name> or symbols()Ananya AnandWould it make things any simpler to store the name separate from the symbols function at this point
I thought about this. Splitting it into two separate fields would let me drop the casts, but then the code could technically end up in states that should never happen (both fields empty, or both filled at once), so I'd have to add checks to guard against that. I'd also have to remember to update both fields in every place that touches it (ex. Equals, Trace, etc). So it doesn't really make things simpler, it just moves the work around. The single field gives us "exactly one is always set" for free. That said, happy to split it if you'd prefer.
Acknowledged
return To<CSSCustomIdentValue>(*list_style_).Value();Ananya AnandShould this also be dymnamicto (in the case that this is accidentally called from a list style symbols)
I feel like dynamicto wouldn't help here since the method returns a reference (a null would just crash at .Value()), and all callers already guard with !ListStyleIsSymbolsFunction() first. Maybe I could add an explciit DCHECK instead?
I guess I would expect to do the same thing as ListStyleSymbolsFunction - so either both be To if we expect this to only be called on that type, or DynamicTo if not.
context.Count(WebFeature::kCSSSymbolsFunction);Same comment here about maybe adding a test for this
// Lazily-built cache of the symbols() anonymous counter style. Mutable and
// excluded from equality/cloning, mirroring
// `ListStyleTypeData::counter_style_`.Similar comment to the other change, do we need to do this lazily, or can we create this up front so we don't need to store list_style_symbols_function above?
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 5 comments![]( "Open in Gerrit")
done, thanks!
needs_tree_scope_population_ = !identifier->IsScopedValue();Can you add a comment on why we use the identifier in this case? (Also unrelated to your change, but might be worth adding to the comment before CSSCounterContentValue to note what identifier is for extra context)
Done
return To<CSSCustomIdentValue>(*list_style_).Value();Ananya AnandShould this also be dymnamicto (in the case that this is accidentally called from a list style symbols)
Alison MaherI feel like dynamicto wouldn't help here since the method returns a reference (a null would just crash at .Value()), and all callers already guard with !ListStyleIsSymbolsFunction() first. Maybe I could add an explciit DCHECK instead?
I guess I would expect to do the same thing as ListStyleSymbolsFunction - so either both be To if we expect this to only be called on that type, or DynamicTo if not.
Done
Same comment here about maybe adding a test for this
Done
// Lazily-built cache of the symbols() anonymous counter style. Mutable and
// excluded from equality/cloning, mirroring
// `ListStyleTypeData::counter_style_`.Similar comment to the other change, do we need to do this lazily, or can we create this up front so we don't need to store list_style_symbols_function above?
- 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 3 comments![]( "Open in Gerrit")
// callers must guard with `ListStyleIsSymbolsFunction()`. The `To<>` cast
// asserts this precondition.nit: this can be removed
// callers must guard with `!ListStyleIsSymbolsFunction()`. The `To<>` cast
// asserts this precondition.nit this can be removed
// Lazily-built cache of the symbols() anonymous counter style. Mutable and
// excluded from equality/cloning, mirroring
// `ListStyleTypeData::counter_style_`.Ananya AnandSimilar comment to the other change, do we need to do this lazily, or can we create this up front so we don't need to store list_style_symbols_function above?
same reasoning from cl 2
Same response here about being able to do the conversion, similar to other value types
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. |