[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. |