Closed Bug 259816 Opened 17 years ago Closed 16 years ago

Browser steals focus from program selected in 'Open With' dialog

Categories

(Toolkit :: Downloads API, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: richardtallent, Assigned: emaijala+moz)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10

When downloading a file and selecting "Open With", Firefox steals the window
focus from the helper program after executing it, leaving the opened document
behind the browser window. Specifically tested with Excel.

Reproducible: Always
Steps to Reproduce:
1. Choose to download a file
2. Choose "Open With" and a helper application
3. Select "OK"

Actual Results:  
The helper application runs, but Firefox steals focus away and becomes the
topmost window again.

Expected Results:  
Not take back focus, allowing the executed helper application (and, thus, the
downloaded document) to have focus.
(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3)
Gecko/20040913 Firefox/0.10
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3)
Gecko/20040913 Firefox/0.10

Confirmed at home as well on Windows XP (initial report from work on Win2k).
This also happens with 
Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041001 Firefox/0.10.1

In KDE 3.2, Firefox does this also, even if it's on a different desktop. I have
to set the focus prevention level in Control Center to High just to keep FF from
taking focus, but this option causes windows from newly started applications not
to take focus, which isn't very usable. FF steals focus both when opening files
with Firefox and clicking web links in Thunderbird. It's *very* annoying (to say
the least!)

IIRC, when I used Mozilla and Firefox on a Windows 98SE computer, they would
even go so far as to ignore the "Prevent applications from stealing focus"
option set in TweakUI and steal focus anyway. This is incredibly arrogant
behavior, and it definately *ought* to be fixed.
This is a truly annoying bug. Please take a look at him as soon as possible.
Probably is just a single code line that needs to be fixed and the user
experience will be SO MUCH better.
Sometimes the simple things are the most important...
I can confirm this bug as well, both on WinXP and Win2000, my Linux build (from
SUSE RPM) does not have this problem however. *Very* annoying. Also, if one sets
"browser.download.manager.showAlertOnComplete" to false in about:config, no
download complete popup shows, but the focus is still stolen. I hope this gets
fixed soon.
Version: unspecified → Trunk
Also confirmed.  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060131 Firefox/1.5

One thing to note, though:  This bug could be considered a feature IF, say, the "buggy behavior" happened ONLY when opening a file in a new unselected tab AND it opened into the background from the start (e.g. it never flickered in front and then went behind the window.)  I will open a feature request for this if that would be possible:  So can Firefox specify the way that OTHER programs that it calls are opened, as far as whether they open in front or behind, minimized or maximized, etc?
*** Bug 330438 has been marked as a duplicate of this bug. ***
Hello Firefox developers.

There is a while that this ANNOYING bug was reported.

This should be a trivial bug to fix.

Is there any plans to fix this in near future?

Regards
Assignee: bugs → nobody
Component: File Handling → Download Manager
QA Contact: aebrahim-bmo-507 → download.manager
Firefox 1.5.0.2 is out.

This issue has still not been fixed.

It was reported 2004-09-16.

When will this annoying bug be fixed??? Will it ever be?
Taking...
Assignee: nobody → emaijala
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
First stab at an universal fix at core level. This patch makes it so that a toplevel window doesn't take focus when it's shown if the currently active window doesn't belong to the current process. Any objections?
Attachment #220249 - Flags: review?(neil)
So who's calling Show (it is Show right? you didn't cvs diff -p) and why?
nsXULWindow::OnChromeLoaded() -> nsXULWindow::SetVisibility(int aVisibility=1) -> nsWindow::Show(int bState=1). The download manager window doesn't have a chance to load before the external application is launched if the download is already complete. Then when the app is launched the download manager chrome starts loading and steals focus when it's done. 

Good to see that someone finally tries to fix this:

Have looked at the last comment from Ere Maijala which ends like this:
******************
Then when the app is launched the download manager chrome
starts loading and steals focus when it's done.
******************

THIS (my post here) MAY SEEM LIKE A REPETITION OF THE REPORTED PROBLEM, BUT IS A COMMENT TO THE  POST ABOVE, SO PLEASE READ WHOLE COMMENT HERE!

The main problem for my customer is this:
They do a lot of downloading of Word and Excel files.
The Firefox window that contain the link to the file to be downloaded, are put in front of the downloaded file which is auto-opened in Excel or Word application, after the Excel or Word application has opened.

