Closed
Bug 474201
Opened 16 years ago
Closed 16 years ago
background-color not reset when visiting new page
Categories
(Core :: Web Painting, defect)
Core
Web Painting
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: crazy-daniel, Assigned: zwol)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(3 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
dbaron
:
approval1.9.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review-
|
Details | Diff | Splinter Review |
The background-color isn't reset when a page is visited that doesn't define its own background-color.
For example:
1. Visit Acid3 (http://acid3.acidtests.org/)
2. In the location bar of that tab, enter about:blank and press go.
Actual Result:
about:blank got the same background-color as Acid3.
Expected Result:
about:blank should have the default background-color.
It also works if you visit any website that doesn't define its own background-color.
Updated•16 years ago
|
Comment 1•16 years ago
|
||
works with 20090116 nightly fails with 20090117 nightly. I am running hg
bisect now to identify the responsible check-in.
Keywords: regressionwindow-wanted
Updated•16 years ago
|
OS: Windows Vista → All
Comment 2•16 years ago
|
||
Testcase for this issue.
Comment 4•16 years ago
|
||
Interestingly, the testcase runs correctly from the attachment details page.
Comment 5•16 years ago
|
||
The first bad revision is:
changeset: 23836:5f9daba6c730
user: Zack Weinberg <zweinberg@mozilla.com>
date: Fri Jan 16 21:15:29 2009 +1300
summary: Bug 473398. nsCSSRendering::PaintBackground should not try to fix up the canvas background color to be opaque. r+sr=roc
Updated•16 years ago
|
Flags: blocking1.9.2?
Eek, this is a pretty bad regression ... Zack, please look into it
Assignee | ||
Comment 8•16 years ago
|
||
I understand what's going on here but I'm not sure what the right fix is.
In the test case, when we load the page with the green background, nsCSSRendering::PaintBackground sets the view manager's default background to green.
When we then refresh to about:blank, nsDocumentViewer creates a new view manager and sets its default background to the user default. But then nsDocShell overrides that with the green from the previous view manager (around line 6600 of nsDocShell.cpp), because "this improves page load continuity" (so say the comments). That is then the color used by nsPresShell::Paint for the ultimate fallback.
about:blank's background is transparent, so nsCSSRendering::PaintBackground does *not* reset the fallback color again, nor does it paint anything itself, so we get the fallback color from the previous document.
If I #if 0 the code in nsDocShell that copies the default background color from the old to the new view manager, the regression is fixed, but we lose the "page load continuity". Or we could go back to always setting the view manager's background to something opaque in nsCSSRendering::PaintBackground. I don't know which would be better.
Ah, sigh.
I guess we need to compose the root document's background color onto the prescontext's default background color and set that as the default background color, always.
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #9)
> Ah, sigh.
>
> I guess we need to compose the root document's background color onto the
> prescontext's default background color and set that as the default background
> color, always.
In PaintBackground? That'll regress bug 469170 again.
I'm tempted to throw out the docshell code that copies the last background color into the new view manager.
(In reply to comment #10)
> (In reply to comment #9)
> > Ah, sigh.
> >
> > I guess we need to compose the root document's background color onto the
> > prescontext's default background color and set that as the default background
> > color, always.
>
> In PaintBackground? That'll regress bug 469170 again.
No, I think the trick is that we should set the viewmanager's default background color there, but NOT use the new color when we call PaintBackgroundWithSC there. Does that make sense? So the backstop color used by PresShell::Paint will be changed, but not the color used to draw the canvas background itself.
Hmm, but that would mean that the first paint would use the wrong color.
How about this: always set the view manager's DefaultBackgroundColor to the canvas background color, no matter what, and do not mess with the background color in nsCSSRendering::PaintBackground. In PresShell::Paint, always use the prescontext's default background color as the background color passed to nsLayoutUtils::PaintFrame, ignoring view manager colors. This means the view manager DefaultBackgroundColor would only be used for nsViewManager::DefaultRefresh but I think that's what we want. What do you think?
In the meantime, can you make an automated test for this bug and bug 469170? The latter should be checked in right away.
Assignee | ||
Comment 12•16 years ago
|
||
> [...] what do you think?
I'll answer this tomorrow when I'm awake :)
> In the meantime, can you make an automated test for this bug and bug 469170?
> The latter should be checked in right away.
I have no idea how to make an automated test for this bug, but I had already written a test for bug 469170, so i've reopened it and attached a patch to it that just adds the test.
Comment 13•16 years ago
|
||
(In reply to comment #8)
> When we then refresh to about:blank, nsDocumentViewer creates a new view
> manager and sets its default background to the user default. But then
> nsDocShell overrides that with the green from the previous view manager (around
> line 6600 of nsDocShell.cpp), because "this improves page load continuity" (so
> say the comments). That is then the color used by nsPresShell::Paint for the
> ultimate fallback.
That is the fix for bug 75591.
Assignee | ||
Comment 14•16 years ago
|
||
This patch does exactly as Robert suggested in comment #11. it seems to work - fixes this bug, none of the relevant manual tests regressed (bug 453566, bug 467459, bug 469170), and did not regress any reftests or mochitests.
I'm a little uneasy about the testing here; I don't see any way to code an automated test for this. I feel like we need reftests that actually read the display buffer rather than repainting from scratch into a canvas surface.
Comment 15•16 years ago
|
||
Hmm. Just painting the about:blank document that has the wrong background into a canvas won't see the wrong background?
No, because of bug 469170; that is not the desired behaviour of drawWindow. We should implement the last part of https://bugzilla.mozilla.org/show_bug.cgi?id=469170#c28, that would let us test this bug.
Comment on attachment 358116 [details] [diff] [review]
rev 1: patch per robert's suggestions
Hmm, that looks suspiciously easy! OTOH it's simple and simple wins.
Attachment #358116 -
Flags: superreview+
Attachment #358116 -
Flags: review?(roc)
Attachment #358116 -
Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Assignee | ||
Comment 18•16 years ago
|
||
The additional drawWindow tweakage is definitely on my list, but i've been prioritizing all these regressions, getting the border-radius and border-image stuff done, &c.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Comment 20•16 years ago
|
||
Comment on attachment 358116 [details] [diff] [review]
rev 1: patch per robert's suggestions
Requesting approval to land this on 1.9.1, because this is a regression caused by blocker-bug 473398 (which hasn't been landed on 1.9.1 yet in part because of this regression, per bug 473398 comment 12)
Attachment #358116 -
Flags: approval1.9.1?
Comment 21•16 years ago
|
||
Daniel, its not clear right now, if the fix has been raised bug 474886.
Comment 22•16 years ago
|
||
I suspect this caused bug 476557, too.
Comment 23•16 years ago
|
||
(In reply to comment #21)
> Daniel, its not clear right now, if the fix has been raised bug 474886.
Looks like it did not, since that was fixed in unrelated code.
Comment 24•16 years ago
|
||
(In reply to comment #23)
> Looks like it did not, since that was fixed in unrelated code.
Yeah -- plus, bug 474886 comment 24 confirmed that a different bug's changeset was responsible for that regression.
However, this did cause bug 476557, as confirmed in bug 476557 comment 9.
Comment 25•16 years ago
|
||
(In reply to comment #24)
> However, this did cause bug 476557, as confirmed in bug 476557 comment 9.
I take that back -- as noted in bug 476557 comment 23, this bug's patch didn't actually *cause* that bug. This bug actually masked the symptoms of bug 476557, and when this bug was fixed, bug 476557 became visible once more.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 approval]
Comment 26•16 years ago
|
||
Comment on attachment 358116 [details] [diff] [review]
rev 1: patch per robert's suggestions
Looks like this is already approved per bug 478784 comment 19, but marking it here as well, although what's there is probably what should be landed.
Attachment #358116 -
Flags: approval1.9.1? → approval1.9.1+
Comment 28•16 years ago
|
||
Verified fixed with a 1.9.1 branch build on OS X and Windows:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090301 Shiretoko/3.1b3pre ID:20090301020400
Can we get the testcase into the testsuite?
Flags: in-testsuite?
Keywords: fixed1.9.1 → verified1.9.1
Comment 29•16 years ago
|
||
Attachment #377010 -
Flags: review?
You need to request review from someone. Requesting a review from nobody is almost certainly not going to result in any action.
+ setTimeout(foo, 1000);
Don't do this, timeouts are unpredictable because test boxes often run very slow. Instead try listening on the load event for the iframe.
Updated•16 years ago
|
Attachment #377010 -
Flags: review? → review?(roc)
Comment 32•16 years ago
|
||
Attachment #377460 -
Flags: review?(roc)
Updated•16 years ago
|
Attachment #377010 -
Attachment is obsolete: true
Attachment #377010 -
Flags: review?(roc)
I lied in comment #31. Like I said in the other bug, you should be using MozReftestInvalidate here.
Comment 34•16 years ago
|
||
Thank's for the feedback Robert, these invalidation issues are interesting to write tests for, and I will make the changes. A question Robert, - may you provide me an example where 'MozReftestInvalidate' must be used and where it should not?
MozReftestInvalidate should be used wherever you want to test that your DOM changes cause invalidation. We make sure that MozReftestInvalidate is fired after all other invalidation has completed.
If you don't care about whether your changes cause invalidation or not, you don't need to use MozReftestInvalidation.
Updated•16 years ago
|
Attachment #377460 -
Flags: review?(roc) → review?(longsonr)
Updated•16 years ago
|
Attachment #377460 -
Flags: review?(longsonr) → review?(roc)
Comment 36•16 years ago
|
||
Comment on attachment 377460 [details] [diff] [review]
invalidation reftest - fix
I think you want roc to review this. I'm not familiar with this code.
You're still using onload and not MozReftestInvalidate.
Comment 38•16 years ago
|
||
Attachment #377460 -
Attachment is obsolete: true
Attachment #378196 -
Flags: review?(roc)
Attachment #377460 -
Flags: review?(roc)
Comment 39•16 years ago
|
||
Hi Rob,
This fix uses MozReftestInvalidate and no OnLoad. Although - this scenario needs a setTimeout to wait a split second for a redraw and then capture the canvas.
Would you prefer the setTimeout(0) or the onLoad?
So just the MozReftestInvalidate doesn't work? That sounds like a bug.
Comment on attachment 378196 [details] [diff] [review]
invalidation reftest - fix (b)
The setTimeout should not be needed
Attachment #378196 -
Flags: review?(roc) → review-
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•