Chromium Code Reviews
Help | Chromium Project | Sign in
(135)

Issue 2482753002: Fix matrix3d transform under page zoom (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by Franklin Ta
Modified:
4 months, 2 weeks ago
Reviewers:
pdr., alancutter, enne
CC:
darktears, apavlov+blink_chromium.org, arv (Not doing code reviews), blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, Eric Willigers, rjwright, rwlbuis, shans
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix matrix3d transform under page zoom The math for adjusting a matrix3d transform when the page is zoomed is incorrect in at least 3 different places. There was a previous attempt at fixing this but they did it the same way as the 2d matrix case and forgot about the translateZ component (ceddaab902432728b12a17fcbf2c42283c7dfe68). But it would've been incorrect even if they did take translateZ into account. The real problem is that the last column can't be interpeted as the translation at all if the matrix is non-affine! To get the general correction for matrix3d, intuitively, we want to first scale down to the original units, do the transform, then scale back up. So checking with octave/matlab it should actually be: octave:1> pkg load symbolic; octave:2> A = sym('a', [3, 3]); B = sym('b', [3, 1]); C = sym('c', [1, 3]); D = sym('d', [1, 1]); s = sym('s'); octave:3> M = [A, B; C D] M = (sym 4×4 matrix) ⎡a₁₁ a₁₂ a₁₃ b₁₁⎤ ⎢ ⎥ ⎢a₂₁ a₂₂ a₂₃ b₂₁⎥ ⎢ ⎥ ⎢a₃₁ a₃₂ a₃₃ b₃₁⎥ ⎢ ⎥ ⎣c₁₁ c₁₂ c₁₃ d₁₁⎦ octave:4> scaleDown = [[eye(3) / s, zeros(3, 1)]; [zeros(1, 3), 1]] scaleDown = (sym 4×4 matrix) ⎡1 ⎤ ⎢─ 0 0 0⎥ ⎢s ⎥ ⎢ ⎥ ⎢ 1 ⎥ ⎢0 ─ 0 0⎥ ⎢ s ⎥ ⎢ ⎥ ⎢ 1 ⎥ ⎢0 0 ─ 0⎥ ⎢ s ⎥ ⎢ ⎥ ⎣0 0 0 1⎦ octave:5> scaleUp = [[s * eye(3), zeros(3, 1)]; [zeros(1, 3), 1]] scaleUp = (sym 4×4 matrix) ⎡s 0 0 0⎤ ⎢ ⎥ ⎢0 s 0 0⎥ ⎢ ⎥ ⎢0 0 s 0⎥ ⎢ ⎥ ⎣0 0 0 1⎦ octave:6> scaleUp * M * scaleDown ans = (sym 4×4 matrix) ⎡a₁₁ a₁₂ a₁₃ b₁₁⋅s⎤ ⎢ ⎥ ⎢a₂₁ a₂₂ a₂₃ b₂₁⋅s⎥ ⎢ ⎥ ⎢a₃₁ a₃₂ a₃₃ b₃₁⋅s⎥ ⎢ ⎥ ⎢c₁₁ c₁₂ c₁₃ ⎥ ⎢─── ─── ─── d₁₁ ⎥ ⎣ s s s ⎦ So in the above matrix, if it is affine (which is exactly when the `c` entries are 0 and `d` is 1) you can get away with just scaling the last column interpreting `b` as the translation. But for a general matrix3d you still need a few more divides on the bottom row! Sidenote: The perspective transform is the only other CSS transform that is non-affine and it gets the zoom division implicitly when the parameter is scaled up since it's in the form of: ⎡1 0 0 0⎤ ⎢ ⎥ ⎢0 1 0 0⎥ ⎢ ⎥ ⎢0 0 1 0⎥ ⎢ ⎥ ⎢ -1 ⎥ ⎢0 0 ─── 1⎥ ⎣ p ⎦ The reported issue is solved by fixing TransformBuilder as described above. But the same type of bug also appeared in getComputedStyle and animations with matrix3d. I also fixed them in this CL but a longer term solution should probably refactor all them (maybe into TransformOperation::zoom) so that this tricky bit of logic isn't duplicated in so many places. BUG=242685 R=pdr,alancutter Committed: https://crrev.com/6be6a794e7ea4dcc2b9e12da2aa02d00b3c84f1c Cr-Commit-Position: refs/heads/master@{#431208}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Add a unit test #

Patch Set 3 : Add Transformation::zoom #

Total comments: 2

Patch Set 4 : Add a comment for zoom #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -25 lines) Patch
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html View 1 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation-expected.html View 1 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-with-zoom-expected.txt View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/script-tests/computed-style-with-zoom.js View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/transforms/3d/general/cssmatrix-3d-zoom.html View 3 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/transforms/3d/general/cssmatrix-3d-zoom-expected.png View Binary file 0 comments Download
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp View 1 2 4 chunks +13 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp View 1 2 1 chunk +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/transforms/Matrix3DTransformOperation.cpp View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/transforms/TransformOperationsTest.cpp View 1 1 chunk +32 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/transforms/TransformationMatrix.h View 1 2 3 1 chunk +10 lines, -0 lines 1 comment Download
M third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 21 (6 generated)
Franklin Ta
4 months, 2 weeks ago (2016-11-06 17:33:25 UTC) #2
pdr.
+reviewer enne This is a pretty cool fix! Wonderful change description as well. https://codereview.chromium.org/2482753002/diff/1/AUTHORS File ...
4 months, 2 weeks ago (2016-11-08 07:04:08 UTC) #4
Franklin Ta
Only working on this on the side so I might be a bit slow with ...
4 months, 2 weeks ago (2016-11-08 07:52:18 UTC) #5
Franklin Ta
Minor comments https://codereview.chromium.org/2482753002/diff/1/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2482753002/diff/1/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp#newcode1352 third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1352: double zoomFactor = style.effectiveZoom(); On 2016/11/08 07:52:18, ...
4 months, 2 weeks ago (2016-11-08 16:47:02 UTC) #6
pdr.
On 2016/11/08 at 16:47:02, fta2012 wrote: > Minor comments > > https://codereview.chromium.org/2482753002/diff/1/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp > File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp ...
4 months, 2 weeks ago (2016-11-08 19:13:03 UTC) #7
Franklin Ta
On 2016/11/08 19:13:03, pdr. wrote: > On 2016/11/08 at 16:47:02, fta2012 wrote: > > Minor ...
4 months, 2 weeks ago (2016-11-08 20:03:10 UTC) #8
alancutter
Please add a new test case that fails prior to this patch. My dealings with ...
4 months, 2 weeks ago (2016-11-09 02:35:59 UTC) #9
Franklin Ta
Verified that all the test changes here do fail without the changes. - Added a ...
4 months, 2 weeks ago (2016-11-09 07:12:06 UTC) #10
alancutter
Non-owner lgtm. https://codereview.chromium.org/2482753002/diff/40001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp File third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp (right): https://codereview.chromium.org/2482753002/diff/40001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp#newcode1269 third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp:1269: TransformationMatrix& TransformationMatrix::zoom(double zoomFactor) { It might be ...
4 months, 2 weeks ago (2016-11-09 23:31:01 UTC) #11
pdr.
On 2016/11/09 at 23:31:01, alancutter wrote: > Non-owner lgtm. > > https://codereview.chromium.org/2482753002/diff/40001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp > File third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp ...
4 months, 2 weeks ago (2016-11-10 03:54:45 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2482753002/60001
4 months, 2 weeks ago (2016-11-10 04:19:24 UTC) #15
Franklin Ta
Thanks for the quick turnaround! https://codereview.chromium.org/2482753002/diff/40001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp File third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp (right): https://codereview.chromium.org/2482753002/diff/40001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp#newcode1269 third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp:1269: TransformationMatrix& TransformationMatrix::zoom(double zoomFactor) { ...
4 months, 2 weeks ago (2016-11-10 04:19:57 UTC) #16
alancutter
https://codereview.chromium.org/2482753002/diff/60001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.h File third_party/WebKit/Source/platform/transforms/TransformationMatrix.h (right): https://codereview.chromium.org/2482753002/diff/60001/third_party/WebKit/Source/platform/transforms/TransformationMatrix.h#newcode347 third_party/WebKit/Source/platform/transforms/TransformationMatrix.h:347: // Nice comment! I'd even put this in the ...
4 months, 2 weeks ago (2016-11-10 06:50:03 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 months, 2 weeks ago (2016-11-10 08:14:28 UTC) #19
commit-bot: I haz the power
4 months, 2 weeks ago (2016-11-10 08:16:22 UTC) #21
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/6be6a794e7ea4dcc2b9e12da2aa02d00b3c84f1c
Cr-Commit-Position: refs/heads/master@{#431208}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62