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

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[richardtallent]

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.

Comment 1

[richardtallent]

(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).

Comment 2

[Travis Evans]

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.

Comment 3

[Ricardo Almeida]

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...

Comment 4

[Arend van Beelen jr.]

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.

Comment 5

[mmortal03]

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?

Comment 6

[zug_treno]

*** Bug 330438 has been marked as a duplicate of this bug. ***

Comment 7

[Tore Krudtaa]

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

Comment 8

[Tore Krudtaa]

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?

Comment 9

[Ere Maijala (slow)]

Taking...

Comment 10

[Ere Maijala (slow)]

Attachment: Patch v1

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?

Comment 11

[[e-mail]]

So who's calling Show (it is Show right? you didn't cvs diff -p) and why?

Comment 12

[Ere Maijala (slow)]

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. 

Comment 13

[Tore Krudtaa]

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 :-)

Comment 14

[Ere Maijala (slow)]

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 15

[[e-mail]]

Comment on attachment 220249 [details] [diff] [review]
Patch v1

Ok, makes sense, except I think you ought to call GetAttention in the background case.

Comment 16

[Ere Maijala (slow)]

Attachment: Patch v1.1

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+...

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 18

[Ere Maijala (slow)]

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.

Comment 19

[Ere Maijala (slow)]

Roc?

Comment 20

[Tore Krudtaa]

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.

Comment 21

[Ere Maijala (slow)]

Checked in to trunk. If everything looks good, I'll check this in to 1.8.1 too.

Comment 22

[Ere Maijala (slow)]

Checked in to 1.8.1.

Comment 23

[Radek 'sysKin' Czyz]

Will this patch also fix identical Thunderbird bug (when opening an attachment)?

Comment 24

[Ere Maijala (slow)]

I believe it does, though I haven't tested.

Comment 25

[Doug Turner (:dougt)]

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.

Comment 26

[Ere Maijala (slow)]

Doug, do you have a test case?

Comment 27

[Doug Turner (:dougt)]

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?

Comment 28

[Doug Turner (:dougt)]

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().

Comment 29

[Ere Maijala (slow)]

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?

Comment 30

[[e-mail]]

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.

Comment 31

[Ere Maijala (slow)]

Neil, any chance for a test case?

Comment 32

[Doug Turner (:dougt)]

yeah ere, i would like to ifndef it.  is that okay with you?

Comment 33

[Ere Maijala (slow)]

(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. 

Comment 34

[Doug Turner (:dougt)]

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.

Comment 35

[[e-mail]]

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.

Comment 36

[[e-mail]]

Although with either build the profile selection dialog always takes focus...

Comment 37

[Ere Maijala (slow)]

Attachment: Additional patch

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?

Comment 38

[[e-mail]]

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.

Comment 39

[Ere Maijala (slow)]

Attachment: Additional patch v2

Neil, how about this one? It seems I have to resort to a flag telling when we're really doing the first show.

Comment 40

[[e-mail]]

(In reply to comment #39)
>Neil, how about this one?
I can't actually get it to apply :-(

Comment 41

[Ere Maijala (slow)]

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. 

Comment 42

[Phil Ringnalda (:philor)]

*** Bug 264746 has been marked as a duplicate of this bug. ***

Comment 43

[Ere Maijala (slow)]

Attachment: Additional patch v2 for trunk

This should apply to branch.

Comment 44

[Ere Maijala (slow)]

Err... trunk.

Comment 45

[[e-mail]]

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.

Comment 46

[Ere Maijala (slow)]

What's the configuration of your debug build? How about startup params?

Comment 47

[[e-mail]]

(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.

Comment 48

[Ere Maijala (slow)]

What parameters did you use? Can I somehow test the same?

Comment 49

[[e-mail]]

(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.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Perhaps this is the cause of bug 341184 ?

Comment 51

[Ere Maijala (slow)]

I think I'll have to revise the patch to always grab the focus if there are no existing windows belonging to the process.

Comment 52

[[e-mail]]

(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?

Comment 53

[Ere Maijala (slow)]

(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. 

Comment 54

[Ere Maijala (slow)]

Attachment: Patch to tame it

Ok, this should do it (fingers crossed). Neil, could you check?

Comment 55

[[e-mail]]

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.

Comment 56

[Ere Maijala (slow)]

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 57

[Ere Maijala (slow)]

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.

Comment 58

[[e-mail]]

(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 59

[Darin Fisher]

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.

Comment 60

[Darin Fisher]

BTW, I think that gEnumWindowsProc should be declared static since it is never referenced outside the nsWindow.cpp translation unit.

Comment 61

[Ere Maijala (slow)]

I made gEnumWindowsProc static and checked the patch in to branch. I'll make the static change to trunk too.

Comment 62

[Ere Maijala (slow)]

Done.

Comment 63

[Jason Chu]

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.

Comment 64

[Ere Maijala (slow)]

Reopening again. I failed to test properly with Firefox, this seems to work only in Seamonkey.

Comment 65

[:Gavin Sharp [email: [e-mail]]]

Should fixed1.8.1 be cleared too, then (seeing as this is a Firefox: DM bug)?

Comment 66

[Ere Maijala (slow)]

Yes..

Comment 67

[Ere Maijala (slow)]

Attachment: Patch to enhance it

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.

Comment 68

[[e-mail]]

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.

Comment 69

[Ere Maijala (slow)]

Attachment: Patch v2 to enhance it

Good points (don't know what I was thinking with the GetNextWindow stuff). This patch should have them fixed.

Comment 70

[[e-mail]]

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.

Comment 71

[Ere Maijala (slow)]

Attachment: Patch v2.1 to enhance it

I managed to forget nsWindow.h from the patch. Here it is for the sake of completeness.

Comment 72

[Ere Maijala (slow)]

Latest patch checked into trunk.

Comment 73

[Jason Chu]

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!

Comment 74

[Adam Guthrie]

Marking as VERIFIED per comment 73.

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Ere, do you plan to ask for approval1.8.1 for the latest patches?

Comment 76

[Ere Maijala (slow)]

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.

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 78

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229528 [details] [diff] [review]
Patch v2.1 to enhance it

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.

Comment 79

[Ere Maijala (slow)]

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.