This bug will need to deal with the conversions between string/date and string/number as described here http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#date-state-%28type=date%29 A lot of code form js/src/jsdate.cpp could be useful to do those conversions, though I'm not sure of the proper way to reuse it.

Comment on attachment 638780 [details] [diff] [review] patch Review of attachment 638780 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I think you attached the wrong patch here ;)

>Hmm, I think you attached the wrong patch here ;) Indeed.

Comment on attachment 639007 [details] [diff] [review] patch Review of attachment 639007 [details] [diff] [review]: ----------------------------------------------------------------- Those dates and timezones are horrible... :( I will need to have another look at this patch after the comments are applied. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +992,5 @@ > + break; > + case NS_FORM_INPUT_DATE: > + { > + JSContext* ctx = (JSContext*) OwnerDoc()->GetScriptGlobalObject()-> > + GetContext()->GetNativeContext(); Use nsContentUtils::GetContextFromDocument(OwnerDoc()). You will have to check if the return value is null. @@ +994,5 @@ > + { > + JSContext* ctx = (JSContext*) OwnerDoc()->GetScriptGlobalObject()-> > + GetContext()->GetNativeContext(); > + PRUint32 year, month, day; > + if (NS_FAILED(GetValueAsDate(year, month, day, stringValue))) { This method doesn't exist. Is it in a patch I haven't reviewed? @@ +1003,5 @@ > + // the tz into account > + JSObject* date = JS_NewDateObject(ctx, year, month-1, day+1, 0, 0, 0); > + jsval hour = INT_TO_JSVAL(0); > + jsval rval; > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); Why do you do that? @@ +1006,5 @@ > + jsval rval; > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); > + jsval timestamp; > + if (JS_FALSE == JS_CallFunctionName(ctx, date, "getTime", 0, NULL, > + &timestamp)) { Well, this might be working but according to the documentation, this should return an UTC-based time. However, it seems that we don't do that. @@ +1085,5 @@ > + break; > + case NS_FORM_INPUT_DATE: > + { > + JSContext* ctx = (JSContext*) OwnerDoc()->GetScriptGlobalObject()-> > + GetContext()->GetNativeContext(); Use nsContentUtils. @@ +1110,5 @@ > + > + PRUint32 year, month, day; > + nsAutoString value; > + GetValueInternal(value); > + if (NS_FAILED(GetValueAsDate(year, month, day, value))) { That method doesn't exist. @@ +1115,5 @@ > + *aDate = JSVAL_NULL; > + return NS_OK; > + } > + > + JSObject * date = JS_NewDateObject(aCtx, year, month-1, day+1, 0, 0, 0); nit: JSObject* date = [...] @@ +1118,5 @@ > + > + JSObject * date = JS_NewDateObject(aCtx, year, month-1, day+1, 0, 0, 0); > + jsval hour = INT_TO_JSVAL(0); > + jsval rval; > + JS_CallFunctionName(aCtx, date, "setUTCHours", 1, &hour, &rval); why? @@ +1119,5 @@ > + JSObject * date = JS_NewDateObject(aCtx, year, month-1, day+1, 0, 0, 0); > + jsval hour = INT_TO_JSVAL(0); > + jsval rval; > + JS_CallFunctionName(aCtx, date, "setUTCHours", 1, &hour, &rval); > + *aDate = OBJECT_TO_JSVAL(date); aDate->setObjectOrNull(date); @@ +1130,5 @@ > + if (mType != NS_FORM_INPUT_DATE ) { > + return NS_ERROR_DOM_INVALID_STATE_ERR; > + } > + > + if (JSVAL_IS_PRIMITIVE(aDate)) { Maybe you should do: if (!aDate.isObject() || !JS_ObjectIsDate(aDate)) { @@ +1136,5 @@ > + SetValue(EmptyString()); > + return NS_OK; > + } > + > + JSObject * date = JSVAL_TO_OBJECT(aDate); Given that this is an object you can do: JSObject& date = aDate.toObject(); @@ +1145,5 @@ > + return NS_OK; > + } > + > + double value = timestamp.toNumber(); > + if (value != std::numeric_limits<double>::quiet_NaN()) { if (!MOZ_DOUBLE_IS_NAN(value) { ::: content/html/content/test/forms/test_valueasdate_attribute.html @@ +106,5 @@ > + var validData = > + [ > + "1970-01-01", > + "2255-12-12", // we must support the same date range as the ecma date object > + "1685-01-01", more tests @@ +111,5 @@ > + ]; > + var invalidData = > + [ > + "invaliddate", > + "", // the empty string is not a date ditto @@ +133,5 @@ > + var validData = > + [ > + "1970-01-01", > + "2255-12-12", // we must support the same date range as the ecma date object > + "1685-01-01", more tests @@ +138,5 @@ > + ]; > + var invalidData = > + [ > + "invaliddate", > + "", // the empty string is not a date ditto @@ +144,5 @@ > + > + element.type = "date"; > + for each (data in validData) { > + element.valueAsDate = new Date(data); > + is(element.value, data, "valueAsDate should set the value"); You might want to compare .valueAsDate and new Date(data). ::: content/html/content/test/forms/test_valueasnumber_attribute.html @@ +170,5 @@ > +{ > + var validData = > + [ > + "1970-01-01", > + "2255-12-12", // we must support the same date range as the ecma date object I think that comment isn't useful. Also, feel free to add more tests. Particularly tests value before 1970-01-01: Date.parse() will return NaN but it shouldn't return that. Depending on the implementation, that might need to be a todo() or not. In any case, you should explain that we have a bug in our JS engine, add the bug number (which requires searching the bug, hoping it exists). @@ +176,5 @@ > + ]; > + var invalidData = > + [ > + "invaliddate", > + "", // the empty string is not a date You can add a bit more invalid dates. No need to be over-careful here because we have other tests checking date validity but better to have more than two quite obvious invalid dates. @@ +192,5 @@ > + "when the element value is not a valid date"); > + } > +} > + > +function checkDateSet() Same thing for the tests. @@ +208,5 @@ > + ]; > + > + element.type = "date"; > + for each (data in validData) { > + element.valueAsDate = new Date(data); I think you want to do: element.valueAsNumber = [...] .valueAsDate is in another test ;) @@ +212,5 @@ > + element.valueAsDate = new Date(data); > + is(element.value, data, "valueAsDate should set the value"); > + } > + for each (data in invalidData) { > + element.valueAsDate = new Date(data); ditto @@ +222,5 @@ > checkAvailability(); > +checkNumberGet(); > +checkNumberSet(); > +checkDateGet(); > +checkDateSet(); For readability, could you do: // Tests for <input type='number'> checkNumberGet(); checkNumberSet(); // Tests for <input type='date'> checkDateGet(); checkDateSet();

(In reply to Mounir Lamouri (:mounir) from comment #4) > @@ +1003,5 @@ > > + // the tz into account > > + JSObject* date = JS_NewDateObject(ctx, year, month-1, day+1, 0, 0, 0); > > + jsval hour = INT_TO_JSVAL(0); > > + jsval rval; > > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); > > Why do you do that? > > @@ +1006,5 @@ > > + jsval rval; > > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); > > + jsval timestamp; > > + if (JS_FALSE == JS_CallFunctionName(ctx, date, "getTime", 0, NULL, > > + &timestamp)) { > > Well, this might be working but according to the documentation, this should > return an UTC-based time. However, it seems that we don't do that. The problem is the JS_NewDateObject method. We want a date whose time is set on midnight UTC, but the constructor understand that we pass a date whose time is set on midnight in the local tz. So we have to reset the time to midnight. I'm actually just thinking that this code might fail for negative time-zones. > @@ +1118,5 @@ > > + > > + JSObject * date = JS_NewDateObject(aCtx, year, month-1, day+1, 0, 0, 0); > > + jsval hour = INT_TO_JSVAL(0); > > + jsval rval; > > + JS_CallFunctionName(aCtx, date, "setUTCHours", 1, &hour, &rval); > > why? See comment above. > @@ +1130,5 @@ > > + if (mType != NS_FORM_INPUT_DATE ) { > > + return NS_ERROR_DOM_INVALID_STATE_ERR; > > + } > > + > > + if (JSVAL_IS_PRIMITIVE(aDate)) { > > Maybe you should do: > if (!aDate.isObject() || !JS_ObjectIsDate(aDate)) { Actually JS_ObjectIsDate() returns true for date objects created in the mochitests, but return false for date objects created in normal js. I think we should find an other way to check if it's a Date object. Maybe test if its proto is equal to a new Date object proto. Or something similar. > ::: content/html/content/test/forms/test_valueasdate_attribute.html > @@ +106,5 @@ > > + var validData = > > + [ > > + "1970-01-01", > > + "2255-12-12", // we must support the same date range as the ecma date object > > + "1685-01-01", > > more tests > > @@ +111,5 @@ > > + ]; > > + var invalidData = > > + [ > > + "invaliddate", > > + "", // the empty string is not a date > > ditto > > @@ +133,5 @@ > > + var validData = > > + [ > > + "1970-01-01", > > + "2255-12-12", // we must support the same date range as the ecma date object > > + "1685-01-01", > > more tests > > @@ +138,5 @@ > > + ]; > > + var invalidData = > > + [ > > + "invaliddate", > > + "", // the empty string is not a date > > ditto > > @@ +144,5 @@ > > + > > + element.type = "date"; > > + for each (data in validData) { > > + element.valueAsDate = new Date(data); > > + is(element.value, data, "valueAsDate should set the value"); > > You might want to compare .valueAsDate and new Date(data). > > ::: content/html/content/test/forms/test_valueasnumber_attribute.html > @@ +170,5 @@ > > +{ > > + var validData = > > + [ > > + "1970-01-01", > > + "2255-12-12", // we must support the same date range as the ecma date object > > I think that comment isn't useful. > Also, feel free to add more tests. I changed a lot of things in those tests. In particular I tried not to rely as much on the date object and use expected values instead. > Particularly tests value before 1970-01-01: Date.parse() will return NaN but > it shouldn't return that. Depending on the implementation, that might need > to be a todo() or not. In any case, you should explain that we have a bug in > our JS engine, add the bug number (which requires searching the bug, hoping > it exists). Given our discussion on irc, I think this is ok, right?

(In reply to Raphael Catolino from comment #6) > (In reply to Mounir Lamouri (:mounir) from comment #4) > > > @@ +1003,5 @@ > > > + // the tz into account > > > + JSObject* date = JS_NewDateObject(ctx, year, month-1, day+1, 0, 0, 0); > > > + jsval hour = INT_TO_JSVAL(0); > > > + jsval rval; > > > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); > > > > Why do you do that? > > > > @@ +1006,5 @@ > > > + jsval rval; > > > + JS_CallFunctionName(ctx, date, "setUTCHours", 1, &hour, &rval); > > > + jsval timestamp; > > > + if (JS_FALSE == JS_CallFunctionName(ctx, date, "getTime", 0, NULL, > > > + &timestamp)) { > > > > Well, this might be working but according to the documentation, this should > > return an UTC-based time. However, it seems that we don't do that. > > The problem is the JS_NewDateObject method. We want a date whose time is set > on midnight UTC, but the constructor understand that we pass a date whose > time is set on midnight in the local tz. So we have to reset the time to > midnight. I'm actually just thinking that this code might fail for negative > time-zones. And that was the case. I don't use the constructor from jsapi to set the time any more. I use the setUTC* methods of the date object instead so that I won't have any problems with the timezones.

Comment on attachment 642564 [details] [diff] [review] patch Review of attachment 642564 [details] [diff] [review]: ----------------------------------------------------------------- Try to use C++ JS API which means you should avoid macros from jsapih. but JS::Value methods and you should use JS::Call() instead of JS_CallFunctionName(). Also, you are checking sometimes the return value of JS_CallFunctionName() but sometimes you don't. Be consistent and always check the return value. With JS::Call(), you get a simple |bool| so you can simply do: |if (!JS::Call([...])) {|. Looks good but I would like to see another version of the patch with the comments fixed. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1141,5 @@ > + if (!date) { > + break; > + } > + > + jsval timestamp = DOUBLE_TO_JSVAL(aValue); Could be: jsval timestamp; timestamp.setDouble(aValue); I wouldn't mind if you keep the macro syntax given that it is more readable in that case. @@ +1143,5 @@ > + } > + > + jsval timestamp = DOUBLE_TO_JSVAL(aValue); > + jsval rval; > + JS_CallFunctionName(ctx, date, "setUTCMilliseconds", 1, &timestamp, &rval); You could actually prevent doing that call and pass |timestamp| to |JS_NewDateObjectMsec|. This method is calling |setUTCMilliseconds|, see jsdate.cpp. @@ +1146,5 @@ > + jsval rval; > + JS_CallFunctionName(ctx, date, "setUTCMilliseconds", 1, &timestamp, &rval); > + > + jsval year, month, day; > + JS_CallFunctionName(ctx, date, "getUTCFullYear", 0, NULL, &year); Use |JS::Call| instead of |JS_CallFunctionName| and |nullptr| instead of |NULL|. @@ +1162,5 @@ > NS_IMETHODIMP > +nsHTMLInputElement::GetValueAsDate(JSContext* aCtx, jsval* aDate) > +{ > + if (mType != NS_FORM_INPUT_DATE) { > + *aDate = JSVAL_NULL; aDate->setNull(); @@ +1170,5 @@ > + PRUint32 year, month, day; > + nsAutoString value; > + GetValueInternal(value); > + if (!GetValueAsDate(value, year, month, day)) { > + *aDate = JSVAL_NULL; aDate->SetNull(); @@ +1193,5 @@ > + if (mType != NS_FORM_INPUT_DATE) { > + return NS_ERROR_DOM_INVALID_STATE_ERR; > + } > + > + if (!aDate.isObject()) { if (!aDate.isObject() || !JS_ObjectIsDate(aCtx, aDate)) { Maybe that would work too: if (!aDate.isObject() || !aDate.toObject.isDate()) { @@ +1200,5 @@ > + } > + > + JSObject& date = aDate.toObject(); > + jsval timestamp; > + JSBool ret = JS_CallFunctionName(aCtx, &date, "valueOf", 0, NULL, &timestamp); You could call "getTime" to be consistent with other calls. @@ +1207,5 @@ > + return NS_OK; > + } > + > + double value = timestamp.toNumber(); > + if (MOZ_DOUBLE_IS_NaN(value)) { You could change the previous condition to: if (!ret || !timestamp.isNumber() || MOZ_DOUBLE_IS_NAN(timestamp.toNumber()) so you don't need two blocks to handle failures. ::: content/html/content/src/nsHTMLInputElement.h @@ +573,5 @@ > + * or parse a number string to its value if type=number. > + * @param the string to be parsed > + * @return the timestamp as a double > + */ > + bool GetDoubleFromString(nsAString &aValue, double &aResultValue) const; No need for that method I think. You can keep everything inside "GetValueAsDouble" Given that you pass the value to "GetDoubleFromString" and you are using |mType| which makes the method not very generic. ::: content/html/content/test/forms/test_valueasdate_attribute.html @@ +3,5 @@ > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=769370 > +--> > +<head> > + <title>Test for Bug 636737769370</title> nit: "Test for input.valueAsDate" @@ +141,5 @@ > + } > + > + for each (data in invalidData) { > + element.value = data[0]; > + is(element.valueAsDate, data[1] ? "Invalid Date" : null, .valueAsDate == "Invalid Date"? What does that mean? The issue here is that the specs doesn't seem to say what should happen if creating a new Date objects fail with the parsing date. We should probably raise that issue and return 'null'. ::: content/html/content/test/forms/test_valueasnumber_attribute.html @@ +243,5 @@ > + [ "invaliddatenumber", "" ], > + [ NaN, "" ], > + [ undefined, "" ], > + // Out of range, the corresponding date string is the empty string > + [ -62167219200001, "" ], I don't see where the specs say that we should use the empty string in case of all the conversions fail. However, Webkit and Presto seem to have the same behavior so let's call it sane. Did you find anything saying that we should use the empty string? If not, we should open a bug against the specifications.

(In reply to Mounir Lamouri (:mounir) from comment #9) > @@ +1193,5 @@ > > + if (mType != NS_FORM_INPUT_DATE) { > > + return NS_ERROR_DOM_INVALID_STATE_ERR; > > + } > > + > > + if (!aDate.isObject()) { > > if (!aDate.isObject() || !JS_ObjectIsDate(aCtx, aDate)) { > > Maybe that would work too: > if (!aDate.isObject() || !aDate.toObject.isDate()) { JS_ObjectIsDate(aCtx, aDate) returns false when we call input.valueAsDate = new Date(). I opened bug 781154. In the meantime there is no way to check that aDate really is a Date. > ::: content/html/content/src/nsHTMLInputElement.h > @@ +573,5 @@ > > + * or parse a number string to its value if type=number. > > + * @param the string to be parsed > > + * @return the timestamp as a double > > + */ > > + bool GetDoubleFromString(nsAString &aValue, double &aResultValue) const; > > No need for that method I think. You can keep everything inside > "GetValueAsDouble" Given that you pass the value to "GetDoubleFromString" > and you are using |mType| which makes the method not very generic. Several features to implement for Date, and probably for other types as well, will need such a method to get the number value corresponding to the string value in a type dependent way as described in http://www.whatwg.org/specs/web-apps/current-work/#concept-input-value-string-number. I think it might be better to do that now. > @@ +141,5 @@ > > + } > > + > > + for each (data in invalidData) { > > + element.value = data[0]; > > + is(element.valueAsDate, data[1] ? "Invalid Date" : null, > > .valueAsDate == "Invalid Date"? What does that mean? > > The issue here is that the specs doesn't seem to say what should happen if > creating a new Date objects fail with the parsing date. We should probably > raise that issue and return 'null'. The specs are not very clear, but if we follow them to the letter the algorithm to convert a string to a date object http://www.whatwg.org/specs/web-apps/current-work/#concept-input-value-string-date (which is what we do here) is supposed to return a date object when the string value parsing succeed, which will be true in this case. Note that the date object creation didn't exactly fail, the resulting object just holds a NaN value. We could file a bug against this part of the spec, but IMO it's a good idea to differentiate the case where the input value cannot be parsed as a valid date string, and the case where it can, but the date object is unable to represent it (because it's out of range). > ::: content/html/content/test/forms/test_valueasnumber_attribute.html > @@ +243,5 @@ > > + [ "invaliddatenumber", "" ], > > + [ NaN, "" ], > > + [ undefined, "" ], > > + // Out of range, the corresponding date string is the empty string > > + [ -62167219200001, "" ], > > I don't see where the specs say that we should use the emptyWe c string in case > of all the conversions fail. However, Webkit and Presto seem to have the > same behavior so let's call it sane. > Did you find anything saying that we should use the empty string? If not, we > should open a bug against the specifications. Hum, the specs are not clear. The first part http://www.whatwg.org/specs/web-apps/current-work/#dom-input-valueasnumber says to create a Date object whose time value is the argument of SetValueAsNumber, in all those invalid values, this value will be converted to NaN (either because of the conversion of the value to a number, as defined by ecmascript spec, or because they are out-of the date object range). Then the algorithm to convert a Date object to a string, http://www.whatwg.org/specs/web-apps/current-work/#date-state-(type=date), does not specify explicitly how to convert a Date object holding a NaN time value to a valid date string. Maybe a bug should be opened to add that.

-Changed GetDoubleFromString to ConvertNumberToString according to https://bugzilla.mozilla.org/show_bug.cgi?id=769355#c2 -Removed a typo in > if (!ret || !timestamp.isNumber()i || MOZ_DOUBLE_IS_NAN(timestamp.toNumber())

And it's even better with an actually updated patch. Sorry.

Comment on attachment 650127 [details] [diff] [review] patch Review of attachment 650127 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments below fixed. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1042,5 @@ > + return false; > + } > + > + jsval timestamp; > + if (!JS::Call(ctx, date, "getTime", 0, NULL, &timestamp)) { s/NULL/nullptr/ @@ +1046,5 @@ > + if (!JS::Call(ctx, date, "getTime", 0, NULL, &timestamp)) { > + return false; > + } > + > + aResultValue = timestamp.toNumber(); Do we want to do: if (!timestamp.isNumber()) { return false; } before? @@ +1065,5 @@ > > GetValueInternal(stringValue); > + bool validValue = ConvertStringToNumber(stringValue, doubleValue); > + > + return !validValue ? MOZ_DOUBLE_NaN() : doubleValue; Could be: return ConvertStringToNumber(stringValue, doubleValue) ? doubleValue : MOZ_DOUBLE_NaN(); @@ +1159,5 @@ > + > + value.AppendPrintf("%04.0f-%02.0f-%02.0f", year.toNumber(), > + month.toNumber() + 1, day.toNumber()); > + } > + break; We could actually have a ConvertNumberToString() method. That's more like a not for later. Not that much useful here. Feel free to open a follow-up. @@ +1212,5 @@ > + } > + > + JSObject& date = aDate.toObject(); > + jsval timestamp; > + bool ret = JS::Call(aCtx, &date, "getTime", 0, NULL, &timestamp); s/NULL/nullptr/ @@ +1214,5 @@ > + JSObject& date = aDate.toObject(); > + jsval timestamp; > + bool ret = JS::Call(aCtx, &date, "getTime", 0, NULL, &timestamp); > + double value = timestamp.toNumber(); > + if (!ret || !timestamp.isNumber() || MOZ_DOUBLE_IS_NaN(value)) { Ouch, be careful here: you call |timestamp.toNumber()| without checking if |timestamp.isNumber()|. Instead, do: if (!ret || !timestamp.isNumber() || MOZ_DOUBLE_IS_NaN(timestamp.toNumber())) { and call |double value = timestamp.toNumber();| later. ::: content/html/content/src/nsHTMLInputElement.h @@ +570,5 @@ > /** > + * Convert a string to a number in a type specific way, > + * ie parse a date string to a timestamp if type=date > + * or parse a number string to its value if type=number. > + * @param the string to be parsed nit: could you point to the relevant whatwg spec part: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#concept-input-value-string-number @@ +571,5 @@ > + * Convert a string to a number in a type specific way, > + * ie parse a date string to a timestamp if type=date > + * or parse a number string to its value if type=number. > + * @param the string to be parsed > + * @return the timestamp as a double That's not really true: @param aValue the string to be parsed. @param aResultValue the result of the parsing [out] @return whether the parsing was successful

And open a follow-up about the Date object from .valueAsDate having a NaN time representation.

(In reply to Mounir Lamouri (:mounir) from comment #14) > @@ +1159,5 @@ > > + > > + value.AppendPrintf("%04.0f-%02.0f-%02.0f", year.toNumber(), > > + month.toNumber() + 1, day.toNumber()); > > + } > > + break; > > We could actually have a ConvertNumberToString() method. That's more like a > not for later. Not that much useful here. Feel free to open a follow-up. No need for a follow-up it's implemented in bug 769359's patch.

(In reply to Raphael Catolino (:raphc) from comment #17) > (In reply to Mounir Lamouri (:mounir) from comment #14) > > > @@ +1159,5 @@ > > > + > > > + value.AppendPrintf("%04.0f-%02.0f-%02.0f", year.toNumber(), > > > + month.toNumber() + 1, day.toNumber()); > > > + } > > > + break; > > > > We could actually have a ConvertNumberToString() method. That's more like a > > not for later. Not that much useful here. Feel free to open a follow-up. > No need for a follow-up it's implemented in bug 769359's patch. Cool :)

Updated patch.

See bug 825247 for some problems with the patch as landed. :(

See bug 866440 for documentation.