If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

For ISO 8601 syntax, Date.parse should accept " " in places where "T" is allowed.

RESOLVED INVALID

Status

()

Core
JavaScript: Standard Library
RESOLVED INVALID
5 years ago
4 years ago

People

(Reporter: kennyluck, Assigned: Colin Su)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=kennyluck][lang=c++], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
After some discussions and research around this[1] with HTML people, the spec now accepts this syntax "2011-12-06 13:28:00" along with "2011-12-06T13:28:00". For consistency, the JS engine should do it too.

The URL: field has the test written by Thomasy. Also from his testing, Opera and WebKit (which engine? :p) already support this while IE9 and Firefox are not.

The code is located near[2].


[1] http://wiki.whatwg.org/wiki/Time_element#permit_space_instead_of_T_in_datetimes
[2] http://hg.mozilla.org/mozilla-central/annotate/tip/js/src/jsdate.cpp#l812
(Reporter)

Updated

5 years ago
Summary: Date.parse should accept a " " in place of "T" in ISO 8601 syntax → Date.parse should accept " " in place of "T" in ISO 8601 syntax
(Reporter)

Updated

5 years ago
Summary: Date.parse should accept " " in place of "T" in ISO 8601 syntax → For ISO 8601 syntax, Date.parse should accept " " in places where "T" is allowed.
(Assignee)

Comment 1

5 years ago
this bug has been resolved and passed the following test case manually:

d = new Date("2011-10-10 14:48:00");alert(d); -> correct 
d = new Date("2011-10-10T14:48:00");alert(d); -> correct
d = new Date("2011-10-10A14:48:00");alert(d); -> invalid
d = new Date("2011-10-10:14:48:00");alert(d); -> invalid
d = new Date("2011-12-11-14:14:00");alert(d); -> invalid

did I miss something?

and now i'm starting to write tests, but I can not locate where is it.
(Assignee)

Comment 2

5 years ago
is it in "mozilla-central/testing/mochitest/tests/MochiKit-1.4.2/tests/test_DateTime.js"?
(Reporter)

Comment 3

