258672 - grey text in search box showing search engine name

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[chris hofmann]

take off on bug http://bugzilla.mozilla.org/show_bug.cgi?id=258199

recomendation from ben

need to grey text in search box showing search engine name
grey text goes away when box is focused
text with engine name shows whenever box is empty

Comment 1

[Peter van der Woude [:Peter6]]

This would make bug 258636 invalid I suppose

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'd be interested to hear usability feedback on this, which I've always found to
be confusing. mpt makes some compelling (IMO) arguments against this practice at
http://listserver.dreamhost.com/pipermail/whatwg-whatwg.org/2004-July/000857.html.

Would we do this for the URL bar as well?

Comment 3

[Asa Dotzler [:asa]]

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

Comment 4

[Asa Dotzler [:asa]]

Ideally, I think we'd label each field like we label all of the other toolbar
items but because we cram it all onto one toolbar, there isn't really any room
for the labels to be outside of the fields. IE has an always on label for the
addressfield that I think is a good solution but they have a full toolbar for
the addressfield and we don't.

Comment 5

[Brendan Eich [:brendan]]

Ghostly grey text is not going to be confused with user input ("real text"), and
if ghostly enough, ought not make the input appear "occupied".  Although it
might be confusing for other reasons.

/be

Comment 6

[Pierre Chanial]

What about a grey italicized engine name?

Comment 7

[Pierre Chanial]

Hum... Unless it is a marketing request, I think that showing additional text in
the chrome should be pondered very carefully. In addition to mpt's very good
poings, it puts extra (always visible, even if it is light grey) information
that will distract the user most of the time. On the other hand, I understand
that new users may not know what this stuff is about.

So, why not showing a text like "Search Google" in the text box, and disable it
after that the user completes a given number of requests (sth between 1 and 5)?

In case the icon is not clear enough (that shouldn't be the case for the default
set of engine), and after that we don't show anymore a text in the textbox, a
tooltip on the engine icon could help clarifying which search engine is selected.

Finally, it requires adding an entity. I then request a quick r+a for the
addition of:
searchbar_description=Search %S in searchbar.properties. (final string can be
decided later)

Comment 8

[Brendan Eich [:brendan]]

No marketing, just discoverability.  Some usability testing showed people have
no clue what the search box is for.

/be

Comment 9

[Steven Garrity [:sgarrity]]

I'll not weigh in on whether this is a good idea or not, but if it is going to
be implemented, see the search box in Thunderbird - it already has light grey
text showing what you are searching ("Subject or Sender" by default). Presumably
(without having looked at the code), whatever Thunderbird is doing could be
relatively easily to fix this "bug".

Comment 10

[chris hofmann]

steven/others,  can you put together a patch.  I'm not sure if pch is looking at
mozilla stuff right now, and we could use some help to get this one done quickly.

Comment 11

[chris hofmann]

ben says he can get this one.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

We can't do this because xul/focus event firing is inept right now. 

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

(wrt xul text boxes)

Comment 14

[Brendan Eich [:brendan]]

Ben, sorry if this is a dumb question: how come Thunderbird's quicksearch can do
this, or something seemingly like it, now?

/be

Comment 15

[jan klopper]

Maybe instead of having the title of the engine in the search field could we at
least have the title of the active search engine as a tooltip for the icon?

See my bug: https://bugzilla.mozilla.org/show_bug.cgi?id=288622

The problem is different, but the sollution would be the same as this bug. Add a
greyed txt, or add a tooltip.

Comment 16

[Steve England [:stevee]]

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

Comment 17

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

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

Comment 18

[Chris Beard]

I'd like to see this, but I believe it should be "Search the Web" as the primary
purpose of the search box.

/cb

Comment 19

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

Using grey-text as a way of giving users a hint of what the contents of a
particular field should be is becoming more common in UIs, with more predictable
behaviour (where the grey-text disappears). Where I think it gets confusing,
however, is when the text acts both as a drop-down label AND a hint of what to
do; it mixes the metaphor a little.

I agree with Chris, that the hint text should say something like "Enter search
terms" or "Search the web" to give the user a hint about what the box is for and
how they should use it.

