72230 - When loading a site, mozilla stops redrawing content area - bad user experience

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[mar_garina]

When mozilla loads a site, _BEFORE_ it starts displaying it, it shows a whole
black screen. The problem is that when I move a window over it, it doesn't
redraw itself as it should (it redraws the menus and all, just not the area
where the site should be displayed), and it doesn't look too nice..

How it should be:
Mozilla should re-draw the black screen when it gets a redraw event (I forgot
its exact name).

Comment 1

[Christopher Blizzard (:blizzard)]

Thanks for filing this bug.  I've been meaning to file it myself for a while.

Comment 2

[Christopher Blizzard (:blizzard)]

Accept and assign...

Comment 3

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 4

[Christopher Blizzard (:blizzard)]

Here's a patch that will fill in the undrawn area of the nsWebBrowser before
pages are loaded.  It's pretty simple.  Valeksi or Adam can you review this? 
Thanks.

Comment 5

[ajbu]

Hi,
I just tried this patch using the ActiveX embedding control.
Works fine runtime except the ActiveX uses the old non-painting to draw it's 
own text on the control area in design mode. Now the ActiveX paints it's own 
text, and clicking on it clears it.
This should probably be fixed for the ActiveX after this is checked in.

Comment 6

[Christopher Blizzard (:blizzard)]

valeski?  danm?  Can you guys look at this patch?

Comment 7

[Judson Valeski]

r=valeski

Comment 8

[Chris Waterson]

blizzard: as per conversation on IRC, it would be great if we could do this at 
the docshell level, as that would fix bug 5569, too. (I'm waving my hands 
wildly, because I don't really understand most of this stuff.)

Comment 9

[Christopher Blizzard (:blizzard)]

Attachment: better patch

Comment 10

[Christopher Blizzard (:blizzard)]

This patch just sets the window color.  Since we don't get need to paint on that
window we don't ever disturb it so we can let the OS/server do the drawing for us.

Comment 11

[Christopher Blizzard (:blizzard)]

I need reviews from valeski or adam or someone.

Comment 12

[Adam Lock]

The patch looks fine, but if the original problem is that background window 
isn't painting *anything*, what good will changing the window background colour 
do?

Comment 13

[Christopher Blizzard (:blizzard)]

If you set the background color on the window then you don't have to ever handle
paint requests for the window since the OS will handle the paints for you.

Comment 14

[Christopher Blizzard (:blizzard)]

Still waiting for reviews.  La la la.

Comment 15

[Adam Lock]

If the patch works on Win32 then r=adamlock.

I'm guessing that setting the background colour affects the way NS_PAINT is 
handled in the view manager or somewhere. As far as I can see in the Win32 
widget there is no code associated with setting the background colour that would 
affect the way the background is drawn.

Comment 16

[Christopher Blizzard (:blizzard)]

I don't have a win32 box to test this out on so I'm depending on someone else to
test it.

IIRC if the PAINT method isn't handled on win32 it's automatically drawn with
the default color.  But I could be wrong since I'm not a win32 expert.

Comment 17

[ajbu]

Windows 2000:
I tried the last attachment 29944 [details] [diff] [review] with the ActiveX control, but it doesn't seem 
to work with Win32/ActiveX: Opening a form with an ActiveX Mozilla control 
doesn't paint the control. (Background showing in the control-area, and no 
redraw when moving a window over it)
I tried attachment 28041 [details] [diff] [review] before and that patch did fix this for the ActiveX 
control.

Comment 18

[Christopher Blizzard (:blizzard)]

OK.  Since windows needs to have an explicit paint I'll use the first patch.

Comment 19

[Chris Waterson]

Ok, so we're going with the original patch, right? 03/18/01 12:08?

Here's some henpeckery take it or leave it. In general, this looks fine; 
sr=waterson

+NS_IMETHODIMP nsWebBrowser::FillBackground(nsRect &aRect)

Might as well make it |const nsRect& aRect|; you're not going to mutate aRect.

