In order to help address the problem that is search hijacking, and to make search engine selection more intuitive, I'd like to have location bar, context menu, and about:home searches use the same engine, and have that be controlled by a single pref (to be configured in the preferences dialog). It's not clear exactly what the plan is yet, but this could mean: - adding some prefs UI for "Default search engine" selection - removing the possibility of override with keyword.URL (or perhaps just changing the pref name?) - adding support for arbitrary engines to about:home - fixing bug 587780 to better support tracking multiple search origins

This sounds good to me. I think we should have a search provider selection widget in the General prefs. But what does that show when your searches are mis-aligned? Do we have a "reset all to default" button or does picking a default from the list automatically do that? Also, should this be a metabug and we get bugs on file for making each of these changes? Or should we do it all here?

I propose that we fix Bug 784189 until there are resources to tackle this properly.

Actually, as the default is already to use the default engine if it's unset, I wonder if we should just remove keyword.URL completely. (In reply to Asa Dotzler [:asa] from comment #1) > I think we should have a search provider selection > widget in the General prefs. FYI, when I ported the OpenSearch code we are using in Firefox to SeaMonkey, I did put such a selection into prefs there, as the suite always had something like that (Firefox had "removed" it compared to that in its large reorg of UI including prefs). Should be pretty easy to "backport" that to Firefox in some form. :)

The rough outline of my action plan is: 1) Implement prefs UI for controlling the browser.search.defaultenginename pref (which controls the value of nsIBrowserSearchService.defaultEngine and originalDefaultEngine). That's what this WIP patch does. Need to sort out the right UX for that, and then also complete the in-content version. 2) Audit the nsIBrowserSearchService code to ensure that it properly deals with the concept of a user-settable default engine. I don't quite recall the details offhand but I think there were a few things that need to change here. originalDefaultEngine probably no longer makes sense as a concept. nsSearchService probably needs to ensure that the defaultEngine pref always points to an non-hidden engine, which might involve adding some code to removeEngine to reset the pref when removing/hiding the default engine. 3)a) Remove the use of the keyword.URL for Firefox in nsDefaultURIFixup.cpp. This might be controversial, other apps might want to continue to have the pref. In that case we may have to add a build-time flag that Firefox can opt into. 3)b) Switch nsDefaultURIFixup from using originalDefaultEngine to just using defaultEngine (given 2 above) 4) Audit uses of nsIBrowserSearchService.defaultEngine to make sure they're OK with the new semantics of that property. Audit consumers of currentEngine to see whether we should switch them to defaultEngine instead. One longstanding example is bug 652487. There are other related bugs in the dependency chain that we'll need to revisit.

I created a screencast that shows I'm at crossroads where I could use some input from a UX point of view: http://www.screenr.com/O0E7 Does having a distinction between a default and a current search engine still make sense? Should we merge the two settings or should we make the current engine pref in the search box dropdown active per tab, instead of all tabs?

Note: if we remove the distinction between current and default engine, it will be harder to implement bug 384124

I assume this is a first step toward a unified location-search bar ?

