241187 - [gtk2]combo box shows up unexpectly due to nsWindow::IsVisible() does not return the right value

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Robin Lu]

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.

Comment 1

[Robin Lu]

Attachment: proposal patch

Comment 2

[Robin Lu]

Attachment: test case

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.

Comment 3

[Robin Lu]

Attachment: patch

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.

Comment 4

[Robin Lu]

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 5

[Christopher Blizzard (:blizzard)]

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.

Comment 6

[Robin Lu]

Attachment: patch addressing Blizzard's comment

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

Comment 7

[Brian Ryner (not reading)]

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

Comment 8

[Robin Lu]

patch checked in with bryner's comments.

Thank you very much!

Comment 9

[Christopher Blizzard (:blizzard)]

This is a good 1.7 candidate as well.

Comment 10

[Christopher Blizzard (:blizzard)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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