That is especially annyoing and confusing if the Firefox window with the link to the document to be downloaded are maximized, because the end user may not even notice that the Word or Excel file did download and open in their respective application, because it is hidden behind the Firefox window.

I DO NOT, consider it a problem if the Download Manager in Firefox puts itself in front of the opened Excel or Word file/application because of this:
1. The download manager can be set to autohide/close after the download is finished.
2. If the download manager does not close after the download then the download
manager is not taking that much of the screensize and will probably very seldom, if ever, be larger than the Excel or Word application.....

So therefore, if it is only the downloadmanager that now (after the fix), is put in front of the Excel or Word application then that is A LOT BETTER than how
it worked before.

Thumbs up.... (and personally, I would prefer to see the download manager in front of the opened application, since it probably never will hide the downloaded and autopened document). 
(If the download manager was not set to auto-close after download is finished, and the download manager is not minimized by user then I would prefer it to be put in front of the opened/downloaded document. But if the user has minimized the downloadmanager, then I would prefer it to stay minimized (if possible)).

Regards and thanks to Ere for trying to fix this.... much appreciated :-)
Why would you want to see even the download manager steal focus from the opened app? It's not that it's offering you any useful information, right? 

I have to check how this behaves with an auto-closed download manager.
Comment on attachment 220249 [details] [diff] [review]
Patch v1

Ok, makes sense, except I think you ought to call GetAttention in the background case.
Attachment #220249 - Flags: review?(neil) → review+
Attached patch Patch v1.1Splinter Review
Patch with GetAttention(2) that practically just inverts the taskbar button so as not to get too annoying (trying to avoid blinking as a top level window probably isn't asking anything critical). Assuming I can carry on r+...
Attachment #220249 - Attachment is obsolete: true
Attachment #221152 - Flags: superreview?(roc)
Attachment #221152 - Flags: review+
Is this really what we want? Doesn't this mean that if you start Firefox and then switch to another window while it loads, when the window appears it doesn't take focus?
Would you really want it to steal the focus when it has loaded? Well, the question is actually meaningless because the first time an app calls ShowWindow the parameter is ignored. Firefox doesn't steal focus when starting up even without this patch.
Roc?
Attachment #221152 - Flags: superreview?(roc) → superreview+
A little extra info for those that have not seen this:

If Excel and Word is the helper application then the active Firefox window steals focus and places itself on top of helper application.

But if the helper application is Notepad or Acrobat Reader then the helper application stays in front.

Tested on Firefox 1.5.0.2 on Win XP pro.

Checked in to trunk. If everything looks good, I'll check this in to 1.8.1 too.
Attachment #221152 - Flags: approval-branch-1.8.1+
Checked in to 1.8.1.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Will this patch also fix identical Thunderbird bug (when opening an attachment)?
I believe it does, though I haven't tested.
During startup, if an mozilla application does not use the "hidden window", the first mozilla window is hidden -- it isn't activated.  I am reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Doug, do you have a test case?
minimo. ;-)

You launch it, then the first xul window is left in the background.  you have to go out of your way to bring it to the front -- or you can run minimo_runner.exe again which does a FindWindow() and activates the result.

I think we really want something like:

if ( << there is more then one top level window >> && !GetNSWindowPtr(::GetForegroundWindow()))
              mode = SW_SHOWNOACTIVATE;

Thoughts?
hmm. I thought I could simply do something like:

+  if (mIsTopWidgetWindow)
+    sTopWindowCount++;  // new global var. :-)

in StandardWindowCreate(), but it looks like sTopWindowCount is always larger than one when I get to the test in Show().

