search

web-platform-tests / wpt

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

test_no_interpolation tests don't have the right expectations for transitionable properties that have value-pairs that interpolate discretely #39871

Closed dholbert closed 1 year ago

dholbert commented 1 year ago

I think there's a test bug (and a Chromium bug) in a bunch of interpolation WPT tests that use test_no_interpolation() with discretely-interpolatable values of a property that is otherwise generally interpolatable.

For example, "caret-color" (which takes colors and hence can generally interpolate) and "offset-rotate" (which takes angles and can generally interpolate), whose interpolation is tested in these tests: http://wpt.live/css/css-ui/animation/caret-color-interpolation.html http://wpt.live/css/motion/animation/offset-rotate-interpolation.html

General issue / tl:dr: right now, these^ tests expect transition:all to magically include or exclude these properties depending on whether or not the particular begin/end values happen to be discretely vs. smoothly interpolatable. And I don't think that matches the csswg resolution or spec edit -- the spec talks about the property as a whole being included in transition:all depending on whether the property as a whole has a discrete animation type.

Quoting some relevant bits of the test expectations (from running them on wpt.live):

For a given pair of values, the tests are expecting a discrete 50% flip for transition: <property name> BUT they're expecting an immediate value-change (no transition) for transition: all. I don't think that matches the spec.

Spec reference: https://drafts.csswg.org/css-transitions-2/#transition-property-property

"the all keyword continues to expand only to all transitionable properties whose animation type is not discrete."

In this case, the property's animation type is not discrete[1], so it should be included in transition:all. (It does have some keyword values like auto that aren't transitionable, but the value as a whole still has a non-discrete Animation Type.

