Last Comment Bug 434494 - Opening a new window with Cmd-K does not always focus the search field
: Opening a new window with Cmd-K does not always focus the search field
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: PowerPC Mac OS X
: -- minor (vote)
: Firefox 6
Assigned To: Cameron McCormack (:heycam)
:
: Florian Quèze [:florian] [:flo]
Mentors:
: 447959 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-19 06:03 PDT by Marco De Vitis
Modified: 2011-06-29 05:26 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make Command+K successfully focus the search box when it creates a new window. (1.78 KB, patch)
2011-01-03 01:36 PST, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Make Command+K successfully focus the search box when it creates a new window (v2) (1.79 KB, patch)
2011-04-14 03:27 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Make Command+K successfully focus the search box when it creates a new window (v3) (1.69 KB, patch)
2011-04-14 17:26 PDT, Cameron McCormack (:heycam)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Marco De Vitis 2008-05-19 06:03:15 PDT
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 2008-05-19 16:15:08 PDT
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 2008-05-19 16:17:24 PDT
This has apparently happened before, in bug 343903.
Comment 3 fluffy 2008-06-20 23:19:36 PDT
I'm experiencing this as well.
Comment 4 Jesse Ruderman 2008-07-25 06:23:27 PDT
*** Bug 447959 has been marked as a duplicate of this bug. ***
Comment 5 Cameron McCormack (:heycam) 2011-01-03 01:36:09 PST
Created attachment 500758 [details] [diff] [review]
Make Command+K successfully focus the search box when it creates a new window.
Comment 6 Cameron McCormack (:heycam) 2011-04-14 03:27:22 PDT
Created attachment 525969 [details] [diff] [review]
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: gavin@gavinsharp.com] 2011-04-14 11:59:57 PDT
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) 2011-04-14 17:26:10 PDT
Created attachment 526163 [details] [diff] [review]
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: gavin@gavinsharp.com] 2011-04-18 09:51:52 PDT
(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: gavin@gavinsharp.com] 2011-04-18 11:44:28 PDT
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 2011-04-20 11:17:47 PDT
(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] (remove +bmo to email) 2011-04-22 22:46:16 PDT
|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: gavin@gavinsharp.com] 2011-04-28 11:29:23 PDT
(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] (remove +bmo to email) 2011-04-28 15:24:55 PDT
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: gavin@gavinsharp.com] 2011-04-29 08:08:51 PDT
(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: gavin@gavinsharp.com] 2011-05-04 09:10:33 PDT
Cameron, are you going to land this?
Comment 17 Cameron McCormack (:heycam) 2011-05-08 16:41:25 PDT
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 2011-05-08 17:25:41 PDT
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) 2011-05-08 17:33:24 PDT
Thanks.
Comment 20 Henrik Skupin (:whimboo) 2011-05-09 04:14:10 PDT
Does it mean we can close out the bug as fixed now?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-09 12:58:01 PDT
yes
Comment 22 Vlad [QA] 2011-06-29 05:26:20 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110628 Firefox/7.0a1

Note You need to log in before you can comment on or make changes to this bug.