Last Comment Bug 295540 - Multi-monitor: download "save-as" dialog remains on secondary monitor even if detached
: Multi-monitor: download "save-as" dialog remains on secondary monitor even if...
Status: RESOLVED FIXED
: polish
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: x86 Windows XP
: -- critical with 3 votes (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
: Jim Mathies [:jimm]
Mentors:
: 378757 444262 487914 (view as bug list)
Depends on: ContextMenuScreen 684506
Blocks:
  Show dependency treegraph
 
Reported: 2005-05-25 16:53 PDT by Martin Crundall
Modified: 2011-09-03 10:53 PDT (History)
13 users (show)
benjamin: blocking1.9.2-
benjamin: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible. (6.24 KB, patch)
2011-08-11 08:11 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible. v2. (7.18 KB, patch)
2011-08-24 08:41 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible. v3. (7.18 KB, patch)
2011-08-24 08:48 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Martin Crundall 2005-05-25 16:53:08 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

The file "save-as" modal dialog box doesn't appear to update its own position
when sections of the desktop are removed ("de-attaching" a second monitor, which
created additional desktop space that had been used to show firefox).

The symptom is that -- after selecting a "save link as" -- the user's firefox
window becomes unresponsive -- only beeping when clicked on (because "save-as"
dialog is modal, I'm guessing).

workaround: if a user just presses "enter", the file gets saved to whatever
directory/filename comes up as the default and is then accessible from the
download manager window (which is correctly showing on the visible desktop space).

The "save-as" dialog box doesn't seem to recognize the fact that the desktop
area has changed.

Reproducible: Always

Steps to Reproduce:
This sequence replicates the problem:
1.  start windows
2.  attach second monitor, modify desktop properties so monitor appears to
"left" of main monitor as a desktop extension
3.  start and use firefox on the new, "left" monitor.  download file.  file-save
dialog moved to "left" monitor, populated and closed.
4.  shutdown firefox.  detatch second monitor by modifying desktop properties.  
5.  restart firefox.  go somewhere and try to download something.
Actual Results:  
unresponsive firefox window (click-ding, click-ding)

Expected Results:  
the file "save-as" dialog should be re-positioned in to the visible desktop space.
Comment 1 Steve England [:stevee] 2005-05-26 00:33:57 PDT
Dupe of/Related to bug 245418?
Comment 2 Garth 2006-09-06 20:07:48 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

This bug is easily reproducable and terribly annoying if working on a laptop and switching between single/multiple monitors.

From my every day usage, the Open/Save dialogs appear to be the only ones affected by this.

Comment 3 guanxi 2007-04-18 10:17:04 PDT
Confirming on FF 2.0.0.3 on WinXP Pro SP2.

Notes:

    *   IMPORTANT: It's a serious problem for end users:
        -   Modal dialogs (like Save As) effectively *disable* FF
        -   The user is given no clue what the cause of the
            problem is; FF simply stops working.
        -   If the user is disconnected long term from the
            secondary monitor, they have no opportunity to fix it.
            They'll need help from an advanced user. who'll have
            to stumble on the right question ('was another monitor
            attached?')
        -   They can't use affected dialogs until it's fixed

    *   Not sure what other dialogs are affected. See bug 368746 for
        a similar problem with page info dialogs.

    *   Workaround: With the dialog open, press ALT+SPACE. The
        Windows window control menu (Restore/Move/Size/etc) will
        appear. Click Move, the move the dialog with the arrow
        keys on the keyboard.

    *   Reproduced on mine with secondary monitor to right of main
        monitor.


Severity -> Critical

(Not a dupe of bug 245418, which was about context menus.)
Comment 4 Steve England [:stevee] 2007-04-18 10:23:55 PDT
This bug is not critical ("crashes, loss of data, severe memory leak")
Comment 5 guanxi 2007-04-18 14:34:54 PDT
Steve - The bug hangs Firefox for affected users with no workaround (or not one that 99% of users will figure out). If hang = crash, which it does in this context, it's "critical".

Given it only affects a small number of users (those using multi-monitor setups that have detached a monitor), but they are hit hard.  

To shorten this discussion, I'll set it to Major as a compromise and, if you agree with my reasoning above, just change it to Critical.
Comment 6 Aiko 2008-12-23 03:43:02 PST
*** Bug 378757 has been marked as a duplicate of this bug. ***
Comment 7 BS87 2009-02-23 08:01:24 PST
Please see comment #3 in bug 378757.
Comment 8 Matthias Versen [:Matti] 2009-04-11 14:48:08 PDT
*** Bug 444262 has been marked as a duplicate of this bug. ***
Comment 9 Matthias Versen [:Matti] 2009-04-11 14:48:13 PDT
*** Bug 487914 has been marked as a duplicate of this bug. ***
Comment 10 rupert.parson 2009-09-04 01:42:55 PDT
This bug also affects users (like me) who have an expanded desktop across two monitors at work.

1. Open firefox and move it to the 2nd screen of the expanded desktop.
2. Remote desktop into the machine from a single-monitor system.
3. Open a modal file chooser dialog in firefox.
4. The dialog will open "off-screen" and make firefox appear to have hung.
Comment 11 Timothy Nikkel (:tnikkel) 2009-09-30 17:45:54 PDT
I failed to reproduce this on Firefox 2.0, 3.0, 3.5, and a recent mozilla-central nightly using the steps in comment 0. Unless the steps actually mean for one to physically disconnect the second monitor's cable from the back of the computer; but it sounds to me like just modifying the desktop properties is intended.
Comment 12 Donald Bell 2009-10-18 11:14:02 PDT
I tried to recreate this problem on the Windows XP machine that I originally had this problem on.  I am no longer able to reproduce this bug on that machine.

I think this defect has been indirectly fixed by a Windows or Firefox update.
Comment 13 Brian R. Bondy [:bbondy] 2011-08-10 11:23:38 PDT
OK I've been playing around with reproducing this for about an hour and I have a consistent way to reproduce it.

Windows 7: The problem seems fixed, probably by Windows itself.  
I even tried the original steps and my new steps below with Firefox 2.0.0.3 and could not reproduce on Win7.

Windows Vista: Did not try.

Windows XP SP3: 
Problem 100% reproducible with FF 2.0.0.3, FF5, and 2011-08-10 Nightly with the following steps: 
1. Setup a primary monitor and an extended monitor.
2. Open Firefox and move it to the primary monitor.
3. Right click on a link and select save as...
4. Move the popup save as dialog to the extended monitor.
5. Close the popup save as dialog by pressing cancel.
6. Optional: Verify if that you do step 3. It should open in the extended monitor.  Cancel out again. 
7. Go to Windows XP settings and disable the extended monitor.
8. Right click and save as the same link as in step 4. 
Result: The window is disabled because of the modal popup that you can't see.
Expected result: The window should open on the primary monitor. 
Note: If you do a step 9 and re-enable the extended monitor while firefox seems frozen, you will see the popup at the original location.
Comment 14 Brian R. Bondy [:bbondy] 2011-08-10 11:36:38 PDT
2 more additional important points I found: 

1. With the steps in Comment 13 you can also reproduce this with an open file dialog.   

2. This bug is equally reproducible with a single monitor if you set your resolution to something high like 1600x1200, move the window towards the bottom, and then adjust your resolution to 800x600.
Comment 15 Brian R. Bondy [:bbondy] 2011-08-11 08:11:06 PDT
Created attachment 552379 [details] [diff] [review]
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible.

Setting you as the reviewer since you've been doing the other filpicker related patch reviews.
Comment 16 Jim Mathies [:jimm] 2011-08-23 09:14:33 PDT
Comment on attachment 552379 [details] [diff] [review]
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible.

Review of attachment 552379 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/nsFilePicker.cpp
@@ +119,5 @@
> +static UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg,
> +                                        WPARAM wParam, LPARAM lParam) 
> +{
> +  // Define an arbitrary user defined message we will post back to ourselves
> +  static const int UWM_ENSUREVISIBLE = WM_APP + 14159;

Hmm, could we move this to nsWindowDefs.h w/a good comment maybe? I know that doesn't fit perfectly but for the time being it's better than burying it here. We have a few custom message defined there currently.

Independent of that, looks good.
Comment 17 Brian R. Bondy [:bbondy] 2011-08-24 08:41:33 PDT
Created attachment 555416 [details] [diff] [review]
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible. v2.

Moved user defined message and renamed to match the other user defined messages with MOZ_ prefix instead of UWM.  Marking as r+ since it was previously r+'ed by :jimm.
Comment 18 Brian R. Bondy [:bbondy] 2011-08-24 08:42:12 PDT
Comment on attachment 552379 [details] [diff] [review]
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible.

># HG changeset patch
># Parent 95de91ae34a5a0f456f016be2fd2d8c819ad7885
># User Brian R. Bondy <netzen@gmail.com>
>Bug 295540 - File picker popups are shown in last visible position, but should not if those positions are now non visible.
>
>diff --git a/widget/src/windows/nsFilePicker.cpp b/widget/src/windows/nsFilePicker.cpp
>--- a/widget/src/windows/nsFilePicker.cpp
>+++ b/widget/src/windows/nsFilePicker.cpp
>@@ -93,36 +93,81 @@ int CALLBACK BrowseCallbackProc(HWND hwn
>     if (filePath)
>       ::SendMessageW(hwnd, BFFM_SETSELECTIONW,
>                      TRUE /* true because lpData is a path string */,
>                      lpData);
>   }
>   return 0;
> }
> 
>+static void EnsureWindowVisible(HWND hwnd) 
>+{
>+  // Obtain the monitor which has the largest area of intersection 
>+  // with the window, or NULL if there is no intersection.
>+  HMONITOR monitor = MonitorFromWindow(hwnd, MONITOR_DEFAULTTONULL);
>+  if (!monitor) {
>+    // The window is not visible, we should reposition it to the same place as its parent
>+    HWND parentHwnd = GetParent(hwnd);
>+    RECT parentRect;
>+    GetWindowRect(parentHwnd, &parentRect);
>+    BOOL b = SetWindowPos(hwnd, NULL, 
>+                          parentRect.left, 
>+                          parentRect.top, 0, 0, 
>+                          SWP_NOACTIVATE | SWP_NOSIZE | SWP_NOZORDER);
>+  }
>+}
>+
>+// Callback hook which will ensure that the window is visible
>+static UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg,
>+                                        WPARAM wParam, LPARAM lParam) 
>+{
>+  // Define an arbitrary user defined message we will post back to ourselves
>+  static const int UWM_ENSUREVISIBLE = WM_APP + 14159;
>+  if (msg == WM_NOTIFY) {
>+    LPOFNOTIFYW lpofn = (LPOFNOTIFYW) lParam;
>+    if (!lpofn || !lpofn->lpOFN) {
>+      return 0;
>+    }
>+    
>+    if (CDN_INITDONE == lpofn->hdr.code) {
>+      // The Window will be automatically moved to the last position after
>+      // CDN_INITDONE.  We post a message to ensure the window will be visible
>+      // so it will be done after the automatic last position window move.
>+      PostMessage(hwnd, UWM_ENSUREVISIBLE, 0, 0);
>+    }
>+  } else if (msg == UWM_ENSUREVISIBLE) {
>+    EnsureWindowVisible(GetParent(hwnd));
>+  }
>+  return 0;
>+}
>+
>+
> // Callback hook which will dynamically allocate a buffer large
> // enough for the file picker dialog.
>-static UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg,
>-                                        WPARAM wParam, LPARAM lParam)
>+static UINT_PTR CALLBACK MultiFilePickerHook(HWND hwnd, UINT msg,
>+                                             WPARAM wParam, LPARAM lParam)
> {
>   switch (msg) {
>     case WM_INITDIALOG:
>       {
>         // Finds the child drop down of a File Picker dialog and sets the 
>         // maximum amount of text it can hold when typed in manually.
>         // A wParam of 0 mean 0x7FFFFFFE characters.
>         HWND comboBox = FindWindowEx(GetParent(hwnd), NULL, 
>                                      L"ComboBoxEx32", NULL );
>         if(comboBox)
>           SendMessage(comboBox, CB_LIMITTEXT, 0, 0);
>       }
>       break;
>     case WM_NOTIFY:
>       {
>         LPOFNOTIFYW lpofn = (LPOFNOTIFYW) lParam;
>+        if (!lpofn || !lpofn->lpOFN) {
>+          return 0;
>+        }
>         // CDN_SELCHANGE is sent when the selection in the list box of the file
>         // selection dialog changes
>         if (lpofn->hdr.code == CDN_SELCHANGE) {
>           HWND parentHWND = GetParent(hwnd);
> 
>           // Get the required size for the selected files buffer
>           UINT newBufLength = 0; 
>           int requiredBufLength = CommDlg_OpenSave_GetSpecW(parentHWND, 
>@@ -158,17 +203,18 @@ static UINT_PTR CALLBACK FilePickerHook(
> 
>             lpofn->lpOFN->lpstrFile = filesBuffer;
>             lpofn->lpOFN->nMaxFile  = newBufLength;
>           }
>         }
>       }
>       break;
>   }
>-  return 0;
>+
>+  return FilePickerHook(hwnd, msg, wParam, lParam);
> }
> 
> NS_IMETHODIMP nsFilePicker::ShowW(PRInt16 *aReturnVal)
> {
>   NS_ENSURE_ARG_POINTER(aReturnVal);
> 
>   // suppress blur events
>   if (mParentWidget) {
>@@ -241,17 +287,19 @@ NS_IMETHODIMP nsFilePicker::ShowW(PRInt1
>     
>     ofn.lpstrTitle   = (LPCWSTR)mTitle.get();
>     ofn.lpstrFilter  = (LPCWSTR)filterBuffer.get();
>     ofn.nFilterIndex = mSelectedType;
>     ofn.hwndOwner    = (HWND) (mParentWidget.get() ? mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : 0); 
>     ofn.lpstrFile    = fileBuffer;
>     ofn.nMaxFile     = FILE_BUFFER_SIZE;
>     ofn.Flags = OFN_SHAREAWARE | OFN_LONGNAMES | OFN_OVERWRITEPROMPT |
>-                OFN_HIDEREADONLY | OFN_PATHMUSTEXIST;
>+                OFN_HIDEREADONLY | OFN_PATHMUSTEXIST | OFN_ENABLESIZING | 
>+                OFN_EXPLORER | OFN_ENABLEHOOK;
>+    ofn.lpfnHook = FilePickerHook;
> 
>     // Handle add to recent docs settings
>     nsCOMPtr<nsIPrivateBrowsingService> pbs =
>       do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
>     PRBool privacyModeEnabled = PR_FALSE;
>     if (pbs) {
>       pbs->GetPrivateBrowsingEnabled(&privacyModeEnabled);
>     }
>@@ -300,26 +348,25 @@ NS_IMETHODIMP nsFilePicker::ShowW(PRInt1
>     
>     MOZ_SEH_TRY {
>       if (mMode == modeOpen) {
>         // FILE MUST EXIST!
>         ofn.Flags |= OFN_FILEMUSTEXIST;
>         result = ::GetOpenFileNameW(&ofn);
>       }
>       else if (mMode == modeOpenMultiple) {
>-        ofn.Flags |= OFN_FILEMUSTEXIST | OFN_ALLOWMULTISELECT | 
>-                     OFN_EXPLORER | OFN_ENABLEHOOK;
>+        ofn.Flags |= OFN_FILEMUSTEXIST | OFN_ALLOWMULTISELECT;
> 
>         // The hook set here ensures that the buffer returned will always be
>         // long enough to hold all selected files.  The hook may modify the
>         // value of ofn.lpstrFile and deallocate the old buffer that it pointed
>         // to (fileBuffer). The hook assumes that the passed in value is heap 
>         // allocated and that the returned value should be freed by the caller.
>         // If the hook changes the buffer, it will deallocate the old buffer.
>-        ofn.lpfnHook = FilePickerHook;
>+        ofn.lpfnHook = MultiFilePickerHook;
>         fileBuffer.forget();
>         result = ::GetOpenFileNameW(&ofn);
>         fileBuffer = ofn.lpstrFile;
>       }
>       else if (mMode == modeSave) {
>         ofn.Flags |= OFN_NOREADONLYRETURN;
> 
>         // Don't follow shortcuts when saving a shortcut, this can be used
Comment 19 Brian R. Bondy [:bbondy] 2011-08-24 08:48:20 PDT
Created attachment 555420 [details] [diff] [review]
Patch for file picker popups are shown in last visible position, but should not if those positions are now non visible. v3.

Typo in comment.
Comment 20 Brian R. Bondy [:bbondy] 2011-08-31 13:50:41 PDT
Pushed to try, results:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ec8056cbd584
Comment 21 Brian R. Bondy [:bbondy] 2011-09-01 07:23:26 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/59c205c5d0ad
Comment 22 Ed Morley [:emorley] 2011-09-01 13:53:49 PDT
http://hg.mozilla.org/mozilla-central/rev/59c205c5d0ad

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