Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emojify spoiler and content in web templates #785

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

blackle
Copy link
Contributor

@blackle blackle commented Aug 30, 2022

fixes #648

funny enough, emojification bugs were one of the first things I worked on for mastodon 5 years ago

probably my most complicated commit so far, the creation of the regex and template can maybe be optimized?

emojify takes a template.HTML parameter for the text because it assumes that everything is already escaped, and its output can't be escaped again or it will destroy the img tags. I noticed that .Content gets sent through noescape, but not .SpoilerText. Should SpoilerText be html escaped?

the "emoji" css class still needs to be made, not sure where to put it

@blackle blackle marked this pull request as ready for review September 1, 2022 00:55
@blackle
Copy link
Contributor Author

blackle commented Sep 1, 2022

added an emoji class, reused the EmojiFinder regex, and escaped the spoiler text before emojifying it, since escaping the spoiler text would have been the original behaviour of the template

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty cool and I'm excited to try it out :) just a few small comments

I noticed that .Content gets sent through noescape, but not .SpoilerText. Should SpoilerText be html escaped

the reason we put .Content through noescape is because .Content is html, so we want it to render as such

.SpoilerText is always sanitized to plaintext with no HTML in it, so we don't need to pass it through the noescape function. Ah, but I see in this PR you're making it so that spoilertext can contain html because it can contain emojis... is that correct? [edit] I misunderstood :D I think this looks fine then!

internal/router/template.go Outdated Show resolved Hide resolved
internal/router/template.go Outdated Show resolved Hide resolved
@NyaaaWhatsUpDoc
Copy link
Member

This looks good to me! Maybe another quick once over from @tsmethurst as they know our templating system better than me and should be good to merge :)

@tsmethurst
Copy link
Contributor

nice, thank you!! :)

@tsmethurst tsmethurst merged commit 1e1cdee into superseriousbusiness:main Sep 2, 2022
@tsmethurst
Copy link
Contributor

tsmethurst commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Custom emojis aren't rendered properly in web views
3 participants