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

RESOLVED FIXED in seamonkey2.50

Status

SeaMonkey
Preferences
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: Rainer Bielefeld, Assigned: Philip Chee)

Tracking

(Blocks: 2 bugs, {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.49? 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

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

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

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

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

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

a year ago
Blocks: 1266642

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

a year ago
OS Due to Comment 7
OS: Windows 7 → All
(Assignee)

Comment 9

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

9 months ago
Duplicate of this bug: 1287373

Comment 11

8 months ago
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

8 months ago
Created attachment 8781635 [details]
search2.png

Same with no separate search button.

Comment 13

8 months ago
Created attachment 8781636 [details]
search3.png

No search buttuon no search bar.

Comment 14

8 months ago
Created attachment 8781637 [details]
search4.png

Search bar could also be put into the bookmarks toolbar.

Comment 15

8 months ago
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.

Updated

5 months ago
Blocks: 1319091
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

4 months ago
Stable 2.46: same behaviour as described here.

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

Comment 18

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

4 months ago
Duplicate of this bug: 1224839
(Assignee)

Comment 20

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

Updated

4 months ago
User Story: (updated)

Comment 21

4 months ago
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

4 months ago
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

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

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

4 months ago
Duplicate of this bug: 1326342
(Assignee)

Comment 26

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

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

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

Comment 29

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

4 months 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+

Comment 31

4 months ago
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.49: affected → fixed
Target Milestone: seamonkey2.50 → seamonkey2.47

Updated

4 months ago
status-seamonkey2.46: --- → wontfix
(Reporter)

Updated

4 months ago
Duplicate of this bug: 1328827
(Assignee)

Comment 33

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

Updated

4 months ago
Duplicate of this bug: 1329217
(Reporter)

Updated

3 months ago
Duplicate of this bug: 1329948

Updated

3 months ago
Target Milestone: seamonkey2.47 → seamonkey2.50
You need to log in before you can comment on or make changes to this bug.