456229 - Implement <input type="search">

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Andy Lyttle]

We should implement <input type="search"> in a Safari-compatible way.  Attributes are:

placeholder="Search" - display this text in gray, when the value is empty and the field does not have focus

onsearch="foo();" - JavaScript function called when the user initiates a search

incremental - if present, onsearch fires on every keypress, and on clicking the cancel button; otherwise onsearch fires when pressing Enter

results - if present, shows a little magnifying glass icon, which helps to visually identify the field as a search box

results=10 - the magnifying glass functions as a drop-down menu containing a history of the last (whatever number) search terms

autosave="foo" - define a unique name to identify the search history (so the same history may be shared across multiple search boxes, even across different domains)

All other attributes of <input type="text"> are also supported.

Apple's implementation adds a cancel (x) button which appears whenever text has been typed into the box; clicking the cancel button clears the text without removing focus.  They also use rounded corners to make search boxes visually distinct from plain text boxes.  Pictures:
http://alexking.org/blog/2006/11/12/safari-search-boxes

Google has indicated that they are interested in adding support in Chrome:
http://code.google.com/p/chromium/issues/detail?id=17#c9

A relevant comment:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2007-May/011367.html

Comment 1

[Mounir Lamouri (:mounir)]

Actually, in HTML 5 specifications, the search input should behave exactly like a text input [1]. The results, onsearch and autosave attribute have been added by webkit.
The search input aims to improve accessibility by telling the user this input will be used for a search. We can tweak the style of the element to make sure it is clear it is here to search but you have to be cautious because it should not be used by webdevs for the "cool" looking [2].

By the way, webkit adds a "cancel" button which clear the value of the input.

[1] http://www.w3.org/TR/html5/forms.html#text-state-and-search-state
[2] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2007-May/011367.html

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Tests v0.1

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v0.1

Comment 4

[Mounir Lamouri (:mounir)]

As I said in comment 1, the search state is exactly the same as the text state so it's somewhat weird to implement. I've added NS_FORM_INPUT_SEARCH when I think it was needed and the tests are minimalistic. I don't think it is doable to test everything linked to the search state as I will have to test everything linked to the text state...

I think the interesting part would be the per-platform style. I don't know if the cancel button inside the search box is really needed. Maybe on MacOS X to keep the general UI style ?

Comment 5

[Mounir Lamouri (:mounir)]

CCing:
- David for accessibility. This new input state may interest you.
- Alex for UI questions.

Comment 6

[Andy Lyttle]

Attachment: Sample search field from Windows 7

A search field from Windows 7, which includes an "x" cancel button.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

check out the xul searchbox element to see how to style a search box for each platform (or perhaps directly use this element if possible, not entirely up to speed on what HTML5 defines for a search box).

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Search Box in GTK

The same style could be used for the input search state. As far as I know, depending on the platform, the magnifying glass has not the same look.
The magnifying glass will block the text (as it is for the search box) but will not be clickable (as opposite as the search box).

Alex, will you agree with this style ?

Comment 9

[Mounir Lamouri (:mounir)]

By the way, the specs does say we can style the search input so I think we can do the styling in a follow if you agree Jonas.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

I would prefer we did the styling in a follow up bug.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 435211 [details] [diff] [review]
Patch v0.1

Thank you for your review Jonas.
Olli, as you will review the telephone patch, could you sr this one ? They are very similar.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Tests v2

I've added new tests based on comments I got from Olli in bug 557620.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

This patch is based on patch from bug 557620 which is introducing helper methods.

Comment 14

[Mounir Lamouri (:mounir)]

Adding bug 557620 and bug 551670 in depend list because attached patches are based on patches from those bugs.

Jonas, do you want to re-review ?

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 435353 [details]
Sample search field from Windows 7

Let's talk about the field style in bug 558594.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 437878 [details] [diff] [review]
Tests v2

A few tests have been added. It's very similar to the tests from bug 557620.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 437880 [details] [diff] [review]
Patch v2

This patch is using the patch from bug 557620 so I'm asking Olli to review it too.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> CCing:
> - David for accessibility. This new input state may interest you.

What UI changes are going to be introduced? We should ensure new UI is accessible. 

Also it's worth to get Marco for testing when patch will be ready.

Comment 19

[Mounir Lamouri (:mounir)]

