As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1265881 - Location Bar Internet Search does not respect ˋEdit → Preferences → Browser → Internet Searchˊ
: Location Bar Internet Search does not respect ˋEdit → Preferences → Browser ...
Status: RESOLVED FIXED
: regression, reproducible
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: SeaMonkey 2.43 Branch
: Unspecified All
: -- normal with 3 votes (vote)
: seamonkey2.47
Assigned To: Philip Chee
:
:
Mentors:
https://hg.mozilla.org/mozilla-centra...
: 1224839 1287373 1326342 1328827 1329217 1329948 (view as bug list)
Depends on: 1237648
Blocks: 1266642 2.50BulkMalfunctions
  Show dependency treegraph
 
Reported: 2016-04-19 13:27 PDT by Rainer Bielefeld
Modified: 2017-01-19 09:38 PST (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
?
fixed
?
fixed
?
fixed
+
fixed


Attachments
search1.png (62.35 KB, image/png)
2016-08-16 10:54 PDT, Frank-Rainer Grahl
no flags Details
search2.png (19.83 KB, image/png)
2016-08-16 10:54 PDT, Frank-Rainer Grahl
no flags Details
search3.png (5.17 KB, image/png)
2016-08-16 10:55 PDT, Frank-Rainer Grahl
no flags Details
search4.png (17.10 KB, image/png)
2016-08-16 10:56 PDT, Frank-Rainer Grahl
no flags Details
search5-mock.jpg (73.74 KB, image/jpeg)
2016-08-16 10:58 PDT, Frank-Rainer Grahl
no flags Details
Patch 1.0 make every search widget synchronized. (15.16 KB, patch)
2016-12-27 12:13 PST, Philip Chee
iann_bugzilla: review+
frgrahl: feedback+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

User Story
In the old Mozilla Suite (XPFE) internet search implementation there was a pref called "browser.search.defaultenginename". When Firefox moved to using the new Toolkit search, the preference was migrated to the search service ss.defaultEngine property. ref:

[Android] Bug 1238516 - Migrate the defaultenginename preference
https://hg.mozilla.org/mozilla-central/rev/4abbb197ebfe

For some reason when SeaMonkey moved to Toolkit Search the old pref wasn't migrated.

Time moved on.

A mozilla developer decided to make the defaultEngine and currentEngine synchronized in an attempt to reduce end user confusion in the support forums:

Bug 860560: make sure that defaultEngine and currentEngine stay in sync
https://hg.mozilla.org/mozilla-central/rev/9d6db6508757

Time moved on.

A mozilla developer decided to make the readonly defaultEngine read-write allowing users to change their default search engine.

Bug 738818 - consolidate Firefox search preferences
part 1: replace originalDefaultEngine with defaultEngine and make defaultEngine a settable
https://hg.mozilla.org/mozilla-central/rev/cc569e87074c

Time moved on.

A mozilla developer decided to remove the defaultEngine implementation and just made it forward to the current engine, so these two are now always the same.

Bug 1237648 - The defaultEngine attribute should just be an alias to the currentEngine attribute 
Part 1: remove the defaultEngine implementation and forward the getter/setter to currentEngine,

===========================================================================
In this bug we'll just make sure that all search widgets (urlbar search, searchbar, sidebar search) just respect the currentEngine setting and also make them all synchronized.

===========================================================================
Down the line Ratty plans to address in more detail these problems on the mozilla-release52 and mozilla-esr52 (SeaMonkey 2.49) channels by creating a SeaMonkey specific release branch in these repositories. And then backing out changes that we disagree with.      
Description User image Rainer Bielefeld 2016-04-19 13:27:34 PDT
Steps how to reproduce  NOT reproducible REPRODUCIBLE with  English SeaMonkey 2.45a1  (Windows NT 6.1; WOW64; rv:48.0)  Gecko/20100101 Firefox/48.0 Build 20160308001946  (Default Classic Theme)  on German WIN7 64bit:

1. Select Search Enginge in ˋEdit → Preferences → Browser → 
   Internet Searchˊ
2. Quit SeMonkey, relaunch Browser
3. Type a string into (URL-) Location Bar → [Enter]
   Expected: Search Engine from step 1 used
   Actual: Search engine preselected in Sidebar Internet Search will be
           used.

Additional info:
a) Until SM 2.40 I observed that Location Bar Search Engine will
   change to selection due to step 1
b) I have not yet tested with what Version that started
c) Possibly unwanted side effect of fix for ""Bug 1190185 - Switch 
   default search engine to DuckDuckGo""?
Comment 1 User image Rainer Bielefeld 2016-04-19 13:37:11 PDT
d) Same problem for Separate Internet Search Bar
e) It seems  ˋEdit → Preferences → Browser → Internet Searchˊ has influence 
   for nothing?
