The default bug view has changed. See this FAQ.

implement valueAsNumber and valueAsDate for input <input type=time>

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: raphc, Assigned: mounir)

Tracking

({doc-bug-filed})

Trunk
mozilla21
doc-bug-filed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox -)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 781572
(Reporter)

Comment 1

5 years ago
Created attachment 650609 [details] [diff] [review]
patch
Attachment #650609 - Flags: feedback?(mounir)
(Reporter)

Updated

5 years ago
Attachment #650609 - Flags: feedback?(mounir) → review?(mounir)
(Assignee)

Comment 2

5 years ago
Comment on attachment 650609 [details] [diff] [review]
patch

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

Looks generally good but not r+'ing because I want to figure out some details.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1058,5 @@
> +        if (!GetValueAsTime(aValue, hour, min, sec, msec)) {
> +          return false;
> +        }
> +
> +        aResultValue = msec + sec * 1000 + min * 60000 + hour * 3600000;

I believe this is more readable:
aResultValue = msec + 1000 * (sec + 60 * (min + 60 * hour));

@@ +1185,5 @@
> +        if (MOZ_DOUBLE_IS_NaN(aValue)) {
> +          break;
> +        }
> +
> +        PRInt64 timestamp = NS_floorModulo((PRInt64)aValue, 86400000);

Do we want to do the double -> PRInt64 conversion with 'round' or 'floor'? Maybe that should be more explicit.

@@ +1207,5 @@
> +        } else if (optionalField && msec % 10 == 0) {
> +          aResultString.AppendPrintf(":%05.2f", optionalField);
> +        } else if (optionalField) {
> +          aResultString.AppendPrintf(":%06.3f", optionalField);
> +        }

That entire block could be simpler and only check if msec > 99, > 9 and > 0.

@@ +1255,5 @@
> +          return NS_OK;
> +        }
> +
> +        jsval time;
> +        time.setInt32(msec + sec * 1000 + min * 60000 + hour * 3600000);

msec + 1000 * (sec + 60 * (min + 60 * hour) ?

@@ +4325,5 @@
>    switch (mType)
>    {
>      case NS_FORM_INPUT_NUMBER:
>      case NS_FORM_INPUT_DATE:
> +    case NS_FORM_INPUT_TIME:

Why is that added in that patch?

::: content/html/content/test/forms/test_valueasnumber_attribute.html
@@ +275,5 @@
> +    [ "02:12:25.006", 7945006 ],
> +    [ "02:12:25.000", 7945000 ],
> +    [ "02:12:00.000", 7920000 ],
> +    [ "00:00:00.000", 0 ],
> +    [ "00:00:00.0", 0 ],

[ "00:00:00.001", 1 ],
[ "00:00:00.01", 10 ],
[ "00:00:00.1", 100 ],

@@ +329,5 @@
> +    "84:05",
> +    "14:60",
> +    "24:60",
> +    "23:59:60",
> +    "23:59:59:1000",

"00:00:00.0000"

@@ +360,5 @@
> +    [ -27847212,          "16:15:52.788" ],
> +    [ 86399999,          "23:59:59.999" ],
> +    // Only the time part of the date is taken into account
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],

I wonder if those two values should return "00:00" or "". The specs are not clear IMO.

@@ +362,5 @@
> +    // Only the time part of the date is taken into account
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],
> +    [ -62167219200000,   "00:00" ],
> +    [ 1330473600000,     "00:00" ],

Same here.

@@ +363,5 @@
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],
> +    [ -62167219200000,   "00:00" ],
> +    [ 1330473600000,     "00:00" ],
> +    [ 7243748535100,     "16:22:15.1" ],

And here.

@@ +368,5 @@
> +    // "Values must be truncated to valid times"
> +    [ 42.1234,           "00:00:00.042" ],
> +    [ 12345.1234567891,  "00:00:12.345" ],
> +    [ 1e-1,              "00:00" ],
> +    [ 1298851745010,     "00:09:05.01" ],

Here maybe too?
Attachment #650609 - Flags: review?(mounir)
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 3

4 years ago
Created attachment 703196 [details] [diff] [review]
.valueAsNumber
Assignee: raphael.catolino → mounir
Status: NEW → ASSIGNED
Attachment #703196 - Flags: review?(bugs)
(Assignee)

Comment 4

4 years ago
Created attachment 703197 [details] [diff] [review]
.valueAsDate
Attachment #650609 - Attachment is obsolete: true
Attachment #703197 - Flags: review?(bugs)
Comment on attachment 703196 [details] [diff] [review]
.valueAsNumber


>+    case NS_FORM_INPUT_TIME:
>+      {
>+        uint32_t value = NS_floorModulo(floor(aValue), 86400000);
This could use some comment that what 86400000 is.

>+    { value: -1, result: "23:59:59.999" },
>+    { value: -86400000, result: "00:00" },
>+    { value: -86400001, result: "23:59:59.999" },
>+    { value: -56789, result: "23:59:03.211" },
Hmm, really? Could you tell where the spec say this is ok?
(I think the behavior is ok, I just couldn't find it in the spec.)

r=me assuming the spec really allows negative values.

Hmm, or do allow negative values because 'new Date(negative_value)' is ok... I guess so.
Attachment #703196 - Flags: review?(bugs) → review+
Comment on attachment 703197 [details] [diff] [review]
.valueAsDate


>+      jsval rval;
>+      jsval fullYear[3];
>+      fullYear[0].setInt32(year);
>+      fullYear[1].setInt32(month-1);
You're just moving this code, but there should be a space before and after -
Attachment #703197 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Olli Pettay [:smaug] from comment #5)
> >+    { value: -1, result: "23:59:59.999" },
> >+    { value: -86400000, result: "00:00" },
> >+    { value: -86400001, result: "23:59:59.999" },
> >+    { value: -56789, result: "23:59:03.211" },
> Hmm, really? Could you tell where the spec say this is ok?
> (I think the behavior is ok, I just couldn't find it in the spec.)
> 
> r=me assuming the spec really allows negative values.
> 
> Hmm, or do allow negative values because 'new Date(negative_value)' is ok...
> I guess so.

I explicitly asked Hixie about that on IRC because I believe the spec is hard to read. He confirmed that negative values are allowed and indeed, this is because we use Date(). Though, honestly, I think the specs are a pain to read and I will send an email to have them specified more clearly. I think specifying the algorithm I used would be way more understandable for human beings.
(Assignee)

Comment 8

4 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bc74ed9256
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f8673cfafa
Flags: in-testsuite+
Target Milestone: --- → mozilla21
Backed out for Windows and b2g build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2f0c12218b
(Assignee)

Comment 10

4 years ago
Damn compilers that can't just do the right thing... Hopefully, this will work better.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ffc20b4545
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c62ff59f9c54
https://hg.mozilla.org/mozilla-central/rev/f7ffc20b4545
https://hg.mozilla.org/mozilla-central/rev/c62ff59f9c54
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
relnote-firefox: --- → ?
(Assignee)

Comment 12

4 years ago
Jean-Yves, shouldn't we put the entire <input type='time'> supported on Mobile in the relnotes?
We'll track bug 777279 for relnotes.

Updated

4 years ago
relnote-firefox: ? → -
See bug 866430 for documentation.
Keywords: dev-doc-needed → doc-bug-filed
You need to log in before you can comment on or make changes to this bug.