The UI is probably going to look like the searchbox. The decision will probably be taken in bug 558594.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 437878 [details] [diff] [review]
Tests v2

Would be great to add some tests for context menus etc. That could be done in a separate bug, so that you could test all the new input types there.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 437880 [details] [diff] [review]
Patch v2


> PRBool
> nsGenericHTMLFormElement::IsTextControl(PRBool aExcludePassword) const
> {
>   PRInt32 type = GetType();
>   return type == NS_FORM_INPUT_TEXT ||
>+         type == NS_FORM_INPUT_SEARCH ||
>          type == NS_FORM_INPUT_TELEPHONE ||
>          (!aExcludePassword && type == NS_FORM_INPUT_PASSWORD) ||
>          type == NS_FORM_TEXTAREA;
> }
> 
> PRBool
> nsGenericHTMLFormElement::IsSingleLineTextControl(PRBool aExcludePassword) const
> {
>   PRInt32 type = GetType();
>   return type == NS_FORM_INPUT_TEXT ||
>+         type == NS_FORM_INPUT_SEARCH ||
>          type == NS_FORM_INPUT_TELEPHONE ||
>          (!aExcludePassword && type == NS_FORM_INPUT_PASSWORD);
> }

Perhaps you could add a simple static method which these methods would use.
That way you wouldn't need to add new form types to both these methods.
You could do that in the "tel" patch or here.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> (From update of attachment 437878 [details] [diff] [review])
> Would be great to add some tests for context menus etc. That could be done in a
> separate bug, so that you could test all the new input types there.

bug 558651 and bug 559309 should cover that.

(In reply to comment #21)
> (From update of attachment 437880 [details] [diff] [review])
> 
> > PRBool
> > nsGenericHTMLFormElement::IsTextControl(PRBool aExcludePassword) const
> > {
> >   PRInt32 type = GetType();
> >   return type == NS_FORM_INPUT_TEXT ||
> >+         type == NS_FORM_INPUT_SEARCH ||
> >          type == NS_FORM_INPUT_TELEPHONE ||
> >          (!aExcludePassword && type == NS_FORM_INPUT_PASSWORD) ||
> >          type == NS_FORM_TEXTAREA;
> > }
> > 
> > PRBool
> > nsGenericHTMLFormElement::IsSingleLineTextControl(PRBool aExcludePassword) const
> > {
> >   PRInt32 type = GetType();
> >   return type == NS_FORM_INPUT_TEXT ||
> >+         type == NS_FORM_INPUT_SEARCH ||
> >          type == NS_FORM_INPUT_TELEPHONE ||
> >          (!aExcludePassword && type == NS_FORM_INPUT_PASSWORD);
> > }
> 
> Perhaps you could add a simple static method which these methods would use.
> That way you wouldn't need to add new form types to both these methods.
> You could do that in the "tel" patch or here.

I did that in 'tel' patch.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

This is is updated to conform to 'tel' patch.
Jonas, I'm asking you to sr it because it should be quite trivial to do that after the sr of the 'tel' patch. Do not hesitate to cancel the sr or give it to someone else if you do not have enough time.

Comment 24

[Mounir Lamouri (:mounir)]

Thank you for the reviews.
Now we need to wait for bug 551670 and bug 557620 to be landed before asking for a checkin.
I will probably work on the UI in a day or two.

Comment 25

[Mounir Lamouri (:mounir)]

r=smaug, sr=sicking

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Tests v2.1

r=smaug

Updated patch to apply correctly in the current tree.

Comment 27

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.2

r=smaug sr=sicking

Updated patch to apply correctly in the current tree.

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/f7a1cdc9d708

Comment 29

[Marco Bonardo [:mak]]

comment28 was the test, this is the patch:
http://hg.mozilla.org/mozilla-central/rev/ac8653880111

Comment 30

[Mounir Lamouri (:mounir)]

Thank you Marco :)

Comment 31

[Ruediger Lahl]

