Overlaying nodes doesn't work in IE (checked on 10 & 11), Safari (the newest 7.0) & Opera Presto (checked on 12.16). This is because current l20n code for overlaying uses the template element which is only supported in Gecko and Blink so far. The problematic code is the lines 268-271 of the HTML bindings file: var translation = document.createElement('template'); translation.innerHTML = entity.value; // overlay the node with the DocumentFragment overlayElement(node, translation.content); located here: https://github.com/l20n/l20n.js/blob/a803a43e1e0b7db9a40b69600de7135daab7f237/bindings/l20n/html.js#L268-271 These kinds of bugs will happen as long as there's no automatic testing in real supported browsers... :/

This is intended. The template element gives us the required security by creating an inert DOM. For implementations adapted to browsers that don't support <template>, I think we should recommend using a polyfill (e.g. http://jsfiddle.net/brianblakely/h3EmY/). This also means lower security, so developers choosing this route will need to perform additional work and check translations to ensure they're safe.

(In reply to Staś Małolepszy :stas from comment #1) > This is intended. The template element gives us the required security by > creating an inert DOM. > > For implementations adapted to browsers that don't support <template>, I > think we should recommend using a polyfill (e.g. > http://jsfiddle.net/brianblakely/h3EmY/). This also means lower security, > so developers choosing this route will need to perform additional work and > check translations to ensure they're safe. I know the reasons. :) If <template> is used only in this one place, it might be better to use a script tag with custom type instead of <template> polyfill, it should be shorter.

BTW, template element has been recently implemented in WebKit nightly so it will be available in the next Safari version. We don't know what are IE plans and there's still Opera Presto so unless we have our own parsing mechanism we'll need a way to communicate that translations are not safe everywhere. But the nearby future looks good.

I'd consider Opera Presto to not be out target. For IE I'd like to find a polyfill that could help us here.

Wouldn't a script tag with a custom type work? That's how most SPA frameworks specify their templates. The content is inert (@stas, I've checked that; also, see http://www.html5rocks.com/en/tutorials/webcomponents/template/); the only problem is missing `.content` and having to use innerHTML for assignment. If we use <template> only in one place (it's currently only used once in each binding), polyfill might be an overkill, regular if-else should work.

(In reply to Michał Gołębiowski from comment #6) > If we use <template> only in one place (it's currently only used once in > each binding), polyfill might be an overkill, regular if-else should work. That's misleading. We use Template once because it is a whole, complex technology stack. It implicitly does a lot of what we need that otherwise we'd have to emulate. If I understand correctly it also provides a much increased security characteristics.

(In reply to Zibi Braniecki [:gandalf] from comment #7) > (In reply to Michał Gołębiowski from comment #6) > > If we use <template> only in one place (it's currently only used once in > > each binding), polyfill might be an overkill, regular if-else should work. > > That's misleading. We use Template once because it is a whole, complex > technology stack. It implicitly does a lot of what we need that otherwise > we'd have to emulate. If I understand correctly it also provides a much > increased security characteristics. Right, I simplified it too much. Basically it creates a DocumentFragment containing the contents that is associated with a different document. Script with a custom MIME type is inert as well but doesn't provide the structure and once we innerHTML it inside current document, we lose being inert. The question is - do we want to find a perfectly safe polyfill for <template> to make community translations secure even in browsers without <template> or is it enough to just fallback to an insecure implementation & add a note in the docs that overlaying nodes is not safe for user-provided translations in such browsers? If that solution was decided to be OK then we wouldn't even need this script tag, as the template element is used to access .content immediately after it's created: https://github.com/l20n/l20n.js/blob/986794bcd653b5b3c18a1a629852ec4c15612f22/bindings/l20n/html.js#L268-L271 so a regular detached div would work. If we do want to have perfect security in IE & Safari as well, then it gets more complicated. Perhaps a sandboxed iframe could work as a sort-of polyfill? Not perfectly inert but at least it wouldn't affect the main document. IE & Safari support sandboxed iframes; only Opera Presto doesn't but it was already decided support for this browser is out of scope. I hope I didn't say sth terribly stupid. :)

Stas, Gandalf, any thoughts about my last comment?

Comment on attachment 8543275 [details] [diff] [review] template.patch Review of attachment 8543275 [details] [diff] [review]: ----------------------------------------------------------------- ::: bindings/l20n/html.js @@ +257,5 @@ > if (!entity) { > return; > } > + if (entity.value || > + !templateSupported && typeof ctx.getTemplate !== 'function') { I think this check should be a bit lower in order to make textContent the ultimate fallback. var templateSupported = 'content' in document.createElement('template'); var canUseOverlay = templateSupported || L20n.getTemplate; if ((entity.value.indexOf('<') === -1 && entity.value.indexOf('&') === -1) || !canUseOverlay) { node.textContent = entity.value; } else ... I'd also expose the getTemplate function on L20n or even on L20n.shims? Gandalf, any thoughts?

Updated the description with info about the fallback to the regular translation.

Sorry, premature update. :) I'll address your comment in a moment.

Fixed putting the check in the wrong if... Oops!

Comment on attachment 8543276 [details] [diff] [review] template.patch Obsoleted by https://bug942594.bugzilla.mozilla.org/attachment.cgi?id=8543280

Comment on attachment 8543280 [details] [diff] [review] template.patch I'll update the description as well to write in more detail what should be returned via ctx.getTemplate

OK, it should be good now. :)

(In reply to Staś Małolepszy :stas from comment #12) > I'd also expose the getTemplate function on L20n or even on L20n.shims? > Gandalf, any thoughts? I didn't do this change yet but I don't see L20n.shims anywhere in the code; does it exist on the v1.0.x branch? Exposing on L20n does seem better as it's related to the environment, not the context. I guess I should've marked it as "feedback?", not "review?" when there are still remaining questions to answer?

(In reply to Michał Gołębiowski [:m_gol] from comment #19) > (In reply to Staś Małolepszy :stas from comment #12) > > I'd also expose the getTemplate function on L20n or even on L20n.shims? > > Gandalf, any thoughts? > > > I didn't do this change yet but I don't see L20n.shims anywhere in the code; > does it exist on the v1.0.x branch? No, we'd need to create it :) It might be worth it if we're planning to allow other monkey-patching on v1.x. > Exposing on L20n does seem better as it's related to the environment, not > the context. Agree. > I guess I should've marked it as "feedback?", not "review?" when there are > still remaining questions to answer? No worries, some questions pop up as the code is written or reviewed.

(In reply to Staś Małolepszy :stas from comment #20) > (In reply to Michał Gołębiowski [:m_gol] from comment #19) > > (In reply to Staś Małolepszy :stas from comment #12) > > I didn't do this change yet but I don't see L20n.shims anywhere in the code; > > does it exist on the v1.0.x branch? > > No, we'd need to create it :) It might be worth it if we're planning to > allow other monkey-patching on v1.x. Ah, right. :) I assume it should be created in the binding code in html.js since e.g. gaia won't need any shims.

1. The shim moved from ctx.getTemplate to L20n.shims.getTemplate 2. The documentation about it moved to docs/html.md

Comment on attachment 8543286 [details] [diff] [review] template.patch Review of attachment 8543286 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Michał. r=me.

Comment on attachment 8543286 [details] [diff] [review] template.patch Zibi, are you okay with L20n.shims here?

Is there a way to get a polyfill that would not require shims and special codepaths? Like, overload createElement to special case 'template'?

(In reply to Zibi Braniecki [:gandalf] from comment #25) > Is there a way to get a polyfill that would not require shims and special > codepaths? > > Like, overload createElement to special case 'template'? That's possible but this would create a performance penalty to *every* document.createElement invocation which is used very often in Single Page Apps so this wouldn't be ideal... Also, I'm a little afraid of modifying native DOM methods. I was also thinking about including it by default in my ng-l20n project (https://github.com/EE/ng-l20n) but this is a library and a library shouldn't overwrite core platform APIs so if this had to be done via monkey-patching document.createElement, I'd probably need to punt on it, at least by default.

Comment on attachment 8543286 [details] [diff] [review] template.patch Review of attachment 8543286 [details] [diff] [review]: ----------------------------------------------------------------- Yeah. Fair enough. Totally not happy about it, but I see no better way and I we can't ignore IE :(