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

RESOLVED FIXED in seamonkey2.50

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: RainerBielefeldNG, Assigned: philip.chee)

Tracking

(Blocks: 1 bug, {regression, reproducible})

SeaMonkey 2.43 Branch
seamonkey2.50
Unspecified
All
regression, reproducible
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.46 wontfix, seamonkey2.47 fixed, seamonkey2.48 fixed, seamonkey2.49esr fixed, seamonkey2.50+ fixed)

Details

(URL)

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.

Attachments

(6 attachments)

(Reporter)

Description

3 years ago
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""?
(Reporter)

Comment 1

3 years ago
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>
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, reproducible
OS: Unspecified → Windows 7
Version: SeaMonkey 2.45 Branch → SeaMonkey 2.43 Branch
(Reporter)

Comment 2

3 years ago
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
Version: SeaMonkey 2.43 Branch → SeaMonkey 2.42 Branch
(Reporter)

Comment 3

3 years ago
(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
Version: SeaMonkey 2.42 Branch → SeaMonkey 2.43 Branch
(Reporter)

Comment 4

3 years ago
(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")
(Reporter)

Updated

3 years ago
Blocks: 1266642
Looks like fallout from Bug 1237648. defaultengine is no longer there.
Workaround: set engine in Sidebar->Search and do a search. Default engine is updated to this name.

Comment 7

3 years ago
Tested with Suse Leap and SM 2.43. Same behaviour as described above.
(Reporter)

Comment 8

3 years ago
OS Due to Comment 7
OS: Windows 7 → All
(Assignee)

Comment 9

3 years ago
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)

Updated

3 years ago
Duplicate of this bug: 1287373
Posted image 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.
Posted image search2.png
Same with no separate search button.
Posted image search3.png
No search buttuon no search bar.
Posted image search4.png
Search bar could also be put into the bookmarks toolbar.
Posted image 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.
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

2 years ago
Stable 2.46: same behaviour as described here.

The workaround described in comment6 working in Stable 2.46 too
(Assignee)

Comment 18

2 years ago
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1224839
(Assignee)

Comment 20

2 years ago
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.
Attachment #8822079 - Flags: review?(iann_bugzilla)
Attachment #8822079 - Flags: feedback?(frgrahl)
(Assignee)

Updated

2 years ago
User Story: (updated)
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 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.
Attachment #8822079 - Flags: feedback?(frgrahl) → feedback+
(Assignee)

Comment 23

2 years ago
> 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.
(Assignee)

Comment 24

2 years ago
(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.
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1326342
(Assignee)

Comment 26

2 years ago
>> 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

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

r/a=me
Attachment #8822079 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 28

2 years ago
pushed comm-central changeset 3e261acfbfb599a3b55501bb945a6bcb6ba8d2ea
pushed comm-central changeset 3e261acfbfb5
http://hg.mozilla.org/comm-central/rev/3e261acfbfb5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.47: --- → affected
status-seamonkey2.48: --- → affected
status-seamonkey2.49esr: --- → affected
status-seamonkey2.50: --- → fixed
tracking-seamonkey2.47: --- → ?
tracking-seamonkey2.48: --- → ?
tracking-seamonkey2.49esr: --- → ?
tracking-seamonkey2.50: --- → +
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.50
(Assignee)

Comment 29

2 years ago
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.
Attachment #8822079 - Flags: approval-comm-beta?
Attachment #8822079 - Flags: approval-comm-aurora?

Comment 30

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

a=me for c-a / c-b
Attachment #8822079 - Flags: approval-comm-beta?
Attachment #8822079 - Flags: approval-comm-beta+
Attachment #8822079 - Flags: approval-comm-aurora?
Attachment #8822079 - Flags: approval-comm-aurora+
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
status-seamonkey2.47: affected → fixed
status-seamonkey2.48: affected → fixed
status-seamonkey2.49esr: affected → fixed
Target Milestone: seamonkey2.50 → seamonkey2.47
status-seamonkey2.46: --- → wontfix
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1328827
(Assignee)

Comment 33

2 years ago
Followup: fix some DOS line endings that crept in rs=typos a=typofix
https://hg.mozilla.org/comm-central/rev/b41082f7b0c6d6223c7f7000276b2e0ad329a5f8
Duplicate of this bug: 1329217
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1329948

Updated

2 years ago
Target Milestone: seamonkey2.47 → seamonkey2.50
tracking-seamonkey2.47: ? → ---
tracking-seamonkey2.48: ? → ---
tracking-seamonkey2.49esr: ? → ---
You need to log in before you can comment on or make changes to this bug.