Fixed issue with textwatcher throttle invalid selection

[jacekbogdanski]

What is the purpose of this pull request?

Bug fix

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?

Throttling textwatcher#check function sometimes results with out of sync selection. I moved selection getter into a throttled function.

Closes #2373

[mlewand]

It fixes the error, yes, but it takes away optimization that we want our devs to have - that we make sure that callback is not called by check too often.

See the comments for more info.

[mlewand]

Code style, missing spaces in [0].

[mlewand]

I don't know why this test is asynchronous and has a timeout? The assertion could be placed straight inside callback passed to attachTextWatcher making test synchronous and allowing us to drop this timeout.

[jacekbogdanski]

Not sure if I understand you correctly. The issue occurs when the selection changes between throttled inputs and the late input is fired with an invalid selection.

Also, the issue is only reproducible with enabled throttling.

How could we test this case with the synchronous path? There will be no time to broke selection between textwatcher.check and textwatcher._buffer.input execution.

[mlewand]

What this test is doing is essentially an equivalent of the following:

		// (#2373)
		'test throttle synchronization': function() {
			var editor = this.editor,
				bot = this.editorBot,
				textWatcher = attachTextWatcher( editor, function( selectionRange ) {
					var range = editor.getSelection().getRanges()[ 0 ];

					if ( editor.getSelection().getSelectedText() === 'yyy' ) {
						resume( function() {
							assert.isTrue( range.startContainer.equals( selectionRange.startContainer ) );
						} );
					}

					return {
						text: 'test'
					};
				}, 100 );

			bot.setHtmlWithSelection( '<b>[xxx]</b><i>yyy</i>' );

			textWatcher.check( {} );
			textWatcher.check( {} );

			bot.setHtmlWithSelection( '<b>xxx</b><i>[yyy]</i>' );

			wait();
		},

Note that there is no timeout. Same can be used for the next test case.

[mlewand]

That's nice that we were able to drop this quirk code 👍

[mlewand]

By inlining this code we're breaking the feature of this API, that check's are throttled out of the box, and end user does not need to worry about it.

With this proposition you're changing how the API behaves, and it will always call the TextWatcher.callback() which might be expensive.

What if we change thinking a bit? What if the actual fetch of selection and range happens inside the throttled function? It would make sense to put it in both places, I mean: leave it where it is now on master and put inside throttled function.

[mlewand]

I pushed solution based on above in the t/2373b branch. It also results with smaller LoC added compared to the original code.

[mlewand]

Note that there's one issue - in case if during thorttled call there's no selection (edge case, but we need to consider it) there's an early return.

https://github.com/ckeditor/ckeditor-dev/blob/b5e9817/plugins/textwatcher/plugin.js#L177-L179

It would be much better to notify unmatch, because we're no longer matching anything, right? It would be nice to add unit test coverage for this edge case.

[jacekbogdanski]

You are right that I accidentally removed the throttling feature from check function. I merged branches and added required changes.

[mlewand]

Please simplify the tests a little bit, ignore the test for mobile and I think we're good to go.

[mlewand]

This code can be further simplified. Now you have two separate code branches that call this.unmatch() if that's possible you want to reuse code branches if possible - that increases the readability.

Also this code has a bug as it is possible that it will fire unmatch event even if nothing was matched prior testTextMatch call. This is exactly what is happening in your test case. See test empty selection does not fire stray unmatched event unit test, and revert plugins\textwatcher\plugin.js file to previous version to see how it works.

I have fixed this issue in c8bca2a.

[mlewand]

This manual test case is not relevant for mobile browsers, and should be ignored.

[mlewand]

I have added a couple of commits that do address some issues.