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 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.

Changes according to comment 3.

(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 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 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 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.

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