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

RESOLVED FIXED in Firefox 3 alpha1

Status

()

Firefox
General
P4
normal
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: Jesse Weinstein, Assigned: Gavin)

Tracking

Trunk
Firefox 3 alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

14 years ago
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.
Is this MacOSX only?  This worksforme on Linux with a current trunk build... 
Also, is this a problem without Multizilla?
(Reporter)

Comment 2

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

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

13 years ago
Reproduced on Mozilla 1.7 beta (build 2004031616) on Win98SE

Confirming and changing hardware/OS to All
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: MacOS X → All
Hardware: Macintosh → All

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

13 years ago
Created attachment 168557 [details] [diff] [review]
Possible fix: replace some calls to _content.focus() with focusElement(_content)

Updated

13 years ago
Component: DOM: Level 0 → General
Product: Core → Firefox

Comment 11

13 years ago
Created attachment 168558 [details] [diff] [review]
Another possible fix: do _content.focus() before loading the URL

Updated

13 years ago
Attachment #168557 - Attachment description: Possible fix: replace those calls to _content.focus() with focusElement(_content) → Possible fix: replace some calls to _content.focus() with focusElement(_content)

Updated

13 years ago
Attachment #168558 - Flags: review?(bryner)

Comment 12

13 years ago
Btw, bug 108394 is the Seamonkey version of this bug.
Assignee: general → jruderman

Comment 13

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

Comment 14

13 years ago
I think danm's solution (skip the focus() call for javascript: URLs) makes more
sense than the patches I posted.
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?
Attachment #168558 - Flags: review?(bryner) → review-
Created attachment 176817 [details] [diff] [review]
Don't call focus for javascript: urls
Assignee: jruderman → gavin.sharp
Status: NEW → ASSIGNED
Attachment #176817 - Flags: review?(bryner)
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.
Attachment #176817 - Flags: review?(bryner) → review-
(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.
Priority: -- → P3
Target Milestone: --- → Firefox1.1

Comment 19

12 years ago
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
Priority: P3 → P5
Target Milestone: Firefox1.5 → ---
Priority: P5 → P4
Target Milestone: --- → Firefox 2
Priority: P4 → --
Target Milestone: Firefox 2 → ---
*** Bug 346114 has been marked as a duplicate of this bug. ***
Created attachment 240187 [details] [diff] [review]
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).
Attachment #168557 - Attachment is obsolete: true
Attachment #168558 - Attachment is obsolete: true
Attachment #176817 - Attachment is obsolete: true
Attachment #240187 - Flags: review?
Attachment #240187 - Flags: review? → review?(mano)
Priority: -- → P4
Whiteboard: [patch-r?]
Target Milestone: --- → Firefox 3 alpha1

Comment 22

11 years ago
(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?

(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

11 years ago
(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 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).
(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 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.
Attachment #240187 - Flags: review?(mano) → review+
Created attachment 241851 [details] [diff] [review]
updated patch
Attachment #240187 - Attachment is obsolete: true
Whiteboard: [patch-r?]
Whiteboard: [checkin needed]
Created attachment 241880 [details] [diff] [review]
updated updated patch
Attachment #241851 - Attachment is obsolete: true
Created attachment 241881 [details] [diff] [review]
real updated updated patch
Attachment #241880 - Attachment is obsolete: true
mozilla/browser/base/content/browser.js 	1.718
mozilla/browser/base/content/utilityOverlay.js 	1.44
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 32

11 years ago
Any chance this can go on the 2.0 branch or a future update of it after some "bake" time?
Gavin, can we get a branch patch put together and nominated for 1.8.1.1?
(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.
Flags: in-testsuite?

Comment 35

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

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

Updated

10 years ago
Duplicate of this bug: 377094

Comment 39

10 years ago
Why is this "fixed"?
(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.
Duplicate of this bug: 397634
Depends on: 431672
Depends on: 451663
Flags: in-testsuite?

Comment 42

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

5 years ago
and yes this does it in safe mode as well.

Comment 44

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

Comment 45

5 years ago
new bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=790013
You need to log in before you can comment on or make changes to this bug.