Closed Bug 456229 Opened 16 years ago Closed 14 years ago

Implement <input type="search">

Categories

(Core :: Layout: Form Controls, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: mozilla7, Assigned: mounir)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, html5, Whiteboard: [parity-webkit])

Attachments

(3 files, 7 obsolete files)

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
Depends on: 457800
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
Keywords: helpwanted
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
No longer depends on: 457800
Keywords: html5
Whiteboard: [parity-webkit]
Version: unspecified → Trunk
Assignee: nobody → mounir.lamouri
Summary: Implement <input type="search"> for compatibility with Safari → Implement <input type="search">
Attached patch Tests v0.1 (obsolete) — Splinter Review
Attachment #435209 - Flags: review?(jonas)
Attached patch Patch v0.1 (obsolete) — Splinter Review
Attachment #435211 - Flags: review?(jonas)
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 ?
Status: NEW → ASSIGNED
CCing:
- David for accessibility. This new input state may interest you.
- Alex for UI questions.
Attached image Sample search field from Windows 7 (obsolete) —
A search field from Windows 7, which includes an "x" cancel button.
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).
Attached image Search Box in GTK (obsolete) —
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 ?
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.
I would prefer we did the styling in a follow up bug.
Attachment #435209 - Flags: superreview?(Olli.Pettay)
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.
Attachment #435211 - Flags: superreview?(Olli.Pettay)
Attached patch Tests v2 (obsolete) — Splinter Review
I've added new tests based on comments I got from Olli in bug 557620.
Attachment #435209 - Attachment is obsolete: true
Attachment #435209 - Flags: superreview?(Olli.Pettay)
Attached patch Patch v2 (obsolete) — Splinter Review
This patch is based on patch from bug 557620 which is introducing helper methods.
Attachment #435211 - Attachment is obsolete: true
Attachment #435211 - Flags: superreview?(Olli.Pettay)
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 ?
Depends on: 557620, 551670
Blocks: 558594
Attachment #436663 - Attachment is obsolete: true
Comment on attachment 435353 [details]
Sample search field from Windows 7

Let's talk about the field style in bug 558594.
Attachment #435353 - Attachment is obsolete: true
Blocks: 558651
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.
Attachment #437878 - Flags: review?(Olli.Pettay)
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.
Attachment #437880 - Flags: review?(Olli.Pettay)
(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.
The UI is probably going to look like the searchbox. The decision will probably be taken in bug 558594.
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.
Attachment #437878 - Flags: review?(Olli.Pettay) → review+
Attachment #437880 - Flags: review?(Olli.Pettay) → review+
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.
Blocks: 559309
(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.
Attached patch Patch v2.1 (obsolete) — Splinter Review
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.
Attachment #437880 - Attachment is obsolete: true
Attachment #438984 - Flags: superreview?(jonas)
Attachment #438984 - Flags: superreview?(jonas) → superreview+
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.
Blocks: 345822
Blocks: 562625
Keywords: checkin-needed
r=smaug, sr=sicking
Attached patch Tests v2.1Splinter Review
r=smaug

Updated patch to apply correctly in the current tree.
Attachment #437878 - Attachment is obsolete: true
Attached patch Patch v2.2Splinter Review
r=smaug sr=sicking

Updated patch to apply correctly in the current tree.
Attachment #438984 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/f7a1cdc9d708
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Thank you Marco :)
Flags: in-testsuite+
Blocks: 566348
(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: <4BF46D8F.6030903@hfigge.myfqdn.de>
(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: <4BF46D8F.6030903@hfigge.myfqdn.de>

Could you open a follow-up and attach the changes made by Hartmut Figge ? A link to the thread would be fine too.
(In reply to comment #32)

> > Confirmed bei Hartmut Figge in the german nighty-builds-newsgroup by backing
> > out the patch. MID: <4BF46D8F.6030903@hfigge.myfqdn.de>
> 
> 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 <4BF44FD2.3020407@hfigge.myfqdn.de> or http://de.nntp2http.com/comm/software/mozilla/nightly-builds/2010/05/5fd901e2ec8183722b8a393d0ac604e5.html
(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.
(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 ?
(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'>
(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.
(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 :)
(In reply to comment #39)

> That regression should have been fixed through bug 566716

Indeed, it is fixed now as my fresh build shows. :)
(In reply to comment #41)

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

I can confirm that too. Thanks.
Depends on: 570230
Blocks: 581596
Blocks: 1055085
You need to log in before you can comment on or make changes to this bug.