460694 - Make use of emptytext attribute in search textboxes

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Stefan [:stefanh]]

We use the new search textbox widget now, but we're still combining that with a label "Search". I think it'd look better if we used the emptytext attribute. Comments/thoughts?

Comment 1

[Stefan [:stefanh]]

There are 2 ways we could use emptytext:
1) Have a generic value, like "Type some text to search" (not the best suggestion, but you know what I mean)
2) Have "Search Bookmarks", "Search History" etc

Comment 2

[Stefan [:stefanh]]

I add Neil's comment from bug 460756 here since I'm quite convinced that his purpose was to comment here ;)

"  -------  Comment #10 From  [e-mail]   2008-10-24 06:25:50 PDT 
It would be good to have Accel+F focus the search box (dao pointed this out)."

Comment 3

[Stefan [:stefanh]]

Regarding having accel+F focus the search box in some windows:

1) In Bookmarks manager accel+F brings up the Advanced search.
2) In the Help window (not comm-central, but I have a patch in bug 460668 and I can switch to emptytext) accel+f brings up the find bar.
3) MailNews and Addressbook seems OK, at least it looks like accel+F doesn't do anything. I could be wrong, though.

One possible solution to 1) is to switch to accel+shift+f for Advanced search.

Comment 4

[Stefan [:stefanh]]

> One possible solution to 1) is to switch to accel+shift+f for Advanced search.

One argument for this is that accel+shift+F is used in MailNews/Addressbook to bring up Advanced search.

Comment 5

[Stefan [:stefanh]]

(In reply to comment #3)

> 3) MailNews and Addressbook seems OK, at least it looks like accel+F doesn't do
> anything. I could be wrong, though.
Oops, "Find in this message"... Wonder how I missed that...

Comment 6

[Stefan [:stefanh]]

Attachment: Patch for Helper apps pref pane

Just attaching the easy one - the search box in the Helper apps pref pane.

Comment 7

[Stefan [:stefanh]]

Attachment: Patch for bookmarks

Here's a patch for Bookmarks manager/bm-panel. No key specified for focusing the search box (yet).

Comment 8

[Stefan [:stefanh]]

Attachment: Helper apps pref pane, take 2

New version. I don't think we need the select() and use both "f" and "k". Also nuke accesskey in .dtd.

Comment 9

[Stefan [:stefanh]]

So, I think we can use accel+k for all the other ones.

Comment 10

[Stefan [:stefanh]]

Attachment: Bookmarks, take 2

New version, bookmarks. Can't focus the search field from the browser window, though.

Comment 11

[Stefan [:stefanh]]

Attachment: Mailnews version

This one covers mailnews/addressbook. I made the addressbook a little bit more customize-friendly. I need some feedback from Karsten on this one, because I've removed the addressbook advanced search button and moved the search field up to the main toolbar. Initially, I was going to put the button in the main toolbar as well, but I need some nice icons... Karsten, what should I do here? Leave the advanced search button in it's original place? The good thing with emptytext is that we now can show the advanced button in mailnews.

Comment 12

[Philip Chee]

> Created an attachment (id=348443) [details]
> Mailnews version

> I've
> removed the addressbook advanced search button and moved the search field up to
> the main toolbar.

I know that Thunderbird does this, but I disagree. Their searchbox looks really out of place on the main toolbar. And when Thunderbird 2.0 moved the search bar in the main messenger window to the main toolbar, people were really, really unhappy (And I mean REALLY REALLY unhappy) so I wrote xSearchbarT2 for those people <http://xsidebar.mozdev.org/tsearchbar/index.html>

+  <hbox id="searchBox" align="center" flex="1">
+    <spacer id="searchSpacer" flex="1"/>

What does the addition of this spacer do?

Comment 13

[Stefan [:stefanh]]

 > +  <hbox id="searchBox" align="center" flex="1">
> +    <spacer id="searchSpacer" flex="1"/>
> 
> What does the addition of this spacer do?

The same as the toolbarspring does in addressbook.xul. I didn't made any customize-friendly changes to mailnews, because I thought that'll be an overkill.

Comment 14

[Stefan [:stefanh]]

