[SVG] Handle empty path data in SMIL animations [chromium/src : main]
Virali Purbey (Gerrit)
Virali Purbey added 12 comments![]( "Open in Gerrit")
Virali PurbeyShould we write some test for accumulate="sum" with empty base across repeats?
Virali PurbeyGood call. The existing `animate-path-additive-sum-empty-base.tentative.html` test covers `additive="sum"` with an empty base, but it doesn't test across repeats with `accumulate="sum"`. I'll add a test case for that.
Will cover in a separate CL as suggested by FS.
Chromium conflates an empty string d="" with the 'd' attribute being
absent, because `CalculateAnimationMode()` used `.empty()` which
returns true for both null (absent) and empty (present) strings.
This causes empty 'd' values to be silently ignored in animation
endpoints.Virali PurbeyCould we try fixing this across the board rather than adding a specific workaround?
Done. Replaced `.empty()` with `.IsNull()` in `CalculateAnimationMode()` and the from-string error-suppression checks in `CalculateFromAndToValues()` / `CalculateFromAndByValues()`. No attribute-specific virtual method needed, the observable change is path-only since empty string is only a valid value for path data.
An empty 'd' is a valid SVG path with zero segments, distinctVirali Purbeyshould we give a spec reference (or SVG WG issue if not available) corresponding to this?
Virali PurbeyAdded SVG spec ref where the path data grammar is defined which allows an empty string (zero path commands). However, the SMIL interaction behavior is not explicitly specified. I'll file an SVG WG issue to clarify but this is an existing behavior in Firefox.
Done
// Empty paths can't be smoothly interpolated with non-empty paths.
if (GetAnimationMode() == kFromByAnimation &&
AttributeName() == svg_names::kDAttr) {
const auto& from_path = To<SVGPath>(*from_property_);
const auto& to_path = To<SVGPath>(*to_property_);
if (from_path.ByteStream().IsEmpty() != to_path.ByteStream().IsEmpty()) {
return false;
}
}Virali PurbeyI think we handle these cases in the animated type itself at the moment. Otherwise we could have `Add()` return false if the types couldn't be added. I think that's better done as a separate CL though. Regardless this is not a scalable approach (and quite ugly).
Agreed. Removed the SVGAnimateElement-level workaround. Incompatible empty paths are now handled entirely in `SVGPath::CalculateAnimatedValue()`, it returns early (base preserved) for additive/by cases and uses discrete for non-additive from-to. Will follow up with the `Add()` return-bool refactor in a separate CL.
// Empty + by = by (additive identity).Virali PurbeyThis seems to assume that "empty" is some form of "neutral value", but that doesn't match rendering of an empty path? Wouldn't it be better to stick to the old failure mode? I don't think it makes sense to have discrete animation for `by`.
Agreed. Reworked: when `is_additive` is true and paths are incompatible (one/both empty), the animation now fails (base preserved). Discrete is only used for non-additive from-to where the user explicitly specified both endpoints.
if (from_stream.IsEmpty() && to_stream.IsEmpty()) {
path_value_ = CSSPathValue::EmptyPathValue();
return;
}Virali PurbeyFor additive="sum" animations where both from/to are empty, won't this unconditionally replaces the underlying (base) value with empty, even though the animation's contribution is zero (identity). The additive step that would preserve the base is skipped, is that expected?
Atleast consider adding a test case that test something like below:
```
<path d="<non-empty-d>">
<animate id="animation" attributeName="d"
from=""
to=""
additive="sum"/>
</path>
```in above should the result be base value or empty throughout? (From the code it will be empty, but I am asking what's the expectation from spec )
Virali PurbeyYou're right to flag this. When from/to are empty, the animation value is empty regardless of additive mode, the "both empty → empty" short-circuit fires before the additive step. Per SMIL, the animation function result is `f(t)=empty` and additive would compute `base + empty`.
Since empty paths have zero segments and can't be structurally added to a non-empty base, treating empty as the result is the safest behavior. I'll add the test case you suggested to document this explicitly.
Done
// Test case: accumulate="sum" with empty base across repeatsVirali PurbeyThe base isn't expected to contribute here though? I don't think there's anything tentative about this test. We could land it spearately.
Acknowledged. Will land separately as suggested.
// Test case: additive="sum" with both from and to empty over non-empty base should result in empty pathVirali PurbeyI think this should just fail (no animate). Empty can't be added to non-empty.
Done. Test now expects base value preserved throughout (no animation).
// Test case: additive animation with empty base should add to empty (= animation values)Virali PurbeyDitto here.
Removed from this CL. Same pattern, will be addressed with the `Add()` return-bool follow-up.
// Test case: empty + by = by (identity for addition)Virali PurbeyDitto.
Done. Test now expects base preserved (empty throughout), animation fails since empty can't be added to non-empty by value.
// Test case: by animation with no base value should smoothly interpolate from empty to by valueVirali PurbeyAnd ditto here I guess.
Done. Test now expects base preserved (empty throughout), animation fails since empty can't be added to non-empty by value.
// Test case: from + empty = from (identity for addition)Virali PurbeyAnd here. Empty can't be added -> failure.
Acknowledged. This case requires `Add()` to signal failure (currently it's void and a no-op for incompatible types).
Related details
- Divyansh Mangal
- Fredrik Söderquist
- Vinay Singh
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 added 6 comments![]( "Open in Gerrit")
// Empty to: cannot animate toward an empty target path.interpolate?
if (to_stream.IsEmpty()) {
if (parameters.is_additive) {
// Additive with empty to: can't add empty result to base. No animation.Could we perhaps reject this case first, and just have a common case for the discrete cases? I think that could simplify the logic a little.
// from-by with both empty: Add() sees ByteStream().IsEmpty() (the by value isFormulate in "abstract" terms, not in specific implementation terms.
// from-by with empty by: Add() sees ByteStream().IsEmpty() (the by value isDitto.
// from-by with empty from: Add() sees empty other (identity no-op, returns
// true). CalculateAnimatedValue gets empty from vs non-empty to; from-by isDitto
// true). CalculateAnimatedValue gets empty from vs non-empty to; from-by is
// not inherently additive, so discrete animation at 50%.I think it is... the `by` can't be added to the `from` in this case, so I'd expect this to fail.
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. |
Virali Purbey (Gerrit)
Virali Purbey added 6 comments![]( "Open in Gerrit")
// Empty to: cannot animate toward an empty target path.Virali Purbeyinterpolate?
Updated.
if (to_stream.IsEmpty()) {
if (parameters.is_additive) {
// Additive with empty to: can't add empty result to base. No animation.Could we perhaps reject this case first, and just have a common case for the discrete cases? I think that could simplify the logic a little.
Tried updating the logic.
// from-by with both empty: Add() sees ByteStream().IsEmpty() (the by value isFormulate in "abstract" terms, not in specific implementation terms.
Done
// from-by with empty by: Add() sees ByteStream().IsEmpty() (the by value isVirali PurbeyDitto.
Done
// from-by with empty from: Add() sees empty other (identity no-op, returns
// true). CalculateAnimatedValue gets empty from vs non-empty to; from-by isVirali PurbeyDitto
Done
// true). CalculateAnimatedValue gets empty from vs non-empty to; from-by is
// not inherently additive, so discrete animation at 50%.I think it is... the `by` can't be added to the `from` in this case, so I'd expect this to fail.
- Divyansh Mangal
- Fredrik Söderquist
- Vinay Singh
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |