Closed Bug 1450071 Opened 2 years ago Closed 2 years ago

Application panel: support localization

Categories

(DevTools :: Application Panel, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Follow up to Bug 1445197. 

Bug 1445197 will land with hardcoded strings. The goal of this bug is to use l10n mechanisms to read strings, while keeping the strings non visible to localizers until the UI is stable.
Francesco, I want to reuse the same approach as what we did for the onboarding screen: Bug 1408368. Are you fine with this?
Flags: needinfo?(francesco.lodolo)
Yes, that approach works fine for me.
Flags: needinfo?(francesco.lodolo)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Change of plans here: we can't do the same trick as in Bug 1408368 to land strings not exposed to localizers due to the way devtools localization helpers work right now. 

The currently attached patch is using the usual localization tools used in the rest of devtools. 

However the application panel is an occasion to try using fluent-react (react bindings for fluent). 

To do so, we need a vendored/bundled version of fluent-react that can be loaded with the DevTools loader. Currently the bundling of react-fluent is done with rollup, see https://github.com/projectfluent/fluent.js. Talking with :stas, we should get started with the artifact fluent-react.js. We might do what we do for libraries (e.g. as react-dom) and modify the built artifact in order to fetch our vendored versions of React, proptypes etc...

Once we figure out a way to load fluent-react.js in a devtools compatible way, we will need to fetch the ftl resources and perform the language negociation. Here L10nRegsitry should help.

One thing to keep in mind here is that this panel is defining a handful of new strings (see currently attached patch) so we should avoid landing this at the last minute in 62 cycle.
Summary: Application panel: support localization (without exposing strings to localizers yet) → Application panel: support localization
I built and modified fluent-react to be able to load it with devtools. I wrapped one of my components with withLocalization, now I need to wrap everything with a LocalizationProvider component and therefore I need to figure out how to get messages.

I added an application.ftl file to devtools/client/locales/en-US/ and modified the corresponding jar.mn to add
  [localization] @AB_CD@.jar:
    devtools               (en-US/**/*.ftl)

After that I am not sure what I should do to get a context and/or messages. Any example I could look at here?
Flags: needinfo?(stas)
Thanks to the support from both :stas and :zibi I have a prototype to use fluent react in the application panel.

However, we might have an issue with the "parseMarkup" logic from https://github.com/projectfluent/fluent.js/blob/master/fluent-react/src/markup.js:

  const TEMPLATE = document.createElement("template");

  export function parseMarkup(str) {
    TEMPLATE.innerHTML = str;
    return TEMPLATE.content;
  }

When using a <template> element in a chrome-privileged document, we hit sanitizer code which prevents us from using "random" elements in the innerHTML for the template.

For instance I had defined:

serviceworker-list-aboutdebugging = Desc. <openabout>Link text</openabout> 

Which was used as follows in my component

Localized(
  {
    id: "serviceworker-list-aboutdebugging",
    openabout: a({onClick: () => doClick()})
  },
  footer({}, "Desc. <openabout>Link text</openabout>")
)

But the <openabout> was not transformed into a link and the browser console logged "Flattening unsafe node (descendants are preserved). openabout", which comes from https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/base/nsTreeSanitizer.cpp#1453

If, instead of <openabout>, I use any known HTML tag (p, div, etc...) then the parsing and templating works properly. Do you use alternate approaches to parse this kind of templates in the non-react variants of fluent?
Interesting. There's a mismatch between html and react here as well.

Andrei is hitting a related problems with his snippets-in-activity-stream work, where I'm hoping to use an approach that is closer to what fluent-dom does when it comes down to white-listing presentational html.
The vendor patch needs documentation and an update to our licensing file at https://searchfox.org/mozilla-central/source/toolkit/content/license.html.

For now let's focus on the second patch and the usage of fluent-react in the devtools panel.

(clearing ni? since the issue has been solved)
Flags: needinfo?(stas)
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

I cleaned up the patch a bit, and could use some feedback.

Some comments: 
- as mentioned in a previous comment I had to use a "real" tag name in my templates (which is why I use <a></a> in several places in my ftl file)
- one of my links (Debug) only shows a title when it is disabled. I found this part a bit weird to implement, I only define `attrs: { title: true }` when the link is disabled, but I'm not sure how that would work if the enabled link also had a title

Thanks!
Attachment #8981348 - Flags: feedback?(gandalf)
Attachment #8981348 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review253486

::: devtools/client/locales/en-US/application.ftl:27
(Diff revision 3)
> +
> +# Text for the debug link displayed for an already started Service Worker. Clicking on the
> +# link opens a new devtools toolbox for this service worker. The title attribute is only
> +# displayed when the link is disabled.
> +serviceworker-worker-debug = Debug
> +                    .title = Only running service workers can be debugged

Only one tab indentation on attributes, no alignment on the equal sign ;-)

