Add support for hourCycle to Intl.DateTimeFormat

RESOLVED FIXED in Firefox 58

Status

()

P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

({dev-doc-complete})

unspecified
mozilla58
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In the effort to complete the overlap between unicode extnesion keys and Intl API options, we recently switched from `hour12` to `hourCycle` - https://github.com/tc39/ecma402/commit/bb18b4878f16f584dd56f809a1ae91f754ce4e21

We should update SpiderMonkey API to reflect that.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

The patch seems very simple, but testing may be harder.

In particular, it seems that at the moment there's only one language in CLDR that has K in the "allowed" and that's `ja` - http://www.unicode.org/repos/cldr/trunk/common/supplemental/supplementalData.xml

It also seems that ICU always returns one or the other.

For example, for "en" and skeleton "K", ICU returns "h a". For "en" and skeleton "h" it returns the same "h a".

For "ja" and skeleton "K" it returns "aK\u6642", and for "ja" and skeleton "h" it also returns the same "aK\u6642".

In result I don't see a way to test between h11/h12 and h23/h24 since at the moment it seems that it cannot be enforced.
Whichever one you pick, the ICU will select the closest equivalent consistently irrelevant if you went for 0-based or 1-based.

Andre, can you take a look at the patch? Also, if you have ideas on what would you like to see in tests for that, I'll do my best to write them :)

Thanks!
Attachment #8892319 - Flags: review?(andrebargull)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review168632

::: js/src/builtin/Intl.js:2440
(Diff revision 1)
>      //
>      //     localeOpt: // *first* opt computed in InitializeDateTimeFormat
>      //       {
>      //         localeMatcher: "lookup" / "best fit",
>      //
> -    //         hour12: true / false,  // optional
> +    //         hour12: true / false  // optional

Keep the comma and also add a comma after the new hourCycle entry, so it's consistent with the other entries (like requestedLocales, localeMatcher, timeZone, etc.).

::: js/src/builtin/Intl.js:2614
(Diff revision 1)
>      //       {
>      //         // all the properties/values listed in Table 3
>      //         // (weekday, era, year, month, day, &c.)
>      //
>      //         hour12: true / false  // optional
> +    //         hourCycle: "h11", "h12", "h23", "h24" // optional

And add commas here, too.

::: js/src/builtin/Intl.js:2869
(Diff revision 1)
>          break;
>      case "numeric":
>          skeleton += "d";
>          break;
>      }
> -    var hourSkeletonChar = "j";
> +    var hourSkeletonChar;

This doesn't seem to follow the spec, does it? According to the current spec, if both hour12 and hourCycle are present, the hourCycle option is ignored. So I'd expect:
```
var hourSkeletonChar = "j";
if (options.hour12 !== undefined) {
    // If hour12 and hourCycle are both present, hour12 takes precedence.
    if (options.hour12)
        hourSkeletonChar = "h";
    else
        hourSkeletonChar = "H";
} else {
    switch (options.hourCycle) {
    case "h11":
        hourSkeletonChar = "K";
        break;
    case "h12":
        hourSkeletonChar = "h";
        break;
    case "h23":
        hourSkeletonChar = "H";
        break;
    case "h24":
        hourSkeletonChar = "k";
        break;
    }
}
```

::: js/src/builtin/Intl.js:3237
(Diff revision 1)
>              default:
>                  // skip other pattern characters and literal text
>              }
>              if (hasOwn(c, icuPatternCharToComponent))
>                  _DefineDataProperty(result, icuPatternCharToComponent[c], value);
> -            if (c === "h" || c === "K")
> +            switch (c) {

This is also doesn't seem to follow the current spec: https://tc39.github.io/ecma402/#sec-initializedatetimeformat, step 27, the hourCycle option, if present, is directly saved in dateTimeFormat.[[HourCycle]]. That means for https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions, the resolved options object should return the input hourCycle, even if that hourCycle option isn't the one which is used in the actual pattern. But hold on, isn't that a bug in the spec? I'd expect resolvedOptions() to return the actual options used in the resolved pattern, and not the ones from the requested options object.
Attachment #8892319 - Flags: review?(andrebargull) → review-
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Andre, can you take a look at the patch? Also, if you have ideas on what
> would you like to see in tests for that, I'll do my best to write them :)

We should definitely test that resolvedOptions() returns the correct hourCycle and hour12 values. But first thing should probably be fixing the spec to return the used hourCycle from the resolved pattern. :-)

