I first started seeing this with my Linux build but that's a somewhat dirty tree so I didn't think anything of it, but recently I had to use the Error Console in one of my Windows builds, and I saw something similar there. Basically after evaluating an expression the caret remains in the input field and I can continue to type but the cursor keys don't work until I blur and refocus the field. asrail on IRC discovered a variant of the bug which is much easier to reproduce: Steps to reproduce problem: 1. Open the Error Console 2. Evaluate alert('test'); top.document.commandDispatcher.focusedElement Expected result: [object HTMLInputElement] (possibly with debug info!) Actual result: null

JFTR, with my old tree (Dec 20, mac) I get the expected results from asrail's str.

So... why would the textfield ever be losing focus? To be honest, this is a very very very low priority for me, which means I don't know when I'll get to this, if ever... :(

OK, so as the stack trace I'm about to attach will show, something pretty weird is going on here; in particular at frame 6 the ESM tries to restore focus to the input element, but the focused window is the evaluator frame, so that when SendFocusBlur calls IsFocusable it looks for the frame on the wrong presShell.

So when did this regress?

(In reply to comment #0) > asrail on IRC discovered a variant of the bug which is much easier to > reproduce: > > Steps to reproduce problem: > 1. Open the Error Console > 2. Evaluate alert('test'); top.document.commandDispatcher.focusedElement > > Expected result: [object HTMLInputElement] (possibly with debug info!) And this is what I get in Linux.

Filter "spam" on "guifeatures-nobody-20080610".

OK, so when the Error Console opens, the collapsed about:blank child frame loads first, and this calls CheckForFocus which fails to find any focus, and thus calls SetFocusedWindow with the frame's window. Manually setting the correct focused window seems to work around this bug.

CheckForFocus is only supposed to set focus if loaded window is an ancestor of the previously focused window, and not if there was no previously focused window, in which case the window will get focused later on when it is shown. I also removed the useless "static cast" of aOurWindow to ourWin.

BTW I tested on Windows and I got Mac and Linux users to test this over IRC.

Comment on attachment 341104 [details] [diff] [review] Proposed patch - nsCOMPtr<nsIDOMWindowInternal> ourWin = do_QueryInterface(aOurWindow); I guess the only reason we'd want this is if the callers of this function doesn't properly hold on to aOurWindow so that it could get deleted while we're executing this function. If that's not likely to be the case (as I hope it's not!) then I'm fine with getting rid of this. If not, we should just rename this to kungFuDeathGrip or something. r+sr=jst

(In reply to comment #11) >(From update of attachment 341104 [details] [diff] [review]) >>- nsCOMPtr<nsIDOMWindowInternal> ourWin = do_QueryInterface(aOurWindow); >I guess the only reason we'd want this is if the callers of this function >doesn't properly hold on to aOurWindow so that it could get deleted while we're >executing this function. If that's not likely to be the case (as I hope it's >not!) then I'm fine with getting rid of this. There's only one caller, and it has an nsCOMPtr anyway: nsCOMPtr<nsPIDOMWindow> ourWindow = do_GetInterface(container);

Pushed changeset 893b2c3b521f to mozilla-central.

Could this have broken the test for bug 427559?

Backed out for unittest orange: http://hg.mozilla.org/mozilla-central/rev/311161f3d11e http://hg.mozilla.org/mozilla-central/rev/cfaa8db2c177

I've been poking at this test failure, and I still don't have a conclusion. The test for bug 427559 passes if only that test is run, but fails if you run the browser_autodiscovery test first (which opens a window). Here's some debug output from running browser_bug427599 by itself. I added a call to DumpJSStack() in nsFocusController::SetFocusedWindow, as well as a few other printfs and dump statements in the tests. You can see in this output that SetFocusedWindow gets called with a NULL window twice, and then twice with a valid window before the pageload event occurs.

Here's some output from running browser_autodiscovery and browser_bug427559 in series. Note that SetFocusedWindow gets called twice with a NULL window, and doesn't get called with a valid window after that, so the test fails. I don't really know enough to finish the analysis here.

Pushed changeset 0e52c4677cb4 and reordered tests to get them to pass...

/me thwaps bugzilla...

Could you file a followup on sorting out why running those two tests in that order with this patch breaks things?