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.

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?)?

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.

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?

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

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.

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.

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

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 #).

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?

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!)

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.

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.

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?

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?

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.

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

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

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

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

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.

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

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

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

(*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?

To roc.

Linux. "Works for me"

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.

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.

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

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

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

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

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

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.

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

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

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.

r=waterson

sr=attinasi

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.

Someone expand those tabs! /be

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

Fix checked in.

can this have caused bug 77507?

vrfy fixed: linux comm 2001.05.02.08 mac comm 2001.05.02.09 winnt moz 2001.05.02.12