[SVG] Disallow Unitless Lengths in CSS Variable Substitution for `SVGLength` [chromium/src : main]
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
MakeGarbageCollected<CSSUnparsedDeclarationValue>(variable_data);Divyansh MangalThis seems to exploit unwanted behavior (not specifying a parser context yields a strict-mode context). See https://issues.chromium.org/41471328 .
Also, my take-away from the discussion is that this should follow the quirks-mode of the document? I guess that will work out ATM because we always override the parsing mode for the relevant properties? (Which makes me wonder why this happens to work...)
Anders Hartvoll RuudAlso, my take-away from the discussion is that this should follow the quirks-mode of the document? I guess that will work out ATM because we always override the parsing mode for the relevant properties? (Which makes me wonder why this happens to work...)
I see your point and agree with it, but to get QuirksMode in SVGLength class, I would need the document object here, which I don't think we have the access here.
One way I could think is to have a new function `SetValueAsString(string, quirksmode)` and make the caller responsible for passing the quirks mode, but that way I think I will need to make 2-3 similar changes in the call hierarchy?As for why this happens to work, I am not entirely sure, I mainly checked the function https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/properties/css_parsing_utils.cc;drc=98ea873bd650963ffde5b098841b9949b5688823;l=1183 which allows unitless length for `kSVGAttributeMode` and hence I added the svg context parser earlier.
Divyansh MangalThis seems to exploit unwanted behavior.
Indeed.
Also, my take-away from the discussion is that this should follow the quirks-mode of the document?
Does that mean CSS _properties_ should not allow unitless (in standards mode) even when specified literally?
Example: https://jsbin.com/zeyewezeve/edit?html,output
Is `stroke-width: 10` valid here?
Which makes me wonder why this happens to work...
We appear to only test `width` and `height`; it probably doesn't work for properties that override the mode. (We should test them as well.)
One way I could think is to have a new function SetValueAsString(string, quirksmode) and make the caller responsible for passing the quirks mode, but that way I think I will need to make 2-3 similar changes in the call hierarchy?
In that case, maybe you can pass the actual `CSSParserContext` associated with the document to also get proper use-counting (etc). (`document.ElementSheet().Contents()->ParserContext()`)
In that case, maybe you can pass the actual `CSSParserContext` associated with the document to also get proper use-counting (etc). (`document.ElementSheet().Contents()->ParserContext()`)
I tried the idea of passing the actual `CSSParserContext`, though it ended up resulting in lot of plumbing as you can see in the patchset. @f...@opera.com, is this a good way of passing `CSSParserContext` to `SetValueAsString`?
> Does that mean CSS properties should not allow unitless (in standards mode) even when specified literally?
>
> Example: https://jsbin.com/zeyewezeve/edit?html,output
>
> Is stroke-width: 10 valid here?
That's a very valid use case that you suggested @and...@chromium.org, based on the discussion i think it should be disallowed as well, but I can ask this from folks from gecko and webkit, in case I am missing something.
As you mentioned it's rendering in Chromium, because we explicitly override the mode for stroke-width https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc;drc=a340ed8f2a4d05131c7438f608e37fbb64937600;l=9772
Related details
- Anders Hartvoll Ruud
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
// The SVG parser context allows unitless lengths, but CSS variable
// resolution occurs during the cascade phase where unitless lengths
// are forbidden. To maintain consistent CSS behavior and avoid SVG-
// specific quirks, we deliberately omit the parser context.TODO(dmangal): Update the comment based on the solution
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
In that case, maybe you can pass the actual `CSSParserContext` associated with the document to also get proper use-counting (etc). (`document.ElementSheet().Contents()->ParserContext()`)
I tried the idea of passing the actual `CSSParserContext`, though it ended up resulting in lot of plumbing as you can see in the patchset. @f...@opera.com, is this a good way of passing `CSSParserContext` to `SetValueAsString`?
> Does that mean CSS properties should not allow unitless (in standards mode) even when specified literally?
>
> Example: https://jsbin.com/zeyewezeve/edit?html,output
>
> Is stroke-width: 10 valid here?
It shouldn't be, but it currently is (see `Element::EnsureMutableInlineStyle`). This used be issue 40658704, but that was closed in favor of issue 40869667 which was then closed (although I don't think it's actually been fixed - proof in the function referenced above...).
(I'm honestly not sure why we have all these mode overrides everywhere...)
As for forwarding the document's parser context, it would be better to limit it to just `SVGAnimatedLength`.
Related details
- Anders Hartvoll Ruud
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
// The SVG parser context allows unitless lengths, but CSS variable
// resolution occurs during the cascade phase where unitless lengths
// are forbidden. To maintain consistent CSS behavior and avoid SVG-
// specific quirks, we deliberately omit the parser context.TODO(dmangal): Update the comment based on the solution
- Anders Hartvoll Ruud
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
It shouldn't be, but it currently is (see Element::EnsureMutableInlineStyle). This used be issue 40658704, but that was closed in favor of issue 40869667 which was then closed (although I don't think it's actually been fixed - proof in the function referenced above...).
I have started conversation with folks from Gecko regarding stroke-width
https://github.com/web-platform-tests/wpt/pull/56390#issuecomment-3722873859,
I agree we should not allow unitless for stroke-width as well but will probably handle that in a different CL based on the discussion above. Should I still go ahead and create a new Chromium issue for this?
As for forwarding the document's parser context, it would be better to limit it to just SVGAnimatedLength.
Restricted the passing as suggested.
Related details
- Anders Hartvoll Ruud
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
Member<const CSSParserContext> parser_context_;We shouldn't store this. This costs memory for all `SVGLength`.
Related details
- Anders Hartvoll Ruud
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
We should add a test for the quirks-mode case.
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
Member<const CSSParserContext> parser_context_;We shouldn't store this. This costs memory for all `SVGLength`.
I removed the variable but to pass the Parser Context while restricting to `SVGAnimatedLength` I have to duplicate the code from `UpdateBaseValueFromAttribute` into `SVGAnimatedLength::AttributeChanged` and change it accordingly. I am not very confident about this approach given this TODO there but let me know your thoughts on this.
```
// TODO: Use UpdateBaseValueFromAttribute() when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
- Anders Hartvoll Ruud
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 7 comments![]( "Open in Gerrit")
enum ContentAttributeState : unsigned {
// The content attribute is not set (hasAttribute(...) === false).
kNotSet,
// The content attribute is set (hasAttribute(...) === true).
kHasValue,
// The SVG DOM base value has been changed and is waiting to be
// synchronized to content attribute storage.
kUnsynchronizedValue,
// The SVG DOM base value has been changed such that the content attribute
// would be removed, and is waiting to be synchronized to content attribute
// storage.
kUnsynchronizedRemoval,
};
void SetContentAttributeState(ContentAttributeState content_attribute_state) {
content_attribute_state_ = content_attribute_state;
}I think we could make these protected instead of adding a new function.
void SetAttributeStateFromValue(const String& value);Can be protected. However, see below.
ContextElement() ? ContextElement()->GetParserContext() : nullptr);The context element can't be null here.
return GetDocument().ElementSheet().Contents()->ParserContext();Let's just use this directly in the one spot where it is used.
Member<const CSSParserContext> parser_context_;Divyansh MangalWe shouldn't store this. This costs memory for all `SVGLength`.
I removed the variable but to pass the Parser Context while restricting to `SVGAnimatedLength` I have to duplicate the code from `UpdateBaseValueFromAttribute` into `SVGAnimatedLength::AttributeChanged` and change it accordingly. I am not very confident about this approach given this TODO there but let me know your thoughts on this.
```
// TODO: Use UpdateBaseValueFromAttribute() when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
You could probably also add the ability of passing a parameter-pack to `UpdateBaseValueFromAttribute`.
<!DOCTYPE quirks-mode>Just drop this, and make sure to note it's testing quirks mode.
<title>Usage of css var() on circle radius attribute (units not specified and document in quirks mode)</title>I think the parenthesis can be dropped.
Related details
- Anders Hartvoll Ruud
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Divyansh Mangal (Gerrit)
Divyansh Mangal added 6 comments![]( "Open in Gerrit")
enum ContentAttributeState : unsigned {
// The content attribute is not set (hasAttribute(...) === false).
kNotSet,
// The content attribute is set (hasAttribute(...) === true).
kHasValue,
// The SVG DOM base value has been changed and is waiting to be
// synchronized to content attribute storage.
kUnsynchronizedValue,
// The SVG DOM base value has been changed such that the content attribute
// would be removed, and is waiting to be synchronized to content attribute
// storage.
kUnsynchronizedRemoval,
};
void SetContentAttributeState(ContentAttributeState content_attribute_state) {
content_attribute_state_ = content_attribute_state;
}I think we could make these protected instead of adding a new function.
Done
void SetAttributeStateFromValue(const String& value);Can be protected. However, see below.
Done
ContextElement() ? ContextElement()->GetParserContext() : nullptr);The context element can't be null here.
Done
return GetDocument().ElementSheet().Contents()->ParserContext();Let's just use this directly in the one spot where it is used.
Done
Just drop this, and make sure to note it's testing quirks mode.
Dropped this and I think title already contain the info for the quirks mode.
<title>Usage of css var() on circle radius attribute (units not specified and document in quirks mode)</title>I think the parenthesis can be dropped.
- Anders Hartvoll Ruud
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Member<const CSSParserContext> parser_context_;Divyansh MangalWe shouldn't store this. This costs memory for all `SVGLength`.
Fredrik SöderquistI removed the variable but to pass the Parser Context while restricting to `SVGAnimatedLength` I have to duplicate the code from `UpdateBaseValueFromAttribute` into `SVGAnimatedLength::AttributeChanged` and change it accordingly. I am not very confident about this approach given this TODO there but let me know your thoughts on this.
```
// TODO: Use UpdateBaseValueFromAttribute() when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
You could probably also add the ability of passing a parameter-pack to `UpdateBaseValueFromAttribute`.
The parameter-pack suggestion did indeed help in reducing the duplicity, Do you think we should update the TODO as well because I am directly using `UpdateBaseValueFromAttribute` here? New TODO can be like
```
// TODO: Use correct validator when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 3 comments![]( "Open in Gerrit")
enum ContentAttributeState : unsigned {
// The content attribute is not set (hasAttribute(...) === false).
kNotSet,
// The content attribute is set (hasAttribute(...) === true).
kHasValue,
// The SVG DOM base value has been changed and is waiting to be
// synchronized to content attribute storage.
kUnsynchronizedValue,
// The SVG DOM base value has been changed such that the content attribute
// would be removed, and is waiting to be synchronized to content attribute
// storage.
kUnsynchronizedRemoval,
};
void SetContentAttributeState(ContentAttributeState content_attribute_state) {
content_attribute_state_ = content_attribute_state;
}No need to move this now that we've updated `UpdateBaseValueFromAttribute()` instead.
Member<const CSSParserContext> parser_context_;Divyansh MangalWe shouldn't store this. This costs memory for all `SVGLength`.
Fredrik SöderquistI removed the variable but to pass the Parser Context while restricting to `SVGAnimatedLength` I have to duplicate the code from `UpdateBaseValueFromAttribute` into `SVGAnimatedLength::AttributeChanged` and change it accordingly. I am not very confident about this approach given this TODO there but let me know your thoughts on this.
```
// TODO: Use UpdateBaseValueFromAttribute() when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
Divyansh MangalYou could probably also add the ability of passing a parameter-pack to `UpdateBaseValueFromAttribute`.
The parameter-pack suggestion did indeed help in reducing the duplicity, Do you think we should update the TODO as well because I am directly using `UpdateBaseValueFromAttribute` here? New TODO can be like
```
// TODO: Use correct validator when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
Yes, that sounds like a good update.
MakeGarbageCollected<CSSUnparsedDeclarationValue>(variable_data);- Anders Hartvoll Ruud
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
enum ContentAttributeState : unsigned {
// The content attribute is not set (hasAttribute(...) === false).
kNotSet,
// The content attribute is set (hasAttribute(...) === true).
kHasValue,
// The SVG DOM base value has been changed and is waiting to be
// synchronized to content attribute storage.
kUnsynchronizedValue,
// The SVG DOM base value has been changed such that the content attribute
// would be removed, and is waiting to be synchronized to content attribute
// storage.
kUnsynchronizedRemoval,
};
void SetContentAttributeState(ContentAttributeState content_attribute_state) {
content_attribute_state_ = content_attribute_state;
}No need to move this now that we've updated `UpdateBaseValueFromAttribute()` instead.
Done
Member<const CSSParserContext> parser_context_;Divyansh MangalWe shouldn't store this. This costs memory for all `SVGLength`.
Fredrik SöderquistI removed the variable but to pass the Parser Context while restricting to `SVGAnimatedLength` I have to duplicate the code from `UpdateBaseValueFromAttribute` into `SVGAnimatedLength::AttributeChanged` and change it accordingly. I am not very confident about this approach given this TODO there but let me know your thoughts on this.
```
// TODO: Use UpdateBaseValueFromAttribute() when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
Divyansh MangalYou could probably also add the ability of passing a parameter-pack to `UpdateBaseValueFromAttribute`.
Fredrik SöderquistThe parameter-pack suggestion did indeed help in reducing the duplicity, Do you think we should update the TODO as well because I am directly using `UpdateBaseValueFromAttribute` here? New TODO can be like
```
// TODO: Use correct validator when we can set proper initial
// values on error (for example 'auto' for 'rx' and 'ry').
```
Yes, that sounds like a good update.
- Anders Hartvoll Ruud
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted Code-Review+1![]( "Open in Gerrit")
- Anders Hartvoll Ruud
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57125.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process