781701 - rotate3d rotates about wrong diagonal axis when rotating 180deg

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Trevor Cook]

Attachment: simplified test

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120809030541

Steps to reproduce:

Created a rotation transition using rotate3d, with a negative value as one of the vector coordinates (e.g. -1, 1, 0) to perform a diagonal rotation, using 180deg as the angle. In the attached test case, hover over the circle enclosed in the green box to observe the transition.

I could not reproduce this using any other value for the angle. Using 179deg, 181deg, and 3.1415rad as angle values results in the expected behavior.


Actual results:

The rotation is performed about the opposite axis, as if it is ignoring the negative value and interpreting it as a positive value. e.g. using -1, 1, 0 the rotation is performed about the Northeast/Southwest axis.


Expected results:

The rotation should be performed about the correct axis, e.g. using -1, 1, 0 the rotation should be performed about the Northwest/Southeast axis.

Comment 1

[Alice0775 White]

Bug 769892 ?

Comment 2

[Trevor Cook]

This bug has to do with rotation using the incorrect axis, while that bug has to do with rotation transitioning between angles in the wrong way, so I don't think these two bugs are related.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Matt, can you take a look?

Comment 4

[agamemnus]

Sounds like Matt went up in smoke.

Here is a fiddle illustrating the issue.

http://jsfiddle.net/vuyhexqv/

Comment 5

[Matt Woodrow (:mattwoodrow)]

Oops, looks like this slipped through the cracks.

Looks like we're not implementing this bit of the spec [1]:

"For interpolations with the primitive rotate3d(), the direction vectors of the transform functions get normalized first. If the normalized vectors are equal, the rotation angle gets interpolated numerically. Otherwise the transform functions get converted into 4x4 matrices first and interpolated as defined in section Interpolation of Matrices afterwards. "

Patch incoming.


[1] http://www.w3.org/TR/css3-transforms/#interpolation-of-transform-functions

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Interpolate rotate3d if the vectors match

Comment 7

[David Baron :dbaron:]

Are the changes to nsStyleTransformMatrix just cleanup, or is there somthing substantive in there?

Comment 8

[Matt Woodrow (:mattwoodrow)]

Just cleanup.

Comment 9

[David Baron :dbaron:]

Comment on attachment 8474327 [details] [diff] [review]
Interpolate rotate3d if the vectors match

>+          arr = StyleAnimationValue::AppendTransformFunction(tfunc, resultTail);

Please comment here that you skipped the AppendTransformFunction above for all rotate3d cases, so you need to do it now.

>+          arr->Item(1).SetFloatValue(vector1.x, eCSSUnit_Number);
>+          arr->Item(2).SetFloatValue(vector1.y, eCSSUnit_Number);
>+          arr->Item(3).SetFloatValue(vector1.z, eCSSUnit_Number);
>+
>+          AddCSSValueAngle(aCoeff1, a1->Item(4), aCoeff2, a2->Item(4),
>+                           arr->Item(4));
>+          break;
>+        }

"// FALL THROUGH" comment here.  In all caps.

>+      }



r=dbaron with that

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/309b1499160d

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/309b1499160d

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8474327 [details] [diff] [review]
Interpolate rotate3d if the vectors match

Approval Request Comment
[Feature/regressing bug #]: Not a regression, this has existed since 3d transforms was first added. We're just not implementing the 3d transforms spec correctly and it would be nice to get it fixed.
[User impact if declined]: Incorrect animation/transition behaviour for some values of rotate3d.
[Describe test coverage new/current, TBPL]: Tested manually.
[Risks and why]: Very low risk, just a small change to the way we interpolate value for rotate3d.
[String/UUID change made/needed]: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e4f0cb37da1

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Matt - Even though this is a relatively small fix, given the amount of work that needs to land today, I'm reluctant to take anything that isn't required. As this has existed for quite a while, I think it falls in that boat. Are you OK with waiting until 33 to get this fix out?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Yeah, that's absolutely fine.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8474327 [details] [diff] [review]
Interpolate rotate3d if the vectors match

Beta- (see comment 14 and comment 15). This has already landed in 33.

Comment 17

[Bogdan Maris [:bogdan_maris], Desktop QA]

I tested this issue on old Nightly 34.0a1 (2014-08-20) and it`s not fixed there, Aurora 33.0a2 (2014-08-20) looks fine also verified that using Firefox 33 beta 1 the issue looks fixed. Latest Nightly 35.0a1 and Aurora 34.0a2 still have this issue so, based on comment 14, will this fix be planed to land in 34 and 35 as well if so we should track it for 35?

Comment 18

[Matt Woodrow (:mattwoodrow)]

This has landed in both 34 and 35, so it should be fixed there.

Comment 19

[Bogdan Maris [:bogdan_maris], Desktop QA]

Just tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.10 using latest Nightly 35.0a1 (2014-09-14) and latest Aurora 34.0a2 (2014-09-14). The issues seems fixed only on Windows and Mac, it can still be reproduced on Ubuntu using Firefox 34 and 35. 

David, can you take a look at this since Matt is on PTO? Or if you could point to someone who can.

Comment 20

[David Baron :dbaron:]

Floating point issues seem most likely; transferring ni? back to matt, though.

Comment 21

[David Baron :dbaron:]

Could you file a separate bug on the remaining issue, though?

Comment 22

[Bogdan Maris [:bogdan_maris], Desktop QA]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #21)
> Could you file a separate bug on the remaining issue, though?

Seems that Alice already logged a bug on that so the remaining issue will be tracked there.
I will close this issue as verified fixed.