If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ctrl+K should focus the search bar if it is in the toolbar

VERIFIED FIXED in Firefox 34

Status

()

Firefox
Search
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Patrick Westerhoff, Assigned: alexbardas)

Tracking

({regression, ux-consistency})

34 Branch
Firefox 34
regression, ux-consistency
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34- affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 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
Not really a regression, this is intentional behavior of the new design. This is perhaps a further enhancement request.
Keywords: regression
No longer blocks: 1041678

Comment 3

3 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

3 years ago
Blocks: 1041678
Keywords: regression

Updated

3 years ago
Keywords: ux-consistency
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

3 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

3 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.
(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.
Blocks: 1054933
(Assignee)

Comment 8

3 years ago
Created attachment 8474630 [details] [diff] [review]
Cmd / Ctrl + K should focus toolbar searchbar if present otherwise the search input from the page Patch 1

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

3 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

3 years ago
Created attachment 8474733 [details] [diff] [review]
Cmd / Ctrl + K should focus toolbar searchbar if present otherwise the search input from the page Patch 2
Attachment #8474630 - Attachment is obsolete: true
Attachment #8474733 - Flags: review?(adw)

Comment 11

3 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

3 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

3 years ago
You're right, my bad.

Comment 14

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b9d3bfad08d8
https://hg.mozilla.org/integration/fx-team/rev/11d845395638
https://hg.mozilla.org/mozilla-central/rev/11d845395638
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Duplicate of this bug: 1055057
Flags: qe-verify+
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.
QA Whiteboard: [bugday-20140820]

Comment 18

3 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

3 years ago
Duplicate of this bug: 1055985
tracking-firefox34: ? → -
You need to log in before you can comment on or make changes to this bug.