Last Comment Bug 401417 - Add an option for a search box in SeaMonkey toolbar
: Add an option for a search box in SeaMonkey toolbar
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: seamonkey2.1b3
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
: 434957 464771 (view as bug list)
Depends on: CustomToolbars 410613
Blocks: 595246 617577 639189 640418 640420 640425 640426 640427 640514 641330 643172 645062
  Show dependency treegraph
 
Reported: 2007-10-27 19:30 PDT by Charles
Modified: 2011-03-25 10:25 PDT (History)
15 users (show)
kairo: wanted‑seamonkey2.0-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
WIP1: make a rough variant appear in the customize palette (74.17 KB, patch)
2010-09-05 18:30 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
WIP 2: update to checked-in OpenSearch code (79.53 KB, patch)
2010-09-30 10:15 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1: add optional search box (80.97 KB, patch)
2011-02-20 18:15 PST, Robert Kaiser (not working on stability any more)
neil: review-
Details | Diff | Review
v2: address all comments I could address (87.29 KB, patch)
2011-03-05 13:43 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v2.1: also fix the search suggestion/history popup (85.39 KB, patch)
2011-03-05 16:35 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v2.2: style "suggestions" comment and add modern support (94.60 KB, patch)
2011-03-05 17:26 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v2.3: address another round of comments (95.30 KB, patch)
2011-03-06 17:23 PST, Robert Kaiser (not working on stability any more)
neil: review+
Details | Diff | Review
v2.4: fix the last few comments (95.46 KB, patch)
2011-03-09 17:05 PST, Robert Kaiser (not working on stability any more)
kairo: review+
neil: ui‑review+
Details | Diff | Review

Description Charles 2007-10-27 19:30:13 PDT
User-Agent:       Opera/9.24 (Windows NT 5.1; U; en)
Build Identifier: 

It is possible to type a search string in the location bar and click "Search", but it's not very "familiar", at least for new users. It would be a good thing to have a search box in the place of the "Search" button in the right-hand side. This way you type your text and hit "Enter", like in other popular browsers.

Reproducible: Always
Comment 1 Tony Mechelynck [:tonymec] 2008-04-20 08:27:03 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041601 SeaMonkey/2.0a1pre

I can type text in the Search box in the sidebar (and change the search engine there either temporarily or permanently). (The sidebar is opened or closed by hitting F9.) I can also type text in the URL bar, then select one of the completions from history, or the Search item below them all -- or click the Search button if present.

I don't think an additional Search Bar next to the Location Bar (and taking up real estate there) is in the works for SeaMonkey. KaiRo, what do you think?
Comment 2 Robert Kaiser (not working on stability any more) 2008-04-21 11:17:45 PDT
We probably won't do this as a default, but we could think about providing this search box optionally through toolbar customization, once we have that. Making this depend on bug 394288 because of that.
Comment 3 lenochod 2008-11-13 15:00:41 PST
*** Bug 434957 has been marked as a duplicate of this bug. ***
Comment 4 lenochod 2008-11-13 15:05:23 PST
*** Bug 464771 has been marked as a duplicate of this bug. ***
Comment 5 Dave Buco 2009-08-05 07:36:53 PDT
So, is this dead. Or is this even being considered? 
A discussion has arisen here: http://forums.mozillazine.org/viewtopic.php?f=6&t=1380105

I, for one, really like a separate searchbar, fully customizable. If implemented into SeaMonkey 2, it doesn't have to be next to the url bar by default. It can be in the customize box and be placed by the user if desired.
Comment 6 Robert Kaiser (not working on stability any more) 2009-08-05 08:00:15 PDT
If someone comes up with a patch for it (as you say, in the customize items only, not on the toolbar by default), and it gets the necessary reviews, all in time before the respective freezes, we'll take it. We have higher priority things for our existing team to tend to, though - but if someone else writes a patch, we surely welcome it.
Comment 7 Dave Buco 2009-08-05 12:34:42 PDT
Glad to hear your at least open to the idea, Robert. Having ZERO experience in such things, I hope someone can take this and run with it before any respective freezes.

Something similar to the current Firefox searchbar, perhaps. In that it is fully customizable with the ability to add various search engines and view them via a dropdown. To be included in the customize items box only, NOT a default toolbar item. 

IMAGE: http://i27.tinypic.com/iwn23a.jpg

Anyway, I appreciate your listening to me on this issue, Robert.
Comment 8 Dave Buco 2009-10-04 12:21:58 PDT
I hope this will be considered for releases beyond 2.0. Until then I hope a smart add-on dev will create something. This is the only thing keeping me from using SeaMonkey as my default browser.
Comment 9 Robert Kaiser (not working on stability any more) 2009-10-05 10:30:02 PDT
Dave, it's surely considered, what it really needs is someone writing up the code, though. Everyone working on this project is "only" volunteering for it, and usually overloaded with what he's already doing. Anyone helping us with a patch is appreciated.
Comment 10 Robert Kaiser (not working on stability any more) 2009-11-04 07:22:05 PST
no new features for 2.0 series.
Comment 11 Dave Buco 2009-11-04 08:29:41 PST
Sorry, Robert. Forgot. Got a bit overzealous.
Comment 12 Robert Kaiser (not working on stability any more) 2010-04-27 07:53:04 PDT
A really reasonable implementation of this probably needs bug 410613 fixed first. When that's done, this should be easy to solve by porting the Firefox code.

Note that nobody is working on either this or bug 410613 right now, but help is surely wanted in both places.
Comment 13 Robert Kaiser (not working on stability any more) 2010-09-05 18:30:21 PDT
Created attachment 472316 [details] [diff] [review]
WIP1: make a rough variant appear in the customize palette

I'll not assign this to myself for the moment, as I don't know if I'll drive this into the tree ultimately, but here's a very rough first version, needing the bug 410613 work to do anything at all, but then adding an item to the customize palette that when moved into the toolbar can list the available search engines. Actual search doesn't even work with it, though, so don't hold your breath for it (yet). Same with the engine manager.
Comment 14 Robert Kaiser (not working on stability any more) 2010-09-30 10:15:24 PDT
Created attachment 479816 [details] [diff] [review]
WIP 2: update to checked-in OpenSearch code

I still won't assign this to myself, if anyone else wants to adopt it while I'm away, feel free to do so.

