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)
Tracking
()
RESOLVED
FIXED
mozilla0.8.1
People
(Reporter: colinp, Assigned: danm.moz)
References
Details
Attachments
(2 files)
2.33 KB,
patch
|
Details | Diff | Splinter Review | |
2.11 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 3•24 years ago
|
||
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.
Comment 5•24 years ago
|
||
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.
Comment 7•24 years ago
|
||
Reporter | ||
Comment 8•24 years ago
|
||
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.
Comment 9•24 years ago
|
||
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?
Reporter | ||
Comment 10•24 years ago
|
||
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.
Comment 11•24 years ago
|
||
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.
Reporter | ||
Comment 12•24 years ago
|
||
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.
Assignee | ||
Comment 13•24 years ago
|
||
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.
Comment 14•24 years ago
|
||
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. :)
Comment 15•24 years ago
|
||
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
Comment 16•24 years ago
|
||
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
Comment 17•24 years ago
|
||
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?
Comment 18•24 years ago
|
||
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
Comment 19•24 years ago
|
||
Comment 20•24 years ago
|
||
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.
Comment 21•24 years ago
|
||
Comment 22•24 years ago
|
||
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.
Updated•24 years ago
|
Severity: blocker → critical
Target Milestone: --- → mozilla0.8.1
Comment 23•24 years ago
|
||
r/sr=brendan, looks good to me.
/be
Assignee | ||
Comment 24•24 years ago
|
||
r=me
Comment 25•24 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 26•24 years ago
|
||
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.
Comment 28•24 years ago
|
||
*** Bug 66487 has been marked as a duplicate of this bug. ***
Comment 29•24 years ago
|
||
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.
Comment 30•24 years ago
|
||
*** Bug 70040 has been marked as a duplicate of this bug. ***
Comment 31•24 years ago
|
||
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.
Description
•