>And when Thunderbird 2.0 moved the search bar
> in the main messenger window to the main toolbar, people were really, really
> unhappy (And I mean REALLY REALLY unhappy) so I wrote xSearchbarT2 for those
> people <http://xsidebar.mozdev.org/tsearchbar/index.html>
>

Sure, but I'm not doing anything with the search field in the main messenger window here (apart from making the advanced search button visible).

Comment 15

[Philip Chee]

>> What does the addition of this spacer do?
> The same as the toolbarspring does in addressbook.xul.

No I mean why it it needed? I don't understand sorry.

Comment 16

[Stefan [:stefanh]]

(In reply to comment #15)
> >> What does the addition of this spacer do?
> > The same as the toolbarspring does in addressbook.xul.
> 
> No I mean why it it needed? I don't understand sorry.

It pushed the search field to the right when you make your window wider (just like the toolbarspring in bookmarksManager). It also serves as an overlay point for the viewpicker that is overlayed from msgViewPickerOverlay.xul

Comment 17

[Philip Chee]

Ah, I see.

Comment 18

[Stefan [:stefanh]]

Comment on attachment 347182 [details] [diff] [review]
Helper apps pref pane, take 2

Maybe it's time to start with the first patch. I see now that I used the form &focusSearch[number].key.. Should it be &focusSearch.key[number]?

Comment 19

[Stefan [:stefanh]]

(In reply to comment #12)
> > Created an attachment (id=348443) [details] [details]
> > Mailnews version
> 
> > I've
> > removed the addressbook advanced search button and moved the search field up to
> > the main toolbar.
> 
> I know that Thunderbird does this, but I disagree. Their searchbox looks really
> out of place on the main toolbar.

I think the search box looks really lonely when we've gotten so much space in the Addressbook's secondary toolbar. I think it's not good ui to waste an extra toolbar for something that could be in the main toolbar. And there is also room for it there (there is also enough space in the main toolbar for an advanced search button, I believe).

Would it be possible for users to add a new addressbook toolbar and drop the search box there when you've finished the customize stuff?

Comment 20

[[e-mail]]

Comment on attachment 347182 [details] [diff] [review]
Helper apps pref pane, take 2

The problem with a keyset is that it works even when the pane is hidden.

Comment 21

[Stefan [:stefanh]]

Oops...

Comment 22

[Robert Kaiser]

Stefan:
Please make shortcut key strings IDs always end in .key, this is what localizers and L10n tools are used to.

Comment 23

[Stefan [:stefanh]]

(In reply to comment #22)
> Stefan:
> Please make shortcut key strings IDs always end in .key, this is what
> localizers and L10n tools are used to.

Ah, ok - then I was right, then.

Comment 24

[Robert Kaiser]

Comment on attachment 348387 [details] [diff] [review]
Bookmarks, take 2

How is one supposed to get to those search box with the keyboard after that patch? There's at least no easily visible clue to it, if there's any accessibility at all.

Additionally, IMHO, "Search" is a pretty lame emptytext, in most cases where I have seen useful emptytexts so far, 1) it was clear that it was a search box even without the text, 2) it was phrased something like "Enter search text here"

Comment 25

[Stefan [:stefanh]]

(In reply to comment #24)
> (From update of attachment 348387 [details] [diff] [review])
> How is one supposed to get to those search box with the keyboard after that
> patch? There's at least no easily visible clue to it, if there's any
> accessibility at all.

Well, the current problem is that there seems to be no way to have a shortcut focus the bm-panels search field unless you have focused in the sidebar. What do you btw mean with "if there's any accessibility at all"?

> 
> Additionally, IMHO, "Search" is a pretty lame emptytext, in most cases where I
> have seen useful emptytexts so far, 1) it was clear that it was a search box
> even without the text, 2) it was phrased something like "Enter search text
> here"

1) It is clear that it is a search box even without emptytext, just look at it (dunno about linux, though).

2) Yeah, we can have some other string, sure. See comment #1, you're the first one that reacts to that comment. I don't think the string should be too long, though.

Comment 26

[Robert Kaiser]

