Support localization / i18n message in CSS

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
13 days ago

People

(Reporter: callahad, Assigned: kmag, Mentored)

Tracking

(Depends on: 1 bug, {dev-doc-complete, DevAdvocacy})

unspecified
mozilla45
dev-doc-complete, DevAdvocacy
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [i18n])

MozReview Requests

()

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

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
The chrome docs for chrome.i18n suggest using @@extension_id to reference files within an extension, e.g.:

    body {
        background-image:url('chrome-extension://__MSG_@@extension_id__/background.png');
    }

While we support @@extension_id in chrome.i18n.getMessage(), we do not currently translate it in CSS. This limitation is documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#i18n

Without support for automatically replacing __MSG_@@extension_id__ in injected CSS, add-on authors can't reference bundled images from their CSS.

Tests that make use of this are available at https://github.com/callahad/webextension-tests/tree/master/background_image

For this bug to be of practical use, we also need to resolve Bug 1208763 and Bug 1208756
(Reporter)

Updated

2 years ago
Depends on: 1208756

Updated

2 years ago
Whiteboard: [i18n]
(Assignee)

Comment 1

2 years ago
This should be implemented as a simple stream filter on .css files in the moz-extension: protocol, using the Extension.localize method.

I'm willing to mentor anyone who wants to work on this.

Incidentally, I think that @@extension_id is the least interesting use for this. I'd rather extension CSS files reference images using relative URLs rather than hacks like this.
Mentor: kmaglione+bmo@mozilla.com
Whiteboard: [i18n] → [i18n][good first bug]
Blocks: 1214433
Priority: -- → P1
Summary: Support the @@extension_id localization / i18n message in CSS → Support localization / i18n message in CSS
(Assignee)

Comment 2

2 years ago
Documented here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#Localized_String_Interpolation

Be sure to update once this is implemented.
(Assignee)

Comment 3

2 years ago
On second thought, this probably isn't a good first bug. The stream filter portion is, but integrating it with the protocol handler will be a bit tricky.
Assignee: nobody → kmaglione+bmo
Whiteboard: [i18n][good first bug] → [i18n]

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Iteration: --- → 45.2 - Nov 30
(Assignee)

Updated

2 years ago
Blocks: 1208761
(Assignee)

Comment 4

2 years ago
Created attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

Bug 1209184: Part 1a - [webext] Make localization work in content processes. r?billm
Attachment #8689176 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

2 years ago
Created attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r?billm
Attachment #8689177 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

2 years ago
Created attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r?billm
Attachment #8689178 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

2 years ago
Created attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r?billm
Attachment #8689179 - Flags: review?(wmccloskey)
(Assignee)

Comment 8

2 years ago
Created attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r?billm
Attachment #8689180 - Flags: review?(wmccloskey)
(Assignee)

Comment 9

