CSS Gradient backgrounds are not repainted when DOM is changed

VERIFIED FIXED

Status

()

Core
Layout: View Rendering
--
major
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: pascalc, Assigned: zwol)

Tracking

({testcase})

1.9.2 Branch
x86
Linux
testcase
Points:
---
Bug Flags:
blocking1.9.2 +
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(Whiteboard: verified1.9.2)

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 415923 [details]
screenshot of the bug

I am attaching to this bug a screenshot of the display bug with 3.6b4
nominating per discussion with pascal
status1.9.1: --- → ?
Whiteboard: testcase

Updated

8 years ago
status1.9.1: ? → ---
Flags: blocking1.9.2?
Version: 1.9.1 Branch → 1.9.2 Branch
Zack, can you look into this?
Assignee: nobody → zweinberg
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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.
Attachment #415923 - Attachment is obsolete: true
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.
Summary: CSS Gradiant backgrounds are not repainted when DOM is changed → CSS Gradient backgrounds are not repainted when DOM is changed
(Assignee)

Comment 7

8 years ago
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.
Attachment #416109 - Flags: review?(dbaron)
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.
Attachment #416109 - Flags: review?(dbaron) → review+
(Assignee)

Comment 9

8 years ago
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.
Attachment #416109 - Attachment is obsolete: true
Keywords: testcase
Whiteboard: testcase → [needs landing]
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
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attachment #416217 - Flags: approval1.9.2+
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.
I think this needs to block; it makes one of our major new features in 1.9.2 unusable in large numbers of cases.
Flags: blocking1.9.2- → blocking1.9.2?
(Assignee)

Comment 13

8 years ago
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.
Attachment #416217 - Attachment is obsolete: true
Attachment #417747 - Flags: review+
(Assignee)

Comment 14

8 years ago
Created attachment 417748 [details] [diff] [review]
patch for 1.9.2
Attachment #417748 - Flags: review+
(Assignee)

Comment 15

8 years ago
Created attachment 417749 [details] [diff] [review]
actually revised less conservative patch

forgot to refresh the patch
Attachment #417747 - Attachment is obsolete: true
Attachment #417749 - Flags: review+
(Assignee)

Comment 16

8 years ago
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.)
Attachment #417751 - Flags: review+
(Assignee)

Comment 17

8 years ago
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.
(Assignee)

Comment 18

8 years ago
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.
Your tests should be using MozReftestInvalidate.
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;
+      }
Flags: blocking1.9.2? → blocking1.9.2+
Whiteboard: [needs landing]
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED
(Assignee)

Comment 21

8 years ago
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.
Attachment #417816 - Flags: review?(roc)
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 on attachment 417816 [details] [diff] [review]
follow-up tweaks per dbaron and roc (for trunk)

Yeah, ABORT_IF_FALSE would be OK here
Attachment #417816 - Flags: review?(roc) → review+
(Assignee)

Comment 24

8 years ago
Changed in my copy.
(Assignee)

Comment 25

8 years ago
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
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
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2) Gecko/20100105 Firefox/3.6

Verified fixed
Status: RESOLVED → VERIFIED
Whiteboard: verified1.9.2
You need to log in before you can comment on or make changes to this bug.