Asynchronous load of emoji list #2049

msamsel · 2018-06-04T09:40:35Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

provide asynchronous load of emojis
provide configuration option to load custom emoji list

Close #2036

mlewand · 2018-06-08T13:56:20Z

plugins/emoji/plugin.js

@@ -158,3 +174,34 @@
 * @cfg {Number} [emoji_minChars = 2]
 * @member CKEDITOR.config
 */
+
+/**
+ * Address to json file containing emoji list. File is downloaded through {@link CKEDITOR.ajax#load} method


JSON is an acronym should be uppercased.

Comandeer

There isn't any error handling. When emojis fail to load, then editor will throw new error for every try to insert emoji. In case of failing to load emojis file, plugin should shutdown gracefully.
Listening to instanceReady event to bind all listeners fails when editor is ready before emojis are loaded (e.g. super lag on server-side). If editor is already ready, just create emoji autocomplete and init it.
There is also very weird bug connected with undo
1. Open emoji sample.
2. Click into the editor.
Expected result:

Nothing happens.

Actual result:

Wild undo step appears. After applying it, preinserted emoji is changed back into text.

What's even weirder, clicking inside the editor after undoing causes redo step to disappear…

Comandeer · 2018-06-12T09:11:23Z

plugins/emoji/plugin.js

+
+/**
+ * Address to json file containing emoji list. File is downloaded through {@link CKEDITOR.ajax#load} method
+ * and url address is processed by {@link CKEDITOR#getUrl}.


URL is also an acronym ;)

… graceful shutdown in case of emoji loading error.

msamsel · 2018-06-18T09:10:35Z

I've rebase branch to contain change introduced in #2032

Comandeer

There are no tests for case when editor is loaded faster than file with emojis. There is also no test for failed loading of emojis.

I'm also not sure about current undo integration. If there's a situation when user already started typing and then emojis are loaded, selection jumps to the beginning of the editor and emoji substitution is recorded as extra undo step.

mlewand · 2018-06-19T10:17:34Z

Issue reported by @Comandeer is valid. What's more imagine a case when CKEditor makes few snapshots before the :emoji_tokens: are replaced. That would mean that we'd have to change an entire snapshots history.

Instead let's make a tough decision and drop the feature which replaced valid emoji tokens in the content, at paste, drop, etc.

Ideally I'd like to keep this feature at paste event, however looking at the implementation, it's implemented using core text filter mechanics and it would require some refactoring to make it happen. We want to conclude it so instead we'll remove the feature of replacing text tokens with emoji entirely.

To sum it up:

Keep the async emoji loading
Remove parts of emoji that are replacing text tokens with real emojis on setData/insertText/paste/drop etc.

…us but not convert emoji during paste data to editor.

Comandeer

http://tests.ckeditor.test:1030/tests/plugins/emoji/customemojilist#tests%2Fplugins%2Femoji%2Fcustomemojilist%20test%20long%20ajax%20loading fails in IE9.

There are also some issues with support for particular emojis in IEs. Forcing OS emoji font (Segoe UI Emoji) should be a proper fix for this. However I'm not sure if it's present on systems older than Windows 10.

The other issue is support for Linux as this system AFAIR does not have any built-in font with emoji support.

Comandeer · 2018-06-20T11:31:42Z

plugins/emoji/plugin.js


-				if ( !match ) {
-					return null;
+				if ( editor.status !== 'ready' ) {


Is this if needed now, when substitution is removed and initialisation just creates autocomplete instance?

yes, it's required because of this bug: #2114

Inlining this function cause random error, where plugin might be not initialised during loading stage (depend how fast json will be loaded). Error was clearly visible in Firefox browser.

Comandeer · 2018-06-20T11:32:09Z

plugins/emoji/plugin.js

-					return elementsPath.contains( forbiddenScope ) ? false : true;
-				} else {
-					return true;
+				function initPlugin() {


If we remove the if mentioned above, then this function could be inlined.

related to previous comment :)

Comandeer · 2018-06-20T11:33:50Z

tests/plugins/emoji/customemojilist.js

+		},
+
+		// #2036
+		'test custom emoji list is loadd': function() {


Typo: loadd → load

Comandeer · 2018-06-20T11:34:35Z

tests/plugins/emoji/customemojilist.js

+		'test invalid emoji list': function() {
+			var editor = this.editors.classic2;
+			emojiTools.runAfterInstanceReady( editor, null, function( editor ) {
+				assert.isUndefined( editor._.emoji, 'There are created emoji private data, so emoji was loaded what is wrong for this case.' );


This doesn't seem like good assertion message.

…ading to support emoji in IE, correct test to run on supported environments only.

msamsel · 2018-06-20T14:29:49Z

Font injection for IE browser in divarea editor is extracted here: #2121

Comandeer

When running tests/plugins/emoji/basic there are several errors in the console:

plugin.js:689 Uncaught TypeError: Cannot read property 'getClientRects' of undefined
    at View.getViewPosition (plugin.js:689)
    at View.updatePosition (plugin.js:910)
    at Autocomplete.onChange (plugin.js:418)
    at Autocomplete.<anonymous> (plugin.js:267)
    at CKEDITOR.dom.window.listenerFirer (event.js:144)
    at CKEDITOR.dom.window.CKEDITOR.event.fire (event.js:290)
    at domobject.js:46

I also slightly updated sample (Firefox had some styling issues) and fixed typo in tests, which disabled ignoring.

…or in console).

msamsel · 2018-06-21T08:06:59Z

I correct those tests. :)

Comandeer

Updated API docs, LGTM now!

mlewand reviewed Jun 8, 2018

View reviewed changes

msamsel force-pushed the t/1746 branch from 294c5d3 to 5a20beb Compare June 11, 2018 09:37

msamsel force-pushed the t/2036 branch from 77fe705 to 343a72a Compare June 11, 2018 10:32

Comandeer requested changes Jun 12, 2018

View reviewed changes

msamsel force-pushed the t/2036 branch 2 times, most recently from 25e5355 to 781de6d Compare June 18, 2018 08:58

Mateusz Samsel added 5 commits June 18, 2018 11:03

Make emoji plugin async.

f8d8481

Provide config option with documentantion.

047b769

Add unit and maual tests for custom emoji list.

5a4f1c4

Review fixes: update typos, better synchronization with undo manager,…

bd398db

… graceful shutdown in case of emoji loading error.

Review fixes: add tests for fixed issues.

9fe91ef

msamsel force-pushed the t/2036 branch from 781de6d to 9fe91ef Compare June 18, 2018 09:09

msamsel requested a review from Comandeer June 18, 2018 09:10

Comandeer requested changes Jun 18, 2018

View reviewed changes

Add test for long time emoji loading.

3421493

Mateusz Samsel added 5 commits June 19, 2018 15:21

Remove unnecessary methids from emoji plugin. Now plugin is asynchrno…

d9b2bfb

…us but not convert emoji during paste data to editor.

Update test to be coherent with last plugin changes.

280d04e

Remove unnecessary manual tests for emoji plugin.

015fa7a

Simplify manual test after modification in emoji plugin.

cd40631

Make emoji load on instance ready and adapt tests to it.

6a54a90

Comandeer requested changes Jun 20, 2018

View reviewed changes

Review fixes, drop support for IE lower than 11, add fallback font lo…

b2f9f3d

…ading to support emoji in IE, correct test to run on supported environments only.

Comandeer added 3 commits June 20, 2018 17:02

Fix typo in variable name.

ba30190

Update sample.

5899d04

Update issue reference in tests.

16833d5

Comandeer requested changes Jun 20, 2018

View reviewed changes

Close autcomplete panel in emoji's unit test (prevent of throwing err…

d472f09

…or in console).

Update API docs.

a386717

Comandeer approved these changes Jun 21, 2018

View reviewed changes

Comandeer merged commit a299443 into t/1746 Jun 21, 2018

Comandeer deleted the t/2036 branch June 21, 2018 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronous load of emoji list #2049

Asynchronous load of emoji list #2049

msamsel commented Jun 4, 2018

mlewand Jun 8, 2018

Comandeer left a comment

Comandeer Jun 12, 2018

msamsel commented Jun 18, 2018

Comandeer left a comment

mlewand commented Jun 19, 2018

Comandeer left a comment

Comandeer Jun 20, 2018

msamsel Jun 20, 2018

Comandeer Jun 20, 2018

msamsel Jun 20, 2018

Comandeer Jun 20, 2018

Comandeer Jun 20, 2018

msamsel commented Jun 20, 2018

Comandeer left a comment

msamsel commented Jun 21, 2018

Comandeer left a comment

Asynchronous load of emoji list #2049

Asynchronous load of emoji list #2049

Conversation

msamsel commented Jun 4, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jun 18, 2018

Comandeer left a comment

Choose a reason for hiding this comment

mlewand commented Jun 19, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jun 20, 2018

Comandeer left a comment

Choose a reason for hiding this comment

msamsel commented Jun 21, 2018

Comandeer left a comment

Choose a reason for hiding this comment