191 bytes, text/html
4.39 KB, patch
|Details | Diff | Splinter Review|
1.74 KB, patch
|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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
works with 20090116 nightly fails with 20090117 nightly. I am running hg bisect now to identify the responsible check-in.
Created attachment 357549 [details] Testcase Corrected testcase
Attachment #357548 - Attachment is obsolete: true
Interestingly, the testcase runs correctly from the attachment details page.
The first bad revision is: changeset: 23836:5f9daba6c730 user: Zack Weinberg <firstname.lastname@example.org> 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
Component: Layout → Layout: View Rendering
QA Contact: layout → layout.view-rendering
Eek, this is a pretty bad regression ... Zack, please look into it
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.
(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.
> [...] 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.
(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.
Created attachment 358116 [details] [diff] [review] rev 1: patch per robert's suggestions 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.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #358116 - Flags: review?(roc)
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.
Whiteboard: [needs landing]
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
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?
Daniel, its not clear right now, if the fix has been raised bug 474886.
I suspect this caused bug 476557, too.
(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.
(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.
(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.
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+
Whiteboard: [needs 1.9.1 approval]
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?
Keywords: fixed1.9.1 → verified1.9.1
Created attachment 377010 [details] [diff] [review] invalidation reftest
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.
Created attachment 377460 [details] [diff] [review] invalidation reftest - fix
I lied in comment #31. Like I said in the other bug, you should be using MozReftestInvalidate here.
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.
Attachment #377460 - Flags: review?(roc) → review?(longsonr)
Attachment #377460 - Flags: review?(longsonr) → review?(roc)
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.
Created attachment 378196 [details] [diff] [review] invalidation reftest - fix (b)
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-
You need to log in before you can comment on or make changes to this bug.