581697 - restored window size and position lost when using Aero Snap

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[torhu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8
Build Identifier: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/4.0b2-candidates/build1/win32/en-US/Firefox%20Setup%204.0%20Beta%202.exe

I tried this with 4.0b2 build 1 (upgraded install from 3.6.6), and also with a clean install of 4.0b1 from http://portableapps.com/apps/internet/firefox_portable/test.



Reproducible: Always

Steps to Reproduce:
0. Place the Firefox window in the middle of the screen, not maximized.
1. Fill right half of screen by pressing winkey+right arrow.
2. Minimize without first restoring window size and position by pressing winkey+M.
3. Alt-tab back to Firefox.
4. Press winkey+left arrow.
Actual Results:  
The window now fills the left half of the screen.

Expected Results:  
Original window size and position should have been restored.

This also happens if you press winkey+D twice instead of winkey+M and alt-tabbing.

Comment 1

[daf]

works for me using:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9

Comment 2

[torhu]

This bug is about Firefox 4.0 beta, not 3.x.  It's still present in 4.0 beta 6.

Comment 3

[Jim Mathies [:jimm]]

Maybe I didn't follow the right steps.

> 0. Place the Firefox window in the middle of the screen, not maximized.
> 1. Fill right half of screen by pressing winkey+right arrow.
> 2. Minimize without first restoring window size and position by pressing
> winkey+M.

done, the window minimizes.

> 3. Alt-tab back to Firefox.

the window pops back up on the left hand side of the screen.

> 4. Press winkey+left arrow.

first press nothing happen, second press it pops to the right hand side of the screen.

Comment 4

[torhu]

Looks like you're mixing left and right in your description of what happens.  Other than that, you have reproduced the bug.

Comment 5

[torhu]

Wait, are you really using 4 beta 6?  The first keypress not getting registered doesn't happen in the 4.0 betas for me, but IIRC it happens with 3.6.x.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> Wait, are you really using 4 beta 6?  The first keypress not getting registered
> doesn't happen in the 4.0 betas for me, but IIRC it happens with 3.6.x.

I'm working with a nightly.

The bug as I understand it is that the window doesn't cycle through three states, it just cycles through two: left and right side of the screen?

Comment 7

[torhu]

Yes, that's right.  The original state is lost.

Comment 8

[Asa Dotzler [:asa]]

Win+Left and Win+right should cycle like this a)restored, b)right side fill, c)restored, d)left side fill, e)restored, f)right side fill, etc. etc. 

This bug is that if you minimize the window at step 2 in the original description, that cycle becomes a)right side fill, b)left side fill, c)right side fill, etc. etc. 

I can't reproduce except with that minimizing step. Take that step out and we cycle through the three states properly. 

This is a minor bug, but a legitimate one. Confirming with latest trunk nightly on Windows 7.

Comment 9

[torhu]

It also happens if you click on the minimize button (or use the corresponding window menu item), or if you press winkey+home (which I do frequently).  Basically, you have to remember not to do any of those things with Firefox in a 'snapped' state.  Even if Firefox is in the background.  Kind of a PITA, if you ask me.

For me, the nicest thing about snap is not how easy it to make the window fill the left or right half the screen, but how easy it is to go back to the original size and position.

Comment 10

[Luke Iliffe (Harlequin99)]

This is still an issue with the beta 10pre nightlies. The incorrect window mode also occurs (i.e.window fails to restore to proper window size), after a restart from a windows snapped position.

I have a slightly simpler STR:

1. Start minefield with a new profile.
2. Resize it so it sits in the middle of the screen.
3. Snap minefield to edge of screen (win+left arrrow) or drag.
4. Quit application.
5. Restart application.

Expected:
Minefield restarts with window size from step 2 above, nicely in the middle of the screen (the restore window mode).

Observed:
Minefield restarts with a window size from step 3, the snapped window size. The window is not snapped however.

Repeat steps 1-5 with windows explorer and on restart it behaves as expected, with a window size of the restored mode not the snapped mode.

Comment 11

[Gary [:streetwolf52]]

Any relationship to mu problem?

https://bugzilla.mozilla.org/show_bug.cgi?id=665249

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 - Build ID: 20110624082701

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