@fantasai, @dbaron, am I understanding this spec text (from https://github.com/w3c/csswg-drafts/pull/8520 ) correctly? Do we need to fix the tests, or is the spec missing some nuance?

(Note that Chromium passes these tests which I think means Chromium has a bug. Firefox starts failing the above-quoted transition: all subtests with our patch to add the 50% flip, in https://bugzilla.mozilla.org/show_bug.cgi?id=1805727 -- but as I understand it, our test-failures are actually what the spec requires.)

[1] e.g. https://drafts.csswg.org/css-ui-3/#caret-color has "Animation type: by computed value" (not "discrete"). Note that "by computed value" is defined such that certain values "combine as discrete" but the property's animation type is still "by computed value".

dbaron commented 1 year ago

cc @josepharhar

dholbert commented 1 year ago

Here's a reduced testcase to get to the crux of the matter here, with a hover-triggered restyle of background-size on 4 boxes: https://jsfiddle.net/p9jL0chs/

In this testcase, I would expect that cases (1) and (2) should behave the same as each other (with a 50% flip), and cases (3) and (4) should behave the same as each other (with a smooth transition).

I expect those pairs to behave the same because they only differ in whether they use transition-property: <property-name> vs. all; and I expect all to expand to include background-size, based on https://drafts.csswg.org/css-transitions-2/#transition-property-property , specifically because background-size has an Animation Type that is not discrete, as defined here: https://www.w3.org/TR/css-backgrounds-3/#the-background-size

However, WPT test http://wpt.live/css/css-backgrounds/animations/background-size-interpolation.html is essentially expecting that cases (1) and (2) should behave differently, since (as with the other properties in my initial comment here) it's expecting an immediate style-change for non-interpolatable values iff transition:all is used.

dholbert commented 1 year ago

If my understanding is correct, then I think probably the fix here would be for the test_no_interpolation WPT utility function to gain some sort of propertyIsDiscrete parameter, which would be based on the property's general "Animation Type" , and would influence whether the test expects transition:all to generate a transition for the property or not.

josepharhar commented 1 year ago

Hi, I made the changes to test_no_interpolation recently.

Requiring the property to be explicitly listed to opt into this new and potentially breaking behavior, instead of also changing everything that uses transition:all, will help us mitigate damage to websites.

Here is a regression bug from me trying to ship the new discrete transition behavior due to changes when transition-property: top, left was used and the computed value was changed between "auto" and an explicit value: https://bugs.chromium.org/p/chromium/issues/detail?id=1439802 If we made top and left also have the new discrete transition behavior between auto and fixed values when transition:all is used, that would probably break more websites.

It may turn out that we can't ship this new discrete transition behavior at all, but I am still working on figuring that out.

dholbert commented 1 year ago

That sounds like we need to update the spec to better-match what-is-possible-to-ship, then.

dholbert commented 1 year ago

It may turn out that we can't ship this new discrete transition behavior at all, but I am still working on figuring that out.

Do you have a timeline on figuring that out? We've got an implementation that's nearly ready to land in https://bugzilla.mozilla.org/show_bug.cgi?id=1805727 . If we're $X days away from potentially learning that this feature is shippable vs. doomed-to-be-un-shippable, then that'd be useful for how to proceed there (e.g. we might hold off on landing the 8-part patch stack right away, or might hold off on polishing, depending on the value of $X).

josepharhar commented 1 year ago

I am going to do some preliminary research this week to see if it is for sure unshippable. If we decide to go forward with it, then I wouldn't be 100% certain that it would be compatible until it gets to stable chrome 115, which would happen on July 18th.

dholbert commented 1 year ago

Thanks!

josepharhar commented 1 year ago

@dholbert here is an issue discussing this: https://github.com/w3c/csswg-drafts/issues/8857

dholbert commented 1 year ago

Thanks.

For the purposes of this WPT github-issue here (about the tests agreeing/disagreeing with the spec)... While the hypothetical new syntax is being hashed out, could all of the {property}-interpolation.html WPT tests just be reverted to test the shipping-in-all-browsers behavior (which will now probably continue to be the default)?

These tests are all contributing to interop-2023 score deficits, and from an "interop in 2023" perspective, that doesn't really make sense if the new behavior is going to behind a new syntax that's not used anywhere on the web yet. When we were considering changing the default interpolation behavior, it made sense to update these tests' expectations; but now that we're leaving the default behavior as-is, it feels like we should revert them, and either add a new test or extend these existing tests (after reverting them) once we've got the new syntax hashed out.

josepharhar commented 1 year ago

Oh sorry I didn't know that the affected tests were in interop23. Would this change be enough? https://github.com/web-platform-tests/wpt/pull/40177

dholbert commented 1 year ago

@josepharhar Thanks -- I didn't look too closely, but I see Boris did, and I think that generally addresses the thing I was asking about.

I took a quick look at some of the results on the staging server here... https://staging.wpt.fyi/results/css/css-flexbox/animation/flex-basis-interpolation.html?sha=8792d59098&label=pr_head https://staging.wpt.fyi/results/css/css-backgrounds/animations/border-image-width-interpolation.html?sha=8792d59098&label=pr_head https://staging.wpt.fyi/results/css/css-ui/animation/caret-color-interpolation.html?sha=8792d59098&label=pr_head https://staging.wpt.fyi/results/css/motion/animation/offset-rotate-interpolation.html?sha=8792d59098&label=pr_head ...and it looks like Firefox is passing all of those tests now, which is good news. :)

josepharhar commented 1 year ago

I finally finished undoing the test changes, sorry it took so long. Let me know if there is anything unexpected leftover

dholbert commented 1 year ago

Thanks! Yup, I think we're all set here. I'll let you know if I run across anything unexpected.

dholbert commented 1 year ago

Let me know if there is anything unexpected leftover

@josepharhar I found one thing left over: https://wpt.fyi/results/css/motion/animation/offset-interpolation.html shows 6 Firefox/Safari subtests failures that look like they're versions of this behavior (where the test is expecting the experimental behavior that we've now decided against).

Here's a historical link to wpt.fyi results for the test, before the discrete-interpolation changes, where Firefox passes all subtests (probably Safari as well, though they don't have results recorded in wpt.fyi): https://wpt.fyi/results/css/motion/animation/offset-interpolation.html?sha=ad172b15c6&label=master Here's the test itself at that point: https://github.com/web-platform-tests/wpt/blame/ad172b15c6091ba467d23de9a5f84a58d173e429/css/motion/animation/offset-interpolation.html ...vs. here's the current version of the test that Firefox/Safari fail: https://github.com/web-platform-tests/wpt/blame/c86ad3a0d8113da415930cd3a0807819368de54d/css/motion/animation/offset-interpolation.html

Comparing the historical version against the current version, there are two sets of differences, both of which were added as part of various discrete-transitions churn there: (1) In the first test_interpolation(, the test is currently expecting the path(...) expression to do do a 50% flip (i.e. it's got the from value for -0.3, 0, and 0.3) (2) In the second test_interpolation(, the test is currently expecting the auto component to do a 50% flip (i.e. auto is present for -0.3, 0, and 0.3)

I think both of those changes are bogus at this point and the test probably wants to be reverted entirely to its historical ad172b15c6091ba467d23de9a5f84a58d173e429 state -- does that make sense?

(FWIW we're tracking these subtest failures in https://bugzilla.mozilla.org/show_bug.cgi?id=1840438 )

josepharhar commented 1 year ago

Sorry about that, I believe this PR should change the test back to normal: https://github.com/web-platform-tests/wpt/pull/40551/files#diff-cd8c420542d6833285c2aa4af0724735203b305fd9ada3257a28a49f800738ab

dholbert commented 1 year ago

Thanks! The changes to this test there look good to me; I see Firefox passing 36/36 subtests in the wpt.fyi view of the PR test-results: https://wpt.fyi/results/css/motion/animation/offset-interpolation.html?sha=2a95428b48&label=experimental&label=pr_head&product=chrome&product=firefox&view=interop

\o/