Weird focus behaviour when evaluating expressions in the Error Console

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

(Blocks: 1 bug, {regression})

Trunk
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

12 years ago
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

12 years ago
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... :(
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Comment 4

12 years ago
Created attachment 250944 [details]
Stack backtrace

Comment 5

12 years ago
So when did this regress?

Comment 6

12 years ago
(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

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
(Assignee)

Comment 8

10 years ago
Created attachment 340943 [details] [diff] [review]
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.
(Assignee)

Comment 9

10 years ago
Created attachment 341104 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #341104 - Flags: superreview?(jst)
Attachment #341104 - Flags: review?(jst)
(Assignee)

Comment 10

10 years ago
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+
(Assignee)

Comment 12

10 years ago
(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);
(Assignee)

Comment 13

10 years ago
Pushed changeset 893b2c3b521f to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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 → ---
Created attachment 342457 [details]
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.
Created attachment 342458 [details]
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.
(Assignee)

Comment 18

10 years ago
Pushed changeset 0e52c4677cb4 and reordered tests to get them to pass...
(Assignee)

Comment 19

10 years ago
/me thwaps bugzilla...
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 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?
(Assignee)

Updated

10 years ago
Blocks: 459394
Depends on: 459949

Updated

10 years ago
Depends on: 465964

Updated

10 years ago
No longer depends on: 465964

Updated

10 years ago
Blocks: 465964

Updated

10 years ago
Depends on: 466332
You need to log in before you can comment on or make changes to this bug.