1363100 - Don't hard-code parenthesis for "(learn more)"

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P3)

Description

[Francesco Lodolo [:flod]]

While reusing an existing string might look like a smart approach, that's actually a bad one for localization.

To get "(learn more)" you reused an existing string and hard-coded parenthesis. Please let's never do that for content that is localized.

If you need "(learn more)", let's add a string that says explicitly "(learn more)". Some languages might not like the parenthesis, or the capitalization might be wrong.

One other thing to remember: localizers don't have context, they see the string. 

What's 'web-ext'? That's information useful for a localization comment, together with (I guess) a note that it should not be localized.

Comment 1

[Francesco Lodolo [:flod]]

And I realize that's not Andy's fault, that's what the code was already doing :-\

Comment 2

[Francesco Lodolo [:flod]]

Unassigning myself because it's a broader problem, besides the localization note for web-ext.

There are 3 cases where Strings.GetStringFromName("moreInfo") is used with hard-coded parenthesis, and all to avoid including them in the link. Is that really needed? Do we need the parenthesis at all? That doesn't sound like the direction other parts of Firefox are taking, I always see "Learn more" (see prefs)

Comment 3

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #2)
> Unassigning myself because it's a broader problem, besides the localization
> note for web-ext.
> 
> There are 3 cases where Strings.GetStringFromName("moreInfo") is used with
> hard-coded parenthesis, and all to avoid including them in the link. Is that
> really needed? Do we need the parenthesis at all? That doesn't sound like
> the direction other parts of Firefox are taking, I always see "Learn more"
> (see prefs)

Julian, you were the reviewer in all 3 cases. Thoughts?

Comment 4

[Julian Descottes [:jdescottes]]

It was originally introduced in Bug 1227137 and then we simply kept reusing the same approach.

I don't mind switching the "some words (<a>more info</a>)" to "some words. <a>Learn more</a>" 

> While reusing an existing string might look like a smart approach, 
> that's actually a bad one for localization.
> 
> To get "(learn more)" you reused an existing string and hard-coded parenthesis. 
> Please let's never do that for content that is localized.

I am not sure if the problem is with reusing a string or with the parentheses. Do we need a different "Learn more" string for each link in about debugging?

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1363100 - change aboutdebugging links text to "Learn more";

Review commit: https://reviewboard.mozilla.org/r/137246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137246/

Comment 6

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fa146236c019ff008b28105b620b8ed844f2c85

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8865622 [details]
Bug 1363100 - change aboutdebugging links text to "Learn more";

https://reviewboard.mozilla.org/r/137246/#review140418

In general reusing a string is bad, if it's used consistently (like it seems to be the case here), it's less of a problem. The real issue is adding content to the text (parenthesis).

This approach is definitely good, thanks.

::: devtools/client/aboutdebugging/components/workers/panel.js:32
(Diff revision 1)
>  
>  const Strings = Services.strings.createBundle(
>    "chrome://devtools/locale/aboutdebugging.properties");
>  
>  const WorkerIcon = "chrome://devtools/skin/images/debugging-workers.svg";
> -const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging";
> +const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging" +

You should drop en-US from MDN links.
https://developer.mozilla.org/docs/Tools/about%3Adebugging/#Service_workers_not_compatible

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the review! 

(In reply to Francesco Lodolo [:flod] from comment #7)
> 
> ::: devtools/client/aboutdebugging/components/workers/panel.js:32
> (Diff revision 1)
> >  
> >  const Strings = Services.strings.createBundle(
> >    "chrome://devtools/locale/aboutdebugging.properties");
> >  
> >  const WorkerIcon = "chrome://devtools/skin/images/debugging-workers.svg";
> > -const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging";
> > +const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging" +
> 
> You should drop en-US from MDN links.
> https://developer.mozilla.org/docs/Tools/about%3Adebugging/
> #Service_workers_not_compatible

I actually thought about changing it, but then I checked the French version of MDN, and the relevant section is not present on this page. I'm not sure what's the best solution here, on one hand I have a link which is in english but is almost guaranteed to have the content or a localized link with no content :/

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8865622 [details]
Bug 1363100 - change aboutdebugging links text to "Learn more";

https://reviewboard.mozilla.org/r/137246/#review140472

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6589eee7c841
change aboutdebugging links text to "Learn more";r=flod,ochameau

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Julian Descottes [:jdescottes] from comment #8)
> I actually thought about changing it, but then I checked the French version
> of MDN, and the relevant section is not present on this page. I'm not sure
> what's the best solution here, on one hand I have a link which is in english
> but is almost guaranteed to have the content or a localized link with no
> content :/

Ugh, good point. I don't manage MDN or SUMO content localization, I assumed the missing bits would be present, but that's obviously not the case (and it makes sense, now that I think of it, since it's basically a wiki).

Comment 12

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/6589eee7c841

Comment 13

[Ton [:Tonnes]]

-addonDebugging.label = Enable add-on debugging
+addonDebugging.label2 = Enable add-on debugging.

Sorry to mention, but I have serious doubts about adding the trailing period here. After all, periods should not be used for tooltips (as mentioned before in various devtools bugs) and neither in any option/preference text, unless they are their descriptions (i.e. sentences, such as in webExtTip).

Again, adding a period generally changes infinitive to imperative (instructive) mood both in English and for many localizations, causing possible misunderstanding in English and bad localizations for the same reason (even though it's obvious this is an option text in this case).

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Ton from comment #13)
> -addonDebugging.label = Enable add-on debugging
> +addonDebugging.label2 = Enable add-on debugging.
> 
> Sorry to mention, but I have serious doubts about adding the trailing period
> here. After all, periods should not be used for tooltips (as mentioned
> before in various devtools bugs) and neither in any option/preference text,
> unless they are their descriptions (i.e. sentences, such as in webExtTip).
> 
> Again, adding a period generally changes infinitive to imperative
> (instructive) mood both in English and for many localizations, causing
> possible misunderstanding in English and bad localizations for the same
> reason (even though it's obvious this is an option text in this case).

I wanted a separator between the text "Enable add-on debugging" and the link "Learn more" since we got rid of the parentheses used with the previous link. Looking at other similar links in Firefox, indeed options are not followed by a period even if they have a "Learn more" link right after.

Francesco, can I push a follow-up to revert this back to "addonDebugging.label" or will this be an issue for some reason?

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Julian Descottes [:jdescottes] from comment #14)
> Francesco, can I push a follow-up to revert this back to
> "addonDebugging.label" or will this be an issue for some reason?

Yes, it would be OK in this case. We haven't exposed this changeset to all locales yet, the only people affected are me and Ton (thanks for noticing it).

BTW, I've just discovered that the whole "(learn more)" thing was utterly broken in localized builds :-\