Closed Bug 1278185 Opened 6 years ago Closed 6 years ago

Implement valueAsNumber and valueAsDate for <input type=month>

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → jjong
I am a little bit confused about getting the attribute 'valueAsNumber'. Per spec [1], "if the valueAsDate attribute applies, run the algorithm to convert a string to a Date object defined for that state to the element's value; if the algorithm returned a Date object, then return the time value of the object (the number of milliseconds from midnight UTC the morning of 1970-01-01 to the time represented by the Date object)".

However, the tests in chromium [2] for valueAsNumber, is the result of running the algorithm to convert a string to a number (number of months between January 1970 and the parsed month).

So, I wonder if I'm reading the spec correctly.

[1] https://html.spec.whatwg.org/multipage/forms.html#dom-input-valueasnumber
[2] https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/fast/forms/month/input-valueasnumber-month.html
Flags: needinfo?(bugs)
yes, valueAsDate does apply to month per spec, so valueAsNumber should be milliseconds.
What do other browsers do here? Is this a spec bug?
Though, based on other tests in chromium, it does seem to still somehow apply valueAsDate to type=month when valueAsDate property is explicitly used.
Flags: needinfo?(bugs) → needinfo?(annevk)
Whiteboard: btpp-active
Jessica, I think those algorithms for <input type=month> end up giving identical results so it doesn't matter:

"The algorithm to convert a string to a number, given a string input, is as follows: If parsing a month from input results in an error, then return an error; otherwise, return the number of months between January 1970 and the parsed month."

"The algorithm to convert a string to a Date object, given a string input, is as follows: If parsing a month from input results in an error, then return an error; otherwise, return a new Date object representing midnight UTC on the morning of the first day of the parsed month."

Note that while valueAsNumber runs the second algorithm here, because valueAsDate applies, it doesn't return a Date object, but rather its underlying value. So I suspect for <input type=month> you can use either algorithm and the former is probably easier, but there might be other input types where the difference matters.

I agree that this is not really an ideal way of describing things. But rewriting that section would be a lot of effort.
Flags: needinfo?(annevk)
Chatted with anne, and we agree chrome is doing against the spec.
What do other browsers do?
If also Safari and Edge return number of months, we certainly need a spec change.
Load about:blank and then execute the following in browser console:
document.body.innerHTML = "<input type='month' value='1971-01'>"; if (document.body.firstChild.type != "month") { console.log("The test doesn't apply."); } else { document.body.firstChild.valueAsNumber == 12 ? console.log("against the spec") : console.log("follows the spec") }
I was told Edge does the same as Chrome, returns number of months.
Safari doesn't support input type=month for now, so I tested Opera, which returns number of months as well. So, should we follow what other browsers do to make devs' life easier or should we hold this until we have a conclusion?
Opera is identical to Chrome. Mobile Safari (which does support <input type=month>) returns the same as Chrome for https://dump.testsuite.org/html/input-month-valueasnumber.html. I suggest we do so as well and get the specification change. Too much of an uphill battle to get everyone else to change.
I agree. Need to get the spec changed and make it follow what browsers actually have implemented. And we should follow other browsers too.
Attached patch patch, v1. (obsolete) — Splinter Review
Attachment #8768668 - Flags: review?(bugs)
Comment on attachment 8768668 [details] [diff] [review]
patch, v1.

> HTMLInputElement::ValueAsDateEnabled(JSContext* cx, JSObject* obj)
> {
>   return Preferences::GetBool("dom.experimental_forms", false) ||
>-    Preferences::GetBool("dom.forms.datepicker", false);
>+    Preferences::GetBool("dom.forms.datepicker", false) ||
>+    Preferences::GetBool("dom.forms.datetime", false);
At some point it might make sense to add some helper method to get the pref value, and the value would be cached using BoolVarCache

+uint32_t
+HTMLInputElement::MonthsSinceJan1970(uint32_t aYear, uint32_t aMonth) const
+{
+  return (aYear - 1970) * 12 + aMonth - 1;
+}
Hm, how does that work in case aYear is < 1970?
The method is returning uint32_t. I'm pretty sure you want int32_t
Or does this all work just because you then explicitly cast to int32_t on the caller side?
I think it would be better to just make this return the right value as int32_t
That fixed, r+
Attachment #8768668 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 8768668 [details] [diff] [review]
> patch, v1.
> 
> > HTMLInputElement::ValueAsDateEnabled(JSContext* cx, JSObject* obj)
> > {
> >   return Preferences::GetBool("dom.experimental_forms", false) ||
> >-    Preferences::GetBool("dom.forms.datepicker", false);
> >+    Preferences::GetBool("dom.forms.datepicker", false) ||
> >+    Preferences::GetBool("dom.forms.datetime", false);
> At some point it might make sense to add some helper method to get the pref
> value, and the value would be cached using BoolVarCache

Sure, will keep this in mind on the coming bugs.

> 
> +uint32_t
> +HTMLInputElement::MonthsSinceJan1970(uint32_t aYear, uint32_t aMonth) const
> +{
> +  return (aYear - 1970) * 12 + aMonth - 1;
> +}
> Hm, how does that work in case aYear is < 1970?
> The method is returning uint32_t. I'm pretty sure you want int32_t
> Or does this all work just because you then explicitly cast to int32_t on
> the caller side?
> I think it would be better to just make this return the right value as
> int32_t
> That fixed, r+

You're right, it makes more sense to return it as int32_t. Thanks.
Attached patch patch, v2.Splinter Review
Let HTMLInputElement::MonthsSinceJan1970 return int32_t
Attachment #8768668 - Attachment is obsolete: true
Attachment #8769581 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c5fc5f13a7
Implement valueAsNumber and valueAsDate for <input type=month>. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69c5fc5f13a7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.