User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140716183446 Steps to reproduce: If you open a new tab in 31, you get some tiles and a Google search bar. I have removed my search bar and use search keywords in the address bar. I thought if I hit command K in the new tab window, my cursor would go to the search bar so I could search Actual results: Instead of the expected behavior, a new tab with the firefox start page opens. This is annoying since the Firefox start page doesn't allow me to switch search engines (while the new tab menu does) Expected results: 1.) We should have CTRL/CMD K navigate to the search bar in a new tab if the old search bar is hidden 2.) We should consider sending the user to this search bar even if the old bar is present (but this is less clear cut)

This seems like a relatively easy usability win, also in order to have a quick keyboard shortcut to focus the search field on the new tab page. Gavin?

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #1) > This seems like a relatively easy usability win, also in order to have a > quick keyboard shortcut to focus the search field on the new tab page. Gavin? Err, that was meant to be: can we include this in the prioritized backlog? :-)

This is reasonable. Bug 1029889 would also address this case in one way, though if there's already a search bar on screen avoiding a new load seems better. This would make a good mentored bug, I think - flagging it as [diamond] (not sure if doing this correctly). Do you want to mentor, Gijs?

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #3) > This is reasonable. Bug 1029889 would also address this case in one way, > though if there's already a search bar on screen avoiding a new load seems > better. > > This would make a good mentored bug, I think - flagging it as [diamond] (not > sure if doing this correctly). Do you want to mentor, Gijs? Sure. In short, I believe this boils down to: 1) fixing up the cmd/ctrl-k logic ( http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3078 ) to check if the current tab is about:newtab and/or about:home before calling openUILinkIn("about:home", "current"). If the tab is already in the right place, it should focus the search bar. I don't know if we should try to reuse non-selected about:home/about:newtabs - we don't currently, so I guess we should probably continue with that behaviour... although I find it odd that we load about:home in a new tab instead of reusing the about:newtab tab... 2) fixing up the tests we have for this behaviour to include the cases here (about:home already present, about:newtab present, neither present) inasmuch as they don't already.

Doing this in an e10s-friendly way might add a little bit of extra complication.

It is e10s compatible and I've added tests for both about:home & about:newtab. Drew suggested to use the ContentSearch module to deal with about:newtab.