f) In <nntp://news.aioe.org/de.comm.software.mozilla.browser> 
   "DuckDuckGo drängelt sich vor" user Ralf Lehmeier complains the problem 
    already for SM 2.34
    I will try no narrow down version of appearance later
g) NEW due to confirmations in NG
h) no obvious DUP found with <https://bugzilla.mozilla.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=DUPs1265881&sharer_id=41036&list_id=12973246>
Comment 2 User image Rainer Bielefeld 2016-04-19 13:42:15 PDT
Already REPRODUCIBLE with  SeaMonkey 2.42a1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0 from public download area)  Gecko/20100101  Firefox/ 45.0  Build 20151111021032, (Classic Theme) on German WIN7 64bit
Comment 3 User image Rainer Bielefeld 2016-04-19 22:34:31 PDT
(In reply to Rainer Bielefeld from comment #2)

I did some more research and saw that I made a mistake, the problem did  not appear with 2.42, but with 2.43:



* 2.35 - Build 20150807220800
  i) SidebarSearch ←follows→  Searchbar
  k) Preferences follows: SidebarSearch=Searchbar
  l1) Preferences change without influence to actual search machine selection
  l2) After SM relaunch LocationbarSearch follows Preferences
  m) SidebarSearch=Searchbar IMMEDIATELY changes  LocationbarSearch
* 2.38 – Build 20150923195647
  Like 2.35
* 2.39B – Build 20151028234211
  Like 2.35
* 2.39 – Build 20151103191810
  Like 2.35
* 2.40 – Build 20160120202951
  Rest Like 2.35
* 2.41 – Build 20160201082047 
  Like 2.35
* 2.42 – Build 20151111021032 
  Like 2.35

* 2.43 – build 20160116041507
  k): Preferences NO LONGER follows: SidebarSearch=Searchbar
  l2): After SM relaunch LocationbarSearch NO LONGER follows Preferences
  rest Like 2.35
* 2.45 – Build 20160308001946
  Like 2.43
Comment 4 User image Rainer Bielefeld 2016-04-19 22:44:10 PDT
(In reply to Rainer Bielefeld from comment #0)
should have been: "Steps how to reproduce with ..."

c): Because behavior changed between 2.42 and 2.43 I now doubt that this one has
    to do with Bug 1190185, which has TM 2.41
f): twisted numbers, should have been "complains the problem already for SM 2.43"
    (instead of "2.34")
Comment 5 User image Frank-Rainer Grahl 2016-04-23 01:38:56 PDT
Looks like fallout from Bug 1237648. defaultengine is no longer there.
Comment 6 User image Frank-Rainer Grahl 2016-04-23 02:07:37 PDT
Workaround: set engine in Sidebar->Search and do a search. Default engine is updated to this name.
Comment 7 User image Markus Grob 2016-04-24 23:25:58 PDT
Tested with Suse Leap and SM 2.43. Same behaviour as described above.
Comment 8 User image Rainer Bielefeld 2016-04-25 01:34:39 PDT
OS Due to Comment 7
Comment 9 User image Philip Chee 2016-04-26 07:37:26 PDT
This is probably deliberate. See Bug 1237648 (The defaultEngine attribute should just be an alias to the currentEngine attribute)

Not sure what we can do besides forking the whole of
toolkit/components/search/ plus
netwerk/base/nsIBrowserSearchService.idl

(if we do fork there are a lot of other silly changes that I want to back out as well)
Comment 10 User image lvm 2016-07-18 00:57:19 PDT
*** Bug 1287373 has been marked as a duplicate of this bug. ***
Comment 11 User image Frank-Rainer Grahl 2016-08-16 10:54:11 PDT
Created attachment 8781634 [details]
search1.png

I think if the current search engine can be set easily the default engine can be dropped. If you add the search bar to the toolbar this is currently possible.
Comment 12 User image Frank-Rainer Grahl 2016-08-16 10:54:58 PDT
Created attachment 8781635 [details]
search2.png

Same with no separate search button.
Comment 13 User image Frank-Rainer Grahl 2016-08-16 10:55:35 PDT
Created attachment 8781636 [details]
search3.png

No search buttuon no search bar.
Comment 14 User image Frank-Rainer Grahl 2016-08-16 10:56:18 PDT
Created attachment 8781637 [details]
search4.png

Search bar could also be put into the bookmarks toolbar.
Comment 15 User image Frank-Rainer Grahl 2016-08-16 10:58:03 PDT
Created attachment 8781638 [details]
search5-mock.jpg

