1265881 - Location Bar Internet Search does not respect ˋEdit → Preferences → Browser → Internet Searchˊ

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Rainer Bielefeld]

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

[Rainer Bielefeld]

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

[Rainer Bielefeld]

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

[Rainer Bielefeld]

(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

[Rainer Bielefeld]

(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

[Frank-Rainer Grahl (:frg)]

Looks like fallout from Bug 1237648. defaultengine is no longer there.

Comment 6

[Frank-Rainer Grahl (:frg)]

Workaround: set engine in Sidebar->Search and do a search. Default engine is updated to this name.

Comment 7

[Markus Grob]

Tested with Suse Leap and SM 2.43. Same behaviour as described above.

Comment 8

[Rainer Bielefeld]

OS Due to Comment 7

Comment 9

[Philip Chee]

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 11

[Frank-Rainer Grahl (:frg)]

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

[Frank-Rainer Grahl (:frg)]

Attachment: search2.png

Same with no separate search button.

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: search3.png

No search buttuon no search bar.

Comment 14

[Frank-Rainer Grahl (:frg)]

Attachment: search4.png

Search bar could also be put into the bookmarks toolbar.

Comment 15

[Frank-Rainer Grahl (:frg)]

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

[Tony Mechelynck [:tonymec]]

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

[AnyFile]

Stable 2.46: same behaviour as described here.

The workaround described in comment6 working in Stable 2.46 too

Comment 18

[Philip Chee]

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 20

[Philip Chee]

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

[Frank-Rainer Grahl (:frg)]

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

[Frank-Rainer Grahl (:frg)]

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

[Philip Chee]

> 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

[Philip Chee]

(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 26

[Philip Chee]

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

[Ian Neal]

Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

r/a=me

Comment 28

[Philip Chee]

pushed comm-central changeset 3e261acfbfb599a3b55501bb945a6bcb6ba8d2ea
pushed comm-central changeset 3e261acfbfb5
http://hg.mozilla.org/comm-central/rev/3e261acfbfb5

Comment 29

[Philip Chee]

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

[Ian Neal]

Comment on attachment 8822079 [details] [diff] [review]
Patch 1.0 make every search widget synchronized.

a=me for c-a / c-b

Comment 31

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-aurora/rev/b8e96387b63dcbde13619b2d9d8cae3514ee02a7
https://hg.mozilla.org/releases/comm-beta/rev/91b652430b952e52144c5190f6496bde7c0468ac

a=IanN for c-r over irc:
https://hg.mozilla.org/releases/comm-release/rev/97c6d580f40f551fb830bb12e521be603400c3a0

Comment 33

[Philip Chee]

Followup: fix some DOS line endings that crept in rs=typos a=typofix
https://hg.mozilla.org/comm-central/rev/b41082f7b0c6d6223c7f7000276b2e0ad329a5f8