If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use plural form for blocked pop-up dialog

RESOLVED FIXED in Firefox 22

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: flod, Assigned: mbrubeck)

Tracking

unspecified
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#428

Current implementation
popup.warning=%S prevented this site from opening a pop-up window. Would you like to show it?
popup.warningMultiple=%S prevented this site from opening %S pop-up windows. Would you like to show them?

This code should use the current plural form
https://developer.mozilla.org/en/docs/Localization_and_Plurals

Note that the same problem exists in Firefox for Android (https://bugzilla.mozilla.org/show_bug.cgi?id=739757#c30) and Firefox Metro (https://bugzilla.mozilla.org/show_bug.cgi?id=840888#c21), not sure if a single bug makes sense or we need a separate bug for each platform.
(Assignee)

Updated

5 years ago
Assignee: nobody → mbrubeck
(Assignee)

Comment 1

5 years ago
Created attachment 727784 [details] [diff] [review]
metro patch

Desktop and mobile patches will follow shortly.
Attachment #727784 - Flags: review?(ally)
(Assignee)

Comment 2

5 years ago
Created attachment 727788 [details] [diff] [review]
patch
Attachment #727788 - Flags: review?(dao)
(Assignee)

Comment 3

5 years ago
Created attachment 727790 [details] [diff] [review]
android patch
Attachment #727790 - Flags: review?(margaret.leibovic)
Comment on attachment 727788 [details] [diff] [review]
patch

>+# LOCALIZATION NOTE (popupWarning2): Semicolon-separated list of plural forms.
>+# #1 is brandShortName and #2 is the number of pop-ups blocked.
>+popupWarning2=#1 prevented this site from opening a pop-up window.;#1 prevented this site from opening #2 pop-up windows.

I'd prefer popupWarning.message
Attachment #727788 - Flags: review?(dao) → review+

Comment 5

5 years ago
Comment on attachment 727790 [details] [diff] [review]
android patch

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

r- because the string wasn't copied over correctly.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +46,5 @@
>  # Popup Blocker
> +
> +# LOCALIZATION NOTE (popup.warning2): Semicolon-separated list of plural forms.
> +# #1 is brandShortName and #2 is the number of pop-ups blocked.
> +popup.warning2=#1 prevented this site from opening a pop-up window.;#1 prevented this site from opening #2 pop-up windows.

Looks like you forgot to include the question part at the end (our strings are different from desktop now).

Naming the string popup.message or popup.ask would also be more consistent with our other notification entity names, if you care to do that.
Attachment #727790 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 727895 [details] [diff] [review]
android patch v2

Oops!  Good catch.
Attachment #727790 - Attachment is obsolete: true
Attachment #727895 - Flags: review?(margaret.leibovic)

Updated

5 years ago
Attachment #727895 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 727900 [details] [diff] [review]
android patch v3

Argh, messed this up again.  Sorry.
Attachment #727895 - Attachment is obsolete: true
Attachment #727900 - Flags: review?(margaret.leibovic)

Comment 8

5 years ago
Comment on attachment 727900 [details] [diff] [review]
android patch v3

Oops, and I missed the mistake that time! Good thing there are two of us :)
Attachment #727900 - Flags: review?(margaret.leibovic) → review+

Updated

5 years ago
Attachment #727895 - Flags: review+
Comment on attachment 727784 [details] [diff] [review]
metro patch

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

Ship it!

The little winged :gps on my shoulder points out that if it takes >2 function invocations & >2 lines for 1 assignment, to split things across intermediate variables for readability. 
That said, this matches the other two patches, so please ignore the winged :gps in favor of keeping the code consistent across 3 products.
Attachment #727784 - Flags: review?(ally) → review+
(Assignee)

Comment 10

5 years ago
(In reply to Dão Gottwald [:dao] from comment #4)
> I'd prefer popupWarning.message

Changed the name to popupWarning.message in the desktop and Metro patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/926a4d3be235
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b9af51ea89
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b76d9b32786
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/926a4d3be235
https://hg.mozilla.org/mozilla-central/rev/62b9af51ea89
https://hg.mozilla.org/mozilla-central/rev/8b76d9b32786
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Updated

4 years ago
See Also: → bug 973346
You need to log in before you can comment on or make changes to this bug.