(In reply to comment #25)
> Well, the current problem is that there seems to be no way to have a shortcut
> focus the bm-panels search field unless you have focused in the sidebar. What
> do you btw mean with "if there's any accessibility at all"?

I mean "if there's any (simple) way to get there with keyboard or a11y tools at all".

> 2) Yeah, we can have some other string, sure. See comment #1, you're the first
> one that reacts to that comment. I don't think the string should be too long,
> though.

"Search Bookmarks" or something like that sounds better to me than just "Search".

Comment 27

[Stefan [:stefanh]]

Comment on attachment 348443 [details] [diff] [review]
Mailnews version

Actually, this needs some more love I think...

Comment 28

[Stefan [:stefanh]]

Attachment: Bookmarks, take 3

Here's a new version for the bm manager/bm-panel. I didn't do any focus key for the bm-panels search field, since I don't think it's possible and we don't have any focus key for the search field in the history panel. I added the attribute 'cocoa-size="small"', a mac-only attribute that makes the search field a bit smaller (I should add that to the search fields in the other panels as well). I also made the search field be focused, when you switch from another panel to the bm-panel (like the history panel).

Comment 29

[Stefan [:stefanh]]

Comment on attachment 356351 [details] [diff] [review]
Bookmarks, take 3

Right, I should probably have "Search Bookmarks" instead of "Search bookmarks", because the history search field has "Search History"...

Comment 30

[Philip Chee]

> -    <label value="&search.label;" accesskey="&search.accesskey;" control="search-box"/>
> -    <textbox id="search-box" flex="1" type="search" clickSelectsAll="true"
> +    <spacer flex="1"/>
> +    <textbox id="search-box" cocoa-size="small" type="search"

What is the spacer for? (you seem to like spacers).

> +             emptytext="&search.emptytext;" clickSelectsAll="true"

Might as well use the one attribute per line style we seem to be using these days.

> -        <textbox id="search-box" type="search" clickSelectsAll="true"
> +        <textbox id="bm-search-box" type="search"

Why the "bm-" in "bm-search-box"? Do we have more than one (or plans to have more than one)?

> +    <key key="&focusBmSearch.key;" modifiers="accel"

Where is this entity defined? I can't find it in MXR.

Comment 31

[Stefan [:stefanh]]

(In reply to comment #30)
> > -    <label value="&search.label;" accesskey="&search.accesskey;" control="search-box"/>
> > -    <textbox id="search-box" flex="1" type="search" clickSelectsAll="true"
> > +    <spacer flex="1"/>
> > +    <textbox id="search-box" cocoa-size="small" type="search"
> 
> What is the spacer for? (you seem to like spacers).

Please see comment #16?
 
> Why the "bm-" in "bm-search-box"? Do we have more than one (or plans to have
> more than one)?

Yes, we have 2 - see the patch.

> 
> > +    <key key="&focusBmSearch.key;" modifiers="accel"
> 
> Where is this entity defined? I can't find it in MXR.

I forgot to diff suite/locales... new version coming up in a few minutes.

Comment 32

[Stefan [:stefanh]]

Attachment: Bookmarks, take 2 (correct version)

This one should be correct (also fixed the emptytext entity). I changed the indentation of the key per Philip's recommendation. However, I'm a bit unsure re the right place for the focus key here... Neil, should I put it in bookmarksManager.xul instead?

Comment 33

[Philip Chee]

>> What is the spacer for? (you seem to like spacers).
> Please see comment #16?
Why do you need to push the search box to the right. It just makes the search box smaller and less functional.

>> Why the "bm-" in "bm-search-box"? Do we have more than one (or plans to have
>> more than one)?
> Yes, we have 2 - see the patch.

Put the keybinding in bookmarksManager.xul, then you don't have to change the text box ID.

+    <key key="&focusBmSearch.key;"
+         modifiers="accel"
+         oncommand="document.getElementById('bm-search-box').focus();"/>

Hmm.

     <!-- These keybindings do have a command specified in the overlay,
          which we need to correct in our Startup() function -->

Your keybinding doesn't have a command specified in the overlay so why did you put it in the overlay?

Comment 34

[Stefan [:stefanh]]

(In reply to comment #33)
> >> What is the spacer for? (you seem to like spacers).
> > Please see comment #16?
> Why do you need to push the search box to the right. It just makes the search
> box smaller and less functional.

Having the search box stretch and fill the whole box when you make the sidebar wider looks abnormal, because it will fill the whole width of the sidebar. I don't see how anyone would want such a behavior and fwiw, I've done the same in the bm manager window.

> >> Why the "bm-" in "bm-search-box"? Do we have more than one (or plans to have
> >> more than one)?
> > Yes, we have 2 - see the patch.
> 
> Put the keybinding in bookmarksManager.xul, then you don't have to change the
> text box ID.
> 
> +    <key key="&focusBmSearch.key;"
> +         modifiers="accel"
> +         oncommand="document.getElementById('bm-search-box').focus();"/>
> 
> Hmm.
> 
>      <!-- These keybindings do have a command specified in the overlay,
>           which we need to correct in our Startup() function -->
> 
> Your keybinding doesn't have a command specified in the overlay so why did you
> put it in the overlay?

mmh, good question. I guess I originally wanted to make this apply to both panel and manager, but hen I changed my mind.

Comment 35

[Stefan [:stefanh]]

But I see now that the other search fields in the panel stretch, so I should probably not add that spacer now.

Comment 36

[Philip Chee]

> Having the search box stretch and fill the whole box when you make the sidebar
> wider looks abnormal, because it will fill the whole width of the sidebar. I

I like it stretching the whole width. I think this trend towards teeny weenie search boxes disturbingly like the way Firefox developers think.

> don't see how anyone would want such a behavior and fwiw, I've done the same in
> the bm manager window.

I've made xSidebar revert your changes in the BM manager window. I suppose I could do the same for the bm-panel and addressbook for those who like their search boxes looking like search boxes.

Comment 37

[Stefan [:stefanh]]

(In reply to comment #36)
> > Having the search box stretch and fill the whole box when you make the sidebar
> > wider looks abnormal, because it will fill the whole width of the sidebar. I
> 
> I like it stretching the whole width. I think this trend towards teeny weenie
> search boxes disturbingly like the way Firefox developers think.

I think you're on the wrong track here, this is not about doing it the way "Firefox developers think" and I don't think this idea is restricted to Firefox either. You just made me download Opera and their search fields doesn't stretch either. Anyway, I will not make it stretch in the sidebar, OK?

> search boxes looking like search boxes.

So, we have different views about how a search box should look.

Comment 38

[Stefan [:stefanh]]

Attachment: Bookmarks take 3, final attempt

I decided to not add any "cocoa-size" since I think it's better do all affected search fields in one bug.

Comment 39

[[e-mail]]

Comment on attachment 356431 [details] [diff] [review]
Bookmarks take 3, final attempt

>-    <textbox id="search-box" flex="1" type="search" clickSelectsAll="true"
>+    <textbox id="search-box" type="search"
Interestingly the history and addressbook panels do flex their search box. Search, on the other hand, is still using an ordinary textbox ;-)
I'd leave the search in like you wanted to originally.

>+      <key key="&focusBmSearch.key;"
>+       modifiers="accel"
>+       oncommand="document.getElementById('search-box').focus();"/>
Weird indentation here. Also don't wrap modifiers if you don't want to.

>-        <textbox id="search-box" type="search" clickSelectsAll="true"
>+        <textbox id="search-box" type="search"
>+                 emptytext="&search.emptytext;" clickSelectsAll="true"
You could leave the clickSelectsAll on the previous line to avoid editing it (it's also a shorter line, so it makes more minusculely sense there!)

>+<!ENTITY focusBmSearch.key              "k">
History just calls this search.key, also maybe we should use Ctrl+F and move Search Bookmarks to Ctrl+Shift+F since it's now less useful.

Comment 40

[Stefan [:stefanh]]

Attachment: Bookmarks, take 4 (also add clickSelectsAll to history panel search)

Addressing Neil's comments. Neil noticed that we miss clickSelectsAll in the history panel's search field, so I added it.

Comment 41

[Stefan [:stefanh]]

>Anyway, I will not make it stretch in the sidebar, OK?

Just to clear out any misunderstanding: I actually ment to say that I will leave it as it is (being stretched) in the sidebar...

Comment 42

[Stefan [:stefanh]]

Attachment: Bookmarks, take 5 (pushed)

Not my day today... should be OK now.

Comment 43

[Robert Kaiser]

(In reply to comment #41)
> Just to clear out any misunderstanding: I actually ment to say that I will
> leave it as it is (being stretched) in the sidebar...

I agree that's probably the best way, as sidebars are rather narrow usually in any case - the full bookmarks/history/etc. windows are a different case, flexing/stretching it there is probably not ideal.

Comment 44

[Stefan [:stefanh]]

Comment on attachment 356449 [details] [diff] [review]
Bookmarks, take 5 (pushed)

I landed this as changeset e1e2aaca0863.

Comment 45

[Stefan [:stefanh]]

Regarding the Helper apps pane's search box, Neil suggested for a while ago that this could be solved with an event handler in prefwindow.xml. While looking at this I discovered an odd mac bug: Now, on mac, if you press a key combo that doesn't do anything, the system will produce a beep.

It appears that having something like <handler event="keypress" key="f" modifiers="accel"> ... </handler> in prefwindow, will do the right thing when accel+f is pressed, but at the same time beep.... It does look to me that somehow the native system doesn't know of the key combo. The only way I've found to work around this is to use preventDefault on the handler. Of course that won't make the system beep when we don't have a search field in a pane, but that can be worked around if you only cancel the event when you *have* a search field in a pane...

This bug also hits Help, because http://hg.mozilla.org/mozilla-central/rev/f7e763f81f1f added a way to open Help from the kb (and is now on 1.9.1). I can reproduce the "help beep" if I:

1) Remove the mac-specific overlay from Firefox's preferences.xul

(<?xul-overlay href="chrome://browser/content/macBrowserOverlay.xul"?>)

2) Have the preference window opened but no other windows opened while you invoke cmd+?

Comment 46

[Stefan [:stefanh]]

Attachment: Helper apps prefpane, take 3 (pushed)

OK, this makes it work at as I'd expect. I belive we want the mac beep when the shortcut shouldn't be available, so we don't cancel the event when there's no search box to focus (same theory/idea as the patch in bug 473444). I also changed the localization note in preferences.dtd since it looked wrong to me. And I couldn't find anything more to say in the search field's emtytext than "Search".

Comment 47

[Stefan [:stefanh]]

Comment on attachment 357446 [details] [diff] [review]
Helper apps prefpane, take 3 (pushed)

Oh, I forgot one thing: Neil, would it be better to also use phase="capturing"?

Comment 48

[Karsten Düsterloh]

Comment on attachment 357446 [details] [diff] [review]
Helper apps prefpane, take 3 (pushed)

Looks good, just some nits.

>diff --git a/suite/common/bindings/prefwindow.xml b/suite/common/bindings/prefwindow.xml
>+      ]]>
>+      </handler>
>+