This patch updates this to what landed for our OpenSearch support, a few things that had been in the patch there earlier are here now as they're only needed for searchbar. This works somewhat but needs more work.
Comment 15 Wyatt Childers 2010-10-05 14:53:57 PDT
*Suggestion*
Why don't we use one of the original ideas for FF4 and add a little drop down search thing to the Location bar like a little icon near the bookmark thing and u click and pick the search engine.
Comment 16 Dave Buco 2010-10-05 15:39:25 PDT
(In reply to comment #15)
> *Suggestion*
> Why don't we use one of the original ideas for FF4 and add a little drop down
> search thing to the Location bar like a little icon near the bookmark thing and
> u click and pick the search engine.

I like that idea. Choosing the search engine via a dropdown is critical, in my view.
Comment 17 Robert Kaiser (not working on stability any more) 2010-10-05 15:55:17 PDT
(In reply to comment #15)
> *Suggestion*
> Why don't we use one of the original ideas for FF4 and add a little drop down
> search thing to the Location bar like a little icon near the bookmark thing and
> u click and pick the search engine.

Multiple icons there would be confusing. That said, this doesn't belong into this bug, but yet another one.
Comment 18 Wyatt Childers 2010-10-05 15:57:31 PDT
(In reply to comment #17)
> (In reply to comment #15)
> > *Suggestion*
> > Why don't we use one of the original ideas for FF4 and add a little drop down
> > search thing to the Location bar like a little icon near the bookmark thing and
> > u click and pick the search engine.
> 
> Multiple icons there would be confusing. That said, this doesn't belong into
> this bug, but yet another one.

No no no I think your miss understanding me. You know how Firefox has the drop down in the search box? Well that same thing only right aligned on the URL/Location bar. It just lets you change search it's sort of related to this bug because it could be used instead of the separate search box.
Comment 19 Robert Kaiser (not working on stability any more) 2010-10-05 16:02:15 PDT
(In reply to comment #18)
> No no no I think your miss understanding me. You know how Firefox has the drop
> down in the search box? Well that same thing only right aligned on the
> URL/Location bar. It just lets you change search it's sort of related to this
> bug because it could be used instead of the separate search box.

Still would be confusing. And still belong in a different bug report, as this one here is specifically about adding an optional search bar, and has been about that even before I ported OpenSearch.
Comment 20 Wyatt Childers 2010-10-05 16:31:51 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > No no no I think your miss understanding me. You know how Firefox has the drop
> > down in the search box? Well that same thing only right aligned on the
> > URL/Location bar. It just lets you change search it's sort of related to this
> > bug because it could be used instead of the separate search box.
> 
> Still would be confusing. And still belong in a different bug report, as this
> one here is specifically about adding an optional search bar, and has been
> about that even before I ported OpenSearch.

I disagree with the confusing part it's far more strait forward than the sidebar in my opinion as well. I think I may file a bug for it thou I think it would be far better than the sidebar (Of course the sidebar should be an option) But that's just my opinion thanks.
Comment 21 Dave Buco 2010-10-05 17:01:26 PDT
(In reply to comment #19)
> Still would be confusing...

I'm at a loss as to how this could ever be considered confusing:

http://i53.tinypic.com/1tpu8g.jpg

Concise, all in one place, easy to manage, easy to navigate. A win-win if you ask me.
Comment 22 Robert Kaiser (not working on stability any more) 2010-10-05 17:08:46 PDT
(In reply to comment #21)
> I'm at a loss as to how this could ever be considered confusing:
> 
> http://i53.tinypic.com/1tpu8g.jpg

Well, that's the search bar this whole bug is about and which will be available in the customize toolbar set when this lands (in the current patch, it does only work halfway)!
Comment 23 Charles 2010-10-05 21:16:08 PDT
(In reply to comment #19)
> adding an optional search bar

Search bar or search box? Please, no search bar but a search box! :)
Thanks.
Comment 24 Robert Kaiser (not working on stability any more) 2010-10-05 21:28:44 PDT
(In reply to comment #23)
> Search bar or search box? Please, no search bar but a search box! :)

Firefox calls this box the "search bar", and it matches our wording that the address entry box is the "location bar", while the toolbar containing it is the "navigation bar". Yes, might be confusing to have some elements inside a bar be called bars as well, but that's how it is. :)
Comment 25 Yhn 2010-12-08 05:29:25 PST
Should I interpret this bug-report as “add an option”? or “add an substitution”? If it is “add an option”, I support.
Comment 26 Robert Kaiser (not working on stability any more) 2010-12-08 06:32:27 PST
(In reply to comment #25)
> Should I interpret this bug-report as “add an option”? or “add an
> substitution”? If it is “add an option”, I support.

What about the summary don't you understand?
Comment 27 Robert Kaiser (not working on stability any more) 2011-02-20 18:15:29 PST
Created attachment 513915 [details] [diff] [review]
v1: add optional search box

I'll take this for the moment, pending on how bad the review gets, as my time is quite limited these days - I still think it would be a good idea to add this option to SeaMonkey 2.1, though.

This patch has a lot of Firefox code, so no idea how much you like it, Neil, but it looks like it works - and esp. the engine manager is something we should have somewhere. Once this patch adds it in the search bar, it should be an easy followup to also call it from other places (sidebar, prefs).
Comment 28 neil@parkwaycc.co.uk 2011-02-21 04:18:34 PST
OK, so I gave this a whirl for a ui-review.

The first problem was the flex on the search box was ridiculously massive. It needs to be comparable to the flex on a flexible space! I was forced to place the search box next to the location bar.

The second problem was that the history dropmarker on the search box appears to do nothing. Perhaps it doesn't belong there at all?

The third problem is that there's a huge gap between the search engine logo and the search text. I think there's supposed to be a dropmarker there.

There appears to be a problem with the "Search the Web" command; a bug in the existing code isn't focusing the search bar correctly.

I didn't get any search suggestions. It tried to offer me something, but all I got was a blank popup.

I expected to be able to change the keyword for an engine by double-clicking.
Comment 29 neil@parkwaycc.co.uk 2011-02-21 05:13:29 PST
Comment on attachment 513915 [details] [diff] [review]
v1: add optional search box

>+    if (getBrowser().shouldLoadFavIcon(targetDoc.documentURIObject))
>+      iconURL = targetDoc.documentURIObject.prePath + "/favicon.ico";
Nit: iconURL = getBrowser().buildFavIconString(targetDoc.documentURIObject);

>+    if (!searchBar || !searchBar.searchButton)
[searchBar is probably not null. searchButton I'm not so sure.]

>+    var searchEnginesURL = formatURL("browser.search.searchEnginesURL", true);
formatURL is not defined.

>-                   flex="1"
>+                   flex="400"
Ah yes, this would be a problem. Try flex="4" here and "1" later.

>+      gEngineView.invalidate();
Probably unnecessary in the engine-added case.

>+    // Remove the observer
>+    var os = Cc["@mozilla.org/observer-service;1"].
>+             getService(Ci.nsIObserverService);
>+    os.removeObserver(this, "browser-search-engine-modified");
This should be in an unload handler.

>+        var emsg = strings.getFormattedString("duplicateEngineMsg",
>+                                              [engine.name]);
engine is not defined. I guess the original code used var engine ;-)

>+    document.getElementById("cmd_remove").setAttribute("disabled",
>+                                                       disableButtons);
>+
>+    document.getElementById("cmd_moveup").setAttribute("disabled",
>+                                            disableButtons || firstSelected);
>+
>+    document.getElementById("cmd_movedown").setAttribute("disabled",
>+                                             disableButtons || lastSelected);
[In theory you don't need disableButtons, you can use noSelection || (firstSelected && lastSelected) instead. Conveniently in the move case, the || collapses to give noSelection || xxxSelected as the condition.]

>+    document.getElementById("cmd_editkeyword").setAttribute("disabled",
>+                                                            noSelection);
[Oddly enough there's no blank line this time.]

>+  _cloneEngine: function ES_cloneObj(aEngine) {
>+    var newO=[];
Should be = {}; also new0 is a weird name, why not call it clone?

>+    this.selection.clearSelection();
>+    this.selection.select(dropIndex);
Making a new selection automatically clears the existing selection.

>+  isEditable: function(index, column) { return false; },
Follow-up bug to make the alias editable!

>+                    ondragstart="onDragEngineStart(event)"/>
Nit: missing ;

>+      <button id="dn"
[I'm sure we can spare a couple of letters ;-) ]

>+  <hbox>
>+    <checkbox id="enableSuggest"
>+              label="&enableSuggest.label;"
>+              accesskey="&enableSuggest.accesskey;"/>
>+    <spacer flex="1"/>
>+  </hbox>
>+  <hbox>
>+    <label id="addEngines" class="text-link" value="&addEngine.label;"
>+           onclick="if (event.button == 0) { gEngineManagerDialog.loadAddEngines(); }"/>
>+    <spacer flex="1"/>
>+  </hbox>
Unnecessary spacers.

I need a break ;-)
Comment 30 neil@parkwaycc.co.uk 2011-02-21 05:31:31 PST
Hmm, the feed icon appearing and disappearing flexes the search box annoyingly.
Comment 31 neil@parkwaycc.co.uk 2011-02-21 07:13:08 PST
(In reply to comment #29)
> (From update of attachment 513915 [details] [diff] [review])
> Unnecessary spacers.
Now that I mention it, git-apply complained about 5 unnecessary spaces too ;-)
Comment 32 neil@parkwaycc.co.uk 2011-02-21 11:43:27 PST
Comment on attachment 513915 [details] [diff] [review]
v1: add optional search box

>+<!DOCTYPE bindings [
>+<!ENTITY % searchBarDTD SYSTEM "chrome://communicator/locale/search/searchbar.dtd" >
>+%searchBarDTD;
>+<!ENTITY % navigatorDTD SYSTEM "chrome://navigator/locale/navigator.dtd">
>+%navigatorDTD;
>+]>
[I wonder why only one of these has a space before the > ;-) ]

>+        setTimeout(function (a) { a.init(); }, 0, this);
[I wonder why this is done on a timeout. Anyway, we can now use .bind(this)]

>+      <field name="_ss">null</field>
[continued...]

>+          return currentEngine || {name:"", uri:null};
Nit: nicer formatting on the dummy object.

>+      <property name="searchService" readonly="true">
>+        <getter><![CDATA[
>+          if (!this._ss) {
>+            const nsIBSS = Components.interfaces.nsIBrowserSearchService;
>+            this._ss =
>+                 Components.classes["@mozilla.org/browser/search-service;1"]
>+                           .getService(nsIBSS);
>+          }
>+          return this._ss;
>+        ]]></getter>
>+      </property>
[continued] Could initialise _ss to the search service (fields are lazy) and keep searchService as the readonly public accessor.


>+          var allbrowsers = getBrowser().mPanelContainer.childNodes;
>+          for (var tab = 0; tab < allbrowsers.length; tab++) {
>+            var browser = getBrowser().getBrowserAtIndex(tab);
>+            if (browser.hiddenEngines) {
>+              // XXX This will need to be changed when engines are identified by
>+              // URL rather than title; see bug 335102.
>+              var removeTitle = aEngine.wrappedJSObject.name;
>+              for (var i = 0; i < browser.hiddenEngines.length; i++) {
>+                if (browser.hiddenEngines[i].title == removeTitle) {
>+                  if (!browser.engines)
>+                    browser.engines = [];
>+                  browser.engines.push(browser.hiddenEngines[i]);
>+                  browser.hiddenEngines.splice(i, 1);
>+                  break;
>+                }
>+              }
>+            }
>+          }
>+          BrowserSearch.updateSearchButton();
This is very tabbrowsery, so maybe this binding belongs in browser?

>+            if (items[i].getAttribute("class").indexOf("addengine") != -1)
[Isn't there a .classList.contains method these days?]

>+              menuitem.setAttribute("selected", "true");
[I wonder why this uses selected and not default.]

>+          newIndex += (isNextEngine) ? 1 : -1;
Nit: don't need the []s

>+            if ((aEvent && aEvent.altKey) ^ newTabPref)
Alt key?

>+      <handler event="DOMMouseScroll"
>+               phase="capturing"
>+#ifdef XP_MACOSX
>+               action="if (event.metaKey) this.selectEngine(event, (event.detail > 0));"/>
>+#else
>+               action="if (event.ctrlKey) this.selectEngine(event, (event.detail > 0));"/>
>+#endif
[Note to self: check whether modifiers="accel" suffices.]

>+      <field name="_stringBundle"/>
>+      <field name="_formHistSvc"/>
>+      <field name="_prefBranch"/>
>+      <field name="_suggestMenuItem"/>
>+      <field name="_suggestEnabled"/>
>+
>+      <method name="initialize">
>+        <body><![CDATA[
>+          const kXULNS =
>+            "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+          // Initialize fields
>+          this._stringBundle = document.getBindingParent(this)._stringBundle;
>+          this._formHistSvc =
>+                   Components.classes["@mozilla.org/satchel/form-history;1"]
>+                             .getService(Components.interfaces.nsIFormHistory2);
>+          this._prefBranch =
>+                    Components.classes["@mozilla.org/preferences-service;1"]
>+                              .getService(Components.interfaces.nsIPrefBranch);
>+          this._suggestEnabled =
>+              this._prefBranch.getBoolPref("browser.search.suggest.enabled");
These should just go directly in the fields.

>+            // Initially the panel used for the searchbar (PopupAutoComplete
>+            // in browser.xul) is hidden to avoid impacting startup / new
Not sure it's a good idea to use PopupAutoComplete?

>+      <!-- overload |onTextEntered| in autocomplete.xml -->
Nit: should be override; overload means something completely different

>+@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>+
>+.searchbar-textbox {
>+  -moz-binding: url("chrome://communicator/content/search/search.xml#searchbar-textbox");
>+}
>+
>+.searchbar-engine-button {
>+  -moz-user-focus: none;
>+}
Hardly seems worth a separate file, might as well go in browser/navigator.css

>-<!ENTITY  bookmarksButton.label       "Bookmarks">
>-<!ENTITY  bookmarksButton.tooltip     "Bookmarks list">
>-<!ENTITY  homeButton.label            "Home">
>-<!ENTITY  bookmarksToolbarItem.label  "Bookmarks Toolbar Items">
>+<!ENTITY searchItem.title             "Search">
>+
>+<!ENTITY bookmarksButton.label        "Bookmarks">
>+<!ENTITY bookmarksButton.tooltip      "Bookmarks list">
>+<!ENTITY homeButton.label             "Home">
>+<!ENTITY bookmarksToolbarItem.label   "Bookmarks Toolbar Items">
[What happened here?]

>+.searchbar-engine-button {
>+  -moz-appearance: none;
>+  min-width: 0;
>+  margin: 0;
>+  padding: 0;
>+  -moz-padding-end: 2px;
>+  -moz-box-align: center;
>+  background: none;
>+  border: none;
>+}
>+
>+.searchbar-engine-button > .button-box {
>+  -moz-appearance: none;
>+  padding: 0;
>+  border: 0;
>+}
Can we not add class="plain" to the button to avoid most of these?

>+  list-style-image: url(mainwindow-dropdown-arrow.png);
Full chrome URLs in quotes please.

>diff --git a/suite/themes/classic/jar.mn b/suite/themes/classic/jar.mn
Any chance of Modern changes?
Comment 33 neil@parkwaycc.co.uk 2011-02-23 16:16:24 PST
(In reply to comment #28)
> I didn't get any search suggestions. It tried to offer me something, but all I
> got was a blank popup.
Well, there were a number of issues:
1. It didn't like the autocomplete popup. Need to look into that.
2. I had an old google.src in my dist; this overrides google.xml :-(
3. google.xml doesn't work anyway because it sniffs for client=firefox
But I was able to tweak my build to get suggestions from Wikipedia.
Comment 34 neil@parkwaycc.co.uk 2011-02-25 14:15:55 PST
Comment on attachment 513915 [details] [diff] [review]
v1: add optional search box

Sorry it took so long to complete the review, but I cracked the last issue:

>+      <method name="openPopup">
>+        <body><![CDATA[
>+          var popup = this.popup;
>+          if (!popup.mPopupOpen) {
>+            // Initially the panel used for the searchbar (PopupAutoComplete
>+            // in browser.xul) is hidden to avoid impacting startup / new
>+            // window performance. The base binding's openPopup would normally
>+            // call the overriden openAutocompletePopup in urlbarBindings.xml's
>+            // browser-autocomplete-result-popup binding to unhide the popup,
>+            // but since we're overriding openPopup we need to unhide the panel
>+            // ourselves.
This is completely untrue in our case. In fact, it breaks opening the popup.
Comment 35 Robert Kaiser (not working on stability any more) 2011-03-05 13:13:18 PST
First, before addressing the comments, I ported the changes that have been done on the Firefox side - bug 492544 (Paste And Search) and 3 followups as well as bug 430627 - see http://hg.mozilla.org/mozilla-central/log/tip/browser/components/search/content/search.xml and note that my first patch on this dates from early September 2010.

(In reply to comment #29)
> >+    if (!searchBar || !searchBar.searchButton)
> [searchBar is probably not null. searchButton I'm not so sure.]

No need to care any more as this one is gone. :)

> >+    var searchEnginesURL = formatURL("browser.search.searchEnginesURL", true);
> formatURL is not defined.

Good catch, fortunately, now Services has the urlformatter to use directly. :)

> >+        var emsg = strings.getFormattedString("duplicateEngineMsg",
> >+                                              [engine.name]);
> engine is not defined. I guess the original code used var engine ;-)

And even if so, that was IMHO dirty coding. Good catch! Now I know I'll need to backport some stuff to FF anyhow, so I might even go one step further. Let's see how I feel when I'm through with all this...

> Should be = {}; also new0 is a weird name, why not call it clone?

It's not new0, it's newO, but still, at least newObj would have been clearer - used clonedObj though.

> >+  isEditable: function(index, column) { return false; },
> Follow-up bug to make the alias editable!

Bug 639189.

(In reply to comment #32)
> >+        setTimeout(function (a) { a.init(); }, 0, this);
> [I wonder why this is done on a timeout. Anyway, we can now use .bind(this)]

Hrm, sorry, I have no idea how to use .bind or what it does.

> >+          return currentEngine || {name:"", uri:null};
> Nit: nicer formatting on the dummy object.

Are spaces after the field names enough?

> [continued] Could initialise _ss to the search service (fields are lazy) and
> keep searchService as the readonly public accessor.

Hope I've done that correctly in the new patch.

> This is very tabbrowsery, so maybe this binding belongs in browser?

Hmm, I'd rather not make this differ that much from the FF implementation. I'm personally not too happy that we keep the engines around on the browser, but I really don't want to diverge too much from the FF implementation.

> >+              menuitem.setAttribute("selected", "true");
> [I wonder why this uses selected and not default.]

I can't tell you, that's just code I ported. But if it's no real problem, I#d rather leave it and have you file a followup to investigate. ;-)

> >+            if ((aEvent && aEvent.altKey) ^ newTabPref)
> Alt key?

Apparently, yes. Is that strange?

> >+            // Initially the panel used for the searchbar (PopupAutoComplete
> >+            // in browser.xul) is hidden to avoid impacting startup / new
> Not sure it's a good idea to use PopupAutoComplete?

Why not? Doesn't seem to be a problem in Firefox, AFAIK.

> >+@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> >+
> >+.searchbar-textbox {
> >+  -moz-binding: url("chrome://communicator/content/search/search.xml#searchbar-textbox");
> >+}
> >+
> >+.searchbar-engine-button {
> >+  -moz-user-focus: none;
> >+}
> Hardly seems worth a separate file, might as well go in browser/navigator.css

Hmm, in that case, I wonder if search.xml as the only user of it should live in browser/ instead of common/ as I don't feel it's a good idea to have the chrome://communicator/ binding load navigator.css.
Left this unchanged because of that open question.

> [What happened here?]

I aligned the few entries there to all have sane spacing.

> >diff --git a/suite/themes/classic/jar.mn b/suite/themes/classic/jar.mn
> Any chance of Modern changes?

Hmm, yes, need to do those as well.

(In reply to comment #34)
> Comment on attachment 513915 [details] [diff] [review]
> v1: add optional search box
> 
> Sorry it took so long to complete the review, but I cracked the last issue:
> 
> >+      <method name="openPopup">
> >+        <body><![CDATA[
> >+          var popup = this.popup;
> >+          if (!popup.mPopupOpen) {
> >+            // Initially the panel used for the searchbar (PopupAutoComplete
> >+            // in browser.xul) is hidden to avoid impacting startup / new
> >+            // window performance. The base binding's openPopup would normally
> >+            // call the overriden openAutocompletePopup in urlbarBindings.xml's
> >+            // browser-autocomplete-result-popup binding to unhide the popup,
> >+            // but since we're overriding openPopup we need to unhide the panel
> >+            // ourselves.
> This is completely untrue in our case. In fact, it breaks opening the popup.

Actually, this seems to be completely untrue for Firefox as well, as they have a different popup for the urlbar as well and we're both using this one only for satchel and this search bar.
I still don't know how to resolve the issue there, you seem to know but not tell here. ;-)


.autocomplete-history-dropmarker {
823   display: none;
824 }

(In reply to comment #28)
> The second problem was that the history dropmarker on the search box appears to
> do nothing. Perhaps it doesn't belong there at all?

The styles in xul.css that invoke autocomplete binding are more elegant for the toolkit variant and only show a dropmarker when enablehistory="true" is set. We need to hide it here manually, unfortunately.

> The third problem is that there's a huge gap between the search engine logo and
> the search text. I think there's supposed to be a dropmarker there.

Right, I had forgotten to add that image - you even commented on the CSS that should show it. ;-)

> There appears to be a problem with the "Search the Web" command; a bug in the
> existing code isn't focusing the search bar correctly.

Good catch. Apparently this.searchBar and searchBar are not the same. ;-)

> I expected to be able to change the keyword for an engine by double-clicking.

I guess that can be done in a followup. Also doesn't work in Firefox, btw.

(In reply to comment #30)
> Hmm, the feed icon appearing and disappearing flexes the search box annoyingly.

Can we do anything reasonable against that?

(In reply to comment #33)
> 3. google.xml doesn't work anyway because it sniffs for client=firefox

We should fix that, we can use the client=firefox for search suggestions only, I'd hope, without triggering the ad income sharing for the actual search results.
Comment 36 Robert Kaiser (not working on stability any more) 2011-03-05 13:43:06 PST
Created attachment 517175 [details] [diff] [review]
v2: address all comments I could address

Here's a vastly updated patch for all comments I could address. The suggestion popup still comes up empty, need Neil's help for that. I've integrated the FF patches I taked about in the last comment, converted engine manager to use Services.jsm, and also added the strings for the followup bug for adding the manager to sidebar and prefs so that the work there can be concluded even after the string freeze.
Comment 37 Robert Kaiser (not working on stability any more) 2011-03-05 16:35:34 PST
Created attachment 517196 [details] [diff] [review]
v2.1: also fix the search suggestion/history popup

Ah, found the solution for the search suggestion/history popup - just remove the whole openPopup and let the default one do its job. ;-)
Comment 38 Robert Kaiser (not working on stability any more) 2011-03-05 17:26:20 PST
Created attachment 517208 [details] [diff] [review]
v2.2: style "suggestions" comment and add modern support

Sorry, for yet another patch in short succession, but I forgot to actually add Modern support, which is in this one now, and I also found that the "suggestions" comment in the autocomplete popup should be styled, so I copied what Firefox has on that as well.
Comment 39 Philip Chee 2011-03-05 21:06:32 PST
> Hrm, sorry, I have no idea how to use .bind or what it does.

See Bug 599833 Add "Paste and Go" to the context menu of the URLbar) to see how I used .bind() to simplify implementing Paste&Go when porting Bug 492544 (Paste And Search). We didn't have a search bar then so I didn't do that part.
Comment 40 neil@parkwaycc.co.uk 2011-03-06 04:10:09 PST
(In reply to comment #35)
> > Should be = {}; also new0 is a weird name, why not call it clone?
> It's not new0, it's newO, but still, at least newObj would have been clearer -
> used clonedObj though.
Ah, must have been the font I was using. But Obj is better anyway.

> (In reply to comment #32)
> > >+        setTimeout(function (a) { a.init(); }, 0, this);
> > [I wonder why this is done on a timeout. Anyway, we can now use .bind(this)]
> Hrm, sorry, I have no idea how to use .bind or what it does.
Maybe Ratty can address those in a follow-up patch.

> > >+          return currentEngine || {name:"", uri:null};
> > Nit: nicer formatting on the dummy object.
> Are spaces after the field names enough?
After the colons, you mean? Might want spaces inside the {}s too.

> > >+            if ((aEvent && aEvent.altKey) ^ newTabPref)
> > Alt key?
> Apparently, yes. Is that strange?
Hmm, well Ctrl switches a "current" link into opening a tab or window, although it's not a toggle.

> > >+            // Initially the panel used for the searchbar (PopupAutoComplete
> > >+            // in browser.xul) is hidden to avoid impacting startup / new
> > Not sure it's a good idea to use PopupAutoComplete?
> Why not? Doesn't seem to be a problem in Firefox, AFAIK.
As you noticed I discovered what the problem really was.

> Left this unchanged because of that open question.
I did say it felt very tabbrowsery ;-)

> > [What happened here?]
> I aligned the few entries there to all have sane spacing.
Strangely enough I noticed it when I read the latest patch ;-)

> (In reply to comment #28)
> > The second problem was that the history dropmarker on the search box appears to
> > do nothing. Perhaps it doesn't belong there at all?
> The styles in xul.css that invoke autocomplete binding are more elegant for the
> toolkit variant and only show a dropmarker when enablehistory="true" is set. We
> need to hide it here manually, unfortunately.
xpfe autocomplete uses disablehistory="true" to hide the dropmarker instead.
(via xbl:inherits="hidden=disablehistory")

> (In reply to comment #30)
> > Hmm, the feed icon appearing and disappearing flexes the search box annoyingly.
> Can we do anything reasonable against that?
Maybe setting a "default" width on the urlbar will work. (Presumably it should be 4 times the "default" width of the search bar.)

> (In reply to comment #33)
> > 3. google.xml doesn't work anyway because it sniffs for client=firefox
> We should fix that, we can use the client=firefox for search suggestions only,
> I'd hope, without triggering the ad income sharing for the actual search
> results.
I don't suppose this is something we can contact Google about?
Comment 41 Robert Kaiser (not working on stability any more) 2011-03-06 04:58:45 PST
(In reply to comment #39)
> > Hrm, sorry, I have no idea how to use .bind or what it does.
> 
> See Bug 599833 Add "Paste and Go" to the context menu of the URLbar) to see how
> I used .bind() to simplify implementing Paste&Go when porting Bug 492544 (Paste
> And Search). We didn't have a search bar then so I didn't do that part.

Mind taking that in a followup, as Neil suggests?

(In reply to comment #40)
> After the colons, you mean? Might want spaces inside the {}s too.

Ah, OK, did only "after the colons" as I thought we usually don't have spaces inside the {} for such objects.

> > > >+            if ((aEvent && aEvent.altKey) ^ newTabPref)
> > > Alt key?
> > Apparently, yes. Is that strange?
> Hmm, well Ctrl switches a "current" link into opening a tab or window, although
> it's not a toggle.

True, though I think it makes sense to stay compatible with any documentation that might be out there on how FF does it instead of adding another undocumented difference (and we don't have good support docs of almost anything on the web, where most people look for that).

> > Left this unchanged because of that open question.
> I did say it felt very tabbrowsery ;-)

Does that mean you want to to go through the pain of moving it "elsewhere" or can I (please) leave it as it is?

> xpfe autocomplete uses disablehistory="true" to hide the dropmarker instead.
> (via xbl:inherits="hidden=disablehistory")

Ah, hrm, not helpful to have the two implementations differ that way. :(
Tell me if I should switch from the CSS approach I have in the latest patch to using this attribute, though.

> Maybe setting a "default" width on the urlbar will work. (Presumably it should
> be 4 times the "default" width of the search bar.)

Can try that. I also noticed that Firefox has a not visible but usable draggable splitter between urlbar and searchbar, might be worth to file a followup for adding such a thing on our side as well.

> > (In reply to comment #33)
> I don't suppose this is something we can contact Google about?

Feel free to find someone who does, biesi might even be able to find a contact for us there - I just don't want to get into such tasks myself any more.
Comment 42 Philip Chee 2011-03-06 05:57:11 PST
>> Hrm, sorry, I have no idea how to use .bind or what it does.
> Maybe Ratty can address those in a follow-up patch.
Actually my plan is to move the current cut'n'paste code (and the searchbar paste'n'go code) into a toolkit binding so that we can share this between Firefox and SeaMonkey - and Thunderbird as well. I think Messaging Labs now has a openSearch bar widget for Thunderbird that will open search results in a content tab.

>> Hardly seems worth a separate file, might as well go in browser/navigator.css

> Hmm, in that case, I wonder if search.xml as the only user of it should live in
> browser/ instead of common/ as I don't feel it's a good idea to have the
> chrome://communicator/ binding load navigator.css.
I think this should live in common/ in case we want something like Thunderbirds openSearch.
Comment 43 neil@parkwaycc.co.uk 2011-03-06 07:50:08 PST
(In reply to comment #32)
>(From update of attachment 513915 [details] [diff] [review])
>>+      <handler event="DOMMouseScroll"
>>+               phase="capturing"
>>+#ifdef XP_MACOSX
>>+               action="if (event.metaKey) this.selectEngine(event, (event.detail > 0));"/>
>>+#else
>>+               action="if (event.ctrlKey) this.selectEngine(event, (event.detail > 0));"/>
>>+#endif
>[Note to self: check whether modifiers="accel" suffices.]
Sadly it does not, due to an XBL bug :-(
Comment 44 neil@parkwaycc.co.uk 2011-03-06 09:44:07 PST
(In reply to comment #43)
>(In reply to comment #32)
>>(From update of attachment 513915 [details] [diff] [review])
>>>+      <handler event="DOMMouseScroll"
>>>+               phase="capturing"
>>>+#ifdef XP_MACOSX
>>>+               action="if (event.metaKey) this.selectEngine(event, (event.detail > 0));"/>
>>>+#else
>>>+               action="if (event.ctrlKey) this.selectEngine(event, (event.detail > 0));"/>
>>>+#endif
>>[Note to self: check whether modifiers="accel" suffices.]
>Sadly it does not, due to an XBL bug :-(
Filed bug 639338.
Comment 45 neil@parkwaycc.co.uk 2011-03-06 14:34:15 PST
Comment on attachment 517208 [details] [diff] [review]
v2.2: style "suggestions" comment and add modern support

>+    var iconURL = getBrowser().buildFavIconString(targetDoc.documentURIObject);
I confused you here by misquoting two lines instead of one, and I only meant you to change the second line. Sorry about that.

>+    var where = newWindowPref == 3 ? "tab" : "window";
Wouldn't "tabfocused" be better?

>+    var regionBundle = document.getElementById("bundle_browser_region");
Not used?

>+    <!-- for search and content formfill/pw manager -->
IIRC they call it login manager these days

>+  init: function engineManager_init() {
..
>+  onunload: function engineManager_onunload() {
IIRC we normally use load/unload or init/destroy

>+      aEngine.QueryInterface(Ci.nsISearchEngine)
Nit: missing ;

>+      case "engine-removed":
>+      case "engine-current":
>+        // Not relevant
>+        return;
Hardly worth mentioning then ;-) Maybe switch to break; for consistency?

>+  onCancel: function engineManager_onCancel() {
>+  },
This can be removed completely. (Default cancel action is to close anyway.)

>+    gEngineView.selection.select(Math.min(index, gEngineView.lastIndex));
>+    gEngineView.ensureRowIsVisible(Math.min(index, gEngineView.lastIndex));
[Could use gEngineView.currentIndex here since select() sets it.]

>+    while (Services.prompt.prompt(window, title, msg, alias, null, { })) {
[(Picked on this one, but there are several) I prefer {} for empty objects.]

>+        prompt.alert(window, dtitle, (eduplicate) ? emsg : bmsg);
Nit: don't need ()s around eduplicate

>+    return (sourceIndex != -1 &&
>+            sourceIndex != targetIndex &&
>+            sourceIndex != (targetIndex + orientation));
[The outer ()s aren't strictly necessary.]

>+  <keyset id="engineManagerKeyset">
>+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
Might need a second key for backspace for the Mac?

>+<bindings id="SearchBindings"
>+      xmlns="http://www.mozilla.org/xbl"
>+      xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+      xmlns:xbl="http://www.mozilla.org/xbl">
[Odd indentation.]

>+                   maxrows="10"
[Oops, this doesn't work. I can fix it in xpfe autocomplete.xml in mozilla...]

>+                   xbl:inherits="disabled,disableautocomplete,searchengine,src,newlines">
[Do we really need to inherit all of these?]

>+          <xul:button class="plain searchbar-engine-button"
>+                      type="menu"
>+                      anonid="searchbar-engine-button">
>+            <xul:image class="searchbar-engine-image" xbl:inherits="src"/>
>+            <xul:image class="searchbar-dropmarker-image"/>
[Not only does a menu button already come with two images, but the dropmarker image is already correct (well, if you exclude that blue highlight, which I can live without, and the fact that Modern is using a gif composited against a typical toolbar background instead of a translucent png... look at DOM Inspector's panel switching buttons to see what they would look like).]
I did notice one issue and that is that clicking on the button causes the search field to lose focus. One workaround for that is to use a toolbarbutton instead of a button.

>+                     tooltiptext="&searchEndCap.label;" />
Nit: don't need space before /

>+    <implementation implements="nsIObserver">
>+
Nit: content and handlers don't have extra blank lines at the start and end.

>+      <field name="searchButton">document.getAnonymousElementByAttribute(this,
>+          "anonid", "searchbar-engine-button");</field>
Lonely ;-)

>+      <property name="textbox" readonly="true"
>+                onget="return this._textbox;"/>
>+
>+      <property name="searchService" readonly="true">
>+        <getter><![CDATA[
>+          return this._searchService;
>+        ]]></getter>
>+      </property>
[Could use onget.]

>+          this._textbox.label = text;
I don't think this property exists.

>+            if ((aEvent && aEvent.altKey) ^ newTabPref)
[Still not convinced Alt is the right key for this.]

>+              where = "tab";
tabfocused again perhaps?

>+          var ps2 = Components.classes["@mozilla.org/preferences-service;1"]
>+                              .getService(Components.interfaces.nsIPrefBranch2);
>+          ps2.removeObserver("browser.search.suggest.enabled", this);
[Nit: s/ps2/pb2/g, there's no nsIPrefService2 interface ;-)]
Better still, make _prefBranch use nsIPrefBranch2 and use that instead.

>+          // Add items to context menu and attach controller to handle them
Ideally this would work the same way that Paste & Go does. Maybe Ratty could be persuaded to port the Paste & Go code in a followup. (And he could roll in the .bind changes too while he's there!)

>+.autocomplete-history-dropmarker {
>+  display: none;
>+}
Use disablehistory="true" instead.

>+#engineList treechildren::-moz-tree-image(engineName) {
>+  -moz-margin-end: 4px;
>+  -moz-margin-start: 1px;
>+  width: 16px;
>+  height: 16px;
>+}
>+
>+#engineList treechildren::-moz-tree-row {
>+  height: 20px !important;
>+}
Use #engineChildren instead of #engineList treechildren.
Also, not sure that the height needs to be !important, but didn't test.

>+.autocomplete-textbox-container {
>+  -moz-box-align: stretch;
>+}
I don't see the point of this.

>+.searchbar-engine-image {
>+  height: 16px;
>+  width: 16px;
>+  list-style-image: url("chrome://global/skin/icons/folder-item.png");
[You never normally see this image because you get the favicon instead right?]

>+.searchbar-engine-button {
>+  min-width: 0;
>+  -moz-padding-end: 2px;
>+  -moz-box-align: center;
>+  background: none;
>+}
background-color: transparent;
[Note that plain toolbarbuttons are already transparent, apparently.]
[I'm also not sure you need the others; in particular the align isn't needed if you remove the stretch on the container.]

>+.searchbar-dropmarker-image {
>+  list-style-image: url("chrome://communicator/skin/search/mainwindow-dropdown-arrow.png");
>+  -moz-image-region: rect(0, 13px, 11px, 0);
>+}
>+
>+.searchbar-engine-button[open="true"] > .searchbar-dropmarker-image {
>+  -moz-image-region: rect(0, 26px, 11px, 13px);
>+}
I quite like the default dropmarker image. Maybe I'll switch it over in a followup.

>+.search-go-container {
>+  -moz-box-align: center;
>+  /* add margins to match urlbar height that includes dropmarker */
>+  margin: 3px 0;
With the patch as it is, I find that the height is 6px more than the URLbar, although interestingly removing this margin only shrinks the height by 5px.
[Don't need the align if you remove the stretch on the container.]

>+.search-go-button {
>+  padding: 1px;
>+  -moz-padding-end: 2px;
The search button in a normal textbox normally has no padding, doesn't it?

>+.searchbar-engine-menuitem[selected="true"] > .menu-iconic-text {
>+  font-weight: bold;
>+}
[I forgot to look into this. Maybe I'll do it as a followup.]

>+.autocomplete-treebody::-moz-tree-cell-text(treecolAutoCompleteComment) {
>+  color: GrayText;
[Nice effect. Why have we never had it before?]

>+.searchbar-textbox {
>+}
???

>+.autocomplete-treebody::-moz-tree-cell(suggesthint) {
>+  border-top: 1px solid GrayText;
[How do I see this?]

>diff --git a/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png b/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png
This file looked different. Was it supposed to?
Comment 46 Robert Kaiser (not working on stability any more) 2011-03-06 16:59:02 PST
(In reply to comment #45)
> >+    var where = newWindowPref == 3 ? "tab" : "window";
> Wouldn't "tabfocused" be better?

Whatever you like better. :)

> >+    var regionBundle = document.getElementById("bundle_browser_region");
> Not used?

Good catch, same on the FF side.

> >+    <!-- for search and content formfill/pw manager -->
> IIRC they call it login manager these days

Internally, yes, in UI it's password manager. I left the comment this way as that's the same comment as in FF, so it's easier to compare code. I guess devs understand both well enough. ;-)

> >+      case "engine-removed":
> >+      case "engine-current":
> >+        // Not relevant
> >+        return;
> Hardly worth mentioning then ;-) Maybe switch to break; for consistency?

So, should I remove them or switch to break? Done the latter for now.

> >+  <keyset id="engineManagerKeyset">
> >+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
> Might need a second key for backspace for the Mac?

Firefox gets away with not having it, and they have way more Mac testing than we do.

> >+                   maxrows="10"
> [Oops, this doesn't work. I can fix it in xpfe autocomplete.xml in mozilla...]

Please do so!

> >+                   xbl:inherits="disabled,disableautocomplete,searchengine,src,newlines">
> [Do we really need to inherit all of these?]

I'd rather be consistent with FF in this.

> >+          <xul:button class="plain searchbar-engine-button"
> >+                      type="menu"
> >+                      anonid="searchbar-engine-button">
> >+            <xul:image class="searchbar-engine-image" xbl:inherits="src"/>
> >+            <xul:image class="searchbar-dropmarker-image"/>
> [Not only does a menu button already come with two images, but the dropmarker
> image is already correct (well, if you exclude that blue highlight, which I can
> live without, and the fact that Modern is using a gif composited against a
> typical toolbar background instead of a translucent png... look at DOM
> Inspector's panel switching buttons to see what they would look like).]

I actually like the highlight, esp. as there's not much of an indicator that this does anything without that highlight.

> I did notice one issue and that is that clicking on the button causes the
> search field to lose focus. One workaround for that is to use a toolbarbutton
> instead of a button.

If we do any such break with FF compat, please do it in a followup, ideally once I leave the project.

> >+      <field name="searchButton">document.getAnonymousElementByAttribute(this,
> >+          "anonid", "searchbar-engine-button");</field>
> Lonely ;-)

No idea what you mean with that.

> >+          this._textbox.label = text;
> I don't think this property exists.

https://developer.mozilla.org/en/XUL/textbox#a-textbox.label says it does.

> >+          // Add items to context menu and attach controller to handle them
> Ideally this would work the same way that Paste & Go does. Maybe Ratty could be
> persuaded to port the Paste & Go code in a followup. (And he could roll in the
> .bind changes too while he's there!)

Actually, comment #42 sounds like he intends to do something like that.

> >+.searchbar-engine-image {
> >+  height: 16px;
> >+  width: 16px;
> >+  list-style-image: url("chrome://global/skin/icons/folder-item.png");
> [You never normally see this image because you get the favicon instead right?]

Right, this is just the fallback.

> >+.searchbar-dropmarker-image {
> >+  list-style-image: url("chrome://communicator/skin/search/mainwindow-dropdown-arrow.png");
> >+  -moz-image-region: rect(0, 13px, 11px, 0);
> >+}
> >+
> >+.searchbar-engine-button[open="true"] > .searchbar-dropmarker-image {
> >+  -moz-image-region: rect(0, 26px, 11px, 13px);
> >+}
> I quite like the default dropmarker image. Maybe I'll switch it over in a
> followup.

I quite like the highlight.

> >+.search-go-container {
> >+  -moz-box-align: center;
> >+  /* add margins to match urlbar height that includes dropmarker */
> >+  margin: 3px 0;
> With the patch as it is, I find that the height is 6px more than the URLbar,
> although interestingly removing this margin only shrinks the height by 5px.
> [Don't need the align if you remove the stretch on the container.]

I'll remove that hack and leave it look ugly (a few pixel shorter than the urlbar) on Linux then. Who cares about beaty in SeaMonkey land anyhow.

> >+.search-go-button {
> >+  padding: 1px;
> >+  -moz-padding-end: 2px;
> The search button in a normal textbox normally has no padding, doesn't it?

No idea, just looked better for me this way.

> >+.autocomplete-treebody::-moz-tree-cell-text(treecolAutoCompleteComment) {
> >+  color: GrayText;
> [Nice effect. Why have we never had it before?]

No idea, I just saw it was there in Firefox when investigating this and thought it would be a good idea to port it along. ;-)

> >+.searchbar-textbox {
> >+}
> ???

If we don't need anything for it here, it can go. Just didn't want to search for it if I may need it.

> >+.autocomplete-treebody::-moz-tree-cell(suggesthint) {
> >+  border-top: 1px solid GrayText;
> [How do I see this?]

We save a history of items you already searched for, and show that for any engine, suggestions or not. When you have such entries matching and suggestions in addition, this border separates them.

> >diff --git a/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png b/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png
> This file looked different. Was it supposed to?

Looked different than what? It's supposed to differ from the classic one by the highlight matching the Modern main navigation buttons' highlight color in hue.
Comment 47 Robert Kaiser (not working on stability any more) 2011-03-06 17:20:39 PST
(In reply to comment #45)
> >+.autocomplete-history-dropmarker {
> >+  display: none;
> >+}
> Use disablehistory="true" instead.

rotfl, that doesn't work. Reverting the patch to this approach, may someone else find out what's up fix it, I don't think I care enough any more.
Comment 48 Robert Kaiser (not working on stability any more) 2011-03-06 17:23:46 PST
Created attachment 517324 [details] [diff] [review]
v2.3: address another round of comments

I think this patch is as far as I'm willing to take this, as I'm entering a deeply frustrated state once again - though usually with those porting patches, I ended up there when it was almost done, so let's hope that's the case here as well.
In any case, I addressed all comments I could from the last round, hoping this passes as I won't be able to address anything than a couple of nits withing the next probably two weeks and I'd hope we'll be in feature and string freeze by then.
Comment 49 neil@parkwaycc.co.uk 2011-03-07 01:25:33 PST
(In reply to comment #46)
> (In reply to comment #45)
> > >+      case "engine-removed":
> > >+      case "engine-current":
> > >+        // Not relevant
> > >+        return;
> > Hardly worth mentioning then ;-) Maybe switch to break; for consistency?
> So, should I remove them or switch to break? Done the latter for now.
I left the decision up to you.

> > >+  <keyset id="engineManagerKeyset">
> > >+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
> > Might need a second key for backspace for the Mac?
> Firefox gets away with not having it, and they have way more Mac testing than
> we do.
Well, let's see whether stefanh files a bug ;-)

> > >+                   xbl:inherits="disabled,disableautocomplete,searchengine,src,newlines">
> > [Do we really need to inherit all of these?]
> I'd rather be consistent with FF in this.
So, removing bogus lines is OK, but removing unnecessary words isn't?

> I actually like the highlight, esp. as there's not much of an indicator that
> this does anything without that highlight.
I'll remember to address that concern when I investigate it.

> > I did notice one issue and that is that clicking on the button causes the
> > search field to lose focus. One workaround for that is to use a toolbarbutton
> > instead of a button.
> If we do any such break with FF compat, please do it in a followup, ideally
> once I leave the project.
I don't see why we have to have compat with Fx bugs.

> > >+      <field name="searchButton">document.getAnonymousElementByAttribute(this,
> > >+          "anonid", "searchbar-engine-button");</field>
> > Lonely ;-)
> No idea what you mean with that.
The other fields were all in their own group.

> > >+          this._textbox.label = text;
> > I don't think this property exists.
> https://developer.mozilla.org/en/XUL/textbox#a-textbox.label says it does.
Ah, I was confused because your textbox doesn't actually have a label element labelling it. So setting its label has no effect.

> > >+.search-go-container {
> > >+  -moz-box-align: center;
> > >+  /* add margins to match urlbar height that includes dropmarker */
> > >+  margin: 3px 0;
> > With the patch as it is, I find that the height is 6px more than the URLbar,
> > although interestingly removing this margin only shrinks the height by 5px.
> > [Don't need the align if you remove the stretch on the container.]
> I'll remove that hack and leave it look ugly (a few pixel shorter than the
> urlbar) on Linux then. Who cares about beauty in SeaMonkey land anyhow.
One problem is that the urlbar has a dropmarker, which affects its height.

> > >+.autocomplete-treebody::-moz-tree-cell-text(treecolAutoCompleteComment) {
> > >+  color: GrayText;
> > [Nice effect. Why have we never had it before?]
> No idea, I just saw it was there in Firefox when investigating this and thought
> it would be a good idea to port it along. ;-)
I'm all for compat with Fx features :-)

> > >+.autocomplete-treebody::-moz-tree-cell(suggesthint) {
> > >+  border-top: 1px solid GrayText;
> > [How do I see this?]
> We save a history of items you already searched for, and show that for any
> engine, suggestions or not. When you have such entries matching and suggestions
> in addition, this border separates them.
Aha, I need to go back and test that.

> > This file looked different. Was it supposed to?
> Looked different than what? It's supposed to differ from the classic one by the
> highlight matching the Modern main navigation buttons' highlight color in hue.
I only tried patch using Modern very briefly, since the CSS looked reasonable, but something struck me as being different. Let me check closely.

(In reply to comment #47)
> (In reply to comment #45)
> > >+.autocomplete-history-dropmarker {
> > >+  display: none;
> > >+}
> > Use disablehistory="true" instead.
> rotfl, that doesn't work. Reverting the patch to this approach, may someone
> else find out what's up fix it, I don't think I care enough any more.
Ah yes, you're extending the toolkit autocomplete binding, but we're getting the xpfe autocomplete styles, which is why things don't work out correctly. Obviously something else for me to follow up on at a later date.
Comment 50 neil@parkwaycc.co.uk 2011-03-07 01:39:01 PST
(In reply to comment #46)
> Who cares about beauty in SeaMonkey land anyhow.
IMHO it's much better to get the basic functionality landed first. Obviously we can look to what styles Fx is using to get an idea as to what we might need but our UI isn't the same as Fx and some of their styles may not be relevant to us.
Comment 51 neil@parkwaycc.co.uk 2011-03-07 03:03:34 PST
(In reply to comment #46)
> (In reply to comment #45)
> > >diff --git a/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png b/suite/themes/modern/communicator/search/mainwindow-dropdown-arrow.png
> > This file looked different. Was it supposed to?
> Looked different than what? It's supposed to differ from the classic one by the
> highlight matching the Modern main navigation buttons' highlight color in hue.
Ah, so the fact that you ended up with a 8-bit PNG instead of a 32-bit PNG?
Comment 52 neil@parkwaycc.co.uk 2011-03-07 03:45:47 PST
(In reply to comment #46)
> Who cares about beauty in SeaMonkey land anyhow.
Modern:
URL bar: 24px
"Standard" autocomplete (DOM Inspector): 22px
Search textbox (History): 22px
Search bar (latest patch): 23px

I did notice that the Search bar uses the Search button's icon; possibly the search textbox icon would be a better choice?

Classic (Windows/Linux):
URL bar: 24px/33px(!)
"Standard" autocomplete (DOM Inspector): 20(25)px/22(27)px*
Search textbox (History): 25px/27px
Search bar (latest patch): 25px/24px

>>>+.search-go-button {
>>>+  padding: 1px;
>>>+  -moz-padding-end: 2px;
>>The search button in a normal textbox normally has no padding, doesn't it?
>No idea, just looked better for me this way.
What's happening here is that the autocomplete normally has less padding (which explains why it's only 20px instead of 25px - at least, I think that DOM Inspector could probably do with class="padded" on its autocomplete). But in the case of the search bar, the padding on the button messes things up. So in the interim, you could restore the end padding, until I fix things properly.

Interestingly with my toolbarbutton changes on Linux, I end up with a height of 27px, which matches search textboxes :-)

*These measurements were taken with a fix for a regression that you caused :-(
Comment 53 Robert Kaiser (not working on stability any more) 2011-03-07 18:11:12 PST
(In reply to comment #49)
> > > >+                   xbl:inherits="disabled,disableautocomplete,searchengine,src,newlines">
> > > [Do we really need to inherit all of these?]
> > I'd rather be consistent with FF in this.
> So, removing bogus lines is OK, but removing unnecessary words isn't?

Removing things that get inherited IMHO sounds like breaking compat, and I'm not willing to do that. Feel free to do it once I'm off the project, but I don't want to be a part of it.

> > > I did notice one issue and that is that clicking on the button causes the
> > > search field to lose focus. One workaround for that is to use a toolbarbutton
> > > instead of a button.
> > If we do any such break with FF compat, please do it in a followup, ideally
> > once I leave the project.
> I don't see why we have to have compat with Fx bugs.

Well, IMHO, a toolbarbutton that is not a (visually) direct child of a toolbar is a bug. Actually, I think even having a toolbarbutton in the first place is bad design, as a button on a toolbar just should behave like a toolbar button and other buttons should behave like other buttons, without needing to create different tags for them - but that goes off topic for this bug.

> > > >+          this._textbox.label = text;
> > > I don't think this property exists.
> > https://developer.mozilla.org/en/XUL/textbox#a-textbox.label says it does.
> Ah, I was confused because your textbox doesn't actually have a label element
> labelling it. So setting its label has no effect.

Sounds like a bug if we make a11y worse, which is how I understand it.

> One problem is that the urlbar has a dropmarker, which affects its height.

That why UI added those borders in the first place, that unfortunately only seem to have a merit on Linux. I guess I'll let someone else deal with that.

> I only tried patch using Modern very briefly, since the CSS looked reasonable,
> but something struck me as being different. Let me check closely.

Well, one thing is not ideal in Modern and that is the fancy border it has around the urlbar, which doesn't include the searchbar. I think that warrants to go into a followup. though.

> Ah yes, you're extending the toolkit autocomplete binding, but we're getting
> the xpfe autocomplete styles, which is why things don't work out correctly.
> Obviously something else for me to follow up on at a later date.

Sounds like it, yes. I think we should try to get into a position where we have the toolkit style enabled in xul.css and our urlbar autocomplete comes with its own style that doesn't conflict the toolkit one. But that's a different bug as well. ;-)

(In reply to comment #51)
> Ah, so the fact that you ended up with a 8-bit PNG instead of a 32-bit PNG?

Might be a result of running optipng on the image. Should I use an unoptimized one instead?

(In reply to comment #52)
> *These measurements were taken with a fix for a regression that you caused :-(

Not sure which one that is, but it looks like you agreed above that we can ignore if it looks nice anyhow.
Comment 54 Philip Chee 2011-03-07 19:30:50 PST
> Might be a result of running optipng on the image. Should I use an unoptimized
> one instead?
optipng does take optional parameters:
<http://optipng.sourceforge.net/manual.html>

−nb Do not apply bit depth reduction.
−nc Do not apply color type reduction.
−np Do not apply palette reduction.
−nx Do not apply any lossless image reduction: enable the options −nb, −nc and −np.
...etc...
Comment 55 neil@parkwaycc.co.uk 2011-03-08 06:37:00 PST
(In reply to comment #53)
> Removing things that get inherited IMHO sounds like breaking compat
Fair enough.

> > I don't see why we have to have compat with Fx bugs.
> Well, IMHO, a toolbarbutton that is not a (visually) direct child of a toolbar
> is a bug. Actually, I think even having a toolbarbutton in the first place is
> bad design, as a button on a toolbar just should behave like a toolbar button
> and other buttons should behave like other buttons, without needing to create
> different tags for them - but that goes off topic for this bug.
It was only a suggestion (although as it turns out toolbarbuttons seem to have other benefits, but it may be possible to stick with a button.)

> > Ah, I was confused because your textbox doesn't actually have a label element
> > labelling it. So setting its label has no effect.
> Sounds like a bug if we make a11y worse, which is how I understand it.
It's probably a hangover from before the placeholder property, when a11y had nothing else to go on.

> > One problem is that the urlbar has a dropmarker, which affects its height.
> That why UI added those borders in the first place, that unfortunately only
> seem to have a merit on Linux. I guess I'll let someone else deal with that.
It's weird how Linux seems to have such a large dropmarker.

> > I only tried patch using Modern very briefly, since the CSS looked reasonable,
> > but something struck me as being different. Let me check closely.
> Well, one thing is not ideal in Modern and that is the fancy border it has
> around the urlbar, which doesn't include the searchbar. I think that warrants
> to go into a followup. though.
Most certainly.

> > Ah yes, you're extending the toolkit autocomplete binding, but we're getting
> > the xpfe autocomplete styles, which is why things don't work out correctly.
> > Obviously something else for me to follow up on at a later date.
> Sounds like it, yes. I think we should try to get into a position where we have
> the toolkit style enabled in xul.css and our urlbar autocomplete comes with its
> own style that doesn't conflict the toolkit one. But that's a different bug as
> well. ;-)
Ideally I'd like to wean Thunderbird off it first. (Remember that we use our autocomplete in more places than just the URLbar.)

> (In reply to comment #51)
> > Ah, so the fact that you ended up with a 8-bit PNG instead of a 32-bit PNG?
> Might be a result of running optipng on the image. Should I use an unoptimized
> one instead?
As Ratty pointed out, you should disable reductions on 32-bit PNGs.

> (In reply to comment #52)
> > *These measurements were taken with a fix for a regression that you caused :-(
> Not sure which one that is, but it looks like you agreed above that we can
> ignore if it looks nice anyhow.
I filed bug 639455. (And before you ask, it wasn't your fault; it's the old "extensions break if they were incorrectly relying on internals" issue.)
Comment 56 neil@parkwaycc.co.uk 2011-03-08 06:55:44 PST
Comment on attachment 517324 [details] [diff] [review]
v2.3: address another round of comments

OK, so we'll take this patch for now, and address other issues in followups, except for two things:

1. Obviously, we need that PNG fixed.

2. I notice that for Classic you used the same search icon as a search box. But for Modern you used the same search icon as the search button. I think it would also look better if it used the same search icon as a search box.
Comment 57 Robert Kaiser (not working on stability any more) 2011-03-09 16:58:00 PST
(In reply to comment #55)
> It's weird how Linux seems to have such a large dropmarker.

I agree, perhaps we should just change that for the urlbar.

> Ideally I'd like to wean Thunderbird off it first. (Remember that we use our
> autocomplete in more places than just the URLbar.)

I know we need the mailnews side (esp. LDAP) fixed first - I was more talking about the situation we should get to in the future, where our urlbar should probably the only thing left not using straight-out toolkit autocomplete.


(In reply to comment #56)
> OK, so we'll take this patch for now, and address other issues in followups,
> except for two things:

I hope I'll catch all issues to be filed as such followups here - if I don't please file those I'm missing!

> 1. Obviously, we need that PNG fixed.

Opened in GIMP, converted to "RGB" again and re-saved, and that seems to work. :)

> 2. I notice that for Classic you used the same search icon as a search box. But
> for Modern you used the same search icon as the search button. I think it would
> also look better if it used the same search icon as a search box.

Hmm, it seems to me that in Modern the search button and the search boxes actually _do_ use the same icon (I copied this from textbox.css while on Classic, I copied the Firefox styling).
Comment 58 Robert Kaiser (not working on stability any more) 2011-03-09 17:05:07 PST
Created attachment 518233 [details] [diff] [review]
v2.4: fix the last few comments

This contains only those small changes to v2.3:
* moved searchButton field up to the other fields
* removed this._textbox.label line
* re-added .search-go-button -moz-padding-end in default theme
* converted modern mainwindow-dropdown-arrow.png to 32bit

Asking for ui-r here because I couldn't resolve the icon issue because Modern uses the same image for both search button and search textboxes.
Comment 59 Robert Kaiser (not working on stability any more) 2011-03-09 17:29:45 PST
I filed the following bugs due to discussions here: bug 640418, bug 640420, bug 640421, bug 640425, bug 640426.

If there are any more to be filed, please do so.
Comment 60 neil@parkwaycc.co.uk 2011-03-10 00:35:24 PST
(In reply to comment #58)
> Asking for ui-r here because I couldn't resolve the icon issue because Modern
> uses the same image for both search button and search textboxes.
My fault, I was comparing it to an "instant search" textbox, I should have compared it to a "manual search" textbox.
Comment 61 Stefan [:stefanh] 2011-03-10 02:55:57 PST
(In reply to comment #59)
> I filed the following bugs due to discussions here: bug 640418, bug 640420, bug
> 640421, bug 640425, bug 640426.
> 
> If there are any more to be filed, please do so.

Filed bug 640514 for the lack of mac styling.
Comment 62 neil@parkwaycc.co.uk 2011-03-10 03:09:20 PST
(In reply to comment #60)
> (In reply to comment #58)
> > Asking for ui-r here because I couldn't resolve the icon issue because Modern
> > uses the same image for both search button and search textboxes.
> My fault, I was comparing it to an "instant search" textbox, I should have
> compared it to a "manual search" textbox.
Even worse, I was testing it in a build which had a patch that only affected "instant search" textboxes. D'oh!
Comment 63 neil@parkwaycc.co.uk 2011-03-10 12:32:39 PST
Comment on attachment 518233 [details] [diff] [review]
v2.4: fix the last few comments

>+      <field name="_stringBundle">document.getAnonymousElementByAttribute(this,
>+          "anonid", "searchbar-stringbundle");</field>
>+      <field name="_textbox">document.getAnonymousElementByAttribute(this,
>+          "anonid", "searchbar-textbox");</field>
>+      <field name="_popup">document.getAnonymousElementByAttribute(this,
>+          "anonid", "searchbar-popup");</field>
>+      <field name="_searchService">
>+        Components.classes["@mozilla.org/browser/search-service;1"]
>+                  .getService(Components.interfaces.nsIBrowserSearchService);
>+      </field>
>+      <field name="_engines">null</field>
>+
>+      <field name="searchButton">document.getAnonymousElementByAttribute(this,
>+          "anonid", "searchbar-engine-button");</field>
[I was actually hoping to see this next to the other anonymous element fields]
Comment 64 Robert Kaiser (not working on stability any more) 2011-03-10 13:44:26 PST
Landed as http://hg.mozilla.org/comm-central/rev/6609dcc741a7 - this is probably my last major SeaMonkey patch, thanks for all the reviews!
Comment 65 Ian Neal 2011-03-13 17:59:46 PDT
I got when adding/removing the search box from the toolbar:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://communicator/content/search/search.xml ::  :: line 129"  data: no]
Comment 66 Philip Chee 2011-03-13 20:58:23 PDT
> I got when adding/removing the search box from the toolbar:
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> chrome://communicator/content/search/search.xml ::  :: line 129"  data: no]

See Firefox Bug 640970
Comment 67 Philip Chee 2011-03-13 21:04:03 PDT
Possibly a regression from bug 640970
Comment 68 Robert Kaiser (not working on stability any more) 2011-03-14 05:05:53 PDT
(In reply to comment #65)
> I got when adding/removing the search box from the toolbar:
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> chrome://communicator/content/search/search.xml ::  :: line 129"  data: no]

And please file a followup on this, as I'm pretty sure tracking it here in this already huge and already marked fixed bug is a bad idea.
Comment 69 Philip Chee 2011-03-14 05:17:06 PDT
>Possibly a regression from bug 640970
I meant bug 396236 of course.

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