1260858 - Update the DateTimeFormat.prototype.formatToParts to the final spec

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

In bug 1216150 we introduced `formatToParts` method for DateTimeFormat. We need it for a lot of UI/UX code in FxOS/CD.

We since then moved forward with standardization, we've reached Stage 3 and we're on the path to Stage 4.

We have several implementations including Gecko, Intl.js and we have a good reason to hope to see it relatively soon in Chrome.

There are several cosmetic changes to the spec since we implemented it and before we expose it to the Web we'll need to make sure that the spec is final and apply the changes to our code.

This bug will track that.

So far we know of three changes:

1) formatToParts is not bound
2) `dayperiod` token name changed to `dayPeriod` for compatibility with `timeZoneName`
3) the `separator` label changed to `literal`.

I'd like to give it a month or two to see if any other things come up between Stage 3 and Stage 4, and then will write a patch to align our implementation with the spec and ask to expose the feature to the Web.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch

Ok, I think we're ready to finalize the work. No changes has been proposed for some time, and I'm going to propose moving the proposal to Stage 4 soon, so let's update the implementation.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8754571 [details] [diff] [review]
patch

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

Where's the latest spec language for all this?  It's not yet in tc39, presumably because it's not officially part of the standard quite yet.  And I'm not 100% certain which user repo I should be looking at.  With some assumptions made, tho, this looks well on the right track.

::: js/src/builtin/Intl.cpp
@@ +1764,2 @@
>      // If the still-experimental DateTimeFormat.prototype.formatToParts method
>      // is enabled, also add its getter.

Comment needs changing.

::: js/src/builtin/Intl.js
@@ -2747,5 @@
>  
>  
> -function Intl_DateTimeFormat_formatToParts_get() {
> -    // Check "this DateTimeFormat object" per introduction of section 12.3.
> -    var internals = getDateTimeFormatInternals(this, "formatToParts");

Eliminating the getter-caching/binding, also eliminates this check.  If I follow into Intl.cpp, js::intl_FormatDateTime right off asserts that args[0] is an object.  That's no longer true without this check, e.g.:

  Intl.DateTimeFormat.prototype.formatToParts.call(0);

I believe to fix this you'll need to add

  getDateTimeFormatInternals(this, "formatToParts");

to the top of the formatToParts function to do the necessary this-testing.  We'll also want tests of passing various primitive values, and non-DateTimeFormat objects, as |this|, as the above does.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Where's the latest spec language for all this?  

https://tc39.github.io/ecma402/#sec-intl.getcanonicallocales

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ugh, sorry, wrong bug. And my patch shouldn't have remove the experimental guard on the feature. Updating.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch v2

Updated version to your feedback.

The proposal has not yet been merged into master. But it got r+ and is just waiting for 3rd edition release.

The patch for spec is here: https://github.com/tc39/ecma402/pull/64

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8755698 [details] [diff] [review]
patch v2

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

::: js/src/builtin/Intl.js
@@ +2742,1 @@
>      // Steps 1.a.i-ii

Add a blank line before this "// Steps" comment -- separate distinct operations in the algorithm deserve separate blocks of code.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch, v3

carry-over r+

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b4391bfdd4
"Update the DateTimeFormat.prototype.formatToParts to the final spec". r=waldo

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5b4391bfdd4

Comment 10

[Florian Scholz (Open Web Docs)]

Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/formatToParts with the changes "separator" -> "literal" and "dayperiod" -> "dayPeriod".

Comment 11

[Sebastian Zartner [:sebo]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> I'd like to give it a month or two to see if any other things come up
> between Stage 3 and Stage 4, and then will write a patch to align our
> implementation with the spec and ask to expose the feature to the Web.

Looks like the missing pieces were implemented here, so I've filed bug 1289340 to expose this feature to the Web.

Sebastian