Last Comment Bug 769385 - add date to the list of valid types attributes for <input>
: add date to the list of valid types attributes for <input>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Raphael Catolino (:raphc)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 764481
Blocks: 769352 769355 769357 769359 769370 769379
  Show dependency treegraph
 
Reported: 2012-06-28 11:27 PDT by Raphael Catolino (:raphc)
Modified: 2013-01-07 08:34 PST (History)
2 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (18.63 KB, patch)
2012-06-28 11:42 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (22.29 KB, patch)
2012-07-04 01:04 PDT, Raphael Catolino (:raphc)
mounir: review+
Details | Diff | Splinter Review
patch (23.77 KB, patch)
2012-07-12 05:36 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (23.77 KB, patch)
2012-07-12 05:59 PDT, Raphael Catolino (:raphc)
mounir: review+
Details | Diff | Splinter Review
patch (23.88 KB, patch)
2012-07-16 00:07 PDT, Raphael Catolino (:raphc)
mounir: review+
Details | Diff | Splinter Review
patch (25.63 KB, patch)
2012-08-16 11:12 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review

Description Raphael Catolino (:raphc) 2012-06-28 11:27:00 PDT

    
Comment 1 Raphael Catolino (:raphc) 2012-06-28 11:42:34 PDT
Created attachment 637618 [details] [diff] [review]
patch
Comment 2 Raphael Catolino (:raphc) 2012-06-28 11:50:40 PDT
Comment on attachment 637618 [details] [diff] [review]
patch

I thought about using DoesMinMaxApply() in

> +  if (mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE) {

and other places where something apply to date, number and will apply to the other date/time types (when they are implemented), but the relation between MinMaxApply and those types is not obvious at first sight so I wonder whether this is really a good idea.
Comment 3 Mounir Lamouri (:mounir) 2012-07-03 16:40:47 PDT
Comment on attachment 637618 [details] [diff] [review]
patch

Review of attachment 637618 [details] [diff] [review]:
-----------------------------------------------------------------

You probably also want to have the NS_FORM_INPUT_DATE seen as a single line text control for the moment. But MozIsTexTField() will have to return false. That would also require you to make sure @placeholder and @pattern does not apply.

Also, I think this test is going to fail: toolkit/components/satchel/test/test_form_autocomplete.html
I think we could have no autocompletion on date fields.

Please attach a new patch with those changes.

::: content/html/content/public/nsIFormControl.h
@@ +61,5 @@
>    NS_FORM_INPUT_SUBMIT,
>    NS_FORM_INPUT_TEL,
>    NS_FORM_INPUT_TEXT,
>    NS_FORM_INPUT_URL,
> +  NS_FORM_INPUT_DATE,

Please keep them alphabetically ordered.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +129,5 @@
>    { "submit", NS_FORM_INPUT_SUBMIT },
>    { "tel", NS_FORM_INPUT_TEL },
>    { "text", NS_FORM_INPUT_TEXT },
>    { "url", NS_FORM_INPUT_URL },
> +  { "date", NS_FORM_INPUT_DATE },

Keep this list alphabetically ordered too.

@@ +134,5 @@
>    { 0 }
>  };
>  
>  // Default type is 'text'.
>  static const nsAttrValue::EnumTable* kInputDefaultType = &kInputTypeTable[13];

... which is going to require you to change that.

@@ +3772,5 @@
>      case NS_FORM_INPUT_TEL:
>      case NS_FORM_INPUT_EMAIL:
>      case NS_FORM_INPUT_URL:
> +    case NS_FORM_INPUT_DATE:
> +      // TODO: move NS_FORM_INPUT_DATE to minmax apply, once it's implemented

I would prefer if you could change IsRangeOverflow and IsRangeUnderflow instead.
Comment 4 Raphael Catolino (:raphc) 2012-07-04 01:04:48 PDT
Created attachment 638993 [details] [diff] [review]
patch

