rotate3d rotates about wrong diagonal axis when rotating 180deg

VERIFIED FIXED in Firefox 33

Status

()

VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: trevdc, Assigned: mattwoodrow)

Tracking

({testcase})

17 Branch
mozilla34
x86
All
testcase
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox31 wontfix, firefox32 wontfix, firefox33 verified, firefox34 verified, firefox35 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 650741 [details]
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.
(Reporter)

Updated

6 years ago
Attachment #650741 - Attachment mime type: text/plain → text/html

Comment 1

6 years ago
Bug 769892 ?
(Reporter)

Comment 2

6 years ago
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?

Comment 4

4 years ago
Sounds like Matt went up in smoke.

Here is a fiddle illustrating the issue.

http://jsfiddle.net/vuyhexqv/
(Assignee)

Comment 5

4 years ago
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
Assignee: nobody → matt.woodrow
(Assignee)

Comment 6

4 years ago
Created attachment 8474327 [details] [diff] [review]
Interpolate rotate3d if the vectors match
Attachment #8474327 - Flags: review?(dbaron)
Are the changes to nsStyleTransformMatrix just cleanup, or is there somthing substantive in there?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 8

4 years ago
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+
(Assignee)

Updated

4 years ago
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
https://hg.mozilla.org/mozilla-central/rev/309b1499160d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Comment 12

4 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e4f0cb37da1
status-firefox31: affected → wontfix
status-firefox33: affected → fixed
status-firefox34: affected → fixed
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)
(Assignee)

Comment 15

4 years ago
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?
status-firefox33: fixed → verified
status-firefox34: fixed → affected
status-firefox35: --- → ?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 18

4 years ago
This has landed in both 34 and 35, so it should be fixed there.
Flags: needinfo?(matt.woodrow)
status-firefox32: affected → wontfix
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)

Updated

4 years ago
See Also: → bug 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.
Status: RESOLVED → VERIFIED
status-firefox34: affected → verified
status-firefox35: ? → verified
Flags: needinfo?(bogdan.maris)

Updated

4 years ago
Duplicate of this bug: 1081116
(Assignee)

Updated

4 years ago
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.