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

Improve emojis heights #2890

Closed
MightyCreak opened this issue Jan 5, 2017 · 12 comments · Fixed by matrix-org/matrix-react-sdk#5401
Closed

Improve emojis heights #2890

MightyCreak opened this issue Jan 5, 2017 · 12 comments · Fixed by matrix-org/matrix-react-sdk#5401

Comments

@MightyCreak
Copy link

MightyCreak commented Jan 5, 2017

I find the emojis to be difficult to see when inlined in text.
Also, when you write one emoji, you can see a bigger image, but when you write two of them, they are considered as inlined and appear very small again.

Here's how Slack deal with it:

  • The emojis within text lines are a bit bigger than the text height
  • When only emojis (and spaces) are on a line, display the on a bigger line

Slack screenshot:
emoji_heights

@MightyCreak
Copy link
Author

Part of #2984

@ara4n
Copy link
Member

ara4n commented Feb 19, 2017

We already bump emoji size when a line is nothing but emoji, as Slack does. Agreed we might want to increase the size of the inline ones a bit.

@MightyCreak
Copy link
Author

I've seen that as well later. There is still one subtleties: Slack allows spaces between emojis. The emojis are reduced only if there is a non space character.

@ara4n
Copy link
Member

ara4n commented Nov 8, 2020

This is surprisingly hard to fix, as we render emoji via fonts these days, and CSS doesn't give us a way to say "please make glyphs in this font disproportionately big". The only way i can think of doing it would be to run a regexp over every message to wrap emoji unicode in a <span/> which has a bigger font applied (and which could also provide an alt text for accessibility and to help discover the 'names' of emoji by rolling over them, as per #15674).

I'm not sure that the additional complexity and slowdown of this additional render phase is necessarily going to be worth the win, though.

@MightyCreak
Copy link
Author

Thanks for keeping us informed 👍

I admit the emoji sizes are not of the utmost priority, though if this would also bring caption on the emojis (as per #15674), that would be a very nice side effect!

Considering performances, obviously you know better, but I wonder if there would be a visible impact as this would happen user-side (i.e. Element) and, I imagine, only when receiving a new message. I'm less sure as how to filter out emojis.. is it possible to simply check for a specific range of UTF-8 characters?

@t3chguy
Copy link
Member

t3chguy commented Nov 8, 2020

only when receiving a new message.

That assumes that no wrongful rerenders occur, but in reality a lot of the app re-renders when it doesn't need to so it would happen a lot more

@rkfg
Copy link
Contributor

rkfg commented Nov 8, 2020

I implemented it in matrix-org/matrix-react-sdk#5401, please take a look. The additional processing won't be used if the message doesn't contain any emojis (and this check already exists for every message).

@MightyCreak
Copy link
Author

@t3chguy But even on a full redraw, we're talking of a few hundreds of text lines to parse. Our machines sure have the computing power to do that without any hickups (especially if done asynchronously), don't you think?

@rgpublic
Copy link

rgpublic commented Nov 9, 2020

I think this is a classic case of "Premature optimization is the root of all evil". There is no proof that implementing this has any negative performance impact. I can run a grep (regexp!) on hundered thousands of lines on the command line in a mere second. We have CPUs that decode jpegs and videos with highly complicated math and 50 frames/seconds and video games and what not. And we have browsers that render complicated websites with dozens of images, youtube, comments with lots of formatting, Slack (with their properly sized emojis) etc etc. and to just run a regexp over a few lines of text should cause performance problems? I'm very sceptical. Can someone provide any proof for that?

@rkfg
Copy link
Contributor

rkfg commented Nov 9, 2020

From learning React I realized that JS own speed is quite good, it's the rest of the browser that's slow (layout, styles, rendering etc.). So if React's declarative model and shadow DOM work fine I don't think we should worry about a loop over every new message with O(n) complexity. And a simple and predictable loop is much better than a complex regexp with nested groups and unpredictable run time. Unless it all becomes a noticeable bottleneck, that is. But there already is an O(n) algorithm for HTML sanitizing, trimming, applying highlights etc. So if O(n) becomes O(1.1n) in some rare cases when emojis are present, is it that significant? I'd agree if Matrix was used primarily for non-stop posting hundreds of kilobytes of raw text with smilies, maybe then it would become noticeable (and again, only on the client side). But if that were the case it wouldn't be readable by normal humans anyway.

@MightyCreak
Copy link
Author

MightyCreak commented Nov 9, 2020

While I agree with you @rgpublic and @rkfg, I've learnt that what might appears obvious to the external user can be very difficult in the eyes of those actually with their hands in the code. There are surely blind spots we don't see.

That is mainly why we are asking these questions @t3chguy: in our heads it doesn't seem such a performance hazard, but you seem to disagree. Could you explain to us why you think so?

Thank you 😉

@t3chguy
Copy link
Member

t3chguy commented Nov 9, 2020

I didn't say the performance impact would be large, I just corrected a mistaken understanding as to when it would run

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

Successfully merging a pull request may close this issue.

7 participants