232605 - windows opened by window.open open behind the window with focus

Status: RESOLVED FIXED

(Firefox :: General, defect, P4)

Description

[Jesse Weinstein]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6) Gecko/20040113 MultiZilla/1.5.0.3o

When a window is opened by window.open, it opens behind the window which had
focus(and still does.)  This may be a web site writer's problem, not a bug in
Mozilla.

Reproducible: Always
Steps to Reproduce:
1. Enter: javascript:void(window.open('http://google.com', '_blank',
'width=400,height=400'))
 in the location bar.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this MacOSX only?  This worksforme on Linux with a current trunk build... 
Also, is this a problem without Multizilla?

Comment 2

[Jesse Weinstein]

It is probably limited to Mac OS X, but I just reproduced it without Multizilla.
 Here's the User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.6) Gecko/20040113

Comment 3

[Stefan [:stefanh]]

I can repro this with Mozilla 2004012705. The new window flashes up in front of
the browser window for 1/10 sec or so. Then it disappears behind the browser window.

Comment 4

[Alex Eng]

Reproduced on Mozilla 1.7 beta (build 2004031616) on Win98SE

Confirming and changing hardware/OS to All

Comment 5

[Dan M]

I traced this to the opener browser window focusing its content window after
beginning to load the URL. That's what's responsible for moving the focus off
the URLbar, into the content area, in case the website doesn't focus itself.
Then I traced that line back to May 2000. Milestone M16. Unless this bug is due
to some new focus interaction, which seems unlikely, it's very old behaviour.

It might be reasonable to leave the focus in the URLbar for javascript URLs.

Comment 6

[Jesse Weinstein]

This is still a problem in Firefox 1.0.  Dan, as I am not very familiar with
modifying the Mozilla code, could you give me some pointers on how I could make
your suggestion into running code.  I'd really like to get this fixed, and I'd
be happy to do it myself, but I don't know enough about Mozilla hacking.  sigh.

Comment 7

[Jesse Weinstein]

From my attempts at tracing it, based on Dan's comments, it seems that the
culprit (in Firefox) is line 1849 in browser/base/content/browser.js.  The line
is _content.focus(); and if it is disabled, the problem goes away.  This will
delight all bookmarklet fans everywhere.  I'll post a patch as soon as I can
figure out how.

Comment 8

[Jesse Weinstein]

Actually, it's line 186 in utilityOverlay.js (in browser.jar).  The other one
works with stuff entered in the address bar, this with items from the bookmark
bar.  

Comment 9

[Jesse Weinstein]

Ok.  This probably does other bad stuff, but it fixes the problem, and for
bookmarklet users, that's the most important thing.  There are two lines to be
commented out, one for javascript entered in the addressbar, and one for
javascript loaded from a bookmarklet.  First, comment out(by putting "//" in
front) line 186 "    w._content.focus();" in utilityOverlay.js, that fixes
javascript loaded from bookmarks.  Next, comment out line 1849 "   
_content.focus();" in browser.js, that fixes the address bar.  In a standard
Firefox installation, both of these files are in chrome/browser.jar.  I use
Emacs to open up the jar and edit the files; it works flawlessly.  Then restart
Firefox, and enjoy your bookmarklets.

Comment 10

[Jesse Ruderman]

Attachment: Possible fix: replace some calls to _content.focus() with focusElement(_content)

Comment 11

[Jesse Ruderman]

Attachment: Another possible fix: do _content.focus() before loading the URL

Comment 12

[Jesse Ruderman]

Btw, bug 108394 is the Seamonkey version of this bug.

Comment 13

[porl]

The following workaround works for me:
javascript:w=window.open();setTimeout("w.focus()",
0);void(w.location="http://www.mozilla.org")

Comment 14

[Jesse Ruderman]

I think danm's solution (skip the focus() call for javascript: URLs) makes more
sense than the patches I posted.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 168558 [details] [diff] [review]
Another possible fix: do _content.focus() before loading the URL

Sounds good to me; any takers for writing up the patch?

Comment 16

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

Attachment: Don't call focus for javascript: urls

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 176817 [details] [diff] [review]
Don't call focus for javascript: urls

This should be factored out into a function like in Jesse's original patch. 
Also, are you sure these are the only locations that need changes?  Jesse's
patch changes 3 callers in browser.js and 2 in utilityOverlay.js.

Comment 18

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

(In reply to comment #17)
> This should be factored out into a function like in Jesse's original patch.

What should be factored into a function? You mean something like
"isJavascriptURL()" or "shouldFocusContent()"?

> Also, are you sure these are the only locations that need changes?  Jesse's
> patch changes 3 callers in browser.js and 2 in utilityOverlay.js.

Jesse's second patch changes the same two as mine, his first just changes some
other callers to simplify code.

openUILinkIn handles bookmarks
(
http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/content/bookmarks.js#603
)  and BrowserLoadURL handles the location bar
( http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#1993 ),
which as far as I can see are the only two places where this bug is present.

Comment 19

[janusz]

Similar problem but items in dropdown menus under Tools "vanish" behind the 
browser window.  They are there, just hidden.  Close the Firefox browser and 
there they are.  I do not have the problem in HELP.

This is mostly a nuisance. These windows do not open within Firefox. In any 
case, I expect to see the selected item when clicked instead of having it hide 
behind the browser window. 

Windows 2000 Pro; Firefox version 1.0.6 

In Safe Mode, these windows appear behind the browser window: 

Tools- 
Downloads 
Extensions (all disabled) 
Themes (disabled) 
JavaScript Console 
Page Info 

In standard mode, these windows appear behind the browser window: 

Tools- 
Downloads 
Adblock Preferences 
Session Saver Preferences 
Extensions 
Themes 
Tweak Network Settings 
JavaScript Console 
Page Info 


A noticed that a similar problem exists in Thunderbird E-mail (Tools); version 
1.0 I am using.

Hope this helps.
janusz

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

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

Attachment: patch

This is essentially an updated attachment 168557 [details] [diff] [review]. I factored out the "jag fix" into it's own function, and then changed any explicit focus() calls that occur after a loadURI to use the helper instead (which doesn't steal focus).

Comment 22

[Oliver Yeoh]

(In reply to comment #21)
Er.. I'm not a reviewer. I've got a couple of questions regarding this patch. Hope I'm not stepping on anyone's toes :) Otherwise, my sincere apologies.

>+  w.focus();
I can't think of a reason why this is necessary?

>+  // Focus the content area now that the URI is loaded. This function won't
>+  // raise background windows, since the loaded URI may have resulted in a new
>+  // frontmost window (e.g. "javascript:window.open('');")
>+  focusElement(w.content);
Is it better to move this up into the "current" case?
I don't think it's necesary for the case of loading in tabs. Besides, the call to loadOneTab() would have already set the focus to window.content?

Comment 23

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

(In reply to comment #22)
> Er.. I'm not a reviewer. I've got a couple of questions regarding this patch.
> Hope I'm not stepping on anyone's toes :) Otherwise, my sincere apologies.

No problem at all. The more feedback the better.

> >+  w.focus();
> I can't think of a reason why this is necessary?

It's probably not, you're right. I added it because I figured it doesn't make sense to load a UI link in an unfocused window, but since it appears that there are no callers of openUILinkIn that don't rely on user input in a given window, it probably has no effect.

> >+  // Focus the content area now that the URI is loaded. This function won't
> >+  // raise background windows, since the loaded URI may have resulted in a new
> >+  // frontmost window (e.g. "javascript:window.open('');")
> >+  focusElement(w.content);
> Is it better to move this up into the "current" case?
> I don't think it's necesary for the case of loading in tabs. Besides, the call
> to loadOneTab() would have already set the focus to window.content?

I don't see where loadOneTab focuses the content area. Or are you saying that it happens implicitly? There used to be an explicit call in this function, but it was removed by attachment 209152 [details] [diff] [review] of bug 308396, with no substitute as far as I can tell.

Comment 24

[Oliver Yeoh]

(In reply to comment #23)
> I don't see where loadOneTab focuses the content area. Or are you saying that
> it happens implicitly? There used to be an explicit call in this function, but
> it was removed by attachment 209152 [details] [diff] [review] [edit] of bug 308396, with no substitute as far as
> I can tell.

Ah, yes implicitly is the right word. But only for the case when the loadBookmarksInBackground pref is false. loadOneTab will then select the new tab which subsequently triggers the onselect event handler updateCurrentBrowser(). updateCurrentBrowser is where the content area gets focused.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.204#757

So, I guess if loadBookmarksInBackground is false, this bug may still bite.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 240187 [details] [diff] [review]
patch

>Index: browser/base/content/browser.js
>===================================================================

> 
>+  w.focus();
>+

Yeah, do get rid of this please.

>   switch (where) {
>   case "current":
>     w.loadURI(url, null, postData, allowThirdPartyFixup);
>-    w.content.focus();
>     break;
>   case "tabshifted":
>     loadInBackground = !loadInBackground;
>     // fall through
>   case "tab":
>     var browser = w.getBrowser();
>     browser.loadOneTab(url, null, null, postData, loadInBackground,
>                        allowThirdPartyFixup || false);
>     break;
>   }
>+
>+  // Focus the content area now that the URI is loaded. This function won't
>+  // raise background windows, since the loaded URI may have resulted in a new
>+  // frontmost window (e.g. "javascript:window.open('');")
>+  focusElement(w.content);
> }
> 

This comment doesn't parse here, it wouldn't raise a background window because focusElement takes care of this case, right (i.e. not because a frontmost window might be opened).

Comment 26

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

(In reply to comment #25)
> >+  w.focus();
> 
> Yeah, do get rid of this please.

OK

> >+  // Focus the content area now that the URI is loaded. This function won't
> >+  // raise background windows, since the loaded URI may have resulted in a new
> >+  // frontmost window (e.g. "javascript:window.open('');")
> >+  focusElement(w.content);
> > }
> > 
> 
> This comment doesn't parse here, it wouldn't raise a background window because
> focusElement takes care of this case, right (i.e. not because a frontmost
> window might be opened).

I don't understand - I'm explaining why we're calling focusElement instead of .focus(). Would you prefer:

// Call focusElement(w.content) instead of w.content.focus() to make sure that
// we don't raise the old window, since the URI we just loaded may have resulted 
// in a new frontmost window (e.g. "javascript:window.open('');")

Comment 27

[Asaf Romano (gone)]

Comment on attachment 240187 [details] [diff] [review]
patch

>Index: browser/base/content/utilityOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/utilityOverlay.js,v
>retrieving revision 1.43
>diff -u -p -8 -r1.43 utilityOverlay.js
>--- browser/base/content/utilityOverlay.js	21 Sep 2006 12:57:21 -0000	1.43
>+++ browser/base/content/utilityOverlay.js	26 Sep 2006 19:05:47 -0000
>@@ -84,16 +84,38 @@ function getBoolPref ( prefname, def )
>                        .getService(Components.interfaces.nsIPrefBranch);
>     return pref.getBoolPref(prefname);
>   }
>   catch(er) {
>     return def;
>   }
> }
> 
>+// Change focus for this browser window to |element|, without focusing the
>+// window itself.
>+function focusElement(element) {

nit: aElement.

>+  // This is a redo of the fix for jag bug 91884
>+  var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+                     .getService(Components.interfaces.nsIWindowWatcher);
>+  if (window == ww.activeWindow) {
>+    element.focus();
>+  } else {

No } ... { lines please ;)

>+    // set the element in command dispatcher so focus will restore properly
>+    // when the window does become active
>+    var cmdDispatcher = document.commandDispatcher;
>+    if (element instanceof Components.interfaces.nsIDOMWindow) {
>+      cmdDispatcher.focusedWindow = element;
>+      cmdDispatcher.focusedElement = null;
>+    } else if (element instanceof Components.interfaces.nsIDOMElement) {
>+      cmdDispatcher.focusedWindow = element.ownerDocument.defaultView;
>+      cmdDispatcher.focusedElement = element;
>+    }

(element instanceof Window) and (element instanceof Element) is cleaner IMO.


r=mano otherwise.

Comment 28

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

Attachment: updated patch

Comment 29

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

Attachment: updated updated patch

Comment 30

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

Attachment: real updated updated patch

Comment 31

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

mozilla/browser/base/content/browser.js 	1.718
mozilla/browser/base/content/utilityOverlay.js 	1.44

Comment 32

[Worcester12345]

Any chance this can go on the 2.0 branch or a future update of it after some "bake" time?

Comment 33

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

Gavin, can we get a branch patch put together and nominated for 1.8.1.1?

Comment 34

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

(In reply to comment #33)
> Gavin, can we get a branch patch put together and nominated for 1.8.1.1?

I could, yes, but I'm not sure I understand why this is suddenly a branch-worthy patch. We might have broken the "explicitly call focus on the opened window" workaround to this bug in 2.0 (by changing the pref for whether remote code can call focus() on windows), but the "open the window from a small timeout" workaround still works fine.

Comment 35

[dale tan]

in your pop up window, you can do a setTimeout call to the opener and give it a blur event:

setTimeout("window.opener.blur()",0);

Comment 36

[dale tan]

in your pop up window, you can do a setTimeout call to the opener and give it a blur event:

setTimeout("window.opener.blur()",0);

Comment 37

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

(In reply to comment #36)
> in your pop up window, you can do a setTimeout call to the opener and give it a
> blur event:

That sounds much more complicated than the workaround in comment 34 (put the window.open() in a timeout).

Comment 39

[John]

Why is this "fixed"?

Comment 40

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

(In reply to comment #39)
> Why is this "fixed"?

Because it's fixed on the trunk, which is what will become Firefox 3. See the "Target milestone" field.

Comment 42

[iccor56]

i appear to be having this problem since the new version of firefox. when i use just tabs or use the rightclick menu it's fine but if i turn tabs off links set to open in a new window show up behind current window. even stranger it happens for links when i first open the program but if i close the window and follow the link again it opens in front like it should.

Comment 43

[iccor56]

and yes this does it in safe mode as well.

Comment 44

[Jesse Ruderman]

iccor, please file a new bug report (and mention the bug number here).

Comment 45

[iccor56]

new bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=790013