Wrong search engine in right-click menu only in "wide screen" mode

When the search box is hidden (either because you've removed it from the toolbar or you've hidden the toolbar), the browser uses the "default" search engine. There are two problems here:

1) There is no way to change what the "default" search engine is (the whole "default" search engine thing is mostly vestigal, left over from the old days, but there are a few remaining places, such as here, where it still gets used). This is bug 331105.

2) The search box shouldn't even be considered as hidden for the purposes of search engine selection when you're in full-screen mode with the toolbars set on auto-hide, since auto-hide represents temporary hiding and not the result of you trying to remove the search box. AFAIK, there is no pre-existing bug to deal with this particular case.

This bug is confirmed downstream on Launchpad (Ubuntu) - https://bugs.launchpad.net/bugs/310217

I'm unfamiliar with FF's bugtracker, do I need to do anything else here? I will attach this bug to Launchpad so we can track its progress from there.

Thank you.

*** Bug 504568 has been marked as a duplicate of this bug. ***

*** Bug 527427 has been marked as a duplicate of this bug. ***

*** Bug 561113 has been marked as a duplicate of this bug. ***

*** Bug 579703 has been marked as a duplicate of this bug. ***

FWIW, the "Full Screen" case seems to be WFM on Trunk.
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100717 Minefield/4.0b2pre ID:20100717054110

*** Bug 595511 has been marked as a duplicate of this bug. ***

*** Bug 607597 has been marked as a duplicate of this bug. ***

This does not occur anymore with 4.0 beta 7 as packaged by the Debian Mozilla Team (http://mozilla.debian.net/packages/).

*** Bug 622790 has been marked as a duplicate of this bug. ***

*** Bug 625466 has been marked as a duplicate of this bug. ***

Same also applies for the keyboard shortcut to focus the search bar.

Created attachment 503583
patch

Would something like this work?

Comment on attachment 503583
patch

You will have to ask for review/feedback. Gavin should be able to give you the answer.

(In reply to comment #14)
> Created attachment 503583 [details]
> patch
>
Hrm, all the comments around that change explicitly mention taking the default rather than current if the search bar is removed.

And it looks like I missed a few places for changing it to 'currentEngine' for loadSearch() right below the patch cuts off in the file.

I can change loadSearch to only consider currentEngine in a revised patch.

Why is the behavior changed based on the presence of the search bar in the first place, though?

Created attachment 503735
Change loadSearch as well (and remove references to defaultEngine from comments)

This patch also changes loadSearch to only use the currentEngine, and it changes the comments for the affected functions to no longer reference defaultEngine.

(Are there any weird cases where currentEngine isn't set, but defaultEngine is?)

*** Bug 635136 has been marked as a duplicate of this bug. ***

Comment on attachment 503735
Change loadSearch as well (and remove references to defaultEngine from comments)

Gavin says this needs ui-r.
But who?

Comment on attachment 503735
Change loadSearch as well (and remove references to defaultEngine from comments)

Pinging Beltzner for ui-r.

This patch changes the behavior to use currentEngine instead of defaultEngine. Is this acceptable?

As I alluded to in comment 1, there is a bit of history behind this bug.

First, the use of the "default" engine is explained in bug 331105 comment 3 and bug 317107 comment 40. Next, when I confirmed this bug in 2008, the main problem was that in full-screen mode, the toolbar autohides, which the browser misinterprets as "the search box is unavailable", causing it to use the "default" engine. AFAICT, this particular problem with the autohide is no longer present in 4.0, so it was fixed at some point in the intervening years (see comment 7).

So the problem that we have here is this: when the search box is not present (removed by user customization), the context menu search uses a "default" search engine that is configurable by the user only via about:config.

The possible solutions to this are:
1) Do what this (now slightly morphed bug) is proposing, and that is to just not use the "default" engine and use whatever was the previously-selected engine.
2) Disable the context menu search if the search box is hidden (one of the options brought up in bug 331105 comment 4).
3) Fix bug 331105 and add a UI to set a "default" engine.
4) Fix bug 331105 by way of bug 429513 (the "lite" solution). The original intent of bug 429513 was to serve as a stopgap fix for bug 331105 (since at the time, it was too late in the release cycle to really consider bug 331105). Personally, I would prefer bug 331105, but bug 429513 is an option if a new UI is too much for an edge-caseish scenario like this.

Personally, I don't like #2. As for #1 vs. #3 vs. #4, for people who remove the search box, what is the best way for them to communicate to the browser their preferred search engine choice? Explicitly via a dedicated button or option in the search manager UI? Implicitly via selecting their preferred engine prior to removing the search box? Or implicitly by moving their preferred engine to the top of the list in the search manager? Right now, there is no way for them to communicate that to the browser (except via about:config).

*** Bug 655106 has been marked as a duplicate of this bug. ***

Comment on attachment 503735
Change loadSearch as well (and remove references to defaultEngine from comments)

nsContextMenu.js drives the engine name that shows up in the context menu, so with the current patch, it'll use the right engine but show the wrong text.

See isTextSelection in that file for a code path similar to the one you already changed.

if (isElementVisible(BrowserSearch.searchBar))

This seems to have been fixed in Nightly (9.0a1).

There's two parts to this bug:

1. When in Full screen mode

2. When the Search bar is removed

Issue 1 is fixed for me in Nightly (10.0a1). Issue 2 is not. Even though I use DuckDuckGo as my "default" search engine (it's on the top of the list, and it's the one I have selected when I remove the search bar) I am still sent to google.com when I press Ctrl+K.

There's a workaround for this: in about:config, set browser.search.defaultenginename to the name of the selected search engine.

This changes the search engine used by the context menu, and the destination of Ctrl+K. You'll have to do this each time you change your selected search engine.

A fix for this would be for Firefox to change the value of this key each time a search engine is chosen in the search bar, and not to change this key when hiding the search bar.

…There is actually a preference browser.search.selectedEngine — is it possible that the context menu and Ctrl+K are referring to browser.search.defaultenginename when they should be referring to this instead?

The code intentionally uses the default engine, it's not a matter of accidentally reading the wrong pref (the prefs are internal to the search service, the users can choose to use either .defaultEngine or .currentEngine, as shown in the patch).

*** Bug 790956 has been marked as a duplicate of this bug. ***