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

VERIFIED FIXED

Status

()

Toolkit
Downloads API
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: richardtallent, Assigned: Ere Maijala (slow))

Tracking

({fixed1.8.1})

Trunk
x86
Windows XP
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
(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

13 years ago
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

13 years ago
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

13 years ago
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.
(Assignee)

Updated

12 years ago
Version: unspecified → Trunk

Comment 5

12 years ago
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

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

Comment 7

12 years ago
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

Comment 8

12 years ago
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?
(Assignee)

Comment 9

12 years ago
Taking...
Assignee: nobody → emaijala
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

12 years ago
Created attachment 220249 [details] [diff] [review]
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?
Attachment #220249 - Flags: review?(neil)

Comment 11

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

Comment 12

12 years ago
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

12 years ago
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 :-)
(Assignee)

Comment 14

12 years ago
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

12 years ago
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+
(Assignee)

Comment 16

12 years ago
Created attachment 221152 [details] [diff] [review]
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+...
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?
(Assignee)

Comment 18

12 years ago
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.
(Assignee)

Comment 19

12 years ago
Roc?
Attachment #221152 - Flags: superreview?(roc) → superreview+

Comment 20

12 years ago
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.

(Assignee)

Comment 21

12 years ago
Checked in to trunk. If everything looks good, I'll check this in to 1.8.1 too.
(Assignee)

Updated

12 years ago
Attachment #221152 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 22

12 years ago
Checked in to 1.8.1.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 23

12 years ago
Will this patch also fix identical Thunderbird bug (when opening an attachment)?
(Assignee)

Comment 24

12 years ago
I believe it does, though I haven't tested.

Comment 25

12 years ago
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 → ---
(Assignee)

Comment 26

12 years ago
Doug, do you have a test case?

Comment 27

12 years ago
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

12 years ago
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().

(Assignee)

Comment 29

12 years ago
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

12 years ago
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.
(Assignee)

Comment 31

12 years ago
Neil, any chance for a test case?

Comment 32

12 years ago
yeah ere, i would like to ifndef it.  is that okay with you?
(Assignee)

Comment 33

12 years ago
(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

12 years ago
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

12 years ago
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

12 years ago
Although with either build the profile selection dialog always takes focus...
(Assignee)

Comment 37

12 years ago
Created attachment 224401 [details] [diff] [review]
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?
Attachment #224401 - Flags: review?(neil)

Comment 38

12 years ago
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-
(Assignee)

Comment 39

12 years ago
Created attachment 224471 [details] [diff] [review]
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.
Attachment #224401 - Attachment is obsolete: true

Comment 40

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

Comment 41

12 years ago
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. ***
(Assignee)

Comment 43

12 years ago
Created attachment 224825 [details] [diff] [review]
Additional patch v2 for trunk

This should apply to branch.
(Assignee)

Comment 44

12 years ago
Err... trunk.

Comment 45

12 years ago
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.
(Assignee)

Comment 46

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

Comment 47

12 years ago
(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.
(Assignee)

Comment 48

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

Comment 49

12 years ago
(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 ?
(Assignee)

Comment 51

12 years ago
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

12 years ago
(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?
(Assignee)

Comment 53

12 years ago
(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. 

(Assignee)

Comment 54

12 years ago
Created attachment 225305 [details] [diff] [review]
Patch to tame it 

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 55

12 years ago
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+
(Assignee)

Updated

12 years ago
Attachment #225305 - Flags: superreview?(roc)
Attachment #225305 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 56

12 years ago
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?
(Assignee)

Comment 57

12 years ago
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?
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1

Comment 58

12 years ago
(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

12 years ago
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+

Comment 60

12 years ago
BTW, I think that gEnumWindowsProc should be declared static since it is never referenced outside the nsWindow.cpp translation unit.
(Assignee)

Comment 61

12 years ago
I made gEnumWindowsProc static and checked the patch in to branch. I'll make the static change to trunk too.
Keywords: fixed1.8.1
(Assignee)

Comment 62

12 years ago
Done.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 63

12 years ago
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.
(Assignee)

Comment 64

12 years ago
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)?
(Assignee)

Comment 66

12 years ago
Yes..
Keywords: fixed1.8.1
(Assignee)

Comment 67

12 years ago
Created attachment 227258 [details] [diff] [review]
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.
Attachment #227258 - Flags: superreview?(roc)
Attachment #227258 - Flags: review?(neil)
Attachment #227258 - Flags: superreview?(roc) → superreview+

Comment 68

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #227258 - Attachment is obsolete: true
Attachment #227258 - Flags: review?(neil)
(Assignee)

Comment 69

12 years ago
Created attachment 229492 [details] [diff] [review]
Patch v2 to enhance it

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 70

12 years ago
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+
(Assignee)

Comment 71

12 years ago
Created attachment 229528 [details] [diff] [review]
Patch v2.1 to enhance it

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+

Updated

12 years ago
Blocks: 277406
Attachment #229528 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 72

12 years ago
Latest patch checked into trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 73

12 years ago
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

12 years ago
Marking as VERIFIED per comment 73.
Status: RESOLVED → VERIFIED
Ere, do you plan to ask for approval1.8.1 for the latest patches?
(Assignee)

Comment 76

12 years ago
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+
(Assignee)

Comment 79

12 years ago
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.
Keywords: fixed1.8.1
Depends on: 347513
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.