memo -- change the default behavior of `commitStyles` pt2
unupdated base value
In the previous post, I tweaked the behavior of commitStyles() on the animation endpoint.
I thought it would work well, and some small fixes would make things finished.
However, it was not so simple. Specifically, the below case didn't pass.
// new wpt case added in this change
promise_test(async t => {
const div = createDiv(t);
div.style.opacity = '0.1';
const animA = div.animate(
{ opacity: '0.2' },
{ duration: 1 }
);
const animB = div.animate(
{ opacity: '0.2', composite: 'add' },
{ duration: 1 }
);
const animC = div.animate(
{ opacity: '0.3', composite: 'add' },
{ duration: 1 }
);
animA.persist();
animB.persist();
await animB.finished;
animA.commitStyles();
animB.commitStyles();
assert_numeric_style_equals(getComputedStyle(div).opacity, 0.4); // no... it's 0.3!
}, 'Commits the intermediate value of an animation up to the middle of the stack');
After a long debug, it turned out that the base value of the keyframe for animB was still 0.1 after animA was committed.
Even though the inline-style of div was changed to 0.2 by animA.commitStyles(), animB "didn't notice" div's style already changed.
So, committing animB just added 0.2 to its base value 0.1 and set 0.3 to div's inline-style.
The key factor here is that: the style to be committed by commitStyles is calculated based solely on values held by the corresponding keyframe. So it's necessary to update the keyframe with the current style in order to get the correct result.
but why not updated?
Intuitively, all the related key frames would be updated by the style flush executed at the beginning of commitStyles.
void Animation::CommitStyles(ErrorResult& aRv) {
// ...omitted... //
if (doc) {
// style flush occurs!!
doc->FlushPendingNotifications(FlushType::Frames);
}
// ...omitted... //
// Get the computed animation values
UniquePtr<StyleAnimationValueMap> animationValues(
Servo_AnimationValueMap_Create());
if (!presContext->EffectCompositor()->ComposeServoAnimationRuleForEffect(
*keyframeEffect, CascadeLevel(), animationValues.get())) {
NS_WARNING("Failed to compose animation style to commit");
return;
}
// ...omitted... //
}
This flush is almost required by the spec of commitStyles, because it says "after applying any pending style changes". But we can see, at least, it's not for updating keyframe but for updating target's style.
If, after applying any pending style changes, target is not being rendered, throw an "InvalidStateError" DOMException and abort these steps.
https://www.w3.org/TR/web-animations-1/#commit-computed-styles
Servo doesn't update the key frame by style flush when it doesn't see any animation undergoing. Although I'm not 100% sure, but it's obviously for optimization.
So, after animA.commitStyles, animB's key frame is not updated.
// new wpt case added in this change
promise_test(async t => {
const div = createDiv(t);
div.style.opacity = '0.1';
const animA = div.animate(
{ opacity: '0.2' },
{ duration: 1 }
);
const animB = div.animate(
{ opacity: '0.2', composite: 'add' },
{ duration: 1 }
);
const animC = div.animate(
{ opacity: '0.3', composite: 'add' },
{ duration: 1 }
);
animA.persist();
animB.persist();
await animB.finished;
animA.commitStyles();
// Here, Servo doesn't see any animation ongoing. So it doesn't think it needs to update keyframes.
animB.commitStyles(); // As a result, the keyframe of animB thinks div's opacity is still 0.1.
assert_numeric_style_equals(getComputedStyle(div).opacity, 0.4);
}, 'Commits the intermediate value of an animation up to the middle of the stack');
Let's look at the code.
In process_animations, Servo has determined what kind of tasks should be executed.
#[cfg(feature = "gecko")]
fn process_animations(
&self,
context: &mut StyleContext<Self>,
old_styles: &mut ElementStyles,
new_styles: &mut ResolvedElementStyles,
restyle_hint: RestyleHint,
important_rules_changed: bool,
) {
// ...omitted... //
let mut tasks = UpdateAnimationsTasks::empty();
// ...omitted... //
if self.needs_animations_update(
context,
old_values.as_deref(),
new_styles.primary_style(),
/* pseudo_element = */ None,
) {
tasks.insert(UpdateAnimationsTasks::CSS_ANIMATIONS);
}
// ...omitted... //
if self.has_animations(&context.shared) {
tasks.insert(UpdateAnimationsTasks::EFFECT_PROPERTIES);
// ...omitted... //
}
// ...omitted... //
}
If tasks includes UpdateAnimationsTasks::EFFECT_PROPERTIES, Gecko_UpdateAnimations updates animations.
void Gecko_UpdateAnimations(const Element* aElement,
const ComputedStyle* aOldComputedData,
const ComputedStyle* aComputedData,
UpdateAnimationsTasks aTasks) {
// ommitted //
if (aTasks & UpdateAnimationsTasks::EffectProperties) {
presContext->EffectCompositor()->UpdateEffectProperties(
aComputedData, const_cast<Element*>(element), pseudoRequest);
}
// ommitted //
}
In the test case, no animation is ongoing when animB.commitStyles is executed. All the three animations are already finished. So the keyframe of animB is not updated.
so how you fix?
Brian and I think it can be a existing problem. Although the style flush at the beginning of Animation::CommitStyles is not intended to update key frames, it's a bit unnatural that some key frames are left unupdated.
So, simply, we decided to update the committed animation's key frame before calculating the inline style.
When style flush leads to update of key frame, KeyframeEffect::EnsureBaseStyles replaces the base value with the current value. So we use it before calculating the inline style. That's it.
ci
By the way, it was the first time to run CI on the try server.
You can find results here. "repo" should be "try" and "My pushes only" filter is useful.