nsWindow::IsVisible returns the value of mIsVisible. This value is only changed in nsWindow::OnVisibilityNotifyEvent. However, there are some situations when a window is unmapped and no visibilitynotify event is sent. The nsWindow::IsVisible still gives PR_TRUE even the window is not visible due to unmapped.

a test case to show the problem. 1. open the file with the browser 2. click on the list to drop it down and choose any one to roll it up 3. click on the "disable" button expect: the listbox is disabled actual result: the list dropes down and is disabled. You can never roll it up again.

Current trunk may call nsWindows::IsVislbe() even after nsWindow::Destory() has been called, which may cause the last patch crashing. Update the patch to check if mShell is still exist.

another easy test case can show you the problem just in this bugzilla page. 1. click and roll down any combo box here, for example, Hardware, OS, Severity, etc, and then click anywhere to roll it up. 2. press ctrl-+ to zoom the page result: you will see the combo box you rolled down lasttime shows up again. If you unfortunatly dropped down more than one combo boxes before, you are really in a big mess when you zoom the page. This bug impact both mozilla and firefox.

Comment on attachment 148270 [details] [diff] [review] patch >Index: nsWindow.cpp >=================================================================== >RCS file: /export/src/cvs/mozilla1.4/mozilla/widget/src/gtk2/nsWindow.cpp,v >retrieving revision 1.11 >diff -u -r1.11 nsWindow.cpp >--- nsWindow.cpp 2003/09/15 09:29:55 1.11 >+++ nsWindow.cpp 2004/04/12 05:48:24 >@@ -417,6 +417,11 @@ > nsWindow::IsVisible(PRBool & aState) > { > aState = mIsVisible; >+ if(mIsTopLevel&&mShell&&!GTK_WIDGET_MAPPED(mShell)) { How about if (mIsTopLevel && mShell && !GTK_WIDGET_MAPPED(mShell)) { Other than that, this patch is fine.

blizzard, thanks for your comment! Could you add r to the patch?

Comment on attachment 149460 [details] [diff] [review] patch addressing Blizzard's comment >Index: nsWindow.cpp >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v >retrieving revision 1.99 >diff -u -r1.99 nsWindow.cpp >--- nsWindow.cpp 30 Mar 2004 10:33:26 -0000 1.99 >+++ nsWindow.cpp 28 May 2004 01:29:31 -0000 >@@ -435,6 +435,11 @@ > nsWindow::IsVisible(PRBool & aState) > { > aState = mIsVisible; >+ if(mIsTopLevel && mShell && !GTK_WIDGET_MAPPED(mShell)) { Please put a space after the |if|. >+ /* we do not change mIsVisible to PR_FALSE here so we don't bother to >+ to change it back to PR_TRUE when the mShell is mapped again. */ Fix the double-word in the comment ("bother to to change").

patch checked in with bryner's comments. Thank you very much!

This is a good 1.7 candidate as well.

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

Comment on attachment 149460 [details] [diff] [review] patch addressing Blizzard's comment blizzard, if you think this is safe enough I'd say we want this on the branch....