[SVG][Media Fragments] Parse spatial media fragments in SVGImage [chromium/src : main]
Divyansh Mangal (Gerrit)
Message from Divyansh Mangal![]( "Open in Gerrit")
Set Ready For Review
Related details
- Divyansh Mangal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Message from Divyansh Mangal![]( "Open in Gerrit")
Set Ready For Review
Related details
- Divyansh Mangal
- 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. |
Vinay Singh (Gerrit)
Vinay Singh added 7 comments![]( "Open in Gerrit")
bool is_valid_ = true;Why would `MediaFragmentURIParser` have a field `is_valid_`?
It is sort of giving an impression as if the parser can be valid or invalid.
`is_url_valid_` probably might be a better choice.
gfx::Rect GetRect() const { return rect; }`rect` is public already. Either make it private or remove this function and access directly.
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
MediaFragmentURIParser parser(StringView("t=5,10"));
EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}```suggestion
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
MediaFragmentURIParser parser(StringView("t=5,10"));
EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesSpatial) {
MediaFragmentURIParser parser(StringView("xywh=10,20,30,40"));
SpatialClip clip = parser.SpatialFragment();
EXPECT_TRUE(clip.IsValid());
EXPECT_EQ(clip.rect, gfx::Rect(10, 20, 30, 40));
EXPECT_EQ(clip.unit, SpatialClip::Unit::kPixel);
}
```We are not testing the primary functionality of this CL. This additional test should be helpful.
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {``&& fragment.Contains("xywh")`` or something similar??
I guess this will make for an early return sparing initialization of MediaFragmentURIParser for all the non-`xywh` cases which I'm assuming must be in majority.
gfx::SizeF svg_size(SizeWithConfig(SizeConfig()));Consider `SizeWithConfigAsFloat`
SizeWithConfig truncates the fraction as it returns back a RoundedSize which can be a bit premature at this point and may result into pixel errors.
background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");Same here.
Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.
background-image: url("reference/green-100x100.svg#xywh=percent:0,0,100,100");Wouldn't this produce the same 100x100 green image regardless of the fragment?
Please correct me if I'm wrong.
Related details
- Divyansh Mangal
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Divyansh Mangal (Gerrit)
Divyansh Mangal added 4 comments![]( "Open in Gerrit")
Why would `MediaFragmentURIParser` have a field `is_valid_`?
It is sort of giving an impression as if the parser can be valid or invalid.`is_url_valid_` probably might be a better choice.
Done
`rect` is public already. Either make it private or remove this function and access directly.
Removed the function
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
MediaFragmentURIParser parser(StringView("t=5,10"));
EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}```suggestion
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
MediaFragmentURIParser parser(StringView("t=5,10"));
EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}TEST(MediaFragmentURIParserTest, FragmentConstructorParsesSpatial) {
MediaFragmentURIParser parser(StringView("xywh=10,20,30,40"));
SpatialClip clip = parser.SpatialFragment();
EXPECT_TRUE(clip.IsValid());
EXPECT_EQ(clip.rect, gfx::Rect(10, 20, 30, 40));
EXPECT_EQ(clip.unit, SpatialClip::Unit::kPixel);
}
```We are not testing the primary functionality of this CL. This additional test should be helpful.
Done
Consider `SizeWithConfigAsFloat`
SizeWithConfig truncates the fraction as it returns back a RoundedSize which can be a bit premature at this point and may result into pixel errors.
- 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 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {``&& fragment.Contains("xywh")`` or something similar??
I guess this will make for an early return sparing initialization of MediaFragmentURIParser for all the non-`xywh` cases which I'm assuming must be in majority.
`ParseFragments` in `MediaFragmentURIParser` handles this much more gracefully, so such an early out is not needed in my opinion. (Also, we would have to decode the fragment first too before applying such checks)
background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");Same here.
Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.
Thats the expectation, we actually want to see the entire image itself, even if we give out of bounds spatial fragment per spec.
background-image: url("reference/green-100x100.svg#xywh=percent:0,0,100,100");Wouldn't this produce the same 100x100 green image regardless of the fragment?
Please correct me if I'm wrong.
Yes, but what we want to check here is that the entire image is indeed getting correctly displayed with 100% of the bounds, so that's the expectation.
Virali Purbey (Gerrit)
Virali Purbey added 12 comments![]( "Open in Gerrit")
Please consider adding WPTs for:
1. SVG without explicit width/height (viewBox-only). We are explicitly handling the `svg_size.IsEmpty()` case, we should test it.
2. `<img>` element with percent fragment: Currently only pixel-mode is tested via <img>.
3. Non-square SVG with percent clip: verifies the x/y scaling applies independently.
bool is_url_valid_ = true;The name `is_url_valid_` is misleading for the StringView constructor path, where there's no URL involved at all, true just means "parsing should proceed." Consider renaming to `is_valid_` or `should_parse_` to avoid confusion.
// TODO(dmangal): Move MediaFragmentURIParser to a shared location since it is
// now used by both HTML media and SVG.Can you file a separate tracking bug for this layering issue? Now that SVG depends on a parser in `//core/html/media/`, it's a real cross-component dependency that should be tracked explicitly rather than relying on a TODO comment.
}Good, these verify the new StringView constructor.
Can we consider also adding a test that the StringView constructor correctly handles an empty string (to match the `InvalidUrlYieldsNaN` semantics for the `KURL` constructor).
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {This block adds ~55 lines to `CreateViewInfo`. Can we consider extracting the media-fragment logic into a private helper (e.g., `CreateViewInfoFromSpatialFragment(const String& fragment))` to keep the two code paths easy to follow.
const SpatialClip spatial_clip = media_parser.SpatialFragment();`SpatialClip::rect` is `gfx::Rect (integer)`, so by this point any fractional values from the fragment string have already been truncated. The conversion to `gfx::RectF` on the next line and the subsequent `Scale()` for percent mode operate on pre-truncated ints. For pixel mode this is fine, but for percent mode with a non-power-of-100 SVG size, the early truncation can produce off-by-one results. Can we consider changing `SpatialClip::rect` to `gfx::RectF`?
/*target*/ nullptr);nit: Per Chromium style, named parameter comments should use =: `/*target=*/nullptr` (no space before nullptr).
if (!clip_rect.IsEmpty()) {
spatial_view_spec = SVGViewSpec::CreateFromSpatialFragment(
MakeGarbageCollected<SVGRect>(clip_rect.x(), clip_rect.y(),
clip_rect.width(),
clip_rect.height()));
}
}
if (spatial_view_spec) {
return MakeGarbageCollected<SVGImageViewInfo>(spatial_view_spec,
/*target*/ nullptr);
}What happens when the URL contains both `xywh=...` and `svgView(viewBox(...))`? Currently the spatial fragment takes absolute priority and the `svgView()` is silently ignored. Is this intentional? The SVG 2 spec doesn't clearly define this interaction.
Please add a comment documenting the intended precedence and ideally add a test for the combined case `(e.g., #svgView(viewBox(0,0,50,50))&xywh=10,10,20,20)`.
const SVGViewSpec* SVGViewSpec::CreateFromSpatialFragment(const SVGRect* rect) {
if (!rect) {
return nullptr;
}
SVGViewSpec* view_spec = MakeGarbageCollected<SVGViewSpec>();
view_spec->view_box_ = rect;
return view_spec;
}This sets `view_box_` but leaves `preserve_aspect_ratio_` as nullptr. When `PreserveAspectRatio()` returns null, the SVG element's own `preserveAspectRatio` attribute is used. If the source SVG has `preserveAspectRatio="none"`, a non-square spatial clip (e.g., `xywh=0,0,100,50`) rendered into a square CSS box will get stretched, which is unlikely to be what we would expect.
Consider explicitly setting `preserveAspectRatio` to `xMidYMid meet` (the SVG default) on the view spec to ensure consistent behavior regardless of what the SVG element declares.
background-image: url("reference/green-100x100.svg#xywh=0.0,0.0,100.0,100.0");This doesn't actually test decimal value handling, it's again a full-extent clip on a solid-green SVG. To test that decimal values parse correctly, use a case where truncation/rounding would produce a visibly different result, e.g., a clip that selects a sub-region of the two-tone SVG.
Also note: the parser uses `gfx::Rect` (integers), so 0.0 is truncated to 0, this test won't catch regressions if decimal handling changes. Perhaps worth noting this limitation in the test title.
background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");Divyansh MangalSame here.
Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.
Thats the expectation, we actually want to see the entire image itself, even if we give out of bounds spatial fragment per spec.
Acknowledged, showing the full image is the correct expected result here.
The concern is discriminating power: this test produces the same green `100×100` output whether the fragment is parsed and clamped correctly, completely ignored, or fails to parse entirely. So it can't catch a regression in the clamping codepath.
Could you add a companion test with a non-zero origin? e.g., `red-green-200x200.svg#xywh=50,50,300,300` → clamped to `50,50,150,150`. That way the clamped clip selects a visible sub-region that differs from "no fragment," and the test actually exercises the clamping logic. This test can stay as-is for spec-behavior documentation.
background-image: url("reference/green-100x100.svg#xywh=percent:0,0,100,100");Divyansh MangalWouldn't this produce the same 100x100 green image regardless of the fragment?
Please correct me if I'm wrong.
Yes, but what we want to check here is that the entire image is indeed getting correctly displayed with 100% of the bounds, so that's the expectation.
Same concern as the `overflow-clamped test` - `percent:0,0,100,100` resolves to the full image, so this passes identically with or without fragment support. I note that `spatial-percent.html` already meaningfully tests a sub-region percent clip, so coverage exists. Still, consider either making this test non-trivial (e.g., a sub-region on the two-tone SVG), or adding a comment in the test noting it intentionally verifies that a full-extent percent clip renders identically to no fragment.
Related details
- Divyansh Mangal
- Vinay Singh
Virali Purbey (Gerrit)
Virali Purbey added 1 comment![]( "Open in Gerrit")
Bug: 40362942, 495475010Can you please verify why the `mac-rel` policy is failing and if there is anything missing?
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
Can you please verify why the `mac-rel` policy is failing and if there is anything missing?
Just an infra, resolved in the next run by itself, not an all related to the CL.
bool is_url_valid_ = true;The name `is_url_valid_` is misleading for the StringView constructor path, where there's no URL involved at all, true just means "parsing should proceed." Consider renaming to `is_valid_` or `should_parse_` to avoid confusion.
Please see the reasoning for this name per comment https://chromium-review.googlesource.com/c/chromium/src/+/7675457/comment/30a18874_ce599c87/
I am ok with `should_parse_`, but maybe need more inputs from an owner as well before we change it again
// TODO(dmangal): Move MediaFragmentURIParser to a shared location since it is
// now used by both HTML media and SVG.Can you file a separate tracking bug for this layering issue? Now that SVG depends on a parser in `//core/html/media/`, it's a real cross-component dependency that should be tracked explicitly rather than relying on a TODO comment.
Done
Good, these verify the new StringView constructor.
Can we consider also adding a test that the StringView constructor correctly handles an empty string (to match the `InvalidUrlYieldsNaN` semantics for the `KURL` constructor).
Done
nit: Per Chromium style, named parameter comments should use =: `/*target=*/nullptr` (no space before nullptr).
- 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 3 comments![]( "Open in Gerrit")
if (!clip_rect.IsEmpty()) {
spatial_view_spec = SVGViewSpec::CreateFromSpatialFragment(
MakeGarbageCollected<SVGRect>(clip_rect.x(), clip_rect.y(),
clip_rect.width(),
clip_rect.height()));
}
}
if (spatial_view_spec) {
return MakeGarbageCollected<SVGImageViewInfo>(spatial_view_spec,
/*target*/ nullptr);
}What happens when the URL contains both `xywh=...` and `svgView(viewBox(...))`? Currently the spatial fragment takes absolute priority and the `svgView()` is silently ignored. Is this intentional? The SVG 2 spec doesn't clearly define this interaction.
Please add a comment documenting the intended precedence and ideally add a test for the combined case `(e.g., #svgView(viewBox(0,0,50,50))&xywh=10,10,20,20)`.
Thanks for the question Virali, seems like the interaction between various fragments is indeed listed in the spec which I earlier missed completely.
https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions
For our purpose this statement will apply:
_If the SVG fragment identifier addresses a combination of the above non-time related identifiers with a time-related identifier (i.e. a timesegment), the behavior of each identifier is applied._
if i am reading this correctly it means each fragments type effect will apply, which makes for pretty interesting WPTs as well. I will add those and the one that you suggested.
const SVGViewSpec* SVGViewSpec::CreateFromSpatialFragment(const SVGRect* rect) {
if (!rect) {
return nullptr;
}
SVGViewSpec* view_spec = MakeGarbageCollected<SVGViewSpec>();
view_spec->view_box_ = rect;
return view_spec;
}This sets `view_box_` but leaves `preserve_aspect_ratio_` as nullptr. When `PreserveAspectRatio()` returns null, the SVG element's own `preserveAspectRatio` attribute is used. If the source SVG has `preserveAspectRatio="none"`, a non-square spatial clip (e.g., `xywh=0,0,100,50`) rendered into a square CSS box will get stretched, which is unlikely to be what we would expect.
Consider explicitly setting `preserveAspectRatio` to `xMidYMid meet` (the SVG default) on the view spec to ensure consistent behavior regardless of what the SVG element declares.
Per the SVG spec's fragment identifier definitions https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions, only explicitly specified view attributes override the root element's attributes.
The existing `svgView(viewBox(...))` and `<view>` paths follow this same pattern, when `preserveAspectRatio` isn't specified in the fragment, the element's own `preserveAspectRatio` is used (`CreateFromFragment` and `CreateForViewElement` both leave `preserve_aspect_ratio_` null when unspecified).
forcing `xMidYMid meet` would make spatial fragments behave differently.
Though I should definitely add some tests with `preserveAspectRatio` specified which will give us more coverage.
background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");Divyansh MangalSame here.
Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.
Virali PurbeyThats the expectation, we actually want to see the entire image itself, even if we give out of bounds spatial fragment per spec.
Acknowledged, showing the full image is the correct expected result here.
The concern is discriminating power: this test produces the same green `100×100` output whether the fragment is parsed and clamped correctly, completely ignored, or fails to parse entirely. So it can't catch a regression in the clamping codepath.
Could you add a companion test with a non-zero origin? e.g., `red-green-200x200.svg#xywh=50,50,300,300` → clamped to `50,50,150,150`. That way the clamped clip selects a visible sub-region that differs from "no fragment," and the test actually exercises the clamping logic. This test can stay as-is for spec-behavior documentation.
Just a thing that I noticed, if you try the same svg on Firefox they won't show 100x100 image, most likely because they dont do the clamping as suggested in the spec (atleast that's what I could gather from their code https://searchfox.org/firefox-main/source/dom/svg/SVGFragmentIdentifier.cpp#166) so this is a good test to have regardless.
Though I will also the test which Virali suggested it will also increase coverage.
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 6 comments![]( "Open in Gerrit")
It's non-obvious to me how this should interact with sizing, but it will probably need to override the natural dimensions? The CSS images spec[1] says "clipped". (This mechanism would in theory be the same for raster images.)
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {Fredrik SöderquistThis block adds ~55 lines to `CreateViewInfo`. Can we consider extracting the media-fragment logic into a private helper (e.g., `CreateViewInfoFromSpatialFragment(const String& fragment))` to keep the two code paths easy to follow.
This sounds like a good idea given that the added code is quite a bit larger than the existing code.
const SVGViewSpec* SVGViewSpec::CreateFromSpatialFragment(const SVGRect* rect) {
if (!rect) {
return nullptr;
}
SVGViewSpec* view_spec = MakeGarbageCollected<SVGViewSpec>();
view_spec->view_box_ = rect;
return view_spec;
}Divyansh MangalThis sets `view_box_` but leaves `preserve_aspect_ratio_` as nullptr. When `PreserveAspectRatio()` returns null, the SVG element's own `preserveAspectRatio` attribute is used. If the source SVG has `preserveAspectRatio="none"`, a non-square spatial clip (e.g., `xywh=0,0,100,50`) rendered into a square CSS box will get stretched, which is unlikely to be what we would expect.
Consider explicitly setting `preserveAspectRatio` to `xMidYMid meet` (the SVG default) on the view spec to ensure consistent behavior regardless of what the SVG element declares.
Per the SVG spec's fragment identifier definitions https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions, only explicitly specified view attributes override the root element's attributes.
The existing `svgView(viewBox(...))` and `<view>` paths follow this same pattern, when `preserveAspectRatio` isn't specified in the fragment, the element's own `preserveAspectRatio` is used (`CreateFromFragment` and `CreateForViewElement` both leave `preserve_aspect_ratio_` null when unspecified).forcing `xMidYMid meet` would make spatial fragments behave differently.
Though I should definitely add some tests with `preserveAspectRatio` specified which will give us more coverage.
Why would a spatial fragment apply to (override) the `viewBox` though? That's not how I'm reading the spec (the little of it there is). The way I'm reading it, it should rather act as a clip/subregion into the image. (Overriding the `viewBox' may affect the viewport, natural aspect ratio, etc.)
<link rel="help" href="https://www.w3.org/TR/media-frags/#naming-space">Should probably also reference the section in the SVG spec. (Here and all the cases below.)
background-image: url("reference/green-100x100.svg#xywh=foo:0,0,50,50");How will you be able to tell if this fails?
<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">Don't do this. If you need a fixed-size version, then add one. (Also it looks like what you want is really a `support/` file.)
Related details
- Divyansh Mangal
- 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 6 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {Divyansh Mangal``&& fragment.Contains("xywh")`` or something similar??
I guess this will make for an early return sparing initialization of MediaFragmentURIParser for all the non-`xywh` cases which I'm assuming must be in majority.
`ParseFragments` in `MediaFragmentURIParser` handles this much more gracefully, so such an early out is not needed in my opinion. (Also, we would have to decode the fragment first too before applying such checks)
Done
if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {Fredrik SöderquistThis block adds ~55 lines to `CreateViewInfo`. Can we consider extracting the media-fragment logic into a private helper (e.g., `CreateViewInfoFromSpatialFragment(const String& fragment))` to keep the two code paths easy to follow.
This sounds like a good idea given that the added code is quite a bit larger than the existing code.
Done
<link rel="help" href="https://www.w3.org/TR/media-frags/#naming-space">Should probably also reference the section in the SVG spec. (Here and all the cases below.)
Done
background-image: url("reference/green-100x100.svg#xywh=0.0,0.0,100.0,100.0");This doesn't actually test decimal value handling, it's again a full-extent clip on a solid-green SVG. To test that decimal values parse correctly, use a case where truncation/rounding would produce a visibly different result, e.g., a clip that selects a sub-region of the two-tone SVG.
Also note: the parser uses `gfx::Rect` (integers), so 0.0 is truncated to 0, this test won't catch regressions if decimal handling changes. Perhaps worth noting this limitation in the test title.
Changed the test, since decimal values are invalid I have given a different width and height in decimal values.
background-image: url("reference/green-100x100.svg#xywh=percent:0,0,100,100");Wouldn't this produce the same 100x100 green image regardless of the fragment?
Please correct me if I'm wrong.
Divyansh Mangal
Yes, but what we want to check here is that the entire image is indeed getting correctly displayed with 100% of the bounds, so that's the expectation.
Same concern as the `overflow-clamped test` - `percent:0,0,100,100` resolves to the full image, so this passes identically with or without fragment support. I note that `spatial-percent.html` already meaningfully tests a sub-region percent clip, so coverage exists. Still, consider either making this test non-trivial (e.g., a sub-region on the two-tone SVG), or adding a comment in the test noting it intentionally verifies that a full-extent percent clip renders identically to no fragment.
Added the comment as suggested
<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">Don't do this. If you need a fixed-size version, then add one. (Also it looks like what you want is really a `support/` file.)
- 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 2 comments![]( "Open in Gerrit")
const SpatialClip spatial_clip = media_parser.SpatialFragment();`SpatialClip::rect` is `gfx::Rect (integer)`, so by this point any fractional values from the fragment string have already been truncated. The conversion to `gfx::RectF` on the next line and the subsequent `Scale()` for percent mode operate on pre-truncated ints. For pixel mode this is fine, but for percent mode with a non-power-of-100 SVG size, the early truncation can produce off-by-one results. Can we consider changing `SpatialClip::rect` to `gfx::RectF`?
I think `gfx::Rect` is the correct choice here as the spec does not allow decimal values. The conversion from `gfx::Rect` to `gfx::RectF` before scaling is lossless. No precision is lost. But changing to `gfx::RectF` would be a harmless future-proofing if the spec ever adds decimal support, IMO it's just not strictly necessary today. (I can still update if its strongly preferred in another CL).
if (!clip_rect.IsEmpty()) {
spatial_view_spec = SVGViewSpec::CreateFromSpatialFragment(
MakeGarbageCollected<SVGRect>(clip_rect.x(), clip_rect.y(),
clip_rect.width(),
clip_rect.height()));
}
}
if (spatial_view_spec) {
return MakeGarbageCollected<SVGImageViewInfo>(spatial_view_spec,
/*target*/ nullptr);
}Divyansh MangalWhat happens when the URL contains both `xywh=...` and `svgView(viewBox(...))`? Currently the spatial fragment takes absolute priority and the `svgView()` is silently ignored. Is this intentional? The SVG 2 spec doesn't clearly define this interaction.
Please add a comment documenting the intended precedence and ideally add a test for the combined case `(e.g., #svgView(viewBox(0,0,50,50))&xywh=10,10,20,20)`.
Thanks for the question Virali, seems like the interaction between various fragments is indeed listed in the spec which I earlier missed completely.
https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions
For our purpose this statement will apply:
_If the SVG fragment identifier addresses a combination of the above non-time related identifiers with a time-related identifier (i.e. a timesegment), the behavior of each identifier is applied._
if i am reading this correctly it means each fragments type effect will apply, which makes for pretty interesting WPTs as well. I will add those and the one that you suggested.
A little correction in my above statement, the above "interesting cases" are only applicable for when time segment is involved. For spatial segment fortunately we dont have such interaction.
```
SVGFragmentIdentifier ::= BareName *( "&" timesegment ) |
SVGViewSpec *( "&" timesegment ) |
spacesegment *( "&" timesegment ) |
timesegment *( "&" spacesegment )
```
In my opinion, giving anything apart from the above allowed values should be discarded, but that certainly needs clarification. Will open a SVG spec issue for this.
Related details
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");Same here.
Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.
Divyansh Mangal
Thats the expectation, we actually want to see the entire image itself, even if we give out of bounds spatial fragment per spec.
Divyansh MangalAcknowledged, showing the full image is the correct expected result here.
The concern is discriminating power: this test produces the same green `100×100` output whether the fragment is parsed and clamped correctly, completely ignored, or fails to parse entirely. So it can't catch a regression in the clamping codepath.
Could you add a companion test with a non-zero origin? e.g., `red-green-200x200.svg#xywh=50,50,300,300` → clamped to `50,50,150,150`. That way the clamped clip selects a visible sub-region that differs from "no fragment," and the test actually exercises the clamping logic. This test can stay as-is for spec-behavior documentation.
Just a thing that I noticed, if you try the same svg on Firefox they won't show 100x100 image, most likely because they dont do the clamping as suggested in the spec (atleast that's what I could gather from their code https://searchfox.org/firefox-main/source/dom/svg/SVGFragmentIdentifier.cpp#166) so this is a good test to have regardless.
Though I will also the test which Virali suggested it will also increase coverage.
Added the test as suggested in the earlier comment.
background-image: url("reference/green-100x100.svg#xywh=foo:0,0,50,50");How will you be able to tell if this fails?
Thats a good question, i think I added the test just to cover the syntax errors. But on thinking a little more such a test makes more sense in the unit test for the parser which it already has. Deleted this test, as a WPT it has little value.
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Please consider adding WPTs for:
1. SVG without explicit width/height (viewBox-only). We are explicitly handling the `svg_size.IsEmpty()` case, we should test it.
2. `<img>` element with percent fragment: Currently only pixel-mode is tested via <img>.
3. Non-square SVG with percent clip: verifies the x/y scaling applies independently.
Added the tests as suggested pls check.
Related details
- 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)
Message from Divyansh Mangal![]( "Open in Gerrit")
- Divyansh Mangal
- 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 13 comments![]( "Open in Gerrit")
bool is_url_valid_ = true;Divyansh MangalThe name `is_url_valid_` is misleading for the StringView constructor path, where there's no URL involved at all, true just means "parsing should proceed." Consider renaming to `is_valid_` or `should_parse_` to avoid confusion.
Please see the reasoning for this name per comment https://chromium-review.googlesource.com/c/chromium/src/+/7675457/comment/30a18874_ce599c87/
I am ok with `should_parse_`, but maybe need more inputs from an owner as well before we change it again
You could just let a null `String` represent the "invalid" state, and then just do `if (!fragment_) ...`. (Could also use `.empty()`, since empty strings will never contain anything interesting.)
MediaFragmentURIParser::MediaFragmentURIParser(const KURL& url)This constructor could delegate to the above one.
const SpatialClip spatial_clip = media_parser.SpatialFragment();Divyansh Mangal`SpatialClip::rect` is `gfx::Rect (integer)`, so by this point any fractional values from the fragment string have already been truncated. The conversion to `gfx::RectF` on the next line and the subsequent `Scale()` for percent mode operate on pre-truncated ints. For pixel mode this is fine, but for percent mode with a non-power-of-100 SVG size, the early truncation can produce off-by-one results. Can we consider changing `SpatialClip::rect` to `gfx::RectF`?
I think `gfx::Rect` is the correct choice here as the spec does not allow decimal values. The conversion from `gfx::Rect` to `gfx::RectF` before scaling is lossless. No precision is lost. But changing to `gfx::RectF` would be a harmless future-proofing if the spec ever adds decimal support, IMO it's just not strictly necessary today. (I can still update if its strongly preferred in another CL).
A conversion from `gfx::Rect` to `gfx::RectF` are not necessarily lossless (a float cannot exactly represent all `int` values). (But I agree with the choice of type because the spec only allows integers from what I can tell.)
const SVGImageViewInfo* SVGImage::CreateViewInfoFromSpatialFragment(It looks like this could live in `SVGViewSpec`, and just have appropriate information passed to it. I think that would make the interface look better, now the division of responsibilities are a bit weird.
gfx::SizeF svg_size(SizeWithConfigAsFloat(SizeConfig()));
if (spatial_clip.unit == SpatialClip::Unit::kPercent) {
// Convert percent to pixel values based on the SVG's intrinsic size.
// TODO(crbug.com/979895): When the SVG has no explicit width/height
// (or they are percentages), `IntrinsicWidth()`/`IntrinsicHeight()`
// return nullopt, causing `intrinsic_size_` to be 0x0. This makes
// percent-based spatial fragments for such cases produce a zero-size
// clip.
clip_rect.Scale(svg_size.width() / 100.0f, svg_size.height() / 100.0f);It's unclear to me how this should work when there's no natural size. (In general, using `SVGImage`s `Size*()` accessors is an indication of any issue.)
// Only clamp when intrinsic dimensions are known (non-zero); SVGs
// without explicit width/height report 0x0 (see TODO above).Ditto here I guess. It would probably be good to be more specific here. (The natural width/height can be zero as well, and "no width/height" can be different from "width/height is zero".
const SVGViewSpec* SVGViewSpec::CreateFromSpatialFragment(const SVGRect* rect) {
if (!rect) {
return nullptr;
}
SVGViewSpec* view_spec = MakeGarbageCollected<SVGViewSpec>();
view_spec->view_box_ = rect;
return view_spec;
}Divyansh MangalThis sets `view_box_` but leaves `preserve_aspect_ratio_` as nullptr. When `PreserveAspectRatio()` returns null, the SVG element's own `preserveAspectRatio` attribute is used. If the source SVG has `preserveAspectRatio="none"`, a non-square spatial clip (e.g., `xywh=0,0,100,50`) rendered into a square CSS box will get stretched, which is unlikely to be what we would expect.
Consider explicitly setting `preserveAspectRatio` to `xMidYMid meet` (the SVG default) on the view spec to ensure consistent behavior regardless of what the SVG element declares.
Fredrik SöderquistPer the SVG spec's fragment identifier definitions https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions, only explicitly specified view attributes override the root element's attributes.
The existing `svgView(viewBox(...))` and `<view>` paths follow this same pattern, when `preserveAspectRatio` isn't specified in the fragment, the element's own `preserveAspectRatio` is used (`CreateFromFragment` and `CreateForViewElement` both leave `preserve_aspect_ratio_` null when unspecified).forcing `xMidYMid meet` would make spatial fragments behave differently.
Though I should definitely add some tests with `preserveAspectRatio` specified which will give us more coverage.
Why would a spatial fragment apply to (override) the `viewBox` though? That's not how I'm reading the spec (the little of it there is). The way I'm reading it, it should rather act as a clip/subregion into the image. (Overriding the `viewBox' may affect the viewport, natural aspect ratio, etc.)
So, we've cleared that bit up - and it should affect the `viewBox`. There's no way to override `preserveAspectRatio`, so one needs to be cognizant of that. (This is not different from the same `svgView` case.)
background-image: url("support/green-100x100.svg#xywh=0.0,0.0,50.0,50.0");If an implementation parses this and either ignores or accepts the fractions (giving 0,0,50,50), then this will still appear to pass. Maybe adjust the parameter values to prevent that or use a different image?
background-image: url("support/green-100x100.svg#xywh=0,0,-50,100");Same here. Try to use values that make this more "resilient" to implementation bugs.
background-image: url("support/green-100x100.svg#xywh=0,0,100,0");Ditto here.
background-image: url("support/green-100x100.svg#xywh=0,0,0,100");And here.
(percent:0,0,100,100) renders identically to no fragment at all,
i.e. that applying a 100% clip does not break or alter the image.This looks like a 50% clip though?
Only `width` or `height` (and combined with `viewBox`) is probably good to test as well.
Related details
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
Divyansh Mangal (Gerrit)
Divyansh Mangal added 7 comments![]( "Open in Gerrit")
bool is_url_valid_ = true;Divyansh MangalThe name `is_url_valid_` is misleading for the StringView constructor path, where there's no URL involved at all, true just means "parsing should proceed." Consider renaming to `is_valid_` or `should_parse_` to avoid confusion.
Fredrik SöderquistPlease see the reasoning for this name per comment https://chromium-review.googlesource.com/c/chromium/src/+/7675457/comment/30a18874_ce599c87/
I am ok with `should_parse_`, but maybe need more inputs from an owner as well before we change it again
You could just let a null `String` represent the "invalid" state, and then just do `if (!fragment_) ...`. (Could also use `.empty()`, since empty strings will never contain anything interesting.)
Incorporated fs's suggestion.
MediaFragmentURIParser::MediaFragmentURIParser(const KURL& url)This constructor could delegate to the above one.
Done
background-image: url("support/green-100x100.svg#xywh=0.0,0.0,50.0,50.0");If an implementation parses this and either ignores or accepts the fractions (giving 0,0,50,50), then this will still appear to pass. Maybe adjust the parameter values to prevent that or use a different image?
Fair enough, I have created a new support file and updated the fragment accordingly. Let me know if more changes are needed.
(Similarly updated other WPTs as well.)
background-image: url("support/green-100x100.svg#xywh=0,0,-50,100");Same here. Try to use values that make this more "resilient" to implementation bugs.
Done
background-image: url("support/green-100x100.svg#xywh=0,0,100,0");Divyansh MangalDitto here.
Done
background-image: url("support/green-100x100.svg#xywh=0,0,0,100");Divyansh MangalAnd here.
Done
(percent:0,0,100,100) renders identically to no fragment at all,
i.e. that applying a 100% clip does not break or alter the image.This looks like a 50% clip though?
Good catch, based on previous feedback as well, updated the support file and fragment accordingly.
Related details
- 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 3 comments![]( "Open in Gerrit")
const SVGImageViewInfo* SVGImage::CreateViewInfoFromSpatialFragment(It looks like this could live in `SVGViewSpec`, and just have appropriate information passed to it. I think that would make the interface look better, now the division of responsibilities are a bit weird.
Done
gfx::SizeF svg_size(SizeWithConfigAsFloat(SizeConfig()));
if (spatial_clip.unit == SpatialClip::Unit::kPercent) {
// Convert percent to pixel values based on the SVG's intrinsic size.
// TODO(crbug.com/979895): When the SVG has no explicit width/height
// (or they are percentages), `IntrinsicWidth()`/`IntrinsicHeight()`
// return nullopt, causing `intrinsic_size_` to be 0x0. This makes
// percent-based spatial fragments for such cases produce a zero-size
// clip.
clip_rect.Scale(svg_size.width() / 100.0f, svg_size.height() / 100.0f);It's unclear to me how this should work when there's no natural size. (In general, using `SVGImage`s `Size*()` accessors is an indication of any issue.)
You're right @f...@opera.com this is the existing limitation for the `percent` unit, which is what the TODO/crbug.com/979895 is calling out. I was thinking of handling it it in a follow-up CL. Two options I'm considering:
1. Resolve percentages against the SVG's natural coordinate space by reusing `LayoutSVGRoot::UnscaledNaturalSizingInfo()` instead of `SizeWithConfig*()`. That gives us intrinsic width/height when present, and falls back to the viewBox size otherwise, matching the sizing rules already codified for `LayoutSVGRoot`. If neither is available, the fragment genuinely can't resolve and we return nullptr.
Sample code:
```
const SVGSVGElement* svg = ...;
gfx::SizeF percent_base(
svg->IntrinsicWidth().value_or(0),
svg->IntrinsicHeight().value_or(0));
if (percent_base.IsEmpty()) {
percent_base = svg->CurrentViewBox().Rect().size(); // viewBox fallback
}
if (percent_base.IsEmpty()) {
return nullptr; // genuinely no natural size — fragment cannot resolve
}
```
2. Defer resolution to draw time and resolve the percent rect against the concrete object size in `SVGImage::DrawForContainer`. More spec-correct for the CSS-image case ("size of the resource" = concrete object size), but a larger change and would require restructuring how the spatial fragment flows through `SVGViewSpec`.
Let me know if these are valid options and we can take it up in a follow up CL or do you some other suggestion in mind?
// Only clamp when intrinsic dimensions are known (non-zero); SVGs
// without explicit width/height report 0x0 (see TODO above).Ditto here I guess. It would probably be good to be more specific here. (The natural width/height can be zero as well, and "no width/height" can be different from "width/height is zero".
Updated the comment to be more specific, pls have a look. (Though I am realising a case of `width=0` explicitly will need to be handled now)
Related details
- 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 3 comments![]( "Open in Gerrit")
Divyansh MangalPlease consider adding WPTs for:
1. SVG without explicit width/height (viewBox-only). We are explicitly handling the `svg_size.IsEmpty()` case, we should test it.
2. `<img>` element with percent fragment: Currently only pixel-mode is tested via <img>.
3. Non-square SVG with percent clip: verifies the x/y scaling applies independently.
Added the tests as suggested pls check.
Done
const SVGViewSpec* SVGViewSpec::CreateFromSpatialFragment(const SVGRect* rect) {
if (!rect) {
return nullptr;
}
SVGViewSpec* view_spec = MakeGarbageCollected<SVGViewSpec>();
view_spec->view_box_ = rect;
return view_spec;
}Divyansh MangalThis sets `view_box_` but leaves `preserve_aspect_ratio_` as nullptr. When `PreserveAspectRatio()` returns null, the SVG element's own `preserveAspectRatio` attribute is used. If the source SVG has `preserveAspectRatio="none"`, a non-square spatial clip (e.g., `xywh=0,0,100,50`) rendered into a square CSS box will get stretched, which is unlikely to be what we would expect.
Consider explicitly setting `preserveAspectRatio` to `xMidYMid meet` (the SVG default) on the view spec to ensure consistent behavior regardless of what the SVG element declares.
Fredrik SöderquistPer the SVG spec's fragment identifier definitions https://svgwg.org/svg2-draft/linking.html#SVGFragmentIdentifiersDefinitions, only explicitly specified view attributes override the root element's attributes.
The existing `svgView(viewBox(...))` and `<view>` paths follow this same pattern, when `preserveAspectRatio` isn't specified in the fragment, the element's own `preserveAspectRatio` is used (`CreateFromFragment` and `CreateForViewElement` both leave `preserve_aspect_ratio_` null when unspecified).forcing `xMidYMid meet` would make spatial fragments behave differently.
Though I should definitely add some tests with `preserveAspectRatio` specified which will give us more coverage.
Fredrik SöderquistWhy would a spatial fragment apply to (override) the `viewBox` though? That's not how I'm reading the spec (the little of it there is). The way I'm reading it, it should rather act as a clip/subregion into the image. (Overriding the `viewBox' may affect the viewport, natural aspect ratio, etc.)
So, we've cleared that bit up - and it should affect the `viewBox`. There's no way to override `preserveAspectRatio`, so one needs to be cognizant of that. (This is not different from the same `svgView` case.)
Closing this as discussed offline
Only `width` or `height` (and combined with `viewBox`) is probably good to test as well.
Added the test
Virali Purbey (Gerrit)
Virali Purbey added 1 comment![]( "Open in Gerrit")
<img src="support/red-green-200x200.svg#xywh=100,100,100,100" width="100" height="100">Would it be right to remove the explicit width/height on the `<img>` to verify the fragment overrides the natural size?
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Vinay Singh