75591 - default background color flashes when switching btwn iframes

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[sairuh (rarely reading bugmail)]

thanks to blake for pointing this out --blizzard, he claims this belongs to you.
:)

using 2001.04.11.08 opt comm bits on linux and winNT [oddly, not a problem on
mac, so far], occurs in both modern and classic themes.

1. open Preferences dialog.
2. switch btwn the Navigator, Fonts, Colors and Appearance panels.

observe: when you switch panels, rather than having the background color of
theme, you'll see the system default background color [at least i have that set
in my Colors prefs]. eg, for me the system background on winNT is white, so when
i switch panels the panel region flashes white.

Comment 1

[Blake Ross]

This is ugly, nominating and cc'ing roc, karnaze, hewitt...

I think this is going to be Hard.  We just load the pref panes into a normal 
(<iframe/>) container.  I don't know how we'll distinguish between the chrome 
and a webpage.  Or can this just be fixed by specifying a bg color in the css 
for this specific iframe (and should we have to do that?)?

Comment 2

[Christopher Blizzard (:blizzard)]

I suspect that this is because of the change that I made where the view manager
paints the default color when there's an expose and there's no view handling the
paint.

Over to waterson who expressed interest in fixing it.  Chris, you will probably
want to update the summary to something like "view manager default color should
be set from layout" or something.

Comment 3

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

What we really need here are transparent IFRAMEs, but that's a big chunk of work  
(which will have to be done eventually, but now may not be a good time).

Even setting the default background color from layout is going to be difficult. 
How are we going to figure it out?

Comment 4

[Blake Ross]

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

Comment 5

[David Hyatt]

Chrome should be reverted to the original behavior.  I don't care what we do 
for Web pages, but for chrome we should revert back to the original behavior.  

Comment 6

[Chris Waterson]

Chrome relied on a bug that paints would get dropped. Switching between prefs 
panes, I see two or three invalidates (unh unh unh!) of the content area. I have 
a patch that preserves the correct background color; I'm now trying to figure 
out why we are invalidating so aggressively.

Comment 7

[Chris Waterson]

Attachment: wave the magic wand to turn a bug into a feature

Comment 8

[Chris Waterson]

Okay, I banged around here for a few hours today, and decided that the best
thing to do would be to turn a bug into a feature. The above patch let's chrome
docshells set a new, special flag on a view that says, ``don't do a default
refresh''. Also moved blizzard's patch into a separate routine to avoid running
off the right end of the screen.

Still to do: figure out why we are so eager to invalidate the web page (which
forces the flashing on web pages).

Comment 9

[Chris Waterson]

Attachment: oops. patch that compiles.

Comment 10

[Chris Waterson]

Attachment: oh good grief

Comment 11

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

You're really turning a bug into another bug. If, for example, a transient 
window goes away at just the wrong time, we end up with garbage displayed for 
these non-Refreshing views.

