😄 Emoji autocomplete and unicode emoji to image conversion using emojione.

[aviraldg]

(should have fixed) element-hq/element-web#1031, fixes element-hq/element-web#883, (should have fixed) element-hq/element-web#117

[ara4n]

what's going on with this _.escape()? I'm dubious about adding a new dependency just for an escaping function, and i'm failing to see why it's needed?

[aviraldg]

We're setting safeBody as HTML (so we can render emojis as images), but it is arbitrary content (since this was originally plain text) so it needs to be escaped. Lodash is generally useful and contains quite a few similar utilities, but yeah, we can probably drop the dependency.

[ara4n]

Firstly - sorry for having not managed to progress the review this week; I've been stuck doing mainly non-dev and the rest of the team has been elsewhere. Meanwhile, this is a really security-sensitive part of Vector (in terms of the risk of XSS if the escaping and dangerouslySetInnerHTMLs aren't handled correctly, hence wanting to review it fully first).

So, previously, the way this works originally is that TextHighlighter.applyHighlights() returns a list of React <span/> nodes which wrap text nodes of the original plain text of the event body (which isn't HTML or escaped), as per the method's javascript doc. React then automatically safely escapes these textnodes when rendering the spans to the DOM as normal, so they don't need to be escaped. It's no different to doing render() { foo='&amp;'; return <span>{ foo }</span>; } - the foo variable gets automatically escaped and you see a literal & string rendered by the browser.

To pose a different question to try to understand how this is meant to work: we seem to be taking the list of <span/>s returned by TextHighlighter.applyHighlights() and handing it straight to emojione.toImage(). How does this handle this? Is it somehow collapsing the React components back down to their raw string representations? If so, this is a no-go because it will remove any highlights we just added. On the other hand, if it's actually processing the React components correctly as components, they shouldn't need to be pre-escaped as the original logic should still work. And one should not be handing them to dangerouslySetInnerHTML, which takes an escaped string, rather than a list of React components.

So... did you envisage this working? :)

[aviraldg]

The only major difference here should be the handing of plantext bodies. Our goal is to embed images in them, which means they must be rendered as HTML, so they need to be escaped (which wasn't needed earlier.) Which is why the _.escape is only in the plaintext code path.

By the time we're at L225, if we had an HTML body, it's been sanitized and highlighted, and if we had a plaintext body, it's been escaped and highlighted. L225 is problematic though, since it assumes it's getting a string. I'll fix this in another commit - but it'll most likely mean that we'll have to use HtmlHighlighter for plaintext as well, but then it's pretty much the same since we've escaped that plaintext.

[ara4n]

generally LGTM, but a few questions i'd like to understand more first :) @aviraldg PTAL

[aviraldg]

@ara4n Have removed the emoji to image code from here so this should be good to go just for the emoji autocomplete.

[ara4n]

@aviraldg: just to check; this is being superceded by a PR that lazyloads the autocomplete entries?

[aviraldg]

@ara4n It is, but that's probably going to take a while to land.

[aviraldg]

Closing, since this will now be done in #296 and #292.