Closed Bug 949365 Opened 8 years ago Closed 8 years ago

[Australis] Ctrl-K/Cmd-K doesn't focus the search bar

Categories

(Firefox :: Toolbars and Customization, defect)

28 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: smontagu, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

In a fresh nightly build the behaviour of Cmd-K/Crtl-K has changed. Instead of focusing the search bar it opens google.com (or whatever is selected as default search engine). If I remove the search bar via Toolbars | Customize, Cmd-K does nothing.

Seen on both Linux and OSX, with both a previously existing profile and a newly created one.
N.B. This is not a dupe of bug 901207, since I have the fix for that in my tree -- could it be regression from that fix?
Flags: needinfo?(jaws)
Looks like it is:

> -    if (searchBar && window.fullScreen)
...
> +    if (searchBar && window.fullScreen) {
>        FullScreen.mouseoverToggle(true);
> -    if (searchBar)
>        searchBar.select();
Blocks: 901207
Attached patch Patch (obsolete) — Splinter Review
Hm, I'm not sure how that got checked in. I was pretty sure that the code I had for this was what I have attached as a patch here. Thank you for filing the bug and getting attention to this so quickly!
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8346569 - Flags: review?(mconley)
Flags: needinfo?(jaws)
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → 28 Branch
Attached patch Patch with testSplinter Review
Added a test because I hate regressions.
Attachment #8346569 - Attachment is obsolete: true
Attachment #8346569 - Flags: review?(mconley)
Attachment #8346573 - Flags: review?(mconley)
Comment on attachment 8346573 [details] [diff] [review]
Patch with test

Review of attachment 8346573 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. For brownie points, if no test exists for the shortcut opening the webpage if the control has been entirely removed, can you file a followup bug for that? Non-trivial because it needs a dummy search provider because otherwise we'd access the network, which is a no-no in tests. Also, I missed some nits originally...

::: browser/base/content/browser.js
@@ +2951,5 @@
>          // The searchBar gets moved when the overflow panel opens.
>          searchBar = this.searchBar;
>          searchBar.select();
>          openSearchPageIfFieldIsNotActive(searchBar);
>        });

This callback is identical to the one above it. As it's an arrow function anyway, assigning it to a local variable beforehand would simplify this code.

@@ +2952,5 @@
>          searchBar = this.searchBar;
>          searchBar.select();
>          openSearchPageIfFieldIsNotActive(searchBar);
>        });
>        return;

After the return in the previous conditional, nix the 'else' part of the else if. No else after return ever, please.
Attachment #8346573 - Flags: review?(mconley) → review+
Summary: Ctrl-K/Cmd-K doesn't focus the search bar → [Australis] Ctrl-K/Cmd-K doesn't focus the search bar
https://hg.mozilla.org/mozilla-central/rev/4bc1aa287621
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Duplicate of this bug: 950011
Blocks: 949428
QA Contact: cornel.ionce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on Firefox 29 beta 1 (build ID:20140318013849).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.