2 years ago
(In reply to Kris Maglione [:kmag] from comment #4)
> Created attachment 8689176 [details]
> MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in
> content processes. r?billm
> 
> Bug 1209184: Part 1a - [webext] Make localization work in content processes.
> r?billm

I didn't realize this was going to be necessary until I ran e10s tests, or I would have done it as a separate bug. I was hoping ReviewBoard would mark the ExtensionData move as an unchanged code move, but alas... In any case, there are no changes to the moved code.
Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

https://reviewboard.mozilla.org/r/25563/#review23225

I think this would be nicer if we didn't have to move so much code to ExtensionUtils. Most of it will never be used by the content process. As we do more schema checking of the manifest, I worry it will get more awkward if this code doesn't stay in Extension.jsm. Could we just move localize/localizeMessage to ExtensionUtils and pass the data they need as parameters?

::: toolkit/components/extensions/ExtensionUtils.jsm:210
(Diff revision 1)
> +      return Locale.getLocale();

Just noticed this: we need this to be a Chrome-style locale. And we might want to use selectedLocale here, though I'm not sure.
Attachment #8689176 - Flags: review?(wmccloskey)
Attachment #8689177 - Flags: review?(wmccloskey) → review+
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23227

::: toolkit/components/utils/simpleServices.js:175
(Diff revision 1)
> +  getAddonId(aContext) {

It might be nice to comment that we expect aContext to be a nsIURI.

::: toolkit/components/utils/simpleServices.js:195
(Diff revision 1)
> +  convert(aStream, aFromType, aToType, aContext) {

Can we throw an exception if the from/to types aren't as we expect? Same for the async version.
Attachment #8689177 - Flags: review+
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23229

::: toolkit/components/utils/simpleServices.js:165
(Diff revision 1)
> -this.NSGetFactory = XPCOMUtils.generateNSGetFactory([RemoteTagServiceService, AddonPolicyService]);
> +function AddonLocalizationConverter()

Can you also refer people to the code in ExtensionProtocolHandler and say that these two are closely tied together?
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Reviewboard canceled my r+ for some reason. I guess I shouldn't have commented twice.
Attachment #8689177 - Flags: review+
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

https://reviewboard.mozilla.org/r/25567/#review23231
Attachment #8689178 - Flags: review?(wmccloskey) → review+
Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://reviewboard.mozilla.org/r/25569/#review23233
Attachment #8689179 - Flags: review?(wmccloskey) → review+
Attachment #8689180 - Flags: review?(wmccloskey) → review+
Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

https://reviewboard.mozilla.org/r/25571/#review23235
(Assignee)

Comment 17

2 years ago
(In reply to Bill McCloskey (:billm) from comment #10)
> Just noticed this: we need this to be a Chrome-style locale. And we might
> want to use selectedLocale here, though I'm not sure.

Chromium appears to use the browser locale.

(In reply to Bill McCloskey (:billm) from comment #11)
> ::: toolkit/components/utils/simpleServices.js:195
> (Diff revision 1)
> > +  convert(aStream, aFromType, aToType, aContext) {
> 
> Can we throw an exception if the from/to types aren't as we expect? Same for
> the async version.

I guess. I thought about checking initially, but I couldn't think of any reason it should matter.
(Assignee)

Comment 18

2 years ago
Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25563/diff/1-2/
Attachment #8689176 - Flags: review?(wmccloskey)
(Assignee)

Comment 19

2 years ago
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25565/diff/1-2/
Attachment #8689177 - Attachment description: MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r?billm → MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm
Attachment #8689177 - Flags: review+ → review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8689178 - Attachment description: MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r?billm → MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm
(Assignee)

Comment 20

2 years ago
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25567/diff/1-2/
(Assignee)

Comment 21

2 years ago
Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25569/diff/1-2/
Attachment #8689179 - Attachment description: MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r?billm → MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm
(Assignee)

Updated

2 years ago
Attachment #8689180 - Attachment description: MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r?billm → MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm
(Assignee)

Comment 22

2 years ago
Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25571/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8689178 - Flags: review+ → review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8689177 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 23

2 years ago
https://reviewboard.mozilla.org/r/25567/#review23231

Re-requesting review on this one, since I realized the pipe was being held open until the JS wrapper for the listener was GCed. I just added a callback to close it when the request completes.
(Assignee)

Comment 24

2 years ago
https://reviewboard.mozilla.org/r/25563/#review23225

> Just noticed this: we need this to be a Chrome-style locale. And we might want to use selectedLocale here, though I'm not sure.

Since I was fixing this anyway, I also fixed the bidi strings. I was going to do it in another bug, but they're arguably the main use case for CSS localization.
Attachment #8689176 - Flags: review?(wmccloskey) → review+
Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

https://reviewboard.mozilla.org/r/25563/#review23251

This looks great. Thanks!

::: toolkit/components/extensions/Extension.jsm:351
(Diff revision 2)
> -  // Map(locale-name -> Map(message-key -> localized-strings))
> +  this.locales = null;

I think it would be clearer to call this localeData.
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://reviewboard.mozilla.org/r/25565/#review23253
Attachment #8689177 - Flags: review+
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Someone who understand the networking code better should look at this. Josh, do you mind?
Attachment #8689178 - Flags: review?(wmccloskey) → review?(josh)
(Assignee)

Comment 28

2 years ago
Comment on attachment 8689176 [details]
MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25563/diff/2-3/
Attachment #8689176 - Attachment description: MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r?billm → MozReview Request: Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm
(Assignee)

Comment 29

2 years ago
Comment on attachment 8689177 [details]
MozReview Request: Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25565/diff/2-3/
Attachment #8689177 - Flags: review+
(Assignee)

Comment 30

2 years ago
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25567/diff/2-3/
Attachment #8689178 - Attachment description: MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm → MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm
Attachment #8689178 - Flags: review?(wmccloskey)
(Assignee)

Comment 31

2 years ago
Comment on attachment 8689179 [details]
MozReview Request: Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25569/diff/2-3/
(Assignee)

Comment 32

2 years ago
Comment on attachment 8689180 [details]
MozReview Request: Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25571/diff/2-3/
(Assignee)

Comment 33

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/eb7962bf88a8517d92c21263f3982fe95ff80eb4
Bug 1209184: Part 1a - [webext] Make localization work in content processes. r=billm
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 34

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb7962bf88a8
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

Josh's review is sufficient.
Attachment #8689178 - Flags: review?(wmccloskey)

Updated

2 years ago
Attachment #8689178 - Flags: review?(josh) → review+
Comment on attachment 8689178 [details]
MozReview Request: Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=billm r?jdm

https://reviewboard.mozilla.org/r/25567/#review23809

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:51
(Diff revision 3)
> +  PipeCloser(nsIOutputStream* aOutputStream) :

explicit

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:63
(Diff revision 3)
> +    return mOutputStream->Close();

Let's null out the pointer here, too.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:88
(Diff revision 3)
> +  if (ext.LowerCaseEqualsLiteral("css")) {

Could we invert this condition and return early instead?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:141
(Diff revision 3)
> +  return rv;

Let's return NS_OK explicitly.
(Assignee)

Comment 37

2 years ago
https://reviewboard.mozilla.org/r/25567/#review23809

Thanks

> Could we invert this condition and return early instead?

I guess we can. I went with the `if` block in case we wanted to add more substitutions later, but that's probably not very likely.
(Assignee)

Comment 38

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e75f9f24d0dc7c08131ebc08b0dcfcb4f310269c
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/9c63ffd499ebc4ed4f164d4b35e09a7a03c36387
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/f9ab766896106b5718920a274ef4fe3605823b75
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/8e692344588a9b0155259d0d3fb1050b5f22230c
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm
(Assignee)

Updated

2 years ago
Keywords: leave-open
Backed out in https://hg.mozilla.org/integration/fx-team/rev/2e446ebafe6c - I don't see why this would be the case, but that broke Android and B2G xpcshell, with a couple of "KeyError: 'tail'" exceptions, https://treeherder.mozilla.org/logviewer.html#?job_id=5951327&repo=fx-team
(Assignee)

Comment 40

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a4f1765a6cdf7bca6ed2d22788be9e03e6ed8c24
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/a517959befe9b9e426380771c8712e568bb62832
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/399404ff25e40003c620cd1d6193790127a5c121
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/24282235336dd3dd3c725bcd6e8025dcf9fe0fb4
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Comment 41

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/d0404928473e
Rebacked out in https://hg.mozilla.org/integration/fx-team/rev/cbf641f8da0a because I'm pretty sure this is what's causing bug 1228742.

Updated

2 years ago
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
(Assignee)

Comment 43

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/62ff0d59cd62619bede132718cf524f2a142646d
Bug 1209184: Part 1b - [webext] Create a stream converter for localization placeholders. r=billm

https://hg.mozilla.org/integration/fx-team/rev/ceef1da728bf078fe100f35d80ff237ac7f3b7ba
Bug 1209184: Part 2 - [webext] Localize CSS files served from moz-extension: channels. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/b8fe968c1775a69b166b5c4fae1684d06efec74a
Bug 1209184: Part 3 - [webext] Add tests for locale stream converter. r=billm

https://hg.mozilla.org/integration/fx-team/rev/af59d2160097bfd49aefe71e0ee786111c7ea7a2
Bug 1209184: Part 4 - [webext] Add tests for CSS localization filters. r=billm

Comment 44

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/62ff0d59cd62
https://hg.mozilla.org/mozilla-central/rev/ceef1da728bf
https://hg.mozilla.org/mozilla-central/rev/b8fe968c1775
https://hg.mozilla.org/mozilla-central/rev/af59d2160097
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Keywords: dev-doc-needed
Updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities.
Keywords: dev-doc-needed → dev-doc-complete

Updated

13 days ago
Depends on: 1389099
You need to log in before you can comment on or make changes to this bug.