This one doesn't exist yet. If search bar is not displayed just display the icon. Search will be possible in the location bar.
Comment 16 User image Tony Mechelynck [:tonymec]. (NEEDINFO me if you want my attention) 2016-12-14 07:36:59 PST
The fix for this bug, if and when it comes around, should be checked not to interfere with the working of the "Keyword Search" extension, by Mike Kaply -- so I'm adding him to the Cc.
Comment 17 User image AnyFile 2016-12-24 03:15:57 PST
Stable 2.46: same behaviour as described here.

The workaround described in comment6 working in Stable 2.46 too
Comment 18 User image Philip Chee 2016-12-27 11:44:16 PST
In the old Mozilla Suite (XPFE) internet search implementation there was a pref called "browser.search.defaultenginename". When Firefox moved to using the new Toolkit search, the preference was migrated to the search service ss.defaultEngine property. ref:

[Android] Bug 1238516 - Migrate the defaultenginename preference
https://hg.mozilla.org/mozilla-central/rev/4abbb197ebfe

For some reason when SeaMonkey moved to Toolkit Search the old pref wasn't migrated.

Time moved on.

A mozilla developer decided to make the defaultEngine and currentEngine synchronized in an attempt to reduce end user confusion in the support forums:

Bug 860560: make sure that defaultEngine and currentEngine stay in sync
https://hg.mozilla.org/mozilla-central/rev/9d6db6508757

Time moved on.

A mozilla developer decided to make the readonly defaultEngine read-write allowing users to change their default search engine.

Bug 738818 - consolidate Firefox search preferences
part 1: replace originalDefaultEngine with defaultEngine and make defaultEngine a settable
https://hg.mozilla.org/mozilla-central/rev/cc569e87074c

Time moved on.

A mozilla developer decided to remove the defaultEngine implementation and just made it forward to the current engine, so these two are now always the same.

Bug 1237648 - The defaultEngine attribute should just be an alias to the currentEngine attribute 
Part 1: remove the defaultEngine implementation and forward the getter/setter to currentEngine,

===========================================================================
In this bug we'll just make sure that all search widgets (urlbar search, searchbar, sidebar search) just respect the currentEngine setting and also make them all synchronized.

===========================================================================
Down the line Ratty plans to address in more detail these problems on the mozilla-release52 and mozilla-esr52 (SeaMonkey 2.49) channels by creating a SeaMonkey specific release branch in these repositories. And then backing out changes that we disagree with.
Comment 19 User image Philip Chee 2016-12-27 12:09:02 PST
*** Bug 1224839 has been marked as a duplicate of this bug. ***
Comment 20 User image Philip Chee 2016-12-27 12:13:48 PST
Created attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

This patch makes all our search widgets (urlbar, searchbar, sidebar search) broadcast an observer notification whenever the current search engine is changed. Each of our search widgets will also listen for such notifications from the other widgets and update appropriately.

> +++ b/suite/browser/urlbarBindings.xml
> -          var defaultEngine = Services.search.defaultEngine;
> +          var defaultEngine = this.mSearchService.defaultEngine;
......
> +      <field name="mSearchService">
> +        Components.classes["@mozilla.org/browser/search-service;1"]
> +                  .getService(Components.interfaces.nsIBrowserSearchService);
> +      </field>
Remove dependency on Services.jsm introduced accidentally in previous patches.

> +++ b/suite/common/pref/pref-search.js

