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

Change motion path <ray> <contain> test references as per spec change #321

Closed
danielsakhapov opened this issue Apr 21, 2023 · 9 comments
Closed
Assignees
Labels
focus area: Motion Path test-change-proposal Proposal to add or remove tests for an interop area

Comments

@danielsakhapov
Copy link

Test List

https://wpt.fyi//css/motion/offset-path-ray-contain-001-ref.html
https://wpt.fyi//css/motion/offset-path-ray-contain-002-ref.html
https://wpt.fyi//css/motion/offset-path-ray-contain-003-ref.html
https://wpt.fyi//css/motion/offset-path-ray-contain-004-ref.html
https://wpt.fyi//css/motion/offset-path-ray-contain-005-ref.html

Rationale

Spec: https://drafts.fxtf.org/motion/#valdef-ray-contain
Test changes are due to the recent spec change: previously implied that the box should be inside the circle, formed by the radius of the ray length. Now it became easier as just reduces the ray length by a fixed value.

CL to see the changes: https://chromium-review.googlesource.com/c/chromium/src/+/4453583

@danielsakhapov danielsakhapov added the test-change-proposal Proposal to add or remove tests for an interop area label Apr 21, 2023
@danielsakhapov
Copy link
Author

@tabatkins @BorisChiou @dirkschulze FYI

@BorisChiou
Copy link
Member

Thanks for updating the tests. :)

@foolip
Copy link
Member

foolip commented Apr 24, 2023

@BorisChiou can we treat that as a thumbs from Gecko to update these tests? cc @jgraham

@nt1m @gsnedders can you take a look for WebKit?

@BorisChiou
Copy link
Member

Yes. Just a nitpick: perhaps we can tweak offset-path-ray-contain-001.html a little bit to avoid using non integer value, e.g. translate(64.645px, -164.645px) in the ref html.

Anyway, the current version of your patch (WPT part) looks good to me (from Gecko).

@foolip
Copy link
Member

foolip commented Apr 28, 2023

We discussed this in #323 and the summary is that while not everyone has studies the changes in detail, there was no objection to making the changes and following up as implementers uncover more issues in the course of working on the feature. (Which is business as usual.) But we wanted some paper trail of WebKit approval to update the tests, so I'll assign this to @nt1m.

@gsnedders
Copy link
Member

Spec: https://drafts.fxtf.org/motion/#valdef-ray-contain Test changes are due to the recent spec change: previously implied that the box should be inside the circle, formed by the radius of the ray length. Now it became easier as just reduces the ray length by a fixed value.

Is this primarily w3c/fxtf-drafts#363?

@danielsakhapov
Copy link
Author

@gsnedders yes, it is

@nt1m
Copy link
Member

nt1m commented Apr 28, 2023

Based on internal feedback, I think we can go ahead and adjust again later as we find issues when implementing.

@foolip
Copy link
Member

foolip commented May 2, 2023

Thank all, this proposal is approved based on feedback above. @danielsakhapov please feel free to land the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Motion Path test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

7 participants