436265 - Use currently selected search engine in the context menu (or Ctrl/Cmd+K) when search bar is hidden

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[chunlin zhang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

the popup menu means when I selected some words,and right click the selected part,the popup menu then appears.One of the menu items is "Search google for "the selected part""

Reproducible: Always

Steps to Reproduce:
1.F11 to full screen mode
2.selected some words
3.check the popup menu,it is ok now
4.change the search engine(toolbar) to one of non-google search engine
4.selected some words
5.check the popup menu,now is "search Google for "the selected part""

Comment 1

[Kai Liu]

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.

Comment 2

[Connor Imes]

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.

Comment 7

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

Comment 10

[Denis Laxalde]

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: patch

Would something like this work?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 503583 [details] [diff] [review]
patch

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #14)
> Created attachment 503583 [details] [diff] [review]
> 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?

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: 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?)

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 503735 [details] [diff] [review]
Change loadSearch as well (and remove references to defaultEngine from comments)

Gavin says this needs ui-r.
But who?

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 503735 [details] [diff] [review]
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?

Comment 21

[Kai Liu]

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).

Comment 23

[Ed Lee :Mardak]

Comment on attachment 503735 [details] [diff] [review]
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))

Comment 24

[a2200854]

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

Comment 25

[børge]

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.

Comment 26

[Grey Nicholson (they/them)]

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.

Comment 27

[Grey Nicholson (they/them)]

…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?

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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).