Alternatively / additionally, I think we could probably come up with some icon
that represented "search" which we could place to the right of the search box,
in parallel to the green arrow that appears by default beside the URL box.

Comment 20

[Greg Campbell]

If the gray text is generic ("Enter search terms here"), it doesn't help the
problem of helping to identify multiple search engines with the same icon
(different Google locales, different bugzilla search types, etc.). That was the
primary point of my dupe (Bug 296627), and seems to be what the title of this
bug suggests is the real point of this bug.

Icons aren't enough to properly identify the type of search. The gray text could
possibly show the description attribute instead of the name attribute; although,
in practice, the description attribute is usually nothing more than name+"Search".

Generic text also has l10n impact, but if the text is lifted from the search
plugin itself, there isn't any l10n issue.

A "Go" button for the search bar is Bug 204691.

Comment 21

[Josh Aas]

In response to Brendan's comment #14, I think the reason Thunderbird could do it
and Firefox couldn't is that Thunderbird's implementation of the search bar uses
an HTML input field. That may have better focus handling than xul text fields.
I'm not knowledgeable enough on xbl and xul widget things to know if that quite
makes sense, but I'm investigating. I think I'm going to try to add this
functionality to the existing FF xbl binding, and if Ben's comments are still
true I'll evaluate whether or not it is worth shaking things up more by using
something more like what Thunderbird does. That would involve more major code
changes though.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

beard says we're punting on this for 1.5

Comment 23

[José Jeria]

Attachment: Screenshot

IE 7 Beta 1 has this approach.

Personally I think the search engines logotype is enough though. Having the
logotype + text would be a bit too much.

Comment 24

[Annie Sullivan]

Attachment: Adds grey-ed out text with search engine name

Not sure if I should add my rule for making the search engine name grey to more/different css files?

Comment 25

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

