769385 - add date to the list of valid types attributes for <input>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 2

[Raphael Catolino (:raphc)]

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)]

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)]

Attachment: patch

Changes according to comment 3.

Comment 5

[Raphael Catolino (:raphc)]

(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)]

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)]

Attachment: patch

Comment 8

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 9

[Mounir Lamouri (:mounir)]

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)]

Attachment: patch

Comment 11

[Mounir Lamouri (:mounir)]

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)]

Attachment: patch

Removed the b2g hack for date inputs in /b2g/chrome/content/forms.js.

Comment 13

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/0ec205214f5f