63052 - Window state not persisted if state is changed by doubleclicking titlebar

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Blake Ross]

Build ID: 12/16 trunk (tip)

Steps to Reproduce:

(1) Open an instance of Navigator (not in the maximized state).
(2) Open the sidebar if it's not already.
(3) Double click the titlebar to maximize the window.
(4) Open a new Navigator window (File | New Navigator Window, Ctrl+N)

Result: it's not maximized, but it should be; this works if you use the 
maximize button in the top right corner (in step 3).  see bug 32148.

Comment 1

[Blake Ross]

(not restricted to maximized state -- we apparently never recognize that state 
is changed if you change it using the titlebar)

Comment 2

[Péter Bajusz]

No wonder it doesn't work...
Recognizing states currently implemented as:

------------------------------
case WM_SYSCOMMAND:
  if (wParam == SC_MINIMIZE || wParam == SC_MAXIMIZE || wParam == SC_RESTORE) {
    nsSizeModeEvent event;
    event.eventStructType = NS_SIZEMODE_EVENT;
    event.mSizeMode = nsSizeMode_Normal;
    if (wParam == SC_MINIMIZE)
      event.mSizeMode = nsSizeMode_Minimized;
    if (wParam == SC_MAXIMIZE)
      event.mSizeMode = nsSizeMode_Maximized;
    InitEvent(event, NS_SIZEMODE);

    result = DispatchWindowEvent(&event);
    NS_RELEASE(event.widget);
  }
  break;
------------------------------

This is all wrong.
WM_SYSCOMMAND is a "Keyboard Accelerator Message" not a "Window Message".

This should be handled in eighter WM_SIZE or WM_WINDOWPOSCHANGED.
Handling in WM_SIZE is the simpler it goes something like that:
------------------------------
case WM_SIZE:
  {
    nsSizeModeEvent event;
    event.eventStructType = NS_SIZEMODE_EVENT;
    if (wParam == SIZE_MAXIMIZED)
      event.mSizeMode = nsSizeMode_Maximized;
    else if (wParam == SIZE_MINIMIZED)
      event.mSizeMode = nsSizeMode_Minimized;
    else
      event.mSizeMode = nsSizeMode_Normal;
    InitEvent(event, NS_SIZEMODE);

    result = DispatchWindowEvent(&event);
    NS_RELEASE(event.widget);
  }
  break;
------------------------------

Handling in WM_WINDOWPOSCHANGED could be done by putting the following in the
"if" inside the already existing "case WM_WINDOWPOSCHANGED"
------------------------------
WINDOWPLACEMENT pl;
pl.length = sizeof(pl);
::GetWindowPlacement(mWnd, &pl);

nsSizeModeEvent event;
event.eventStructType = NS_SIZEMODE_EVENT;
if (pl.showCmd == SW_SHOWMAXIMIZED)
  event.mSizeMode = nsSizeMode_Maximized;
else if (pl.showCmd == SW_SHOWMINIMIZED)
  event.mSizeMode = nsSizeMode_Minimized;
else
  event.mSizeMode = nsSizeMode_Normal;
InitEvent(event, NS_SIZEMODE);

result = DispatchWindowEvent(&event);
NS_RELEASE(event.widget);
------------------------------

On a side note, I wonder why WM_WINDOWPOSCHANGED is used in the first place
instead of WM_SIZE, when it's only used for detecting resize events.
Its content might be moved into WM_SIZE, possibly saving the ::GetWindowRect call...

I haven't tested the above code(s) in mozilla nor I have diffs since I don't
have the source, I only used lxr. I hope that is helps though.

Comment 3

[Péter Bajusz]

Forgot to say that all of the above is in:
mozilla/widget/src/windows/nsWindow.cpp

Comment 4

[timeless]

Do both events happen when you maximize a window that currently has maximal 
dimensions?
I need to read why we use SC_ (MINIMIZE/MAXIMIZE) and NS_ (SIZEMODE)

references:
<q src="http://msdn.microsoft.com/library/psdk/winui/windows_2pk5.htm">
The WM_SIZE message is sent to a window <em>after</em> its size has changed. 
</q>

<q src="http://msdn.microsoft.com/library/psdk/winui/windows_717r.htm">
The WM_WINDOWPOSCHANGING message is sent to a window whose size, position, or 
place in the Z order is <em>about</em> to change as a result of a call to the 
SetWindowPos function or another window-management function. 

While this message is being processed, modifying any of the values in WINDOWPOS 
affects the window's new size, position, or place in the Z order. An application 
can prevent changes to the window by setting or clearing the appropriate bits in 
the flags member of WINDOWPOS. 
</q>
<q src="http://msdn.microsoft.com/library/psdk/winui/windows_5q90.htm">
The WM_WINDOWPOSCHANGED message is sent to a window whose size, position, or 
place in the Z order has changed as a result of a call to the SetWindowPos 
function or another window-management function. 

Return Values

If an application processes this message, it should return zero. 

Remarks

By default, the DefWindowProc function sends the WM_SIZE and WM_MOVE messages to 
the window. The WM_SIZE and WM_MOVE messages are not sent if an application 
handles the WM_WINDOWPOSCHANGED message without calling DefWindowProc. <em>It is 
more efficient</em> to perform any move or size change processing during the 
WM_WINDOWPOSCHANGED message without calling DefWindowProc. </q>

Conclusion: we should use WM_WINDOWPOSCHANGED. <em>phasis added ;-). portions 
removed from block<q>oute.

Comment 5

[Péter Bajusz]

Well, MSDN is not exactly right...
Windows calls WM_SIZE and WM_MOVE at the window creation without calling
WM_WINDOWPOSCHANGED. (I found that out using a test program.)

What made me (erroneously) think that we could do this in WM_SIZE, is that
nsWindow.cpp cantains a WM_MOVE handler as well. (I think we should file a
separate bug about that...)

I wonder if WM_WINDOWPOSCHANGED not being called on window creation will cause
problems...

Comment 6

[Péter Bajusz]

Attachment: Patch to put NS_SIZEMODE event generation into WM_WINDOWPOSCHANGED

Comment 7

[Péter Bajusz]

Here it is in diff -u.
One more thing is corrected, I didn't put the code inside the existing "if",
because that's triggered only when the size is changed, and it's not garanteed
to happen on maximize... (Since mozilla persists the maximised size, not the
normal size - yet another bug - this case is LIKELY to happen.)

The code is still untested, so no garantees, but it simple enough it might even
work. :)

Comment 8

[timeless]

The patch looks good, but it's up to danm to review. Resetting Platform PC. This
is a win32 problem, and we ignore the fact that win32 runs on things other than PC.

Péter Bajusz: do you have cvs access?

Comment 9

[Péter Bajusz]

Only as anonymous...

Comment 10

[Péter Bajusz]

Resetting platform to PC again.
I know I didn't set it to All, my guess is that it happened because bug 62782.

Comment 11

[Péter Bajusz]

Just managed to get to build with this patch and it works fine for me on WinME.
It also eliminates bug 63053.

Comment 12

[Dan M]

Patch checked in. Thanks Péter!

(Hooking in at WM_SYSCOMMAND was bogus. The trick is to find a place where you'll 
be notified of the impending state change before the window is actually sized. 
Péter found a better one.)

Comment 13

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old