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

Undo CSSTransitionDiscrete interpolation test changes #40177

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented May 23, 2023

Now that we are planning to add a new opt-in syntax for
CSSTransitionDiscrete, the test changes which I made for
CSSTransitionDiscrete will not eventually stick. Since these tests are
interop23 tracked, the other browsers would like them to be updated
ASAP.

I thought about also disabling the CSSTransitionDiscrete flag, which
would make this patch a lot cleaner, but that would break demos and
stuff people are working on so I decided against it.

This was asked for here:
#39871

CSSTransitionDiscrete opt in syntax:
w3c/csswg-drafts#8857

Change-Id: If28b7e9fc4fc0d6dc851e35007fd7614a9b497a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4558664
Auto-Submit: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1160284}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

Copy link
Member

@BorisChiou BorisChiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update cssTransitionsInterpolation::nonInterpolationExpectations instead?

@dholbert
Copy link
Contributor

dholbert commented Jun 2, 2023

@josepharhar just checking in, is there anything blocking this at this point, or is this good to be merged?

(Maybe that'll happen automagically after something else happens; I'm not sure about the lifecycle for Chromium WPT merges. I do see that the linked chromium-review page ends with This CL has failed the run. Reason: [...] blink_wpt_tests failed [...]. Not sure if that's blocking this or not; it looks like that might be just showing Blink's prototype interpolation behavior which differs from the test's [legacy/non-prototype] expectations?)

@josepharhar
Copy link
Contributor

I'm on it, I had to mess with the flag and do some rebaselining. I'll send it out for review in chromium today.

@dholbert
Copy link
Contributor

@josepharhar hi! Checking in again -- do you think we can get this merged soon?

@josepharhar
Copy link
Contributor

Sorry for being slow, the change caused a large number of changes in our testexpectations which got uneasy feedback in the code review. I just responded with a justification, hopefully it goes over well

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-4558664 branch 2 times, most recently from 02e4aea to 0e398c7 Compare June 20, 2023 20:47
Now that we are planning to add a new opt-in syntax for
CSSTransitionDiscrete, the test changes which I made for
CSSTransitionDiscrete will not eventually stick. Since these tests are
interop23 tracked, the other browsers would like them to be updated
ASAP.

I thought about also disabling the CSSTransitionDiscrete flag, which
would make this patch a lot cleaner, but that would break demos and
stuff people are working on so I decided against it.

This was asked for here:
#39871

CSSTransitionDiscrete opt in syntax:
w3c/csswg-drafts#8857

Change-Id: If28b7e9fc4fc0d6dc851e35007fd7614a9b497a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4558664
Auto-Submit: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1160284}
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.

6 participants