should nsGlobalWindow::Focus() return NS_ERROR_FAILURE or NS_OK if we try to set focus on a disabled element? martijn has a patch, but I think we need to audit all the callers to see if they expect NS_ERROR_FAILURE. note, JF added this code for bug #109081 (back in 2002) so I bet the cached compose window for seamonkey / tbird requires it. see bug #348183 where this came up. alternatively, this bug could be thrown back to the browser code, based on bryner comment: "It sounds like what's happening here is that the window is disabled, which happens when a modal dialog is shown, when the setFocus timeout runs. Catching the exception seems reasonable, though it's unfortunate that focus won't actually end up in the right place (well, you could poke at the commandDispatcher, maybe...)" for now, I've just wallpapered over the bug by using try / catch around the call to element.focus() in tabbrowser.xml

note, depending on we fix this, we may end up removing the following try / catch I added to tabbrowser.xml + try { + element.focus(); + } + catch(ex) { + dump("XXX focus() failed, see bug #348183: ex = " + ex + "\n"); + }

It seems to me that this patch would not break bug bug 109081, because it doesn't change behavior. It still returns early, instead of focusing, only it doesn't return an error anymore.

see bug #327139 for a test case to reproduce this bug using a onbeforeunload handler.

(In reply to comment #1) > Created an attachment (id=233247) [edit] > martijn's patch, but I fear this will break (at least) the cached compose > window in tbird You mean you're afraid it would regress bug 131259?

Comment on attachment 233247 [details] [diff] [review] martijn's patch, but I fear this will break (at least) the cached compose window in tbird Well, I just tested the patch again, it doesn't seem to regress bug 130581, so maybe just try this?

Comment on attachment 233247 [details] [diff] [review] martijn's patch, but I fear this will break (at least) the cached compose window in tbird Yes, this is IMO correct.

Checking in nsGlobalWindow.cpp; /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v <-- nsGlobalWindow.cpp new revision: 1.894; previous revision: 1.893 done Checked in on trunk.

>> martijn's patch, but I fear this will break (at least) the cached compose >> window in tbird > >You mean you're afraid it would regress bug 131259? I was just afraid it would break the cached compose window. see http://www.mozilla.org/mailnews/arch/compose/cached.html It is a little out of date, but still covers the important points.

(In reply to comment #9) > I was just afraid it would break the cached compose window. > > see http://www.mozilla.org/mailnews/arch/compose/cached.html > > It is a little out of date, but still covers the important points. Ok, thanks for the link. I don't think this would break the cached compose window. The only thing that was changed is that now not an error is being thrown. But still the cached compose window would not be focused. However, I was thinking about removing that piece of code completely, to be able to fix bug 360686, but I guess that might indeed regress the cached compose bug.