622818 - Background image on elements with a css skew transform applied doesn't render completely

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Les James]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.231 Safari/534.10
Build Identifier: Firefox 4b8

I skewed two divs with css transform. Everything displays correctly when only applying background colors to the divs. When applying a background image in FF4 the background image doesn't fill the entire height of the skewed div. This seems to be a regression because it displays correctly in FF 3.6. Please see http://design.lesjames.com/post/2585617458/webkit-skew-and-background-images for examples.

Reproducible: Always

Comment 1

[Les James]

Update: the rendering error occurs when the background attachement is set to fixed. When set to scroll the image fills the entire area of the skewed div.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Seeing this regressional Behavior with Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110103 Firefox/4.0b9pre ID:20110103030359 too.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Image for testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

The behavior changed in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3ddfa411f716&tochange=d253c44465ae

And fwiw, I think this was in the right place.  There are painting changes in the range, but not CSS changes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This seems to be a regression from bug 593839 part 1.  If I change the if-condition changed in nsCSSRendering.cpp in that diff <http://hg.mozilla.org/mozilla-central/rev/fbdc3be86de4> to always be false, we get the 3.6 behavior.  roc, were we setting DESTINED_FOR_SCREEN to false when painting transformed stuff?  Seems like we should be; the testcase is showing us clipping to viewport _before_ transforming.

Comment 7

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

I think that change is correct. We are indeed painting to the window. Here we should have an additional check that we have no transformed ancestor.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 9

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

Comment on attachment 502328 [details] [diff] [review]
patch

+    if (aFlags & nsCSSRendering::PAINTBG_TO_WINDOW &&
+        !aFlags & nsCSSRendering::PAINTBG_IN_TRANSFORM) {

Put () around the & subexpressions, I think precedence is all wrong here

Comment 10

[Boris Zbarsky [:bzbarsky]]

& has higher precedence than && according to my K&R, and unary ! has higher precedence than &.  So the code is correct, in fact, but I agree that making people remember whether & or && is higher precedence is just not called for.  ;)

Comment 11

[Timothy Nikkel (:tnikkel)]

Added the parenthesis in my queue.

Comment 12

[Timothy Nikkel (:tnikkel)]

It must have been wrong because with the parenthesis it no longer fixes the bug.

Comment 13

[Timothy Nikkel (:tnikkel)]

We only set mInTransform on the display list builder while we are in the display list building phase, not the painting phase, so thats why the patch doesn't work.

Comment 14

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

Add another flag to nsDisplayBackground I guess!

Comment 15

[Timothy Nikkel (:tnikkel)]

Now I'm thinking that we need to worry about all the other PaintBackground call sites.

Comment 16

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

OK, add a non-default flag to PaintBackground to enable the optimization, and use it just from the one call site.

Comment 17

[Timothy Nikkel (:tnikkel)]

I meant I'm worrying about them because I think we want to disable this optimization for those other call sites as well if they are transformed. (I'm mostly just thinking out loud in this bug, haven't gotten down to writing code.)

Comment 18

[Timothy Nikkel (:tnikkel)]

Oh, to enable. I see what you meant now.

Comment 19

[Timothy Nikkel (:tnikkel)]

It would be nice if we could still enable the optimization for those other cases too when they aren't transformed.

Comment 20

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

We could look at the frame tree to see if the frame is transformed, I guess.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 22

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

Comment on attachment 510369 [details] [diff] [review]
patch v2

+      bool transformed = false;
+      for (nsIFrame* f = aForFrame; f != topFrame; f = f->GetParent()) {
+        if (f->IsTransformed()) {
+          transformed = true;
+          break;
+        }
+      }

Move this to a helper function and use PRBool.

Comment 23

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/ffe65159d361