(In reply to comment #24)
> Created an attachment (id=210818) [edit]
> Adds grey-ed out text with search engine name

This patch doesn't apply to the trunk search.xml, which is going to be landed on the branch (bug 317369).

Comment 26

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

This is also something I'd already completed as part of the search rewrite, but my implementation still had some issues, and doing this seperately is probably a better idea anyways.

Comment 27

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

Attachment: mockup of search box with grey text & logo

Something Ben and I talked about was including the searchplugin icon as well as the grey text. This is just a quick mockup of what that might look like. The drop-down to select search engines would move to a button that would be next to the search bar.

Comment 28

[Brian Rakowski]

FWIW, I'm not a big fan of having the favicon in the search box.  I feel that it muddies the water a bit both in terms of the box being a search box and in terms of cluttering the UI.  My vote would be to have a simple magnifying glass icon beside the box and rely on the dimmed text for the search engine name.  That will give us a chance to keep the UI looking cleaner.

Comment 29

[u88484]

(In reply to comment #28)
> FWIW, I'm not a big fan of having the favicon in the search box.  I feel that
> it muddies the water a bit both in terms of the box being a search box and in
> terms of cluttering the UI.  My vote would be to have a simple magnifying glass
> icon beside the box and rely on the dimmed text for the search engine name. 
> That will give us a chance to keep the UI looking cleaner.
> 

And then how would you know which search engine you are using to search with? The icon has to be there.

Comment 30

[Brian Rakowski]

Safari, IE7, Opera, and Mike's mockup all handle this by putting the
name of the search engine in the box with gray text.  Also see the
name of this bug...

Comment 31

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

(In reply to comment #30)
> Safari, IE7, Opera, and Mike's mockup all handle this by putting the
> name of the search engine in the box with gray text.  Also see the
> name of this bug...

I presume Kurt meant while you're typing in the search box. If the icon is removed, there is no way to tell which engine you're using once focus is in the textbox and the grey text is removed.

Comment 32

[Brian Rakowski]

I wish I had data here but my guess is that users change their search engine so infrequently that having this permanent indication isn't worth the extra clutter and the loss of length in the (already too short) text box.

As background information, IE7 and Safari don't use an icon. Opera does use an icon. It's fair to point out that changing the search engine in Safari is only possible from the prefs.

Comment 33

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

(In reply to comment #28)
> it muddies the water a bit both in terms of the box being a search box and in
> terms of cluttering the UI.  My vote would be to have a simple magnifying 

Hm. The reason I proposed it was that I expected that the brand recognition of the [G] or the Y! would make it more clear to the user what would happen if they put something in that box. Our search partners have done a lot of work to build up a brand, and the web-surfing population identifies with those brands, so I felt that we should preserve that in the UI.

I know that the text will (hopefully) be customized based on the search plugin (ideally there would be a string that the user could provide) but the icon will, I think, be the most immediately recognizable object.

> I wish I had data here but my guess is that users change their search engine 
> so infrequently that having this permanent indication isn't worth the extra

Yes, well, I think that's more because they don't know that they can easily do so.

Comment 34

[jan klopper]

(In reply to comment #32)

So i guess the question comes down to the following:

We make the search box completely idiot proof by giving it a magnifying glass, or we make it usable for all users, and especially the power users that seem to be the only ones using it anyway.

Most normal users use the google startpage to go anywhere, including other sites, some will actually type in google.com in the content searchbox to go to google.

Making the search box in the chrome more clean will actually remove a piece of recognition from it as being a search box, ask around, more people will recognize the G or Y! as a search-engine than the magnifying glass. (maybe you could even find people thinking the magnifying glass indicates a search on the currently loaded page)

When you remove the icon from the searchbox, and you have a text still sitting in there, which almost all power user will have more often than not will make it impossible to see what search-engine he's using.

Comment 35

[Brian Rakowski]

There are a couple of different issues being discussed here. Let me try to keep them separate.

1. Feature discovery: Can we make it clear that that the search box is a search box?

A usability test conducted on FF 1.0 showed that the search box is not recognizable as a search box for many users.  We didn't conduct the same usability test on other browsers with built in search, but, based on logs showing usage of these built-in search boxes, it seems that Safari doesn't have this problem.  The usability team suggested that we add gray text "Search with Google" to the box to help people recognize that the box is for searching. In other words, the [G] that we have now is not working. If Safari's example shows us anything, it's that, for feature discovery, the favicon is not necessary.  Also, I should point out that the default provider's favicon ([G]) has, at best, only a weak association with their brand.

2. Selection confusion: Without an always-visible favicon, users may forget which search engine they've selected after they start typing into the box.

Is this really a problem? Do users actually change their search engine? I have no data but I'm guessing that they don't.  Are we doing anything to encourage them to do this more often in FF2?  If we are, will this be the type of change that would lead users to change their search engine often enough that they'd need a permanent reminder of which engine they had selected (several times/session)?  It seems that gray text that appears whenever the box is empty, plus the fallback mechanism of peeking in the dropdown to verify the current selection would be sufficient indicators of the current selection.

If we aren't reasonably sure that adding something to the chrome is useful, I'm lobbying to err on the side of simplicity and leave it out.  

Comment 36

[Peter van der Woude [:Peter6]]

(In reply to comment #35)
> 2. Selection confusion: Without an always-visible favicon, users may forget
> which search engine they've selected after they start typing into the box.
> 
> Is this really a problem? Do users actually change their search engine? I have
> no data but I'm guessing that they don't.  Are we doing anything to encourage
> them to do this more often in FF2?  If we are, will this be the type of change
> that would lead users to change their search engine often enough that they'd
> need a permanent reminder of which engine they had selected (several
> times/session)?  It seems that gray text that appears whenever the box is
> empty, plus the fallback mechanism of peeking in the dropdown to verify the
> current selection would be sufficient indicators of the current selection.
> 
> If we aren't reasonably sure that adding something to the chrome is useful, I'm
> lobbying to err on the side of simplicity and leave it out.  
> 
I can only speak for myself, but i think this is a bad idea because:
The default size searchbox is probably too small to display what is needed
"Search with Google" will fit, but 
"search with Van Dale woordenboek" (a dutch dictonary) won't.
Unless you use smaller letters in which case the will be very hard to read.
a Logo and the words "search the internet" or something along those lines will do.
The 16 px gain for hosing the icon will result in 100+ px loss for extending the searchbox so it can actually display the full search plugin title.

Comment 37

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

ASCII time ~ I can provide some better art if people feel it would help:

[G] = search icon logo
 O. = magnifying glass (what do you want from ASCII?)
 v  = drop down widget


  .---------------------------.
 ( O.                    |[G]v )
  `---------------------------'

 - no grey text
 - adds a magnifying glass to clarify the function of the field
 - preserves the search logo, and uses it as the drop-down button for picking the engine

  .---------------------------.
 ( [G] Search with Google |O.v )
  `---------------------------'

 - keeps some parity with 1.x
 - adds a search button w/magnifying glass to clarify the function of the field
 - grey text makes it very clear it's a search field, but mpt's comments apply; grey-text fields aren't always more usable, and I'd add to his list the fact that it clutters the UI with more text

  .---------------------------.
 ( [G]v Search with Google |O. )
  `---------------------------'

 - just like 1.x, but adds a search "Go" button

  .---------------------------.
 ( Search with Google     |O.v )
  `---------------------------'

 - no more search logo, and grey text

  .---------------------------.
 (                        |O.v )
  `---------------------------'

 - no more search logo, and no grey text


Right now I'm leaning towards the first of these options, with tooltips for the search button/dropdown that would say "Search with Google" and "Select a different search engine". Thoughts? Critiques? Harsh invective?

Comment 38

[Brian Rakowski]

Let me start by saying that we should see if we can answer this with some usability testing. Google would be happy to pony up the resources to make this happen (Mike, feel free to design the test or audit a proposal from Andrea). 

To recap, the issues we're trying to solve are:
1. Usability testing implied that users didn't know that the 1.x search box was a search box. Google's data suggests that this may be true.
2. The search box isn't big enough to type longer queries in to
3. Generally, we want to clean up the UI.

If possible, it would be nice to test a few different designs since goal 1 and 3 pull us in opposite directions. I'm nervous that the gray text is necessary to get users to realize that the box is a search box. However, I agree that it would be great to keep the UI as clean as possible. 

Also, I'd like to observe that the Google favicon isn't very recognizable as an indicator of Google (Y! and eBay do a nicer job). I'm trying to get that changed but we're short on better ideas here.  

2 (accommodating longer queries) will mostly be satisfied by making the search box scale with the width of the window but it would also be nice to reserve as much space for text as possible (less icons and logos, etc).

For those reasons, I have a preference for option #4:
  .---------------------------.
 ( Search with Google     |O.v )
  `---------------------------'

For a nice sense of closure, like a junior high essay, I'll end with this: Let's be sure to decide which we want to test and get usability lined up to help.

Comment 39

[Greg Campbell]

Research sounds like a good idea. Here are my hypotheses:

1. A magnifying glass without gray text, even with a tooltip, will not give users sufficient indication that the search provider can be changed. Note that most novice users will never change the default search provider anyway.

2. Gray text makes it easiest to understand the function of the box.

3. Gray text with a magnifying glass is no different from 2.

4. Gray text with a search-provider icon is no different from 2, but might provide power users with short-term memory problems a better experience. (Click in box; gray text disappears; "wait, what kind of search am I doing?")

5. Many users need a "Go" button (which should say "Search" and possibly use the magnifying glass icon).

I think getting rid of the "with" in the gray text would help eliminate overflow problems (at least in English). "Search with Google" and "Search with Yahoo" are nice, but "Seach Amazom.com" and "Search eBay" are better and "Search Google" and "Search Yahoo" are sufficient.

I also like the idea of making the Go button attach to the Location bar, and the hypothetical Search button do the same to the search bar. Currently, the Go button can be placed anywhere, and placement anywhere other than adjacent to the location bar is bizarre. Having similar ability for bizarre placement of the Search button seems bad too. 

To summarize (example with dropdown):

  .---------------------------.------------+
 ( [G] Search Google        |v ) O. Search |
  `---------------------------'------------+
  |[Y] Search Yahoo           |
  |[a] Search Amazon.com      |
  |[e] Search eBay            |
  +---------------------------+

Comment 40

[jan klopper]

> 2. Gray text makes it easiest to understand the function of the box.
 ---
> 4. Gray text with a search-provider icon is no different from 2, but might
> provide power users with short-term memory problems a better experience. (Click
> in box; gray text disappears; "wait, what kind of search am I doing?")

I'd like to comment on this. A gray text with icon seems fine. If and only if the search engine box is EMPTY. As soon as there's a query in the searchbox, the gray text will be gone, and you will need the icon to veryify which search engine you're using.

In my office we use two custom search providers, who both use the same icon, and to make sure you are sarching the correct search engine, you'd need to doule check the search box before submitting.

Changing the icon to be unique would solve this, but how would you go about adding for example images.google? there's no obvious icon for that, it would just wind up being the google icon again. (and thus being mistaken with the normal google search.)

I'd suggest we also make available a tooltip with the grey-text when the (advanced) user mouses over the search box.

Comment 41

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

Comment on attachment 210818 [details] [diff] [review]
Adds grey-ed out text with search engine name

This patch needs to be updated to match changes to the search binding.

Comment 42

[Pam Greene]

Greg's example in Comment #39 is exactly what I would have suggested.

- Keeping the favicon on the left makes it match the tabs, URL bar, bookmarks, bookmark folders, and drop-down listings.  I don't see a good argument for moving it to the right end and breaking all that expectation.

- Similarly, moving the drop-down arrow to the right makes it match the location bar and bookmark toolbar folders.

- Finally, adding a "Search" button beyond the right end of the search field matches the location field and its "Go" button.  (And then if the "Go" button becomes dockable to the location field, the "Search" button should likewise be dockable to the search field.)

Consistency is good.

As an aside, this portion of the discussion is getting a bit far afield from the original subject of the bug.  Placement of search buttons and icons is in bug 335441.

Comment 43

[Jon Henry]

I agree with Pam/Greg, FWIW. I would have initially voted against a dedicated "Search" button, but a recent ad-hoc usability study with my mom changed my mind. The last time I visited her house, I messed around with the Firefox configuration a little, including removing the "Go" button from the toolbar to save space. This proved to be a complete disaster, as she had no idea how to make the browser "go" to the location she had just entered. I'm sure hitting Enter is fine for 99% of the people who install Firefox on their own, but someone familiar with IE who runs into a Firefox install can (apparently) be easily confused by the lack of a button to press in lieu of a key.

Adding a "Search" button would enhance usability for newcomers, and probably only affect the URL bar when it contains a long query string. Newcomers won't notice or care that the query string is cut off, and advanced users will have removed the Search button after 30 seconds of use anyway.

Comment 44

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

(In reply to comment #39)
>   .---------------------------.------------+
>  ( [G] Search Google        |v ) O. Search |
>   `---------------------------'------------+
>   |[Y] Search Yahoo           |
>   |[a] Search Amazon.com      |
>   |[e] Search eBay            |
>   +---------------------------+

The drop-down here will conflict with the drop-down in the location bar which grants access to the previous entries into that field; in this case it selects between search engines. Consistency is good only if the resulting function is consistent ;)

Keep in mind the goals of this bug:

 1. Make it clear that this field is a search field
 2. Make it clear what engine is being used to perform that search

The dedicated "Search" button accomplishes #1, and means that we don't need the grey text to *also* say search. Let's not fix this solution with a sledgehammer.

Since 335441 is being used to track the search drop-down button, let's just continue to think about the use of grey text in this bug.

Can we get a patch that implements the grey shadow text which lists the name of the search engine, and disappears the moment focus is given to the field? I suggest that we get some quick informal experimentation done on this to see how it looks and feels.

Comment 45

[Joe Hughes]

Attachment: Patch to enable grayed search engine name within new binding structure

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220081 [details] [diff] [review]
Patch to enable grayed search engine name within new binding structure

>+          if (this.value != newValue) {
>+            window.__searchTextbox = this;
>+            window.__searchEngine = newValue;
>+            setTimeout("__searchTextbox.value = __searchEngine;" +
>+                       "__searchTextbox = undefined;" +
>+                       "__searchEngine = undefined;", 50);
>+          }

I don't know that you need to set properties on the global window here... can't you encapsulate this data in a closure or something similar to the |hitch| method Annie was using in the places toolbar/menu builder?

Comment 47

[Joe Hughes]

Attachment: Same as previous but with better timeout binding

Comment 48

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

Comment on attachment 220083 [details] [diff] [review]
Same as previous but with better timeout binding

Some things that don't work very well with this patch:
- Ctrl+K to focus the search box (text remains in the box, but is un-greyed)
- Focusing the textbox (text disappears), then changing the engine using the dropdown (text for the new engine reappears, un-greyed)
- using the dropdown in general

I recall having some problems with these cases when I originally included the feature as part of the binding rewrite. I ended up adding a lot of dump()s to find the event sequences and noticed some weird things happening (multiple focus events in the Ctrl+K case, etc).

> +            setTimeout(function() {searchTextbox.value = newValue;}, 50);

Why 50? Usually timeouts like these use 0 (which gets treated as 10) to indicate that they are bogus "run this at some point later in the event queue" timeouts.

Comment 49

[Joe Hughes]

Attachment: Shakes out some asynchronous glitches Gavin identified

Comment 50

[Joe Hughes]

Attachment: Tweaked to improve patching against current search.properties

Comment 51

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

Comment on attachment 220177 [details] [diff] [review]
Tweaked to improve patching against current search.properties

>Index: browser/locales/en-US/chrome/browser/search.properties

>+searchbox_hint=%S Search

add a newline before this? or put it next to searchtip.

>Index: browser/components/search/content/search.xml

>-
>+          this._textbox._displayCurrentEngine();
>+  

remove the spaces on the added line.

>+      <!-- Displays a grayed-out hint string containing the name of the
>+           current search engine in the search text box.  (It makes it gray
>+           by setting an empty="true" attribute on the searchbox element.)
>+      -->
>+      <method name="_displayCurrentEngine">
>+        <body><![CDATA[
>+          var searchbar = this._getParentSearchbar();
>+          var newValue = searchbar._stringBundle.getFormattedString(
>+            "searchbox_hint", [searchbar.currentEngine.name]);
>+
>+          // This section is a wee bit hacky; without the timeout, the CSS
>+          // style corresponding to the "empty" attribute doesn't kick in
>+          // until the text has changed, leading to an unpleasant moment
>+          // where the engine name flashes black before turning gray.
>+          searchbar.setAttribute("empty", "true");
>+          if (this.value != newValue) {

Why this test? This can only be false when this is called in the hasAttribute("empty") case, and I don't know when that would be true (see below).

>+            var searchTextbox = this;
>+            setTimeout(function() {
>+              if (!searchbar.hasAttribute("empty")) return;
>+              searchTextbox.value = newValue;

Reverse the test and get rid of the return?

>+            }, 0);
>+          }
>+        ]]></body>
>+      </method>
>+

>+      <handler event="focus" phase="capturing"><![CDATA[
>+        var searchbar = this._getParentSearchbar();
>+        if (searchbar.hasAttribute("empty")) {
>+          searchbar.removeAttribute("empty");
>+          this.value = "";
>+        }
>+      ]]></handler>
>+
>+      <handler event="blur" phase="capturing"><![CDATA[
>+        var searchbar = this._getParentSearchbar();
>+        if (searchbar.hasAttribute("empty") || this.value == "") {

As mentioned above, why check hasAttribute here? When can that be true if the second test is false?

>+          this._displayCurrentEngine();
>+        }
>+      ]]></handler>

I couldn't find any issues with the patch in testing, but I'd like to understand what the extra checks are for. When I removed them all the cases I tried still seemed to work, but I'm probably missing something.

I noticed that some of the engine names were cut off because they were longer than the textbox, maybe we should just increase the default width until rezising is implemented.

Comment 52

[Joe Hughes]

(In reply to comment #51)
> (From update of attachment 220177 [details] [diff] [review] [edit])
> >Index: browser/locales/en-US/chrome/browser/search.properties
> 
> >+searchbox_hint=%S Search
> 
> add a newline before this? or put it next to searchtip.

Done.

> >Index: browser/components/search/content/search.xml
> 
> >-
> >+          this._textbox._displayCurrentEngine();
> >+  
> 
> remove the spaces on the added line.

Done.

> >+      <!-- Displays a grayed-out hint string containing the name of the
> >+           current search engine in the search text box.  (It makes it gray
> >+           by setting an empty="true" attribute on the searchbox element.)
> >+      -->
> >+      <method name="_displayCurrentEngine">
> >+        <body><![CDATA[
> >+          var searchbar = this._getParentSearchbar();
> >+          var newValue = searchbar._stringBundle.getFormattedString(
> >+            "searchbox_hint", [searchbar.currentEngine.name]);
> >+
> >+          // This section is a wee bit hacky; without the timeout, the CSS
> >+          // style corresponding to the "empty" attribute doesn't kick in
> >+          // until the text has changed, leading to an unpleasant moment
> >+          // where the engine name flashes black before turning gray.
> >+          searchbar.setAttribute("empty", "true");
> >+          if (this.value != newValue) {
> 
> Why this test? This can only be false when this is called in the
> hasAttribute("empty") case, and I don't know when that would be true (see
> below).

I put that in because repeated focus/blur events were causing the text to flicker slightly, but I think that probably went away when I switched to the timeout.  Removed the test.
 
> >+            var searchTextbox = this;
> >+            setTimeout(function() {
> >+              if (!searchbar.hasAttribute("empty")) return;
> >+              searchTextbox.value = newValue;
> 
> Reverse the test and get rid of the return?

I generally write them that way because it improves readability in longer functions, but I'll reverse in this case.

> 
> >+            }, 0);
> >+          }
> >+        ]]></body>
> >+      </method>
> >+
> 
> >+      <handler event="focus" phase="capturing"><![CDATA[
> >+        var searchbar = this._getParentSearchbar();
> >+        if (searchbar.hasAttribute("empty")) {
> >+          searchbar.removeAttribute("empty");
> >+          this.value = "";
> >+        }
> >+      ]]></handler>
> >+
> >+      <handler event="blur" phase="capturing"><![CDATA[
> >+        var searchbar = this._getParentSearchbar();
> >+        if (searchbar.hasAttribute("empty") || this.value == "") {
> 
> As mentioned above, why check hasAttribute here? When can that be true if the
> second test is false?
> 
> >+          this._displayCurrentEngine();
> >+        }
> >+      ]]></handler>
> 
> I couldn't find any issues with the patch in testing, but I'd like to
> understand what the extra checks are for. When I removed them all the cases I
> tried still seemed to work, but I'm probably missing something.

I think that other check was ported from Annie's original logic, and I figured that it handled some edge case.  (Like if the engine changes and you somehow get another blur without having gotten a focus in the meantime.)  But you're right that it seems to work without it, so I'll take it out and if some flakiness manifests itself later we can fix it then.
 
> I noticed that some of the engine names were cut off because they were longer
> than the textbox, maybe we should just increase the default width until
> rezising is implemented.

Yeah, that's why the string has "Search" after the engine name.  Since I'm taking on bug 205011, I'm going to pass on this issue and just work on that.

Comment 53

[Joe Hughes]

Attachment: Tidying up per Gavin's comments.

Comment 54

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

Comment on attachment 220191 [details] [diff] [review]
Tidying up per Gavin's comments.

>Index: browser/components/search/content/search.xml

>+      <method name="_displayCurrentEngine">

>+          setTimeout(function() {
>+            if (searchbar.hasAttribute("empty")) {
>+              searchTextbox.value = newValue;
>+            }
>+          }, 0);

>+      <handler event="blur" phase="capturing"><![CDATA[
>+        var searchbar = this._getParentSearchbar();
>+        if (this.value == "") {
>+          this._displayCurrentEngine();
>+        }

nit: this file doesn't use brackets around single line if() blocks.

Comment 55

[Adam Kowalczyk]

I insist that the icon is very useful for identifying the current engine and that the grey text does not make it redundant.

The grey text may be helpful for users that don't necessarily recognize the icons, but for those familiar with them, the icons are the best solution and the quickest way to identify the current engine. What's more, as the engine name in grey text is associated with an engine icon, more users would learn what particular icons stand for. So not only doesn't the grey text make the icons redundant, but it may make them even more useful. These two may actually be complementary.

Comment 56

[Asaf Romano (gone)]

Comment on attachment 220191 [details] [diff] [review]
Tidying up per Gavin's comments.

foo.hasAttribute("empty") is by no means the same as foo.getAttribute("empty") == "true".

Also, please get rid of the single line brackets here.

Comment 57

[Joe Hughes]

Attachment: Updated to address Asaf's nits

Comment 58

[Joe Hughes]

BTW, ignore that spurious whitespace change to browser.xul in attachment 220407 [details] [diff] [review]--I won't be checking that in...

Comment 59

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220407 [details] [diff] [review]
Updated to address Asaf's nits

sr=[e-mail]

Comment 60

[Joe Hughes]

Checked in on branch & trunk.  Sadly, I accidentally put a random number (2358968) in the checkin message instead of the correct one, and I have no good way to fix that.

Comment 61

[Masatoshi Kimura [:emk]]

Joe:
You changed background color for url bar.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.30#909
I couldn't find a change of browser.css in attachment #220407 [details] [diff] [review].
Is this intentional? If it's landed accidentally, please back out.

Comment 62

[Jim Jeffery not reading bug-mail 1/2/11]

If the 'active' search engine is to have a gray-hightlight with this patch, it appears that it does not work in today's trunk-cairo.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060503 Minefield/3.0a1 ID:2006050305${Flags}

Comment 63

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #62)
> If the 'active' search engine is to have a gray-hightlight with this patch, it
> appears that it does not work in today's trunk-cairo.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060503
> Minefield/3.0a1 ID:2006050305${Flags}
> 

Nevermind my comment, I mis-read the purpose.  The gray text is there, but I really wonder why?  I know its a search box..

Comment 64

[Bryan]

There is an inconsistency in the tooltip / gray text.  Example, Google Search is the gray text yet the tooltip says "Search Google".  Should it not be consistent?

I would suggest, although I'm sure it was already dicussed in this bug that the gray text be changed to match the tooltip "Search XXX"...

~B

Comment 65

[Joe Hughes]

(In reply to comment #64)
> There is an inconsistency in the tooltip / gray text.  Example, Google Search
> is the gray text yet the tooltip says "Search Google".  Should it not be
> consistent?

Bryan,

The reason that the engine name comes first is that I didn't want it to get cut off.  For example, "Search Creative Commons" would have much of the "Creative Commons" text cut off in the current searchbox size.

Comment 66

[Bryan]

(In reply to comment #65)
> (In reply to comment #64)
> > There is an inconsistency in the tooltip / gray text.  Example, Google Search
> > is the gray text yet the tooltip says "Search Google".  Should it not be
> > consistent?
> 
> Bryan,
> 
> The reason that the engine name comes first is that I didn't want it to get cut
> off.  For example, "Search Creative Commons" would have much of the "Creative
> Commons" text cut off in the current searchbox size.
> 

I understand.  Can we change the tooltip?  No offense, it just looks sloppy using two different terminologies.  Also, is there not a bug to increase the default size of the search bar?  It's way too small, at least for me.  I've got mine set to 300px.

~B

Comment 67

[Bryan]

(In reply to comment #65)
> (In reply to comment #64)
> > There is an inconsistency in the tooltip / gray text.  Example, Google Search
> > is the gray text yet the tooltip says "Search Google".  Should it not be
> > consistent?
> 
> Bryan,
> 
> The reason that the engine name comes first is that I didn't want it to get cut
> off.  For example, "Search Creative Commons" would have much of the "Creative
> Commons" text cut off in the current searchbox size.
> 
I might add that some seach engines sound weird having it XXX Search.

~B

Comment 68

[Joe Hughes]

(In reply to comment #66)
> I understand.  Can we change the tooltip?  No offense, it just looks sloppy
> using two different terminologies.  Also, is there not a bug to increase the

Sure, feel free to open a bug on that and assign it to me.

> default size of the search bar?  It's way too small, at least for me.  I've got
> mine set to 300px.

There are a couple bugs related to searchbar size being tracked by 335435 (and thus slated for "soon".

Comment 69

[Nickolay_Ponomarev]

> I might add that some seach engines sound weird having it XXX Search.
> 
For example "MSN Search Search": http://www.microsoft.com/windows/ie/searchguide/default_new.mspx

Is it their bug or ours?

Comment 70

[Mike Connor [:mconnor]]

I would say that its our bug, certainly.  That's their preferred branding, aiui, so we should probably check the searchengine name for " Search" and not append in that case.  Can you file a bug on that?

Comment 71

[Nickolay_Ponomarev]

Filed bug 337106.