1574930 - [Protections Panel] Blocked trackers count isn't locale-formatted

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

We should do Number.prototype.toLocaleString [1] in gProtectionsHandler.setTrackersBlockedCounter() [2]

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString
[2] https://searchfox.org/mozilla-central/rev/5912f376ab6a17afcba2b7654586013158ed64b5/browser/base/content/browser-siteProtections.js#1955

Tim, want to take this?

Comment 1

[(no longer active)]

Please note that toLocaleString() by default (unless passed an explicit locale argument) uses the locale of the OS not that of the Firefox UI, so if for example the user is running an en-US build on an OS set to use Arabic locale information by default, using toLocaleString() without an explicit locale argument produces the wrong results. The only correct way to use it would be to pass the %AB_CD% locale arguments we receive from the build system to the function, I think...

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to :Ehsan Akhgari from comment #1)

Please note that toLocaleString() by default (unless passed an explicit locale argument) uses the locale of the OS not that of the Firefox UI, so if for example the user is running an en-US build on an OS set to use Arabic locale information by default, using toLocaleString() without an explicit locale argument produces the wrong results. The only correct way to use it would be to pass the %AB_CD% locale arguments we receive from the build system to the function, I think...

Thanks for this hint. I think [3] might be a good example of this...

[3] https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/toolkit/content/widgets/datetimebox.js#1044

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

If the example in [3] proves not to be useful and it turns out to be difficult/complicated to get this perfectly right, I would not be against landing a simple toLocaleString() fix with a follow-up filed to figure out the OS/Firefox locale disparity situation.

Comment 4

[(no longer active)]

Note that the example you gave uses the getWebExposedLocales() API. That is useful for cases where we want to expose our locale information to web pages (which is the case for the <input type=date/time> controls. But this isn't exactly the same case. For example the aforementioned API honours the privacy.spoof_english and intl.regional_prefs.use_os_locales prefs.

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)

I would not be against landing a simple toLocaleString() fix with a follow-up filed to figure out the OS/Firefox locale disparity situation.

If by "simple" here you mean omitting the locales argument to that function, that will certainly do the wrong thing for all of our users who are on builds which do not match their Firefox UI locale. (The fact that maybe a large portion of our developers use en-US builds on English OS locales is a very sad coincidence but it doesn't represent what our users see.)

intl/docs/locale.rst explains how to do this correctly (see "Language Negotiation" onwards).

Comment 5

[Tim Huang[:timhuang] [PTO 10/19 ~ 11/3]]

There is an issue that the tracker counter will be shown even if the number of blocked trackers is zero. I will also fix this in this bug.

Comment 6

[Tim Huang[:timhuang] [PTO 10/19 ~ 11/3]]

Attachment: Bug 1574930 - Part 1: Use the app locale for the tracker counter number in protections panel. r?nhnt11!,Ehsan!

We use the locale of the application to show the number of the trakcer
counter.

Comment 7

[Tim Huang[:timhuang] [PTO 10/19 ~ 11/3]]

Attachment: Bug 1574930 - Part 2: Fix the issue the the number of blocked trackers is still shown if the count is zero. r?nhnt11!

By adding a check for ensuring the counter will be only shown when the
count is not zero.

Depends on D42716

Comment 8

[Tim Huang[:timhuang] [PTO 10/19 ~ 11/3]]

Attachment: Bug 1574930 - Part 3: Add a test case for making sure that the track counter displays properly and fix an issue for BrowserTestUtils.waitForAttribute(). r?nhnt11!,johannh!

This patch adds a test case to check the visibility of the track counter
is correct in different situations, including zero tracker, one tracker
and multiple trackers.

Also, this patch fixes an issue that the
BrowserTestUtils.waitForAttributes() won't work if the waiting
attributes doesn't have a value or has an empty string as its value.
We check the value of the observing attribute to verify if it is
existing, however an empty string will be treated as a false, so it
won't pass the check if the attribute doesn't have a value. Hence, we
should use hasAttribute() instead of getAttribute() to check the
existence of the attribute.

Depends on D42717

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/085122dae600
Part 1: Use the app locale for the tracker counter number in protections panel. r=nhnt11,Ehsan
https://hg.mozilla.org/integration/autoland/rev/96854f5ed90d
Part 2: Fix the issue the the number of blocked trackers is still shown if the count is zero. r=nhnt11
https://hg.mozilla.org/integration/autoland/rev/5dbac06bc222
Part 3: Add a test case for making sure that the track counter displays properly and fix an issue for BrowserTestUtils.waitForAttribute(). r=nhnt11,johannh

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/085122dae600
https://hg.mozilla.org/mozilla-central/rev/96854f5ed90d
https://hg.mozilla.org/mozilla-central/rev/5dbac06bc222