If we're going to make interface changes, I think we should do them right and 
avoid short-term hacks. In this case I think doing the right thing means adding 
ns[I]ViewManager::SetBackgroundColor and calling it from the PresShell when the 
PresShell updates its background color style rules. (PresShell will have to do 
something special for chrome docs.) We can use this color for various things 
inside the view manager actually, including platform background painting (forget 
the bug #).

Comment 12

[Chris Waterson]

roc, I agree that this fix sucks, to say the least. But...

The chrome is taking advantage of the bug (i.e., that the view doesn't paint
itself) to avoid flicker in IFRAMEs. (Take a look at the prefs panel in a
current build; e.g.)

An earlier revision of this hackery (which I've since nuked, but could easily
revive) did manage to divine the correct background color and push it into the
root view, but I *still* ended up with visible artifacts when switching between
panels in the prefs window.

So, why does this happen? When the docshell loads a new document, it tears down
the old one mercilessly. Specifically, it destroys the old root widget and
replaces it with a new one; see nsDocShell::SetupNewViewer(). The creation and
sizing of the new widget causes a toolkit-level paint event, which causes us to
blank the IFRAME before loading new content. (At least, this is what I think
happens given my limited understanding of the view and widget magic.)

Is my pain real? Or is there a way out?

Comment 13

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

Hmm. I'm not an expert here. So it looks like nsDocShell::SetupNewViewer is 
tearing down the old content viewer, which destroys its view manager and root 
widget ... then it creates a new content viewer with a new view manager and root 
widget. But where, in the middle of that, could we process a paint event? Is 
someone forcing a synchronous paint?

If no-one's doing a synchronous paint, couldn't we at the very least record the 
background color from the old view manager and copy it into the new one?

(Do we do all this work on every page load? Crikey!)

Comment 14

[Chris Waterson]

Attachment: superhack to get the bgcolor from the previous page and push it into the new widget

Comment 15

[Chris Waterson]

No, we don't process a paint in the middle of that. But one is enqueued, so when
we unwind back out to the main event loop, it'll be processed, well, whenever
the event queue decides to get 'round to it. If that happens before we've
managed to built up a new frame model (the source of many bugs lumped under bug
5569), then we'll end up calling into the view manager with refresh disabled,
and will fall into blizzard's new code.

Comment 16

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

That patch is unimaginably vile. Congratulations.

Actually, I meant that *if* we used a new method to set/get the background color 
on the view manager, we could transfer the color to the new view manager. That 
frame crawling code is not only vile, it's completely wrong when the IFRAME 
doesn't have scrollbars (for example).

Anyway, if indeed no synchronous paint is forced through here, then we should be 
in great shape. Let's have SetupNewViewer copy the background color from the old 
view manager to the new view manager. We can set the view manager's background 
color in, say, CanvasFrame::Reflow.

Comment 17

[Chris Waterson]

Yes, that certainly sounds better like a better way to propagate background 
color to the next document. But...

We're still stuck with the IFRAME flicker that is intolerable to the chrome 
folks, and AFAICT can only be solved with something like the patch in ``04/17/01 
16:41 oh good grief''

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31236

(Recall that we used to avoid the flicker by ignoring the paint altogether.) One 
alternative would be a fairly serious rewrite of the docshell that would allow 
us to keep the old document around until the new one is ready for presentation. 
(I can hear Hixie cackling.) Do you have any suggestions as to how to proceed 
short of that?

Comment 18

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

If we copy the background color into the new view manager somewhere inside 
SetupNewViewer, I don't see how that can cause flicker. Inside SetupNewViewer 
we will tear down the old widget, create and install a new view manager, create 
the new widget, and set its background color all before any repaint events can 
be processed. Right?

Comment 19

[Chris Waterson]

Sure, but my point is that with blizzard's patch, the new widget will still
FillRect() with the background color in response to the toolkit-generated expose
event that's fired when the widget is created. (At least, that's what I suspect
is happening.)

The bottom line is that what it looks like is:

  Panel1 --> Grey Fill --> Panel2

as opposed to:

  Panel1 ----------------> Panel2

It's not the worst thing in the world, and is certainly better than

  Panel1 --> White Fill --> Panel2

(given that the panels have a grey background). Nevertheless, the grey fill is
noticable even on a fast machine, and it Makes Hyatt Mad.

Comment 20

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

Sorry for being obtuse, but I don't see how the "Grey Fill" repaint event is 
going to be handled before the new view manager is initialized with the old 
background color.

> the new widget will still
> FillRect() with the background color in response to the toolkit-generated
> expose event that's fired when the widget is created.

When you say the event is *fired* when the widget is created, are you saying 
that creating the widget does a synchronous paint on some platforms? Even if 
that's so, we can initialize the new view manager with the correct background 
color before the widget is actually created.

I guess I should just write the patch and figure out what's wrong with it myself 
:-).

Comment 21

[Chris Waterson]

Attachment: less vile patch

Comment 22

[Chris Waterson]

I'll save you the trouble. Try the last attachment.

Comment 23

[Chris Waterson]

> Sorry for being obtuse, but I don't see how the "Grey Fill" repaint event is 
> going to be handled before the new view manager is initialized with the old 
> background color.

The point is that the grey fill happens before the *initial reflow*. Let me try
to explain the sequence of events in more detail. Specifically, when switching
between panels in the preferences pane is:

1. We start on Preference Panel A. All the XUL widgetry is visible.

2. Click in the tree view to select Preference Panel B. This starts
   a load in the slaved IFRAME.

3. nsDocShell::SetupNewViewer() is called in the IFRAME. The old root widget
   is destroyed, a new one is created. We unwind back out to the event loop.

4. The new root widget's paint is processed (on Win32, anyway), filling
   the IFRAME with whatever color we pick as the default fill. (My last
   patch ends up painting it brownish-grey, the classic chrome's widget
   color.)

5. The IFRAME completes loading and does an initial reflow of Preference
   Panel B. At this point, Preference Panel B's XUL controls are drawn.

Between steps 4 and 5, the user is starting (ever so briefly) at a nasty, blank,
brownish grey slab of an empty IFRAME.

Comment 24

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

