Closed Bug 532721 Opened 15 years ago Closed 15 years ago

CSS Gradient backgrounds are not repainted when DOM is changed

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Version:
1.9.2 Branch
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: pascalc, Assigned: zwol)

Details

(Keywords: testcase, Whiteboard: verified1.9.2)

Attachments

(6 files, 4 obsolete files)

testcase
(deleted), text/html
Details
screenshot of the bug (corrected)
(deleted), image/png
Details
patch for 1.9.2
(deleted), patch
zwol
: review+
Details | Diff | Splinter Review
actually revised less conservative patch
(deleted), patch
zwol
: review+
Details | Diff | Splinter Review
just the revisions (for trunk)
(deleted), patch
zwol
: review+
Details | Diff | Splinter Review
follow-up tweaks per dbaron and roc (for trunk)
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase (deleted) —
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.
Attached image screenshot of the bug (obsolete) (deleted) —
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
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
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.
Attached image screenshot of the bug (corrected) (deleted) —
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
Attached patch fix (obsolete) (deleted) — — Splinter Review
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+
Attached patch patch with test cases (obsolete) (deleted) — — Splinter Review
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-
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?
Attached patch revised less conservative patch (obsolete) (deleted) — — Splinter Review
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+
Attached patch patch for 1.9.2 (deleted) — — Splinter Review
Attachment #417748 - Flags: review+
forgot to refresh the patch
Attachment #417747 - Attachment is obsolete: true
Attachment #417749 - Flags: review+
Attached patch just the revisions (for trunk) (deleted) — — Splinter Review
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+
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
Closed: 15 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED
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+
Changed in my copy.
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
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: