Don't expose SUMO link as localizable string in Kill Addon dialog

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: flod, Assigned: flod)

Tracking

(Depends on: 1 bug)

unspecified
mozilla57
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
https://hg.mozilla.org/mozilla-central/rev/6aa43c3fc0c5

+# LOCALIZATION NOTE (processHang.add-on.label): The first %S is the name of
+# an extension. The second %S is the name of the product (e.g., Firefox)
+processHang.add-on.label = A script in the extension “%S” is causing %S to slow down.

If there are multiple placeholders, use %1$S and %2%S.


+# LOCALIZATION NOTE (KillAddonScriptMessage): The first %S is the name of an add-on. The second %S is the name of the application (e.g., Firefox).
+KillAddonScriptMessage=A script from the add-on “%S” is running on this page, and making %S unresponsive.\n\nIt may be busy, or it may have stopped responsing permanently. You can stop the script now, or you can continue to see if it will complete.

Same here.

NOTE: typo in the string (responsing).

Why is "add-on" in DOM strings and "extension" in BROWSER?

+processHang.add-on.learn-more.url = https://support.mozilla.org/en-US/kb/warning-unresponsive-script?cache=no#w_other-causes

This should have raised red flags in review:
* Why are you exposing this as a localizable string? It shouldn't be localized, and to change the link you would need to update the string ID, and deal with string frozen branches. It should be in the code.
* You never put en-US in a link to SUMO, let the system do its own locale detection.
(Assignee)

Comment 1

a year ago
Is there any chance to have a quick fix landed? I'd really like to avoid exposing that URL for localization.
Flags: needinfo?(kmaglione+bmo)
(In reply to Francesco Lodolo [:flod] from comment #0)
> Why is "add-on" in DOM strings and "extension" in BROWSER?

Because UX didn't give me any updated copy for the non-e10s version, since users aren't expected to see it in practice after 57, and the original versions both said "add-on".

> +processHang.add-on.learn-more.url =
> https://support.mozilla.org/en-US/kb/warning-unresponsive-
> script?cache=no#w_other-causes
> 
> This should have raised red flags in review:

Should it? Is it documented somewhere?

There's no particular review process for localized strings aside from what we have in terms of automated tests. I used a localized string with an included locale code because this is what I've been asked to do for MDN URLs in the past. If we need to do something different for SUMO links, then it should be documented somewhere, and people writing front-end copy should have an obvious way to find it.

(In reply to Francesco Lodolo [:flod] from comment #1)
> Is there any chance to have a quick fix landed? I'd really like to avoid
> exposing that URL for localization.

I'll try to post a patch today, but I'm quite busy at the moment.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 3

a year ago
(In reply to Kris Maglione [:kmag] from comment #2)
> Should it? Is it documented somewhere?

I'm not sure, but that's a good point.

> There's no particular review process for localized strings aside from what
> we have in terms of automated tests. I used a localized string with an
> included locale code because this is what I've been asked to do for MDN URLs
> in the past. If we need to do something different for SUMO links, then it
> should be documented somewhere, and people writing front-end copy should
> have an obvious way to find it.

I've seen MDN links break when doing locale redirection in the past, but I think that was fixed a long time ago.

Having said that, MDN links are usually put inside a sentence in console errors (you can filter by clicking DOM on top)
https://transvision.mozfr.org/?recherche=developer.mozilla.org&repo=central&sourcelocale=en-US&locale=en-US&search_type=strings

(side note: we should use en or en-US consistently…)

As you can note, there are no strings where the only content is a link. That happens a couple of time in DevTools, and should likely be fixed.
(Assignee)

Updated

a year ago
Summary: Don't expose SUMO link as localizable string in → Don't expose SUMO link as localizable string in Kill Addon dialog
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
I hope I didn't break anything in the process…

Note that these changes would normally require a new string ID
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

But since I'm in charge of pushing strings from mozilla-central to the repository used by tools (and these strings haven't been exposed yet), I can skip that part.
Assignee: nobody → francesco.lodolo
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8899228 [details]
Bug 1391933 - Fix localization issues in Kill Add-on dialogs,

https://reviewboard.mozilla.org/r/170490/#review176146

Thanks
Attachment #8899228 - Flags: review?(kmaglione+bmo) → review+

Comment 8

a year ago
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/2486c6e1f89d
Fix localization issues in Kill Add-on dialogs, r=kmag
https://hg.mozilla.org/mozilla-central/rev/2486c6e1f89d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I'm sorry but I need to reopen this. It is not OK and safe to reuse strings like below.

https://hg.mozilla.org/mozilla-central/diff/b83aea215a82/browser/locales/en-US/chrome/browser/browser.properties
> processHang.label = A web page is slowing down your browser. What would you like to do?
>+# LOCALIZATION NOTE (processHang.add-on.label): The first %S is the name of
>+# an extension. The second %S is the name of the product (e.g., Firefox)
>+processHang.add-on.label = A script in the extension "%S" is causing %S to slow down.
>+processHang.add-on.learn-more.text = Learn more
>+processHang.add-on.learn-more.url = https://support.mozilla.org/en-US/kb/warning-unresponsive-script?cache=no#w_other-causes
> processHang.button_stop.label = Stop It
> processHang.button_stop.accessKey = S
> processHang.button_wait.label = Wait
> processHang.button_wait.accessKey = W
> processHang.button_debug.label = Debug Script
> processHang.button_debug.accessKey = D

It may work for English but we need translate differently processHang.button_stop.label for addon/extension/web page/ ("Zatrzymaj ją" vs "Zatrzymaj go").
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

a year ago
I told you in the past: please file new bugs (yours definitely a legitimate request), don't reopen bugs assigned with landed patches.
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Depends on: 1399104

Comment 12

a year ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Updated

a year ago
Flags: needinfo?(francesco.lodolo) → qe-verify-

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.