809478 - CSS Gradients aren't rotated correctly when used with CSS transforms rotate(90deg) and rotate(270deg)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[vincent]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.2 Safari/537.17

Steps to reproduce:

I apply a linear-gradient from top to bottom on a div and rotate it with an angle of 90deg. 
Check: http://codepen.io/iamvdo/pen/FcKmb


Actual results:

The linear-gradient is still from top to bottom


Expected results:

The linear-gradient has to be from right to left

Comment 1

[Mardeg]

Attachment: Testcase: rotation with linear-gradient

I added a transition of one second on the rotation to see what the gradient does.
In Firefox 17 on XP I see the gradient rotate along with the div until the transition to rotate(90deg) ends, then the gradient resets to the pre-rotated rendering. At 180 degrees the gradient doesn't switch back after the transiton, and at 270 degrees the gradient switches to the 180 degree rotation after the transition.

In Opera the gradient rotates along with the div as expected and stays rotated.

Comment 2

[Mardeg]

Attachment: Testcase: linear-gradient rotations with 45 and 225 degrees added

The additional 45 and 225 degree rotations don't reset after the transition, so the bug only appears to happen on 90 and 270 degrees (I also confirmed 89 and 271 degree rotations don't have the bug).

Comment 3

[Mardeg]

Attachment: Testcase: linear-gradient rotations

Corrected mispaste of checked attributes.

Comment 4

[Mardeg]

Attachment: Testcase: linear-gradient rotations

Forgot to rename the fixed attachment, sorry.

Comment 5

[Mardeg]

Others on IRC confirmed the same behaviour on 32-bit Linux and Windows 8.

Comment 6

[Sander]

This worked okay in Firefox 16; it's a regression since 17.

Comment 7

[Sander]

This regresed on central between 2012-08-16-03-05-39 http://hg.mozilla.org/mozilla-central/rev/50e4ff05741e and 2012-08-17-03-05-55 http://hg.mozilla.org/mozilla-central/rev/a79132ac2f05

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50e4ff05741e&tochange=a79132ac2f05

roc: Based on checkin comments, I suspect this could be you from bug 779399?

Comment 8

[Mardeg]

Attachment: Testcase: rotation with linear-gradient

I made the transition optional, since the gradient switching bug after changing the degree of rotation is still visible without it, at least on my slow machine.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

We've already built and are about to release the 5th beta for 17.  How serious is this regression? Can we live with it for 6 weeks?  Is a straight-up backout of bug 779399 the solution (though it looks like doing that will just replace this regression with another)? What other options might we have this late in the beta cycle?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I *think* we can probably live with this regression on 17. I don't think we should back out 779399 at this point; that's known to cause problems on real Web sites and the impact of backing it would be unclear. AFAIK this bug hasn't shown up before and bug 779399 has been on nightly/Aurora since FF16 (it was backed out on FF16 release due to a different regression, now fixed).

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Thanks for weighing in, based on comment 10 I'll wontfix this for 17 and we can have time to try & test a proper fix for 18 in the coming weeks.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=5b018f367234

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reftests are green.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 679992 [details] [diff] [review]
fix

Review of attachment 679992 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2060,5 @@
>  
> +/**
> + * Return the transform matrix that maps aFrom to the rectangle defined by
> + * aToTopLeft/aToTopRight/aToBottomRight. The destination rectangle must be
> + * nonempty and must be axis-aligned.

Might be worth adding why it takes three points instead of two
as that took me a minute to figure out.

@@ +2064,5 @@
> + * nonempty and must be axis-aligned.
> + */
> +static gfxMatrix
> +TransformRectToRect(const gfxRect& aFrom, const gfxPoint& aToTopLeft,
> +                    const gfxPoint& aToTopRight, const gfxPoint& aToBottomRight)

It might be worth putting this in a more common place.

@@ +2076,5 @@
> +    m.x0 = aToTopLeft.x - m.xx*aFrom.x;
> +    m.y0 = aToTopLeft.y - m.yy*aFrom.y;
> +  } else {
> +    NS_ASSERTION(aToTopRight.y == aToBottomRight.y && aToTopRight.x == aToTopLeft.x,
> +                 "Destination rectangle not axis-aligned");

MOZ_ASSERT?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7789895e492d

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7789895e492d

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 679992 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779399
User impact if declined: CSS gradients on a few rare pages will be rendered incorrectly
Testing completed (on m-c, etc.): just landed on central
Risk to taking this patch (and alternatives if risky): Pretty low risk. We have reftests covering most of this stuff
String or UUID changes made by this patch: None

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 679992 [details] [diff] [review]
fix

[Triage Comment]
FF17 regression, and early enough in the cycle that we should be able to react to any new fallout prior to release. Approving for branches.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e08c004cd12c
https://hg.mozilla.org/releases/mozilla-beta/rev/0479c73e8afd