::: devtools/client/locales/en-US/application.ftl:55
(Diff revision 3)
> +# started or debugged.
> +serviceworker-worker-status-registering = Registering
> +
> +# Text displayed when no service workers are visible for the current page. Clicking on the
> +# link will open https://developer-mozilla-org/docs/Web/API/Service_Worker_API/Using_Service_Workers
> +serviceworker-empty-intro = You need to register a Service Worker to inspect it here.<a>Learn more</a>

Missing a space before the "learn more" link

::: devtools/client/locales/en-US/application.ftl:69
(Diff revision 3)
> +
> +# Suggestion to use the debugger to investigate why a Service Worker registration did not complete successfully.
> +# Clicking on the link will switch from the Application panel to the debugger.
> +serviceworker-empty-suggestions-debugger = Step through your Service Worker registration and look for exceptions. <a>Open the Debugger</a>
> +
> +# LOCALIZATION NOTE (serviceworker-empty-suggestions-aboutdebugging): Suggestion to go to

No need for the LOCALIZATION NOTE part
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

The FTL file looks definitely good, only a couple of nits to fix.
Attachment #8981348 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review253508

I'm happy that you were able to work around the TreeSanitizer by using the "a" elements. I think it's a valid solution here. At the same time, it won't scale well if there are multiple elements which localizations might want to reorder. I'll try to find a better solution for the future.

::: devtools/client/application/src/components/App.js:57
(Diff revision 3)
>  
>  // Exports
>  
>  module.exports = connect(
>    (state) => ({ workers: state.workers.list, domain: state.page.domain }),
> -)(App);
> +)(withLocalization(App));

`withLocalization` is only useful if you need to use the imperative `getString` API. See https://github.com/projectfluent/fluent.js/wiki/withLocalization. I think in this case the `LocalizationProvider` should be enough, I think.

::: devtools/client/application/src/components/Worker.js:130
(Diff revision 3)
>  
>      const debugLinkDisabled = this.isRunning() ? "" :
>                                                   "worker__debug-link--disabled disabled";
> -    const debugLink = a({
> +    const debugLink = Localized({
> +      id: "serviceworker-worker-debug",
> +      attrs: this.isRunning() ? null : { title: true }

Could this be written as `attrs: {title: !this.isRunning}`?
Thanks for the feedback! I implemented the suggested changes here. 

I removed the import of withLocalization but I'm glad to see we still have access to getString if we have a use case that gets too complex for <Localized>. I am almost tempted to use it for the title of the disabled Debug link here.

Some comments about the first patch to include react-fluent:

- As discussed if we can extract CachedIterable, mapContextSync and getContextForId to a separate package that would be great. And if we can bundle it in fluent-react.js at build time, even better. For what it's worth, I tried to reduce fluent.js to contain only CachedIterable, mapContextSync and getContextForId, and everything still works fine.

- For now I am still manually changing the require() at the beginning of fluent-react.js, but I opened a DevTools RFC so that require("react") could work with the DevTools loader.
Starting review requests for the second patch, the first patch is blocked on extracting CachedIterable and others out of fluent.js to avoid vendoring fluent.js in m-c.
Note that I haven't implemented anything related to language switching in this version, I was hoping to handle that in a second step.
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review253994

::: devtools/client/application/src/components/Worker.js:149
(Diff revision 5)
> +          onClick: this.start,
> +          className: "worker__start-link"
> +        })
> +      ) : null;
>  
>      const lastUpdated = worker.lastUpdateTime

Shouldn't this be wrapped in `Localized` too?
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review253994

Thanks for the patch! I'm not familiar with the library, so I'm not sure if everything's alright –besides an element which was not wrapped, the rest it seemed so!
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review254000

::: devtools/client/locales/en-US/application.ftl:69
(Diff revision 5)
> +
> +# Suggestion to use the debugger to investigate why a service worker is not registered.
> +# Clicking on the link will switch from the Application panel to the debugger.
> +serviceworker-empty-suggestions-debugger = Step through your Service Worker registration and look for exceptions. <a>Open the Debugger</a>
> +
> +# Suggestion to go to # about:debugging in order to see Service Workers for all domains.

There's an extra # before about:debugging
Attachment #8981348 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review253994

Thanks for the review! I'm expecting :gandalf to review the usage of the library in details, so as long as you are ok with the impact on the existing code for the Application panel, we should be good :)

> Shouldn't this be wrapped in `Localized` too?

Ah good catch! This string was added in my last rebase and didn't notice it. Will fix this.
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

I'm switching my r? to f+ because the patch looks good, but since it's the initial landing of fluent-react use, I'd prefer :stas (who wrote fluent-react) to do the review from the fluent side.
Attachment #8981348 - Flags: review?(stas)
Attachment #8981348 - Flags: review?(gandalf)
Attachment #8981348 - Flags: feedback+
Comment on attachment 8968256 [details]
Bug 1450071 - Vendor fluent-react in devtools/client/shared;

https://reviewboard.mozilla.org/r/236924/#review254340

::: devtools/client/application/src/components/Worker.js:157
(Diff revision 7)
> +          id: "serviceworker-worker-updated",
> +          // XXX: $date should normally be a Date object, but we pass the timestamp as a
> +          // workaround. See Bug 1465718. worker.lastUpdateTime is in microseconds,
> +          // convert to a valid timestamp in milliseconds by dividing by 1000.
> +          "$date" : worker.lastUpdateTime / 1000,
> +          time: time({ className: "js-sw-updated" })

Q: Since `$date` contains a `<time>` tag in the localization file, will it get "merged" with the `<time>` tag that is here and has classes applied?
(In reply to Belén [:ladybenko] from comment #31)
> Comment on attachment 8968256 [details]
> Bug 1450071 - Vendor fluent-react in devtools/client/shared
> 
> https://reviewboard.mozilla.org/r/236924/#review254340
> 
> ::: devtools/client/application/src/components/Worker.js:157
> (Diff revision 7)
> > +          id: "serviceworker-worker-updated",
> > +          // XXX: $date should normally be a Date object, but we pass the timestamp as a
> > +          // workaround. See Bug 1465718. worker.lastUpdateTime is in microseconds,
> > +          // convert to a valid timestamp in milliseconds by dividing by 1000.
> > +          "$date" : worker.lastUpdateTime / 1000,
> > +          time: time({ className: "js-sw-updated" })
> 
> Q: Since `$date` contains a `<time>` tag in the localization file, will it
> get "merged" with the `<time>` tag that is here and has classes applied?

Short answer yes. The "<time>{ DATETIME ... }</time>" from the localized string will be replaced by the time element we create here, with all the classnames and attributes we set. But the text content of <time> will be the localized content (so {DATETIME ...}).

I didn't find a clear example in the Fluent documentation at https://projectfluent.org/fluent/guide/, but the following example relies on this pattern heavily: https://github.com/projectfluent/fluent.js/tree/master/fluent-react/examples/html-markup

Note I also used this approach for several <a> elements in this patch if you want to look for other examples.

Sidenote: normally we should be able to use more descriptive names in the localized string, such as <update-time> and then in React do "update-time": time({ className: "..." }), but there is a technical limitation (sanitizer) which forces to use known HTML tags at the moment.
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

Francesco: I added a new string, with some attempt to use multiline in the FTL file, I'd prefer to get an extra check from you.

# Text displayed for the updated time of the service worker. The <time> element will
# display the last update time of the service worker script.
serviceworker-worker-updated =
  Updated
  <time>{ DATETIME($date, month: "long", year: "numeric", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric") }</time>
Attachment #8981348 - Flags: review+ → review?(francesco.lodolo)
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review254356

::: devtools/client/locales/en-US/application.ftl:37
(Diff revision 7)
> +
> +# Text displayed for the updated time of the service worker. The <time> element will
> +# display the last update time of the service worker script.
> +serviceworker-worker-updated =
> +  Updated
> +  <time>{ DATETIME($date, month: "long", year: "numeric", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric") }</time>

Let's avoid multiline for now. There's a long ongoing discussion about formalizing how Fluent behaves in these cases, and having it all on one line would make things safer.
(In reply to Francesco Lodolo [:flod] from comment #35)
> Comment on attachment 8981348 [details]
> Bug 1450071 - Use fluent-react to localize application panel;
> 
> https://reviewboard.mozilla.org/r/247486/#review254356
> 
> ::: devtools/client/locales/en-US/application.ftl:37
> (Diff revision 7)
> > +
> > +# Text displayed for the updated time of the service worker. The <time> element will
> > +# display the last update time of the service worker script.
> > +serviceworker-worker-updated =
> > +  Updated
> > +  <time>{ DATETIME($date, month: "long", year: "numeric", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric") }</time>
> 
> Let's avoid multiline for now. There's a long ongoing discussion about
> formalizing how Fluent behaves in these cases, and having it all on one line
> would make things safer.

Thanks for checking, put everything back on a single line.
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review254360
Attachment #8981348 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review254384
Attachment #8981348 - Flags: review?(balbeza) → review+
I would like to move forward and land the current version if possible, even though the vendored fluent.js file is just a temporary thing. fluent.js is not that big, so I think we can accept to have it temporarily vendored in devtools/client/shared.
Comment on attachment 8968256 [details]
Bug 1450071 - Vendor fluent-react in devtools/client/shared;

https://reviewboard.mozilla.org/r/236926/#review254414

::: devtools/client/shared/vendor/FLUENT_REACT_UPGRADING:13
(Diff revision 5)
> +1. git clone https://github.com/projectfluent/fluent.js - clone the repo
> +2. checkout the tag for the version to update
> +3. build bundles:
> +  3.1. npm install
> +  3.2. make deps
> +  3.3. make

Nit: you may want to only build fluent-react so that it takes less time:

    make -C fluent-react deps
    make -C fluent-react
Attachment #8968256 - Flags: review?(stas) → review+
Comment on attachment 8981348 [details]
Bug 1450071 - Use fluent-react to localize application panel;

https://reviewboard.mozilla.org/r/247486/#review254434

Nice! Thanks again for blazing the path for using fluent-react with the L10nRegistry!

::: devtools/client/application/src/components/Worker.js:156
(Diff revision 9)
> +        {
> +          id: "serviceworker-worker-updated",
> +          // XXX: $date should normally be a Date object, but we pass the timestamp as a
> +          // workaround. See Bug 1465718. worker.lastUpdateTime is in microseconds,
> +          // convert to a valid timestamp in milliseconds by dividing by 1000.
> +          "$date" : worker.lastUpdateTime / 1000,

There's one more way to fix this but I don't think it'll work now. I filed bug 1465797 to fix the root problem. When it's fixed perhaps we could revisit this part of the code?

Fluent should expose the `MessageDateTimeArgument` class which in fact is the class whose instances are returned by the `DATETIME()` function available inside of FTL.

It should be possible to define $date like this:

    "$date": new Fluent.MessageDateTimeArgument(dateObjectOrTimestamp, {
        month: "long",
        year: "numeric",
        // ...
    })
    
This is called "partial arguments", where some formatting is already pre-defined by the developer. Localizers may choose to override some options, or to leave the defaults.
Attachment #8981348 - Flags: review?(stas) → review+
Comment on attachment 8968256 [details]
Bug 1450071 - Vendor fluent-react in devtools/client/shared;

https://reviewboard.mozilla.org/r/236926/#review254512

Looks reasonable to me!  With require RFC[1] in motion, perhaps you'll change this to bare "react" etc. later, but that can happen in the future.

[1]: https://github.com/devtools-html/rfcs/issues/50

::: devtools/client/shared/vendor/FLUENT_REACT_UPGRADING:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I don't think this text file needs a license header...?

::: devtools/client/shared/vendor/FLUENT_REACT_UPGRADING:5
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +

Probably only one blank line is needed...?
Attachment #8968256 - Flags: review?(jryans) → review+
(In reply to Staś Małolepszy :stas from comment #44)
> Comment on attachment 8981348 [details]
> Bug 1450071 - Use fluent-react to localize application panel;
> 
> https://reviewboard.mozilla.org/r/247486/#review254434
> 
> Nice! Thanks again for blazing the path for using fluent-react with the
> L10nRegistry!
> 

Thanks for the support! The integration was pretty smooth.

> ::: devtools/client/application/src/components/Worker.js:156
> (Diff revision 9)
> > +        {
> > +          id: "serviceworker-worker-updated",
> > +          // XXX: $date should normally be a Date object, but we pass the timestamp as a
> > +          // workaround. See Bug 1465718. worker.lastUpdateTime is in microseconds,
> > +          // convert to a valid timestamp in milliseconds by dividing by 1000.
> > +          "$date" : worker.lastUpdateTime / 1000,
> 
> There's one more way to fix this but I don't think it'll work now. I filed
> bug 1465797 to fix the root problem. When it's fixed perhaps we could
> revisit this part of the code?
> 
> Fluent should expose the `MessageDateTimeArgument` class which in fact is
> the class whose instances are returned by the `DATETIME()` function
> available inside of FTL.
> 
> It should be possible to define $date like this:
> 
>     "$date": new Fluent.MessageDateTimeArgument(dateObjectOrTimestamp, {
>         month: "long",
>         year: "numeric",
>         // ...
>     })
>     
> This is called "partial arguments", where some formatting is already
> pre-defined by the developer. Localizers may choose to override some
> options, or to leave the defaults.

Thanks for the info, happy to revisit this once Bug 1465797 is fixed.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #45)
> Comment on attachment 8968256 [details]
> Bug 1450071 - Vendor fluent-react in devtools/client/shared;
> 
> https://reviewboard.mozilla.org/r/236926/#review254512
> 
> Looks reasonable to me!  With require RFC[1] in motion, perhaps you'll
> change this to bare "react" etc. later, but that can happen in the future.
> 
> [1]: https://github.com/devtools-html/rfcs/issues/50
> 

Yes! As soon as the RFC is accepted and implemented, I will update this.

> ::: devtools/client/shared/vendor/FLUENT_REACT_UPGRADING:1
> (Diff revision 5)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> I don't think this text file needs a license header...?
> 

Felt weird to me as well, but I initially added this because all the other "upgrading" files in VENDOR had a license header.
Removed it now.

Try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb89a6880d02a5542fd8d1b188cccff22569c4e7, landing.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 69e26567a5ca6d7f90d969543dca4777554bb762 -d dfc6d3905891: rebasing 466409:69e26567a5ca "Bug 1450071 - Vendor fluent-react in devtools/client/shared;r=jryans,stas"
rebasing 466410:b221f5bc7c7c "Bug 1450071 - Use fluent-react to localize application panel;r=flod,ladybenko,stas" (tip)
merging devtools/client/application/initializer.js
merging devtools/client/application/src/components/App.js
merging devtools/client/application/src/components/WorkerList.js
merging devtools/client/application/src/components/WorkerListEmpty.js
warning: conflicts while merging devtools/client/application/src/components/App.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35ca05d04f7e
Vendor fluent-react in devtools/client/shared;r=jryans,stas
https://hg.mozilla.org/integration/autoland/rev/d6ecdb3a9806
Use fluent-react to localize application panel;r=flod,ladybenko,stas
https://hg.mozilla.org/mozilla-central/rev/35ca05d04f7e
https://hg.mozilla.org/mozilla-central/rev/d6ecdb3a9806
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1466400
Depends on: 1466744
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.