Ensure scripts provided by textformatter are run

[askvortsov1]

Fixes #2412

It seems that between Mithril 0.2 and Mithril 2.0, scripts contained within m.trust are no longer automatically evaluated. So, now we have to make them run ourselves. To do this, we "clone" the script node (using node.cloneNode() didn't work unfortunately), and replace the text formatter provided script node with our cloned one. This works for both external and inline scripts, and also allows us to get rid of eval.

comments for reviewers

If I'm understanding this correctly (pun intended), this in the <script> tag previously pointed to the script tag, now it acts as the default this in the global scope. The script tag is still available via document.currentScript. However, this was never available by the default use of the textformatter extension, so I believe we should be fine. Is this something we are concerned about?

[tankerkiller125]

I have not tested this yet, but the code looks good to me.

[luceos]

@clarkwinkelmann or @datitisev could either of you check this out locally and review, 🙏

[clarkwinkelmann]

I tested the PR and it looks good.

However in an effort to use the same strategy everywhere, I checked what TextFormatter does in preview. I think they don't actually have the same issue as ourselves, because the javascript is supposed to run fine with the way they insert their HTML. But they do have a snippet almost identical to ours to handle a bug in Chrome.

I actually quite like the differences in TextFormatter. replaceChild is used instead of inserting then removing a script tag. And they use textContent instead of innerText, which is the standard property name and might be a bit more performant, although I didn't find any explanation specific to script tags.

https://github.com/s9e/TextFormatter/blob/12bdf56b7fd3667290c5336cadb06a0c7720c7d3/src/render.js#L113-L129

So my suggestion would be to change the code as follows, which I have also tested working locally:

      this.$('.Post-body script').each(function () {
        const script = document.createElement('script');
        script.textContent = this.textContent;
        Array.from(this.attributes).forEach((attr) => script.setAttribute(attr.name, attr.value));
        this.parentNode.replaceChild(script, this);
      });

Regarding the this question, I have no idea. I've never used javascript in a TextFormatter template.

I would actually much prefer if we completely got rid of javascript in templates and force developers to use a frontend extension to add the javascript, but obviously this would be a question for another time.

[askvortsov1]

Done. I think the this issue isn't an issue: I don't know any situation where someone would expect this to refer to the script DOM node, and I don't know of a valid reason for supporting it.

[clarkwinkelmann]

I tested the last commit locally for good measure 👍