localization: migrate inspector.xul to use properties file instead of dtd

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fix-optional, firefox52 fixed)

Details

(Whiteboard: [devtools-html])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

For devtools.html, XHTML files will no longer be able to load entities from external DTD files. All XHTML/XUL files relying on DTDs for localization should move all the DTD entities to properties files.

This bug is about migrating all localization strings defined in inspector.xul.
Whiteboard: [devtools-html] [triage]

Updated

2 years ago
Blocks: 1290947, 1263750
Flags: qe-verify?
Depends on: 1260552
Flags: qe-verify? → qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

Some of our XUL/XHTML files have a lot of localized strings coming from DTDs. I feel a better approach for migrating those is to use something similar to what l20n.js is doing: add specific attributes in the markup and use those to localize in JS.

I added a simple helper in l10n.js which does just that. It's not intended to be a long term / bulletproof solution, but I think it can bridge the gap until we wait for l20n.js, without having to add too much code in all panels. 

Let me know what you think (didn't add unit tests for this yet because I'd prefer to get your feedback first). 

FWIW, performance-wise localizing the inspector takes ~7ms on my machine if the properties files are not loaded yet. If they are this goes down to < 3 ms.
Attachment #8784777 - Flags: feedback?(bgrinstead)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1

Comment 5

a year ago
mozreview-review
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review72062

General thoughts:
* I'm OK with building a solution like this that lets us keep things defined in markup
* Maybe use '=' instead of ':' to delimit attribute names / values
* I think I'd rather have content be less special cased, and look something like: `<foo data-localization="content:showAllFontsUsed;title:showAllFonts"></foo>` instead of `<foo data-localization="content;title:showAllFonts">{showAllFontsUsed}</foo>`.  I realize this will require some rearranging in the case of `<html:p class="font-css">&usedAs; "<html:span class="font-css-name"></html:span>"</html:p>`, but we'll need to do the same ultimately for l20n: https://github.com/l20n/l20n.js/blob/master/docs/html.md
* Apparently annotating attributes in l20n will be quite different and require the attributes to be stored by the strings: http://l20n.org/learn/html-attributes.  Don't think we should try to duplicate that logic though
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

See Comment 5
Attachment #8784777 - Flags: feedback?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8784778 [details]
Bug 1294186 - Migrate inspector.xul from DTDs to .properties files;

https://reviewboard.mozilla.org/r/74106/#review73150

::: devtools/client/locales/en-US/layoutview.properties:6
(Diff revision 2)
> +# The Layout View is the panel accessible at the bottom of the Inspector
> +# sidebar.

You might want to change this comment. First of all, why "at the bottom", and secondly, now that the layout-view is inside the computed-view, this comment should reflect this.
Attachment #8784778 - Flags: review?(pbrosset) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8784776 [details]
Bug 1294186 - Add memoized getter for properties files in l10n.js;

https://reviewboard.mozilla.org/r/74102/#review73182
Attachment #8784776 - Flags: review?(bgrinstead) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review73184

::: devtools/shared/l10n.js:164
(Diff revision 2)
> +
> +    let attributes = element.getAttribute("data-localization").split(";");
> +    for (let attribute of attributes) {
> +      let [name, value] = attribute.trim().split("=");
> +      if (name === "content") {
> +        element.innerHTML = properties[value];

As discussed, let's use textContent instead

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:10
(Diff revision 2)
> +
> +// Tests that the markup localization works properly.
> +
> +const { localizeMarkup, LocalizationHelper } = require("devtools/shared/l10n");
> +
> +function test() {

Use add_task instead

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:55
(Diff revision 2)
> +  is(div3.getAttribute("title"), str2, "The title of #d3 is localized");
> +  is(div4.innerHTML, "Some content", "The content of #d4 is not replaced");
> +  is(div4.getAttribute("aria-label"), str1, "The aria-label of #d4 is localized");
> +  is(div5.innerHTML, str3, "The content of #d5 is localized with another bundle");
> +
> +  finish();

No need for finish() with add_task
Attachment #8784777 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8784777 [details]
Bug 1294186 - Create localization helper for markup using data-localization attributes;

https://reviewboard.mozilla.org/r/74104/#review73394
Attachment #8784777 - Flags: review?(bgrinstead) → review+
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Rebased and pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0974881a027c

Will probably land parts 1 & 2 if try is green and leave-open. Would like to have some progress on the dtd->properties migration script before landing part 3 (Bug 1295157).
Try is green landing part 1 & 2.
Keywords: leave-open

Comment 22

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8afcd614f50e
Add memoized getter for properties files in l10n.js;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/cba87d584348
Create localization helper for markup using data-localization attributes;r=bgrins
Depends on: 1295157
Blocks: 1291049
Comment hidden (mozreview-request)
Attachment #8784776 - Attachment is obsolete: true
Attachment #8784777 - Attachment is obsolete: true
Depends on: 1302496
Created attachment 8791898 [details] [diff] [review]
bug1294186.patch

I rebased the patch on top of changes introduced in bug 1260552
(I needed it for bug 1262443)

Please verify before landing.

Honza
Comment hidden (mozreview-request)
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Rebased and updated after 1302496 landed.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a90c8a62db
No longer depends on: 1295157
Comment hidden (mozreview-request)
Got a permafail on linux32debug dt6, but I have the same on fx-team [1], so not related to this bug. Landing.

[1] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=675331e308773c07e5b39cafd92ec1f4f591d152&selectedJob=11677272
Keywords: leave-open

Comment 30

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/41f5b98f7b55
Migrate inspector.xul from DTDs to .properties files;r=pbro

Comment 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41f5b98f7b55
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
status-firefox51: affected → fix-optional

Updated

a year ago
See Also: → bug 1308503
You need to log in before you can comment on or make changes to this bug.