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

CSS 'transformation: skew()' should take <angle> type as parameters #6238

Closed
wants to merge 1 commit into from

Conversation

@Jinwoo-Song
Copy link
Contributor

Jinwoo-Song commented Jun 1, 2015

Current implementation is taking type as parameter so skew()
does not work properly. Let the skew() to get as specification
described.

Fixes #6237.

r? @pcwalton
cc @yichoi

Review on Reviewable

@nox nox self-assigned this Jun 1, 2015
@nox
Copy link
Member

nox commented Jun 1, 2015

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/style/properties.mako.rs @ r1
  • tests/ref/basic.list @ r1
  • tests/ref/transform_skew_a.html @ r1
  • tests/ref/transform_skew_ref.html @ r1

components/style/properties.mako.rs, line 3267 [r1] (raw file):
The spec says "specifies a 2D skew by [ax,ay] for X and Y. If the second parameter is not provided, it has a zero value."


components/style/properties.mako.rs, line 3461 [r1] (raw file):
Out of curiosity, why do we need to subtract it from PI_2 here and in the Rotate case?


Comments from the review on Reviewable.io

Current implementation is taking <number> type as parameter so skew()
does not work properly. Let the skew() to get <angle> as specification
described.

Fixes #6237.
@Jinwoo-Song Jinwoo-Song force-pushed the Jinwoo-Song:skew branch from c185321 to 2c83111 Jun 2, 2015
@Jinwoo-Song
Copy link
Contributor Author

Jinwoo-Song commented Jun 2, 2015

Review status: 3 of 4 files reviewed, 2 unresolved discussions, all commit checks successful.


components/style/properties.mako.rs, line 3267 [r1] (raw file):
Done. You're right! I fixed the code.


components/style/properties.mako.rs, line 3461 [r1] (raw file):
It seems that it's to apply the clockwise angle. If we do not subtract, the rotation and skew are done in the counter-clockwise.


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 2, 2015

@bors-servo: r+


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/style/properties.mako.rs @ r2

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

📌 Commit 2c83111 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

Testing commit 2c83111 with merge 2356273...

bors-servo pushed a commit that referenced this pull request Jun 2, 2015
Current implementation is taking <number> type as parameter so skew()
does not work properly. Let the skew() to get <angle> as specification
described.

Fixes #6237.

r? @pcwalton 
cc @yichoi

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6238)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 2, 2015

💔 Test failed - mac2

@nox
Copy link
Member

nox commented Jun 2, 2015

This isn't working.

failures:
    transform_skew_a.html == transform_skew_ref.html
@nox
Copy link
Member

nox commented Jun 3, 2015

The problem is that tan(0.3rad) ≠ 0.3 and tan(0.5rad) ≠ 0.5.

@nox
Copy link
Member

nox commented Jun 3, 2015

servo-reftest-000292-diff

@nox
Copy link
Member

nox commented Jun 3, 2015

Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.


components/style/properties.mako.rs, line 3170 [r2] (raw file):
Nit: rename theta_x and theta_y to alpha and beta like in the spec, and include link to it:

http://dev.w3.org/csswg/css-transforms/#SkewDefined


components/style/properties.mako.rs, line 3171 [r2] (raw file):
beta.tan()


components/style/properties.mako.rs, line 3172 [r2] (raw file):
alpha.tan()


components/style/properties.mako.rs, line 3461 [r2] (raw file):
Remove the subtraction from PI_2 here and you don't need to negate the angles in ComputedMatrix::skew().


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2015

The latest upstream changes (presumably #6161) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox added the S-needs-rebase label Jun 3, 2015
@Jinwoo-Song
Copy link
Contributor Author

Jinwoo-Song commented Jun 3, 2015

Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.


components/style/properties.mako.rs, line 3170 [r2] (raw file):
Done.


components/style/properties.mako.rs, line 3461 [r2] (raw file):
Done.


Comments from the review on Reviewable.io

@Jinwoo-Song
Copy link
Contributor Author

Jinwoo-Song commented Jun 3, 2015

I'll update the patch to apply nox's comments. As the latest upstream code has been changed, I need to change rust-geom package as well. Rebased patch will be uploaded after below patch is merged.
servo/euclid#77


Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.


components/style/properties.mako.rs, line 3171 [r2] (raw file):
Done.


components/style/properties.mako.rs, line 3172 [r2] (raw file):
Done.


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 3, 2015

I think you forgot to push the changes here. :)

@Jinwoo-Song
Copy link
Contributor Author

Jinwoo-Song commented Jun 3, 2015

Yes, I did not push the changes yet. I'll do it after the patch in rust-geom is landed for updating the cargo file.

@nox
Copy link
Member

nox commented Aug 27, 2015

@Jinwoo-Song Ping?

bors-servo pushed a commit to servo/euclid that referenced this pull request Sep 11, 2015
bors-servo
Apply skew transformation for alpha and beta angles.

This patch is to fix servo/servo#6238.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/euclid/100)
<!-- Reviewable:end -->
@nox nox reopened this Sep 11, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Sep 11, 2015

🔒 Merge conflict

@nox
Copy link
Member

nox commented Sep 11, 2015

Fixed at master...nox:skew, but is currently blocked by servo/surfman#28.

@nox
Copy link
Member

nox commented Sep 11, 2015

Superseded by #7605, thanks for the patch @Jinwoo-Song!

@nox nox closed this Sep 11, 2015
@nox nox removed the S-needs-new-owner label Sep 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.