942594 - Overlaying nodes doesn't work in IE, Safari & Opera Presto

Status: RESOLVED FIXED

(L20n :: JS Library, defect)

Description

[Michał Gołębiowski-Owczarek [:mgol]]

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... :/

Comment 1

[Staś Małolepszy :stas]

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.

Comment 2

[Michał Gołębiowski-Owczarek [:mgol]]

(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.

Comment 3

[Michał Gołębiowski-Owczarek [:mgol]]

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.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 6

[Michał Gołębiowski-Owczarek [:mgol]]

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.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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.

Comment 8

[Michał Gołębiowski-Owczarek [:mgol]]

(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. :)

Comment 9

[Michał Gołębiowski-Owczarek [:mgol]]

Stas, Gandalf, any thoughts about my last comment?

Comment 10

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

Comment 11

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

Comment 12

[Staś Małolepszy :stas]

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?

Comment 13

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

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

Comment 14

[Michał Gołębiowski-Owczarek [:mgol]]

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

Comment 15

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

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

Comment 16

[Michał Gołębiowski-Owczarek [:mgol]]

Comment on attachment 8543276 [details] [diff] [review]
template.patch

Obsoleted by https://bug942594.bugzilla.mozilla.org/attachment.cgi?id=8543280

Comment 17

[Michał Gołębiowski-Owczarek [:mgol]]

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

Comment 18

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

OK, it should be good now. :)

Comment 19

[Michał Gołębiowski-Owczarek [:mgol]]

(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?

Comment 20

[Staś Małolepszy :stas]

(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.

Comment 21

[Michał Gołębiowski-Owczarek [:mgol]]

(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.

Comment 22

[Michał Gołębiowski-Owczarek [:mgol]]

Attachment: template.patch

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

Comment 23

[Staś Małolepszy :stas]

Comment on attachment 8543286 [details] [diff] [review]
template.patch

Review of attachment 8543286 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Michał.  r=me.

Comment 24

[Staś Małolepszy :stas]

Comment on attachment 8543286 [details] [diff] [review]
template.patch

Zibi, are you okay with L20n.shims here?

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Is there a way to get a polyfill that would not require shims and special codepaths?

Like, overload createElement to special case 'template'?

Comment 26

[Michał Gołębiowski-Owczarek [:mgol]]

(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 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :(

Comment 28

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/738455a4b6b61dad8499c73c3fbb81d38d2c5d50