Drop this empty line.

>diff --git a/suite/common/pref/pref-applications.xul b/suite/common/pref/pref-applications.xul
>       <textbox id="filter" flex="1" type="search" clickSelectsAll="true"
>-               oncommand="gApplicationsPane._rebuildView();"/>
>+               emptytext="&search.emptytext;" oncommand="gApplicationsPane._rebuildView();"/>

If not all attributes fit onto one line, put each one on its line and align them vertically.

>diff --git a/suite/locales/en-US/chrome/common/pref/pref-applications.dtd b/suite/locales/en-US/chrome/common/pref/pref-applications.dtd
>+<!ENTITY search.emptytext        "Search">

Hm, this is pretty non-descriptive. 
How about "Search helpers"? "Search app list"? Maybe replace Search by Filter?

r=me with these, given we find a more descriptive .emptytext.

Comment 49

[Stefan [:stefanh]]

JFTR, on irc we came to the conclusion that "Search Types and Actions" was a suitable emptytext string.

Comment 50

[Stefan [:stefanh]]

Comment on attachment 357446 [details] [diff] [review]
Helper apps prefpane, take 3 (pushed)

Landed this as changeset 1a22abcfbc7c (Karstens nits addressed and emptytext as "Search Types and Actions")

Comment 51

[Stefan [:stefanh]]

In mailNews, depending of what folder is selected, search box labels are:

"Subject or From contains:"
"Name or Email contains:"

Should we have "Search Subject and From" and "Search Name and Email"?

Comment 52

[Ian Neal]

You also have "Subject or Recipient contains:"

Comment 53

[Stefan [:stefanh]]

Right, sorry. Anyway, suggestions are welcome :)

Comment 54

[Karsten Düsterloh]

"X or Y contains:" ==> "Search X and Y" sounds fine, imo.

Comment 55

[Stefan [:stefanh]]

Attachment: mailNews, take 2

OK, this is the mailnews part - it also covers addressbook. This chages the ui a bit, so I'd like Standard8 to look at the addressbook part. What I do here is that I move the search field up to the addressbook main toolbar. At the same time I remove the advanced button (we could actually have it in the main toolbar, but we'd need an icon for it). In mailNews, on the other hand, I add the currently hidden advanced search button since switching to the emptytext attribute makes enough space available for it.

I think in the long-time perspective we'd want to move to something like thunderbirds search.xml, but I believe my changes makes sense atm.


A few comments:

I had to add a comment in the shared file mailnews.js because of the switch to emptytext.



+    <spacer id="searchSpacer" flex="2"/>
+    <textbox id="searchInput" flex="1" type="search" emptytext="&searchSubjectOrFrom.emptytext;"

I want to push the search field and the advanced button to the right when you enlarge the window. However, I don't want the searchbox to expend as much as the spacer, since I think it'll look bad if you make your window very wide - making the search box stretch half of what the spacer does, looks reasonable to me (the other option would be to have a max-width set on the search box).

Comment 56

[[e-mail]]

Comment on attachment 358599 [details] [diff] [review]
mailNews, take 2

>+  <!-- Search field -->
>+  <key key="&focusSearchInput.key;" modifiers="accel"
>+       oncommand="document.getElementById('searchInput').focus();"/>
Hmm... what if the search input isn't focusable (toolbar hidden)?

>+    <textbox id="searchInput" flex="1" type="search" emptytext="&searchSubjectOrFrom.emptytext;"
Won't we set the emptytext anyway? Less work for the translators :-)

>+<!-- focusSearchInput.key also used by addressbook -->
>+<!ENTITY focusSearchInput.key "k">
>+<!-- searchNameOrEmail.emptytext is used by addressbook -->
>+<!ENTITY searchNameOrEmail.emptytext "Search Name and Email">
If there aren't any other entities used by addressbook, then it might be worth moving/copying these onese there and not using this DTD at all. (And it's not more work for the translators, because it will be easier for them to match the entity when they have fewer DTD files to look in.)

Comment 57

[Stefan [:stefanh]]

(In reply to comment #56)
> (From update of attachment 358599 [details] [diff] [review])
> >+  <!-- Search field -->
> >+  <key key="&focusSearchInput.key;" modifiers="accel"
> >+       oncommand="document.getElementById('searchInput').focus();"/>
> Hmm... what if the search input isn't focusable (toolbar hidden)?

I'll add a check.

> 
> >+    <textbox id="searchInput" flex="1" type="search" emptytext="&searchSubjectOrFrom.emptytext;"
> Won't we set the emptytext anyway? Less work for the translators :-)

Yes, we do set it with js, but if we don't have the attribute from the beginning,  the search box will be empty for a brief moment when you launch mailnews.
> 
> >+<!-- focusSearchInput.key also used by addressbook -->
> >+<!ENTITY focusSearchInput.key "k">
> >+<!-- searchNameOrEmail.emptytext is used by addressbook -->
> >+<!ENTITY searchNameOrEmail.emptytext "Search Name and Email">
> If there aren't any other entities used by addressbook, then it might be worth
> moving/copying these onese there and not using this DTD at all. (And it's not
> more work for the translators, because it will be easier for them to match the
> entity when they have fewer DTD files to look in.)

There are actually more, but I'll move searchNameOrEmail.emptytext to abMainWindow.dtd (and I'll rename it to searchNameAndEmail).

Comment 58

[Stefan [:stefanh]]

Comment on attachment 358599 [details] [diff] [review]
mailNews, take 2

