79720 - [FIX]selects do not roll up when winembed is minimized

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Here is the patch:
Index: src/windows/nsWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v
retrieving revision 3.333
diff -u -r3.333 nsWindow.cpp
--- nsWindow.cpp        2001/05/01 13:29:14     3.333
+++ nsWindow.cpp        2001/05/03 10:54:26
@@ -830,7 +830,7 @@
     if (inMsg == WM_ACTIVATE || inMsg == WM_NCLBUTTONDOWN || inMsg == 
WM_LBUTTONDOWN ||
       inMsg == WM_RBUTTONDOWN || inMsg == WM_MBUTTONDOWN || 
       inMsg == WM_NCMBUTTONDOWN || inMsg == WM_NCRBUTTONDOWN || inMsg == 
WM_MOUSEACTIVATE ||
-      inMsg == WM_MOUSEWHEEL || inMsg == uMSH_MOUSEWHEEL)
+      inMsg == WM_MOUSEWHEEL || inMsg == uMSH_MOUSEWHEEL || inMsg == WM_SIZE)
     {
       // Rollup if the event is outside the popup.
       PRBool rollup = !nsWindow::EventIsInsideWindow(inMsg, 
(nsWindow*)gRollupWidget);

Comment 2

[Kevin McCluskey (gone)]

r=[e-mail]

Comment 3

[David Hyatt]

sr=hyatt

Comment 4

[dcone (gone)]

checked into the .9 branch

Comment 5

[rods (gone)]

checked into tip - fixed

Comment 6

[:deanis74]

This patch doesn't sit well with me.  I don't know why.  Shouldn't WM_ACTIVATE
already handle minimizing the window, since it loses focus?

Comment 7

[Brendan Eich [:brendan]]

Hyatt says he conditioned his sr= (via e-mail) on testing that this didn't hose
menus.  Was the patch so tested?  Did the post-0.9 branch get hosed too?

/be

Comment 8

[David Hyatt]

I had to back out the fix for this to correct today's smoketest blockers.  

Comment 9

[David Hyatt]

You can't roll up in response to WM_SIZE.  Menus are created dynamically, and 
they can change their sizes during construction.  Seems like another solution 
will have to be found for this.

Comment 10

[Kevin McCluskey (gone)]

Patch causes regression. Can not change the content of a combobox. Need a 
different solution.

Comment 11

[Brendan Eich [:brendan]]

Someone gonna back out of the 0.9 branch too?

/be

Comment 12

[Kevin McCluskey (gone)]

Don, Can you back this out of the 0.9 branch?

Comment 13

[dcone (gone)]

I pulled that last change out of the .9 branch.

Comment 14

[rods (gone)]

Instead of WM_SIZE, I am using WM_ACTIVATEAPP, I think this is much safer. I 
have done a lot of testing, this time with the new code actually linked in.

Index: nsWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v
retrieving revision 3.338
diff -u -r3.338 nsWindow.cpp
--- nsWindow.cpp        2001/05/12 03:49:51     3.338
+++ nsWindow.cpp        2001/05/14 03:20:13
@@ -828,7 +828,7 @@
     if (inMsg == WM_ACTIVATE || inMsg == WM_NCLBUTTONDOWN || inMsg == WM_LBUTTO
NDOWN ||
       inMsg == WM_RBUTTONDOWN || inMsg == WM_MBUTTONDOWN ||
       inMsg == WM_NCMBUTTONDOWN || inMsg == WM_NCRBUTTONDOWN || inMsg == WM_MOU
SEACTIVATE ||
-      inMsg == WM_MOUSEWHEEL || inMsg == uMSH_MOUSEWHEEL)
+      inMsg == WM_MOUSEWHEEL || inMsg == uMSH_MOUSEWHEEL || inMsg == WM_ACTIVAT
EAPP)
     {
       // Rollup if the event is outside the popup.
       PRBool rollup = !nsWindow::EventIsInsideWindow(inMsg, (nsWindow*)gRollupW
idget);

Comment 15

[:deanis74]

If we're getting a WM_ACTIVATEAPP, shouldn't there also be a WM_ACTIVATE in 
there, which is already handled in this code?

Comment 16

[Kevin McCluskey (gone)]

I added the WM_ACTIVATEAPP change on WINNT and it doesn't cause any ill-effects.
Combobox'es and menu's behave normally. (However, I don't see the original
problem described in this bug with or without the change. For me, The combo-box
always rolls up when the winembed window is minimized.)

r=[e-mail]

Comment 17

[:deanis74]

So if you don't see the original problem, how can you confirm this patch does 
anything?

I'm not trying to cause trouble here, I'm just wondering if someone can confirm 
that this isn't already trapped with WM_ACTIVATE?

Comment 18

[rods (gone)]

I think Kevin's point is that although he can't reproduce the problem on his 
machine, the patch doesn't appear to cause any type of regression.

Comment 19

[Kevin McCluskey (gone)]

"I think Kevin's point is that although he can't reproduce the problem on his 
machine, the patch doesn't appear to cause any type of regression."

Thats right. 

Comment 20

[:deanis74]

Understood.  But my question is whether the patch actually addresses the 
problem, or is needed, since Kevin is unable to replicate the problem without 
the patch?

No regressions is definitely a good thing, though!

Comment 21

[rods (gone)]

I am on NT and I can see the problem and it does fix it for me.

Comment 22

[:deanis74]

OK, cool.  I still think it's odd that WM_ACTIVATE doesn't handle it, but OK.

Do we need to worry about only doing this when being deactivated, ie. if 
wParam==FALSE?

Comment 23

[David Hyatt]

sr=hyatt, but test context menus and menus to make sure there are no 
regressions.  Thanks.

Comment 24

[rods (gone)]

fixed

Comment 25

[Chad Austin]

verified in winembed (Moz build id: 2001060509)