1877383 - [css-properties-values-api] Animated registered property in @keyframes without end state doesn't seem to interpolate properly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Navigate to data:text/html,<meta charset=utf8><style>@property --bg { syntax: "<color>"; initial-value: cyan; inherits: true;}%20 div { display: inline-block; margin: 20px;background-color: var(--bg);width: 50px;height: 50px; animation: color 2s infinite;} .a {%20 animation-name: colora; } .b { animation-name: colorb;} @keyframes colora { 0% { --bg: gold; } 100% { --bg: tomato; } } @keyframes colorb { 0%,50% { --bg: tomato; } } </style><div class=a>A</div><div class=b>B</div>

Expected results

Both A and B squares background color is animated

Actual results

B square background color is not animated, it jumps to cyan mid-animation

---

Here's the CSS in a more readable way:

@property --bg {
  syntax: "<color>";
  initial-value: cyan;
  inherits: true;
}
div {
  background-color: var(--bg);
  animation: color 2s infinite;
}
.a {
  animation-name: colora;
}
.b {
  animation-name: colorb;
}
@keyframes colora {
  0% {
    --bg: gold;
  }
  100% {
    --bg: tomato;
  }
}
@keyframes colorb {
  0%,
  50% {
    --bg: tomato;
  }
}

The issue seems to be in colorb keyframes, where we don't set an end state.
In Chrome and Safari, B square is properly animated

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

zsun, is this a known limitation? If so feel free to close this bug if there's another one to cover this case

Comment 2

[Zach Hoffman [:zrhoffman]]

In the case of colorb, it looks like the color values for self and other are the same in Animate::CustomAnimatedValue <https://searchfox.org/mozilla-central/rev/2c3d657cbba5484ccac44443c4417baed7b5fafb/servo/components/style/properties_and_values/value.rs#544>, which causes self.value.animate to fail for most Procedure::Interpolate progresses.

Probably Servo_StyleSet_GetKeyframesForName <https://searchfox.org/mozilla-central/rev/2c3d657cbba5484ccac44443c4417baed7b5fafb/servo/ports/geckolib/glue.rs#6483> will need to be changed, since that's where the duplicate values are coming from, but I haven't pinpointed where yet.

Comment 3

[zsun]

Thanks Zach!

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Clearer test-case.

Both squares should probably animate equally.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The issue is not comment 2. The problem is that when we get an animated value from the computed style (such as when omitting the start or end keyframe) we lose the type information here.

We should carry over that type information somehow or look it up.

Comment 6

[Zach Hoffman [:zrhoffman]]

Sorry, I should have corrected myself when I realized that comment 2 was not correct.

Specifically, hasAdditiveValues <https://searchfox.org/mozilla-central/rev/6b8a3f804789fb865f42af54e9d2fef9dd3ec74d/dom/animation/KeyframeEffect.cpp#571> is returning false because aProperty.mToValue is initially null, resulting in trying to use a ComputedValue::Universal for the other value used for interpolation, since not yet having implemented bug 1864736 means that the computed style for registered custom properties is still untyped.

If the condition at <https://searchfox.org/mozilla-central/rev/6b8a3f804789fb865f42af54e9d2fef9dd3ec74d/dom/animation/KeyframeEffect.cpp#571> is instead changed to

      (hasAdditiveValues(aProperty) && !aProperty.mProperty.IsCustom());

since mToValue is computed later in the case of a custom property, value will interpolate successfully in CustomAnimatedValue::Animate <https://searchfox.org/mozilla-central/rev/2c3d657cbba5484ccac44443c4417baed7b5fafb/servo/components/style/properties_and_values/value.rs#548>, but child_restyle_requirement will still not be set to ChildRestyleRequirement::MustCascadeChildren at <https://searchfox.org/mozilla-central/rev/6b8a3f804789fb865f42af54e9d2fef9dd3ec74d/servo/components/style/traversal.rs#427> for colorb in the testcase like it does for colora (though I have not yet tested carrying over or looking up that type information, as suggested in comment 5).

So if the type information is not looked up or carried over, a condition determining whether compute_style returns ChildRestyleRequirement::MustCascadeChildren should probably be changed.

Comment 7

[Zach Hoffman [:zrhoffman]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

The issue is not comment 2. The problem is that when we get an animated value from the computed style (such as when omitting the start or end keyframe) we lose the type information here.

We should carry over that type information somehow or look it up.

You are right, carrying over the type animation does make the testcase behave as expected.

Comment 8

[Zach Hoffman [:zrhoffman]]

Attachment: Bug 1877383 - Preserve computed registered custom property types. r=zsun,fredw,#style

animation/custom-property-animation-transform-none.tentative.html now
fails for the first time since bug 1846516, and is tracked in bug
1861408.

Also add a TODO for bug 1883255, since this fixes bug 1870348.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/230e45fd2ece
Preserve computed registered custom property types. r=firefox-style-system-reviewers,emilio

Comment 10

[Cristian Tuns]

Backed out for causing build bustages.

• Backout link
• Push with failures
• Failure Log
• Failure line: error[E0599]: no method named is_parsed found for reference &Value<GenericValueComponent<CSSPixelLength, f32, Percentage, ..., ..., ..., ..., ..., ..., ..., ..., ...>> in the current scope

Comment 11

[Zach Hoffman [:zrhoffman]]

Had to add #[cfg(debug_assertions)] to a debug_assert using a debug-only function.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4f2a707dbc
Preserve computed registered custom property types. r=firefox-style-system-reviewers,emilio

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1f4f2a707dbc