web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
5.01k stars 3.11k forks source link

web-animations/responsive tests make some questionable assumptions about clamping #37053

Open graouts opened 1 year ago

graouts commented 1 year ago

In https://github.com/web-platform-tests/wpt/commit/d55ae322ed3351c7216003b48ca010b1e7ccb0fb @mehdi-kazemi upstreamed (and @kevers-google reviewed) a fair few tests about the effect of inherit and font-size on keyframes. A few of those tests name "… clamped to 0px" are surprising to me, for instance in web-animations/responsive/perspective.html:

test(function() {
    element.style.fontSize = '10px';
    var player = element.animate([{perspective: '40px'}, {perspective: 'calc(40px - 2em)'}], 10);
    player.pause();

    player.currentTime = 5;
    element.style.fontSize = '40px';
    assert_equals(getComputedStyle(element).perspective, '0px');

    player.currentTime = 7.5;
    assert_equals(getComputedStyle(element).perspective, '0px');
}, 'perspective clamped to 0px');

This test expects that after setting font-size to 40px the calc(40px - 2em) keyframe value resolves to 40px - 2 * 40px, or -40px and that blending at the mid-way point resolves to 0. Safari and Firefox both fail this test, and in the case of Safari that is because we clip the resolved keyframe value to 0 since the CSS Transforms Level 2 spec for perspective says that negative values are not allowed. As such we animate between 40px and 0 and not 40px and -40px.

I believe this test was written by @ericwilligers on the Chromium side. Eric, could you explain how this test is believed to be correct?

Cc @birtles, @flackr and @BorisChiou on the animations side, but also @dbaron for the transforms aspect of this.

ewilligers commented 1 year ago

font-size can change during the perspective animation, for example font-size might itself be animated. Thus perspective is effectively animated as a calc expression. [Blink might internally use a vector with coefficients corresponding to px, em, vx etc.]

At 50% progress through the animation, we have the midpoint between calc(40px - 0em) and calc(40px - 2em), i.e. calc(40px - 1em). As font-size is 40px, this results in 0px.

At 75% progress, calc(40px - 1.5em) would result in a negative value, if not for clamping.

graouts commented 1 year ago

The issue I was highlighting is that Safari (and I assume Firefox) resolve the calc() value on the keyframes prior to interpolating, meaning that at 50% with font-size set to 40px we interpolate with the from value set to 40px and the to value set to 0px since perspective clamps to 0.

It's not obvious to me that the calc() values should not be resolved at the keyframe level.

graouts commented 1 year ago

I think the relevant spec text here is the Calculating computed keyframes section. Here are some excerpts:

Before calculating the effect value of a keyframe effect, the property values on its keyframes are computed.

[…]

Computed keyframes are produced using the following procedure:

[…]

  1. For each property specified in keyframe:

To me this clearly states that interpolation is performed over computed keyframes and those would compute calc() values.

graouts commented 1 year ago

To me this clearly states that interpolation is performed over computed keyframes and those would compute calc() values.

Not so fast though, Chrome, Firefox and Safari all agree that animation.effect.getKeyframes() returns calc() values here.

graouts commented 1 year ago

The CSS Values and Units spec has this to say in the Range Checking section for calc():

Parse-time range-checking of values is not performed within math functions, and therefore out-of-range values do not cause the declaration to become invalid. However, the value resulting from an expression must be clamped to the range allowed in the target context.

The question then is: at what stage of the value computation during interpolation is clamping performed.

Going back to the test in question, and in the context of the CSS Values and Units spec, I draw a parallel to computing the result value to:

<div style="font-size: 40px; perspective: mix(0.5, 40px, calc(40px - 2em))"></div>

What is the computed value of mix(0.5, 40px, calc(40px - 2em))? Is the output of calc(40px - 2em) clamped or the eventual output of mix() alone? I filed https://github.com/w3c/csswg-drafts/issues/8158 to get to the bottom of that too.

graouts commented 1 year ago

Actually, re-reading everything, I think the Range Checking section for calc() spells out that the output of the calc() expression should be clamped and that Chrome is not producing the right value for this test and that the test is incorrect. When font-size is 40px, the output of calc(40px - 2em) is not -40px but 0px since perspective only allows non-negative values and the "value resulting from an expression must be clamped to the range allowed in the target context".

flackr commented 1 year ago

The question then is: at what stage of the value computation during interpolation is clamping performed.

