434494 - Opening a new window with Cmd-K does not always focus the search field

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Marco De Vitis]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; it; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; it; rv:1.9) Gecko/2008051202 Firefox/3.0

When all windows are closed and you press Cmd-K, a new window is opened and it should have the search field (in the toolbar) selected, ready for a search. This indeed happens sometimes, but very often the location field is selected instead. This behaviour appears to be random.

Reproducible: Sometimes

Steps to Reproduce:
1. Launch Firefox.
2. Close all Firefox windows.
3. Press Cmd-K.
Actual Results:  
A new window opens, and sometimes the cursor is inside the location bar.

Expected Results:  
The cursor should ALWAYS be placed in the search bar when opening a new window with Cmd-K.

After the window has opened, anyway, you can of course press Cmd-K again to move to the search field, if it wasn't initially selected.

Comment 1

[Jesse Ruderman]

It looks like the search bar is focused initially but then the address bar takes focus a few tenths of a second later.

Comment 2

[Jesse Ruderman]

This has apparently happened before, in bug 343903.

Comment 3

[fluffy]

I'm experiencing this as well.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Make Command+K successfully focus the search box when it creates a new window.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Make Command+K successfully focus the search box when it creates a new window (v2)

Updated patch in accordance with some comments on IRC from Gavin.  This avoids racing for the window load event.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

You can pass a JS function object directly to addObserver (no need for the object/QI junk), and you can use arguments.callee for the removeObserver call.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Make Command+K successfully focus the search box when it creates a new window (v3)

Removed QueryInterface method from the observer object.  I gave the observer a name though, just because it ended up looking neater than passing in the function directly to addObserver.

When do JS objects implementing interfaces need an explicit QueryInterface method and when not?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> When do JS objects implementing interfaces need an explicit QueryInterface
> method and when not?

They generally never do, in the common cases (handler/observer JS object implementing a single interface being passed to a native C++ method). In this particular case, where the interface being implemented (nsIObserver) is marked "function" (http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserver.idl#45), you can just pass a JS function object directly.

I think you really only ever need the explicit QI for JS-implemented XPCOM components.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526163 [details] [diff] [review]
Make Command+K successfully focus the search box when it creates a new window (v3)

nit: use |function observer(...) {}| rather than |observer = function (...) {}| - that way the name is associated with the function object itself (rather than just being a reference to an anonymous function).

r=me with that change, thanks!

Comment 11

[Ed Lee :Mardak]

(In reply to comment #7)
> you can use arguments.callee for the removeObserver call
If there's plans on moving to strict mode in the future, arguments.callee won't work (or maybe just give a warning?).

Comment 12

[Jeff Walden [:Waldo]]

|arguments.callee| flat-out won't work in strict mode code -- trying to access it will throw a TypeError.  Instead, any place where you'd use |arguments.callee|, just name the function, and use that name.  The only moderately reasonable excuse for ever using |arguments.callee| is to avoid IE's broken handling of function-name-scoping for such things (when you have |var a = function f(){}| it wrongly introduces an |f| binding as well as an |a| binding), but that reason's clearly inapplicable in Mozilla browser code.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> Instead, any place where you'd use |arguments.callee|, just name the function,
> and use that name.

That's not suitable for e.g.:
foo.addEventListener("click", this.handler.bind(this), false);

or any other cases where you can't directly reference the attached function by name.

Comment 14

[Jeff Walden [:Waldo]]

Recursive bound functions don't work as you think they work -- they invoke the target function in a nested call, so the effect is as if the original function were called (tho I'm not sure about the effect on operations that actually try to walk the stack, rather than just look at the immediate frame):

[jwalden@find-waldo-now src]$ dbg/js
js> function f() { return arguments.callee === f; }
js> f.bind(null)()
true

So arguments.callee within a function that's being called from a bound function is that function, not the bound function.

If you have other cases "where you can't directly reference the attached function by name", I'd be interested to hear them.  I'm not aware of other cases, myself.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> So arguments.callee within a function that's being called from a bound function
> is that function, not the bound function.

That's unfortunate... Having to keep event listener functions referencable is annoying.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Cameron, are you going to land this?

Comment 17

[Cameron McCormack (:heycam)]

I forgot, I did land it!

http://hg.mozilla.org/mozilla-central/rev/37ad2e03e38e

Is that still within the "Target Milestone: firefox5" or is it firefox6?  Also, would I determine the answer to that?

I got a drive-by comment earlier to swap the order of the function statement and then window.openDialog() call, because it may be confusing to see the `if (subject == win)` check in the branch where we test that win is falsey.  (Of course it doesn't actually matter, since function statements are hoisted to the top of the containing function, and the `win` is evaluated at the time of the function call, when it's not falsey.)  I wanted to check whether you think it is worth making this change for de-confusion.

Comment 18

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/tags says AURORA_BASE_20110412 is mozilla-central changeset a95d42642281.

The patch in this bug landed as rev mozilla-central changeset 37ad2e03e38e.

"hg debugancestor 37ad2e03e38e a95d42642281" says a95d42642281 (the branch point) is the earlier revision. So this landed for Firefox 6, not for Firefox 5.

Comment 19

[Cameron McCormack (:heycam)]

Thanks.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Does it mean we can close out the bug as fixed now?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

yes

Comment 22

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110628 Firefox/7.0a1