Last Comment Bug 532721 - CSS Gradient backgrounds are not repainted when DOM is changed
: CSS Gradient backgrounds are not repainted when DOM is changed
Status: VERIFIED FIXED
verified1.9.2
: testcase
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: 1.9.2 Branch
: x86 Linux
: -- major with 1 vote (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-03 12:08 PST by Pascal Chevrel:pascalc
Modified: 2010-01-06 21:13 PST (History)
9 users (show)
roc: blocking1.9.2+
mbeltzner: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final-fixed


Attachments
testcase (10.27 KB, text/html)
2009-12-03 12:08 PST, Pascal Chevrel:pascalc
no flags Details
screenshot of the bug (136.62 KB, image/png)
2009-12-03 12:12 PST, Pascal Chevrel:pascalc
no flags Details
screenshot of the bug (corrected) (159.50 KB, image/png)
2009-12-03 18:44 PST, Zack Weinberg (:zwol)
no flags Details
fix (1.68 KB, patch)
2009-12-04 09:26 PST, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review
patch with test cases (8.26 KB, patch)
2009-12-04 19:01 PST, Zack Weinberg (:zwol)
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review
revised less conservative patch (8.26 KB, patch)
2009-12-15 12:18 PST, Zack Weinberg (:zwol)
zackw: review+
Details | Diff | Splinter Review
patch for 1.9.2 (12.14 KB, patch)
2009-12-15 12:19 PST, Zack Weinberg (:zwol)
zackw: review+
Details | Diff | Splinter Review
actually revised less conservative patch (12.37 KB, patch)
2009-12-15 12:21 PST, Zack Weinberg (:zwol)
zackw: review+
Details | Diff | Splinter Review
just the revisions (for trunk) (6.26 KB, patch)
2009-12-15 12:27 PST, Zack Weinberg (:zwol)
zackw: review+
Details | Diff | Splinter Review
follow-up tweaks per dbaron and roc (for trunk) (7.63 KB, patch)
2009-12-15 15:55 PST, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Splinter Review

Description Pascal Chevrel:pascalc 2009-12-03 12:08:09 PST
Created attachment 415921 [details]
testcase

Tested with Firefox 3.6beta4 as well as trunk:
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.3a1pre) Gecko/20091201 Minefield/3.7a1pre

1/ have CSS gradiants as background (HTML tag) and a block element on top (body)
2/ change the body width to a smaller value via javascript

actual result:
block element is correctly redimensioned but the CSS gradiants on the newly uncover sides of the block element are wrong. If you scroll the page, the css gradiant is redrawn correctly

expected result:
Css gradiant background should be correctly rendered.

I am attaching an HTML test case, click on the 'Click Me' text to display the bug.
Comment 1 Pascal Chevrel:pascalc 2009-12-03 12:12:31 PST
Created attachment 415923 [details]
screenshot of the bug

I am attaching to this bug a screenshot of the display bug with 3.6b4
Comment 2 Carsten Book [:Tomcat] 2009-12-03 12:14:54 PST
nominating per discussion with pascal
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-03 13:25:58 PST
Zack, can you look into this?
Comment 4 Zack Weinberg (:zwol) 2009-12-03 18:28:29 PST
It is not actually the gradients in the newly uncovered areas that are wrong, but the gradients in the area that was already visible.  The test case contains a whole lot of text, so changing the width of the <body> element changes the height of the page, which changes the gradient which means the entire area covered by that gradient needs to be repainted, not just the newly uncovered area.  We're not picking up on this because the DOM change does not touch the *specified* gradient.

Adding Boris - I'm not sure what the right way to fix this is: we only know that the *used* gradient has changed in the painting code, but the dirty region has been computed a lot earlier.

There are also two other bugs exposed by (slight modifications of) this test case, which I shall file separately.
Comment 5 Zack Weinberg (:zwol) 2009-12-03 18:44:27 PST
Created attachment 416001 [details]
screenshot of the bug (corrected)

Pascal's screenshot did not show this bug, because of bug 532828.  Here is a corrected screenshot.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2009-12-03 18:54:24 PST
So.... the key issue here is that the <html>'s height changes, right?  Normally we handle this in nsIFrame::CheckInvalidateSizeChange, which detects various cases that need to invalidate more than just the rect difference (e.g. outlines, borders, background-position in percentage units, that sort of thing).  Seems like we should have a check for gradient backgrounds in there and invalidate more area as needed.
Comment 7 Zack Weinberg (:zwol) 2009-12-04 09:26:10 PST
Created attachment 416109 [details] [diff] [review]
fix

Thanks, Boris, that was what I needed to be pointed at.  Here's a fix which works for me.  Automated test case to follow, but I may not be able to get to it till much later today.
Comment 8 David Baron :dbaron: ⌚️UTC-8 2009-12-04 10:19:26 PST
Comment on attachment 416109 [details] [diff] [review]
fix

This looks ok, although it's a little conservative; technically the only difference is that for gradients the background-size depends on the frame size when the background-size is 'auto'.  (So, alternatively, you could fix it by passing a boolean parameter to nsStyleBackground::Size::DependsOnFrameSize.)

You're going to need to write a completely different 1.9.2 patch, though, since this code was reorganized since 1.9.2 branched.

We should get this on 1.9.2.
Comment 9 Zack Weinberg (:zwol) 2009-12-04 19:01:27 PST
Created attachment 416217 [details] [diff] [review]
patch with test cases

