Prevent user pills containing only emoji from embiggening #2907
Conversation
There was a problem hiding this comment.
There must be a more reliable way to detect mentions. The pill itself is well-specified: https://matrix.org/docs/spec/client_server/r0.4.0.html#user-room-and-group-mentions
|
Likely, but the reason for this solution was that emoji-only messages
should never have a `formatted_body`, and thus this may eliminate a
class of potential bugs? I can edit the comment to better reflect.
…On 10/04/2019 19:14, Travis Ralston wrote:
***@***.**** requested changes on this pull request.
There must be a more reliable way to detect mentions. The pill itself
is well-specified:
https://matrix.org/docs/spec/client_server/r0.4.0.html#user-room-and-group-mentions
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2907 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABR7mC1eoSL5BnjyvKDViOB3vbhzgrUmks5vfioXgaJpZM4cn0Zi>.
|
|
if the formatted_body contains just emoji though, we should be rendering it big too. |
|
In practice Riot web doesn't do this, but you're right, other clients might. In that case we can just check to embiggen if:
|
|
I'd argue that wrapped emoji and such also count as "just emoji". Things like:
|
|
Right then, if that's the case then we're quickly going to be chasing a theoretical edge case and we should just mitigate the tangible problem for now. We could be relatively specific and check if a message contains just an |
|
honestly I'd look for |
|
That works too. |
src/HtmlUtils.js
Outdated
| emojiBody = match && match[0] && match[0].length === contentBodyTrimmed.length | ||
| // Prevent user pills expanding for users with only emoji in | ||
| // their username | ||
| && !content.formatted_body.includes("https://matrix.to/"); |
There was a problem hiding this comment.
probably need a (content.formatted_body && !content.formatted_body.includes(...)) here just in case
There was a problem hiding this comment.
Ah, very good point.
src/HtmlUtils.js
Outdated
| @@ -519,7 +519,7 @@ export function bodyToHtml(content, highlights, opts={}) { | |||
| emojiBody = match && match[0] && match[0].length === contentBodyTrimmed.length | |||
| // Prevent user pills expanding for users with only emoji in | |||
| // their username | |||
| && !content.formatted_body.includes("https://matrix.to/"); | |||
| && (content.formatted_body && !content.formatted_body.includes("https://matrix.to/")); | |||
There was a problem hiding this comment.
Actually I might be a liar, sorry. Should be !formatted || !includes because otherwise it'll give false positives
There was a problem hiding this comment.
Can you explain the logic here? With || it'll no longer check whether formatted_body is undefined and thus fail on the includes check if it is.
Did you mean:
... && (!content.formatted_body || (content.formatted_body && content.formatted_body.includes(...)))
?
Fixes element-hq/element-web#8035
Emoji-only messages shouldn't have a formatted body. User pill messages do. We can exploit this to prevent user-pill username messages from being blown up.