Last Comment Bug 581697 - restored window size and position lost when using Aero Snap
: restored window size and position lost when using Aero Snap
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 3 votes (vote)
: mozilla8
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
:
:
Mentors:
Depends on:
Blocks: win7support
  Show dependency treegraph
 
Reported: 2010-07-24 10:39 PDT by torhu
Modified: 2011-07-29 03:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial fix for bug 581697 - Avoids calling ::ShowWindow if we're already restoring the window (1.37 KB, patch)
2011-07-22 19:52 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
Patch for 581697 - Avoid calling ::ShowWindow if we're already restoring the window (1.06 KB, patch)
2011-07-25 15:00 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
Simple patch. Strange whitespace removed (1.20 KB, patch)
2011-07-27 16:34 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
jmathies: review+
Details | Diff | Splinter Review
Checkin-ready patch (1.46 KB, patch)
2011-07-28 15:46 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Splinter Review

Description torhu 2010-07-24 10:39:06 PDT
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 2010-09-15 16:09:20 PDT
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 2010-10-19 10:40:45 PDT
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] 2010-10-19 11:41:55 PDT
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 2010-10-19 12:34:32 PDT
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 2010-10-19 12:45:25 PDT
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] 2010-10-19 13:30:47 PDT
(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 2010-10-19 14:37:17 PDT
Yes, that's right.  The original state is lost.
Comment 8 Asa Dotzler [:asa] 2010-10-24 14:37:31 PDT
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 2010-10-24 16:46:06 PDT
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) 2011-01-16 05:23:03 PST
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 [:streetwolf] 2011-06-24 14:50:19 PDT
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] 2011-07-18 23:36:03 PDT
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] 2011-07-22 19:50:37 PDT
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] 2011-07-22 19:52:06 PDT
Created attachment 547883 [details] [diff] [review]
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] 2011-07-25 15:00:43 PDT
Created attachment 548300 [details] [diff] [review]
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] 2011-07-27 07:48:36 PDT
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] 2011-07-27 07:55:26 PDT
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] 2011-07-27 16:34:34 PDT
Created attachment 548966 [details] [diff] [review]
Simple patch.  Strange whitespace removed

Hopefully the strange whitespace is gone now...
Comment 19 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-07-27 16:39:57 PDT
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] 2011-07-28 15:46:45 PDT
Created attachment 549251 [details] [diff] [review]
Checkin-ready patch

Added info to the patch needed for checkin
Comment 22 Marco Bonardo [::mak] 2011-07-29 03:11:15 PDT
http://hg.mozilla.org/mozilla-central/rev/567a0c458ae0

Note You need to log in before you can comment on or make changes to this bug.