365467 - Weird focus behaviour when evaluating expressions in the Error Console

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

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

Comment 1

[Stefan [:stefanh]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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... :(

Comment 3

[[e-mail]]

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.

Comment 4

[[e-mail]]

Attachment: Stack backtrace

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

So when did this regress?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 7

[Serge Gautherie (:sgautherie)]

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

Comment 8

[[e-mail]]

Attachment: Stack trace opening the Error Console

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.

Comment 9

[[e-mail]]

Attachment: Proposed patch

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.

Comment 10

[[e-mail]]

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

Comment 11

[Johnny Stenback (:jst)]

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

Comment 12

[[e-mail]]

(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);

Comment 13

[[e-mail]]

Pushed changeset 893b2c3b521f to mozilla-central.

Comment 14

[Dão Gottwald [:dao]]

Could this have broken the test for bug 427559?

Comment 15

[(not currently active) Ted Mielczarek]

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

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: some debug output from running browser_bug427559 by itself

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.

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: debug output from running browser_autodiscovery and browser_bug427559

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.

Comment 18

[[e-mail]]

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

Comment 19

[[e-mail]]

/me thwaps bugzilla...

Comment 20

[(not currently active) Ted Mielczarek]

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