Closed
Bug 1054776
Opened 10 years ago
Closed 10 years ago
Ctrl+K should focus the search bar if it is in the toolbar
Categories
(Firefox :: Search, defect)
Tracking
()
VERIFIED
FIXED
Firefox 34
Tracking | Status | |
---|---|---|
firefox33 | --- | unaffected |
firefox34 | - | affected |
People
(Reporter: PatrickWesterhoff, Assigned: alexbardas)
References
Details
(Keywords: regression, ux-consistency)
Attachments
(1 file, 1 obsolete file)
When on the new tab page, pressing CTRL+K will focus the search on that page instead of the one in the toolbar. So as the search box replaces the one built into the interface here, it should deliver the same functionality. Now, one thing I very often use is the ability to switch the search engines using CTRL+Up or CTRL+Down. This however does not work on the search input on the new tab page. I’m also not really a fan of the shortcut focussing the box on the page anyway. It introduces a very inconsistent behavior (in *every* other situation, it will focus the built-in search box), and also prevents me from searching for things when I for example press CTRL+K first, enter content and then open a new tab in which I want to search (as the search content isn’t copied to the box on the new tab page).
Comment 1•10 years ago
|
||
Regression window(fx) Good: https://hg.mozilla.org/integration/fx-team/rev/efe3c7a0091c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140815083616 Bad: https://hg.mozilla.org/integration/fx-team/rev/abd2919b124b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140815090316 Pushlog: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=efe3c7a0091c&tochange=abd2919b124b Regressed by: Bug 1041678
Blocks: 1041678
Status: UNCONFIRMED → NEW
status-firefox33:
--- → unaffected
status-firefox34:
--- → affected
tracking-firefox34:
--- → ?
Ever confirmed: true
Keywords: regression
Version: Trunk → 34 Branch
Comment 2•10 years ago
|
||
Not really a regression, this is intentional behavior of the new design. This is perhaps a further enhancement request.
Keywords: regression
Comment 3•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2) > Not really a regression, this is intentional behavior of the new design. > This is perhaps a further enhancement request. I do not think so. Bug 1041678 intended that If SearchBar is not placed in Toolbar, then Ctrl+K should focus the Search field of the newtab insted of open about:home. see Bug 1041678 coment#0. So, implementation of the Bug 1041678 is wrong. If Searchbar is placed in Toolbar, Ctrl+K should focus the Searchbar.
Updated•10 years ago
|
Blocks: 1041678
Keywords: regression
Updated•10 years ago
|
Keywords: ux-consistency
Comment 4•10 years ago
|
||
Ah, that's a separate issue.
Summary: Ctrl+K focuses search on new tab page but does not allow changing the search engine → Ctrl+K should focus the search bar if it is in the toolbar
Assignee | ||
Comment 5•10 years ago
|
||
There were multiple comments, like: "We should consider sending the user to this search bar even if the old bar is present" or "If the tab is already in the right place, it should focus the search bar." but this bug clarifies the behavior. I will also have make sure that tests work for both cases, as described here.
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•10 years ago
|
||
It was not really my intention to get it changed in the way that the old bar is focused, but since I prefer it that way anyway (as stated in my original comment), I absolutely don’t mind if that’s the way this is going to be resolved ;) Although I think that the search engine switching ability using CTRL+Up/Down should work on the new tab page too. But I guess we can leave that for another bug.
Comment 7•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #2) > Not really a regression, this is intentional behavior of the new design. > This is perhaps a further enhancement request. Bug 1054932 was filed for this.
Assignee | ||
Comment 8•10 years ago
|
||
There are also some tests for Cmd + k functionality in browser/components/customizableui/test/browser_901207_searchbar_in_panel.js , but I also added tests to catch both cases for about:home and about:newtab.
Attachment #8474630 -
Flags: review?(adw)
Comment 9•10 years ago
|
||
Comment on attachment 8474630 [details] [diff] [review] Cmd / Ctrl + K should focus toolbar searchbar if present otherwise the search input from the page Patch 1 Review of attachment 8474630 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3098,5 @@ > #endif > let openSearchPageIfFieldIsNotActive = function(aSearchBar) { > let url = gBrowser.currentURI.spec.toLowerCase(); > let mm = gBrowser.selectedBrowser.messageManager; > + if (!aSearchBar) { This isn't quite right I don't think. With respect to what this chunk of code was originally doing before 1041678, we simply want to add additional checks for about:home and newtab before resorting to opening a new about:home. So this should be if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) { let url = ...; if (url === "about:home") { } else if (url === "about:newtab") { } else { openUILinkIn("about:home"); } } (Bug 1054931 points out that this is simplistic in the newtab case, but we can handle that there.) ::: browser/base/content/test/general/browser_aboutHome.js @@ +419,5 @@ > }); > } > }, > { > + desc: "Cmd+k should focus the search bar element from toolbar", To make this a little clearer: "Cmd+k should focus the search box in the toolbar when it's present" @@ +435,5 @@ > + is(searchInput, doc.activeElement, "Search bar should be the active element."); > + }) > +}, > +{ > + desc: "Cmd+k should focus the search element from page if searchbar from toolbar is removed", Cmd+k should focus the search box in the page when the search box in the toolbar is absent @@ +451,5 @@ > > EventUtils.synthesizeKey("k", { accelKey: true }); > yield promiseWaitForCondition(() => doc.activeElement === searchInput); > is(searchInput, doc.activeElement, "Search input should be the active element."); > + CustomizableUI.reset(); It would be good to reverse these two tests so that the one that removes the search box from the toolbar runs first, like you do in the browser_newtab_search.js test. That way if there's some problem restoring the search box when CustomizableUI.reset is called, it'll be obvious. ::: browser/base/content/test/newtab/browser_newtab_search.js @@ +200,5 @@ > + // Reset changes made to toolbar > + CustomizableUI.reset(); > + > + // Test that Ctrl/Cmd + K will focus the search bar from toolbar. > + let searchBar = window.document.getElementById("searchbar"); gWindow.document @@ +202,5 @@ > + > + // Test that Ctrl/Cmd + K will focus the search bar from toolbar. > + let searchBar = window.document.getElementById("searchbar"); > + EventUtils.synthesizeKey("k", { accelKey: true }); > + is(searchBar.textbox.inputField, getOwnerDocument().activeElement, "Toolbar's search bar should be focused"); Just use gWindow.document.activeElement instead of getOwnerDocument. ::: browser/base/content/test/newtab/head.js @@ +185,5 @@ > /** > + * Returns the selected tab's owner document. > + * @return The owner document. > + */ > +function getOwnerDocument() { This isn't necessary with the gWindow.document change I mentioned above.
Attachment #8474630 -
Flags: review?(adw)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8474630 -
Attachment is obsolete: true
Attachment #8474733 -
Flags: review?(adw)
Comment 11•10 years ago
|
||
Comment on attachment 8474733 [details] [diff] [review] Cmd / Ctrl + K should focus toolbar searchbar if present otherwise the search input from the page Patch 2 Review of attachment 8474733 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! This partly conflicts with your patch in bug 1054600. Once you post your new patch there, I'll resolve the conflict locally, push to try, and then land them together if try is green.
Attachment #8474733 -
Flags: review?(adw) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #11) > Comment on attachment 8474733 [details] [diff] [review] > Cmd / Ctrl + K should focus toolbar searchbar if present otherwise the > search input from the page Patch 2 > > Review of attachment 8474733 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! This partly conflicts with your patch in bug 1054600. Once you > post your new patch there, I'll resolve the conflict locally, push to try, > and then land them together if try is green. The patch from bug 1054600 should be applied first.
Comment 13•10 years ago
|
||
You're right, my bad.
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b9d3bfad08d8 https://hg.mozilla.org/integration/fx-team/rev/11d845395638
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/11d845395638
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Flags: qe-verify+
Comment 17•10 years ago
|
||
As per the comment #1 > Bad: > https://hg.mozilla.org/integration/fx-team/rev/abd2919b124b > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 > ID:20140815090316 I can reproduce the bug in old build 34.0a1 (2014-08-16). I can confirm this is fixed for Linux x86_64 in latest nightly 34.0a1 (2014-08-20). Awaiting confirmation from other platforms.
Updated•10 years ago
|
QA Whiteboard: [bugday-20140820]
Comment 18•10 years ago
|
||
Pressing CTRL+K focus in the search bar in the toolbar on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140820030202 CSet: ffdd1a398105
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•