Closed Bug 71266 Opened 24 years ago Closed 24 years ago

window.focus() function in XUL windows breaks after working once

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla0.8.1

People

(Reporter: colinp, Assigned: danm.moz)

References

Details

Attachments

(2 files)

From Bugzilla Helper: BuildID: stable 0.8 build from cvs & 03/06 nightly I have created an array of window handles which I want to use to "application swap" between windows by using window.focus(). I did this all the time in Netscape 4, so I know it's legitimate. In fact, the product we're building relies critically on this capability. I have actually worked out an identical problem in Mozilla using CycleWindow() and its supporting functions in tasksOverlay.js in the communicator package. Here's an example of how this breaks. Here's the idea: Think of the taskBar buttons (in the lower left) as a launch pad for applications, and if you click on an icon, that application should load. However, if you click on an icon after it has been loaded, you would expect that application to come to focus. steps: 1) launch the mozilla browser 2) launch the mail/news app (by the taskbar icons in the lower right), but keep it over top of the browser 3) launch the address book app (again, by the taskbar icons in the lower right), and keep it on top of the browser and mail/news 4) now click on the mail/news icon again, notice that mail/news takes focus. 5) now click on the address book icon again, notice that address book takes focus. 6) now click on the mail/news icon again, notice that mail/news does NOT take focus. 7) every time you try to task swap between the two from this point on, the window focus is broken, and one will not focus on top of the other. Based on these results, it's fairly clear that it's not incorrectly written javascript code, as the javascript works the first time to focus it correctly, however it appears to be a low-level implementation issue of the javascript window.focus() function. The code that is handling the window.focus() is in tasksOverlay.js in the communicator package as follows: var windowManager = Components.classes['@mozilla.org/rdf/datasource;1?name=window-mediator'].getService(); var windowManagerInterface = windowManager.QueryInterface( Components.interfaces.nsIWindowMediator); var topWindowOfType = windowManagerInterface.getMostRecentWindow( aType ); var topWindow = windowManagerInterface.getMostRecentWindow( null ); if ( topWindowOfType == null ) newWindowOfType( aType ); else if ( topWindowOfType != topWindow ) topWindowOfType.focus(); else { var enumerator = windowManagerInterface.getEnumerator( aType ); var firstWindow = windowManagerInterface.convertISupportsToDOMWindow ( enumerator.getNext() ); var iWindow = firstWindow; // ;-) while ( iWindow != topWindow && enumerator.hasMoreElements() ) iWindow = windowManagerInterface.convertISupportsToDOMWindow ( enumerator.getNext() ); var desiredWindow = firstWindow; if ( enumerator.hasMoreElements() ) desiredWindow = windowManagerInterface.convertISupportsToDOMWindow ( enumerator.getNext() ); if ( desiredWindow == topWindow ) // Only one window, open a new one newWindowOfType( aType ); else desiredWindow.focus(); } From what I can figure out, if the window is not instantiated, it gets instantiated and quits succesfully. However, if it is instantiated, the javascript will give the window focus (but only the first time). I have alerted the window just before it reaches the focus function, and I know the reference is correct, so all signs point to this being a problem with window.focus(). ** This bug only occurs under linux (at least I couldn't get it to happen in windows), and we are using the fvwm2 window manager from a few months ago. We haven't had a chance to try it using the newest fvwm2 yet. Reproducible: Always
Over to Dan.
Assignee: jst → danm
Could be related to bug 68004
Nope, this case has never generated a JS error. If you simply follow the steps, under linux, it will break using window.focus() every time. I'm trying to do something similar to the function CycleWindow(), however I can reproduce the problem with just using the function window.focus() function, and none of the other code. As such: function focusMe(win) { win.focus(); } After the test case I gave. this will break every time.
Setting bug Status to New
Status: UNCONFIRMED → NEW
Ever confirmed: true
I thought I had seen something like this reported before but couldn't find it in my queries. cc:'ing a couple folks who might be able to help.
could this be related to bug 8002?
a couple of other possible related issues are mentioned in bug 20306 and bug 26082
8002 and 26082 sound like they are pretty much the problem. This bug might be able to help them out. However, this bug is critical for me, so if someone could look at it very seriously, I would really appreciate it.
OK, I'm confused. Is the problem that when you call window.focus() on a window it doesn't raise itself? That's expected since it's up to the window manager to give toplevel windows focus so calling window.focus() on a window that doesn't have focus according to the window manager is basically a noop on linux. What is the behaviour you expect?
Yes, I am calling window.focus() on a window that doesn't have focus, for the purpose of raising it to the front to give it focus under linux. I'll re-iterate that this works perfectly fine under netscape 4 (you can do it an infinite number of times and it works every single time). Not only that, but you can in fact do this under Mozilla too, but it only works the very first time you call the function. So I create the window, let it fall in behind, then call window.focus() and it comes to the front. I let it fall in behind a second time and call window.focus() and it won't come to the front. Clearly if it works the first time, it should work the second, and third, and an infinite number of times. Sorry if I sound rather distraught, but all of this is already stated in the early part of this bug.
Sorry, I couldn't decipher your test case. Just to clarify on my box Netscape 4.x will take focus when you use the taskbar with a new window because it is just that - a new window. My window manager is set up so that new windows get focus. If I put the window in back and click the original window the window is raised but it doesn't get focus. ( XMapWindow() vs. XSetInputFocus() ) Since 0.8 I've changed it so that |nsIWidget::SetFocus| doesn't automatically map the window. Callers should be using |GetAttention| when they want to do that. If people think this is a bad idea then I can change it. The downside is that windows will start popping up all over the place as urls finish loading and people complained _endlessly_ about that behaviour.
Ok, that's not the end of the world. I can live with that, it makes sense. However, this needs to be clearly documented somewhere if this is the case. Also, is there a javascript function I can call after calling window.focus() to then bring that window to the front? If there isn't, one has to be built. HAS to. :) After all, that's the whole problem I'm having.
Aha! You did that (Christopher)! According to the "spec," (by which I mean common usage and books) focus is the JS means of bringing a window to the front. It's supposed to work. The problem is that focus is being over- or mis-used. google is the famous one (around here); it focuses itself (actually the text field) in its onload handler, which makes it jump to the front in Nav4 and Mozilla. But besides that, I think we're generating extraneous focuses in the Mozilla chrome. It's being complained about on all platforms, but unix people seem to hate that kind of thing the most vehemently. The right solution is to find and kill the extraneous chrome focuses. I'm linking this bug (calling it dependent, though that's not quite right) to bug 53350, which is the one I think of when I think of extraneous chrome focus, though the original reporter probably wasn't thinking that. If we ever find the time to hunt them down and kill them, the JS focus functionality should probably be restored on unix.
Depends on: 53350
I can put the XMapWindow back in but I have to do some testing first. It's a simple one line change which I can post here so that the oeone guys can at least get their fix. Maybe we need to have window.focus() and <element>.focus() do different things. ( Is there even an <element>.focus()? ) window.focus() seems obvious to bring the window to the front but element.focus() should change the focus within a window but not bring it to the front. I don't know if that would break things, though. Oh, and that change was a result of the focus changes that I made recently that fixed other problems. It was a side effect that I didn't mind. :)
This patch is completely untested. Index: nsWindow.cpp =================================================================== RCS file: /cvsroot/mozilla/widget/src/gtk/nsWindow.cpp,v retrieving revision 1.323 diff -u -r1.323 nsWindow.cpp --- nsWindow.cpp 2001/02/21 15:58:59 1.323 +++ nsWindow.cpp 2001/03/16 20:37:21 @@ -1070,6 +1070,10 @@ printf("top moz area is %p\n", NS_STATIC_CAST(void *, top_mozarea)); #endif + // we always GetAttention so the window will be mapped whether or + // not the window has focus. + GetAttention(); + // If there is a top_mozarea and it doesn't have focus then we're // going to ignore this request if we're part of the browser. If // we're embedded then we do grab focus on our toplevel window since
Yes, there is element.focus(), at least for text inputs. Before window.focus(), JS users (Nav 2.x? I forget) used to focus a text input to give its window focus and raise that window. Yes, the focus method is overloaded (KISS, worse is better). I don't think that's the big problem here, it was not in the past. The big problem is stupid abuse of focus, but if web sites do that, Mozilla users can slap on DOM access controls using the magic prefs. If our chrome JS does too much focusing, we should spank the offending chrome code. Don't be breaking 5-year-old compatibility again! THANKS /be
Though I feel great joy to discover that all the Nav-2.x-era scripts will work, Blizzard is correct that lots of Unix users dislike (ok, despise) having windows pop to the front all over. Is this a good nail for the pref hammer?
I don't find too many sites obnoxiously focusing their windows. But our chrome no doubt does abuse the method. Let's not paper over chrome JS bugs by making focus behave other than how it has for years. /be
I wasn't papering over the problem. I didn't know that the intent of focus() was that it should raise windows. Anyway, here's a patch that fixes the problem, fixes a potential problem with embedders and gives luddites a mozilla.widget.dont-raise-on-setfocus pref that they can set to true.
OK, better patch attached. This gets rid of the double negatives and removes the embedding workaround since it wasn't actually needed. Behaviour for people using notebook widgets will be strange but it won't break anything. I also changed the name of the pref to 'mozilla.widget.raise-on-setfocus' which people can set to false if they want to. That's another double negative that I removed. Looking for reviews.
Severity: blocker → critical
Target Milestone: --- → mozilla0.8.1
r/sr=brendan, looks good to me. /be
r=me
Checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Thanks guys! I think someone should go through the other bugs in this bug and see if they can close off any of the other bugs. ALOT of them seemed to be very similar to this one.
Filed bug 72313 on the regressions caused by this change.
*** Bug 66487 has been marked as a duplicate of this bug. ***
With regards to this (and bug 72313), presumably it would be better to put the GetAttention() call in the code that processes the window.focus method and [element].focus methods. Since there are, roughly, 52,000 such methods, I don't think I'll try making a patch tonight. Maybe we will also need a new GetJSAttention (or perhaps IrritateUserInObnoxiousWay) for use by all JavaScript focus methods that checks a preference before invoking the real GetAttention? I'm imagining that the nsHTMLBlahBlahElement::Focus method would call SetFocus to do the focus, then Get(JS)Attention on the appropriate window, and all the SetFocus methods would be semantically "clean", *just* doing focus shifting, not raising. Also, a note to the original reporter, Colin. Mozilla exposes the method GetAttention, it seems, so you should be able to add xxx.GetAttention(); after your xxx.focus() calls to raise window xxx to the top. However, I assume this is a Mozilla-only extension, so be warned.
*** Bug 70040 has been marked as a duplicate of this bug. ***
bug 72313 filed for new brain-dead window focus behavior on linux.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: