Closed Bug 574690 Opened 9 years ago Closed 9 years ago

Title bar(window without body) appears at the upper left corner of monitor screen for a split second when starting Minefield and restore previous size of browser

Categories

(Core :: XUL, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: alice0775, Assigned: jimm)

References

Details

(Keywords: regression)

Attachments

(3 files, 10 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre ID:20100625040119
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre ID:20100625040119

In Windows7 Classic Style (not Aero).
Title bar(window without body) appears at the upper left corner of monitor screen for a split second when starting Minefield and restore previous size of browser.

This does not happen in Aero style.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2.Please watch a screen carefully

Actual Results:
Window without body appears at the upper left corner of monitor screen for a split second when starting Minefield. And the browser is restored to the position/size of the last time.

Expected Results:
Title bar without body should not appear.

Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/522df66198cf
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre ID:20100624185700

Fails:
http://hg.mozilla.org/mozilla-central/rev/51bd519736c4
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre ID:20100624221810

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=522df66198cf&tochange=51bd519736c4
And it happens on Windows XP.
Blocks: 513162
And it happens not only start up, it also happens when open any dialog such as "Download Manager" and "About Minefield" and new browser.
Jim and I think that it's being caused by painting child windows that have loading documents. No clear solution yet.
The values here are completely messed up. This probably has to do with initialization of the view early in the creation of the window. In the past, children wouldn't be effected by this, but now that a view holds a top level, calls like this can have side effects.
I would think that by the time LoadComplete is called views would be setup already in either case.
We used to see this before when it was the only window open, and doing stuff like trying to restore the window size, etc fixed it.  But I think we were talking about session restore then or not restoring windows that were closed if they were dialogs.  I don't know if that is also at the heart of this bug.. but something to think about.
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #454382 - Flags: review?(tellrob)
Comment on attachment 454382 [details] [diff] [review]
patch v.1

Why do we not set mHasValidSize anywhere? It defaults to PR_FALSE so we have a bunch of dead code.
This is a new approach. We prevent the DocumentViewer from showing the window if it's a top level window and the document shell tree owner is an nsIXULWindow. In this situation we know the nsXULWindow will take care of showing it.
Attachment #454382 - Attachment is obsolete: true
Attachment #454412 - Flags: review?(roc)
Attachment #454382 - Flags: review?(tellrob)
Corrected to NULL-check mParentWidget.
Attachment #454412 - Attachment is obsolete: true
Attachment #454414 - Flags: review?(roc)
Attachment #454412 - Flags: review?(roc)
Attachment #454414 - Attachment is patch: true
Attachment #454414 - Attachment mime type: application/octet-stream → text/plain
I think you should check treeItem->GetParent instead of looking at mParentWidget.
I mean, instead of checking the widget type of 'mWindow'.
Updated as per IRC discussion.
Attachment #454414 - Attachment is obsolete: true
Attachment #454431 - Flags: review?(roc)
Attachment #454414 - Flags: review?(roc)
Pushed http://hg.mozilla.org/mozilla-central/rev/7ad36412775e.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
It is not fixed on latest hourly
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100627 Minefield/3.7a6pre ID:20100627215239

[STR]
1. Start Minefield in Classic style (non Aero)
2. Open Library (Ctrl + Shift + B)
(In reply to comment #15)
> It is not fixed on latest hourly
> Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre)
> Gecko/20100627 Minefield/3.7a6pre ID:20100627215239
> 
> [STR]
> 1. Start Minefield in Classic style (non Aero)
> 2. Open Library (Ctrl + Shift + B)

confirmed, I see it too. We'll have to do some more debugging. This patch definitely fixed the size/show problem for the main browser window.

Alice can you confirm that part of it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug is fixed, indeed there is still a bug with other windows, filed bug 575184. This probably doesn't need to block beta 1.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 575615
No longer depends on: 575281
Blocks: 576096
No longer blocks: 513162
No longer depends on: 575615
Didn't the iid here need to change?
Assignee: nobody → bas.schouten
Yes, sorry! Bas, please rev the IID.
Looks like this patch has been regressed bug 575281 which prevents us from using D&D of tabs. Their content isn't shown anymore.
Reopening since this seems the best place to post a fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #454431 - Attachment is obsolete: true
Comment on attachment 457474 [details] [diff] [review]
patch v.1

I'm backing out the original changes as they weren't needed. The core problem here is that when chrome loads, nxXULWindow fires off a Size() and a Move() to the widget using persisted data. The Size event triggers a gui size mode event, which propagated up to document viewer, triggering an early call to Show. This is causing a number of display anomalies when we first open windows. 

What I've done is just reorder the calls in nxXULWindow's OnChromeLoaded so that Move() is made before Size(). (Since Move doesn't trigger a Show() in document viewer like Size(), the problem goes away.)
Attachment #457474 - Flags: review?(bzbarsky)
Assignee: bas.schouten → jmathies
Component: Graphics → XP Toolkit/Widgets: XUL
QA Contact: thebes → xptoolkit.xul
Attachment #457474 - Flags: review?(timeless)
> I'm backing out the original changes 

Sort of.  You forgot to remove the nsIXULWindow include, and for some reason you're removing curlies that were there all along and that should stay.  ;)

It looks like we used to call Move() before Size() before the fix for bug 113283.  Did that fix just accidentally change the ordering, or was that a necessary part of fixing whatever it was trying to fix?
Duplicate of this bug: 574657
(In reply to comment #24)
> > I'm backing out the original changes 
> 
> Sort of.  You forgot to remove the nsIXULWindow include, and for some reason
> you're removing curlies that were there all along and that should stay.  ;)

I'm planning on just backing out the original patch.

> It looks like we used to call Move() before Size() before the fix for bug
> 113283.  Did that fix just accidentally change the ordering, or was that a
> necessary part of fixing whatever it was trying to fix?

Looks like Dan moved the size up so that it would be on top of the if(mIntrinsicallySized) block, then plopped position in below it. I don't see any reason why that can't be reordered. The original LoadSizeFromXUL did position, then size. If it breaks something, I'm sure we'll hear about it soon enough.
Comment on attachment 457474 [details] [diff] [review]
patch v.1

OK, r=me if the original patch for this bug is backed out instead of the partial backout in this patch landing.

Let's hope you're right about "soon enough"....
Attachment #457474 - Flags: review?(bzbarsky) → review+
(In reply to comment #27)
> Comment on attachment 457474 [details] [diff] [review]
> patch v.1
> 
> OK, r=me if the original patch for this bug is backed out instead of the
> partial backout in this patch landing.
> 
> Let's hope you're right about "soon enough"....

I pushed it to try, we'll see if anything shows up.
Attachment #457474 - Flags: review?(timeless) → review?(neil)
Attached patch patch w/o backed out code (obsolete) — Splinter Review
Attached patch patch w/o backed out code (obsolete) — Splinter Review
Attachment #457490 - Attachment is obsolete: true
Attachment #457474 - Flags: review?(neil)
Attachment #457491 - Flags: review?(neil)
Attachment #457491 - Flags: review?(neil) → review+
Attached patch refreshed w/code backout (obsolete) — Splinter Review
http://hg.mozilla.org/mozilla-central/rev/d4aafb90ff96
http://hg.mozilla.org/mozilla-central/rev/7124132f0506
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Hi,
it is indeed better, but when opening a new window from a maximized window, I can see a first painting of the new window on it's "not maximized" position, then it is maximized.
No problem when the new window doesn't have to be maximized.
still seeing the same problem. A mini caption bar appears when I hide the Fx menu and the Fx button isn't displayed.  I see a white rectangle when the Fx button is displayed.

Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre - Build ID: 20100716040830
(In reply to comment #33)
> Hi,
> it is indeed better, but when opening a new window from a maximized window, I
> can see a first painting of the new window on it's "not maximized" position,
> then it is maximized.
> No problem when the new window doesn't have to be maximized.

Unrelated to this, please file a separate bug if you feel the issue is serious. (Also, you might check 3.6 too, to see if the behavior has changed since that release.)
Problem is back on XP, too, with
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100716
Minefield/4.0b2pre ID:20100716040830

First nightly I've seen this problem reappear (since right before beta1?,
iirc).
Should have mentioned that I did not have this problem for a long time.  It was fixed at one point in the past and then with today's nightly it reappeared.
(In reply to comment #37)
> Should have mentioned that I did not have this problem for a long time.  It was
> fixed at one point in the past and then with today's nightly it reappeared.

Gary, can you reproduce in a clean profile, no add-ons? (Which isn't to say I'll drop this, add-ons may be triggering an early size event to document viewer, so I might have to revisit that area of the code with an additional fix.)
(In reply to comment #34)
> still seeing the same problem. A mini caption bar appears when I hide the Fx
> menu and the Fx button isn't displayed.  I see a white rectangle when the Fx
> button is displayed.
> 
> Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100716
> Minefield/4.0b2pre - Build ID: 20100716040830

This doesn't sound like the same as the app start problem, can you clarify a bit how this relates to this bug?
(In reply to comment #39)
> This doesn't sound like the same as the app start problem, can you clarify a
> bit how this relates to this bug?

It happens for me on my regular profile, but it didn't with that profile's safe-mode. (The window DID move from the left half of the screen over to the right when it drew in all the chrome widgets and stuff in safemode, but I don't know if that's something to do with safemode.)

I'll try to narrow down the list of extensions to find out what's causing it.
(In reply to comment #40)
> (In reply to comment #39)
> > This doesn't sound like the same as the app start problem, can you clarify a
> > bit how this relates to this bug?
> 
> It happens for me on my regular profile, but it didn't with that profile's
> safe-mode. (The window DID move from the left half of the screen over to the
> right when it drew in all the chrome widgets and stuff in safemode, but I don't
> know if that's something to do with safemode.)
> 
> I'll try to narrow down the list of extensions to find out what's causing it.

Looks like it's Firebug 1.6X.0a17 in my case.
Attached image frame before maximize
This shows the problem related to maximized state I'm seeing. If you max the browser, close it down completely, then open again, you might see a normalized window frame briefly before the window maximizes.

I tested in 3.6, but it seems that version doesn't restore the window show state correctly. So I'm not sure if this is a regression or not. Can anyone else confirm?
(In reply to comment #42)
> I tested in 3.6, but it seems that version doesn't restore the window show
> state correctly. So I'm not sure if this is a regression or not. Can anyone
> else confirm?

Actually, scratch that, I had a minefield window minimized on the taskbar. :) I can confirm I do not see the initial frame on 3.6.
Seems to be caused by many add-ons to one degree or another.  Some add-ons will produce the mini title bar very, very fast while others will keep them on the screen longer so you notice it. 

One add-on that keeps it on the screen the longest is LastPass 1.69.1. This might be because LP logs into it's server.  Also Adblock Plus shows the mini title bar but not as long. 

IMO all add-ons might cause this behavior.  Some you might not notice becuase it happens so fast, others you do.
blocking2.0: --- → beta2+
Depends on: 589523
No longer depends on: 589523
Depends on: 597133
(In reply to comment #26)
> (In reply to comment #24)
> > > I'm backing out the original changes 
> > 
> > Sort of.  You forgot to remove the nsIXULWindow include, and for some reason
> > you're removing curlies that were there all along and that should stay.  ;)
> 
> I'm planning on just backing out the original patch.
> 
> > It looks like we used to call Move() before Size() before the fix for bug
> > 113283.  Did that fix just accidentally change the ordering, or was that a
> > necessary part of fixing whatever it was trying to fix?
> 
> Looks like Dan moved the size up so that it would be on top of the
> if(mIntrinsicallySized) block, then plopped position in below it. I don't see
> any reason why that can't be reordered. The original LoadSizeFromXUL did
> position, then size. If it breaks something, I'm sure we'll hear about it soon
> enough.

(In reply to comment #27)
> Comment on attachment 457474 [details] [diff] [review]
> patch v.1
> 
> OK, r=me if the original patch for this bug is backed out instead of the
> partial backout in this patch landing.
> 
> Let's hope you're right about "soon enough"....

soon enough! I'm going to back this out. But first I want a better fix.
Blocks: 597576
Depends on: 591973
Attached patch widget fix (obsolete) — Splinter Review
I need to run through all the test cases and bugs to be sure this gets them all. Looks good so far based on initial testing.
Attachment #457474 - Attachment is obsolete: true
Attachment #457491 - Attachment is obsolete: true
Attachment #457676 - Attachment is obsolete: true
Attached patch widget fix (obsolete) — Splinter Review
This patch includes:

changeset:   55271:044722b9b0d4
parent:      47785:7124132f0506
user:        Jim Mathies <jmathies@mozilla.com>
date:        Sat Oct 09 16:02:52 2010 -0500
summary:     backout of 7124132f0506

tag:         patch
tag:         tip
parent:      55270:904a556a15f2
user:        Jim Mathies <jmathies@mozilla.com>
date:        Sat Oct 09 16:08:26 2010 -0500
summary:     Bug 574690 - Suppress spurious widget Show events during window initialization in nsXULWindow's OnChromeLoaded.
Attachment #481983 - Attachment is obsolete: true
Attachment #482082 - Flags: review?(neil)
Comment on attachment 482082 [details] [diff] [review]
widget fix

> NS_METHOD nsWindow::Show(PRBool bState)
> {
> #if defined(MOZ_SPLASHSCREEN)
>   // we're about to show the first toplevel window,
>   // so kill off any splash screen if we had one
>   nsSplashScreen *splash = nsSplashScreen::Get();
>   if (splash && splash->IsOpen() && mWnd && bState &&
>       (mWindowType == eWindowType_toplevel ||
>        mWindowType == eWindowType_dialog ||
>        mWindowType == eWindowType_popup))
>   {
>     splash->Close();
>   }
> #endif
> 
>+  if (mSuppressShow) {
>+    mSuppressedState = bState;
>+    return NS_OK;
>+  }
Shouldn't this go before the splash screen check, in case we need both?

>+    if (mWindow)
>+      mWindow->SuppressShow(PR_FALSE);
>+
>     if (mShowAfterLoad) {
>       SetVisibility(PR_TRUE);
>       // At this point the window may have been closed during Show(), so
>       // nsXULWindow::Destroy may already have been called. Take care!
>     }
This worries me. On platforms supporting SuppressShow it's possible that the SuppressShow(PR_FALSE) call triggers script that closes the window. On the other hand on platforms not supporting SuppressShow the SetVisibility call might trigger script that closes the window...
(In reply to comment #50)
> >+    mSuppressedState = bState;
> >+    return NS_OK;
> >+  }
> Shouldn't this go before the splash screen check, in case we need both?

I'll move it up.

> 
> >+    if (mWindow)
> >+      mWindow->SuppressShow(PR_FALSE);
> >+
> >     if (mShowAfterLoad) {
> >       SetVisibility(PR_TRUE);
> >       // At this point the window may have been closed during Show(), so
> >       // nsXULWindow::Destroy may already have been called. Take care!
> >     }
> This worries me. On platforms supporting SuppressShow it's possible that the
> SuppressShow(PR_FALSE) call triggers script that closes the window. On the
> other hand on platforms not supporting SuppressShow the SetVisibility call
> might trigger script that closes the window...

Tracking if Destroy has been called could address this. I'll post a new patch tomorrow with the changes.
Comment on attachment 482082 [details] [diff] [review]
widget fix

This wasn't complete either. With script popups we want the show to be delayed past OnChromeLoaded since we still haven't set the correct size and position yet. This patch only addressed persisted attribs on the window.
Attachment #482082 - Flags: review?(neil)
Hmm, now that we've removed all sub widgets, I don't think nsDocumentViewer needs to call Show on the widget at all.

Reopening this since I'm planning on backing out the changes that landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm going to run this through try tonight. I'm pretty sure this is safe since the base window's visibility is handled by other code  in nsXULWindow, nsWindowWatcher, & nsFocusManager.
Attachment #482082 - Attachment is obsolete: true
Attachment #483342 - Flags: review?(roc)
Depends on: 604434
Depends on: 618198
No longer depends on: 618198
Depends on: 618198
Depends on: 629860
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.