Skip to content
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

Revert "Reland #2 "Implement CSSTransitionDiscrete which enables transitions on discrete properties."" #38904

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

graouts
Copy link
Contributor

@graouts graouts commented Mar 9, 2023

Reverts #38812

This made at least two tests fail across all browsers:

css/css-transitions/animations/vertical-align-interpolation.html
css/css-transitions/animations/z-index-interpolation.html
This was caught by the checks on this PR but somehow they appeared green, maybe because the WPT.fyi checks are in beta. It appears there are a lot of regressions:

https://wpt.fyi/results/css?diff&filter=ADC&run_id=5094539348410368&run_id=5077352225177600

Either this change is correct and a fair few tests need to be updated to have new expectations, or this test is wrong. Either way, I think this should be reverted until this is addressed.

Cc @josepharhar and @flackr

…sitions on discrete properties.""

This reverts commit 268250b.
@gsnedders
Copy link
Member

This was caught by the checks on this PR but somehow they appeared green, maybe because the WPT.fyi checks are in beta. It appears there are a lot of regressions:

https://wpt.fyi/results/css?diff&filter=ADC&run_id=5094539348410368&run_id=5077352225177600

We deliberately don't require no regressions, mainly because there are plenty of reasonable cases where tests do go from pass to fail (spec changes, increasing strictness of the pass condition, etc.).

@gsnedders gsnedders enabled auto-merge (rebase) March 9, 2023 12:23
@gsnedders gsnedders merged commit 78f70fd into master Mar 9, 2023
@gsnedders gsnedders deleted the revert-38812-chromium-export-cl-4308075 branch March 9, 2023 12:31
@josepharhar
Copy link
Contributor

These test changes were based on this CSSWG resolution: w3c/csswg-drafts#4441 (comment)
Can we add them back?

@dbaron
Copy link
Member

dbaron commented Mar 9, 2023

Also worth noting the spec edits for this happened in w3c/csswg-drafts#8520

@graouts
Copy link
Contributor Author

graouts commented Mar 9, 2023

These test changes were based on this CSSWG resolution: w3c/csswg-drafts#4441 (comment)
Can we add them back?

I should think the tests ought to be fixed to match the spec so that there are no failures, even if the failures are now expected.

@dbaron
Copy link
Member

dbaron commented Mar 9, 2023

What class of failures do you want to avoid? (That is, what do you mean by "no failures, even if the failures are now expected"?)

I think it's normal that if a spec changes so that an implementation no longer conforms to the spec, that implementation should start failing tests -- it's a good way to notice that the spec has changed.

@graouts
Copy link
Contributor Author

graouts commented Mar 9, 2023

I'm expecting that the tests related to transitions and discrete properties are modified to produce PASS results when the implementation is correct. Right now there are heaps of tests yielding FAIL results. I just want to be sure this is correct and the tests will PASS once implementations implement the spec change. An example are the two tests highlighted in my original PR message.

If those tests now FAIL'ing are doing so correctly, then this PR should be landed back, by all means.

@dbaron
Copy link
Member

dbaron commented Mar 9, 2023

Based on https://chromium-review.googlesource.com/c/chromium/src/+/4308075 those tests were passing in patched Chromium. (There's no entry for them in TestExpectations and no *-expected.txt file.)

The Chromium you see on the "experimental" channel for wpt.fyi is dev channel, which can often be a week or two behind.

@graouts
Copy link
Contributor Author

graouts commented Mar 10, 2023

OK, then I guess this is good. Thanks for providing all the extra information, this helped.

@josepharhar
Copy link
Contributor

I made a PR to re-add the test changes: #38936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants