1383562 - Months in the downloads panel are in nominative instead of genitive

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[Piotr Drąg [:piotrdrag]]

Attachment: Dates in nominative instead of genitive

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Opened the download panel using Polish localization.


Actual results:

"15 lipiec" is displayed ("Months - wide - Standalone" in CLDR.)


Expected results:

"15 lipca" should be displayed ("Months - wide - Formatting" in CLDR.)

Comment 1

[Piotr Drąg [:piotrdrag]]

I can’t find anything in localization that could be responsible. It’s more likely to be a bug in tools or incorrect use of ICU data.

Comment 2

[YF (Yang)]

https://dxr.mozilla.org/mozilla-central/search?q=lipiec

Comment 3

[André Bargull [:anba]]

|new Date(2017, 7-1, 15).toLocaleString("pl", {month: "long", day: "numeric"})| returns "15 lipca" for me (current inbound, but also moz54).

DownloadUtils.jsm computes the standalone month and then concatenates it with the current day [1,2]. Maybe it makes sense to either switch completely to Intl.DateTimeFormat or use Intl.DateTimeFormat.prototype.formatToParts to retrieve the correct month name in a non-standalone context.


[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/mozapps/downloads/DownloadUtils.jsm#367-369
[2] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#107

Comment 4

[André Bargull [:anba]]

Either way, it's not an issue for the "JavaScript: Internationalization API" component.

Comment 5

[YF (Yang)]

new Date(2017, 7-1, 15).toLocaleString("pl", {month: "long"}) return:
"lipiec"

Comment 6

[André Bargull [:anba]]

(In reply to YF (Yang) from comment #5)
> new Date(2017, 7-1, 15).toLocaleString("pl", {month: "long"}) return:
> "lipiec"

Yes, that looks correct to me. "lipiec" in a stand-alone context, otherwise "lipca".

From CLDR (common/main/pl.xml):

<calendar type="gregorian">
  <months>
    <monthContext type="format">
      <!-- ... -->
      <monthWidth type="wide">
        <month type="1">stycznia</month>
        <month type="2">lutego</month>
        <month type="3">marca</month>
        <month type="4">kwietnia</month>
        <month type="5">maja</month>
        <month type="6">czerwca</month>
        <month type="7">lipca</month>
        <month type="8">sierpnia</month>
        <month type="9">września</month>
        <month type="10">października</month>
        <month type="11">listopada</month>
        <month type="12">grudnia</month>
      </monthWidth>
    </monthContext>
    <monthContext type="stand-alone">
      <!-- ... -->
      <monthWidth type="wide">
        <month type="1">styczeń</month>
        <month type="2">luty</month>
        <month type="3">marzec</month>
        <month type="4">kwiecień</month>
        <month type="5">maj</month>
        <month type="6">czerwiec</month>
        <month type="7">lipiec</month>
        <month type="8">sierpień</month>
        <month type="9">wrzesień</month>
        <month type="10">październik</month>
        <month type="11">listopad</month>
        <month type="12">grudzień</month>
      </monthWidth>
    </monthContext>
  </months>

Comment 7

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

Yeah, Andre is right, this should be switched to full Intl API:

aDate.getLocaleString(undefined, {
  month: "long",
  day: "numeric"
});

and remove the entity from .properties

Comment 8

[:Paolo Amadini]

Thanks gandalf, with the solution already outlined in comment 7 this looks like a good first bug for a new contributor, but anyone should feel free to fix it.

Comment 9

[Luis]

Hello. I'd be happy to help if there's still work to do on this. As far as I know, the fix has to be applied in downloadUtils.jsm, in line #367. I'll wait for feedback.

Comment 10

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

yes!

I'll be happy to mentor you.

Comment 11

[Luis]

OK. so, it has to go from this:

let month = aDate.toLocaleDateString(undefined, {month: "long" });

to:

let dateTimeCompact = Intl.DateTimeFormat(undefined, {month: "long", day: "numeric"}).format(aDate);

and then drop the next two lines. Will this fix the problem? I'm not sure how to check the validity of the code; nodejs in my computer shows a valid string, but I'm still not sure.

Comment 12

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

You can build Firefox using Artifact Build - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

This will be quite fast and allow you to modify JS code, rebuild and see the result.

Comment 13

[Luis]

thank you. I'm on it now.

Comment 14

[Luis]

I got my build to run (had already built it, just forgot how to run it), and the solution is already in the works. Will have to wait until tomorrow to continue and hopefully finish before next weekend. I'll keep you updated. Attempted the change proposed before, but it'll take some more time.

Comment 15

[sajattack]

Is this still being worked on?

Comment 16

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

Seems like this bug is for grabs.

The instruction for anyone interested:

1) Replace the code in http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/mozapps/downloads/DownloadUtils.jsm#367-369

to use:

aDate.toLocaleString(undefined, {
  month: "long",
  day: "numeric"
});

2) Remove this line: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#107

Comment 17

[hardik tyagi [:hrdktg]]

Attachment: Bugfix#1383562

Comment 18

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

Comment on attachment 8920421 [details] [diff] [review]
Bugfix#1383562

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

Thank you for the patch! Looks good!

One more nit and we'll be done I believe :)

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +367,5 @@
> +      let curMonthDate = aDate.toLocaleString(undefined, {
> +                    month: "long",
> +                    day: "numeric"
> +      });
> +      dateTimeCompact = gBundle.formatStringFromName(gStr.monthDate, [curMonthDate], 1);

No need for the string: `dateTimeCompact = aDate.toLoclaeString(...);`

Also, you can then remove the `gStr.monthDate` in line 88.

Comment 19

[hardik tyagi [:hrdktg]]

There is a big problem, I did the above changes built the code and ran it. Saw in the downloads panel by changing the dates and it is just blank.

On the original code the download panel shows the Month and date of download.
Is there any proper way of checking the output of this javascript function.

Comment 20

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

Can you update your patch so I can test it?

```
var aDate = new Date();aDate.toLocaleString(undefined, {month: "long",day: "numeric"});
```

in a browser console works for me.

Comment 21

[hardik tyagi [:hrdktg]]

Attachment: ttt.patch

Problem was on my build. I was doing a silly mistake. Its working good now.

Comment 22

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

Just one more nit. Can you also remove this line: http://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#82

Thank you!

Comment 23

[hardik tyagi [:hrdktg]]

Attachment: fixed.patch

Comment 24

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

That looks good!

Sorry for not mentioning in the previous round - you need to update the bug title to something aligned with our bug naming guidelines. 

Instead of `Fixed Bug#1383562` it should be: `Bug 1383562 - Correctly decline date in DownloadUtils.jsm. r=gandalf`

Comment 25

[hardik tyagi [:hrdktg]]

Is "Bug 1383562 - Correctly get Month and date in DownloadUtils.jsm. r=gandalf" fine?

Comment 26

[hardik tyagi [:hrdktg]]

Attachment: Bug 1383562 - Correctly get Month and date in DownloadUtils.jsm. r=gandalf

Comment 27

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

yes! :) But I think you accidentally removed the hunk that removes the line 88 from the file ;)

Comment 28

[hardik tyagi [:hrdktg]]

Attachment: Bug 1383562 - Correctly get Month and date in DownloadUtils.jsm. r=gandalf

:) Sorry..

Comment 29

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

Looks good! 

Aaand, last request - can you remove the localization comment that is right above the line you removed from the .properties file?
It's not needed once the string is gone :)

Comment 30

[hardik tyagi [:hrdktg]]

Attachment: Bug 1383562 - Correctly get Month and date in DownloadUtils.jsm. r=gandalf

Comment 31

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

Comment on attachment 8920530 [details] [diff] [review]
Bug 1383562 - Correctly get Month and date in DownloadUtils.jsm. r=gandalf

Thank you! Looks great!

Comment 32

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

When you're ready to land this - add "checkin-needed" to keywords.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42c58ea0e83
Correctly get Month and date in DownloadUtils.jsm. r=gandalf

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 35

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

Thank you for your contribution hardik!