(In reply to comment #29)
> http://hg.mozilla.org/mozilla-central/rev/ac8653880111

With this patch you're no longer able to write in the Filter-section at about:config from SeaMonkey-Trunk.

Confirmed bei Hartmut Figge in the german nighty-builds-newsgroup by backing out the patch. MID: <[e-mail]>

Comment 32

[Mounir Lamouri (:mounir)]

(In reply to comment #31)
> (In reply to comment #29)
> > http://hg.mozilla.org/mozilla-central/rev/ac8653880111
> 
> With this patch you're no longer able to write in the Filter-section at
> about:config from SeaMonkey-Trunk.
> 
> Confirmed bei Hartmut Figge in the german nighty-builds-newsgroup by backing
> out the patch. MID: <[e-mail]>

Could you open a follow-up and attach the changes made by Hartmut Figge ? A link to the thread would be fine too.

Comment 33

[Ruediger Lahl]

(In reply to comment #32)

> > Confirmed bei Hartmut Figge in the german nighty-builds-newsgroup by backing
> > out the patch. MID: <[e-mail]>
> 
> Could you open a follow-up and attach the changes made by Hartmut Figge ?

Hartmut only backed out Marco's patch from https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c28 : http://hg.mozilla.org/mozilla-central/rev/ac8653880111 and compiled a new SeaMonkey with the former code.

> link to the thread would be fine too.

Message-ID <[e-mail]> or http://de.nntp2http.com/comm/software/mozilla/nightly-builds/2010/05/5fd901e2ec8183722b8a393d0ac604e5.html

Comment 34

[Ruediger Lahl]

(In reply to comment #33)

> https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c28

Sorry, https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c29 was right.

Comment 35

[Hartmut Figge]

(In reply to comment #32)

> Could you open a follow-up and attach the changes made by Hartmut Figge ?

First there was the regression range provided by Ruediger. Jens then showed the checkins for this timeframe.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-05-16+19%3A54%3A33&enddate=2010-05-17+23%3A12%3A28

http://hg.mozilla.org/mozilla-central/rev/ac8653880111 seemed to be worth a try, so i backed it out. The error was gone. An immediately following new build without outbacking showed the error again.

Comment 36

[Mounir Lamouri (:mounir)]

(In reply to comment #35)
> (In reply to comment #32)
> 
> > Could you open a follow-up and attach the changes made by Hartmut Figge ?
> 
> First there was the regression range provided by Ruediger. Jens then showed the
> checkins for this timeframe.
> 
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-05-16+19%3A54%3A33&enddate=2010-05-17+23%3A12%3A28
> 
> http://hg.mozilla.org/mozilla-central/rev/ac8653880111 seemed to be worth a
> try, so i backed it out. The error was gone. An immediately following new build
> without outbacking showed the error again.

That does not really help. Is the about:config field a <input type='search'> in Seamonkey ? Or the error is maybe more vicious. It should be great to find what part of the part is making about:config failing so we can fix it.
Could you investigate that ?

Comment 37

[Ruediger Lahl]

Attachment: about:config-Filter with DOM-I

Comment 38

[Ruediger Lahl]

(In reply to comment #37)
> Created an attachment (id=446605) [details]
> about:config-Filter with DOM-I

To comment that screenshot:

I talked about the Filterfield in about:config. about:config himself works fine. You only can't type anything in the filter-field.
And yes, this field looks like a <input type='search'>

Comment 39

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #31)
> (In reply to comment #29)
> > http://hg.mozilla.org/mozilla-central/rev/ac8653880111
> 
> With this patch you're no longer able to write in the Filter-section at
> about:config from SeaMonkey-Trunk.

That regression should have been fixed through bug 566716 (cf. bug 566716 comment 3). Note that SM uses the same file for about:config as FF (config.xul) but SM's Type Ahead Find was interfering.

Comment 40

[Mounir Lamouri (:mounir)]

(In reply to comment #39)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > http://hg.mozilla.org/mozilla-central/rev/ac8653880111
> > 
> > With this patch you're no longer able to write in the Filter-section at
> > about:config from SeaMonkey-Trunk.
> 
> That regression should have been fixed through bug 566716 (cf. bug 566716
> comment 3). Note that SM uses the same file for about:config as FF (config.xul)
> but SM's Type Ahead Find was interfering.

Great ! Thank you for the information Jens :)

Comment 41

[Hartmut Figge]

(In reply to comment #39)

> That regression should have been fixed through bug 566716

Indeed, it is fixed now as my fresh build shows. :)

Comment 42

[Ruediger Lahl]

(In reply to comment #41)

> Indeed, it is fixed now as my fresh build shows. :)

I can confirm that too. Thanks.

Comment 43

[Eric Shepherd [:sheppy]]

Janet documented this:

https://developer.mozilla.org/en/HTML/Element/input