Closed Bug 365467 Opened 13 years ago Closed 11 years ago

Weird focus behaviour when evaluating expressions in the Error Console

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files)

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.
Attached file Stack backtrace
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".
Assignee: guifeatures → nobody
QA Contact: guifeatures
Component: XP Apps: GUI Features → UI Design
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.
Attached patch Proposed patchSplinter Review
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #341104 - Flags: superreview?(jst)
Attachment #341104 - Flags: review?(jst)
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
Attachment #341104 - Flags: superreview?(jst)
Attachment #341104 - Flags: superreview+
Attachment #341104 - Flags: review?(jst)
Attachment #341104 - Flags: review+
(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Could you file a followup on sorting out why running those two tests in that order with this patch breaks things?
Blocks: 459394
Depends on: 459949
Depends on: 465964
No longer depends on: 465964
Blocks: 465964
Depends on: 466332
You need to log in before you can comment on or make changes to this bug.