And in https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions
> For web compatibility reasons, if the property hourCycle is set, the corresponding property hour12 should be set as well. 

The spec should also make it more clear what the "corresponding property" actually is. That means hour12 is set to |true| if hourCycle is either "h11" or "h12", and set to |false| if hourCycle is either "h23" or "h24".
Keywords: dev-doc-needed
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> For example, for "en" and skeleton "K", ICU returns "h a". For "en" and
> skeleton "h" it returns the same "h a".
> 
> For "ja" and skeleton "K" it returns "aK\u6642", and for "ja" and skeleton
> "h" it also returns the same "aK\u6642".
> 

Hmm, bummer. I just checked every file in the cldr/main directory and searched for "k"/"K" in the /ldml/dates/calendars/calendar/dateTimeFormats/availableFormats/dateFormatItem entries, but just as you said, only "ja.xml" uses "K". (I was just checking in case there are contributed entries which aren't reflected in supplementalData.xml.)


> In result I don't see a way to test between h11/h12 and h23/h24 since at the
> moment it seems that it cannot be enforced.
> Whichever one you pick, the ICU will select the closest equivalent
> consistently irrelevant if you went for 0-based or 1-based.
> 

Hmm, I wonder if it's possible to enforce the hour cycle using udatpg_getBestPatternWithOptions (http://icu-project.org/apiref/icu4c/udatpg_8h.html#a5327a64b164f975dc0636e36b4d0f02c).
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review168632

> This is also doesn't seem to follow the current spec: https://tc39.github.io/ecma402/#sec-initializedatetimeformat, step 27, the hourCycle option, if present, is directly saved in dateTimeFormat.[[HourCycle]]. That means for https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions, the resolved options object should return the input hourCycle, even if that hourCycle option isn't the one which is used in the actual pattern. But hold on, isn't that a bug in the spec? I'd expect resolvedOptions() to return the actual options used in the resolved pattern, and not the ones from the requested options object.

> That means for https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions, the resolved options object should return the input hourCycle, even if that hourCycle option isn't the one which is used in the actual pattern.

In Step 27.f we assign the resolved `hc` to dateTimeFormat.[[HourCycle]] and that's what is used in resolvedOptions.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

I updated the patch to your feedback and also added handling of unicode extension key.

As far as I know there's no precedence for the dual option/extkey in Intl API yet, but we do plan to complete it as much as possible in the future: https://github.com/tc39/ecma402/issues/105

I'd like to write the tests as PR to test262 repo to satisfy the spec editor requirements. If I understand, we use those tests now in SM.
Is this enough or do you need me to also write tests for SM separately?
Attachment #8892319 - Flags: review?(andrebargull)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> I'd like to write the tests as PR to test262 repo to satisfy the spec editor
> requirements. If I understand, we use those tests now in SM.
> Is this enough or do you need me to also write tests for SM separately?

It depends on the tests, for example all general tests should definitely go directly into test262, but tests which are locale-specific, like Japanese uses "aK\u6642", probably belong into SM, because in test262 we can't require that 1. Japanese is actually supported by a specific Intl.DateTimeFormat implementation and 2. If supported, it uses the patterns from CLDR.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> Comment on attachment 8892319 [details]
> Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.
> 
> https://reviewboard.mozilla.org/r/163282/#review168632
> 
> > This is also doesn't seem to follow the current spec: https://tc39.github.io/ecma402/#sec-initializedatetimeformat, step 27, the hourCycle option, if present, is directly saved in dateTimeFormat.[[HourCycle]]. That means for https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions, the resolved options object should return the input hourCycle, even if that hourCycle option isn't the one which is used in the actual pattern. But hold on, isn't that a bug in the spec? I'd expect resolvedOptions() to return the actual options used in the resolved pattern, and not the ones from the requested options object.
> 
> > That means for https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.resolvedoptions, the resolved options object should return the input hourCycle, even if that hourCycle option isn't the one which is used in the actual pattern.
> 
> In Step 27.f we assign the resolved `hc` to dateTimeFormat.[[HourCycle]] and
> that's what is used in resolvedOptions.

Just to be clear: I agree with your implementation, but I don't agree with the current spec. :-)

I'll file spec-issues shortly.
(In reply to André Bargull from comment #11)
> Just to be clear: I agree with your implementation, but I don't agree with
> the current spec. :-)

Hmm, I have to take that back, now that I've spent more time digging through the spec. Sorry about jumping the gun!

So, if I understand the spec proposal correctly, the hourCycle property overrides the default hour representation from the resolved pattern (https://tc39.github.io/ecma402/#sec-partitiondatetimepattern, steps 18.e.v-vi). So what we need to do is to adjust the hour type in the resolved pattern, because "h" and "K" (and "H"/"k"), when being specified in a skeleton, don't affect the actual hour representation in the pattern. From http://unicode.org/reports/tr35/tr35-dates.html#Date_Field_Symbol_Table, for the "h" symbol:
> Hour [1-12]. When used in skeleton data or in a skeleton passed in an API for flexible date pattern generation, it should match the 12-hour-cycle format preferred by the locale (h or K); it should not match a 24-hour-cycle format (H or k).

IOW it doesn't matter if the skeleton uses "h" or "K", the actual hour representation in the pattern depends on the preferred format of the locale. 

That means for us, if an explicit hourCycle was specified, we need to update pattern returned from intl_patternForSkeleton [1] and replace the hour symbol with the correct hour symbol based on the hourCycle. Does that sound correct to you?


[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/js/src/builtin/Intl.js#2904

Comment 14

2 years ago
mozreview-review
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review169248

I think it makes sense to wait for https://github.com/tc39/ecma402/pull/155. Are you okay with that?

::: js/src/builtin/Intl.js:2716
(Diff revision 3)
>      void formatMatcher;
>  
>      // Steps 23-25 provided by ICU, more or less - see comment after this function.
>  
>      // Step 26.
> +    var hc = GetOption(options, "hourCycle", "string", ["h11", "h12", "h23", "h24"], undefined);

This'll need to be changed when https://github.com/tc39/ecma402/pull/155 is merged.

::: js/src/builtin/Intl.js:2878
(Diff revision 3)
>          skeleton += "d";
>          break;
>      }
>      var hourSkeletonChar = "j";
>      if (options.hour12 !== undefined) {
> +      // if hour12 and hourCycle are both present, hour12 takes precedence.

Nit: Titlecase "If" because the comment is a sentence.

::: js/src/builtin/Intl.js:2884
(Diff revision 3)
> -        if (options.hour12)
> +      if (options.hour12)
> -            hourSkeletonChar = "h";
> +          hourSkeletonChar = "h";
> -        else
> +      else
> -            hourSkeletonChar = "H";
> +          hourSkeletonChar = "H";
> +    } else {
> +      switch (options.hourCycle) {

Nit: Indent block by four spaces.

::: js/src/builtin/Intl.js:3043
(Diff revision 3)
>  function dateTimeFormatLocaleData() {
>      return {
>          ca: intl_availableCalendars,
>          nu: getNumberingSystems,
> +        hc: function() {
> +            return ["h11", "h12", "h23", "h24"];

`null` needs to be the first entry here, too.

::: js/src/builtin/Intl.js:3048
(Diff revision 3)
> +            return ["h11", "h12", "h23", "h24"];
> +        },
>          default: {
>              ca: intl_defaultCalendar,
>              nu: intl_numberingSystem,
> +          hc: function() {

Nit: Indentation off.

::: js/src/builtin/Intl.js:3052
(Diff revision 3)
>              nu: intl_numberingSystem,
> +          hc: function() {
> +              // If the value is not set, we're ok returning null, since this
> +              // will result in the skeleton using "j" key and picking up
> +              // the default value for the locale.
> +              return null;

Just for clarification because this "default" entry isn't in the ECMA402 spec:

The "default" entry is used to improve the performance when constructing Intl objects (bug 1365650). So localeData.default.<extensionKey>() should be the same value as localeData.<extensionKey>()[0].
Attachment #8892319 - Flags: review?(andrebargull) → review-
status-firefox57: --- → affected
Priority: -- → P5
Comment hidden (mozreview-request)
I rebased the patch on top of the current m-c and added a test.

I tried to reason about what changes do I need to apply in result of  https://github.com/tc39/ecma402/pull/155 but failed to identify that.

Anba, is there anything else left to do or is that in line with your changes to the spec now?

Comment 17

a year ago
mozreview-review
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review193250

::: js/src/builtin/Intl.js:2511
(Diff revision 4)
>  
>      // Step 18.
>      var formatOpt = lazyDateTimeFormatData.formatOpt;
>  
> +    if (formatOpt.hourCycle === undefined && internalProps.hourCycle) {
> +      // If the user didn't set hourCycle option, but the unicode extension

Nit:
s/set hourCycle option/set the hourCycle option/
s/unicode/Unicode/

::: js/src/builtin/Intl.js:2723
(Diff revision 4)
>      void formatMatcher;
>  
>      // Steps 23-25 provided by ICU, more or less - see comment after this function.
>  
>      // Step 26.
> +    var hc = GetOption(options, "hourCycle", "string", ["h11", "h12", "h23", "h24"], undefined);

GetOption() for "hourCycle" needs to happen before the call to ResolveLocale (change from https://github.com/tc39/ecma402/pull/155).

::: js/src/builtin/Intl.js:2891
(Diff revision 4)
>          if (options.hour12)
>              hourSkeletonChar = "h";
>          else
>              hourSkeletonChar = "H";
> +    } else {
> +        switch (options.hourCycle) {

It doesn't really matter if a skeleton uses "K" or "h" (resp. "k" or "H"), cf. http://unicode.org/reports/tr35/tr35-dates.html#Date_Field_Symbol_Table 

So as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1386146#c12, we need to adjust the returned pattern to select the desired hour representation.

(Assuming ICU4C works the same way as ICU4J when resolving patterns from skeletons.)

::: js/src/builtin/Intl.js:3058
(Diff revision 4)
>              ca: intl_defaultCalendar,
>              nu: intl_numberingSystem,
> +            hc: function() {
> +                // If the value is not set, we're ok returning null, since this
> +                // will result in the skeleton using "j" key and picking up
> +                // the default value for the locale.

This comment is confusing, please remove it.

Reasoning:
The `localedata.default.<extensionkey>` function is required to return the same value as `localedata.<extensionkey>()[0]`, cf. ResolveLocale step 8.e for the default value https://tc39.github.io/ecma402/#sec-resolvelocale. And per 12.3.3 (https://tc39.github.io/ecma402/#sec-intl.datetimeformat-internal-slots), the first entry for the `hc` extension-key is simply `null`.

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:3
(Diff revision 4)
> +// |reftest| skip-if(!this.hasOwnProperty("Intl"))
> +
> +const hourCycleToH12Map = {

Probably wouldn't hurt do have a test which uses invalid hourCycle values (for the extension key and the option value) to make sure it's correctly handled.

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:11
(Diff revision 4)
> +  "h23": false,
> +  "h24": false,
> +};
> +
> +for (const key of Object.keys(hourCycleToH12Map)) {
> +  const langTag = "en-US"; 

Nit: Trailing white-space.

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:14
(Diff revision 4)
> +
> +for (const key of Object.keys(hourCycleToH12Map)) {
> +  const langTag = "en-US"; 
> +  const loc = `${langTag}-u-hc-${key}`;
> +
> +  const dtf = new Intl.DateTimeFormat(loc, {hour: "numeric"});

We should also test `hourCycle` is not present in `resolvedOptions` when the formatter has no hour field, for example:
```js
var options = Intl.DateTimeFormat("en-US", {hourCycle:"h11"}).resolvedOptions();
assertEq("hourCycle" in options, false);
assertEq("hour12" in options, false);
```


And we need tests to make sure the formatted date actually uses the requested hour-cycle option. These tests probably belong in a different file and can't be upstreamed to test262, because they're tied to a specific ECMA-402 implementation. For example I'd expect the following results for "en-US" with ICU:
```sh
js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h11"}).format(new Date(2017,10-1,10, 0));
"0 AM"
js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h12"}).format(new Date(2017,10-1,10, 0));
"12 AM"
js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h23"}).format(new Date(2017,10-1,10, 0));
"00"
js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h24"}).format(new Date(2017,10-1,10, 0));
"24"
```

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:46
(Diff revision 4)
> +  });
> +  assertEq(hourCycleToH12Map[dtf.resolvedOptions().hourCycle], !value);
> +}
> +
> +// When constructed with hourCycle as extkey, resolvedOptions' hour12 is correct.
> +for (const key of Object.keys(hourCycleToH12Map)) {

Nit: Maybe also use `Object.entries` here?

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:47
(Diff revision 4)
> +  assertEq(hourCycleToH12Map[dtf.resolvedOptions().hourCycle], !value);
> +}
> +
> +// When constructed with hourCycle as extkey, resolvedOptions' hour12 is correct.
> +for (const key of Object.keys(hourCycleToH12Map)) {
> +  const langTag = "en-US"; 

Nit: Trailing whitespace.
Attachment #8892319 - Flags: review?(andrebargull) → review-
And also some tests to ensure the resolved language tag contains the hc extension key:

js> Intl.DateTimeFormat("en-u-hc-h11", {hour:"numeric"}).resolvedOptions().locale
"en-u-hc-h11"


And if the hourCycle option is also present, it overrides the hc extension key:

js> Intl.DateTimeFormat("en-u-hc-h11", {hour:"numeric", hourCycle:"h24"}).resolvedOptions().locale
"en"
js> Intl.DateTimeFormat("en-u-hc-h11", {hour:"numeric", hourCycle:"h24"}).resolvedOptions().hourCycle
"h24"
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review193250

> GetOption() for "hourCycle" needs to happen before the call to ResolveLocale (change from https://github.com/tc39/ecma402/pull/155).

I thought it is - we do this in InitializeDateTimeFormat, which, if I understand correctly, happens before resolvedDateTimeFormat where ResolveLocale happens?

I may be confused because I also don't think I understand why. Resolving locale is not affected by the value of hourCycle, is it?

> It doesn't really matter if a skeleton uses "K" or "h" (resp. "k" or "H"), cf. http://unicode.org/reports/tr35/tr35-dates.html#Date_Field_Symbol_Table 
> 
> So as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1386146#c12, we need to adjust the returned pattern to select the desired hour representation.
> 
> (Assuming ICU4C works the same way as ICU4J when resolving patterns from skeletons.)

Should I do this in `resolveDateTimeFormatInternals` right after retrieving the pattern via `toBestICUPattern`?

Also, what should I do in `toBestICUPattern`? Ignore the hourCycle, merge K+h and k+H? Or keep what I'm doing now?

> We should also test `hourCycle` is not present in `resolvedOptions` when the formatter has no hour field, for example:
> ```js
> var options = Intl.DateTimeFormat("en-US", {hourCycle:"h11"}).resolvedOptions();
> assertEq("hourCycle" in options, false);
> assertEq("hour12" in options, false);
> ```
> 
> 
> And we need tests to make sure the formatted date actually uses the requested hour-cycle option. These tests probably belong in a different file and can't be upstreamed to test262, because they're tied to a specific ECMA-402 implementation. For example I'd expect the following results for "en-US" with ICU:
> ```sh
> js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h11"}).format(new Date(2017,10-1,10, 0));
> "0 AM"
> js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h12"}).format(new Date(2017,10-1,10, 0));
> "12 AM"
> js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h23"}).format(new Date(2017,10-1,10, 0));
> "00"
> js> Intl.DateTimeFormat("en-US", {hour:"numeric", hourCycle:"h24"}).format(new Date(2017,10-1,10, 0));
> "24"
> ```

Unfortunately, that doesn't work, because at the moment CLDR has only skeletons for "H" and "h" (so 23 and 12). I scanned CLDR and it seems like there is no single locale that would support both 11 and 12 or 23 and 24.

That makes testing of 11 vs 12 and 23 vs 24 impossible I believe.

Comment 21

a year ago
mozreview-review
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review194396
Attachment #8892319 - Flags: review?(andrebargull) → review-

Comment 22

a year ago
mozreview-review-reply
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review193250

> I thought it is - we do this in InitializeDateTimeFormat, which, if I understand correctly, happens before resolvedDateTimeFormat where ResolveLocale happens?
> 
> I may be confused because I also don't think I understand why. Resolving locale is not affected by the value of hourCycle, is it?

Derp, I reviewed this without context and forgot we delay the ResolveLocale call. But anyway, the `GetOption(options, "hourCycle", ...)` call needs to happen at an earlier time to ensure all effectful operations are performed in the correct order. For example consider the case when `options` has getter properties. If we reorder the `GetOption` calls, it'd be an observable spec violation.

> Should I do this in `resolveDateTimeFormatInternals` right after retrieving the pattern via `toBestICUPattern`?
> 
> Also, what should I do in `toBestICUPattern`? Ignore the hourCycle, merge K+h and k+H? Or keep what I'm doing now?

Yes, after calling `toBestICUPattern` the pattern needs to be updated if an explicit `hourCycle` was given, see the Java snippet in the other comment. And in that Java implementation I've also just merged K+h resp. k+H, based on the comments in https://unicode.org/reports/tr35/tr35-dates.html#Date_Field_Symbol_Table :


For `h`:
Hour [1-12]. When used in skeleton data or in a skeleton passed in an API for flexible date pattern generation, it should match the 12-hour-cycle format preferred by the locale (h or K); it should not match a 24-hour-cycle format (H or k).

For `K`:
Hour [0-11]. When used in a skeleton, only matches K or h, see above.


So when used in a skeleton, it doesn't really seem to matter if `h` or `K` is used (or `H` and `k`).

> Unfortunately, that doesn't work, because at the moment CLDR has only skeletons for "H" and "h" (so 23 and 12). I scanned CLDR and it seems like there is no single locale that would support both 11 and 12 or 23 and 24.
> 
> That makes testing of 11 vs 12 and 23 vs 24 impossible I believe.

What I didn't tell you is that the above output is from a local build of https://github.com/anba/es6draft with hourCycle support implemented. So if it is implementable with ICU4J, it should also be implementable with ICU4C, I guess. :-)

Here are the relevant bits from that implementation:

```java
    public static String BestFitFormatMatcher(FormatMatcherRecord formatRecord, String dataLocale) {
        // Let ICU4J compute the best applicable pattern for the requested input values
        ULocale locale = ULocale.forLanguageTag(dataLocale);
        DateTimePatternGenerator generator = DateTimePatternGenerator.getInstance(locale);
        String pattern = generator.getBestPattern(formatRecord.toSkeleton());

        // Fixup the hour representation to match the expected hour cycle.
        if (formatRecord.isTime() && formatRecord.hasNonDefaultHourCycle()) {
            pattern = modifyHour(formatRecord, pattern);
        }
        return pattern;
    }

    // See https://unicode.org/reports/tr35/tr35-dates.html#Date_Format_Patterns for the pattern format.
    private static String modifyHour(FormatMatcherRecord formatRecord, String pattern) {
        assert formatRecord.hourCycle() != 0;
        char[] result = pattern.toCharArray();
        boolean quote = false;
        for (int i = 0; i < result.length; ++i) {
            char sym = result[i];
            if (quote || !(sym == 'h' || sym == 'H' || sym == 'k' || sym == 'K')) {
                if (sym == '\'') {
                    if (i + 1 < result.length && result[i + 1] == '\'') {
                        i += 1;
                    } else {
                        quote = !quote;
                    }
                }
                continue;
            }
            result[i] = formatRecord.hourCycle();
        }
        return new String(result);
    }
```
Comment hidden (mozreview-request)
Anba,

I applied your feedback to the point where I'm trying to verify that updating the pattern does actually help, and I can't reproduce it.

In the version of the patch I uploaded I left some debugging statements and if I build it, and launch the JS with:

```
var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h11"});
console.log(dtf.format());

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h12"});
console.log(dtf.format());

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h23"});
console.log(dtf.format());

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h24"});
console.log(dtf.format());
```

I'm getting:

```
"Pattern0: h a"
"Pattern: K a"
7 AM
"Pattern0: h a"
"Pattern: h a"
7 AM
"Pattern0: HH"
"Pattern: HH"
07
"Pattern0: HH"
"Pattern: kk"
07
```

which makes me believe that we're *not* able to differentiate the output based on the pattern.

Can you confirm that that's the best we can get, or did you suggest that we somehow can get h11 vs h12 difference in en-US?
Flags: needinfo?(andrebargull)
Created attachment 8922515 [details] [diff] [review]
bug1386146-modify-hour-representation.patch

This patch applies on your patch. It should fix the hour representation issue.

Notable changes:
- The hourCycle setting wasn't stored in the correct options object, it needs to be stored in |localeOpt.hc| for ResolveLocale to work correctly.
- I've renamed the helper function to "replaceHourRepresentation" to be a bit more precise for Firefox code compared to my Java code. :-)
- And I've also renamed the hour symbol variable, because "hourSkeletonChar" could be a bit misleading, because we're working on a pattern and not on a skeleton.
- In toBestICUPattern, I've changed |hourSkeletonChar| to always use "h" or "H", because in a skeleton it doesn't really matter if h/K or H/k are used and it seems less confusing to me to always pick the same hour skeleton character.
- Fixed a typo in resolveICUPattern.
Flags: needinfo?(andrebargull)
Attachment #8922515 - Flags: review?(gandalf)
Comment on attachment 8922515 [details] [diff] [review]
bug1386146-modify-hour-representation.patch

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

::: js/src/builtin/Intl.js
@@ +2593,5 @@
> +            }
> +        } else {
> +            ch = hour;
> +        }
> +        resultPattern += ch;

The loop body can be simplified to just:
```
var ch = pattern[i];
if (inQuote || !(ch === "h" || ch === "H" || ch === "k" || ch === "K")) {
    if (ch === "'")
        inQuote = !inQuote;
} else {
    ch = hour;
}
resultPattern += ch;
```

or alternatively:
```
var ch = pattern[i];
if (ch === "'") {
    inQuote = !inQuote;
} else if (!inQuote && (ch === "h" || ch === "H" || ch === "k" || ch === "K")) {
    ch = hour;
}
resultPattern += ch;
```

because for our use case we don't need to track the quoted parts in detail. And because this makes the code more consistent with the pattern parser in resolveICUPattern(...).
Comment hidden (mozreview-request)
Thank you :anba!

I updated the patch to your feedback.

I believe that the only remaining issue is that we cannot test between h11/h12 and h23/h24.

With the latest patch, the following code:

```
var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h11"});
console.log(dtf.format());
console.log(JSON.stringify(dtf.resolvedOptions()));

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h12"});
console.log(dtf.format());
console.log(JSON.stringify(dtf.resolvedOptions()));

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h23"});
console.log(dtf.format());
console.log(JSON.stringify(dtf.resolvedOptions()));

var dtf = new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h24"});
console.log(dtf.format());
console.log(JSON.stringify(dtf.resolvedOptions()));

```

returns:

```
3 PM
{"locale":"en-US","calendar":"gregory","numberingSystem":"latn","timeZone":"America/Los_Angeles","hour":"numeric","hourCycle":"h11","hour12":true}
3 PM
{"locale":"en-US","calendar":"gregory","numberingSystem":"latn","timeZone":"America/Los_Angeles","hour":"numeric","hourCycle":"h12","hour12":true}
15
{"locale":"en-US","calendar":"gregory","numberingSystem":"latn","timeZone":"America/Los_Angeles","hour":"2-digit","hourCycle":"h23","hour12":false}
15
{"locale":"en-US","calendar":"gregory","numberingSystem":"latn","timeZone":"America/Los_Angeles","hour":"2-digit","hourCycle":"h24","hour12":false}
```

which means, I can test in resolvedOptions that I'm getting the right hourCycle, but not the output.

Is that ok for you?
Flags: needinfo?(andrebargull)
It should be testable when the input date is at either 00:00 or 12:00, for example:

js> new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h11"}).format(new Date(2017,11-1,3, 0,0,0));
"0 AM"
js> new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h12"}).format(new Date(2017,11-1,3, 0,0,0));
"12 AM"
js> new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h23"}).format(new Date(2017,11-1,3, 0,0,0));
"00"
js> new Intl.DateTimeFormat("en-US", {hour: "numeric", hourCycle: "h24"}).format(new Date(2017,11-1,3, 0,0,0));
"24"
Flags: needinfo?(andrebargull)
(Assignee)

Updated

a year ago
Attachment #8922515 - Attachment is obsolete: true
Attachment #8922515 - Flags: review?(gandalf)
Comment hidden (mozreview-request)
D'uh! Of course I need a date with 0 hour :) Thanks :anba!

The patch is ready I believe.

Comment 32

a year ago
mozreview-review
Comment on attachment 8892319 [details]
Bug 1386146 - Add support for hourCycle to Intl.DateTimeFormat.

https://reviewboard.mozilla.org/r/163282/#review201304

Looks good to me. Thanks!

r+ with the additional tests added:

- We should also test the case when the `hc` Unicode extension key is present and also the `hourCycle` option, to ensure the option overrides the extension key value.
- And probably also the case when `hc` is present and the `hour12` option, because of the spec issue I've mentioned yesterday.
- Plus tests for the resolved locale. (See comment #18.)

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:42
(Diff revision 8)
> +  const dtf = new Intl.DateTimeFormat("en-US", {
> +    hour: "numeric",
> +    hourCycle: key,
> +    hour12: !value
> +  });
> +  assertEq(hourCycleToH12Map[dtf.resolvedOptions().hourCycle], !value);

Also test the resolved `hour12` here?

::: js/src/tests/Intl/DateTimeFormat/hourCycle.js:82
(Diff revision 8)
> +for (const key of invalidHourCycleValues) {
> +  const langTag = "en-US";
> +  const loc = `${langTag}-u-hc-${key}`;
> +
> +  const dtf = new Intl.DateTimeFormat(loc, {hour: "numeric"});
> +  assertEq(dtf.resolvedOptions().hour12, true); // default value for en-US

I think we should also test the resolved `hourCycle` and resolved locale here.
Attachment #8892319 - Flags: review?(andrebargull) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b5994fa88ee
Add support for hourCycle to Intl.DateTimeFormat. r=anba
https://hg.mozilla.org/mozilla-central/rev/3b5994fa88ee
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.