970294 - Perspective + Box-shadow leads to incorrect box size model when transform-ing

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Antwan]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140205162153

Steps to reproduce:

Created an fixed size object with these css properties applied :
- ouset box-shadow (either one of 4 props set)
- perspective
- transform3D with deg != 0%180

Test case here : http://jsfiddle.net/LPUTa/3/


Actual results:

Object is transformed as if its size was increased by the shadow's pixels


Expected results:

Object should transform using it's own size, and shadow should not interfere.

Notes:

1/ If the angle is multiple of 180, this doesn't happen. Therefore, an animation/transition using this combination of attributes appears flickering (for example : http://css3playground.com/3d-flip-cards/)

2/ This is a regression : was OK in Firefox 27.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Antwan, what part of the testcase is the part I should be looking at here?

What I see is that the behavior of the third div in the "perspective" set seems to depend on window width, and in particular on the line-wrapping(?).

Comment 2

[Antwan]

The 3rd element of the 2nd line should be positioned like the 1st element of the second line.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So for what it's worth, it doesn't seem to be in Firefox 27, for me...

Comment 4

[Bogdan Maris, Desktop QA]

I loaded the testcase in Firefox 26RC, 27RC, 28beta3, Aurora 29, Nightly 30, Internet Explorer and all browsers display the same result. Am I missing something?

Comment 5

[Bogdan Maris, Desktop QA]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #4)
> I loaded the testcase in Firefox 26RC, 27RC, 28beta3, Aurora 29, Nightly 30,
> Internet Explorer and all browsers display the same result.
The result that I`m talking about can be seen in this screenshot http://bit.ly/1nT3Owe

Comment 6

[Antwan]

Was happening only when hardware acceleration was disabled.
Try it on Firefox 27 vs Firefox 28 with both layers.acceleration.disabled:true.

As this setting is not defaulted, I'm not sure it's worth further investigation.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hardware accel can get disabled for all sorts of reasons....

Comment 8

[Bogdan Maris, Desktop QA]

Ok so with hardware acceleration off I was able to reproduce the issue and got this regression range:

Central:
Last good revision: 53d55d2d0a25 (2013-11-26)
First bad revision: 6ecf0c4dfcbe (2013-11-27)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=53d55d2d0a25&tochange=6ecf0c4dfcbe

Inbound:
Last good revision: c9ff42335f8a
First bad revision: 35014d18ff1f
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9ff42335f8a&tochange=35014d18ff1f

If you need any more assistance from QA please let me know.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Bogdan, that's awesome, thank you!

Comment 10

[Nick Cameron [:nrc]]

I expect this is due to a bug in Azure somewhere - i.e., previously we were using Thebes for drawing and pot-bug 932888 we use Azure for that and so it goes wrong. Passing over to mattwoodrow who knows all about Azure vs Thebes and the basic layer manager.

Comment 11

[Matt Woodrow (:mattwoodrow)]

I see what the bug is, caused by http://hg.mozilla.org/integration/mozilla-inbound/rev/c38b37bee015.

Previously we set a device offset on the intermediate surface, with Azure we instead set a transform on the context.

Unfortunately BasicLayerManager::PaintLayer can clobber the transform (but not the device offset of a gfxASurface), so this gets lost.

I'll have a think about the best way to fix this.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Use gfxContext's device offset functionality

This way the users of the gfxContext can't clobber our device offset.

Comment 13

[Nick Cameron [:nrc]]

Comment on attachment 8383220 [details] [diff] [review]
Use gfxContext's device offset functionality

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +959,5 @@
>      if (!untransformedDT) {
>        return;
>      }
>  
> +    nsRefPtr<gfxContext> groupTarget = new gfxContext(untransformedDT, Point(bounds.x, bounds.y));

nit: long line

::: gfx/thebes/gfxContext.h
@@ +50,5 @@
>       * Initialize this context from a DrawTarget.
>       * Strips any transform from aTarget.
>       * aTarget will be flushed in the gfxContext's destructor.
>       */
> +    gfxContext(mozilla::gfx::DrawTarget *aTarget, const mozilla::gfx::Point& aDeviceOffset = mozilla::gfx::Point());

nit: long line

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0be8765182a3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0be8765182a3

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8383220 [details] [diff] [review]
Use gfxContext's device offset functionality

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 932888.
User impact if declined: Incorrect 3d transform rendering without hardware acceleration.
Testing completed (on m-c, etc.): Been on m-c for a week, confirmed testcase 
Risk to taking this patch (and alternatives if risky): Low risk, just changes a transform matrix.
String or IDL/UUID changes made by this patch: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d788af3a9b20

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb47781c21ef

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/eb47781c21ef