Actually, all our toolbar search fields are plagued by the fact that we don't check if they're visible when we're trying to focus them. I'm going to put up a new patch where I fix all search fields with a function in utilityoverlay.js

Comment 59

[Stefan [:stefanh]]

Attachment: mailNews, take 3

Changes compared with previous patch:

1) Uses a focusToolbarSearchBox function in utilityoverlay.js that checks if the search box is visible before it's focused. This function is used for all toolbar search boxes.

2) Moves the addressbook only entity to abMainWindow.dtd

Asking sr from Neil again, since I think I changed more than what was expected.

Comment 60

[[e-mail]]

Comment on attachment 358864 [details] [diff] [review]
mailNews, take 3

>+function focusToolbarSearchBox(aSearchBox)
I'm not sure that this is a good name for this, since we might possibly want to reuse it in other code, so maybe you could come up with a better name?

Comment 61

[Robert Kaiser]

(In reply to comment #58)
> (From update of attachment 358599 [details] [diff] [review])
> Actually, all our toolbar search fields are plagued by the fact that we don't
> check if they're visible when we're trying to focus them. I'm going to put up a
> new patch where I fix all search fields with a function in utilityoverlay.js

Does that matter for search boxes that can't be hidden right right now?

Comment 62

[Stefan [:stefanh]]

Re the name of the function - neil was ok with using "focusElement(aElement)" instead.

Comment 63

[Stefan [:stefanh]]

Attachment: password manager (pushed)

Turns out that I forgot Password Manager. I think checking if the relevant tab is selected works fine here (instead of including another file). I also added a close key (and one focus key is enough, I think - the toolkit window has 2). I re-used the toolkit entities, the only new suite entity is the emptytext one.

Comment 64

[Stefan [:stefanh]]

Comment on attachment 359912 [details] [diff] [review]
password manager (pushed)

Landed this as changeset 926acda4ef6d.

Comment 65

[Karsten Düsterloh]

Comment on attachment 358864 [details] [diff] [review]
mailNews, take 3

>+++ b/mailnews/addrbook/resources/content/addressbook.xul
>+  <key key="&focusSearchInput.key;" modifiers="accel"
>+       oncommand="focusToolbarSearchBox(document.getElementById('searchInput'));"/>

1. I'm not particularly fond of "wasting" rare ressources like shortcuts on mere focus actions, but I see the need here. What still bothers me is that it's not the same key as in the browser, where we use ^L to focus the urlbar - but ^K does nothing there. Neil, any chance we could change to using ^K in the browser as well? (Not in this bug, of course.) 
OTOH, in MailNews, ^L is used just for "Forward inline", we possibly could reassign that and use ^L everywhere?
(Yes, I know that FF uses ^L for the urlbar and ^K for the search box.)
2. If not all attributes fit into one line, use one line per attribute (id, label etc. first, handlers last) and align them vertically.

>+      <toolbarspring/>
>+      <toolbaritem id="searchBox" align="center">
>+        <textbox id="searchInput" type="search"
>+                 emptytext="&searchNameAndEmail.emptytext;"
>+                 oninput="SearchInputChanged();"
>+                 oncommand="onEnterInSearchBar();"
>+                 onkeypress="if (event.keyCode == KeyEvent.DOM_VK_RETURN) this.select();"
>+                 clickSelectsAll="true"/>
>+      </toolbaritem>

1. This has theming issues on Linux, both Classic and Modern; I'll attach a screenshot shortly.
2. If not all attributes fit into one line, use one line per attribute (id, label etc. first, handlers last) and align them vertically.

>+      <toolbaritem id="throbber-box" align="center">
>+        <button id="navigator-throbber" oncommand="goClickThrobber('addressbook.throbber.url')" tooltiptext="&throbber.tooltip;">
>+          <observes element="broadcaster_throbber" attribute="busy"/>
>+        </button>
>+      </toolbaritem>

Same (2.) here.

>+++ b/mailnews/base/resources/content/commandglue.js
>   if(isSentFolder)
>   {
>     tree.setAttribute("lastfoldersent", "true");
>-    searchCriteria.setAttribute("value", gMessengerBundle.getString("recipientSearchCriteria"));
>+    searchBox.emptyText = gMessengerBundle.getString("searchRecipientEmptyText");

While you're in the vicinity, spend a space on the if. ;-)

>+++ b/mailnews/base/resources/content/mailWindowOverlay.xul
>+  <key key="&focusSearchInput.key;" modifiers="accel"
>+       oncommand="focusToolbarSearchBox(document.getElementById('searchInput'));"/>

Same (2.) here.

>+    <textbox id="searchInput" flex="1" type="search" emptytext="&searchSubjectAndFrom.emptytext;"
>+             onkeypress="if (event.keyCode == KeyEvent.DOM_VK_RETURN) this.select();"
>+             clickSelectsAll="true" oncommand="onEnterInSearchBar();"/>
>+
>+    <button id="advancedButton" label="&advancedButton.label;" tooltiptext="&advancedButton.tooltip;" oncommand="onAdvancedSearch();"
>+            accesskey="&advancedButton.accesskey;"/>

And again.

>+++ b/suite/common/bookmarks/bookmarksManager.xul
>     <key key="&search.key;" modifiers="accel"
>-         oncommand="document.getElementById('search-box').focus();"/>
>+         oncommand="focusToolbarSearchBox(document.getElementById('search-box'));"/>

And here, too, while you're at it, please.

>+++ b/suite/common/utilityOverlay.js
>+function focusToolbarSearchBox(aSearchBox)

Bah, lowercase function names are the file's prevailing style, so at least it's consistent...

r- basically for theming issues and style nits. ^K vs. ^L should be talked about, though.

Comment 66

[Karsten Düsterloh]

Attachment: addressbook theming issues with mailnews v3 patch

Comment 67

[Stefan [:stefanh]]

Attachment: MailNews, take 4

This should fix all issues raised by Karsten. I also changed function name per Neil's comment #60 and my comment #62.

Comment 68

[Robert Kaiser]

(In reply to comment #65)
> ^K vs. ^L should be talked
> about, though.

^F or ^K are "Find"/"Search" while ^L is "Location"/"Address", I wouldn't mix those two. ^F and/or ^K would be something I'd see focusing/showing the find bar in the browser (if we had one).

Comment 69

[Stefan [:stefanh]]

Attachment: Mailnews part, updated for bitrot

It appears that attachment #360173 [details] [diff] [review] doesn't apply anymore - here's an updated patch.

Comment 70

[Mark Banner (:standard8)]

Comment on attachment 361653 [details] [diff] [review]
Mailnews part, updated for bitrot

I'm assuming Karsten/Neil have reviewed this in full. I'm not sure the history.xul change should be in this patch, but as long as you check that (there's a bit of bitrot in it as well), then r=me.

Comment 71

[Stefan [:stefanh]]

Attachment: Mailnews part, unbitrottened again (pushed).

I'd like to get this in for 2.0a3. I don't see any risk and it'd be nice to have a consistent search field ui for a3. Note that this patch touches 2 files shared with TB: mailnews.js (comment change) and msgViewPickerOverlay.xul (packaged by TB, but unused afaiks).

Comment 72

[Stefan [:stefanh]]

(In reply to comment #70)
> I'm not sure the
> history.xul change should be in this patch, but as long as you check that
> (there's a bit of bitrot in it as well), then r=me.

They should be, it's part of the switch to using focusElement(aElement) for focusing search fields (where possible) - see comment #59.

Comment 73

[Stefan [:stefanh]]

Comment on attachment 362387 [details] [diff] [review]
Mailnews part, unbitrottened again (pushed).

Pushed this as changeset 51a765135d91. For the record, Standard8 was OK with landing the parts that touched the shared files.

Comment 74

[Stefan [:stefanh]]

I think we're done here, so I'm resolving this as fixed.

Comment 75

[[e-mail]]

Comment on attachment 357446 [details] [diff] [review]
Helper apps prefpane, take 3 (pushed)

>     <hbox align="center">
>-      <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label>
>       <textbox id="filter" flex="1" type="search" clickSelectsAll="true"
>-               oncommand="gApplicationsPane._rebuildView();"/>
>+               emptytext="&search.emptytext;" oncommand="gApplicationsPane._rebuildView();"/>
>     </hbox>
I've only just noticed that this hbox is now redundant...