+  case NS_PAINT: {
+      nsRect rect = *((nsPaintEvent *)aEvent)->rect;
+      browser->FillBackground(rect);

Too much deref boggle? (I can never remeber if * or -> has precedence.) How 
about:

  const nsRect* rect = NS_STATIC_CAST(nsPaintEvent*, aEvent)->rect;
  browser->FillBackground(*rect);

Comment 20

[Christopher Blizzard (:blizzard)]

Attachment: final patch

Comment 21

[Christopher Blizzard (:blizzard)]

Chris, you were right.  It was much cleaner looking that way.  The final patch
is attached for the sake of posterity.

Comment 22

[Christopher Blizzard (:blizzard)]

Fix checked in.

Checking in nsWebBrowser.cpp;
/cvsroot/mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp,v  <-- 
nsWebBrowser.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in nsWebBrowser.h;
/cvsroot/mozilla/embedding/browser/webBrowser/nsWebBrowser.h,v  <--  nsWebBrowser.h
new revision: 1.30; previous revision: 1.29
done

Comment 23

[Hixie (not reading bugmail)]

As discussed with hyatt, this is IMHO the wrong fix. We shouldn't be tearing 
down the frames of the old page before we have frames for the new page. It
looks bad. See our prefs dialog, for an example of extreme badness. See IE for
an example of how it should work.

Hyatt said he'd fix it. Reassigning to him. Hyatt: be careful not to break
bug 5569 while you're at it...

Comment 24

[Hixie (not reading bugmail)]

reassigning...

Comment 25

[Chris Waterson]

I think you re-opened the wrong bug. Try bug 5569; actually, open a new one.

Comment 26

[Hixie (not reading bugmail)]

ok, hyatt was filing 76303 as I was reopening this one. Marking it as a dup...

*** This bug has been marked as a duplicate of 76303 ***

Comment 27

[Conrad Carlen (not reading bugmail)]

Whether this is a dup of 76303 ? but this checkin caused some ugliness. Since it
went in, the area behind the content area is painted to black which is really
jarring. The code goes to the trouble of finding the background color of the
window which is good. Problem is, before filling the rect, it does not set the
color so it fills with whatever color was last used in that port. I think it
should set the color before painting:

Index: mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp
===================================================================
RCS file: /cvsroot/mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp,v
retrieving revision 1.74
diff -u -2 -r1.74 nsWebBrowser.cpp
--- nsWebBrowser.cpp	2001/04/18 01:42:27	1.74
+++ nsWebBrowser.cpp	2001/04/24 03:28:51
@@ -1436,4 +1443,5 @@
 NS_IMETHODIMP nsWebBrowser::FillBackground(const nsRect &aRect)
 {
+    mRC->SetColor(mBackgroundColor);
     mRC->FillRect(aRect);
     return NS_OK;

Comment 28

[Christopher Blizzard (:blizzard)]

What is your default background color?  And what platform are you using?

Comment 29

[Conrad Carlen (not reading bugmail)]

It's Mac. The default background color is light gray. But, if the color is not
set before painting, that doesn't matter - it's whatever color was last set in
that rendering context - which happens to be black.

Comment 30

[Christopher Blizzard (:blizzard)]

There must be something else going on here.  In nsWebBrowser::Create we
initialize that value:

http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#911

Is that not the same for the mac?

Comment 31

[Conrad Carlen (not reading bugmail)]

Right, but anything can change the value before we actually paint - at least on Mac.

Comment 32

[Christopher Blizzard (:blizzard)]

Can change the value of mBackgroundColor?  It's only set in one place, in
Create().  Or are you getting a FillBackground() before Create() finishes or
something?

Comment 33

[Conrad Carlen (not reading bugmail)]

No, I'm not getting a FillBackground() before a Create(). Looks like, on Mac,
the underlying GrafPort is shared by different instances of rendering contexts.
The color gets set in Create(), other drawing happens, the color gets changed,
and by the time FillRect() happens, the color is black.

Comment 34

[Christopher Blizzard (:blizzard)]

Interesting.  I had no idea.  In that case can you remove the SetColor() call
from Create() and the comment that goes with it while you are at it?

Comment 35

[David Hyatt]

Reassigning to you guys.  Hixie, if you want a bug on keeping the old page
around, make a new one.

Comment 36

[Conrad Carlen (not reading bugmail)]

Attachment: patch so we draw in the right color

Comment 37

[Conrad Carlen (not reading bugmail)]

Can somebody r/sr the patch?

Comment 38

[Conrad Carlen (not reading bugmail)]

updating status, need review

Comment 39

[Adam Lock]

r=adamlock

Comment 40

[chris hofmann]

who are the right people to r= sr=????

Comment 41

[Conrad Carlen (not reading bugmail)]

It has r=. The change (which is only the last patch) is very simple. cc'ing
waterson.

Comment 42

[Chris Waterson]

blizzard should sr=, but FWIW, it looks fine to me.

Comment 43

[Christopher Blizzard (:blizzard)]

r/sr=blizzard a=blizzard for 0.9

Comment 44

[Jesse Ruderman]

Is this bug related to bug 60780?

Comment 45

[Ian Tommins ('thelem')]

has this been fixed with bug 76495 ?

Comment 46

[Conrad Carlen (not reading bugmail)]

No, not related and not fixed. The remaining problem is that the background
erases to black instead of the window color with embedding apps on Mac. Sorry I
let this one slip. I'll check it in today.

Comment 47

[Conrad Carlen (not reading bugmail)]

Fix checked into trunk and branch.

Comment 48

[Adam Lock]

Work on bug 78412 should fix this problem too

Comment 49

[Dawn Endico]

arrgh. this wasn't approved for checkin on the branch. no fair checking
in stuff after the final builds have already spun.  drivers?

Comment 50

[Conrad Carlen (not reading bugmail)]

Then what's this:
> r/sr=blizzard a=blizzard for 0.9

I should have done it before but forgot about it. Sorry if it was too late.

Comment 51

[Dawn Endico]

that a= was from more than a week ago. How annoying is this problem? Is
it worth keeping the patch and respinning new builds?

Comment 52

[Conrad Carlen (not reading bugmail)]

No, it's a small problem. It should not require a respin because it only effects
embedding apps, only on Mac, and has no effect on Seamonkey. You can back it out
of the branch (but not the trunk please) if you want.

Comment 53

[Christopher Blizzard (:blizzard)]

Well, since we're dealing with other problems this will probably end up in 0.9
anyway.

Comment 54

[Chris Petersen]

Problem doesn't reproduce on the Mac May 30th build. Marking verified.