(In reply to Guillaume C. [:ge3k0s] from comment #8) > I assume this is a first step toward a unified location-search bar ? That's not the goal - what we're really trying to solve here is an issue where add-ons can change a user's search preferences programmatically but the user has no UI-exposed way to change them back ( they currently need to edit the value of keyword.URL in about:config ). See also comment 1 on this bug.

(In reply to Jeff Griffiths (:canuckistani) from comment #9) > (In reply to Guillaume C. [:ge3k0s] from comment #8) > > I assume this is a first step toward a unified location-search bar ? > > That's not the goal - what we're really trying to solve here is an issue > where add-ons can change a user's search preferences programmatically but > the user has no UI-exposed way to change them back ( they currently need to > edit the value of keyword.URL in about:config ). See also comment 1 on this > bug. I know, but this clearly block the merge of the location and the search bar (I don't know if the change is planned in a near future though).

(In reply to Guillaume C. [:ge3k0s] from comment #10) ... > I know, but this clearly block the merge of the location and the search bar > (I don't know if the change is planned in a near future though). AFAICT it isn't, maybe Limi has something up his sleeve but the main upcoming changes to Firefox desktop UI that I'm aware of ( Australis and Add-ons navigation bar work ) don't include any plans for this.

(In reply to Jeff Griffiths (:canuckistani) from comment #9) > they currently need to edit the value of keyword.URL in about:config Note that "edit" in this case means "set it back to empty" in the ideal case, as by default we already have it at that and use the default search engine anyhow. Best to have UI for setting that default search engine and not have keyword.URL as a pref at all, so that things cannot be mis-set and the user has obvious and full control over what's being done. Actually, that's what I'd expect of Firefox by how we advertise it. ;-)

Robert: that's what the patch I attached does (removing keyword.URL)

(In reply to Mike de Boer [:mikedeboer] from comment #13) > Robert: that's what the patch I attached does (removing keyword.URL) I know, I read it and I really like that (even if it takes away a way of customizing, I doubt many do intentionally use it pointing to anything else than default search - even though Firefox did that by default in earlier times by using "I feel lucky" there instead of regular search).

In bug 730445, I'm working on adding UI to Fennec to modify the default search engine, and I think it would be really nice if we could split up this patch to land the search service parts separately from the rest of the desktop UI changes. It looks like the patch is getting pretty big, so splitting it up might be a good idea anyway :) And as a bonus, if we land the parts necessary for Fennec first, we could get some testing on Fennec before landing the bigger changes for desktop.

(In reply to :Margaret Leibovic from comment #15) > In bug 730445, I'm working on adding UI to Fennec to modify the default > search engine, and I think it would be really nice if we could split up this > patch to land the search service parts separately from the rest of the > desktop UI changes. > > It looks like the patch is getting pretty big, so splitting it up might be a > good idea anyway :) > > And as a bonus, if we land the parts necessary for Fennec first, we could > get some testing on Fennec before landing the bigger changes for desktop. Margaret, your wish is my command :) Give a few minutes to separate the patch. I'm curious to learn how Fennec will work with these changes.

This patch touches files across b2g, metro, android and browser code. It has to do so, because each product contains its own prefs file (setting the defaults) and keyword.URL is used there.

This patch touches only the browser component. It updates the settings UI, locales and removes the keyword.URL pref change observer (since the pref is gone).

Ad. to comment #17: I forgot to mention that also some mochitests were modified, because they would otherwise fail when you push to try, for example.

Nice work on consolidating this mess! No more preferences, please! :) We already have an extremely visible, reliable and simple way of selecting your search engine: the selector in the main UI. There's no need for additional indirection, hierarchy or more UI here. Let the main window search selector control what is the default search engine — simple, easy and predictable.

I thought that this bug was going to build on top of bug 335781 which moves the search engine manager to the add-on manager. Then the UI for this bug would just be adding a [Set as default] button to each non-default entry along with some other indicator on the default engine. If for some reason we don't want to build on top of bug 335781 we can add a "Set as default" button in the current search engine manager. I don't think it's ideal to have the default search engine controlled from preferences when we already have a place to manage search engines. (In reply to Alex Limi (:limi) — Firefox UX Team from comment #21) > Let the main window search selector control what is the default search > engine — simple, easy and predictable. I agree we shouldn't add a new place to manage search engine related settings but I'm not sure that users who set their search box to Engine A expect that to also change the engine used for a search in the address bar or about:home.

(In reply to Matthew N. [:MattN] from comment #22) > I agree we shouldn't add a new place to manage search engine related > settings but I'm not sure that users who set their search box to Engine A > expect that to also change the engine used for a search in the address bar > or about:home. about:home explicitly says "Google" right now, so until that is fixed, this isn't affected by it. (It should also be working the same way, though — just saying that it's separate right now, and not sure it's in scope for this bug) The conceptual complexity of having multiple "default search engines" depending on what field you happen to be focused in is worse than discovering that the selector applies to searches in general. Of course, the solution to this is Search Tabs. But until that lands, we'll have to do the best we can in simplifying what's already in our UI.

(In reply to Matthew N. [:MattN] from comment #22) > I thought that this bug was going to build on top of bug 335781 which moves > the search engine manager to the add-on manager. Unless someone new steps in to finish the work on that bug, it won't be fixed any time soon.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #23) > about:home explicitly says "Google" right now, so until that is fixed, this > isn't affected by it. (It should also be working the same way, though — just > saying that it's separate right now, and not sure it's in scope for this bug) This is indeed out of scope AFAIK, it would need more work; the google logo, for instance, is hardcoded in about:home right now and other search providers only need to provide a favicon-type logo. > Of course, the solution to this is Search Tabs. But until that lands, we'll > have to do the best we can in simplifying what's already in our UI. I'm interested! Could you point me to the bug that shows the work in progress? I will continue this the way limi suggested. For backward compat I think I will add a checkbox to the engine manager first and show you all how that looks like.

Comment on attachment 726798 [details] [diff] [review] Part 1: removing originalDefaultEngine and keyword.URL pref I think it would be a good idea to split things up further, and have the search service changes be separate from the keyword.URL removal changes. >diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js >-pref("keyword.URL", "https://www.google.com/m?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q="); I don't think b2g uses the toolkit search service. So we need to keep this to avoid breaking its keyword search? Though maybe they don't actually use this at all. >diff --git a/browser/base/content/test/browser_keywordSearch.js b/browser/base/content/test/browser_keywordSearch.js >+ }/*, > { > name: "normal search (keyword.url)", These should just be removed (not commented out). >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js Looks like you forgot to remove the keyword prompt strings in browser.properties, and KeywordURLResetPrompter.jsm itself. >diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp >+ // NOTE: the use of keyword.URL has been deprecated in favor of the search >+ // toolkit. However, if you still want to use this feature, you have >+ // need to unset the MOZ_TOOLKIT_SEARCH build flag. Hrm, perhaps we should use a different flag for this, it seems odd to tie together "use search service" and "support keyword.URL". Or maybe we can get away with ripping it out entirely. >diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl >- readonly attribute nsISearchEngine defaultEngine; >+ attribute nsISearchEngine defaultEngine; Not that it will matter much in practice, but you should change the interface's IID when making changes that affect backwards compatibility. >- readonly attribute nsISearchEngine originalDefaultEngine; I checked https://mxr.mozilla.org/addons/search?string=originalDefaultEngine (use your LDAP credentials) for whether this would impact any add-ons. Two hits: 1) my add-on, I can fix it :) https://github.com/gavinsharp/SearchReset 2) https://addons.mozilla.org/en-US/android/addon/long-tap-search/, we should reach out to the author if possible. >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js >+ set defaultEngine(val) { >+ // un-cache the currentEngine property >+ this._currentEngine = null; >+ if (this._currentEngine == this._defaultEngine) { >+ Services.prefs.clearUserPref(curPref); >+ } It's not clear to me why the defaultEngine setter should impact the currentEngine/selectedEngine at the search service level. In the screencast I assumed you implemented the interactions entirely in the preferences dialog. Can't these just stay independent? >diff --git a/toolkit/content/widgets/preferences.xml b/toolkit/content/widgets/preferences.xml > <method name="getPreferenceElement"> >- return temp.nodeType == Node.ELEMENT_NODE ? temp : aStartElement; >+ return temp && temp.nodeType == Node.ELEMENT_NODE ? temp : aStartElement; > <method name="userChangedValue"> >+ if (!aElement) >+ return; Why were these changes needed?

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #23) > Of course, the solution to this is Search Tabs. But until that lands, we'll > have to do the best we can in simplifying what's already in our UI. Can we pretty please have those soon? They are the best idea at all I have seen our UX teams come up with in years, but unfortunately, they don't seem to get implemented. Is there a bug for them? (In reply to Alex Limi (:limi) — Firefox UX Team from comment #21) > Let the main window search selector control what is the default search > engine — simple, easy and predictable. I think that 1) it's confusing if I do one search with Wikipedia or OpenStreetMap from the search bar and then any non-URL thing I enter in the main address bar goes to Wikipedia or OSM search instead of Google, and 2) if I customize away the search bar (which is mostly useless for me personally atm), I have no UI to switch the address bar search at all. I really like the idea to integrate this with the add-ons manager, though, like Matt says in comment #22.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #23) > about:home explicitly says "Google" right now, so until that is fixed, this > isn't affected by it. (It should also be working the same way, though — just > saying that it's separate right now, and not sure it's in scope for this bug) No, about:home uses the default engine (both before and after this patch). We could change that, but I don't think it would make sense to hard code Google there. > The conceptual complexity of having multiple "default search engines" > depending on what field you happen to be focused in is worse than > discovering that the selector applies to searches in general. Combining "selected engine" and "default engine" seems to ignore the "transient use" search bar use-case. I think it's quite common for people to always want the context menu, about:Home, and location bar searches to use their "primary" search engine (Google by default, but maybe Bing or Duck Duck Go, whatever), and don't expect that doing a couple of searches using Yelp from the search bar will have an effect on all of those other search interfaces.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #26) > I don't think b2g uses the toolkit search service. So we need to keep this > to avoid breaking its keyword search? Though maybe they don't actually use > this at all. They don't use keyword.URL at all. In fact, while this is set to Google here, the B2G browser uses Bing search.

Comment on attachment 727355 [details] [diff] [review] Part 1: replace originalDefaultEngine with defaultEngine and make defaultEngine a settable >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js > SearchService.prototype = { >- classID: Components.ID("{7319788a-fe93-4db3-9f39-818cf08f4256}"), >+ classID: Components.ID("{89beea9f-61a8-45d3-818d-b259f00195fe}"), wrong UUID - this is the component ID that identifies a specific XPCOM component implementation. The IID (interface ID, in nsIBrowserSearchService.idl) is what needs changing to signify that the nsIBrowserSearchService interface has changed in ways that would affect the vtable of C++ objects implementing that interface. >+ set defaultEngine(val) { >+ let newDefaultEngine = this.getEngineByName(val.name); >+ if (!newDefaultEngine) >+ FAIL("Can't find engine in store!", Cr.NS_ERROR_UNEXPECTED); This should short-circuit (and avoid calling notifyAction) when setting defaultEngine to its existing value (i.e. when newDefaultEngine == _currentEngine here). We should also really have some basic tests for the new defaultEngine behavior (including one that covers that edge case). The existing search service coverage isn't great, but there are a few examples here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/ (ping me if you have any questions about this). We'll need to resolve the question of how this should work (whether we should even have separate default/current concepts) before we move forward with this patch, though.

Another screencast that shows off the latest greatest behavior: http://www.screenr.com/uNb7

Comment on attachment 727357 [details] [diff] [review] Part 2: remove keyword.URL >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties > # LOCALIZATION NOTE (keywordPrompt.yesButton): %1$S is the name of the default search engine > keywordPrompt.yesButton = Yes, use %1$S > keywordPrompt.yesButton.accessKey = Y > > # LOCALIZATION NOTE (keywordPrompt.noButton): %1$S is a host name (e.g. "somewebsearch.com") from the current value of keyword.URL > keywordPrompt.noButton = No, continue using '%1$S' > keywordPrompt.noButton.accessKey = N These can go too. >diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp Turns out MOZ_TOOLKIT_SEARCH is now defined by default and only b2g seems to disable it. Also, no other app in moz/comm-central uses keyword.URL. And so I think we're clear to remove it, but a docshell peer should sign off too.

Comment on attachment 727357 [details] [diff] [review] Part 2: remove keyword.URL sr=me, I think, assuming the b2g folks are ok with this. Always glad to see more code disappear from this function. ;)

(In reply to Boris Zbarsky (:bz) from comment #36) > sr=me, I think, assuming the b2g folks are ok with this. Thanks. As Kairo says, they don't use it (I also confirmed with etienne on IRC). That line in their prefs file was just copied from Fennec when they initially forked prefs.

Comment on attachment 727589 [details] [diff] [review] Part 4: remove keyword.URL related unused locale strings You should just fold this in to the part 2 patch and carry over the r+

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #27) > (In reply to Alex Limi (:limi) — Firefox UX Team from comment #23) > > Of course, the solution to this is Search Tabs. But until that lands, we'll > > have to do the best we can in simplifying what's already in our UI. > > Can we pretty please have those soon? They are the best idea at all I have > seen our UX teams come up with in years, but unfortunately, they don't seem > to get implemented. Is there a bug for them? AFAIK no, all I can find is Limi's speech and this mockup : https://www.dropbox.com/s/skuw4edqdapveut/Shorlander-ConceptWeek-Presentation.pdf (last page of the PDF).

Let's please move discussion about search tabs to a new bug (or firefox-dev).

(In reply to Mike de Boer [:mikedeboer] from comment #34) > Another screencast that shows off the latest greatest behavior: > http://www.screenr.com/uNb7 Thanks for the screencast, very helpful! This is still too complicated. :) The currently selected search engine in the main UI *is* the primary. The "Make Primary" button in the Manage Search Engines window should be removed. In other words, the main window search selector controls the current search engine for everything. (Yes, cue http://xkcd.com/1172/ ;) Nice work, we're getting there!

To clarify Alex's comment 43 just in case, when you change the selected search engine in the search bar from Google to, say, Bing, and then enter a search query in the URL bar, it should use Bing. Right-click a text selection should also show "Search Bing for ..." While this might be odd if users often switch to vertical search engines like Amazon in the search bar, but this solves the important problem of users not knowing what their default search engine or how to change it by displaying it prominently in primary UI. This problem lies between ux-feedback and ux-mode-error: http://spreadsheets.google.com/pub?key=tJxF8zTuLdEj9pUcxnLAemA&output=html Alternately, we could display the name of the default search engine in the URL, e.g. replacing "Search or enter address" with "Search Google or enter address", but that still gets confusing when that search engine does not match the one in the search bar. Doing it the way described in Alex's comment teaches users that the URL bar and the search bar do (or should do after we fix some bugs) the same thing for search queries.

Comment on attachment 727358 [details] [diff] [review] Part 3: adjust engineManager UI to allow changing defaultEngine Review of attachment 727358 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this, Mike! :) We really appreciate your quick iterations and narrated screencasts. The code looks good, but the UI is still too complicated as per comments 43 and 44.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #43) > In other words, the main window search selector controls the current search > engine for everything. (Yes, cue http://xkcd.com/1172/ ;) Could you please explicitly address the concern from comment 28? Are you saying you think that user expectation and use case is so uncommon as to not merit being addressed?

I'm going to say that the concern from comment 28 isn't going to be a real world concern. I'd be more concerned with the potential user confusions behind having multiple ways to set default search provider (or at least, it seems that way to the User)

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28) > No, about:home uses the default engine (both before and after this patch). > We could change that, but I don't think it would make sense to hard code > Google there. Ah, didn't know that. Yes, it doesn't make sense to hardcode Google, I was just trying to make it as simple as possible. In that case, can we show the logos for whatever search engine you have set as the default on about:home? Do we even have these logos? If we don't, how about a placeholder in about:home instead of the logo for now, saying "Search Google for…", "Search Bing for…" etc. Not sure what resources are available and what the easiest path is code-wise, but if we can make about:home respect the search engine selection too, that would be great. We can always file a follow-up bug for adding other logos etc. > > The conceptual complexity of having multiple "default search engines" > > depending on what field you happen to be focused in is worse than > > discovering that the selector applies to searches in general. > > Combining "selected engine" and "default engine" seems to ignore the > "transient use" search bar use-case. I think it's quite common for people to > always want the context menu, about:Home, and location bar searches to use > their "primary" search engine (Google by default, but maybe Bing or Duck > Duck Go, whatever), and don't expect that doing a couple of searches using > Yelp from the search bar will have an effect on all of those other search > interfaces. "Transient search engine" is a problem we've always had (with all the mode errors it causes, and Search Tabs solves), and it's more important to make the behavior simple, predictable and consistent. If people want to use multiple search engines in multiple locations, I'm sure add-ons will step in and fill this need.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #48) > In that case, can we show the > logos for whatever search engine you have set as the default on about:home? > Do we even have these logos? We currently have the logos for Google and Yandex. This type of logo is not part of the OpenSearch spec, but we can probably obtain the logos for at least the rest of our built-in engines rather easily. For example, the purple Yahoo! logo in the top right of this sprite sheet works from a visual standpoint: http://l4.yimg.com/dh/ap/default/130307/pc_icons_btns_sprite_0307_10am_x2.png

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #48) > Not sure what resources are available and what the easiest path is > code-wise, but if we can make about:home respect the search engine selection > too, that would be great. We can always file a follow-up bug for adding > other logos etc. about:home should already support non-Google engines, just not with icons. We can try to fix that somehow in followup bugs (but it's a pain, because icons of that size aren't part of OpenSearch and so we'll need to add them all to our shipped engines, and if a user installs a custom engine it's not likely to have a large icon).

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #50) > about:home should already support non-Google engines, just not with icons. > We can try to fix that somehow in followup bugs (but it's a pain, because > icons of that size aren't part of OpenSearch and so we'll need to add them > all to our shipped engines, and if a user installs a custom engine it's not > likely to have a large icon). Indeed, and on top of that we also need HiDPI versions too for each logo. Pain++. I'm afraid you're wrong about the level of support for those images in the opensearch spec: http://www.opensearch.org/Specifications/OpenSearch/1.1#The_.22Image.22_element It's most likely the case that search providers only provide 16x16 (ICO) and 64x64 (PNG) sizes, as they are mentioned in the spec, but there are no restrictions listed. We *could* rally the troops and ask other engines to specify more image formats in their manifests, right?

Aaaand *almost* obligatory screencast: http://www.screenr.com/Oob7

Comment on attachment 728234 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 728234 [details] [diff] [review]: ----------------------------------------------------------------- This is great! :) Thanks for the screencast too. They are informative and fun! Overall, the code looks good to me. Just a few small things: ::: browser/base/content/abouthome/aboutHome.js @@ +205,5 @@ > + let searchEngineName = document.documentElement.getAttribute("searchEngineName"); > + let searchEngineInfo = SEARCH_ENGINES[searchEngineName]; > + let logoElt = document.getElementById("searchEngineLogo"); > + if (!searchEngineInfo) { > + logoElt.style.width = "0px"; The following looks a bit better, because the container has padding: logoElt.parentNode.hidden = true; and in aboutHome.css: #searchLogoContainer[hidden] { display: none; } (This is better than the alternate approach of waiting until this function call to make #searchLogoContainer appear, because we want to minimize visual flicker for the common case: Google.) ::: browser/base/content/browser.js @@ +2639,5 @@ > + // At this point we simply reload about:home to reflect the change. > + let searchObserver = { > + observe: function(aEngine, aTopic, aVerb) { > + if (aTopic == "browser-search-engine-modified") { > + doc.location.href = doc.location.href; doc.location.reload(); @@ +2646,5 @@ > + } > + Services.obs.addObserver(searchObserver, "browser-search-engine-modified", false); > + > + // Remove the observer when the page is reloaded or closed. > + doc.defaultView.top.addEventListener("unload", function() { Remove .top here.

(In reply to Mike de Boer [:mikedeboer] from comment #53) > Aaaand *almost* obligatory screencast: http://www.screenr.com/Oob7 Perfect! Love the about:home reload attention to detail too. Not a necessity, but can we also add the name of the search engine as a placeholder="" element on the search input on about:home for non-Google search engines? That way, it's a bit more obvious which engine is being used (and we sidestep the logo issue for non-standard engines, at least for the time being). Very excited to see such a great simplification of how search works!

Comment on attachment 728234 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home I guess we can do something like: if (searchEngineInfo.image) { ... } else { searchText.placeholder = searchEngineName; } The button already has the label "Search", so we don't need to include that in the placeholder.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #55) > Very excited to see such a great simplification of how search works! Wanted to second what Alex said. This simplification is fantastic! It also empowers our users to recover on their own from search hijacking. Exciting stuff!

Comment on attachment 728475 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 728475 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! :) ::: browser/base/content/browser.js @@ +2648,5 @@ > + > + // Remove the observer when the page is reloaded or closed. > + doc.defaultView.addEventListener("unload", function() { > + Services.obs.removeObserver(searchObserver, "browser-search-engine-modified"); > + }, false); Adding unload handlers causes the browser not to cache the page: See here: https://developer.mozilla.org/en-US/docs/Using_Firefox_1.5_caching We should replace this with the following: doc.addEventListener("pagehide", function removeObserver() { doc.addEventListener("pagehide", removeObserver); Services.obs.removeObserver(searchObserver, "browser-search-engine-modified"); }); To make sure that we pick up a search engine switch if the user clicks back to return to about:home, we need to fix the other onPageHide handler in browser.js to remove the hasBrowserHandlers attribute: aBrowser.addEventListener("pagehide", function onPageHide(event) { if (event.target.defaultView.frameElement) return; event.target.documentElement.removeAttribute("hasBrowserHandlers"); aBrowser.removeEventListener("click", BrowserOnClick, true); aBrowser.removeEventListener("pagehide", onPageHide, true); }, true); ::: browser/components/search/content/search.xml @@ +499,5 @@ > // suggest URI, if different) to reduce request latency > > const SUGGEST_TYPE = "application/x-suggestions+json"; > var engine = this.currentEngine; > + var connector = I think this tabs vs. spaces fix (and the `suggestUI` one below) is already in the tree. Could you verify that by updating your local repo, and, if so, remove these changes? ::: browser/modules/AboutHomeUtils.jsm @@ +32,5 @@ > + return Object.freeze({ > + name: defaultEngine.name, > + searchURL: submission.uri.spec > + }); > + } Why did you change the lazy getter to a regular one?

> doc.addEventListener("pagehide", function removeObserver() { > doc.addEventListener("pagehide", removeObserver); > Services.obs.removeObserver(searchObserver, > "browser-search-engine-modified"); > }); Whoops, that second line should read: doc.removeEventListener("pagehide", removeObserver);

Comment on attachment 729460 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 729460 [details] [diff] [review]: ----------------------------------------------------------------- why is it necessary to do all of these changes to aboutHome.js? Apart the unneeded changes to the coding style, changing the mutation code looks risky, especially if the gain is just to allow switching the homepage engine on-the-fly. The needed added protection to loadSnippets shows how this may make the code path more confusing, plus it's likely to make browser_aboutHome test even more flaky than it is. Imo it would be better to not do the on-the-fly change, rather accept the page is static, next instance will use the new engine. Or, if this is really needed, I'd add a separate mutation observer for the specific search-engine-changed.

Marco, thanks for the review! I will add another observer in that case and will also run by all the tests again, just in case.

The caching behavior of the browser indeed introduces many edge cases and complex flows (not only here) that may seem tedious, but they are there, extending the lifetime of about: pages as well. I think (hope) this was considered before a more aggressive page-cache was introduced in Fx 1.5 and therefore we need to try our best to take it into account, even the little details.

(In reply to Mike de Boer [:mikedeboer] from comment #51) > I'm afraid you're wrong about the level of support for those images in the > opensearch spec: > http://www.opensearch.org/Specifications/OpenSearch/1.1#The_.22Image. > 22_element > It's most likely the case that search providers only provide 16x16 (ICO) and > 64x64 (PNG) sizes, as they are mentioned in the spec, but there are no > restrictions listed. I meant "OpenSearch in practice". Essentially no one actually specifies non-16x16 images, as you mention, because no one uses them. Somewhat of a chicken/egg problem.

Implemented separate MutationObserver for search engine changes - to avoid breaking snippets et al. browser_aboutHome.js mochitest still passes.

no reload necessary anymore.

Comment on attachment 729709 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 729709 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, for no reloading. :D r-, because: - This doesn't work if you navigate away from about:home (via a snippets link or search submission) and then press back. Using pageshow/pagehide, as I suggest below, fixes this. - So many observers make Frank sad. :( ::: browser/base/content/abouthome/aboutHome.js @@ +87,5 @@ > gObserver.disconnect(); > setupSearchEngine(); > ensureSnippetsMapThen(loadSnippets); > + > + watchSearchChanges(); In the future, please don't include in your patches changes to the coding style to blocks of code that you're not otherwise modifying. While I too like to clean up inconsistencies in our code, it causes extra bit-rot (merging difficulties for other developers working on the same code). If your text editor is doing that automatically, please disable that when making patches. In fact, you added a line of empty whitespace here, which is outside our coding style. ;) @@ +93,5 @@ > } > } > }); > > +window.addEventListener("load", function() { Replace "load" with "pageshow". @@ +208,5 @@ > + if (searchEngineInfo && searchEngineInfo.image) { > + logoElt.parentNode.hidden = false; > + logoElt.src = searchEngineInfo.image; > +#ifdef XP_MACOSX > + if (searchEngineInfo.imageHD && window.matchMedia("(min-resolution: 2dppx)")) I wrote this line months ago, and I just realized that I had made a typo. Could you change this to: if (searchEngineInfo.imageHD && !window.matchMedia("(max-resolution: 1dppx)").matches) @@ +224,5 @@ > +function watchSearchChanges() { > + window.gObserver = new MutationObserver(function(mutations) { > + for (let mutation of mutations) { > + if (mutation.attributeName == "searchEngineURL") { > + setupSearchEngine(); I really don't like this second observer that almost does the same thing as the original one. I disagree with Marco on the point about the test flakiness. The most important thing is that what the user experiences is good, not what our test machines experience. Also, I don't see how having two observers is advantageous from a test perspective. Looking at the patch a few iterations ago in which you added a boolean check for skipping the snippet part of the original observer higher up in this file: I think that that was the right approach, except that the boolean check could be moved into the observer handler itself. Something like: if (!snippetsShown) ensureSnippetsMapThen(loadSnippets); snippetsShown = true; Please re-combine the two observers and do the above. It'll be the same logic and easier to follow. Sorry for the back-and-forth. @@ +231,5 @@ > + } > + }); > + window.gObserver.observe(document.documentElement, { attributes: true }); > + > + window.addEventListener("unload", function() { Please convert this into the same pagehide pattern that is in browser.js.

Comment on attachment 729778 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 729778 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me! \o/ I did a bunch of manual race-condition testing on it and didn't find any problems. Please verify that this passes all mochitests (either locally or Tryserver) before pushing to mozilla-inbound. If you don't yet have commit access to inbound, let me know. Thanks!

Comment on attachment 729778 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Please also wait for me to play with this for a bit, and sign-off on the UX and implementation overall (but please go ahead and upload updated patches as needed).

Comment on attachment 729778 [details] [diff] [review] Part 3: changing currentEngine also changes defaultEngine, including about:home Review of attachment 729778 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/abouthome/aboutHome.js @@ +212,5 @@ > + if (searchEngineInfo && searchEngineInfo.image) { > + logoElt.parentNode.hidden = false; > + logoElt.src = searchEngineInfo.image; > +#ifdef XP_MACOSX > + if (searchEngineInfo.imageHD && !window.matchMedia("(min-resolution: 1dppx)").matches) As I wrote in an earlier comment, this should be `max-resolution`, not `min-resolution`. Please fix locally before pushing. No need to upload a new version just for this.

I keep forgetting that Mike doesn't have lvl. 3 commit access yet. Plus, this still needs superreview, so uploading a new version. The only changes I made: * s/min-resolution/max-resolution/ * use imperative mood in commit message

Try run with all 3 parts looks green: https://tbpl.mozilla.org/?tree=Try&rev=f0362519e761

removed coding-style changes. carrying over r+, ui-r+ and sr?

changed proper uuid and bail out settings properties when they don't change.

min -> max. carrying over r+=fryn, ui-r+=limi, sr?

added tests for defaultEngine getter/ setter and improved setter bail outs

forgot to add second test engine

(In reply to Frank Yan (:fryn) from comment #69) > I disagree with Marco on the point about the test flakiness. The most > important thing is that what the user experiences is good, not what our test > machines experience. Also, I don't see how having two observers is > advantageous from a test perspective. My point about the test was only a partial reason. The mutation observer is where all of the initialization happens, having an observer that may potentially trigger multiple times the initialization code for all of the page's life is error-prone. For example just take a look at the patch in Bug 819493 and notice gSnippetsShown is not set in the about:rights case, thus the gSnippetsShown protection would be broken already, and regardless gSnippetsShown happens asynchronously when xhr returns, thus in that time we may still strigger loadSnippets multiple times. May I just ask for better protection here (dedicated global maybe?) Regarding the change from "load" to "pageshow", it's interesting that it still allows the browserOnAboutPageLoad to run after it, I would have expected the opposite, I wonder if this order is reliable though (browserOnAboutPageLoad runs on onStateChange with STATE_STOP and must always run after the code in about:home that adds the mutation observer).

Comment on attachment 730768 [details] [diff] [review] Part 3: make changing currentEngine also change defaultEngine, including for about:home Review of attachment 730768 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marco Bonardo [:mak] from comment #81) I'll defer that decision to Gavin. If we don't fully understand the potential paths the code might take, we should figure them out before trying to paper over problems with more complexity. I haven't found any timing problems with using pageshow in place of load. ::: browser/base/content/abouthome/aboutHome.js @@ -205,5 @@ > - logoElt.src = searchEngineInfo.image; > -#ifdef XP_MACOSX > - if (searchEngineInfo.imageHD && window.matchMedia("(min-resolution: 2dppx)")) > - logoElt.src = searchEngineInfo.imageHD; > -#endif We just landed some changes on m-i that remove this `#ifdef` block altogether, so you will need to update this patch again (after the next m-c merge), making sure that you don't re-add this `#ifdef` block accidentally. Sorry for the inconvenience, Mike.

un-bitrotted patch. carrying over r+, ui-r+ and sr?

(In reply to Frank Yan (:fryn) from comment #82) > I'll defer that decision to Gavin. > If we don't fully understand the potential paths the code might take, we > should figure them out before trying to paper over problems with more > complexity. A dedicated global var tracking initialization would not be more complex than reusing another global var that is tracking something else. > I haven't found any timing problems with using pageshow in place of load. That's good but not enough sanity confirmation. In miss of a proof that events will always flow the same order, the home page may randomly be broken if in some cases STATE_STOP should arrive before pageshow. I honestly don't know if this may happen, but since we are moving towards more parallelism sounds plausible. While I'm not willing to block this change, maybe we should brainstorm a bit (followup?) on the possibility to make browserOnAboutPageLoad use a common event listener than the tabs progress listener, so that we can rely on events order.

Comment on attachment 732758 [details] [diff] [review] Part 1: replace originalDefaultEngine with defaultEngine and make defaultEngine a settable >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js >+ set defaultEngine(val) { >+ notifyAction(this._defaultEngine, SEARCH_ENGINE_CHANGED); Hrm, I think this should probably be a different notification (similar to SEARCH_ENGINE_CURRENT) - the engine itself isn't changing (i.e. "defaultness" isn't a state on the nsISearchEngine), so it doesn't seem right to re-use that topic. I think one thing that isn't addressed by this patch is what happens when the default engine is hidden. The current engine deals with this because the getter checks the "hidden" state, but the defaultEngine getter in this patch doesn't do that. The simplest way to fix is probably to do a similar check in the default engine getter. (Perhaps ideally the "hidden" setter on engines would notify the search service to clear its cache of _defaultEngine, but that's probably a bit more complicated to do.) Can you add an xpcshell test to toolkit/components/search/tests/xpcshell/ that simply tests the defaultEngine getter/setter behavior (including when hiding/removing the default engine)? r- for the hiding issue, but this otherwise looks good with these comments addressed.

* defaultEngine getter/ setter now aware of hidden engines * defaultEngine setter emits correct event subject now * test updated

See comment 86 for what changed in the patch for Part 1.

* removed about:home live-updating; it will move to another bug (together with a patch)

* removed too much in the previous iteration :S

Comment on attachment 735463 [details] [diff] [review] Part 1: replace originalDefaultEngine with defaultEngine and make defaultEngine a settable >diff --git a/toolkit/components/search/tests/xpcshell/test_engineselect.js b/toolkit/components/search/tests/xpcshell/test_engineselect.js >+function run_test() { >+ do_load_manifest("data/chrome.manifest"); This shouldn't be needed for this test. r=me!

Comment on attachment 735478 [details] [diff] [review] Part 3: make changing currentEngine also change defaultEngine >diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js > function setupSearchEngine() >+ if (searchEngineInfo && searchEngineInfo.image) { >+ searchText.placeholder = ""; >+ } >+ else { >+ searchText.placeholder = searchEngineName; Why does the placeholder value depend on the presence of an image? >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml >- if (newIndex >= 0 && newIndex < this.engines.length) >- this.currentEngine = this.engines[newIndex]; >+ if (newIndex >= 0 && newIndex < this.engines.length) { >+ this.defaultEngine = this.currentEngine = this.engines[newIndex]; >- this.currentEngine = target.engine; >+ this.defaultEngine = this.currentEngine = target.engine; Hrm, these changes look unnecessary, given the change to the currentEngine setter. "this.defaultEngine" isn't anything useful in this context.

removed manifest load. carrying over r+

removed extraneous this.defaultEngine sets. carrying over r+=fryn, ui-r+=shorlander

Comment on attachment 735497 [details] [diff] [review] Part 3: make changing currentEngine also change defaultEngine It's probably worth adding a comment to the currentEngine setter explaining why we set both searchService.defaultEngine and searchService.currentEngine. I'm still not convinced this UX model is right, overall. Users who use the "temporary search" feature of the search bar will likely be confused, though at least it should be relatively obvious what's happening. Maybe that set of users is small enough for this not to matter, but I'm skeptical. But let's give it a try out on trunk, and see how it fares on the trains.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c55e92d87a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/8998ff84caef https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6dd0cc8053

Mike, we don't close bugs as RESOLVED FIXED until they land on mozilla-central. :)

I am truly sorry for the bug spam. :( On the bright side, I can't wait to close all the dependent bugs once this lands on mozilla-central!

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a31fc4429a as a followup, because someone decided that aboutHome.js didn't need to be preprocessed, right before you gave it a reason to be preprocessed, so browser-chrome didn't much like seeing that unprocessed #ifdef line. That's the good part. Then I had to back out bug 819493, and this was on top of it, so I backed that followup out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad1b71d0c32 and backed your three out in https://hg.mozilla.org/integration/mozilla-inbound/rev/278ea1d0e072.

Re-pushed standalone, with the issue from comment 99 fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/cc569e87074c https://hg.mozilla.org/integration/mozilla-inbound/rev/ea44f501b0c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/621656e9b09d

https://hg.mozilla.org/mozilla-central/rev/cc569e87074c https://hg.mozilla.org/mozilla-central/rev/ea44f501b0c5 https://hg.mozilla.org/mozilla-central/rev/621656e9b09d

This was incorrectly merged due to conflicting patches from bug 818940 and bug 859576. Since the patches for all three of these bugs were reviewed by me, I took the responsibility to fix the merge and pushed the following changeset to fix that: https://hg.mozilla.org/integration/mozilla-inbound/rev/d210097816a1 If this needs to be backed out of any branch for whatever reason (and I hope it won't), you will need to back out this changeset first.

I searched here ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/, but I wasn't able to find the build with the changeset specified in comment 102, d210097816a1. I looked through all folders from 2013-04-09 untill 2013-04-11. I've done a little exploratory on a Windows 7 64-bit machime with the inbound build that has 621656e9b09d as changeset, and the search seems to work as expected, after changing the search engine. On the other hand, with the Nightly from 2013-04-10, the changes made don't seem to work. Google stays the search engine, even after I choose a distinct one from the drop-down near the search bar.

Can we just get rid of either the defaultengine or selectedEngine since they both do the same thing now? This patch still has the problem that if defaultenginename is set to one thing, and the selected engine is set to another, you'll get keyword searches that are not the same as the search in the upper right. I know you'll say "that's just an implementation detail, defaultenginename won't get changed", but what you've provided here is an easy way for an extension to take keyword search without taking over main search. They just set defaultenginename to their search.

(In reply to Michael Kaply (mkaply) from comment #104) > Can we just get rid of either the defaultengine or selectedEngine since they > both do the same thing now? ...in desktop Firefox, for the moment. I'm not convinced we'll stick with this interaction model, so we decided not to hard-code its constraints in the search service back-end. (There are also other consumers of the search service who might choose not to make the same UI choice.) > I know you'll say "that's just an implementation detail, defaultenginename > won't get changed", but what you've provided here is an easy way for an > extension to take keyword search without taking over main search. They just > set defaultenginename to their search. Good point, we'll need to address that. I'll reopen your bug 860560!

*keyword.enabled;false* set by default wasn't good enough ?

That has different effects (disabled searching from the location bar entirely), and could also easily be subverted by add-ons.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #107) > That has different effects (disabled searching from the location bar > entirely), and could also easily be subverted by add-ons. If the aim truly is protecting casual users from having their address bar hijacked (which -BTW- I frankly don't see can ever be accomplished,with all the crapware floating around the web you'll probably end up disabling/removing almost everything),then why not implementing some sort of control that scans the .xpi file and when it reckons that it is going to modify the keyworld.url parameter, gives a clear,unequivocal warning to the user,in the form of a pop up message telling something along the lines of "This addon/plugin/toolbar/whatever wants to modify your default search engine,do you want to allow it ?" You'll agree that it could be miles better than taking away a core feature of Firefox that us more or less "experienced" users have been using and relying on for ages now...

(In reply to msth67 from comment #109) > If the aim truly is protecting casual users from having their address bar > hijacked (which -BTW- I frankly don't see can ever be accomplished,with all > the crapware floating around the web you'll probably end up > disabling/removing almost everything) This is the "it's impossible to defend against 100%, and so let's not even try" argument, which doesn't hold much weight in my book. Tying the preferences used by Firefox to something the user can directly and obviously control raises the bar for hijacking searches from "change a hidden pref" to "actively counteract the users abilities/intentions", which is a significant enough difference that it will have real practical benefits for users. > control that scans the .xpi file and when it reckons that it is going to > modify the keyworld.url parameter, gives a clear,unequivocal warning to the > user,in the form of a pop up message telling something along the lines of > "This addon/plugin/toolbar/whatever wants to modify your default search > engine,do you want to allow it ?" We do plan do investigate something like that in bug 852152. Simplifying and exposing our search preferences (this bug) was a precursor. > You'll agree that it could be miles better than taking away a core feature > of Firefox that us more or less "experienced" users have been using and > relying on for ages now... keyword.URL customization is not a "core feature of Firefox". An incredibly small minority of our users benefited from it, and many users were being harmed by it, so from my perspective our decision makes a lot of sense. Thankfully, the more "experienced" users have a couple of options: 1) install a custom OpenSearch search engine and use that as your default/selected engine 2) use an add-on that re-implements similar functionality (ability to customize location bar search behavior separately from search bar behavior) Now that this bug is resolved, the better venue for further discussion is the firefox-dev mailing list: https://wiki.mozilla.org/Firefox/firefox-dev

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #110) > > > keyword.URL customization is not a "core feature of Firefox". An incredibly > small minority of our users benefited from it(...) Sorry,are you sure about this ? http://kb.mozillazine.org/Location_Bar_Search https://support.mozilla.org/en-US/kb/awesome-bar-find-your-bookmarks-history-and-tabs?redirectlocale=en-US&redirectslug=search-your-bookmarks-history-and-tabs-awesome-bar

It took me some time to find this bug. This fix seems to break my locationbar search for several search engines. While it works with bing, google, imdb and a lot more, it stopped working for some like DuckDuckGo.com or Startpage.com: Go to DuckDuckGo.com open the searchbar list and add this search engine type something in the location bar -> error Is this an intended behavior (e.g. is there a whitelist for search engines that are allowed to be used in locationbar?) or should I open a new bug?

I found the reason and opened bug 862401

(In reply to msth67 from comment #111) > Sorry,are you sure about this ? > > http://kb.mozillazine.org/Location_Bar_Search > > https://support.mozilla.org/en-US/kb/awesome-bar-find-your-bookmarks-history- > and-tabs?redirectlocale=en-US&redirectslug=search-your-bookmarks-history-and- > tabs-awesome-bar Yes. Firefox has hundreds of millions of users, and the vast majority of them have never used about:config. (That second article makes no mention of keyword.URL, as far as I can tell, but there are other articles on SUMO that do - adding user-doc-needed to get those cleaned up.)

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #114) > > > (That second article makes no mention of keyword.URL, as far as I can tell, > but there are other articles on SUMO that do - adding user-doc-needed to get > those cleaned up.) In the second article there's this link https://support.mozilla.org/en-US/kb/search-web-address-bar,that I suspect even this mythical "average" user -whose hypothetical inability to understand even the most basic usage of a web browser you are trying to accommodate- may eventually click on.

I'm not claiming that no one uses this feature. I'm quite aware that many people do, and I expect many of them will be upset by this change. But we have data and anecdotal evidence that there are many more people who will appreciate that their location bar searches are now more difficult to hijack, and will appreciate a more intuitive way to control how they search. This was a difficult but necessary tradeoff.

So how do the people (ME) go about re-adding this useful feature that you have now removed because people were incapable of turning on their firewalls? (presume that's what you're talking about with "hijacking" searches.)

Power users who wish to customize the URL used for keyword search can define a custom OpenSearch plugin as their default, and have that use a <URL> of type application/x-moz-keywordsearch (bug 865218). This bug isn't the right place to discuss that further, feel free to ask on IRC or email if you need more information. (The "hijacking" I'm referring to had nothing to do with firewalls.)

relnoted (on Aurora) as "Firefox search touchpoints have been consolidated behind a single search default preference. Switching to a different search in the search box will now change the setting for the addressbar search and the context search for selected content. "

Gavin I tried the solution of adding a search plugin for browse by name and it becomes a major pain to have to always switch back and forth from browse by name to another search engine all the time. And there is still no addon to add back the functionality. Example: a lot of times i want to use browse by name but many other times i want to use the search bar to do a normal google search i do this all day long. I thought of a good solution that could please everyone while keeping viruses from hijacking keyword.url. Add back keyword.url but hardcode it so it can't be changed but from a few selections like Google's feeling lucky or browse by name. Most people who use keyword.url only use it for them two services. I would file a separate bug just for this but i wouldn't know how to file this particular bug.

(In reply to Derek from comment #120) > Gavin I tried the solution of adding a search plugin for browse by name and > it becomes a major pain to have to always switch back and forth from browse > by name to another search engine all the time. And there is still no addon > to add back the functionality. > > Example: a lot of times i want to use browse by name but many other times i > want to use the search bar to do a normal google search i do this all day > long. You can solve this problem by adding an additional Google engine that is a copy of the default one, but with the x-moz-keywordsearch <Url> added. It will behave the same for normal searches, but have a custom keyword behavior. You'll just have to give it a slightly modified name (so it doesn't conflict with the default "Google" engine), and then remove the default Google engine and use your custom one instead.

Use "Browse by Name" Google search which replicates the Keyword.URL "smart address" function that is now lost in Versions 23+ Simply add to and select as your "search preference" http://mycroftproject.com/search-engines.html?name=Google+%28Browse+by+Name%29 Uses the URL of keyword.url that gave "intelligent" address search

(In reply to seann55555 from comment #122) > Use "Browse by Name" Google search which replicates the Keyword.URL "smart > address" function that is now lost in Versions 23+ > > Simply add to and select as your "search preference" > > http://mycroftproject.com/search-engines. > html?name=Google+%28Browse+by+Name%29 > > > Uses the URL of keyword.url that gave "intelligent" address search That does not work, installing a search plugin and setting it to default doesn't change my url bar searches. I tried the mycroftproject version and wrote my own, neither affect the url bar behaviour in FF24.

(In reply to Cam from comment #123) > That does not work, installing a search plugin and setting it to default > doesn't change my url bar searches. > > I tried the mycroftproject version and wrote my own, neither affect the url > bar behaviour in FF24. Cam, could you file a separate bug and CC me? We should look into this further.

(In reply to Derek from comment #120) > it becomes a major pain to have to always switch back and forth from browse > by name to another search engine all the time. It's already a pain with the current/"old" UI. The "Search Tab" proposal that has been presented as a concept quite some time ago would help there, but that would be a completely different bug than what is being discussed here.

Verified as fixed on: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130620 Firefox/23.0 Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130620 Firefox/24.0 keyword.URL and originalDefaultEngine have been removed. When setting an engine in the Search bar, it is also set as the search engine for the URL bar and about:home. (Firefox 23 needs a restart for this to happen. Restart not necessary on Firefox 24.) Searches from all three places work fine. I tested this with the engines Firefox comes with and with engines installed by the user. The only problems I noticed here are when the user edits browser.search.defaultenginenam: - bug 885351 - Select a search engine in the Search bar manually (e.g. Amazon). Go to about:config and set the pref to another engine (e.g. Yahoo). Restart Firefox. The URL bar and about:home use the newly set engine (Yahoo), but the Search bar keeps the last engine manually selected for it (Amazon). This issue is not reproducible on Firefox 24. Are there any chances it gets fixed on 23? Should I file a bug for it?

Sorry I ignored the contextual menu option in my above comment. Here how that works: When selecting and engine in the Search bar, that engine is also used for the Search contextual menu option. It works fine with user-installed engines too. Regarding the two issues, the contextual menu uses the Search bar engine in both cases.

(In reply to Ioana Budnar, QA [:ioana] from comment #126) > Verified as fixed on: > Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130620 Firefox/23.0 > Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130620 Firefox/24.0 > > keyword.URL and originalDefaultEngine have been removed. When setting an > engine in the Search bar, it is also set as the search engine for the URL > bar and about:home. (Firefox 23 needs a restart for this to happen. Restart > not necessary on Firefox 24.) Searches from all three places work fine. I > tested this with the engines Firefox comes with and with engines installed > by the user. You don't really need to restart firefox for this work, you can also reload the about:home page or close/ open the about:home tab (I think reloading takes the least amount of effort ;) ) > - Select a search engine in the Search bar manually (e.g. Amazon). Go to > about:config and set the pref to another engine (e.g. Yahoo). Restart > Firefox. The URL bar and about:home use the newly set engine (Yahoo), but > the Search bar keeps the last engine manually selected for it (Amazon). This > issue is not reproducible on Firefox 24. Are there any chances it gets fixed > on 23? Should I file a bug for it? This issue is indeed fixed in 24 - the work is tracked in bug 860560. Gavin already requested approval for Aurora, so you don't need to file a bug for this one... Thanks for verifying that all this behaves as expected! :)

Thanks for the feedback Mike :) Indeed I mixed up the two ways of changing the engines there. I don't need a restart when setting the engine in the search bar, I need it when setting it in about:config.

Thanks for the feedback Mike :) Indeed I mixed up the two ways of changing the engines there. I don't need a restart when setting the engine in the search bar, I need it when setting it in about:config.

I don't understand. So is there a way to search in location bar using Google only?

Just noticed this terrible change. Could we at least have an option to restore the ability to have a custom search bar distinct from Google in the URL bar? All this has achieved is to make us have to constantly change search preference every time we want to use a real name (Google search) shortcut as a URL. I'd be surprised if I'm the only user who hates it.

(In reply to Alby Dangles from comment #132) > Just noticed this terrible change. Could we at least have an option to > restore the ability to have a custom search bar distinct from Google in the > URL bar? > > All this has achieved is to make us have to constantly change search > preference every time we want to use a real name (Google search) shortcut as > a URL. > > I'd be surprised if I'm the only user who hates it. I have set currentengine to those that I don't use quite often.But I do use google to search for the most time(Drag&Drop texts,URL bar search,etc.). This patch forces me to change currentengine in search bar very frequently(everytime I want to use another search engine),because now defaultengine sync with currentengine and there is no option to disable it.

I'd suggest using keywords for non-default search engines. Does that work ?

Using keywords for Google and search field for Wikipedia might work for you and some people, but not others (e.g. behind a proxy, where keywords only work when there are 2 words). The current UI (even before this change) was intended for Google and co and not very good for non-primary-search engines like youtube, wikipedia, amazon etc. This is a UI problem. I filed bug 889113 to fix that.

H(In reply to Julien Wajsberg [:julienw] from comment #134) > I'd suggest using keywords for non-default search engines. Does that work ? Haven't keyword.url be cleaned up to null by default?So how can I get google's keywords url? (In reply to Ben Bucksch (:BenB) from comment #135) > Using keywords for Google and search field for Wikipedia might work for you > and some people, but not others (e.g. behind a proxy, where keywords only > work when there are 2 words). The current UI (even before this change) was > intended for Google and co and not very good for non-primary-search engines > like youtube, wikipedia, amazon etc. This is a UI problem. > I filed bug 889113 to fix that. So if I have more engine installed by "add to search bar" rather than wikipedia.How could I use those engine? as bug 889113 you have just suggest only 2 search engines,Google and Wikipedia.If I have more than 20,how could I choose from what I want to use?

(In reply to celestia_jenova from comment #133) > (In reply to Alby Dangles from comment #132) > > Just noticed this terrible change. Could we at least have an option to > > restore the ability to have a custom search bar distinct from Google in the > > URL bar? > > > > All this has achieved is to make us have to constantly change search > > preference every time we want to use a real name (Google search) shortcut as > > a URL. > > > > I'd be surprised if I'm the only user who hates it. > > I have set currentengine to those that I don't use quite often.But I do use > google to search for the most time(Drag&Drop texts,URL bar search,etc.). > > This patch forces me to change currentengine in search bar very > frequently(everytime I want to use another search engine),because now > defaultengine sync with currentengine and there is no option to disable it. Same here. I frequently use the location bar as I'm used to and am frustrated when some other page opens. I explicitly want to have an option to search different pages on the right when needed but when I want to google something, I just enter it in the location bar. Can you at least provide a patch to disable the sync between defaultengine and currentengine for advanced users via about:config?

> I ... want to have an option to search different [engines] when needed Again, that would be solved by bug 889113. This would list all installed search engines in the autocomplete dropdown after you type, so you can select the right engine for each query, by simply using the "arrow down" key after typing your search term.

Following my comment 134, you can assign keyword to search engines: * click on the arrow on the left of the search box * choose "manage search engines" * from there you can assign keywords to your favorite search engines Then in the address (awesome) bar you can use "<keyword> search query".

(In reply to Ben Bucksch (:BenB) from comment #138) > > I ... want to have an option to search different [engines] when needed > > Again, that would be solved by bug 889113. > > This would list all installed search engines in the autocomplete dropdown > after you type, so you can select the right engine for each query, by simply > using the "arrow down" key after typing your search term. You have ingored many many factors,for example the suggest auto-complete url.If I've visited news.young.com news.old.com news.pc.bad.com news.any.com ,maybe more urls contain "news".And I have more than 10 search engines. If I type "news" in url bar,haven't you imagined what the hell will the list be after bug 889113? It will show firstly more than 4 of the auto-complete urls.then more than 10 of the suggest search terms.This doesn't work as what you have said 'by simply using the "arrow down" key.' (In reply to Julien Wajsberg [:julienw] from comment #139) > Following my comment 134, you can assign keyword to search engines: > * click on the arrow on the left of the search box > * choose "manage search engines" > * from there you can assign keywords to your favorite search engines > > Then in the address (awesome) bar you can use "<keyword> search query". I have another case here is the drag&drop search.Some people use extensions as me,some people use Greasemonkey scripts or userChrome scripts.I used to visit foreign web sites,it's easy to do a search just by drag&drog. Before Firefox 22,it will use google by default.But in 23 it will use the currentengine since defaultengine is sync with currentengine.I'll need to change search engine back to Google before I do a drag&drop search.And then change to what I want again to do another search. Plus Comment 134,your suggestion can't solve my problem completely.If all these happens very often,you will know why I'm asking an option.

I always thought that the entire purpose of the search box was to allow for a separate search engine to be used in addition to the standard search of the address bar. I couldn't think of any reason to have two boxes available for searching (sitting right next to one another, no less) if they didn't perform different functions. This change renders the existence of the search box entirely moot, since entering a query in the search box now acts identically to typing it in the address bar. If changing the search engine in the search box really just sets the search engine for the whole browser now, wouldn't it make more sense to move the search engine selection dropdown over to the address bar (or into a settings dialog, as others have suggested) and remove the search box entirely? I recognize the worries about search hijacking, but I think that the current solution is likely to cause just as much confusion as it helps avoid. Most users have grown used to having the search engines in the address bar and search box decoupled, and the search engine dropdown is visually "attached" to the search box, giving the impression that the two are connected, while there's no visual cue to tell the user that their choice will also impact the address bar. Add to this the fact that there's no explanation to the end-user for why the search box and address bar are suddenly linked (I honestly thought it was a bug with the version 23 betas before I googled the issue) and you have a recipe for a lot of flummoxed users when version 23 rolls out to the release channel. If the search box went away and the search engine dropdown were visually associated with the address bar, I think users would at least get a better sense of the change that occurred, as opposed to assuming something was broken when the browser simply stopped behaving the way it has for all past versions. I would throw my lot in with those who argue this change is removing valuable functionality from the browser, but, since it's being implemented in the next release, please make the new functionality of the search engine selection clearer to end users.

> I always thought that the entire purpose ... standard search of the address bar. The purpose of the urlbar/address bar is to enter an URL. Not a search. keyword search is a bolt-on that sometimes works and sometimes doesn't. It's not always clear for the browser to differentiate. > entering a query in the search box now acts identically to typing it in the address bar. No, it doesn't: 1. Because I use a proxy, a keyword search for "hello" gets me a "no such domain" error, whereas "hello world" works. See screenshot. But even for non-proxy users: 2. The search is slower than it needs to be, because the browser first needs to do an expensive and useless DNS lookup for your search word, if it's a single word, because he can't know whether you mean a hostname or not. That also leaks your search to the open Internet, bypassing https of Google (as little as that is worth). So, while keyword search and real search are close, they are not the same. Esp. when you're being picky about details like privacy, proxies etc..

A quick tip for all those power users here who want to use an additional search engine (this actually allows you to use an arbitrary number): Use bookmark keywords. 1. Go to e.g. http://www.youtube.com 2. In a search box, make a right click 3. Click "Add a keyword for this search..." 4. Enter as name e.g. "YouTube search (keyword youtube)" and keyword "yt", and Save 5. In the urlbar, enter "yt Rick Astley" 6. You have been rickrolled. I do this for a lot of stuff, including wikipedia (in different languages), youtube, word dictionaries etc.

(In reply to Ben Bucksch (:BenB) from comment #142) > 2. The search is slower than it needs to be, because the browser first needs > to do an expensive and useless DNS lookup for your search word, if it's a > single word, because he can't know whether you mean a hostname or not. That > also leaks your search to the open Internet, bypassing https of Google (as > little as that is worth). > > So, while keyword search and real search are close, they are not the same. > Esp. when you're being picky about details like privacy, proxies etc.. That doesn't address the problem that most people are used to searching by typing it into the address bar. If this behavior is changed so radically, then there should at least be an addition to always search Google (or other favorite SE) like Firefox Mobile or Chrome do it.

(In reply to Ben Bucksch (:BenB) from comment #142) > > entering a query in the search box now acts identically to typing it in the address bar. > > No, it doesn't: > 1. Because I use a proxy, a keyword search for "hello" gets me a "no such > domain" error, whereas "hello world" works. See screenshot. But even for > non-proxy users: > 2. The search is slower than it needs to be, because the browser first needs > to do an expensive and useless DNS lookup for your search word, if it's a > single word, because he can't know whether you mean a hostname or not. That > also leaks your search to the open Internet, bypassing https of Google (as > little as that is worth). Both very strong points. I'm so in the habit of typing "g <query>" (g being my keyword for a google search) in the address bar for url-like (e.g., "example.pdf") searches in the address bar that I forgot they would fail outright if typed directly (since Firefox would look for a site with a TLD of pdf). Based on your arguments, I agree that there is merit in retaining the search box, since most people don't use the custom keyword searches, but I would still like to see a better way of visually associating the search engine selection with the address bar now that it is impacting both boxes.

(In reply to Eavesdown from comment #141) > I always thought that the entire purpose of the search box was to allow for > a separate search engine to be used in addition to the standard search of > the address bar. I couldn't think of any reason to have two boxes available > for searching (sitting right next to one another, no less) if they didn't > perform different functions. Agreed > I think users would at least get a better sense of the change that occurred, > as opposed to assuming something was broken when the browser simply stopped > behaving the way it has for all past versions. I thought this is a bug too and asked in some forums and then found here. (In reply to SoWhy from comment #144) > (In reply to Ben Bucksch (:BenB) from comment #142) > > 2. The search is slower than it needs to be, because the browser first needs > > to do an expensive and useless DNS lookup for your search word, if it's a > > single word, because he can't know whether you mean a hostname or not. That > > also leaks your search to the open Internet, bypassing https of Google (as > > little as that is worth). > > > > So, while keyword search and real search are close, they are not the same. > > Esp. when you're being picky about details like privacy, proxies etc.. > > That doesn't address the problem that most people are used to searching by > typing it into the address bar. If this behavior is changed so radically, > then there should at least be an addition to always search Google (or other > favorite SE) like Firefox Mobile or Chrome do it. Agreed If "Search bar <query>" and "Address Bar <query>" actually do the same function,I'd like to ask why you guys still keep "Search bar" there. (In reply to Ben Bucksch (:BenB) from comment #143) > A quick tip for all those power users here who want to use an additional > search engine (this actually allows you to use an arbitrary number): Use > bookmark keywords. > 1. Go to e.g. http://www.youtube.com > 2. In a search box, make a right click > 3. Click "Add a keyword for this search..." > 4. Enter as name e.g. "YouTube search (keyword youtube)" and keyword "yt", > and Save > 5. In the urlbar, enter "yt Rick Astley" > 6. You have been rickrolled. > > I do this for a lot of stuff, including wikipedia (in different languages), > youtube, word dictionaries etc. So all users who want to use alternative search engines should use "keyword" ,which don't support drag&drop.Because you guys thought this is more advanced and more intelligent. I used to drag&drop the <query> found in websites to the search bar would search <query> in alternative engine not google,but I drag&drop the other <query> to address bar (using script/extensions to let drag&drop search works even not to address bar)to search in google.And since 23,I could do nothing but change the search engine again and again. Please don't tell me that you guys don't use a drag&drop extensions or scripts or anything like firegestures.

For one thing, all those discussions should go into a mailing list. Bugzilla is not a discussion forum, but a tool to track the status and actual work on issues. The last comments help nothing in clarifying status or doing actual work, and this bug is VERIFIED FIXED anyhow, so any further issue/work would need to go into a different bug report. (In reply to celestia_jenova from comment #146) > If "Search bar <query>" and "Address Bar <query>" actually do the same > function,I'd like to ask why you guys still keep "Search bar" there. They don't "do the same function". When you type in the location bar, the results directly shown in the dropdown list are from local data (history/bookmarks) only, we guarantee that we don't send info about what you type to external services (which is a major difference to Chrome, BTW). When you type in the search bar, we display "search suggestions" in the dropdown list, which are generated by sending what you typed to the respective search engine. Also, having the search engine selector in the location bar would be confusing. If we'd be able to solve those two issues, I'd be the first to call for merging the two bars - but I haven't heard a good suggestion yet to solve those two. I agree that the current solution to the search engine settings isn't ideal, but having one selector that selects it for everywhere is way, way more logical to normal users than the overly confusing stack of settings we had previously. And switching back and forth is a minor inconvenience, really. In my personal opinion, Search Tabs as shown in https://air.mozilla.org/product-design-at-mozilla/ would be a better way to point users to different installed search providers, but they still don't solve the whole problem (unless someone adds some additional ideas to them). Still, all out of scope for comments on this bug here. Please take this to a mailing list.

I must say the behaviour seems very strange now (even if I'm a supporter of removing options). I got bitten twice in a row by this today, even though I know about the new behavior. I would have prefered a "set as default" button in the search engine manager that would set the keyword.URL pref...

After updating to Firefox 23 on my Kindle Fire HD tablet, I noticed that every time I tried to use the address bar to enter search terms, I would get redirected to an error page saying "The address isn't valid" instead of the expected google search results. On Firefox Mobile, there isn't a separate search bar. I used another browser to search for information and eventually found my way here. Have other users experienced problems with this 'fix' for their mobile versions of Firefox? If the keyword.URL setting is no longer operational, how can we use the address bar to perform searches? Or is this more likely an idiosyncratic problem related to my tablet? This wouldn't be the first time that Firefox and the KFFT haven't gotten along well.

DB> this sounds like a real regression, could you please file a new bug ?

My dear Mozilla developers, i real appreciate your efforts about stop change default browser by plugins or some other software but this is wrong way. This is not solutions this is bug, critical bug which blocking major functionality of browser. Best Wishes, Michał

With this bug closed, you need to get rid of the search box. To those claiming that the url search and search box 'do different things', while this is true, the only benefit of the search box is that it provides suggestions. You want to keep a whole box around just for this one feature??? (Also, you can do one word searches in the url by adding a space to the word, eg 'kitty ') If search bar is so important how does chrome manage to live without it? I think the better solution would have been to add a proper UI to control the url search engine. Being able to differentiate between my url search and my default search was one of few reasons I was staying with firefox instead of chrome. Now that this is gone, I will consider switching to chrome, sorry.

FYI, I built an add-on that lets keyword have it's own search engine selection: https://addons.mozilla.org/en-US/firefox/addon/keyword-search/

For future bug visitors (and previous commenters who still miss the feature), this add-on restores keyword.URL (and therefore completely reverts this bug): https://addons.mozilla.org/en-US/firefox/addon/keywordurl-hack/

If keyword.URL was removed in order to prevent search engine "hijacking" by plugins, it may be a good idea to prevent DNS hijacking as well. DNS hijacking is a very annoying practice some DNS providers have. Instead of returning an NXDOMAIN message which tells the browser that the domain doesn't exist, they provide a custom page with search results (and some advertising), similarly to what Firefox already does. As far as I know, Firefox relies on this NXDOMAIN message in order to launch the URL bar search. As a result, if these DNS are used, anything typed on the URL bar will be recognized as a valid domain name, making it impossible to use the URL bar as a search bar. (I'm not sure this still happens because as soon as the DNS provided by my ISP started doing this I switched to Google's.) Apparently, the algorithm Firefox follows is: - If the text entered in the URL box is resolved by the DNS, go to the obtained IP. - Otherwise, if the text was a valid URL, show a "Server not found" message. - Otherwise, use the search engine. Instead, this could be done: - If the text was not a valid URL or IP, use the search engine. - Otherwise, if the text entered in the URL box is resolved by the DNS, go to the obtained IP. - Otherwise, show a "Server not found" message. The latter method would have 2 advantages: (1) It would prevent DNS hijacking to interfere with URL box search. (2) It would allow the browser to immediately know if the user is entering a search query or a URL, thus being able to provide some kind of visual feedback of what will happen when they hit enter (this feedback could be similar to the behavior obtained when a "search keyword" bookmark is typed).

(In reply to cousteau from comment #156) cousteau: we have other bugs tracking ways to address DNS hijacking, like bug 728670 and bug 700470. You should put your suggestions there.

This was not a bug, and did not need fixing. Why have 2 boxes, that lead to a search to the exact same place. STUPID. There was a reason for wanting searches from the address bar to go else where than from the search plugin. Search bar to default search engine, because its a fast short cut to searching for fave search engine. Plugin bar, I kept on wiki, so I could highlight any text on any website, and search wiki about it right away. A clear reason for keeping the 2 different. This is a step backwards for me, and another reason for wanting to dump Firefox. Soon, I will be. Too many bad choices being made by whoever is in charge. Last month, the DPI changes totally messed up how the browser worked, and now this. Whatever will they fu'k up next, in 6 weeks time.

Please add an UI Option to set a default search. At the moment search functionality in Firefox seems broken: Two fields that do the same thing! Either you add the possibility to set a default search or you merge those two fields. I guess you'll do the latter eventually - and I hope you don't mess up again (as you did with FF23). Here's a workaround for people who can't bear this stuff: https://addons.mozilla.org/en-US/firefox/addon/keywordurl-hack/ By the way: I deeply appreciate that you addressed the search hijacking problem. This doesn't mean you did it the right way.

I'm going to restrict comments on this bug to people with editbugs privileges, since the last few comments here have not been constructive. Bugzilla is not the place to lobby for/against changes, especially after the fact. If you have constructive reports of problems that aren't already known, filing a new bug is the best venue. If you have constructive questions, arguments, or proposals for alternative solutions, you can email firefox-dev.