That patch looks great! I will try it out in an attempt to persuade myself that 
it doesn't work :-).

Comment 25

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

That patch doesn't work because XUL documents don't have a CanvasFrame. The 
background color is only set on the box for the document element. Therefore the 
default background color never gets set and of course painting does the wrong 
them. The timing of events is fine though.

I have a revised patch that fixes this bug. However, I want to change the 
Set/GetDefaultBackgroundColor interface to add an extra parameter that indicates 
whether or not the color is valid. It is vile to just compare the color to zero.  
I also want to set the color in ::Paint rather than ::Reflow ... that way we'll 
be absolutely sure of responding to all changes. I will make these changes and 
attach the revised patch ASAP.

Comment 26

[Chris Waterson]

> That patch doesn't work because XUL documents don't have a CanvasFrame.
> The background color is only set on the box for the document element.
> Therefore the default background color never gets set and of course
> painting does the wrong them. The timing of events is fine though.

That shouldn't matter, because we'll use the widget's bgcolor in the case where 
no default bgcolor has been set on the view manager. That color is inherited 
from the parent widget, and should be just fine. But, whatever. I look forward 
to seeing how your solution avoids the intermediate paint.

Comment 27

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

> That shouldn't matter, because we'll use the widget's bgcolor in the case
> where no default bgcolor has been set on the view manager.

Yeah, but the widget's background color is some ugly grey shade so we see 
flicker!

> That color is inherited from the parent widget, and should be just fine.

We're not setting sensible background colors on any widgets. Although with this 
patch, we will finally have enough information in the view manager to do just 
that...

> I look forward to seeing how your solution avoids the intermediate paint.

There is no intermediate paint. No painting occurs between tearing down the old 
widget and creating a new widget with its view manager initialized to have the 
same default background color as the default background color for the old view 
manager.

Comment 28

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

Attachment: Non-vile Patch

Comment 29

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

That patch includes some changes that I haven't tested yet ... but it should 
compile and do the job.

Comment 30

[Chris Waterson]

(*sigh*) You seem to have missed the point. This patch STILL PAINTS the default
background in the preferences panel when you switch between panels. Do you not
see this? What platform are you using?

Comment 31

[Chris Waterson]

To roc.

Comment 32

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

Linux. "Works for me"

Comment 33

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

It's possible that I buggered up the changes w.r.t. the "valid" flag or 
something else. I'll test it again when I get into school this morning.

Comment 34

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

OK, I can confirm that my latest patch is working for me on Linux. (The previous 
patches and the trunk code do display the problem on Linux, BTW, so I know I did 
*something* good :-).)

If there are platform issues, then I'm mystified ... by the time we leave 
SetupNewViewer and return to the event loop, the view manager has been 
initialized with the correct background color.

Comment 35

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

People should try out the patch on Mac and Windows so we can make progress here.

Comment 36

[Chris Waterson]

(argh, forgot to add myself to cc list.)

> If there are platform issues, then I'm mystified ... by the time we leave 
> SetupNewViewer and return to the event loop, the view manager has been 
> initialized with the correct background color.

Right, as it was (more or less) with my loathsome patches of 04/17/01 22:49.

Would you please re-read my comments on 2001-04-17 23:03? The point is not that
the background color was incorrect. The point (as I understand it) is that the
XUL folks do not want a background-fill to happen *at all* before the new
document is loaded.

Some comments on your implementation...

- It's annoying that we need to duplicate the SetDefaultBackgroundColor()
  method. Why not push this up to a base class?

- Do we really need to reset the background color every time we *paint*? Why
  not just do it when you know the bgcolor has changed?

- It's a shame you have to waste 32 bits just to know if the bgcolor is valid
  (although there aren't many view managers, so it's not a big deal). Tell me
  again why using a distinct value of color is so horrible?

Comment 37

[Brendan Eich [:brendan]]

Eww, tabs.  Roc, please expand 'em.  Doesn't the modeline say indent-tabs-mode:
nil?

/be

Comment 38

[Brendan Eich [:brendan]]

Wow, whole lotta PRBools in nsViewManager.  How about grouping them by fours and
using PRPackedBool?

/be

Comment 39

[Chris Waterson]

If you decide not to move SetDefaultBackgroundColor() to a frame base class, be
sure to check in changes to nsBoxFrame.h (not included in the patch you posted).

Comment 40

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

OK, I understand now. I apologize for not getting it earlier. I was confused 
because I'm using the Modern theme, and with your patch of 04/17/01 22:49, there 
were grey backgrounds drawn during transitions that clash horribly with the 
purple theme background. My patch fixes that problem but now it seems that is 
not enough...

