Closed Bug 1408369 Opened 7 years ago Closed 7 years ago

[dt-onboarding] Move aboutdevtools.dtd back to locales/en-US folder when ready for localization

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1408369 - create about:devtools welcome message dynamically;
59 bytes, text/x-review-board-request
flod
: review+
nchevobbe
: review+
Details
Bug 1408369 - move back aboutdevtools.dtd to devtools/shim/locales;
59 bytes, text/x-review-board-request
flod
: review+
Details 
Follow up to Bug 1408368.

We are temporarily moving aboutdevtools.dtd out of devtools/shim/locales/en-US because the file is not ready for localization. We need to revert this change and comment the DTD file properly when the strings are ready to be localized.
Blocks: dt-onboarding
No longer blocks: dt-addon-uishim
Blocks: 1408339
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8925519 [details]
Bug 1408369 - create about:devtools welcome message dynamically;

https://reviewboard.mozilla.org/r/196654/#review201924

General question: is it OK to have all those "Firefox" hard-coded, instead of a brand name that can change based on the build? My guess would be yes, but better to ask.

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:61
(Diff revision 1)
>          <!-- Form dedicated to the newsletter subscription -->
>          <div class="newsletter">
>            <h2 class="newsletter-title">&aboutDevtools.newsletter.title;</h2>
>            <p>&aboutDevtools.newsletter.message;</p>
>  
>            <form id="newsletter-form" name="newsletter-form" action="https://www.mozilla.org/en-US/newsletter/" method="post">

Drop en-US from links to mozilla.org, let the website do the proper redirection based on the user's preferences.

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:101
(Diff revision 1)
>             alt="Firefox Developer Edition logo"/>
>        <div class="footer-message">
> -        <h1 class="footer-message-title">Firefox Developer Edition</h1>
> -        <p>
> -          Looking for more than just Developer Tools?
> -          Check out the Firefox browser that is built specifically for developers and modern workflows.
> +        <h1 class="footer-message-title">&aboutDevtools.footer.title;</h1>
> +        <p>&aboutDevtools.footer.message;</p>
> +        <a class="external footer-link"
> +           href="https://www.mozilla.org/en-US/firefox/developer/"

Same here, remove /en-US/

::: devtools/shim/aboutdevtools/tmp-locale/aboutdevtools.dtd:72
(Diff revision 1)
>  <!ENTITY  aboutDevtools.newsletter.thanks.message "If you haven’t previously confirmed a subscription to a Mozilla-related newsletter you may have to do so. Please check your inbox or your spam filter for an email from us.">
>  
> +<!ENTITY  aboutDevtools.footer.title "Firefox Developer Edition">
> +<!ENTITY  aboutDevtools.footer.message "Looking for more than just Developer Tools? Check out the Firefox browser that is built specifically for developers and modern workflows.">
> +
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.learnMoreLink): Text for the link to

Wront string reference in the comment

::: devtools/shim/aboutdevtools/tmp-locale/aboutdevtools.dtd:73
(Diff revision 1)
>  
> +<!ENTITY  aboutDevtools.footer.title "Firefox Developer Edition">
> +<!ENTITY  aboutDevtools.footer.message "Looking for more than just Developer Tools? Check out the Firefox browser that is built specifically for developers and modern workflows.">
> +
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.learnMoreLink): Text for the link to
> +  -  https://www.mozilla.org/en-US/firefox/developer/ displayed in the footer. -->

Remove en-US, even if it's just a comment
Thanks for jumping on this :flod, I'll update the patch with your comments before sending it back to you for a final review!
Comment on attachment 8925519 [details]
Bug 1408369 - create about:devtools welcome message dynamically;

https://reviewboard.mozilla.org/r/196654/#review201924

> Drop en-US from links to mozilla.org, let the website do the proper redirection based on the user's preferences.

For this particular link the en-US doesn't have any impact on the actual action performed on the server for POST requests. Newsletter localization will be handled in Bug 1415273 (basically we need to let the user pick one of the available languages for the newsletter)
(In reply to Julian Descottes [:jdescottes] from comment #7)
> Comment on attachment 8925519 [details]
> Bug 1408369 - create about:devtools welcome message dynamically;
> 
> https://reviewboard.mozilla.org/r/196654/#review201924
> 
> > Drop en-US from links to mozilla.org, let the website do the proper redirection based on the user's preferences.
> 
> For this particular link the en-US doesn't have any impact on the actual
> action performed on the server for POST requests. Newsletter localization
> will be handled in Bug 1415273 (basically we need to let the user pick one
> of the available languages for the newsletter)

This first comment was about the en-US in the following form action attribute.

<form id="newsletter-form" 
      name="newsletter-form" 
      action="https://www.mozilla.org/en-US/newsletter/" 
      method="post">
(In reply to Julian Descottes [:jdescottes] from comment #7)
> For this particular link the en-US doesn't have any impact on the actual
> action performed on the server for POST requests. Newsletter localization
> will be handled in Bug 1415273 (basically we need to let the user pick one
> of the available languages for the newsletter)

Sorry but I'm not following. By using https://www.mozilla.org/en-US/newsletter/, I assume that you send all users to a confirmation page in English, instead of sending them to a localized page. That is unrelated to the language of the newsletter itself.

Example: I'm on an Italian browser, if I open https://www.mozilla.org/newsletter/ I get to https://www.mozilla.org/it/newsletter/ (localized), and get a confirmation message in Italian if I suscribe. Note that newsletter are not localized in Italian at all, only 6 languages are available.

I really think that URL should be https://www.mozilla.org/newsletter/, not https://www.mozilla.org/en-US/newsletter/
(In reply to Francesco Lodolo [:flod] from comment #9)
> (In reply to Julian Descottes [:jdescottes] from comment #7)
> > For this particular link the en-US doesn't have any impact on the actual
> > action performed on the server for POST requests. Newsletter localization
> > will be handled in Bug 1415273 (basically we need to let the user pick one
> > of the available languages for the newsletter)
> 
> Sorry but I'm not following. By using
> https://www.mozilla.org/en-US/newsletter/, I assume that you send all users
> to a confirmation page in English, instead of sending them to a localized
> page. That is unrelated to the language of the newsletter itself.
> 
> Example: I'm on an Italian browser, if I open
> https://www.mozilla.org/newsletter/ I get to
> https://www.mozilla.org/it/newsletter/ (localized), and get a confirmation
> message in Italian if I suscribe. Note that newsletter are not localized in
> Italian at all, only 6 languages are available.
> 
> I really think that URL should be https://www.mozilla.org/newsletter/, not
> https://www.mozilla.org/en-US/newsletter/

We are not sending the users to the page, it's just the URL we use to send the subscription request via a XHR. The confirmation message is handled in about:devtools. No navigation involved, we preventDefault() on the submit event of the form.
(In reply to Julian Descottes [:jdescottes] from comment #10)
> We are not sending the users to the page, it's just the URL we use to send
> the subscription request via a XHR. The confirmation message is handled in
> about:devtools. No navigation involved, we preventDefault() on the submit
> event of the form.

Thanks, that was the part that I was missing.
Comment on attachment 8925519 [details]
Bug 1408369 - create about:devtools welcome message dynamically;

https://reviewboard.mozilla.org/r/196654/#review202830

Side question: can "DevTools" be localized? We're localizing "Developer Tools", and we only have 3 strings using "DevTools" in m-c, while these patches add a lot of them.
Attachment #8925519 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8925520 [details]
Bug 1408369 - move back aboutdevtools.dtd to devtools/shim/locales;

https://reviewboard.mozilla.org/r/196656/#review202832

Mostly nits and questions, only one real thing that we should fix.

::: devtools/shim/locales/en-US/aboutdevtools.dtd:28
(Diff revision 2)
> +  -  the Inspect Element menu item. -->
>  <!ENTITY  aboutDevtools.enable.inspectElementTitle "Enable Firefox Developer Tools to use Inspect Element">
>  
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.aboutDebuggingMessage): Message displayed
> +  -  when users come from about:debugging. -->
>  <!ENTITY  aboutDevtools.enable.aboutDebuggingMessage

All messages but inspectElement don't have an associated title?

::: devtools/shim/locales/en-US/aboutdevtools.dtd:33
(Diff revision 2)
>  <!ENTITY  aboutDevtools.enable.aboutDebuggingMessage
>            "Develop and debug WebExtensions, web workers, service workers and more with Firefox DevTools.">
> +
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.inspectElementMessage): Message displayed
> +  -  when users come from using the Inspect Element menu item. -->
>  <!ENTITY  aboutDevtools.enable.inspectElementMessage

We should move this near the aboutDevtools.enable.inspectElementTitle

::: devtools/shim/locales/en-US/aboutdevtools.dtd:47
(Diff revision 2)
> +  -  clicked on a "Enable Developer Tools" menu item. -->
>  <!ENTITY  aboutDevtools.enable.menuMessage
>            "Examine, edit and debug HTML, CSS, and JavaScript with tools like Inspector and Debugger.">
>  
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.commonMessage): Generic message displayed for
> +  -  all possible entry points (keyshortcut, menu item etc...). -->

Uber nit-pick: single unicode … instead of ...

::: devtools/shim/locales/en-US/aboutdevtools.dtd:67
(Diff revision 2)
> +<!-- LOCALIZATION NOTE (aboutDevtools.newsletter.message): Subscribe form message.
> +  -  The newsletter is only available in english at the moment.
> +  -  See Bug 1415273 for support of additional languages.-->
>  <!ENTITY  aboutDevtools.newsletter.message "Get developer news, tricks and resources sent straight to your inbox.">
>  <!ENTITY  aboutDevtools.newsletter.email.placeholder "Email">
>  <!ENTITY  aboutDevtools.newsletter.privacy.label "I’m okay with Mozilla handling my info as explained in this <a class=&#x0022;external&#x0022; href=&#x0022;https://www.mozilla.org/privacy/&#x0022;>Privacy Policy</a>.">

Can we avoid using &#x0022 here? It's quite horrible, and easy to break.

We have plenty of HTML fragments in string, they use a single straight quote, e.g. https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/overrides/netError.dtd#l152

You'll only need to add that string to this test's exceptions
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/browser/base/content/test/static/browser_misused_characters_in_strings.js#17
(In reply to Francesco Lodolo [:flod] from comment #12)
> Comment on attachment 8925519 [details]
> Bug 1408369 - create about:devtools welcome message dynamically;
> 
> https://reviewboard.mozilla.org/r/196654/#review202830
> 
> Side question: can "DevTools" be localized? We're localizing "Developer
> Tools", and we only have 3 strings using "DevTools" in m-c, while these
> patches add a lot of them.

Good question, I think most of the time, it gets expanded to "Developer Tools" and then translated. For instance in french I found DevTools localized to "outils de développement". So I think we should translate them but I also think we should avoid them as much as we can since we don't have a good equivalent in other languages. 

Let's wait for a bit, I'll try to see if we can get rid of some of the "DevTools". Either expand to Developer Tools or remove it if the context is clear.
Comment on attachment 8925520 [details]
Bug 1408369 - move back aboutdevtools.dtd to devtools/shim/locales;

https://reviewboard.mozilla.org/r/196656/#review202832

> Can we avoid using &#x0022 here? It's quite horrible, and easy to break.
> 
> We have plenty of HTML fragments in string, they use a single straight quote, e.g. https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/overrides/netError.dtd#l152
> 
> You'll only need to add that string to this test's exceptions
> http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/browser/base/content/test/static/browser_misused_characters_in_strings.js#17

Good point, done. Let me know if you are not comfortable reviewing the test change.
Comment on attachment 8925520 [details]
Bug 1408369 - move back aboutdevtools.dtd to devtools/shim/locales;

https://reviewboard.mozilla.org/r/196656/#review202832

> All messages but inspectElement don't have an associated title?

Yes, that is the only entry point with a custom title at the moment.
Comment on attachment 8925519 [details]
Bug 1408369 - create about:devtools welcome message dynamically;

https://reviewboard.mozilla.org/r/196654/#review202862

LGTM

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:53
(Diff revision 3)
>      <div class="box">
>        <div class="left-pane" />
>        <div class="right-pane">
>          <h1 class="title" >&aboutDevtools.welcome.title;</h1>
> -        <p id="welcome-message">&aboutDevtools.welcome.message;</p>
> +        <!-- The welcome message is dynamically updated with a keyboard shortcut at
> +             runtime and is added in aboutdevtools.js -->

nit: s/is added/added
Attachment #8925519 - Flags: review?(nchevobbe) → review+
Comment on attachment 8925519 [details]
Bug 1408369 - create about:devtools welcome message dynamically;

https://reviewboard.mozilla.org/r/196654/#review202862

Thanks for the review, fixed the nit.
Comment on attachment 8925520 [details]
Bug 1408369 - move back aboutdevtools.dtd to devtools/shim/locales;

https://reviewboard.mozilla.org/r/196656/#review202870

One final thing to fix, but it's look good to me, and seems simple enough to review.

::: devtools/shim/locales/en-US/aboutdevtools.dtd:29
(Diff revision 4)
>  <!ENTITY  aboutDevtools.enable.inspectElementTitle "Enable Firefox Developer Tools to use Inspect Element">
>  
> -<!ENTITY  aboutDevtools.enable.aboutDebuggingMessage
> -          "Develop and debug WebExtensions, web workers, service workers and more with Firefox DevTools.">
> +<!-- LOCALIZATION NOTE (aboutDevtools.enable.inspectElementMessage): Message displayed
> +  -  when users come from using the Inspect Element menu item. -->
>  <!ENTITY  aboutDevtools.enable.inspectElementMessage
> -          "Examine and edit HTML and CSS with the DevTools Inspector.">
> +          "Examine and edit HTML and CSS with the Developer Tools' Inspector.">

You need to use an apostrophe here, otherwise tests will fail.
Attachment #8925520 - Flags: review?(francesco.lodolo) → review+
Good catch, thanks!
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43a6a070e9d3
create about:devtools welcome message dynamically;r=flod,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/4cd32018fcd2
move back aboutdevtools.dtd to devtools/shim/locales;r=flod
https://hg.mozilla.org/mozilla-central/rev/43a6a070e9d3
https://hg.mozilla.org/mozilla-central/rev/4cd32018fcd2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: