Closed Bug 410075 Opened 14 years ago Closed 14 years ago
Ctrl-T then Ctrl-K gives address bar focus instead of search field
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/220.127.116.11 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/22.214.171.124 Often times I'll start a new google search which I'd like to have in a new tab. I press Ctrl-T to open the new tab, and then Ctrl-K to give focus to the search bar. I hit Ctrl-T and Ctrl-K within a half second of each other. The search bar receives focus and then the address bar has focus (default behavior of Ctrl-T). Since Ctrl-K was the last key typed, the search bar should have focus, not the address bar. Reproducible: Always Steps to Reproduce: 1. Hit Ctrl-T and Ctrl-K in quick succession. 2. See the address bar has focus when the search bar should have focus. Actual Results: The address bar has focus. Expected Results: The search bar should have focus.
Reproduced with Firefox 3.0b3pre nightly as well on Windows. It is not 100% reproducible for me, but often enough it fails. it is a timing issue that swallows CTRL+K, or rather, it invalidates it by setting Focus to Location bar.
OS: Linux → All
Confirming: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007122820 Minefield/3.0b3pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Confirming as well: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122705 Minefield/3.0b3pre ID:2007122705 It must be done *very* quickly though. Example: Have your finger over the K button and it must be near simultaneous pressing of T and then K.
The location bar is focused off a 0ms (effectively 10ms) timeout, see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.920&mark=1535#1531 . This was apparently added by the patch for bug 102598 (attachment 52485 [details] [diff] [review]) but the "need to do the focus on a timeout" is never explained.
I did a bit of more research on this. attachment 52210 [details] on bug 102598 suggests this to go back to bug 91884. But 91884 comment 0 suggests this stem from the fix to bug 91788, which mentions a problem with SetFocus() dated back to 2001 (see bug 91788 comment 0, bug 91788 comment 2, bug 91788 comment 5). Bug 91884 only changes the code under discussion here as a pre-caution (see bug 91884 comment 0). My tests with the attached patch under Windown and Linux don't seem to suggest there is any problem with setting the focus directly in |BrowserOpenTab()|. This patch needs to be tested on Mac as well, but I guess it's pretty safe. And of course, this fixes the issue observed by the reporter in comment 0. :-)
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #294892 - Flags: review?(gavin.sharp)
(In reply to comment #5) > Created an attachment (id=294892) [details] > Fix the issue by removing setTimeout > > I did a bit of more research on this. attachment 52210 [details] on bug 102598 suggests > this to go back to bug 91884. I'm not really sure the two are related - bug 91884 seems to be about calling focus() from a window's onload event handler, which isn't the case here. It does mention "Maybe not in any XUL window" in bug 91884 comment 0, but it doesn't really look like that was ever verified. I suppose it's probably impossible to determine why hyatt felt the need to add the timeout (it's pretty unlikely that he remembers, even), so unless one of the people CCed here know of a reason why its still needed, perhaps we should just remove it and watch for fallout.
I've run a build with my patch for a few hours now, and I can't see anything broken. I've also run the Browser and Embedding smoke tests with this build, and have not observed any failures... What would the next step be here?
Attachment #294892 - Flags: review?(gavin.sharp) → review?(mano)
Comment on attachment 294892 [details] [diff] [review] Fix the issue by removing setTimeout it's worth trying after beta 3.
Attachment #294892 - Flags: review?(mano) → review+
Thanks for the review, Mano. Can I ask for approval now and then mark it checkin-needed after the beta3 code freeze?
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Comment on attachment 294892 [details] [diff] [review] Fix the issue by removing setTimeout This is a simple change which fixes an issue which annoys users who heavily use keyboard to access their browser. Requesting approval.
Attachment #294892 - Flags: approval1.9?
Unbitrotted patch for check-in.
Attachment #294892 - Attachment is obsolete: true
Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.962; previous revision: 1.961 done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Ctrl-T then Ctrl-K gives address bar focus instead of search field. → Ctrl-T then Ctrl-K gives address bar focus instead of search field
Attachment #302659 - Attachment description: Patch for checkin → Patch (checked in)
You need to log in before you can comment on or make changes to this bug.