>    observe: function searchEngineListObj_observe(aEngine, aTopic, aVerb) {
>      if (aTopic != "browser-search-engine-modified")
>        return;
>      MakeList();
> -    var pref = document.getElementById("browser.search.defaultenginename");
> -    if (pref)
> -      pref.updateElements();
This pref doesn't seem to do anything these days. On ESR52 and/or SeaMonkey 2.49 release I plan to de link current and default engines and then migrate this pref to the Toolkit search service.

>  function MakeList() {
>    var menulist = document.getElementById("engineList");
> -  while (menulist.hasChildNodes())
> -    menulist.lastChild.remove();
> +  var currentEngineName = Services.search.currentEngine.name;
> +
> +  // Make sure the popup is empty.
> +  menulist.removeAllItems();
 
>    var engines = Services.search.getVisibleEngines();
> -  for (let i = 0; i < engines.length; i++) {
> -    let name = engines[i].name;
> +  for (engine of engines) {
> +    let name = engine.name;
>      let menuitem = menulist.appendItem(name, name);
>      menuitem.setAttribute("class", "menuitem-iconic");
> -    if (engines[i].iconURI)
> -      menuitem.setAttribute("image", engines[i].iconURI.spec);
> -    menuitem.engine = engines[i];
> +    if (engine.iconURI)
> +      menuitem.setAttribute("image", engine.iconURI.spec);
> +    menuitem.engine = engine;
> +    if (engine.name == currentEngineName) {
> +      // Set selection to the current default engine.
> +      menulist.selectedItem = menuitem;
> +    }
>    }
> +  // If the current engine isn't in the list any more, select the first item.
> +  if (menulist.selectedIndex < 0)
> +    menulist.selectedIndex = 0;
Sync this code with the implementation in the sidebar search panel.

> +  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current");
Sprinkle this around everywhere.

> +++ b/suite/common/src/nsSuiteGlue.js

>        case "browser-search-engine-modified":
> -        if (data != "engine-default" && data != "engine-current") {
> -          break;
> -        }
Remove useless sync code now that it's done in the toolkit backend. But leaving the case in because there is some Firefox code that we might want to port over later on.
Comment 21 User image Frank-Rainer Grahl 2016-12-27 12:58:04 PST
Just a quick observation first.

pref-search.js MakeList() and search-panel.js LoadEngineList() are almost identical code wise on first glance. 
MakeList just additionally defines the menulist in the first line. Not much code but maybe it can be easily shared between the two.
Comment 22 User image Frank-Rainer Grahl 2016-12-27 13:15:04 PST
Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

Setting the default enging in prefs results in a:

> Timestamp: 12/27/2016 10:09:59 PM
> Error: TypeError: temp is null
> Source File: chrome://global/content/bindings/preferences.xml
> Line: 1227

Otherwise it seems to work fine so f+ with it fixed.
Comment 23 User image Philip Chee 2016-12-27 22:44:00 PST
> pref-search.js MakeList() and search-panel.js LoadEngineList() are almost
> identical code wise on first glance. 
> MakeList just additionally defines the menulist in the first line. Not much
> code but maybe it can be easily shared between the two.

In future, I intend to create a SearchUtils.jsm module to hold common search code but that is out of scope for this bug.
Comment 24 User image Philip Chee 2016-12-27 22:45:51 PST
(In reply to Frank-Rainer Grahl from comment #22)
> Comment on attachment 8822079 [details] [diff] [review]
> Patch 1.0 make every search widget synchronized.
> 
> Setting the default enging in prefs results in a:
> 
> > Timestamp: 12/27/2016 10:09:59 PM
> > Error: TypeError: temp is null
> > Source File: chrome://global/content/bindings/preferences.xml
> > Line: 1227
> 
> Otherwise it seems to work fine so f+ with it fixed.

This appears to be a bug in our prefwindow.xml binding and inherited from the Toolkit preferences.xml binding.
Comment 25 User image Rainer Bielefeld 2016-12-29 22:07:02 PST
*** Bug 1326342 has been marked as a duplicate of this bug. ***
Comment 26 User image Philip Chee 2017-01-01 12:22:21 PST
>> Setting the default engine in prefs results in a:
>> 
>>> Timestamp: 12/27/2016 10:09:59 PM
>>> Error: TypeError: temp is null
>>> Source File: chrome://global/content/bindings/preferences.xml
>>> Line: 1227
>> 
>> Otherwise it seems to work fine so f+ with it fixed.
> 
> This appears to be a bug in our prefwindow.xml binding and inherited from
> the Toolkit preferences.xml binding.
FYI This preferences.xml error is fixed in bug 1326210
Comment 27 User image Ian Neal 2017-01-01 12:33:49 PST
Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

r/a=me
Comment 28 User image Philip Chee 2017-01-02 06:23:19 PST
pushed comm-central changeset 3e261acfbfb599a3b55501bb945a6bcb6ba8d2ea
pushed comm-central changeset 3e261acfbfb5
http://hg.mozilla.org/comm-central/rev/3e261acfbfb5
Comment 29 User image Philip Chee 2017-01-02 06:26:06 PST
Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

[Approval Request Comment]
Regression caused by (bug #): long standing problem.

User impact if declined: Searchbar, Sidebar search urlbar search all not synchronized.

Testing completed (on m-c, etc.): smoke tested on comm-central
Risk to taking this patch (and alternatives if risky): none. this is a reasonably safe bug fix.

String changes made by this patch: none.
Comment 30 User image Ian Neal 2017-01-02 06:32:06 PST
Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

a=me for c-a / c-b
Comment 32 User image Rainer Bielefeld 2017-01-05 01:24:10 PST
*** Bug 1328827 has been marked as a duplicate of this bug. ***
Comment 33 User image Philip Chee 2017-01-05 07:11:51 PST
Followup: fix some DOS line endings that crept in rs=typos a=typofix
https://hg.mozilla.org/comm-central/rev/b41082f7b0c6d6223c7f7000276b2e0ad329a5f8
Comment 34 User image Frank-Rainer Grahl 2017-01-06 16:00:39 PST
*** Bug 1329217 has been marked as a duplicate of this bug. ***
Comment 35 User image Rainer Bielefeld 2017-01-10 09:48:22 PST
*** Bug 1329948 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.