Chromium Code Reviews

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

Created:
4 years, 1 month ago by Franklin Ta
Modified:
4 years, 1 month ago
Reviewers:
pdr., alancutter (OOO until 2018), enne (OOO)
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 Stats (+86 lines, -25 lines)
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments
M third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html View 1 chunk +5 lines, -0 lines 0 comments
M third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation-expected.html View 1 chunk +5 lines, -0 lines 0 comments
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-with-zoom-expected.txt View 1 chunk +1 line, -0 lines 0 comments
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/script-tests/computed-style-with-zoom.js View 1 chunk +1 line, -1 line 0 comments
M third_party/WebKit/LayoutTests/transforms/3d/general/cssmatrix-3d-zoom.html View 3 chunks +4 lines, -3 lines 0 comments
M third_party/WebKit/LayoutTests/transforms/3d/general/cssmatrix-3d-zoom-expected.png View Binary file 0 comments
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp View 1 2 4 chunks +13 lines, -15 lines 0 comments
M third_party/WebKit/Source/core/css/resolver/TransformBuilder.cpp View 1 2 1 chunk +3 lines, -4 lines 0 comments
M third_party/WebKit/Source/platform/transforms/Matrix3DTransformOperation.cpp View 1 2 1 chunk +1 line, -2 lines 0 comments
M third_party/WebKit/Source/platform/transforms/TransformOperationsTest.cpp View 1 1 chunk +32 lines, -0 lines 0 comments
M third_party/WebKit/Source/platform/transforms/TransformationMatrix.h View 1 2 3 1 chunk +10 lines, -0 lines 1 comment
M third_party/WebKit/Source/platform/transforms/TransformationMatrix.cpp View 1 2 1 chunk +10 lines, -0 lines 0 comments

Messages

Total messages: 21 (6 generated)
Franklin Ta
4 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month ago (2016-11-08 20:03:10 UTC) #8
alancutter (OOO until 2018)
Please add a new test case that fails prior to this patch. My dealings with ...
4 years, 1 month 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 years, 1 month ago (2016-11-09 07:12:06 UTC) #10
alancutter (OOO until 2018)
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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month ago (2016-11-10 04:19:57 UTC) #16
alancutter (OOO until 2018)
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 years, 1 month ago (2016-11-10 06:50:03 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-11-10 08:14:28 UTC) #19
commit-bot: I haz the power
4 years, 1 month 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}

Powered by Google App Engine