28003 - no background color should be set on a window on linux

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

There are a lot of times when a widget's Show() method is called before the
background color is set.  It looks not-so-nice when a big black block appears
before a web page is loaded.  I'm attaching a patch to detect the situation.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch to detect Show() before SetBackgroundColor() on unix

Comment 2

[troy]

Changing component and re-assigning

Comment 3

[Peter Trudelle]

widgets in web pages are HTML Form Controls, not XPToolkit, changing component 
and reassigning.

Comment 4

[Christopher Blizzard (:blizzard)]

This is the background of web pages, not indvidual widgets just FYI.

Comment 5

[karnaze (gone)]

Reassigning to Rod.

Comment 6

[rods (gone)]

Waqar, Could you take a look?

Comment 7

[Christopher Blizzard (:blizzard)]

*** Bug 28078 has been marked as a duplicate of this bug. ***

Comment 8

[waqar]

I see this happening, it only started to happen today. I will see what can be
done.

Comment 9

[Christopher Blizzard (:blizzard)]

It's been exposed by some code that I check in two days ago.  I had always had
the background color hard set at 192,192,192 at the time of widget creation.  I
stopped doing that since none of the other platforms do it.

Comment 10

[waqar]

I am not sure what the right solution is here, I also see transparent area for
around the toolbar before they get painted.

Comment 11

[Christopher Blizzard (:blizzard)]

*** Bug 25743 has been marked as a duplicate of this bug. ***

Comment 12

[Marko Macek]

Transparent area before painting is not a problem. 

Please do not set any X background color on any mozilla windows. This was
recently fixed (again, I believe) and I don't want to see it broken again.

Since mozilla uses double buffering for everything it is a waste (and ugly
blinking) to paint background twice. The background color should only be for
paint buffer.

Quick test is to run 'xrefresh'. There should be no blinking in mozilla window.

BTW: there are several "black" bugs :) 

Comment 13

[waqar]

What is the resolution on this. Do we want to set the background or do we just 
leave it as is?

Comment 14

[Christopher Blizzard (:blizzard)]

We want to set the background color for documents.  When you are dragging a
window across mozilla it looks really crappy right now since it leaves trails
behind in the browser or when you resize it.

Comment 15

[Marko Macek]

I strongly disagree. If you set the background color, you will force the server
to paint every time (double painting, blinking) and you will have the trail of
the background color (which could be much) different than content in the page).

Also, all expose events will blink. 

Please, please, please, don't change this again.

The correct solution is to make the repainting very fast. :)

Comment 16

[Christopher Blizzard (:blizzard)]

For a long time I had the background color set and it didn't hurt performance. 
For scrolling I actually turned off the background color which was about the
only place that it really seemed to hurt.

Comment 17

[Marko Macek]

It hurts a lot in opaque move and opaque resize! 

And blinking hurts too :)

Comment 18

[Christopher Blizzard (:blizzard)]

*** Bug 33648 has been marked as a duplicate of this bug. ***

Comment 19

[waqar]

Moving

Comment 20

[waqar]

*** Bug 28023 has been marked as a duplicate of this bug. ***

Comment 21

[Christopher Blizzard (:blizzard)]

*** Bug 20776 has been marked as a duplicate of this bug. ***

Comment 22

[xkahn]

Okay.  This bug bothers me enough that I need to post more detail.  I tend to

use virtual desktops with Netscape maximized in it's own viewport.  When

switching to a viewport containing Mozilla, I see the windows in the old

viewport and can watch the mozilla frame draw, and then the web page.  This

takes a little less than a second.  As my computer becomes busier, the time

increases.  Some other programs have this problem too -- they all feel very

clunky.



PLEASE, immediate feedback is good.  Turn it off for scrolling, and resize if

you want...  But it's driving me crazy, and making mozilla *feel* very

unprofessional.

Comment 23

[xkahn]

Attachment: This is a good reason for the NULL color.

Comment 24

[xkahn]

I've just attached a picture of Mozilla while it was performing a site lookup. 
I moved to a new viewport, moved back and started xv. I then performed a screen
grab using a hidden xv window.  Notice that you can see my desktop behind
mozilla, and you can see the "hidden" xv windows.  With no NULL color, mozilla
just becomes inivisible!  When I switch viewports, I see this exact thing, but
it refreshes after half a second.

Comment 25

[Marko Macek]

The repainting is slow and needs to be improved. 

The correct solution is not to set X background color, because it caused double 
painting and makes double buffering mostly useless.

The fact that mozilla doesn't repaint the background (even under windows) in 
some situations (often when going to new page) is a separate bug and should be 
filed separately.

Comment 26

[Tomi Leppikangas]

I think that we sould not set bg color, it just makes double drawing.

I wonder why all "flashes black when loaging page" bugs are marked
duplicate of this? I think that is other problem. 

Here is my thought what is hapening, i could be totally wrong thougt:
Looks like mozilla is creating new window every time it loads page. And when
creating that page, it defaults to black? So fix to is is reuse old window, not
setting bg color to some other default.

This seems to be happening on nsDocShell::SetupNewViewer. There mContentViewer
gets deleted and re-created, window is attached to that via pressshell, so
window it is deleted and created too.

You can see this by putting breakpoint to nsWindow::Destroy and loading
about:blank.

Maybe we should copy window from old contentviewer, there is lots of copying
already in nsDocShell::SetupNewViewer. Or not just reuse old contenviewer, like
suggested in some comment on nsDocShell::CreateContentViewer.

Maybe some docshell guy should look this.

Comment 27

[Christopher Blizzard (:blizzard)]

I think the proper way to fix this is before showing the new widget to set the
background color to the parent's document's background color.

Comment 28

[Marko Macek]

Tomi:
 agree that many black window bugs should probably not be dups of this.
I also agree that setting X background is a bad idea due to double drawing.

Blizzard:
Possibly, as long as you are not talking about X window background. (but some
mozilla widget background)


Any ideas how the black flashed could happen during scrolling. One of my
duplicates contains this problem. Should I request a reopen. (it reports
horizontal scrolling problems, but I now managed to do it also for vertical
scrolling)

Comment 29

[Marko Macek]

Attachment: source for LD_PRELOAD workaround to remove black flashes (behavior is now identical to Windows)

Comment 30

[Marko Macek]

How to compile the attached workaround:
  gcc -g -Wall -fPIC -shared nobg.c -o nobg.so -L/usr/X11R6/lib -lX11 -ldl

The workaround removed any background setting code.

It not only removes black flashes, mozilla seems to be noticeably faster too.

Now we have to find out where the bogus background colors are being set (gtk+?),
and remove them. I'm not experienced in the gtk style system internals, so help
would be appreciated.

Comment 31

[Marko Macek]

gdk_window_new seems to hardcode BlackPixel default. 

Comment 32

[Marko Macek]

Attachment: fix this problem by resetting the background (had to use X11, gdk hates me)

Comment 33

[Marko Macek]

Another related problem:

Click "Cancel" in any dialog. Watch how the widgets disappear first then the
background repaints and only then the window disappears. I will post a patch for
that too (It's basically the same issue as in my last patch)

Comment 34

[Christopher Blizzard (:blizzard)]

This looks completely reasonable to me.  Gtk is lame.  I'll check it in when the
tree opens.  Thanks!

Comment 35

[Christopher Blizzard (:blizzard)]

Taking ownership.

Comment 36

[Christopher Blizzard (:blizzard)]

Attachment: gdk patch

Comment 37

[Christopher Blizzard (:blizzard)]

This is a patch that I did that uses gdk primitives just so it's all gdk.

Comment 38

[Christopher Blizzard (:blizzard)]

..and I fixed the C++ comments in a c file so tor doesn't give me a melvin.

Comment 39

[Marko Macek]

Basically the same thing has to be done in gtk/nsWindow.cpp for each
gtk_window_new.

I used X11 code because I was looking at CVS gdk code that doesn't like what we
are doing to it (different arguments required). Perhaps we should just use X
calls... :(

Comment 40

[Marko Macek]

Attachment: do this after gtk_window_new too

Comment 41

[Christopher Blizzard (:blizzard)]

Yeah, that looks good too.  I'll check that in as well.

Comment 42

[Christopher Blizzard (:blizzard)]

Checked in.

Comment 43

[Marko Macek]

*** Bug 39602 has been marked as a duplicate of this bug. ***

Comment 44

[Marko Macek]

*** Bug 39602 has been marked as a duplicate of this bug. ***

Comment 45

[bsharma]

Updating QA contact.

Comment 46

[Madhur Bhatia]

marking verified as per developers comments.

Comment 47

[jerone]

verified on build 2001-08-07-80-trunk