tc39 / proposal-intl-duration-format

https://tc39.es/proposal-intl-duration-format
MIT License
163 stars 18 forks source link

hours: "numeric" w/ 0 in minutes and non 0 in seconds cause strange output. #170

Closed FrankYFTang closed 4 months ago

FrankYFTang commented 1 year ago

@anba's https://github.com/tc39/test262/pull/3903 surface the following issue intl402/DurationFormat/prototype/format/numeric-hour-with-zero-minutes-and-non-zero-seconds.js

What should be the output of

 (new Intl.DurationFormat("en", {  hours: "numeric",})).format({  hours: 1,  minutes: 0,  seconds: 3})

@anba's code, following the spec text produce the expectation as

1, 03

and currently, the buggy v8 implement 1:03 (which is neither correct) I think it should be 1:00:03 v8 also incorrectly output

 (new Intl.DurationFormat("en", {  hours: "numeric",})).formatToParts({  hours: 1,  minutes: 0,  seconds: 3})
[{type: "integer", value: "1", unit: "hour"}, {type: "literal", value: ":"}, {type: "integer", value: "03", unit: "second"}]

I think it should be

[{type: "integer", value: "1", unit: "hour"}, {type: "literal", value: ":"}, {type: "integer", value: "00", unit: "minute"}, {type: "literal", value: ":"}, {type: "integer", value: "03", unit: "second"}]

but that is not following the spec text.

We should also consider case like

 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9 })).format({  hours: 1,   microseconds: 4})
 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9})).format({  hours: 1,   milliseconds: 5})
 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9})).format({  hours: 1,   nanoseconds: 6})
FrankYFTang commented 1 year ago

@ben-allen @sffc @ryzokuken @anba

FrankYFTang commented 1 year ago

I think the problem is here

9. If unit is "hours" or "minutes", then
  a. If unit is "hours", then
    i. Let nextValue be duration.[[Minutes]].
    ii. Let nextDisplay be durationFormat.[[MinutesDisplay]].
  b. Else,
    i. Let nextValue be duration.[[Seconds]] + duration.[[Milliseconds]] / 103 + duration.[[Microseconds]] / 106 + duration.[[Nanoseconds]] / 109.
    ii. Let nextDisplay be durationFormat.[[SecondsDisplay]].
  c. If nextValue is not 0 or nextDisplay is not "auto", then
    i add  separator block...

We actually do not need to calculate nextValue since the only usage is compare against 0, we just need a boolean for step c and set that boolean in sub steps of a and b

How about

9. If unit is "hours" or "minutes", then
a. Let addSep be *false*.
b If duration.[[Nanoseconds]] is not 0, set addSep to *true*.
c Else if duration.[[Microseconds]] is not 0, set addSep to *true*.
d Else if duration.[[Milliseconds]] is not 0, set addSep to *true*.
e Else if duration.[[Seconds]] is not 0, set addSep to *true*.
f Else if unit is "hours" and duration.[[Minutes]] is not 0, set addSep to *true*.
g Else if unit is "hours" and durationFormat.[[MinutesDisplay]] is "always", set addSep to *true*.
h Else if unit is "minutes" and durationFormat.[[SecondsDisplay]] is "always", set addSep to *true*.
i If addSep is *true*, then
  i add  separator block...
anba commented 1 year ago

The proposed changes in https://github.com/tc39/proposal-intl-duration-format/issues/170#issuecomment-1692666120 will add a separator even when the next unit isn't displayed.

If we want to return "1:00:03" for the above mentioned test case, it's also necessary to make some adjustments to current step 4.l:

If value is not 0 or display is not "auto", then

And change it to:

If value is not 0 or display is not "auto" or separated is true, then

And then remove step 4.l.ii.5:

  1. Else, a. Set separated to false.

If #166 gets accepted, slightly different changes are necessary.

sffc commented 11 months ago

My understanding is that new Intl.DurationFormat("en", { hours: "numeric" }) is supposed to imply that the remainder of fields are numeric and should therefore output the expected results, but it is definitely a problem that minutes are not displayed when zero. Hmm

sffc commented 11 months ago

@ben-allen to discuss with @romulocintra and @ryzokuken on who should work on this.

ben-allen commented 4 months ago

Marking closed as fixed by #180 & #183