474201 - background-color not reset when visiting new page

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Daniel.S]

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.

Comment 1

[Bill Gianopoulos [:WG9s]]

works with 20090116 nightly fails with 20090117 nightly.  I am running hg
bisect now to identify the responsible check-in.

Comment 2

[Bill Gianopoulos [:WG9s]]

Attachment: testcase

Testcase for this issue.

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: Testcase

Corrected testcase

Comment 4

[Bill Gianopoulos [:WG9s]]

Interestingly, the testcase runs correctly from the attachment details page.

Comment 5

[Bill Gianopoulos [:WG9s]]

The first bad revision is:
changeset:   23836:5f9daba6c730
user:        Zack Weinberg <[e-mail]>
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

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Eek, this is a pretty bad regression ... Zack, please look into it

Comment 8

[Zack Weinberg (:zwol)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Zack Weinberg (:zwol)]

(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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 12

[Zack Weinberg (:zwol)]

> [...] 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

[Bill Gianopoulos [:WG9s]]

(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.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Just painting the about:blank document that has the wrong background into a canvas won't see the wrong background?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Zack Weinberg (:zwol)]

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/0eac03a6d8a6

Comment 20

[Daniel Holbert [:dholbert]]

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)

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Daniel, its not clear right now, if the fix has been raised bug 474886.

Comment 22

[Daniel Holbert [:dholbert]]

I suspect this caused bug 476557, too.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Daniel Holbert [:dholbert]]

(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

[Daniel Holbert [:dholbert]]

(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 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6c6948b544fd

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 29

[Aaron Train [:aaronmt]]

Attachment: invalidation reftest

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You need to request review from someone. Requesting a review from nobody is almost certainly not going to result in any action.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 32

[Aaron Train [:aaronmt]]

Attachment: invalidation reftest - fix

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I lied in comment #31. Like I said in the other bug, you should be using MozReftestInvalidate here.

Comment 34

[Aaron Train [:aaronmt]]

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?

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 36

[Robert Longson [:longsonr]]

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.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're still using onload and not MozReftestInvalidate.

Comment 38

[Aaron Train [:aaronmt]]

Attachment: invalidation reftest - fix (b)

Comment 39

[Aaron Train [:aaronmt]]

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?

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So just the MozReftestInvalidate doesn't work? That sounds like a bug.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 378196 [details] [diff] [review]
invalidation reftest - fix (b)

The setTimeout should not be needed