Note that even if we clamp the keyframes we still also have to clamp the interpolated value since you can have easing curves like cubic-bezier(0.1, 2, 0.9, 2) which could exceed the allowed range.

The linked css-values section also states:

and also on used values if computation was unable to sufficiently simplify the expression to allow range-checking.

It could be argued that the keyframes are not directly used and so not sufficiently simple to range-check. Also, when used with composite modes it is even less trivial to say whether they are in range or not, and it seems like it could be useful to allow negative values when added to an underlying positive value.

flackr commented 1 year ago

I'm also worried about cases where the value depends on that animation or other animated values. E.g. what if font-size is also animating, or if the calc uses a variable that could also be animating.

graouts commented 1 year ago

There definitely is value in having "out of range" values used for intermediary computing since the interpolated value is clamped. I'm not really arguing for or against that. What I'm after is whether there is clear spec text to argue one way or another, and if not that it be clarified in the relevant spec(s).

graouts commented 1 year ago

I recently fixed a bug in WebKit where we failed to correctly interpolate opacity with iterationComposite set to accumulate, it was this subtests in web-animations/animation-model/keyframe-effects/effect-value-iteration-composite-operation.html:

test(t => {
  const div = createDiv(t);
  const anim =
    div.animate({ opacity: [0, 0.4] },
                { duration: 100 * MS_PER_SEC,
                  easing: 'linear',
                  iterations: 10,
                  iterationComposite: 'accumulate' });
  anim.pause();

  anim.currentTime = anim.effect.getComputedTiming().duration / 2;
  assert_equals(getComputedStyle(div).opacity, '0.2',
    'Animated opacity style at 50s of the first iteration');
  anim.currentTime = anim.effect.getComputedTiming().duration * 2;
  assert_equals(getComputedStyle(div).opacity, '0.8',
    'Animated opacity style at 0s of the third iteration');
  anim.currentTime += anim.effect.getComputedTiming().duration / 2;
  assert_equals(getComputedStyle(div).opacity, '1', // (0.8 + 1.2) * 0.5
    'Animated opacity style at 50s of the third iteration');
}, 'iteration composition of opacity animation');

We would fail the final assertion because we would interpolate between 0.8 and 1.0 for the third iteration instead of 0.8 and 1.2 as we applied clamping to the computed keyframe values.

graouts commented 1 year ago

@flackr But what do you think about the test I raised this issue about? Do you think it's correct? I think it's not and thinking of changing it and others like it in a pull request, which would make Safari and Firefox pass but Chrome fail.

birtles commented 1 year ago

I think @graouts reading of the spec is right. That is, Web Animations, says that the property values are computed before interpolating. Furthermore, V&U says that the em / px ratio is known at computed value time so it should be resolved there (unlike some percentage values which cannot be resolved yet).

So I think @graouts is correct about that test being in error.

I believe in Gecko we represent computed calc values as a percentage + length tuple and interpolate on that which would explain why Gecko behaves the same as Webkit here. (That said, I haven't touched that code in several years so @emilio or @hiikezoe would be much more qualified to comment on the Gecko behavior.)

I like the Chrome behavior, though. SVG/SMIL animation always emphasised clamping as late as possible (e.g. "User agents should clamp color values to allow color range values as late as possible" in SVG 1.1). If we could eventually spec that behavior for this case too, it seems like that would be preferable.

(Somewhat related, if some variant of the !add proposal ever happens we'll probably want to be able to represent out of range values so that you can write opacity: -0.5 !add. But perhaps that could be achieved by clamping at computed value time anyway.)

emilio commented 1 year ago

If I recall correctly, animation happens between computed values, and thus I'm the first test-case, font-size should be clamped before the interpolation starts.

graouts commented 1 year ago

If I recall correctly, animation happens between computed values, and thus I'm the first test-case, font-size should be clamped before the interpolation starts.

@Emilio are you talking about the Firefox implementation here or something that a spec mandates? If the latter, which?

emilio commented 1 year ago

https://drafts.csswg.org/css-values-4/#combining-values mentions:

The following combining operations—on the two computed values [...] are defined:

graouts commented 1 year ago

https://drafts.csswg.org/css-values-4/#combining-values mentions:

The following combining operations—on the two computed values [...] are defined:

Thanks for spotting this @Emilio.

I will make a pull request to fix these tests to match the current state of specifications.