927974 - Rewrite Connect help text to move link out of translated string

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See bug 926929 comment 3 for more context.  The Connect page's "help2" string should be broken up such that the href is not part of the translated string.

Comment 1

[Tareq Khandaker]

Attachment: Renew-Connect-help-text.patch

I rewrote the the last sentence to leave the period/full stop out at the end. I also added a &documentation; entity so that the link text could be localized.

Comment 2

[Tareq Khandaker]

I forgot to ask if there are any tests that need to be run.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 818697 [details] [diff] [review]
Renew-Connect-help-text.patch

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

Thanks for your contribution! :D

Overall it seems good to me!  I just had some nits about the naming of the string keys.

(In reply to Tareq Khandaker from comment #2)
> I forgot to ask if there are any tests that need to be run.

Nope, no tests for this kind of change.  Just make sure that the page looks okay after the change, which you can check by going to Tools -> Web Developer -> Connect.

After you make the changes I mentioned, attach a new patch for me to review.  Assuming it looks good, I'll pass it to another team member for an official stamp, as I am not yet an official reviewer myself.

::: browser/locales/en-US/chrome/browser/devtools/connection-screen.dtd
@@ +18,5 @@
>  <!ENTITY connectionError "Error:">
>  <!ENTITY errorTimeout "Error: connection timeout.">
>  <!ENTITY errorRefused "Error: connection refused.">
>  <!ENTITY errorUnexpected "Unexpected error.">
> +<!ENTITY help2 "Firefox Developer Tools can debug remote devices (Firefox for Android and Firefox OS, for example). Make sure that you have turned on the 'Remote debugging' option in the remote device. For more, see the">

Each time we change translatable string like this, we have to change the key as well.  It could be changed to "help3", but that's rather unsatisfying...  So, let's go with "remoteHelp".  You'll need to update the HTML of course as well.

@@ +19,5 @@
>  <!ENTITY errorTimeout "Error: connection timeout.">
>  <!ENTITY errorRefused "Error: connection refused.">
>  <!ENTITY errorUnexpected "Unexpected error.">
> +<!ENTITY help2 "Firefox Developer Tools can debug remote devices (Firefox for Android and Firefox OS, for example). Make sure that you have turned on the 'Remote debugging' option in the remote device. For more, see the">
> +<!ENTITY documentation "documentation">

Nit: Perhaps the key should be "remoteDocumentation" to make it a clearer when looking at this file.  The string itself is fine.

Comment 4

[Tareq Khandaker]

Attachment: Renew-Connect-help-text.patch

I changed the entity names in both files, and it looked fine to me when I went to chrome://browser/content/devtools/connect.xhtml .

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 818774 [details] [diff] [review]
Renew-Connect-help-text.patch

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

Great, thanks for making those changes!

It seems I missed something last time though...  When the last sentence was reworded, the period after "documentation" was lost.

So, let's add another key, perhaps "remoteHelpSuffix", after the link that just contains ".".

Sorry for missing this the first time!

Comment 6

[Tareq Khandaker]

Attachment: Renew-Connect-help-text.patch

I added remoteHelpSuffix.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 818814 [details] [diff] [review]
Renew-Connect-help-text.patch

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

Great, thanks again!

Panos, as I am not yet a reviewer, can you take a look at this?  I believe it's ready to go.

Tareq, in the mean time, feel free to look over other Dev Tools bugs for others you'd like to help out with.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 818814 [details] [diff] [review]
Renew-Connect-help-text.patch

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

::: browser/devtools/framework/connect/connect.xhtml
@@ +44,5 @@
>      </section>
>      <section id="connecting">
>        <p><img src="chrome://browser/skin/tabbrowser/loading.png"></img> &connecting;</p>
>      </section>
> +    <footer>&remoteHelp; <a target='_' href='https://developer.mozilla.org/docs/Tools/Remote_Debugging'>&remoteDocumentation;</a>&remoteHelpSuffix;</footer>

I think you need to move the space after remoteHelp into the entity to cater to RTL locales. Francesco would know for sure.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 818814 [details] [diff] [review]
Renew-Connect-help-text.patch

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

::: browser/devtools/framework/connect/connect.xhtml
@@ +44,5 @@
>      </section>
>      <section id="connecting">
>        <p><img src="chrome://browser/skin/tabbrowser/loading.png"></img> &connecting;</p>
>      </section>
> +    <footer>&remoteHelp; <a target='_' href='https://developer.mozilla.org/docs/Tools/Remote_Debugging'>&remoteDocumentation;</a>&remoteHelpSuffix;</footer>

Yes, don't hardcode spaces, add it to remoteHelp.

::: browser/locales/en-US/chrome/browser/devtools/connection-screen.dtd
@@ +18,5 @@
>  <!ENTITY connectionError "Error:">
>  <!ENTITY errorTimeout "Error: connection timeout.">
>  <!ENTITY errorRefused "Error: connection refused.">
>  <!ENTITY errorUnexpected "Unexpected error.">
> +<!ENTITY remoteHelp "Firefox Developer Tools can debug remote devices (Firefox for Android and Firefox OS, for example). Make sure that you have turned on the 'Remote debugging' option in the remote device. For more, see the">

I would add a localization note.

<!-- LOCALIZATION NOTE (remoteHelp, remoteDocumentation, remoteHelpSuffix): these strings will be concatenated in a single label, remoteDocumentation will be used as text for a link to MDN. -->

Comment 10

[Tareq Khandaker]

Attachment: Renew-Connect-help-text.patch

I made the alterations to the patch.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 818859 [details] [diff] [review]
Renew-Connect-help-text.patch

If Francesco is OK with this, it's ready to land.

Comment 12

[Francesco Lodolo [:flod]]

Comment on attachment 818859 [details] [diff] [review]
Renew-Connect-help-text.patch

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

Patch definitely looks good to me, switching to f+ since I'm not sure I should review patches (need to clear that with Pike) ;-)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/52b1205696cd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52b1205696cd

Comment 15

[(no longer active)]

Sorry, but I had to back this out because we suspect that one of the 4 patches in this push has caused frequent failures in the mochitest-bc suite (see <https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=8904c520ed40&tochange=39500fdd5007&jobname=browser-chrome>).  Please verify that your patch is not the culprit by running it through the try server and verifying that multiple retriggers of the mochitest-bc suite are all green, and then request checkin-needed again.

Thanks!

(Backout: https://hg.mozilla.org/mozilla-central/rev/38b4f38e7351)

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am going to guess this change is not the one at fault, but here's a try push:

https://tbpl.mozilla.org/?tree=Try&rev=c9b9df64a284

Comment 17

[Phil Ringnalda (:philor)]

Good guess. Relanded in https://hg.mozilla.org/integration/fx-team/rev/f472feeaefc9 once we finally found something better to blame.

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Julen Ruiz Aizpuru]

Attachment: Remote debug connect help text

While localizing current Aurora into my language I have noticed the en-US string is missing the ending whitespace for the `remoteHelp` entity as seen in the attached screenshot — the recommendation from comment #9 has been somehow overlooked.

Localizers can fix this themselves by adding whitespace wherever they might require it, but en-US looks terrible and needs to be fixed. Note that this could also lead to localizers copying the en-US error.

Comment 20

[Julen Ruiz Aizpuru]

Attachment: Added missing whitespace

This patch adds the missing whitespace. Since there's no change in meaning I haven't changed the entity name.

Comment 21

[:Margaret Leibovic]

Comment on attachment 8334036 [details] [diff] [review]
Added missing whitespace

Nice catch! I think flod should review this, since it's a string-only change, and then we should see if we can uplift this en-US change to Aurora.

Comment 22

[Francesco Lodolo [:flod]]

Comment on attachment 8334036 [details] [diff] [review]
Added missing whitespace

It's OK to fix it on both aurora and central without a new ID (asked Pike's opinion on IRC to be sure).

I'm going to send a message to dev-l10n about it, since only 4 locales currently have the correct trailing space on mozilla-aurora (I managed to miss it on my own locale, how shameful).

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The later patch for whitespace changes never landed.  I've moved it to bug 951518.