If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Robin Lu, Assigned: Robin Lu)

Tracking

({fixed1.7})

Trunk
x86
Linux
fixed1.7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(2 attachments, 2 obsolete attachments)

538 bytes, text/html
Details
658 bytes, patch
blizzard
: review+
Brian Ryner (not reading)
: superreview+
blizzard
: approval1.7+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 146669 [details] [diff] [review]
proposal patch
(Assignee)

Comment 2

14 years ago
Created attachment 148269 [details]
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.
(Assignee)

Comment 3

14 years ago
Created attachment 148270 [details] [diff] [review]
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.
Attachment #146669 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #148270 - Flags: review?(blizzard)
(Assignee)

Comment 4

14 years ago
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.
Summary: [gtk2]nsWindow::IsVisible() does not return the right value → [gtk2]combo box shows up unexpectly due to nsWindow::IsVisible() does not return the right value
(Assignee)

Updated

14 years ago
Attachment #148270 - Flags: review?(blizzard) → review?(bryner)
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.
(Assignee)

Comment 6

14 years ago
Created attachment 149460 [details] [diff] [review]
patch addressing Blizzard's comment

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

Updated

14 years ago
Attachment #148270 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #149460 - Flags: superreview?(bryner)
Attachment #149460 - Flags: review?(blizzard)
(Assignee)

Updated

14 years ago
Attachment #148270 - Flags: review?(bryner)
Attachment #149460 - Flags: review?(blizzard) → review+
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").
Attachment #149460 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 8

14 years ago
patch checked in with bryner's comments.

Thank you very much!
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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....
Attachment #149460 - Flags: approval1.7?
Attachment #149460 - Flags: approval1.7? → approval1.7+

Updated

14 years ago
Keywords: fixed1.7

Updated

14 years ago
Whiteboard: fixed-aviary1.0
You need to log in before you can comment on or make changes to this bug.