317419 - Use new clickSelectsAll property in quick search fields

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[[e-mail]]

Offhand I know of three: help, mail, addressbook.

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> Offhand I know of three: help, mail, addressbook.
> 

Don't forget the address book sidebar as well ;-)

Comment 2

[[e-mail]]

Speaking of sidebars, bookmarks sidebar has a search field, and the bookmarks manager has one too.

Comment 3

[Serge Gautherie (:sgautherie)]

Filter "spam" on "guifeatures-nobody-20080610".

Comment 4

[Nochum Sossonko [:Natch]]

Attachment: This is for browser stuff (i.e. side-bar)

This patches the history and bookmarks sidebar, the browser search bar already has clickSelectsAll set.

Comment 5

[Nochum Sossonko [:Natch]]

Attachment: This is for toolkit stuff (ie dm em)

Heres for the dm and em, this is all (I believe) the searchbars in firefox, anyone care to point me to the others?

Comment 6

[Ian Neal]

Be nice to do the SeaMonkey ones too seeing this is a SeaMonkey bug!

Comment 7

[Nickolay_Ponomarev]

As http://developer.mozilla.org/En/Getting_your_patch_in_the_tree says, 
you should request reviews on the patches according to the rules of the area you're changing. In this case these two pages are relevant:

http://www.mozilla.org/projects/firefox/review.html
http://www.mozilla.org/projects/toolkit/review.html

Comment 8

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

Do any of these have existing code that implements the behavior that would need to be removed? Would be best to put Firefox patches in a Firefox bug, really...

Comment 9

[Nochum Sossonko [:Natch]]

(In reply to comment #8)
> Do any of these have existing code that implements the behavior that would need
> to be removed? Would be best to put Firefox patches in a Firefox bug, really...
> 

none of those places display that behavior yet, did check for it though and the only place is the browser top search bar... and it uses clickselectsall

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: Revised toolkit patch to include help.xul

Any other apps/widgets etc. please point them out to me.

Thanks

Comment 11

[Nochum Sossonko [:Natch]]

Comment on attachment 336009 [details] [diff] [review]
Revised toolkit patch to include help.xul

sorry cant use clickselectsall on a toolbaritem

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: [checked in] Thunderbird patch (mail/sidebar)

Here's some Seamonkey stuff the FF stuff I put in bug 453255.

(Help isn't used in FF anymore but I put it in the other bug b/c it's in toolkit)

Comment 13

[[e-mail]]

Comment on attachment 336443 [details] [diff] [review]
[checked in] Thunderbird patch (mail/sidebar)

Actually this is all Thunderbird code...

Comment 14

[Phil Ringnalda (:philor)]

Comment on attachment 336443 [details] [diff] [review]
[checked in] Thunderbird patch (mail/sidebar)

Well, even if nobody else likes your patches in this bug, I do - among other things, it fixes bug 451051. r=me, thanks, and pushed in http://hg.mozilla.org/comm-central/rev/676331118c6d

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: Hope I got Seamonkey here:)

If there's anything I missed feel free to point it out. I stuck a clickSelectsAll in cookieViewer.xul, just pull it out if you don't like it ;).

Asking neil for review (just mentioning it here b/c in the past my attempts at figuring out your review name failed :(, there ought to be an easier more intuitive way to find those... not this bug though)

Comment 16

[[e-mail]]

Comment on attachment 338382 [details] [diff] [review]
Hope I got Seamonkey here:)

>-        <textbox id="sidebar-search-text" flex="1"
>+        <textbox id="sidebar-search-text" flex="1" clickSelectsAll="true"
>                    onkeypress="if (event.keyCode == 13) doSearch();"
>                    oninput="doEnabling();" />
Please only do the type="search" textboxes.
(Don't even do this one which should be a type="search" but isn't.)
Also, there are some type="search" textboxes in mailnews/ of which I think one belongs to suite and the others are all shared with Thunderbird.

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: Addresses Comment #16

Alright I got the mailnews and took out the ones without type="search", some of the files have onfocus handlers that do this.select(), I can't remove them b/c there are keys and labels attached to them that wouldn't select the text without these handlers (IMO a focusSelectsAll would do better in these cases). I mixed all the mailnews stuff in this patch, dunno if i need a TB review or if you want me to split them up...

Again review ? (neil)

Comment 18

[[e-mail]]

Comment on attachment 338540 [details] [diff] [review]
Addresses Comment #16

>       <textbox id="searchInput" flex="1" type="search"
>-               oncommand="onEnterInSearchBar();"/>  
>+               oncommand="onEnterInSearchBar();"
>+               clickSelectsAll="true"/>  
Nit: trailing spaces

> 	  <textbox id="searchInput" flex="1" type="search"
>                aria-labelledby="lookInLabel addressbookList forLabel"
>-               oncommand="onEnterInSearchBar();"/>
>+               oncommand="onEnterInSearchBar();"
>+               clickSelectsAll="true"/>
> 	</hbox>
For these it would be neater if you put clickSelectsAll on the previous line so that the patch isn't so messy.

>-                onfocus="this.select();" onclick="this.select();"/>
>+                onfocus="this.select()" clickSelectsAll="true"/>
Ah yes, if you're on Linux and use an Alt shortcut to focus the field then it normally won't select all by default. (Maybe we should remove this anyway?)

>+  <textbox id="searchInput" flex="1" onfocus="this.select();" clickSelectsAll="true" onkeypress="if (event.keyCode == KeyEvent.DOM_VK_RETURN) this.select();" oncommand="onEnterInSearchBar();" type="search"/>
For very long lines such as this it would be handy if you could manage to wrap it to fit to about 80 columns. (I know, it's my fault for not doing it when I added type="search".)

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: Nits addressed [Checkin: Comment 22]

(In reply to comment #18)
Done.

I also removed the onfocus handler as it's inconsistent with the rest of the search textboxes and per your previous comment.

Comment 20

[Nochum Sossonko [:Natch]]

neil can this get checked in now or do you want to re-review it?

Comment 21

[[e-mail]]

We're in alpha freeze right at the moment, but once we release you can add the checkin-needed keyword and someone will push it to comm-central.

Comment 22

[[e-mail]]

Pushed changeset 4e3fbb4603b5 to comm-central.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 338562 [details] [diff] [review]
Nits addressed
[Checkin: Comment 22]

"abort: patch failed to apply"

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 338562 [details] [diff] [review]
Nits addressed
[Checkin: Comment 22]

(In reply to comment #23)
> (From update of attachment 338562 [details] [diff] [review])
> "abort: patch failed to apply"

Already checked in :-/