Closed Bug 781701 Opened 12 years ago Closed 10 years ago

rotate3d rotates about wrong diagonal axis when rotating 180deg


(Core :: CSS Parsing and Computation, defect)

17 Branch
Not set



Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified


(Reporter: trevdc, Assigned: mattwoodrow)



(Keywords: testcase)


(2 files)

Attached file 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.
Attachment #650741 - Attachment mime type: text/plain → text/html
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.
Keywords: testcase
OS: Mac OS X → All
Component: Untriaged → Style System (CSS)
Product: Firefox → Core
Matt, can you take a look?
Sounds like Matt went up in smoke.

Here is a fiddle illustrating the issue.
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.

Assignee: nobody → matt.woodrow
Are the changes to nsStyleTransformMatrix just cleanup, or is there somthing substantive in there?
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
Just cleanup.
Flags: needinfo?(matt.woodrow)
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
Attachment #8474327 - Flags: review?(dbaron) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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.
Attachment #8474327 - Flags: approval-mozilla-beta?
Attachment #8474327 - Flags: approval-mozilla-aurora?
Attachment #8474327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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?
Flags: needinfo?(matt.woodrow)
Yeah, that's absolutely fine.
Flags: needinfo?(matt.woodrow)
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.
Attachment #8474327 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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?
Flags: needinfo?(matt.woodrow)
This has landed in both 34 and 35, so it should be fixed there.
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(dbaron)
Floating point issues seem most likely; transferring ni? back to matt, though.
Flags: needinfo?(dbaron) → needinfo?(matt.woodrow)
Could you file a separate bug on the remaining issue, though?
Flags: needinfo?(bogdan.maris)
See Also: → 1067286
(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.
Flags: needinfo?(bogdan.maris)
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.