It looks like, upon restoring, we're getting two WM_WINDOWPOSCHANGED messages: The first message looks correct (it has flags indicating that the size and position should be unchanged from what they were previously), but then we get another WM_WINDOWPOSCHANGED message that places us in the snapped position incorrectly (i.e. it has the size and position specified rather than the "don't change from previous values" flags specified).  When I use the debugger to skip the OnWindowPosChanged call for the errant WM_WINDOWPOSCHANGED message, the "aero snap" functionality works as expected (the window cycles through the "left snap," "original position," and "right snap" positions correctly after restoring).

Now to determine where the errant WM_WINDOWPOSCHANGED message is coming from...

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

When restoring our window, the sequence of events should look something like this:
  1) We process a `WM_WINDOWPOSCHANGING` message containing our unsnapped size/position and a "state changed" flag (to indicate we've gone from "minimized" to "restored"
  2) Windows positions our window for us
  3) We process a `WM_WINDOWPOSCHANGED` message, with our snapped size/pos and the "state changed" flag

However, we need to keep our NS structures in sync, so we dispatch `NS_SIZEMODE` events when we receive `WM_*` messages with the "state changed" flag.  Our processing of any `NS_SIZEMODE` event involves calling `::ShowWindow`, which effectively injects window messages into our sequence from above.  The following now happens between steps 1 and 2 above:
  A) While processing the `WM_WINDOWPOSCHANGING` message, we dispatch an `NS_SIZEMODE` event, landing us in `nsWindow::SetSizeMode` where we call `::ShowWindow`
  B) We now find ourselves processing *another* `WM_WINDOWPOSCHANGING` message.  This one does *not* have the "state change" flag set, but the size/pos is the same as the previous message
  C) Windows repositions our window (stepping in the debugger will show the window in the original restored/unsnapped position)
  D) We process a `WM_WINDOWPOSCHANGED` message corresponding to the `WM_WINDOWPOSCHANGING` message we just processed

Since Windows repositions our window during step C here, it gets confused when it tries to reposition the window again in step 2 from above, and instead of maintaining our old restored position, it positions us in the snapped location but not in a snapped state.

The solution seems to be to avoid repositioning our window between receiving a `WM_WINDOWPOSCHANGING` and its corresponding `WM_WINDOWPOSCHANGED`.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Initial fix for bug 581697 - Avoids calling ::ShowWindow if we're already restoring the window

I've written a small patch that makes this bug go away, but we'll probably want to add more logic to the `if` that determines whether `::ShowWindow` is called.

:jimm - I'm not sure about protocol/etiquette for "initial patch ideas."  I've added you as a reviewer because I'd like you to take a look at this, but I think the `if` block should probably be thought out a little more before this is submitted.  Let me know if I shouldn't have set the "review?" flag yet.  Thanks!

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch for 581697 - Avoid calling ::ShowWindow if we're already restoring the window

I think this patch makes a little more sense: We just handle the issue that we know and expect, while modifying as few code paths as possible.

Comment 16

[Jim Mathies [:jimm]]

Seems to match the behavior of IE, and I don't see any bugs. One minor issue, if you close the window while in the 'side' mode, when session restore restores the window, it places it back on the side of the desktop. I think we can live with that unless you have an idea on how to prevent it. Also tested F11 full screen modes, those seem to be working ok with this as well.

Comment 17

[Jim Mathies [:jimm]]

nit:

+    // Don't call ::ShowWindow if we're trying to "restore" a window that is
+    // already in a normal state.  Prevents bug #581697.á

You have some funny white spacing in there, and there's no need to mention the bug number. hg blame takes care of that.

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Simple patch. Strange whitespace removed

Hopefully the strange whitespace is gone now...

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I looked into the issue with closing while restoring.  It seems like our issue there is that, in `nsXULWindow::SavePersistentAttributes()`, we're calling `GetPositionAndSize` to tell us what position/size to store, but that function gives us the snapped position/size.  I think the solution to that issue would be to use a different function to get the restored pos/size for saving.  That issue might be related to bugs #665249 and/or #646946.

Comment 20

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Checkin-ready patch

Added info to the patch needed for checkin

Comment 21

[Jim Mathies [:jimm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/567a0c458ae0

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/567a0c458ae0