5 years ago
(In reply to Colin Su from comment #1)
> this bug has been resolved and passed the following test case manually:

Resolved in your local clone with your patch, I suppose? (This sentence might be a bit confusing :p)

> d = new Date("2011-10-10 14:48:00");alert(d); -> correct 
> d = new Date("2011-10-10T14:48:00");alert(d); -> correct
> d = new Date("2011-10-10A14:48:00");alert(d); -> invalid
> d = new Date("2011-10-10:14:48:00");alert(d); -> invalid
> d = new Date("2011-12-11-14:14:00");alert(d); -> invalid
> 
> did I miss something?

The current code accepts things like "T14:48:00" so you probably should write a test about " 14:48:00". I am not exactly sure what should happen though. Check other browsers and think about it yourself!

(In reply to Colin Su from comment #2)
> is it in
> "mozilla-central/testing/mochitest/tests/MochiKit-1.4.2/tests/test_DateTime.
> js"?

Yeah, that's probably one (not so sure). It might be helpful if you, for example:

1. Click bhoult@32576 in the /annotate/ URL I gave you.
2. Click the link besides changeset 32576 and see what files were changed in the last patch(es) around this.

I am not sure what kind of tests the JS engine use (I've never contribute to this module at all!), but [1] might be useful, maybe.

[1] https://developer.mozilla.org/en-US/docs/Mozilla_automated_testing
(Assignee)

Comment 4

5 years ago
Created attachment 661484 [details] [diff] [review]
patch for fixing this bug

needs testcases and review.
(Assignee)

Comment 5

5 years ago
Created attachment 661504 [details] [diff] [review]
bug-791320-fix.patch
Attachment #661484 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 661555 [details] [diff] [review]
bug-791320-fix.patch with testcases

solved, and included the testcases.
Attachment #661504 - Attachment is obsolete: true
Attachment #661555 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 7

5 years ago
Comment on attachment 661555 [details] [diff] [review]
bug-791320-fix.patch with testcases

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

This is an informal review from a random guy (which means that you don't have to listen to me :)

::: js/src/jsdate.cpp
@@ +872,5 @@
> +    if (!PEEK(' ')) {
> +        DONE_UNLESS('T');
> +    } else {
> +        DONE_UNLESS(' ');
> +    }

You should probably add a comment like the one below

  /*
   * Non-standard extension to the ISO date format (permitted by ES5):
   * allow "-0700" as a time zone offset, not just "-07:00".
   */

to explain the status. This is, after all, non-standard, but I feel like this is the right thing to do (explained in Comment 0)

::: js/src/tests/ecma_5/extensions/15.9.4.2.js
@@ +55,3 @@
>    // Allow non-standard "-0700" as timezone, not just "-07:00"
>    check("2009-07-23T00:53:21.001-0700", dd(2009,7,23,7,53,21,1));
>  

You should print the right bug number here.
Before we take this, you should discuss this with the ecma people on https://mail.mozilla.org/listinfo/es-discuss.

Comment 9

5 years ago
(In reply to Tom Schuster [:evilpie] from comment #8)
> Before we take this, you should discuss this with the ecma people on
> https://mail.mozilla.org/listinfo/es-discuss.

I would like to see a better motivation present for why this change is important.  In particular, it is a deviation from the ECMAScript standard.  While syntactic extensions to the Date.parse syntax is allowed by the specification.  It is not particularly a good idea.  In particular, every time a specific implementation adds such a non-standard extension, it creates a potential cross-browser interoperability hazard. 

Apparently these extensions are not motivated by a desire to improve interoperability as  reference [1] shows that none of Firefox, IE, and Safari currently provide these extensions. Chrome apparently does (and also Opera??). 

Finally, I don't understand why a WhatWG document is being used to justify a change to an ECMAScript specified function or why the WhatWG is messing with an ECMA specification.  If there is a serious proposal to change the ECMAScript specification it should be brought to TC39 as a proposal where it would be given serious consideration.  But simply bypassing the standards process doesn't seem like a very good idea.  In particular, Google is a member of TC39 and Chrome has apparently implemented these extensions.  Yet as far as I know, they have not made a proposal for this extension to TC39. Why not?
(Reporter)

Comment 10

5 years ago
(In reply to Allen Wirfs-Brock from comment #9)
> Apparently these extensions are not motivated by a desire to improve
> interoperability as  reference [1] shows that none of Firefox, IE, and
> Safari currently provide these extensions. Chrome apparently does (and also
> Opera??). 

Yes, Opera (Opera Next - Opera 12.50 internal) too, and I'd predict that ECMAScript would converge to accepting "2011-12-06 13:28:00" (because it's better) and so I disagree that this particular extension is not motivated by a desire to improve interoperability.
 
> Finally, I don't understand why a WhatWG document is being used to justify a
> change to an ECMAScript specified function or why the WhatWG is messing with
> an ECMA specification.

I don't understand how WhatWG is relevant here. Reference [1] is just a wiki page edited by multiple people, and some of us might not consider ourselves affiliated with the WhatWG. I mentioned that page just because it contains rationale and the status quo of this extension. If you think the HTML spec should not be "messing with" ECMAScript then I have to disagree with you because I don't think having two datetime format for HTML and ECMAScript is a good idea.

(Note: that this extension is already part[3] of the HTML spec.)

[3] http://dev.w3.org/html5/spec/common-microsyntaxes.html#local-dates-and-times

> If there is a serious proposal to change the ECMAScript specification it should
> be brought to TC39 as a proposal where it would be given serious
> consideration.  But simply bypassing the standards process doesn't seem like a 
> very good idea.  In particular, Google is a member of TC39 and Chrome has
> apparently implemented these extensions.  Yet as far as I know, they have not
> made a proposal for this extension to TC39. Why not?

You'd better ask that question on es-discuss, but I can imagine the answer being "the spec already allows this". We all know that we need a full section to completely solve the interoperability hazard about date parsing in ECMA. The question is who has the bandwidth and interest to do so.
(In reply to Allen Wirfs-Brock from comment #9)
> Finally, I don't understand why a WhatWG document is being used to justify a
> change to an ECMAScript specified function or why the WhatWG is messing with
> an ECMA specification.

I don't think WHATWG is "messing with" ECMAScript. The change they made is a change to the HTML standard. It affects the <time datetime=> attribute, for example.

TC39 has no obligation to follow suit. But maybe it should. We should talk about it on es-discuss.

Please have some consideration for new contributors. I know you didn't intend it this way, but your comment came across as a little accusatory, or maybe territorial, and I think that gives the wrong impression of how we work.

It's never going to be a great experience to contribute a patch for a "[good first bug]" and immediately run into standard committee stop energy, but maybe we can soften the blow a little. :)

Comment 12

5 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> 
> I don't think WHATWG is "messing with" ECMAScript. The change they made is a
> change to the HTML standard. It affects the <time datetime=> attribute, for
> example.
> 
> TC39 has no obligation to follow suit. But maybe it should. We should talk
> about it on es-discuss.

My reading of the comments was that this web page was being cited as an updated specification, and that this update motivates implementing a change to Date.parse.  

That's putting the cart before the horse. If we think that the current Ecma standard for Date.parse should be extend, we should bring that as a proposal directly to a TC39 meeting rather than just implementing it and then bring it as a de factor standard.

Now is actually a good time to do that.  es-discuss is a good place to test the waters, but not the place that decisions are actually made. 

> 
> Please have some consideration for new contributors. I know you didn't
> intend it this way, but your comment came across as a little accusatory, or
> maybe territorial, and I think that gives the wrong impression of how we
> work.

Yes, sorry about that.  This isn't a contribution issue. It's a feature selection issue. I also can see how the WhatWG page makes it appear that this is an "approved" revision to the standard and a candidate feature to  worked on.

My territorial reaction, which I think is legitimate, is about that WhatWG page, rather than the actual contribution. The patch is probably good work, but whether we should make that extension is a very different question that should be consider in the context of established processes for the relevant standard.
Allen's right that WhatWG (which drove HTML5 and still drives HTML) was a bit pushy here. But that's not Kenny's fault. Cc'ing AnneVK -- I blame him :-P!

The other protocol in play here, which should not be a "stop energy" event, rather a "redirect your energy to another channel which is upstream in decision-making authority governing what's normative" event.

Yes, the spec allows extensions, but if HTML's datetime attribute accepts space-not-T we will end up with a de-facto requirement that probably becomes normative over time. Or else datetime uptake will remain slow due to jQuery, which currently soaks up date picking use-cases on the interoperable web, even back to old versions of IE!

/be
Comment on attachment 661555 [details] [diff] [review]
bug-791320-fix.patch with testcases

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

This patch is fine except for the question of whether we should actually make the change.  :)  Good tests.

I'm going to clear the review flag for now, while this is settled in TC39.  Colin, you can expect this bug to be dormant for some time, until the standard committee decides what to do.  Thanks for the patch.

::: js/src/jsdate.cpp
@@ +872,5 @@
> +    if (!PEEK(' ')) {
> +        DONE_UNLESS('T');
> +    } else {
> +        DONE_UNLESS(' ');
> +    }

The macros don't help much here, so I would write:

    if (PEEK('T') || PEEK(' '))
        i++;
    else
        goto done;

But that is a minor detail.
Attachment #661555 - Flags: review?(jwalden+bmo)

Comment 15

5 years ago
Comment 11 seems spot on. A change was made to HTML, not ECMAScript. (I do think it would be useful if ECMAScript could parse HTML dates, but I have nothing to do with any of this. :-))
(Assignee)

Comment 16

5 years ago
Thanks for advise, actually this is my original way to fix, but I just wanna try to reuse DONE_UNLESS module, XD

I will pay more attention on this next time, thank you :D

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> Comment on attachment 661555 [details] [diff] [review]
> bug-791320-fix.patch with testcases
> 
> Review of attachment 661555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is fine except for the question of whether we should actually
> make the change.  :)  Good tests.
> 
> I'm going to clear the review flag for now, while this is settled in TC39. 
> Colin, you can expect this bug to be dormant for some time, until the
> standard committee decides what to do.  Thanks for the patch.
> 
> ::: js/src/jsdate.cpp
> @@ +872,5 @@
> > +    if (!PEEK(' ')) {
> > +        DONE_UNLESS('T');
> > +    } else {
> > +        DONE_UNLESS(' ');
> > +    }
> 
> The macros don't help much here, so I would write:
> 
>     if (PEEK('T') || PEEK(' '))
>         i++;
>     else
>         goto done;
> 
> But that is a minor detail.

Updated

5 years ago
Duplicate of this bug: 845195
Hi jorendorff, is this still dormant? Do you know what's happening on the bug? I'm going through all the mentored bugs that have been inactive for a while, and trying to clear them up or find out their status. Thanks!
Flags: needinfo?(jorendorff)
Liz, thanks for doing this.

I wrote to es-discuss about this. We'll see what happens.
Flags: needinfo?(jorendorff)
Jason: what was the conclusion on es-discuss about Date.parse() accepting " " in place of "T"? I see that your current ES6 draft still requires "T":

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-date-time-string-format
Component: JavaScript Engine → JavaScript: Standard Library
Flags: needinfo?(jorendorff)
The thread is here: https://mail.mozilla.org/pipermail/es-discuss/2013-April/030207.html

Nothing changed.

I found that conversation disappointing.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jorendorff)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.