Comment on attachment 8470933 [details] [diff] [review] bug1041678_ctrl_cmd_k_should_go_in_search_bar_in_new_tab.diff Review of attachment 8470933 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3093,5 @@ > + let url = doc.documentURI.toLowerCase(); > + let mm = gBrowser.selectedBrowser.messageManager; > + > + if (url === "about:home") { > + mm.sendAsyncMessage("AboutHome:FocusInput"); We should add a focusInput method to AboutHome.jsm and call that here instead of sending the message directly so that all about:home messages are localized to one site. @@ +3097,5 @@ > + mm.sendAsyncMessage("AboutHome:FocusInput"); > + } else if (url === "about:newtab") { > + ContentSearch.focusInput(mm); > + } else { > + if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) Write this as } else if (!aSearchBar...) { ::: browser/base/content/content.js @@ +219,5 @@ > + > + onFocusInput: function () { > + let doc = content.document; > + let searchBar = doc.getElementById("searchText"); > + if (searchBar) { Why this conditional? searchBar should never be falsey. ::: browser/base/content/newtab/search.js @@ +76,5 @@ > } > }, > > + onFocusInput: function () { > + if (this._nodes.text) { Same here. ::: browser/base/content/test/general/browser_aboutHome.js @@ +423,5 @@ > + desc: "Cmd+k should focus the search bar element", > + setup: function () {}, > + run: function () > + { > + return Task.spawn(function* () { If you'd like, you can write this as run: Task.async(function* () { Task.async() returns a function that returns a Task.spawn(), like you're doing here, so it's just syntactic sugar. You can search mxr for examples. @@ +428,5 @@ > + let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; > + let logo = doc.getElementById("brandLogo"); > + let searchInput = doc.getElementById("searchText"); > + > + is(searchInput, doc.activeElement, "Search input should be the active element."); I don't think you need this check. Plus it's a little dangerous because a previous test function in this file may have changed the focused element. ::: browser/base/content/test/general/head.js @@ +81,5 @@ > }, 100); > var moveOn = function() { clearInterval(interval); nextTest(); }; > } > > +function promiseWaitForCondition(aConditionFn, aMaxTries=50, aCheckInterval=100) { Adding this function is fine, but can't you call waitForCondition as part of its implementation instead of basically reimplementing it? ::: browser/base/content/test/newtab/browser_newtab_search.js @@ +186,5 @@ > EventUtils.synthesizeKey("VK_DELETE", {}); > ok(table.hidden, "Search suggestion table hidden"); > > + // Focus a different element than the search input, like the url bar. > + let urlBar = getOwnerDocument().getElementById("urlbar"); I feel like it's overkill to reach out of the page into chrome just to focus an element. Please get the newtab-customize-button in the page and focus() it instead. ::: browser/modules/ContentSearch.jsm @@ +131,5 @@ > }, > > + /** > + * Used when `Ctrl/Cmd + k` is pressed in about:newtab and sends an async > + * message on the `ContentSearch` channel. Just say something like "Focuses the search input in the page with the given message manager." Also, please move this below so it's below init(). @@ +133,5 @@ > + /** > + * Used when `Ctrl/Cmd + k` is pressed in about:newtab and sends an async > + * message on the `ContentSearch` channel. > + * @param aMessageManager > + * The global MessageManager object. It's not the global message manager -- at least it shouldn't be. If it were global, then if you had multiple about:newtab's open, this would cause Cmd+K to focus the inputs on all of them. It's the message manager of a specific browser, like how you're calling this in browser.js above. @@ +135,5 @@ > + * message on the `ContentSearch` channel. > + * @param aMessageManager > + * The global MessageManager object. > + */ > + focusInput: function (aMessageManager) { Drop the "a" prefix from the parameter so it's just "messageManager" @@ +136,5 @@ > + * @param aMessageManager > + * The global MessageManager object. > + */ > + focusInput: function (aMessageManager) { > + aMessageManager.sendAsyncMessage("ContentSearch", {type: "FocusInput"}); "ContentSearch" -> OUTBOUND_MESSAGE And to match the style here, please write this as sendAsyncMessage(OUTBOUND_MESSAGE, { type: "FocusInput", });

(In reply to Drew Willcoxon :adw from comment #7) > Also, please move this below so it's below init(). *please move this *up* so it's below init().

(In reply to Drew Willcoxon :adw from comment #7) > > ::: browser/base/content/test/general/head.js > @@ +81,5 @@ > > }, 100); > > var moveOn = function() { clearInterval(interval); nextTest(); }; > > } > > > > +function promiseWaitForCondition(aConditionFn, aMaxTries=50, aCheckInterval=100) { > > Adding this function is fine, but can't you call waitForCondition as part of > its implementation instead of basically reimplementing it? > I also took the implementation from another part. In the new patch I'll use waitForCondition, but for me the other implementation is better (also rejects the deferred).

All the suggestions from the review has been addressed.

Comment on attachment 8471045 [details] [diff] [review] bug1041678_ctrl_cmd_k_should_go_in_search_bar_in_new_tab.diff Review of attachment 8471045 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Did you push this to try yet? ::: browser/base/content/browser.js @@ +3102,5 @@ > + AboutHome.focusInput(mm); > + } else if (url === "about:newtab") { > + ContentSearch.focusInput(mm); > + } else if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) { > + openUILinkIn("about:home", "current"); Nit: fix the indentation here

Comment on attachment 8471132 [details] [diff] [review] Ctrl / Cmd + k should go in search bar in new tab Review of attachment 8471132 [details] [diff] [review]: ----------------------------------------------------------------- I think Drew's review is fine here, but as you asked me to look, I looked until I found a thing to ask about, at least. ;-) ::: browser/base/content/test/newtab/browser_newtab_search.js @@ +189,5 @@ > + // Focus a different element than the search input. > + let btn = getContentDocument().getElementById("newtab-customize-button"); > + yield Promise.all([ > + promiseClick(btn), > + ]).then(TestRunner.next); I'm confused. Why Promise.all([]) with an array of just 1 item? Does promiseClick(btn).then(TestRunner.next); not work? If not, please add a comment explaining why. :-)

(In reply to :Gijs Kruitbosch from comment #14) > Comment on attachment 8471132 [details] [diff] [review] > Ctrl / Cmd + k should go in search bar in new tab > > Review of attachment 8471132 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think Drew's review is fine here, but as you asked me to look, I looked > until I found a thing to ask about, at least. ;-) > > ::: browser/base/content/test/newtab/browser_newtab_search.js > @@ +189,5 @@ > > + // Focus a different element than the search input. > > + let btn = getContentDocument().getElementById("newtab-customize-button"); > > + yield Promise.all([ > > + promiseClick(btn), > > + ]).then(TestRunner.next); > > I'm confused. Why Promise.all([]) with an array of just 1 item? Does > promiseClick(btn).then(TestRunner.next); not work? If not, please add a > comment explaining why. :-) Good catch Gijs, it was probably a leftover during development. Calling `then` on a promise returns another promise, so it works just fine how you've proposed. I've also made this fix.

https://tbpl.mozilla.org/?tree=Try&rev=1107cfbf9b83 Failures are not related with this bug. Checkin needed?

Did you update your patch with the promise change? If so, please attach it, and then add checkin-needed to the Keywords field, and if not, then just add checkin-needed.

(In reply to Drew Willcoxon :adw from comment #18) > Did you update your patch with the promise change? If so, please attach it, > and then add checkin-needed to the Keywords field, and if not, then just add > checkin-needed. I ran the tests with that promise changed applied, but forgot to refresh the patch.

Comment on attachment 8473331 [details] [diff] [review] Ctrl / Cmd + K should focus search bar in new tab >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ let doc = gBrowser.selectedBrowser.contentDocument; >+ let url = doc.documentURI.toLowerCase(); I think this is relying on a CPOW to wrap .contentDocument in the e10s case, which is not ideal. Rather than using .contentDocument.documentURI, you should use gBrowser.currentURI.spec. >diff --git a/browser/base/content/content.js b/browser/base/content/content.js >+ onFocusInput: function () { >+ content.document.getElementById("searchText").focus(); In theory, it is possible for about:home to have navigated before this message is received, isn't it? So it should probably handle the case where getElementById("searchText") is null, and better yet you might want to validate that content.document is still about:home.

(It's probably worth fixing those issues in a followup bug, not here.)

Thanks for the review Gavin, they will be fixed soon.

Oops, thank you, Gavin. Sorry you had to clean up after me. AboutHomeListener.onUpdate checks the document URI. onFocusInput should, too. Better yet, that check should be moved to receiveMessage. Better still, that check should be factored out into its own method, and then both receiveMessage and handleEvent should call it so that all the various methods that they delegate to don't have to, and we don't get tripped up by this again. Alex, could you please file a bug for all this, mark it blocking this one, and work on it? Let me know if you need help.

I verified the bug using the following environment: FF 34 Build Id: 20140818030205 OS: Mac Os X 10.8.5 and found some inconsistencies: 1. In new tab page if you don't remove the search bar from the toolbar and use Cmd+K the focus is set to the search bar of the new tab page( this is different behavior from previous Firefox releases). 2. In about:home page if you don't remove the search bar from the toolbar and use CMD + K the focus is set to the search bar of the about:home page on a normal window but doing the same scenario on the e10 about:home page will set the focus to the search bar from the toolbar.

Alex, any feedback on the inconsistencies in comment 27? Should we reopen this issue? Should we track the issues separately?

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #28) > Alex, any feedback on the inconsistencies in comment 27? Should we reopen > this issue? Should we track the issues separately? Bug 1054776 (a followup bug) introduced the correct behavior for this bug and we've also added some regression tests. If the issues from comment 27 cannot be reproduced in the last build from today (and I can't on an OS X 10.9.4), we can consider this bug as fixed because it helped us with regression testing.

Verified as fixed using the following environment: FF 34 Build Id: 20140827030202 OS: Mac Os X 10.8.5

This is definitely not fixed in FF 38.0.5 on Mac (OS 10.10.3), and it wasn't present until recently. It's particularly bad because some sites, like Gmail, allow use of Command+K to hyperlink text, but others allow Firefox default behavior.

(In reply to John from comment #31) > This is definitely not fixed in FF 38.0.5 on Mac (OS 10.10.3), and it wasn't > present until recently. It's particularly bad because some sites, like > Gmail, allow use of Command+K to hyperlink text, but others allow Firefox > default behavior. I also checked running FF in Safe Mode, with same result. When the search bar is shown on the toolbar, Command+K moves the cursor there. With it not, it loads the home page.

(In reply to John from comment #31) > This is definitely not fixed in FF 38.0.5 on Mac (OS 10.10.3), and it wasn't > present until recently. It's particularly bad because some sites, like > Gmail, allow use of Command+K to hyperlink text, but others allow Firefox > default behavior. The point of this bug was focusing the search bar inside the default "new tab" page. That works. We never intended to break ctrl/cmd-k when you're on any other webpage and the webpage does not override it, and so it loading the homepage is still expected. If you think this behaviour should change, please file a new bug.

(In reply to :Gijs Kruitbosch from comment #33) > (In reply to John from comment #31) > > This is definitely not fixed in FF 38.0.5 on Mac (OS 10.10.3), and it wasn't > > present until recently. It's particularly bad because some sites, like > > Gmail, allow use of Command+K to hyperlink text, but others allow Firefox > > default behavior. > > The point of this bug was focusing the search bar inside the default "new > tab" page. That works. We never intended to break ctrl/cmd-k when you're on > any other webpage and the webpage does not override it, and so it loading > the homepage is still expected. If you think this behaviour should change, > please file a new bug. Sorry for my confusion about this. Two questions, if you will: 1) I do feel like it would be a better feature for ctrl/cmd-k to focus on the "awesome bar" when the search bar is hidden. Should that be a separate bug request? 2) Regardless of your answer to #1, is there a way to disable the cmd-k shortcut? Thanks!

(In reply to John from comment #34) > Sorry for my confusion about this. Two questions, if you will: > 1) I do feel like it would be a better feature for ctrl/cmd-k to focus on > the "awesome bar" when the search bar is hidden. Should that be a separate > bug request? Yes. > 2) Regardless of your answer to #1, is there a way to disable the cmd-k > shortcut? Not without an add-on. It's possible that some shortcut configuration add-ons let you do this, I haven't used any recently enough to know for sure ( https://addons.mozilla.org/en-US/firefox/addon/customizable-shortcuts/ would be an example.)