Changes according to comment 3.
Comment 5 Raphael Catolino (:raphc) 2012-07-04 01:14:25 PDT
(In reply to Mounir Lamouri (:mounir) from comment #3)

> Also, I think this test is going to fail:
> toolkit/components/satchel/test/test_form_autocomplete.html
> I think we could have no autocompletion on date fields.

I removed the date type from the todo_list.
Actually the test wasn't failing because it's not done under the pref dom.experimental_forms.
That seems not to pose any problem with the <input type=number> autocompletion test (the behavior is the same with the fallback type=text I guess). But I'd like to add the pref in that test for coherence's sake. I'd rather do that in bug 764481 patch though.
Comment 6 Mounir Lamouri (:mounir) 2012-07-12 02:57:13 PDT
Comment on attachment 638993 [details] [diff] [review]
patch

Review of attachment 638993 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following comments applied.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1225,5 @@
>  
>  NS_IMETHODIMP
>  nsHTMLInputElement::MozIsTextField(bool aExcludePassword, bool* aResult)
>  {
>    // TODO: temporary until bug 635240 is fixed.

Can you update that comment and create a bug about <input type='date'> layout? (could be for all date/time types.

@@ +2621,5 @@
>        bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
>        if (success) {
>          newType = aResult.GetEnumValue();
> +        if ((newType == NS_FORM_INPUT_NUMBER ||
> +            newType == NS_FORM_INPUT_DATE) &&

nit: align the two |newType|. IOW, add a space before that line above.

@@ +3958,5 @@
>  bool
>  nsHTMLInputElement::IsRangeOverflow() const
>  {
>    nsAutoString maxStr;
> +  if (mType != NS_FORM_INPUT_NUMBER ||

I wasn't expecting that change but more something like:
if (!DoesMinMaxApply() || mType == NS_FORM_INPUT_DATE || [...]

@@ +3982,5 @@
>  bool
>  nsHTMLInputElement::IsRangeUnderflow() const
>  {
>    nsAutoString minStr;
> +  if (mType != NS_FORM_INPUT_NUMBER ||

ditto

::: layout/base/nsCSSFrameConstructor.cpp
@@ +3418,5 @@
>      SIMPLE_INT_CREATE(NS_FORM_INPUT_URL, NS_NewTextControlFrame),
>      SIMPLE_INT_CREATE(NS_FORM_INPUT_PASSWORD, NS_NewTextControlFrame),
>      // TODO: this is temporary until a frame is written: bug 635240.
>      SIMPLE_INT_CREATE(NS_FORM_INPUT_NUMBER, NS_NewTextControlFrame),
> +    SIMPLE_INT_CREATE(NS_FORM_INPUT_DATE, NS_NewTextControlFrame),

Please, add a comment pointing to the layout bug.

::: toolkit/components/satchel/test/test_form_autocomplete.html
@@ +136,5 @@
>  fh.addEntry("field10", "42");
>  fh.addEntry("searchbar-history", "blacklist test");
>  
>  // All these non-implemeted types might need autocomplete tests in the future.
> +var todoTypes = [ "datetime", "month", "week", "time", "datetime-local",

Can you write a real test for that.
Comment 7 Raphael Catolino (:raphc) 2012-07-12 05:36:13 PDT
Created attachment 641434 [details] [diff] [review]
patch
Comment 8 Raphael Catolino (:raphc) 2012-07-12 05:59:46 PDT
Created attachment 641438 [details] [diff] [review]
patch
Comment 9 Mounir Lamouri (:mounir) 2012-07-15 19:49:04 PDT
Comment on attachment 641438 [details] [diff] [review]
patch

Review of attachment 641438 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +4026,5 @@
>  
>  bool
>  nsHTMLInputElement::IsRangeOverflow() const
>  {
> +  if (!DoesMinMaxApply() || mType == NS_FORM_INPUT_DATE) {

Please add a comment saying that NS_FORM_INPUT_DATE is temporary excluded because it's not implemented yet.
Ideally, with a bug number ;)

@@ +4046,5 @@
>  
>  bool
>  nsHTMLInputElement::IsRangeUnderflow() const
>  {
> +  if (!DoesMinMaxApply() || mType == NS_FORM_INPUT_DATE) {

ditto
Comment 10 Raphael Catolino (:raphc) 2012-07-16 00:07:59 PDT
Created attachment 642496 [details] [diff] [review]
patch
Comment 11 Mounir Lamouri (:mounir) 2012-08-06 02:25:16 PDT
Comment on attachment 642496 [details] [diff] [review]
patch

Review of attachment 642496 [details] [diff] [review]:
-----------------------------------------------------------------

I did r+ the previous version. No need to re-ask for a review in that case unless you did major changes.
Comment 12 Raphael Catolino (:raphc) 2012-08-16 11:12:59 PDT
Created attachment 652501 [details] [diff] [review]
patch

Removed the b2g hack for date inputs in /b2g/chrome/content/forms.js.
Comment 13 Mounir Lamouri (:mounir) 2012-12-28 04:56:56 PST
https://hg.mozilla.org/mozilla-central/rev/0ec205214f5f

Note You need to log in before you can comment on or make changes to this bug.