EmojiPicker stores state in global module-level variables

bug
1

In emoji-picker.js.es6:

The following variables are used for emoji-pickers, but they are currently module-GLOBAL variables instead of instance fields.

let $picker, $filter, $results, $list, scrollPosition, $visibleSections, _checkTimeout;

In particker, $picker is used to popup the emoji-picker. It is a module-global variable, so what that means is that there CANNOT BE any permanent emoji-picker’s on the page (e.g. discourse/babble and discourse/retort both put emoji-pickers permanently on the page).

When the compose-reply window is open, the standard emoji picker is created, overwriting $picker for everybody, causing all permanent emoji-pickers on the page to break.

Suggestion is to move all these variables into the class definition, and make them instance-specific fields. This should then allow for multiple emoji-pickers to co-exist on the same page with conflicts.

3 Likes
2

Question for @joffreyjaffeux

4

Yes it could probably use some refactoring, will look at it.

5

@joffreyjaffeux Thanks for looking into this.

When the code was written, it was probably assumed that only one emoji picker will ever be present on a page. A reasonable assumption actually, except that we’ll trust plugin authors to promptly break all our assumptions!

6

this and also I was quite new to js and discourse :wink:

7

No worries mate. JS can be a b**ch to pick up.

What you really need is to move those module-global variables into the class definition, so they are class-instance properties.

Then change all the access from $picker to this.$picker etc.

Then Bob’s your uncle! :sunglasses:

P.S. I could have made the changes myself and submit a PR, but I got scared of forking the entire Discourse repo just to test such small changes…

1 Like
8

No worries I will improve this. :wink:

9

Your commit here is what’s breaking everything:

https://github.com/discourse/discourse/commit/6a4d74d9f8681038ea1b273c173308b2fbd0f687#diff-2eadf5f693f654ff37f740c2904219a2

You attempted to optimize by caching some values, but caching them into module-global variables. Should have cached them into instance properties.

10

Oh, by the way, a not very related suggestion:

You are putting out the emoji’s as a huge bunch of PNG image files. When the emoji-picker pop’s up, the browser has to start fetching a huge number of very small PNG files. This affects user experience.

A much better idea is to have one single PNG image with all these sprites. Then use a class to fetch the relevant background-position from CSS. This should make the emoji’s show up much quicker.

So:

Standard emoji’s: put emoji name in class, use CSS to refer to background image and background-position to display it

Custom emoji’s: continue to put emoji name in background-url

1 Like
11

Incorrect, sprite sheets are a HTTP/1.1 optimization. This is also a fragile and complex techinque that breaks, say, simply linking to a given emoji. HTTP 2, which we already use and have for quite a few months, doesn’t require these contortions.

Now the advice “only load what is (currently) visible” is timeless, and damn I wish web browsers had implemented this back in 2002. (Try loading a long web page with 50 images, when 49 of them are not visible due to scroll position.)

5 Likes
12

I agree, but old practices are there for a reason.

Not everybody runs HTTP/2. For example, those behind a corporate proxy may be stuck for 1.1 for a long time.

And it seems like those sites not running HTTPS are not served HTTP/2. There are still lots of sites not on HTTPS.

13

The default Discourse docker image runs http/2, see for yourself at discourse.codinghorror.com etc. https://tools.keycdn.com/http2-test

This is one of the big, big advantages of Docker packaging – we can give people an optimal setup right out of the box.

4 Likes
14

@codinghorror You’re right! Using Let's Encrypt makes it really a no-brainer…

Since it is such a no-brainer to setup HTTP for Discourse, I agree that using HTTP/2 best practices is better, except in the case where somebody is behind a corporate proxy that is very very outdated and doesn’t support HTTP/2.

In other words, there really should not be many Discourse sites out there not running on HTTPS, right?

15

Fixed