Well, d'oh. WinCE doesn't support STARTUPINFO so I guess it doesn't do the magic stuff the first time ShowWindow is called. Doug, should we just ifndef the part from WinCE or do you want a flag for the first call?
I don't know if this is relevant, but although I haven't noticed any problems with my standard SeaMonkey build, on my experimental MOZ_XUL_APP build the first window doesn't take focus, except in some cases such as the profile manager.
Neil, any chance for a test case?
yeah ere, i would like to ifndef it.  is that okay with you?
(In reply to comment #32)
> yeah ere, i would like to ifndef it.  is that okay with you?

Yes, go ahead. According to Neil's comment I have something else to fix too, but this is probably completely useless for WinCE anyway. 

Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.643; previous revision: 3.642
done
Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.569.2.24; previous revision: 3.569.2.23
done

WINCE ISSUE RESOLVED.
The build that "works" is the one that includes a splash screen.
Using -quiet makes that build go wrong too.
The initial window is created at the front of the z-order but is not active.
Although with either build the profile selection dialog always takes focus...
Attached patch Additional patch (obsolete) — Splinter Review
Additional patch to make sure we don't call ShowWindow before we want to actually show something. This should make the first call do what STARTUPINFO specifies.

Neil, could you try if this works for you?
Attachment #224401 - Flags: review?(neil)
Comment on attachment 224401 [details] [diff] [review]
Additional patch

On the xpfe build where I need -quiet to disable the splash screen, this doesn't help at all. On the toolkit build this only seems to work for the browser, not other startup options such as jsconsole or inspector.
Attachment #224401 - Flags: review?(neil) → review-
Attached patch Additional patch v2 (obsolete) — Splinter Review
Neil, how about this one? It seems I have to resort to a flag telling when we're really doing the first show.
Attachment #224401 - Attachment is obsolete: true
(In reply to comment #39)
>Neil, how about this one?
I can't actually get it to apply :-(
Oops, I probably created it from branch. I'll create a trunk version when I'm on my system again. I thought it would have applied to trunk too, but not if you still have the obsolete version in. 
*** Bug 264746 has been marked as a duplicate of this bug. ***
Attached patch Additional patch v2 for trunk (obsolete) — Splinter Review
This should apply to branch.
Err... trunk.
OK, this fixed it for my main Windows build. I'm not sure why it doesn't seem to have affected my test build though.
What's the configuration of your debug build? How about startup params?
(In reply to comment #46)
>How about startup params?
I guess that was the issue... I had used it to test startup, and it was configured to open several windows, some of which then wanted to open further dialogs. If I just try to open the one window then it seems reliable.
What parameters did you use? Can I somehow test the same?
(In reply to comment #48)
>What parameters did you use? Can I somehow test the same?
I'm beginning to think that build is cursed. Both with and without your fix I'm getting random activation, while this build displays consistent behaviour.
Perhaps this is the cause of bug 341184 ?
I think I'll have to revise the patch to always grab the focus if there are no existing windows belonging to the process.
(In reply to comment #51)
>I think I'll have to revise the patch to always grab the focus if there are no
>existing windows belonging to the process.
You mean visible, right?
(In reply to comment #52)
> (In reply to comment #51)
> >I think I'll have to revise the patch to always grab the focus if there are no
> >existing windows belonging to the process.
> You mean visible, right?

Right. 

Ok, this should do it (fingers crossed). Neil, could you check?
Attachment #224471 - Attachment is obsolete: true
Attachment #224825 - Attachment is obsolete: true
Attachment #225305 - Flags: review?(neil)
Comment on attachment 225305 [details] [diff] [review]
Patch to tame it 

OK, so some of my problems were due to me testing via Remote Desktop. However this patch fixes things for good when running from the physical console.
Attachment #225305 - Flags: review?(neil) → review+
Attachment #225305 - Flags: superreview?(roc)
Attachment #225305 - Flags: superreview?(roc) → superreview+
Comment on attachment 225305 [details] [diff] [review]
Patch to tame it 

Checked in to trunk. Neil, can you verify so I can do the branch too?
Comment on attachment 225305 [details] [diff] [review]
Patch to tame it 

Requesting approval. This patch is needed for this to work correctly in branch too.
Attachment #225305 - Flags: approval1.8.1?
Keywords: fixed1.8.1
(In reply to comment #56)
>(From update of attachment 225305 [details] [diff] [review])
>Checked in to trunk. Neil, can you verify so I can do the branch too?
Sorry for the delay (it's been a busy week); it works fine in both my builds.
Comment on attachment 225305 [details] [diff] [review]
Patch to tame it 

a=darin on behalf of drivers

Let's give this a try for beta1.  We can always back it out for beta2 if we get reports of this causing problems.
Attachment #225305 - Flags: approval1.8.1? → approval1.8.1+
BTW, I think that gEnumWindowsProc should be declared static since it is never referenced outside the nsWindow.cpp translation unit.
I made gEnumWindowsProc static and checked the patch in to branch. I'll make the static change to trunk too.
Keywords: fixed1.8.1
Done.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
I'm still seeing this bug on Word documents (and probably other Office documents) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060626 Minefield/3.0a1.  This is on two different computers, same Firefox and OS version), one running Office XP and the other Office 2003.  Tested in and out of safe-mode with current profile as well as new profile.
Reopening again. I failed to test properly with Firefox, this seems to work only in Seamonkey.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Should fixed1.8.1 be cleared too, then (seeing as this is a Firefox: DM bug)?
Yes..
Keywords: fixed1.8.1
Attached patch Patch to enhance it (obsolete) — Splinter Review
Okay, this is better than ever before. Changes to previous version: use SetWindowPos instead of ShowWindow to explicitly set it behind and make sure PlaceBehind doesn't do us a disfavour. Tested with Seamonkey and Firefox.
Attachment #227258 - Flags: superreview?(roc)
Attachment #227258 - Flags: review?(neil)
Attachment #227258 - Flags: superreview?(roc) → superreview+
Comment on attachment 227258 [details] [diff] [review]
Patch to enhance it

>+  gWindowsVisible = PR_FALSE;
>+  EnumWindows(gEnumWindowsProc, 0);
>+  if (!gWindowsVisible) {
>+    return PR_TRUE;
>+  } else {
>+    HWND fgWnd = ::GetForegroundWindow();
>+    DWORD pid;
>+    GetWindowThreadProcessId(fgWnd, &pid);
>+    if (!fgWnd || pid == _getpid()) {
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;
a) wouldn't it be cheaper to check the foreground window first?
b) if you are going to null-check fgWnd shouldn't you do that before passing it to GetWindowThreadProcessId?

>+              // Place the window behind the foreground window
>+              HWND wndAfter = ::GetNextWindow(::GetForegroundWindow(), GW_HWNDNEXT);
>+              if (!wndAfter)
>+                wndAfter = HWND_BOTTOM;
>+              ::SetWindowPos(mWnd, wndAfter, 0, 0, 0, 0, SWP_SHOWWINDOW | SWP_NOSIZE | SWP_NOMOVE |
>+                       SWP_NOACTIVATE);
Doesn't passing the foreground window as the hWndAfter parameter already suffice to place this window behind the foreground window?

>+  if (!CanTakeFocus() && behind == HWND_TOP)
>+    return NS_OK;
It seems to me you should set behind to ::GetForegroundWindow here too.
Attachment #227258 - Attachment is obsolete: true
Attachment #227258 - Flags: review?(neil)
Attached patch Patch v2 to enhance it (obsolete) — Splinter Review
Good points (don't know what I was thinking with the GetNextWindow stuff). This patch should have them fixed.
Attachment #229492 - Flags: review?(neil)
Comment on attachment 229492 [details] [diff] [review]
Patch v2 to enhance it

>+PRBool nsWindow::CanTakeFocus()
This is missing a declaration (or you could make it static).
r=me with this fixed.
Attachment #229492 - Flags: review?(neil) → review+
I managed to forget nsWindow.h from the patch. Here it is for the sake of completeness.
Attachment #229492 - Attachment is obsolete: true
Attachment #229528 - Flags: superreview?(roc)
Attachment #229528 - Flags: review+
Blocks: 277406
Attachment #229528 - Flags: superreview?(roc) → superreview+
Latest patch checked into trunk.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
I can confirm that the problem is fixed with the patch.  Tested on WinXP/OfficeXP and WinXP/Office2003.  Thanks for fixing this nearly 2 year old bug!
Marking as VERIFIED per comment 73.
Status: RESOLVED → VERIFIED
Ere, do you plan to ask for approval1.8.1 for the latest patches?
Comment on attachment 229528 [details] [diff] [review]
Patch v2.1 to enhance it

Yes, indeed now that it's verified in trunk would be great to have it in branch too.
Attachment #229528 - Flags: approval1.8.1?
What does this patch get us, and how risky is it?  It's not clear if this is the fix for the bug or just a tweak to that fix, given that there are already two patches that gave gone in.
Comment on attachment 229528 [details] [diff] [review]
Patch v2.1 to enhance it

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #229528 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 229528 [details] [diff] [review]
Patch v2.1 to enhance it

Checked in to branch. This practically replaces the previous patches with one with the same idea but cleaner and more complete implementation which is needed for some cases to work correctly.
Depends on: 347513
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.