Actually I'm surprised there's ire from the XUL overlords about the situation 
with my patch. The old controls disappear, we wait for a moment, and then the 
new controls appear. The background in the prefs panel never changes and is 
always the same as the rest of the window, so there's no "flashing". The only 
thing one could complain about is the length of the period of time between the 
old controls disappearing and the new controls appearing. But that period of 
time is usually less than the length of time the new controls spend sputtering 
through various half-laid-out states as images load and various other 
initialization happens.

Maybe we could get rid of all of that incremental drawing ugliness in one shot, 
by modifying Prefs to use two IFRAMEs. It can load a new panel into a second 
hidden IFRAME; when the panel is fully loaded, we make it the primary visible 
IFRAME and hide the old panel's IFRAME.

The only other thing we could reasonably do is to fix things so that we only 
tear down the old document when the new document is ready to go (the new bug you 
filed...).

Anyway, I'd like to hear from the chrome people what exactly they want.

============================================================================
Here are the implementation comments, if we want it after all:

> - It's annoying that we need to duplicate the SetDefaultBackgroundColor()
>   method. Why not push this up to a base class?

Agreed, we (I) should do that.

> - Do we really need to reset the background color every time we *paint*? Why
>   not just do it when you know the bgcolor has changed?

I don't how to reliably trap all possible changes to the bgcolor. It can be 
changed in lots of different ways. It sure would be great to be able to do what 
you suggest.

> It's a shame you have to waste 32 bits just to know if the bgcolor is valid
> (although there aren't many view managers, so it's not a big deal). Tell me
> again why using a distinct value of color is so horrible?

We really only need one bit and nsViewManager is ripe for some bit-packing, as 
Brendan observes.

With the code you had using zero as the "unknown" background color, what if the 
theme uses a black background? In the view manager, you'd then assume the 
background was actually unknown, and proceed to use the widget background color, 
which would probably be wrong, so we'd be flashing again.

Brendan, I think I inherited the tabs from Chris' patch ... the nsViewManager in 
the tree has indent-tabs-mode: t. Mine was changed to nil as part of clean-up 
work, but I haven't got all this cleanup together for submission yet. Anyway, 
I'll fix the tabs.

Comment 41

[Chris Waterson]

> Anyway, I'd like to hear from the chrome people what exactly they want.

May be a non-issue; see bug 77002.

> I don't how to reliably trap all possible changes to the bgcolor. It can be 
> changed in lots of different ways. It sure would be great to be able to do
> what you suggest.

Well, the solution you've got is sure to catch them, and is probably efficient 
enough.

> With the code you had using zero as the "unknown" background color, what
> if the theme uses a black background? 

Wouldn't the color be 0xff00 0000? Anyway, brendan's probably got the best 
answer to the problem. Pack the bools and it doesn't matter, so long as you 
don't spill over a multiple of four! ;-)

> Brendan, I think I inherited the tabs from Chris' patch ... the
> nsViewManager in the tree has indent-tabs-mode: t.

Ack. Apologies.

r=waterson, if you need it.

Comment 42

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

> Wouldn't the color be 0xff00 0000?

I guess so ... I never realized that nsColor is RGBA!! In that case I'm getting
rid of the Valid stuff because it's redundant. Sorry sorry sorry ... new patch
coming.

Comment 43

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

Attachment: Patch v2

Comment 44

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

The above patch addresses the implementation concerns of Brendan and Chris Waterson.

Chris, let me know if your review still applies to this patch.

Marc, can you sr?

If you want this into 0.9, somoene needs to send mail to drivers explaining why
they think it's important. I'm willing to vouch that the technical risk is minimal.

Comment 45

[Chris Waterson]

r=waterson

Comment 46

[Marc Attinasi]

sr=attinasi

Comment 47

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

OK, great. Thanks!

Someone still needs to make the case to drivers if they want this in 0.9. I am 
not going to do that because I don't personally think this needs to be in 0.9, 
although I'm open to persuasion.

Comment 48

[Brendan Eich [:brendan]]

Someone expand those tabs!

/be

Comment 49

[Chris Waterson]

brendan: the whole file is fscked, as merrily specified by the emacs modeline.
Let's do tab expansion in another pass.

Comment 50

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

Fix checked in.

Comment 51

[R.K.Aa.]

can this have caused bug 77507?

Comment 52

[sairuh (rarely reading bugmail)]

vrfy fixed:

linux comm 2001.05.02.08
mac comm 2001.05.02.09
winnt moz 2001.05.02.12