1459403 - Some animations in the test-case in bug 1458715 still look bogus.

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Test-case is https://bugzilla.mozilla.org/attachment.cgi?id=8972728.

Quoting:

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from bug 1458715 comment #23)
> Yes, the eventual state is correct on Nightly.
> 
> So I assume we are looking at 3 bugs here?
> 
> 1. For box 1, transition b/t two states seems "crazy" and off.
> 2. For box 2, transition when the cursor leaves the box did not end at the
> right size.

I _think_ they're related, and that the crazy transition in the first box is because we mouse over the inner div because of the wrong scaling. But I need to investigate.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Hiro, Manish, any chance you could get to this sooner than me? Otherwise I'll investigate when I'm back from PTO.

Thanks!

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: testcase-1.html

Test case 1: the transitions in this testcase did not start nor end at the right place on Firefox release.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: testcase-2.html

Test case 2: setting transfrom in rAF loop during the transition generates "crazy" transition.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't have access to Nightly right now so probably one of these got fixed in bug 1458715 already.

Let me know if I missed anything.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8973479 [details]
testcase-2.html

I can see this is gone on Nightly, but I can still trigger the same "crazy" transition on the original test case.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So, I took a look, the problematic animation is:

  [Perspective(CSSPixelLength(500.0)), TranslateZ(CSSPixelLength(30.0))], [Perspective(CSSPixelLength(500.0)), RotateX(Deg(29.8)), RotateY(Deg(23.0)), TranslateZ(CSSPixelLength(-60.0))]

Making the operations match patching the test-case as follows:

diff --git a/t_.html b/t_.html
index 982b766..0f2b66d 100644
--- a/t_.html
+++ b/t_.html
@@ -21,22 +21,22 @@
  left:10px;
  display:block;
  height:280px;
  width:280px;
  transition:transform .3s ease-in-out;
 }
 
 .pic-scene {
- transform:perspective(500px) translateZ(30px);
+ transform:perspective(500px) rotateX(0) rotateY(0) translateZ(30px);
  background: rgba(0,128,0,0.5);
 }
 
 .pic-foreground {
- transform:perspective(500px) translateZ(30px);
+ transform:perspective(500px) rotateX(0) rotateY(0) translateZ(30px);
  background: rgba(0,0,128,0.5);
 }
 
 #motion-pic.active .pic-scene,
 #motion-pic.active .pic-foreground {
 }
 
 #motion-pic2.active .pic-scene,

Makes it behave as expected.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Hiro, per ^ I think this may be a bug either in nsStyleTransforMatrix, or that we should be matching the transforms with zeros alternatively.

Do you know what's the expected outcome there?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Hmm, so I found a "fix":

diff --git a/layout/style/nsStyleTransformMatrix.cpp b/layout/style/nsStyleTransformMatrix.cpp
index 01cbb9b1cc95..8bfff51490e3 100644
--- a/layout/style/nsStyleTransformMatrix.cpp
+++ b/layout/style/nsStyleTransformMatrix.cpp
@@ -518,22 +518,22 @@ ProcessMatrixOperator(Matrix4x4& aMatrix,
 
   Matrix4x4 matrix1 = readTransform(aData->Item(1));
   Matrix4x4 matrix2 = readTransform(aData->Item(2));
   double progress = aData->Item(3).GetPercentValue();
 
   // We cannot use GeckoComputedStyle to check if we use Servo backend because
   // it could be null in Gecko. Instead, use the unit of the nsCSSValue because
   // we use eCSSUnit_SharedList for Servo backend.
-  if (aData->Item(1).GetUnit() == eCSSUnit_SharedList) {
-    aMatrix =
-      OperateTransformMatrixByServo<Operator>(matrix1, matrix2, progress)
-        * aMatrix;
-    return;
-  }
+  // if (aData->Item(1).GetUnit() == eCSSUnit_SharedList) {
+  //   aMatrix =
+  //     OperateTransformMatrixByServo<Operator>(matrix1, matrix2, progress)
+  //       * aMatrix;
+  //   return;
+  // }
 
   aMatrix =
     OperateTransformMatrix<Operator>(matrix1, matrix2, progress) * aMatrix;
 }
 
 /* Helper function to process two matrices that we need to interpolate between */
 void
 ProcessInterpolateMatrix(Matrix4x4& aMatrix,

Investigating what's the bogus part here...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

So I got this. Our transform code is... extra-fun.

This ended up having a lot of cleanup and such. I'll land pretty much each patch in a separate bug for regression-tracking and such, but I'll post them all here because laziness.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Trivial cleanup.

Review commit: https://reviewboard.mozilla.org/r/247080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247080/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Cleanup and add references to Quaternion::animate.

No functional change, but I did this while searching for the bug.

Review commit: https://reviewboard.mozilla.org/r/247082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247082/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Cleanup and add references to decompose_3d_matrix.

Same, no functional change, but I basically rewrote this two times so...

Review commit: https://reviewboard.mozilla.org/r/247084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247084/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Add a FIXME comment which I think reveals a bug but I haven't confirmed it.

Review commit: https://reviewboard.mozilla.org/r/247086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247086/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Cleanup multiply().

We assign all the members, the result matrix doesn't really need to be any clone
of a.

Review commit: https://reviewboard.mozilla.org/r/247088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247088/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: Move skew and rotation application to their own scope for clarity.

The skew resetting of temp I think fixes a bug in presence of skew in every
direction, but again haven't double-checked.

Review commit: https://reviewboard.mozilla.org/r/247090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247090/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459403: The fix: scale and translate appropriately.

Loops are hard. This hasn't been the funniest Saturday evening.

Review commit: https://reviewboard.mozilla.org/r/247092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247092/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Phew, I didn't expect to have to do quaternion math this evening :)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980910 [details]
Bug 1459403: Cleanup and add references to decompose_3d_matrix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247084/diff/1-2/

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980911 [details]
Bug 1459403: Add a FIXME comment which I think reveals a bug but I haven't confirmed it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247086/diff/1-2/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980912 [details]
Bug 1459403: Cleanup multiply().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247088/diff/1-2/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980913 [details]
Bug 1459403: Move skew and rotation application to their own scope for clarity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247090/diff/1-2/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980914 [details]
Bug 1459403: The fix: scale and translate appropriately.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247092/diff/1-2/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980908 [details]
Bug 1459403: Trivial cleanup.

https://reviewboard.mozilla.org/r/247080/#review253146

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980909 [details]
Bug 1459403: Cleanup and add references to Quaternion::animate.

https://reviewboard.mozilla.org/r/247082/#review253150

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1832
(Diff revision 1)
> -                      "animate should only be used for interpolating or accumulating transforms");
> +            "animate should only be used for interpolating or accumulating transforms",
> +        );
>  
> -        // We take a specialized code path for accumulation (where other_weight is 1)
> -        if other_weight == 1.0 {
> +        // We take a specialized code path for accumulation (where other_weight
> +        // is 1).
> +        if let Procedure::Accumulate { .. } = procedure {

Nit: Please add debug_assert!(other_weight == 1.0).

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1873
(Diff revision 1)
> -
> +            .min(1.0).max(-1.0);
> -        // Clamp product to -1.0 <= product <= 1.0
> -        product = product.min(1.0);
> -        product = product.max(-1.0);
>  
> -        if product == 1.0 {
> +        if dot == 1.0 {

I'd prefer 'product' since it matches the name in the spec.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980910 [details]
Bug 1459403: Cleanup and add references to decompose_3d_matrix.

https://reviewboard.mozilla.org/r/247084/#review253152

Nice cleanup!

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1911
(Diff revision 2)
>          Ok(SquaredDistance::from_sqrt(distance))
>      }
>  }
>  
>  /// Decompose a 3D matrix.
>  /// <https://drafts.csswg.org/css-transforms/#decomposing-a-3d-matrix>

Nit: Let's fix the spec link too.
https://drafts.csswg.org/css-transforms-2/#decomposing-a-3d-matrix

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980911 [details]
Bug 1459403: Add a FIXME comment which I think reveals a bug but I haven't confirmed it.

https://reviewboard.mozilla.org/r/247086/#review253154

Yeah, this looks a pure bug, I believe ComputedScale shouldn't have the special handling for addition.  I did look at bug 1207734, but I couldn't find any comments about this.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980912 [details]
Bug 1459403: Cleanup multiply().

https://reviewboard.mozilla.org/r/247088/#review253156

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980913 [details]
Bug 1459403: Move skew and rotation application to their own scope for clarity.

https://reviewboard.mozilla.org/r/247090/#review253158

Indeed, the previous code smells (a) bug(s).  And this new code looks more robust.
We might need to raise a spec isuee for the original pseudo code.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980914 [details]
Bug 1459403: The fix: scale and translate appropriately.

https://reviewboard.mozilla.org/r/247092/#review253160

This looks overkill for me.  matrix.m44 is not used for translation, I think.   And matrix.mx4 is not used scale either.  Emilio, is this change really needed to fix the issue?

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980914 [details]
Bug 1459403: The fix: scale and translate appropriately.

https://reviewboard.mozilla.org/r/247092/#review253160

Pretty sure it is, and that the translation change is the _only_ change required to fix this. Note that matrix is what we return.

`matrix.m44` is not used for translation, but it's used for perspective.

Before this change, `matrix.m44` ended up being higher from the perspective application phase, but with this change it would have eneded up being 1.

You can test this in a funny way (which is what I used to debug), enabling both codepaths and comparing:

```
diff --git a/layout/style/nsStyleTransformMatrix.cpp b/layout/style/nsStyleTransformMatrix.cpp
index 01cbb9b1cc95..9bd56dffaa40 100644
--- a/layout/style/nsStyleTransformMatrix.cpp
+++ b/layout/style/nsStyleTransformMatrix.cpp
@@ -518,25 +518,23 @@ ProcessMatrixOperator(Matrix4x4& aMatrix,
 
   Matrix4x4 matrix1 = readTransform(aData->Item(1));
   Matrix4x4 matrix2 = readTransform(aData->Item(2));
   double progress = aData->Item(3).GetPercentValue();
 
   // We cannot use GeckoComputedStyle to check if we use Servo backend because
   // it could be null in Gecko. Instead, use the unit of the nsCSSValue because
   // we use eCSSUnit_SharedList for Servo backend.
-  if (aData->Item(1).GetUnit() == eCSSUnit_SharedList) {
-    aMatrix =
-      OperateTransformMatrixByServo<Operator>(matrix1, matrix2, progress)
-        * aMatrix;
-    return;
-  }
+  auto servo =
+      OperateTransformMatrixByServo<Operator>(matrix1, matrix2, progress);
+  auto gecko =
+      OperateTransformMatrix<Operator>(matrix1, matrix2, progress);
 
-  aMatrix =
-    OperateTransformMatrix<Operator>(matrix1, matrix2, progress) * aMatrix;
+  MOZ_ASSERT(gecko.FuzzyEquals(servo));
+  aMatrix = servo * aMatrix;
 }
 
 /* Helper function to process two matrices that we need to interpolate between */
 void
 ProcessInterpolateMatrix(Matrix4x4& aMatrix,
                          const nsCSSValue::Array* aData,
                          TransformReferenceBox& aRefBox,
                          bool* aContains3dTransform)
```

If you print the process of recomposition to stdout you'll see how you before you crash and  the perspective component ends up varying because of the lack of this change.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980914 [details]
Bug 1459403: The fix: scale and translate appropriately.

I didn't realize the spec has pseudo-code describing what we were doing in:

  https://drafts.csswg.org/css-transforms-2/#recomposing-to-a-3d-matrix

I'll file a spec bug, since the spec has the same bug as we do.

Re-requesting review based on the previous comment and the verification that reverting this part brings the bug back :)

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8980914 [details]
Bug 1459403: The fix: scale and translate appropriately.

https://reviewboard.mozilla.org/r/247092/#review253176

Oh right, indeed.  This is actually what old gecko style system does (I did check Matrix4x4Typed::PreTranslate and Matrix4x4::PreScale).

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Relevant spec PRs:

  https://github.com/w3c/csswg-drafts/pull/2711
  https://github.com/w3c/csswg-drafts/pull/2712
  https://github.com/w3c/csswg-drafts/pull/2713

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb61509bd148
Trivial cleanup. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59356b84fcc
Cleanup and add references to Quaternion::animate. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9099625aff2
Cleanup and add references to decompose_3d_matrix. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32d6cdc2c97
Add a FIXME comment which I think reveals a bug but I haven't confirmed it. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3548d0baf6
Cleanup multiply(). r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4930c5543b
Move skew and rotation application to their own scope for clarity. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d58738b1b97d
The fix: scale and translate appropriately. r=hiro

Comment 35

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/bb61509bd148
https://hg.mozilla.org/mozilla-central/rev/c59356b84fcc
https://hg.mozilla.org/mozilla-central/rev/b9099625aff2
https://hg.mozilla.org/mozilla-central/rev/d32d6cdc2c97
https://hg.mozilla.org/mozilla-central/rev/1f3548d0baf6
https://hg.mozilla.org/mozilla-central/rev/4c4930c5543b
https://hg.mozilla.org/mozilla-central/rev/d58738b1b97d

Comment 36

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 61.0a1 (2018.05.05) on Windows 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 62.0b8 on Ubuntu 16.04 x64, Windows  8.1 x64 and Mac OS X 10.13.5, now the animations issue in test cases works as in Safari/Chrome.