Same as the original fix, but with test cases.  I haven't adjusted the RenderingMightDependOnFrameSize implementation as dbaron suggested yet.
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-08 11:39:24 PST
Wanted, definitely, but not blocking, I don't think, unless dbaron disagrees (which he'll do by either renominating or changing the flag).

a192=beltzner
Comment 11 Vladimir Vukicevic [:vlad] [:vladv] 2009-12-15 11:29:40 PST
1.9.2 approval for this expires at the end of the day (15 Dec) due to code freeze plans -- please land as soon as possible; if it bounces, it bounces hard.
Comment 12 David Baron :dbaron: ⌚️UTC-8 2009-12-15 12:10:09 PST
I think this needs to block; it makes one of our major new features in 1.9.2 unusable in large numbers of cases.
Comment 13 Zack Weinberg (:zwol) 2009-12-15 12:18:53 PST
Created attachment 417747 [details] [diff] [review]
revised less conservative patch

This adds a third test case and implements the strategy for being less conservative that dbaron suggested.
Comment 14 Zack Weinberg (:zwol) 2009-12-15 12:19:22 PST
Created attachment 417748 [details] [diff] [review]
patch for 1.9.2
Comment 15 Zack Weinberg (:zwol) 2009-12-15 12:21:02 PST
Created attachment 417749 [details] [diff] [review]
actually revised less conservative patch

forgot to refresh the patch
Comment 16 Zack Weinberg (:zwol) 2009-12-15 12:27:23 PST
Created attachment 417751 [details] [diff] [review]
just the revisions (for trunk)

roc landed my original patch, so here's the revisions as an additional patch.  I'll land this shortly and the full patch on 1.9.2.  (The changes required for 1.9.2 were actually quite small.)
Comment 17 Zack Weinberg (:zwol) 2009-12-15 12:39:36 PST
Landed on trunk and 1.9.2:

http://hg.mozilla.org/mozilla-central/rev/54d12a619531
http://hg.mozilla.org/mozilla-central/rev/8090e6b58803
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/365028e2e366

I'll watch tinderbox and back out if necessary.
Comment 18 Zack Weinberg (:zwol) 2009-12-15 12:43:55 PST
Incidentally, the reftests do not appear to catch the bug if it's present.  I think this is because drawWindow does a total repaint - we've seen that problem before.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-15 12:50:06 PST
Your tests should be using MozReftestInvalidate.
Comment 20 David Baron :dbaron: ⌚️UTC-8 2009-12-15 12:58:21 PST
Two comments on the revisions:

The comment:
//                                        We don't currently bother
// trying to identify gradients that don't depend on the frame size.
isn't true anymore.

Also, this:
+      if (aType == eStyleImageType_Image) {
+        return mWidthType <= ePercentage || mHeightType <= ePercentage;
+      } else if (aType == eStyleImageType_Gradient) {
+        return mWidthType <= eAuto || mHeightType <= eAuto;
+      } else {
+        NS_NOTREACHED("unrecognized image type");
+      }

is probably better (safer, and no compiler warning) written as:

+      if (aType == eStyleImageType_Image) {
+        return mWidthType <= ePercentage || mHeightType <= ePercentage;
+      } else {
+        NS_ASSERTION(aType == eStyleImageType_Gradient,
+                     "unrecognized image type");
+        return mWidthType <= eAuto || mHeightType <= eAuto;
+      }
Comment 21 Zack Weinberg (:zwol) 2009-12-15 15:55:58 PST
Created attachment 417816 [details] [diff] [review]
follow-up tweaks per dbaron and roc (for trunk)

Changes suggested by roc and dbaron.  Also, height-dependence-2.html is failing on Mac (filed bug 535007) - marked as such, waiting for another test cycle to see if it needs a random-if instead before doing anything on the branch.
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-15 16:08:39 PST
Comment on attachment 417816 [details] [diff] [review]
follow-up tweaks per dbaron and roc (for trunk)

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h

>     PRBool DependsOnFrameSize(nsStyleImageType aType) const {
>       if (aType == eStyleImageType_Image) {
>         return mWidthType <= ePercentage || mHeightType <= ePercentage;
>-      } else if (aType == eStyleImageType_Gradient) {
>+      } else {
>+        NS_ASSERTION(aType == eStyleImageType_Gradient,
>+                     "unrecognized image type");

NS_ABORT_IF_FALSE?  At the moment it looks like you'll only fail this with some sort of memory corruption.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-15 16:34:53 PST
Comment on attachment 417816 [details] [diff] [review]
follow-up tweaks per dbaron and roc (for trunk)

Yeah, ABORT_IF_FALSE would be OK here
Comment 24 Zack Weinberg (:zwol) 2009-12-15 17:23:34 PST
Changed in my copy.
Comment 25 Zack Weinberg (:zwol) 2009-12-15 19:06:26 PST
Follow-up fixes landed on trunk and branch:
http://hg.mozilla.org/mozilla-central/rev/4b2811f44606
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6e590843b831
Comment 26 Daniel Holbert [:dholbert] 2009-12-15 19:24:16 PST
Original patch was landed earlier today as:
http://hg.mozilla.org/mozilla-central/rev/54d12a619531
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/365028e2e366
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2010-01-06 21:13:04 PST
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2) Gecko/20100105 Firefox/3.6

Verified fixed

Note You need to log in before you can comment on or make changes to this bug.