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

Upstream changes made to rotate CSS property parsing test in WebKit r276554 #28691

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

graouts
Copy link
Contributor

@graouts graouts commented Apr 26, 2021

WebKit was a bit silly and specifically looked for a 1 value in the rotation vector to determine the axis. The was addressed in r276554 with some changes to css/css-transforms/parsing/rotate-parsing-valid.html.

Of note, this new test:

test_valid_value("rotate", "0 0 0 400grad", "0 0 0 400grad");

This passes in Chrome and ToT WebKit but fails in Firefox which returns the value x 400grad. I believe the test to be correct and Firefox to be incorrect.

Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

This mostly looks good, modulo the comment below... which I guess is only partly related, so I'm ok with landing this and fixing that in a followup as well if you prefer.

Though I wonder if this was reviewed as part of WebKit's review process, and, if so, whether it should be auto-imported without review in the WPT repo, like such changes made through the Mozilla or Chromium review processes.

test_valid_value("rotate", "0 1 0 400grad", "y 400grad");

test_valid_value("rotate", "z 400grad");
test_valid_value("rotate", "400grad z", "z 400grad");
test_valid_value("rotate", "0 0 0.5 400grad", "z 400grad");
test_valid_value("rotate", "0 0 1 400grad", "z 400grad");
Copy link
Member

Choose a reason for hiding this comment

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

These 4 tests are all invalid, I think, based on the spec edits made in w3c/csswg-drafts#6147 for w3c/csswg-drafts#3305.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I should clarify that they're invalid, I think, because the z should be omitted in the serialization.

Copy link
Contributor Author

@graouts graouts Apr 26, 2021

Choose a reason for hiding this comment

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

This is great! But I think this should indeed be changed in a different PR which I'm happy to file after this one lands (and fix WebKit too).

@graouts
Copy link
Contributor Author

graouts commented Apr 26, 2021

Though I wonder if this was reviewed as part of WebKit's review process, and, if so, whether it should be auto-imported without review in the WPT repo, like such changes made through the Mozilla or Chromium review processes.

It was reviewed, see WebKit bug 225019. But since WebKit doesn't yet have an auto-import process, I don't mind gathering additional review feedback.

@marcoscaceres marcoscaceres removed their request for review April 27, 2021 00:29
@graouts graouts merged commit 3b5de35 into web-platform-tests:master Apr 27, 2021
@graouts graouts deleted the upstream-wk-r276554 branch April 27, 2021 12:49
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.

3 participants