235204 - Web Search (Ctrl+K, etc) should show a search dialog when search bar is disabled

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, enhancement, P1)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Take the search bar off of the toolbar.
2. Ctrl+K.

Result: Nothing happens.

Expected: 
A) Go to http://www.google.com/ (preferred).
or
B) Pop up a dialog (like Ctrl+L does when the address bar is disabled).

Comment 1

[Jesse Ruderman]

This is more important now that there's a menu item, Tools > Web Search,
corresponding to Ctrl+K.  The menu item does nothing if the search bar is disabled.

Comment 2

[Bogdan Stroe]

*** Bug 246775 has been marked as a duplicate of this bug. ***

Comment 3

[Nian Liu(n/a in a long time)]

what about the expected behavior is to show navigatio toolbar and focus on
search bar?

Comment 4

[Nian Liu(n/a in a long time)]

in fact, in currently nightly build, when navigation bar is hidden, press
ctrl+j, nothing happen. however, input keyword you want to search, then press
return. google with search result will show. so i guess show navigation bar is
better.

Comment 5

[Nian Liu(n/a in a long time)]

Attachment: patch

once navigation bar is hidden, ctrl+j will show navigation bar and focus on
search bar

Comment 6

[Radek 'sysKin' Czyz]

The patch will bring up navigation toolbar even if searchbar has been moved
somewhere else.
It will do nothing if searchbar is completely removed.
The navigation bar will never go away after being brought back.

Comment 7

[Nian Liu(n/a in a long time)]

thanks Radek,
so the solution may be
a)if the toolbar contains search bar is not collapsed, focus on search bar
b)if the toolbar contains search bar is collapsed or
     there is no search bar in toolbars,
  go to search engine home

Comment 8

[Nian Liu(n/a in a long time)]

sounds not too hard, i'll see what i can do

Comment 9

[Nian Liu(n/a in a long time)]

Attachment: patch(always open google)

follow comment#1 A

Comment 10

[Nian Liu(n/a in a long time)]

if this isn't enough, i'll make a patch which opens default search engine
homepage. question here is that what if default engine is "find in this page"?
should we popup a dialog instead of going to homepage as other engines?

Comment 11

[Nian Liu(n/a in a long time)]

since current shortcut for web search is ctrl+J instead of ctrl+K, i modify the
summary

Comment 12

[Jesse Ruderman]

The shortcut for Web Search is Ctrl+E.  Ctrl+K and Ctrl+J are still supported on
some platforms for backwards-compatibility.

> question here is that what if default engine is "find in this page"?

"Find in this page" is no longer handled by the search bar.  In branch
nightlies, it is handled by a separate find toolbar.

Comment 13

[Nian Liu(n/a in a long time)]

for comment 12, two things:

1. i can't see ctrl+E as shortcut for web search. is there any official list
about shortcut key used in firefox?

2. i can't see "find in this page" is moved out from search bar

my firefox is 0.9.1+ Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3)
Gecko/20040810. 

Comment 14

[Jesse Ruderman]

> is there any official list about shortcut key used in firefox?

Official: Help > Contents > Keyboard Shortcuts
Unofficial and better: http://texturizer.net/firefox/keyboard.html

> i can't see "find in this page" is moved out from search bar

You're using a trunk build.  Trunk builds don't have the Find toolbar and many
other UI changes that have happened recently.  Please use and write patches for
the aviary branch for now.

Comment 15

[Nian Liu(n/a in a long time)]

thanks, Jesse

the patch seems fine for aviary branch

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Nian, if you could make the patch that opens up the users default search engine,
like you mentioned in comment 10, I believe it would better solve the issue. As
comment 12 points out, the "Find in this page" option no longer applies.

Comment 17

[Jesse Ruderman]

Comment on attachment 155880 [details] [diff] [review]
patch(always open google)

We have a way to go to the default search engine's home page, so you don't have
to hard-code www.google.com.

We use it when you press Enter in the search bar without typing anything:
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#2748

It's implemented here:
http://lxr.mozilla.org/aviarybranch/source/xpfe/browser/resources/content/navig
ator.js#1095

Comment 18

[Jesse Ruderman]

Comment on attachment 155880 [details] [diff] [review]
patch(always open google)

Please use && instead of nested if, and use if/else instead of an early return.

Comment 19

[Nian Liu(n/a in a long time)]

use BrowserSearchInternet() causes a js error: BrowserSearchInternet() not defined.

although i include <script type="application/x-javascript"
src="chrome://navigator/content/navigator.js"/> in browser-scripts.inc, it
doesn't help.

anybody know possible reason?

Comment 20

[Nian Liu(n/a in a long time)]

navigator.js is just a ref for browser.js and is obseleted,  rihgt?

Comment 21

[u88484]

i think this bug kinda went off track here so I'll be specific.

When the search bar is removed, using the menus tools->Web search nothing
happens. There should be a dialog or something. I really think the Web search in
the tools menu is stupid anywayz...no point in it because all it does is move
the caret to the search bar when its on the toolbar. What is the point of that
when it takes more work to naviagte the mouse or even using keyboard to go to
Tools->click Web search. 

Comment 22

[u88484]

sorry was trying to respond to a similar bug about search bar.

i think Bug# 242862 https://bugzilla.mozilla.org/show_bug.cgi?id=242862 is
related to this one. And both are saying that Tools->Web search should do
something or be completly removed. I personally think it should be removed since
there is a search bar or at least 'Web Search' to the 'Go' menu.

Comment 23

[chris hofmann]

if a approved patch comes along this might be considered, but it is getting late
for 1.0

Comment 24

[E Wiebe]

This is all very confusing.  On Linux I thought ctrl-k meant
kill-to-end-of-line.  That is what I always use it for and frankly that is how
it should be set.  ctrl-j should jump to web search.  BUG# 226273 show this as
being resolved yet it still doesn't seem to work. 

Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041001 Firefox/0.10.1 -->
doesn't work as it should

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041022 --> does work
as it should

Can anyone shed some light on this?

Comment 25

[Jesse Ruderman]

[e-mail]: that's off-topic for this bug, but you can find the
answer at
http://kb.mozillazine.org/index.phtml?title=Emacs_Keybindings_(Firefox).

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: Attempt #1

Tries to get the default search engine URL and loads it if it exists. This
should work for most search sites since it's the equivalent of a blank search.
It works fine for the default.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Also, I don't understand the if (!searchBar.parentNode.parentNode.collapsed)
check in the previous patch. When is the toolbar ever collapsed?

Comment 28

[Mike Connor [:mconnor]]

(In reply to comment #27)
> Also, I don't understand the if (!searchBar.parentNode.parentNode.collapsed)
> check in the previous patch. When is the toolbar ever collapsed?

You can hide toobars, of course, so that'd be it.

I'm going to think about this, since I'm wondering if the right fix is to pop a
search dialog instead.  The problem is that if the toolbar is hidden, there's no
way to configure this, and if you're on, say, the dictionary.com search before
its hidden, then what?

There's also the argument that we could just do http://www.google.com/firefox/
since that's a sane, safe option that's more predictable/accurate.

Or, disable the menuitem if the searchbar is hidden, and don't focus anything if
its hidden/removed.

Lots of options, I'll get some feedback later on this.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> I'm going to think about this, since I'm wondering if the right fix is to pop a
> search dialog instead.

That would match the behavior for Ctrl+L with the location bar. What about the
search dialog's behavior? You're saying the search dialog would default to the
last selected entry in the toolbar (eg dictionary.com)? I would tend to think
that if people have the search bar disabled, they're intention is to not use the
various different searches it offers. It would be pretty ridiculous to hide the
search bar, do a search, then unhide it, change engines, and hide it again.

> There's also the argument that we could just do http://www.google.com/firefox/
> since that's a sane, safe option that's more predictable/accurate.

Or, what about this idea... use the user-specified default search! ;)

> Or, disable the menuitem if the searchbar is hidden, and don't focus anything if
> its hidden/removed.

After hearing this, I'm starting to think that popping a dialog that will search
with the default engine is best. It gives the user some notice before changing
the page (in the case where they might not want to) and does whatever "web
search" is defined in the pref (default being google).

Comment 30

[Asaf Romano (gone)]

Dialog like the following would be better IMO:

|-------------------------|
|Search for:              |
| ( |G|                 ) |  <-  Same searchbinding (search.xml)
|                         |
|       (Cancel) (Search) |
|-------------------------|

Comment 31

[Aaron Leventhal]

What's happening with this patch? It's been waiting for review for a month. We
shouldn't let it rot.

Comment 32

[Mike Connor [:mconnor]]

Comment on attachment 169803 [details] [diff] [review]
Attempt #1

I don't like this behaviour.  If someone prefers another search engine, that's
a big annoyance to change it.  And, of course, I'd probably want to use the
Firefox search page if this was acceptable anyway.

I think the dialog is the better option, matching the Ctrl-L behaviour seems
appropriate.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Attachment: In Progress Patch

Working, but I still want to go over it a bit. Testing would be appreciated,
and comments/suggestions are welcome.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch v2

Here's one that actually works...

One issue that isn't addressed in this patch is the hidden toolbar. In that
case, the search bar does not change it's engine to the one that was last used
with the prompt. It's easier to explain with steps.

1) Ensure that the search bar is on the navigation toolbar 
2) Hide the navigation toolbar
3) Use the dialog (Ctrl+K) and change the engine
4) Do a search
5) Unhide the navigation toolbar.

The search bar in this case is set to the same engine as it was previously. In
all other cases, changing the engine in either the dialog or the bar also
changes it in the other.

Comments/suggestions are welcome, and (now that the patch actually works)
testing would be appreciated.

Comment 35

[u88484]

Is this bug resolved or what? I see two patches were uploaded a few weeks ago
but no comments as if this bug was fixed or now or if the patches were even
checked in.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> Is this bug resolved or what?

The bug's STATUS is ASSIGNED, not RESOLVED. The patch hasn't been reviewed,
because it's not ready to be. Please don't comment on bugs unless you have
something to contribute. This isn't the first time you've done it, so it might
be a good idea to refer to the Bugzilla etiquette at
http://bugzilla.mozilla.org/page.cgi?id=etiquette.html to try and avoid further
conflict. Please keep any further correspondence off the bug.

Comment 37

[Mike Connor [:mconnor]]

Let's try to get this, if it doesn't happen we'll disable the menuitem when the
searchbar is hidden, but I'd rather do something useful than nothing at all.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch v3

Here's an updated, cleaner version. This makes Ctrl+K open a search dialog when
the searchbar is hidden, removed, or not visible. I'll post a screenshot of the
dialog in a minute.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Attachment: Screenshot of new dialog

Comment 40

[Asaf Romano (gone)]

Comment on attachment 179436 [details] [diff] [review]
Patch v3

Why aren't you using the normal search binding (search.xml) rather than two
widgets?

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Attachment: Work in progress (diff -w)

Different approach. Uses the search.xml bindings to reuse some code. Still has
some issues, such as the searchbar trying to work while in the palette, but it
should be fully functional. The search engines always stay synced between
toolbar and dialog, and the usual keyboard shortcuts and search history seem to
work fine. Not ready for review, but feedback would be good.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Attachment: A little closer

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Attachment: Same as diff -w

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Attachment: Updated, some fixes

This gets a little closer to it being finished, any comments/suggestions are
appreciated.

Comment 45

[Asa Dotzler [:asa]]

l10n inplications. must have by b4 if we're going to get it into 1.1.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Attachment: More fixes, updated

This makes the dialog modal. Still has issues while in the customize toolbar
palette, and a mysterious problem where the cursor and text sometimes
disappears from the search bar. I will look into it further.

Comment 47

[Asaf Romano (gone)]

Comment on attachment 188371 [details] [diff] [review]
More fixes, updated

IMO, this dialog should be resiable.

>+      <xul:searchbarbase>
>         <xul:button class="searchbar-dropmarker" type="menu"
>-                    popup="_child" xbl:inherits="src">
>+                    popup="_child" xbl:inherits="src"
>+                    anonid="searchbar-button">
>           <xul:menupopup anonid="searchbar-popup" position="after_start"
>                          datasources="rdf:internetsearch"
>                          ref="NC:SearchEngineRoot" 
>-                         oncommand="this.parentNode.parentNode.onEnginePopupCommand(event);"
>-                         onpopupshowing="this.parentNode.parentNode.onEnginePopupShowing(event);"
>-                         onpopuphidden="this.parentNode.parentNode.onEnginePopupHidden(event);">
>+                         oncommand="this.parentNode.parentNode.parentNode.onEnginePopupCommand(event);"
>+                         onpopupshowing="this.parentNode.parentNode.parentNode.onEnginePopupShowing(event);"
>+                         onpopuphidden="this.parentNode.parentNode.parentNode.onEnginePopupHidden(event);"

Using a filed in order to accees the <xul:searchbarbase> would be better than
.parentNode.parentNode.parentNode...


>-      <!-- nsIController -->
>-      <field name="searchbarController" readonly="true"><![CDATA[
>+    <handlers>
>+      <handler event="keypress" keycode="vk_up"

please capitalize the keycodes.


>Index: browser/base/content/searchDialog.js
>===================================================================

>+
>+const ciPLS      = Components.interfaces.nsIPrefLocalizedString;
>+
>+const TABPREF    = 'browser.tabs.opentabfor.searchdialog';
>+const ENGINEPREF = 'browser.search.selectedEngine';

Why are you using ' instead of " (not just here)?


>+function setEnginePref() {
>+  try {
>+    var pls = Components.classes["@mozilla.org/pref-localizedstring;1"]
>+                        .createInstance(ciPLS);
>+    pls.data = gDialog.list.selectedItem.getAttribute('label');
>+
>+    gPref.setComplexValue(ENGINEPREF, ciPLS, pls);
>+  } catch (ex) { }
>+}

Any reason for the |try| block?


>+function onLoad() {
>+
>+  gPref = Components.classes["@mozilla.org/preferences-service;1"]
>+                    .getService(Components.interfaces.nsIPrefBranch);
>+
>+  gDialog.list    = document.getElementById('searchEngineList');
>+  gDialog.input   = document.getElementById('searchInput');
>+  gDialog.newtab  = document.getElementById('searchInNewTab');
>+
>+  // Find a browser window
>+  if ("arguments" in window && window.arguments.length > 0) {
>+    gBrowser = window.arguments[0];
>+  } else {
>+    gBrowser = window.opener;
>+  }

This is a modal dialog, it's should be ok to /just/ use |window.opener|.

>+  gOpenInNewTab = getBoolPref(TABPREF);
>+  gDialog.newtab.setAttribute('checked', gOpenInNewTab);
>+
>+  setTimeout(onAfterLoad, 0);
>+}
>+
>+function onAfterLoad() {
>+  var engines = gDialog.input.getEngine();
>+  var selectedEngine = gPref.getComplexValue(ENGINEPREF, ciPLS).data;
>+
>+  // Add the engines to the list
>+  for (var i=0; i<engines.length; i++) {
>+    var newItem = gDialog.list.appendItem(engines[i].name, engines[i].value);
>+    newItem.setAttribute('class', 'menuitem-iconic');
>+    newItem.setAttribute('src', engines[i].icon);
>+    if (engines[i].name == selectedEngine) {
>+      gDialog.list.selectedItem = newItem;
>+    }

remove single line bracket please.


>+
>+function startSearch() {
>+  gOpenInNewTab  = gDialog.newtab.checked;
>+  var resultURL  = gDialog.input.resultURL;
>+  var searchText = gDialog.input.value;
>+
>+  if (gPref)
>+    gPref.setBoolPref(TABPREF, gOpenInNewTab);
>+
>+  setEnginePref();
>+
>+  // Add the item to the search bar autocomplete history
>+  var HISTSVC = Components.classes["@mozilla.org/satchel/form-history;1"]
>+                          .getService(Components.interfaces.nsIFormHistory);
>+  HISTSVC.addEntry('searchbar-history', searchText);
>+
>+  gBrowser.delayedSearchLoadURL(resultURL, { altKey: gOpenInNewTab });
>+
>+  window.close()

So, I guess you're manually closing the dialog to workaround the same timing
bug mentioned in openLocation.js. If so, please do |return false;| and add the
missing semicolon.


>Index: browser/base/content/searchDialog.xul
>===================================================================

>+
>+<dialog id="searchDialog"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        title="&searchCaption.label;"
>+        windowtype="Browser:searchDialog"

If we're opening this dialog as a modal one, windowtype is not necessary.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #47)
> (From update of attachment 188371 [details] [diff] [review] [edit])
> IMO, this dialog should be resiable.

Was already done, per your comment on IRC.

> Using a filed in order to accees the <xul:searchbarbase> would be better than
> .parentNode.parentNode.parentNode...

I don't understand what you mean. This was there before, and just as ugly, but I
don't see how else I could do it.

> please capitalize the keycodes.

Those were mostly just taken from the previous search.xml. Fixed anyways.

> >Index: browser/base/content/searchDialog.js
> Why are you using ' instead of " (not just here)?

Out of habit, really :). Fixed.

> Any reason for the |try| block?

No, removed.

> >+  // Find a browser window
> >+  if ("arguments" in window && window.arguments.length > 0) {
> >+    gBrowser = window.arguments[0];
> >+  } else {
> >+    gBrowser = window.opener;
> >+  }
> 
> This is a modal dialog, it's should be ok to /just/ use |window.opener|.

Right, forgot to remove that. Fixed.

> >+    if (engines[i].name == selectedEngine) {
> >+      gDialog.list.selectedItem = newItem;
> >+    }
> 
> remove single line bracket please.

Fine :).

> >+  window.close()
> 
> So, I guess you're manually closing the dialog to workaround the same timing
> bug mentioned in openLocation.js. If so, please do |return false;| and add the
> missing semicolon.

Oops, thought I'd double checked that. Fixed.

> >Index: browser/base/content/searchDialog.xul

> >+        windowtype="Browser:searchDialog"

> If we're opening this dialog as a modal one, windowtype is not necessary.

Another thing I forgot to remove :S. Fixed.

Thanks for the quick review. I have an updated patch ready.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Attachment: With Mano's comments addressed

Comment 50

[Worcester12345]

checkin?

Comment 51

[Jaime Mitchell (use [e-mail] for email)]

(In reply to comment #50)
> checkin?

Gavin will do that and request aproval/reviews etc.. when he is ready. 

Please stop making useless comments to various bugs. Please read
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before making any more
comments in any bug.

(sorry to the others for the bug spam)

Comment 52

[Asa Dotzler [:asa]]

We can take this when it's ready if we think it's safe, but this isn't a
regression, and it's not going to block our branching.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Attachment: More progress

Here's the latest patch, after much discussion. I think this is ready for
review. Any testing or comments would be greatly appreciated.

I've posted a Windows build with this patch included at
http://gavinsharp.com/ff/firefox-1.0+.en-US.win32.installer.exe .

Comment 54

[:Gavin Sharp [email: [e-mail]]]

Attachment: Screenshot of latest patch

Comment 55

[Dave Townsend [:mossop]]

Im surprised to see the option in the dialog to open the results in a new tab 
when we don't expose a similar option for the normal search bar.

Would it be more consistant for the dialog to work the same as the toolbar, 
then put the checkbox to change the default behaviour into options and have it 
apply to both the toolbar and dialog?

Comment 56

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #55)
> Would it be more consistant for the dialog to work the same as the toolbar, 
> then put the checkbox to change the default behaviour into options and have it 
> apply to both the toolbar and dialog?

So, IIRC, that's The Thing Entirely. The search bar DOES allow for search
results to be opened in new tabs, if a person Alt+Enters on it, just like in the
URL bar. Now, it'd be nice if the dialog could support that same advanced-user
shortcut, but apparently it can't (or so I've been told) without some minor
rocket science, thus, the checkbox.

In sum: perfectly consistant = good, somewhat consistent = will have to do :)

Speaking of which, the icon for "Add Search Engines ..." should be removed until
it's added to the web search menubar. And before that happens, I'd like a "+" to
be added to the icon to indicate that it's used to "add" search engines.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch 2

With a few nits picked from Mano, and a fixed mistake in setPrefs().

Comment 58

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated

Comment 59

[Mike Connor [:mconnor]]

Comment on attachment 193860 [details] [diff] [review]
updated


+	   this.PREF = 
+		     Components.classes["@mozilla.org/preferences-service;1"]
+			      
.getService(Components.interfaces.nsIPrefService)
+			       .getBranch(null);
+
+	   this._initializeEngine();
+	   this.mPrefObserver.register();

this should be nsIPrefBranch, no?

also, this.prefService or somesuch is more appropriate, despite the evilness
surrounding, but I'll leave it for now.


+
+ #searchEngineList {
+  min-height: 24px;
+}
+
+.engine-icon > .menulist-label-box > .menulist-icon,
+.engine-icon > .menu-iconic-left > .menu-iconic-icon {
+  width: 16px;
+  height: 16px;
+  min-height: 16px;
+}
+
+.engine-icon > .menu-iconic-left {
+  display: -moz-box;
+  min-height: 16px;
+}

dimensions in em please.


+  } else {
+    // Get the engine's base URL
+    searchURL = makeURI(searchURL).host;
+  }

if the engine has a bogus searchURL, the dialog will fail to close here, 

+function setPrefs() {
+  var pref = Components.classes["@mozilla.org/preferences-service;1"]
+			.getService(Components.interfaces.nsIPrefService)
+			.getBranch(null);

nsIPrefBranch!

+<dialog id="searchDialog"
+	 xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+	 title="&searchCaption.label;"
+	 onload="onLoad()"
+	 ondialogaccept="onDialogAccept()"
+	 buttonlabelaccept="&searchButton.label;"
+	 persist="screenX screenY width height"
+	 screenX="24" screenY="24"
+	 width="320" height="150">

height and width should be localizable entities, specified in em.

otherwise, looks good! let's get this in on the trunk, and then get it on the
branch on the weekend sometime.

Comment 60

[Mike Connor [:mconnor]]

for the css stuff, the engine icon is 16x16, yes, and that's fine in px, but not
the list height, which should be em, unless someone can explain otherwise.

Comment 61

[:Gavin Sharp [email: [e-mail]]]

Attachment: comments addressed

Changed the variable names, fixed the pref service stuff (which I suspect may
be the reason why Adam was crashing), and caught the exception in the case
where the searchURL is bad.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

Checked in on trunk.

mozilla/browser/base/jar.mn; new revision: 1.97;
mozilla/browser/base/content/browser.js; new revision: 1.496;
mozilla/browser/base/content/search.xml; new revision: 1.27;
mozilla/browser/base/content/searchDialog.css; initial revision: 1.1;
mozilla/browser/base/content/searchDialog.js; initial revision: 1.1;
mozilla/browser/base/content/searchDialog.xul; initial revision: 1.1;
mozilla/browser/locales/jar.mn; new revision: 1.26;
mozilla/browser/locales/en-US/chrome/browser/searchDialog.dtd; initial revision: 1.1

Comment 63

[u60234]

The width and height of the search dialog is still specified in px, and are not
localizable.

Comment 64

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Will this patch be checked into the 1.8 branch as well?

Comment 65

[:Gavin Sharp [email: [e-mail]]]

Attachment: localizable dialog width

mconnor says r=him, Mano tested this on the Mac.

Comment 66

[:Gavin Sharp [email: [e-mail]]]

Checked in attachment 193141 [details] [diff] [review] on the trunk.

mozilla/browser/base/content/searchDialog.xul; new revision: 1.2;
mozilla/browser/locales/en-US/chrome/browser/searchDialog.dtd; new revision: 1.2;

Comment 67

[Asa Dotzler [:asa]]

after this is verified on the trunk, we'll consider for branch approval.

Comment 68

[Bob Clary [:bc] (inactive)]

verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050902 Firefox/1.6a1

Comment 69

[Asa Dotzler [:asa]]

Comment on attachment 193987 [details] [diff] [review]
comments addressed

Let's get this fixed on the branch ASAP. Thanks!

Comment 70

[:Gavin Sharp [email: [e-mail]]]

Attachment: robustness fix

This is a little fix I had in my tree to wallpaper over bug 306135, tested by
Adam. I'd like to get this in to the trunk before landing the entire patch on
the branch. I'll attach an all inclusive branch patch shortly.

Comment 71

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 194807 [details] [diff] [review]
robustness fix

This doesn't work, I've realized that something a little more involved is
required to fix this properly. I'm not sure I feel ok landing this on a stable
branch at the moment, given the crash reported in bug 306135. Hopefully I can
sort it all out tomorrow.

Comment 72

[Asa Dotzler [:asa]]

Comment on attachment 193987 [details] [diff] [review]
comments addressed

pulling approval until we find a better fix.

Comment 73

[:Gavin Sharp [email: [e-mail]]]

Attachment: Remove observer, don't try to sync prefs

The crash described in bug 306135 was caused by weird interaction between a
pref observer and the pref setting code in both search.xml and searchDialog.js.
This patch removes the observer, and makes the dialog use a seperate pref to
store it's "selected engine". This means that the search dialog and search bar
won't be in sync, but I see no reason why they necessarily should be, and the
most likely scenario is that people use either one or the other, not both.

This also fixes the preprocessor prefix for searchDialog.css, which would not
be checked in on the branch. Once this is landed on the trunk, I'll attach an
all-encompassing branch patch.

Comment 74

[Asaf Romano (gone)]

Comment on attachment 194958 [details] [diff] [review]
Remove observer, don't try to sync prefs

This is evil, but we're not going to rewrite this five days before beta...

r=mano.

(Also, r=me on the css changes which i've seen on IRC, note you didn't diff
them in this patch).

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Oops, forgot the css file. Checked this in on the trunk.
mozilla/browser/base/content/search.xml; new revision: 1.29;
mozilla/browser/base/content/searchDialog.js; new revision: 1.2;
mozilla/browser/base/content/searchDialog.css; new revision: 1.2;

Comment 76

[Hari Kumar G]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050906 Firefox/1.4

Not working here (1.4) as proposed. But was good in 1.6a1/20050906

Comment 77

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #76)
> Not working here (1.4) as proposed. But was good in 1.6a1/20050906

I'm not sure what you mean by "as proposed". No one has suggested this is fixed
on the branch yet, because it wasn't landed there.

Comment 78

[Hari Kumar G]

my fault :(

Comment 79

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #77)
> I'm not sure what you mean by "as proposed". No one has suggested this is fixed
> on the branch yet, because it wasn't landed there.

This bug has been plussed for 1.8b5; is that a guarantee that it will be landed
after 1.5b1 is released and sr+a is given?

Comment 80

[:Gavin Sharp [email: [e-mail]]]

> (In reply to comment #77)
> This bug has been plussed for 1.8b5; is that a guarantee that it will be landed
> after 1.5b1 is released and sr+a is given?

No.

Comment 81

[:Gavin Sharp [email: [e-mail]]]

Attachment: All inclusive branch patch

Comment 82

[:Gavin Sharp [email: [e-mail]]]

Attachment: Better all inclusive branch patch

Includes "localizable dialog width" (attachment 194141 [details] [diff] [review]), fix for bug 306358,
and wallpaper for bug 306135 (attachment 194958 [details] [diff] [review]). This has been on the trunk
for a while now and should be safe.

Comment 83

[:Gavin Sharp [email: [e-mail]]]

Checked in on the branch.

mozilla/browser/base/jar.mn; new revision: 1.95.2.3;
mozilla/browser/base/content/browser.js; new revision: 1.479.2.24;
mozilla/browser/base/content/search.xml; new revision: 1.25.2.3;
mozilla/browser/base/content/searchDialog.css; new revision: 1.2.2.3;
mozilla/browser/base/content/searchDialog.js; new revision: 1.2.2.3;
mozilla/browser/base/content/searchDialog.xul; new revision: 1.2.4.3;
mozilla/browser/locales/jar.mn; new revision: 1.25.2.3;
mozilla/browser/locales/en-US/chrome/browser/searchDialog.dtd; new revision:
1.2.4.3;

Comment 84

[Martin Creutziger [:MMx]]

This checkin was not announced in n.p.m.l10n yet, or did I miss something?
(CCed firefoxl10n for that purpose)

Comment 85

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #84)
> This checkin was not announced in n.p.m.l10n yet, or did I miss something?
> (CCed firefoxl10n for that purpose)

My apologies, I should have remembered to announce the checkin there.