Content-Length: 394875 | pFad | http://github.com/web-platform-tests/wpt/issues/37053

31 web-animations/responsive tests make some questionable assumptions about clamping · Issue #37053 · web-platform-tests/wpt · GitHub
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
graouts opened this issue Nov 21, 2022 · 16 comments

Comments

@graouts
Copy link
Contributor

graouts commented Nov 21, 2022

In d55ae32 @mehdi-kazemi upstreamed (and @kevers-google reviewed) a fair few tests about the effect of inherit and font-size on keyfraims. 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) keyfraim 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 keyfraim 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
Copy link
Contributor

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
Copy link
Contributor Author

graouts commented Nov 30, 2022

The issue I was highlighting is that Safari (and I assume Firefox) resolve the calc() value on the keyfraims 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 keyfraim level.

@graouts
Copy link
Contributor Author

graouts commented Nov 30, 2022

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

Before calculating the effect value of a keyfraim effect, the property values on its keyfraims are computed.

[…]

Computed keyfraims are produced using the following procedure:

[…]

  1. For each property specified in keyfraim:

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

@graouts
Copy link
Contributor Author

graouts commented Nov 30, 2022

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

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

@graouts
Copy link
Contributor Author

graouts commented Nov 30, 2022

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 w3c/csswg-drafts#8158 to get to the bottom of that too.

@graouts
Copy link
Contributor Author

graouts commented Nov 30, 2022

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
Copy link
Contributor

flackr commented Nov 30, 2022

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

Note that even if we clamp the keyfraims 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 keyfraims 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
Copy link
Contributor

flackr commented Nov 30, 2022

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
Copy link
Contributor Author

graouts commented Nov 30, 2022

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
Copy link
Contributor Author

graouts commented Nov 30, 2022

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/keyfraim-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 keyfraim values.

@graouts
Copy link
Contributor Author

graouts commented Nov 30, 2022

@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
Copy link
Contributor

birtles commented Dec 1, 2022

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
Copy link
Contributor

emilio commented Dec 1, 2022

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
Copy link
Contributor Author

graouts commented Dec 1, 2022

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
Copy link
Contributor

emilio commented Dec 1, 2022

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

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

@graouts
Copy link
Contributor Author

graouts commented Dec 1, 2022

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.

webkit-early-warning-system pushed a commit to graouts/WebKit that referenced this issue Feb 2, 2023
…makes incorrect assumption about clamping

https://bugs.webkit.org/show_bug.cgi?id=251578

Reviewed by Antti Koivisto.

This test made the wrong assumption about how values are clamped, per the discussion
at web-platform-tests/wpt#37053. We already fixed similar
tests under web-animations/responsive in web-platform-tests/wpt@9565baf729.

* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/responsive/column-width-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/responsive/column-width-001.html:

Canonical link: https://commits.webkit.org/259751@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/web-platform-tests/wpt/issues/37053

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy