Closed Bug 1459403 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- verified

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(8 files, 1 obsolete file)

testcase-1.html
2.71 KB, text/html
Details
Bug 1459403: Trivial cleanup.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: Cleanup and add references to Quaternion::animate.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: Cleanup and add references to decompose_3d_matrix.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: Add a FIXME comment which I think reveals a bug but I haven't confirmed it.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: Cleanup multiply().
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: Move skew and rotation application to their own scope for clarity.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1459403: The fix: scale and translate appropriately.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
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.
Hiro, Manish, any chance you could get to this sooner than me? Otherwise I'll investigate when I'm back from PTO.

Thanks!
Flags: needinfo?(emilio)
Attached file testcase-1.html — 
Test case 1: the transitions in this testcase did not start nor end at the right place on Firefox release.
Attached file testcase-2.html (obsolete) — 
Test case 2: setting transfrom in rAF loop during the transition generates "crazy" transition.
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.
Priority: -- → P3
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.
Attachment #8973479 - Attachment is obsolete: true
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.
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?
Flags: needinfo?(emilio) → needinfo?(hikezoe)
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...
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.
Assignee: nobody → emilio
Flags: needinfo?(hikezoe)
Phew, I didn't expect to have to do quaternion math this evening :)
Comment on attachment 8980908 [details]
Bug 1459403: Trivial cleanup.

https://reviewboard.mozilla.org/r/247080/#review253146
Attachment #8980908 - Flags: review?(hikezoe) → review+
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.
Attachment #8980909 - Flags: review?(hikezoe) → review+
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
Attachment #8980910 - Flags: review?(hikezoe) → review+
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.
Attachment #8980911 - Flags: review?(hikezoe) → review+
Comment on attachment 8980912 [details]
Bug 1459403: Cleanup multiply().

https://reviewboard.mozilla.org/r/247088/#review253156
Attachment #8980912 - Flags: review?(hikezoe) → review+
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.
Attachment #8980913 - Flags: review?(hikezoe) → review+
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?
Attachment #8980914 - Flags: review?(hikezoe)
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 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 :)
Attachment #8980914 - Flags: review?(hikezoe)
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).
Attachment #8980914 - Flags: review?(hikezoe) → review+
Pushed by ecoal95@gmail.com:
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
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: