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

VERIFIED FIXED in Firefox1.5

Status

()

enhancement
P1
normal
VERIFIED FIXED
16 years ago
7 years ago

People

(Reporter: jruderman, Assigned: Gavin)

Tracking

(Blocks 1 bug, {access, fixed1.8, late-l10n})

Trunk
Firefox1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 18 obsolete attachments)

14.28 KB, image/png
Details
40.23 KB, patch
Details | Diff | Splinter Review
2.08 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
4.79 KB, patch
mano
: review+
Details | Diff | Splinter Review
40.49 KB, patch
Details | Diff | Splinter Review
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).
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.
*** Bug 246775 has been marked as a duplicate of this bug. ***
Flags: blocking1.0?
Flags: blocking1.0? → blocking1.0+
Priority: -- → P4
Target Milestone: --- → Firefox1.0beta
Priority: P4 → P3
Summary: Ctrl+K should go to search engine when search bar is disabled → Ctrl+E should go to search engine when search bar is disabled
what about the expected behavior is to show navigatio toolbar and focus on
search bar?
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.
Posted patch patch (obsolete) — Splinter Review
once navigation bar is hidden, ctrl+j will show navigation bar and focus on
search bar
Attachment #155788 - Flags: review?(firefox)
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.
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
sounds not too hard, i'll see what i can do
Attachment #155788 - Attachment is obsolete: true
Attachment #155788 - Flags: review?(firefox)
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?
Attachment #155880 - Flags: review?(firefox)
since current shortcut for web search is ctrl+J instead of ctrl+K, i modify the
summary
Summary: Ctrl+E should go to search engine when search bar is disabled → Ctrl+J should go to search engine when search bar is disabled
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.
Summary: Ctrl+J should go to search engine when search bar is disabled → Web Search (Ctrl+E, etc) should go to search engine when search bar is disabled
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. 
> 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.
thanks, Jesse

the patch seems fine for aviary branch
Component: General → Keyboard Navigation
QA Contact: bugzilla
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 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
Attachment #155880 - Flags: review?(firefox) → review-
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.
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?
navigator.js is just a ref for browser.js and is obseleted,  rihgt?
Summary: Web Search (Ctrl+E, etc) should go to search engine when search bar is disabled → Web Search (Ctrl+K, etc) should go to search engine when search bar is disabled
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. 
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.
Flags: blocking-aviary1.0+ → blocking-aviary1.0?
OS: Windows XP → All
if a approved patch comes along this might be considered, but it is getting late
for 1.0
Flags: blocking-aviary1.0? → blocking-aviary1.0-
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?
inform@ocean.seos.uvic.ca: that's off-topic for this bug, but you can find the
answer at
http://kb.mozillazine.org/index.phtml?title=Emacs_Keybindings_(Firefox).
Blocks: firekey
Keywords: access
Posted patch Attempt #1 (obsolete) — Splinter Review
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.
Assignee: firefox → gavin.sharp
Attachment #155880 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #169803 - Flags: review?(mconnor)
Also, I don't understand the if (!searchBar.parentNode.parentNode.collapsed)
check in the previous patch. When is the toolbar ever collapsed?
Target Milestone: Firefox1.0beta → Firefox1.1
(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.
Flags: blocking-aviary1.1?
(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).
Dialog like the following would be better IMO:

|-------------------------|
|Search for:              |
| ( |G|                 ) |  <-  Same searchbinding (search.xml)
|                         |
|       (Cancel) (Search) |
|-------------------------|
What's happening with this patch? It's been waiting for review for a month. We
shouldn't let it rot.
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.
Attachment #169803 - Flags: review?(mconnor) → review-
Attachment #169803 - Attachment is obsolete: true
Hardware: PC → All
Version: unspecified → Trunk
Whiteboard: [wip]
Posted patch In Progress Patch (obsolete) — Splinter Review
Working, but I still want to go over it a bit. Testing would be appreciated,
and comments/suggestions are welcome.
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #174113 - Attachment is obsolete: true
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.
Blocks: 242862
(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.
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.
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #174265 - Attachment is obsolete: true
Posted image Screenshot of new dialog (obsolete) —
Attachment #179436 - Flags: review?(mconnor)
Whiteboard: [wip] → [patch-r?]
Comment on attachment 179436 [details] [diff] [review]
Patch v3

Why aren't you using the normal search binding (search.xml) rather than two
widgets?
Attachment #179436 - Attachment is obsolete: true
Attachment #179436 - Flags: review?(mconnor)
Priority: P3 → P1
Whiteboard: [patch-r?] → [wip]
Posted patch Work in progress (diff -w) (obsolete) — Splinter Review
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.
Attachment #182307 - Attachment description: Work in progress → Work in progress (diff -w)
Attachment #182307 - Attachment filename: cur.diff → 235204.diff
Posted patch A little closer (obsolete) — Splinter Review
Attachment #182307 - Attachment is obsolete: true
Posted patch Same as diff -w (obsolete) — Splinter Review
Posted patch Updated, some fixes (obsolete) — Splinter Review
This gets a little closer to it being finished, any comments/suggestions are
appreciated.
Attachment #183301 - Attachment is obsolete: true
Attachment #183302 - Attachment is obsolete: true
Flags: blocking1.8b4+
l10n inplications. must have by b4 if we're going to get it into 1.1.
Flags: blocking1.8b4+
Flags: blocking1.8b4+
Posted patch More fixes, updated (obsolete) — Splinter Review
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.
Attachment #185905 - Attachment is obsolete: true
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.
(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.
Attachment #188371 - Attachment is obsolete: true
checkin?
(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)
Whiteboard: [wip] → [wip] [being worked on, eta?
Blocks: branching1.8
Whiteboard: [wip] [being worked on, eta? → [l10n impact] [wip] [being worked on, eta?
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.
No longer blocks: branching1.8
Flags: blocking1.8b4+
Depends on: 303541
Posted patch More progress (obsolete) — Splinter Review
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 .
Attachment #188510 - Attachment is obsolete: true
Attachment #191950 - Flags: review?(mconnor)
Attachment #179437 - Attachment is obsolete: true
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?
(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.
Posted patch patch 2 (obsolete) — Splinter Review
With a few nits picked from Mano, and a fixed mistake in setPrefs().
Attachment #191950 - Attachment is obsolete: true
Attachment #192330 - Flags: review?(mconnor)
Attachment #191950 - Flags: review?(mconnor)
Flags: blocking-aviary1.5+ → blocking1.8b4+
Keywords: late-l10n
Whiteboard: [l10n impact] [wip] [being worked on, eta? → [needs review mconnor]
Posted patch updated (obsolete) — Splinter Review
Attachment #192330 - Attachment is obsolete: true
Attachment #193860 - Flags: review?(mconnor)
Attachment #192330 - Flags: review?(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.
Attachment #193860 - Flags: review?(mconnor) → review+
Whiteboard: [needs review mconnor] → [needs trunk landing before branch approval]
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.
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.
Attachment #193860 - Attachment is obsolete: true
Attachment #193987 - Flags: review?(mconnor)
Attachment #193987 - Flags: review?(mconnor)
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 306135
The width and height of the search dialog is still specified in px, and are not
localizable.
Will this patch be checked into the 1.8 branch as well?
mconnor says r=him, Mano tested this on the Mac.
Attachment #194141 - Flags: review+
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;
Depends on: 306358
Blocks: 306358
No longer blocks: 287437
No longer depends on: 233308, 303541, 306358
Attachment #193987 - Flags: approval1.8b4?
after this is verified on the trunk, we'll consider for branch approval.
Whiteboard: [needs trunk landing before branch approval] → [needs verification on trunk + approval]
verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050902 Firefox/1.6a1
Comment on attachment 193987 [details] [diff] [review]
comments addressed

Let's get this fixed on the branch ASAP. Thanks!
Attachment #193987 - Flags: approval1.8b4? → approval1.8b4+
Posted patch robustness fix (obsolete) — Splinter Review
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 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.
Attachment #194807 - Attachment is obsolete: true
Comment on attachment 193987 [details] [diff] [review]
comments addressed

pulling approval until we find a better fix.
Attachment #193987 - Flags: approval1.8b4+
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.
Attachment #194958 - Flags: superreview?(mconnor)
Attachment #194958 - Flags: review?(bugs.mano)
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).
Attachment #194958 - Flags: review?(bugs.mano) → review+
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;
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
(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.
my fault :(
(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?
Attachment #194958 - Flags: superreview?(mconnor)
> (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.
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.
Attachment #195814 - Attachment is obsolete: true
Attachment #195820 - Flags: approval1.8b5?
Attachment #195820 - Flags: approval1.8b5? → approval1.8b5+
Whiteboard: [needs verification on trunk + approval] → [branch checkin needed]
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;
Keywords: fixed1.8
Whiteboard: [branch checkin needed]
This checkin was not announced in n.p.m.l10n yet, or did I miss something?
(CCed firefoxl10n for that purpose)
(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.
Summary: Web Search (Ctrl+K, etc) should go to search engine when search bar is disabled → Web Search (Ctrl+K, etc) should show a search dialog when search bar is disabled
Blocks: 287520
No longer blocks: 306135, 306358
Depends on: 306135, 306358
Duplicate of this bug: 242862
You need to log in before you can comment on or make changes to this bug.