T/629b Alternative fix infinite selection loop. #671

scofalik · 2016-11-02T09:52:13Z

This is an alternative way of solving ckeditor/ckeditor5#3843 (previous solution is in ckeditor/ckeditor5#3887)

Reinmar · 2016-11-03T15:19:00Z

src/model/selection.js

-		return this.isBackward === otherSelection.isBackward;
+		// Every range from this selection...
+		return Array.from( this.getRanges() ).every( ( rangeA ) => {
+			// ...Has a range in other selection...


// ... has :P

Reinmar · 2016-11-03T15:19:19Z

src/model/selection.js

+		return Array.from( this.getRanges() ).every( ( rangeA ) => {
+			// ...Has a range in other selection...
+			return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => {
+				// That it is equal to.


// ... which it is equal to.

Reinmar · 2016-11-03T15:22:25Z

src/view/observer/selectionobserver.js

@@ -102,6 +102,9 @@ export default class SelectionObserver extends Observer {
 		}

 		domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );
+		domDocument.addEventListener( 'keydown', () => this._clearInfiniteLoop() );


As I commented in https://github.com/ckeditor/ckeditor5-engine/issues/535#issuecomment-258168263, I prefer the other solution.

Reinmar · 2016-11-03T15:26:35Z

src/view/selection.js

 		}

-		return this._lastRangeBackward === otherSelection._lastRangeBackward;
+		// Every range from this selection...


Do I see correctly that the ranges can be in a different order? Why so? Then, the _lastRangeBackward property would need a more precise check. Besides, if we don't expect this to happen frequently, I'd consider these selections different.

Backward/forward selection check is indirectly implemented when we check anchor and focus. Basically, if anchor and focus are equal and ranges are equal, this means that selections are equal. If the last range was added in different direction, anchor and focus would differ.

I was rather asking why is this method trying to find a range in the other selection which matches range from this selection... for each range in this selection? Moreover, this implementation is very ineffective, cause it creates new array on every call of every() callback and makes huge number of comparisons (in a bit random way). And last but not least – since we're checking anchor and focus first, then checking ranges again will for a collapsed selection tripple the amount of work and for a single range non collapsed selection double it.

Therefore, I've been asking about the order of ranges (whether it shouldn't be the same in both selections) because the easiest possible implementation is to loop through both selections' ranges and compare the pairs. Not only this will be much faster, but also cleaner.

Okay, I see.

Ranges are not sorted upon insertion. This means that we have to check whether every range in selection A has a matching range in selection B the way we do. If the ranges are ordered that would be easier.

It is a matter of us deciding whether the order range is important. Being strict, I think that we should not care about the ranges order, because from "outside" selection: [ 1, 4 ], [ 5, 7 ], [ 8, 9 ] forward behaves exactly the same as [ 5, 7 ], [ 1, 4 ], [ 8, 9 ] forward. On the other hand, the amount of situations when this will be important is close to 0.

I can change the implementation but... To be honest selection will usually have just one range, except of tables where it may be more but in 99% scenarios it will will be less than 10-20 ranges. I agree with changing so the arrays are not created. If you want to change "order logic" I'm fine with that as well, just confirm please whether we are doing it or leaving it as is.

And last but not least – since we're checking anchor and focus first, then checking ranges again will for a collapsed selection triple.

So, instead one operation we have three? And it is me who get complaints about premature optimization? :)

was rather asking why is this method trying to find a range in the other selection which matches range from this selection... for each range in this selection

BTW. I don't know if you haven't got too confused...

For each range in selection A, we check if there is a matching range in selection B. The only way we can "speed" this up is if we already have sorted arrays... (or care about the order).

Reinmar · 2016-11-03T15:28:00Z

tests/view/observer/selectionobserver.js

+		setTimeout( () => {
+			expect( spy.called ).to.be.false;
+			done();
+		}, 1500 );


If there's a chance to avoid (or shorten) such tests, then please do.

Reinmar · 2016-11-03T15:31:46Z

I'm trying to match this code to the comment that you left me on priv. Is this really comparing the selection as you described it – after "unifying" it by conversion to model and back?

In general, I like the idea very much. I'm not if it will be perfect (there are tricky cases, especially with block widgets), but the improved inf loop check should catch them. So I'm +1, but I'd like to understand how this code works :D.

PS. It's not review ready, right? You wrote something about the tests. If you were worried whether it makes sense to work on them, then for me it does.

scofalik · 2016-11-04T08:50:42Z

This solution prevents creating infinite loop by not setting model selection if it has not changed. This results in not firing renderer and breaks the loop. It's done here.
https://github.com/ckeditor/ckeditor5-engine/pull/671/files#diff-977f0f36f33d59ceb01cc1aaafaa45faR42

This was problem number one and it actually caused an infinite loop if not for the if that breaks it. The problem number two was that current infinite loop check is too restrictive. It does not take into account time in which changes happen or their source. This means that you could slowly press left arrow key and right arrow key and still get a warning.

Your idea with checking time is good to. Conceptually it's similar to what I've done (we both want to check if something happens between selection changes) but I like your idea better.

As far as tests are concerned, I feel like testing DOM selection behavior in unit test might be a mistake due to asynchronous nature of selectonchange event. I had to pick correct params for tests (timeouts) so they won't fail (just like I did on physics labs when I was studying 🎓 ). I think I'd rather have some manual tests for those cases.

However, in my mind this is ready to review. There is just another (the first) solution available too. However I like this one better because less things changed and it works on model instead of view/DOM. In the first solution we check whether new view selection is touching old view selection and do not change it if they do. This means that this change: a^<b>b</b>c -> a<b>^b</b>c if it happened just in DOM, would not be reflected in view. I am afraid this could cause some errors so I came up with this new solution operating on model.

Reinmar · 2016-11-04T12:02:39Z

OK, now all it's clear. So the only thing that I'd like to improve before the review is the way how we test this. I agree with you that this deserves a manual test. Let's try to avoid such complicated (and slow) automated tests.

…ervals.

…d added manual test.

scofalik · 2016-11-09T14:37:08Z

I've changed SelectionObserver so that it clears infinite loop counter in time interval, rather after DOM event.
I've also added manual test, removed unit test.

Reinmar · 2016-11-09T16:28:14Z

src/view/observer/selectionobserver.js

@@ -179,12 +179,23 @@ export default class SelectionObserver extends Observer {
 			this._loopbackCounter = 0;
 		}

-		if ( this._loopbackCounter > 10 ) {
+		if ( this._loopbackCounter > 50 ) {


Deserves some comment that you empirically tested that making more than 50 selection changes in 2s is not possible. BTW. What about making selections by dragging over the text?

BTW. What about making selections by dragging over the text?

The selection has to be same as previous or pre-previous selection, so you would have to drag precisly, which means that it would be probably impossible to do 50 changes in 2 secs.

Reinmar · 2016-11-09T16:38:13Z

I merged master to this branch and fixed the manual test (which now should always be inside manual/ directory to be properly filtered out). There are some red tests. Please check then and let's try to finalize this ticket. I'll check your solution manually tomorrow morning.

Reinmar · 2016-11-09T16:43:34Z

Interesting. This is my status:


SUMMARY:
✔ 2198 tests completed
✖ 5 tests failed

FAILED TESTS:
  ✖ "before each" hook
    Chrome 54.0.2840 (Mac OS X 10.12.1)
  Error: Uncaught TypeError: Cannot read property 'isEqual' of null (tests/engine/controller/editingcontroller.js:21316)

  SelectionObserver
    ✖ should fire selectionChange when it is the only change
      Chrome 54.0.2840 (Mac OS X 10.12.1)
    Error: Uncaught TypeError: Cannot read property 'isEqual' of null (tests/engine/controller/editingcontroller.js:21316)

    ✖ "before each" hook
      Chrome 54.0.2840 (Mac OS X 10.12.1)
    Error: Uncaught TypeError: Cannot read property 'isEqual' of null (tests/engine/controller/editingcontroller.js:21316)

    ✖ "after each" hook for "should throw error if one try to create positions before root"
      Chrome 54.0.2840 (Mac OS X 10.12.1)
    TypeError: Cannot read property 'call' of undefined

    ✖ "after each" hook for "should throw error if one try to create positions before root"
      Chrome 54.0.2840 (Mac OS X 10.12.1)
    TypeError: Cannot read property 'call' of undefined


=============================== Coverage summary ===============================
Statements   : 95.44% ( 4541/4758 )
Branches     : 93.21% ( 2594/2783 )
Functions    : 97.77% ( 963/985 )
Lines        : 95.4% ( 4502/4719 )
================================================================================

On Travis there's less red tests, but the coverage is very low ;|

Reinmar · 2016-11-09T16:50:21Z

I've got this on master now:

Finished in 13.196 secs / 5.218 secs

SUMMARY:
✔ 2656 tests completed

=============================== Coverage summary ===============================
Statements   : 100% ( 4749/4749 )
Branches     : 99.89% ( 2772/2775 )
Functions    : 100% ( 979/979 )
Lines        : 100% ( 4711/4711 )
================================================================================

So it seems that some failing things outside of tests (in beforeEach as the first error says) prevents other tests from running... but it breaks a huge number of tests for some strange reason :D. Anyway, if you fix that, then other tests should start being executed.

Reinmar · 2016-11-10T13:16:32Z

OK, the trick here is that the order of running tests on CI and locally may be a bit different. I've noticed e.g. that when I executed --files=engine/controller,engine/view I've got some failing tests and in the reverse order everything is fine.

This means that some tests are not cleaning up after themselves correctly.

Reinmar · 2016-11-10T13:19:06Z

BTW. running engine/view/observer still logs out a lot of warnings about inf loop.

…ng the observer (so view#selChange won't be fired).

…s hard to find because it depended on how you executed the tests.

Reinmar · 2016-11-10T14:21:57Z

OK, I fixed some smaller things in the tests, but there's one, huge issue which is this ckeditor/ckeditor5#114 (comment).

Run:

gulp test:server -ws --files=engine/controller,engine/view

Open the debug mode (if you go to the Chrome window opened by Karma there's "debug mode" button), open console and run the tests there. See that there are multiple errors thrown. They are thrown because some old selection observers are not destroyed and affect other tests.

There's no other solution, IMO, than working on view document destroy chain which at least should destroy observers which at least should remove all native event listeners.

To do that we need DomEmitterMixin in ckeditor5-utils. It will need to be mixed into Observer class I guess. Then, using listenTo( nativeNode ) you can listen to native events. Calling stopListening() in Observer's destroy() methods should clean up everything nicely.

The last step is to ensure that ViewDocument instances are always being destroyed in afterEach() in all existing tests. This will ensure that engine tests pass nicely. But this is only the beginning if one wants to ensure that all package tests work together, because then we also need to destroy whole editors and work on their destroy chains :D But this is future.

Reinmar · 2016-11-10T14:28:41Z

src/view/observer/selectionobserver.js

@@ -103,6 +105,8 @@ export default class SelectionObserver extends Observer {

 		domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );

+		setInterval( () => this._clearInfiniteLoop(), 2000 );


This interval needs to be cleaned on destroy().

…m events.

scofalik · 2016-11-15T15:09:56Z

I've made changes to the commented parts of code and also worked on destroy chain for EditingController -> ViewDocument -> observers -> dom events.

I've also, hopefully, got rid of inf-selection-loop warning in tests for good :).

Reinmar · 2016-11-15T15:55:40Z

tests/controller/editingcontroller.js

@@ -274,6 +277,8 @@ describe( 'EditingController', () => {
 			} );

 			expect( spy.called ).to.be.false;
+
+			editing.destroy();
 		} );


I miss a test that destroy() destroys the view.

Reinmar · 2016-11-15T16:14:15Z

Manually all this works perfectly.

The remaining issues are:

T/629b Alternative fix infinite selection loop. #671 (comment)
T/629b Alternative fix infinite selection loop. #671 (comment)
missing docs for new destroy() methods.

scofalik · 2016-11-16T08:24:42Z

I've added requested test and changed implementation of isEqual a bit. If you are fine with it, please close the review. If you really want to not care about ranges order, I will simplify the implementation further.

scofalik added 2 commits November 2, 2016 09:34

Fixed: infinite selection fixing loop.

f4c0d25

Fixed: clear infinite loop counter after user interaction.

b9d6bc2

scofalik mentioned this pull request Nov 2, 2016

T/629 Fix infinite selection change loop #668

Closed

scofalik added 3 commits November 2, 2016 10:53

Fixed: view.Selection#isEqual better algorithm.

1b39bfb

Fixed: view.Selection#isEqual for fake selections.

d2d1e73

Tests: fixed failed tests and 100% CC.

a656829

scofalik mentioned this pull request Nov 2, 2016

T/47 Fixed: Selection collapsed in wrong position after new text node is created ckeditor/ckeditor5-typing#62

Merged

Reinmar reviewed Nov 3, 2016

View reviewed changes

scofalik added 2 commits November 9, 2016 15:34

Changed: SelectionObserver clearing infinite loop counter in time int…

d80b367

…ervals.

Tests: SelectionObserver infinite loop removed unstable unit tests an…

317d3dc

…d added manual test.

Tests: updated manual test description.

f220109

Reinmar reviewed Nov 9, 2016

View reviewed changes

Reinmar added 2 commits November 9, 2016 17:29

Merge branch 'master' into t/629b

a83c2ab

Manual ticket tests should also be inside manual/ directory.

6b6da0c

Tests: Improved cleanup.

3768f04

Reinmar mentioned this pull request Nov 10, 2016

Verify destruction chain ckeditor/ckeditor5#114

Closed

Tests: Cleaning listeners should not be necessary since we're disabli…

3dc704c

…ng the observer (so view#selChange won't be fired).

Tests: Fixed a selection observer test which could never work, but wa…

a8e53ca

…s hard to find because it depended on how you executed the tests.

Reinmar reviewed Nov 10, 2016

View reviewed changes

scofalik added 4 commits November 15, 2016 12:59

Added: Implemented destroy chain for EditingController->observers->do…

dcf65b8

…m events.

Fixed: Selection#isEqual was throwing in the selection had no ranges.

0de2bf7

Docs: added/fixed code comments.

25b8ba2

Tests: Additional tests for model.Selection and view.SelectionObserver.

a3690d5

Reinmar reviewed Nov 15, 2016

View reviewed changes

Merge branch 'master' into t/629b

b1e0e1f

Reinmar added the status:review- label Nov 15, 2016

scofalik added 2 commits November 16, 2016 10:10

Changed: simplified implementation of model/view.Selection#isEqual.

adcdbdc

Tests: added additional test for EditingController#destroy.

6a2418b

scofalik force-pushed the t/629b branch from b5e00dd to 6a2418b Compare November 16, 2016 09:11

scofalik and others added 4 commits November 16, 2016 11:21

Docs: added missing documentation.

7ccaa13

Tests: expanded tests for model/view.Selection to cover more cases.

bf9fc9f

Tests: Should not log a warning which is expected to be logged.

bf87840

Merge branch 'master' into t/629b

6a9306e

Reinmar merged commit 94ad63d into master Nov 16, 2016

Reinmar deleted the t/629b branch November 16, 2016 10:53

Reinmar removed the status:review- label Nov 16, 2016

Reinmar mentioned this pull request Oct 9, 2019

Move DomEmitterMixin to utils ckeditor/ckeditor5#5273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/629b Alternative fix infinite selection loop. #671

T/629b Alternative fix infinite selection loop. #671

scofalik commented Nov 2, 2016 •

edited by pomek

Loading

Reinmar Nov 3, 2016

Reinmar Nov 3, 2016

Reinmar Nov 3, 2016

Reinmar Nov 3, 2016

scofalik Nov 15, 2016

Reinmar Nov 15, 2016

scofalik Nov 16, 2016 •

edited

Loading

scofalik Nov 16, 2016 •

edited

Loading

Reinmar Nov 3, 2016

Reinmar commented Nov 3, 2016

scofalik commented Nov 4, 2016 •

edited

Loading

Reinmar commented Nov 4, 2016

scofalik commented Nov 9, 2016

Reinmar Nov 9, 2016

scofalik Nov 15, 2016

Reinmar commented Nov 9, 2016 •

edited

Loading

Reinmar commented Nov 9, 2016

Reinmar commented Nov 9, 2016

Reinmar commented Nov 10, 2016

Reinmar commented Nov 10, 2016

Reinmar commented Nov 10, 2016

Reinmar Nov 10, 2016

scofalik commented Nov 15, 2016

Reinmar Nov 15, 2016

Reinmar commented Nov 15, 2016

scofalik commented Nov 16, 2016

		@@ -103,6 +105,8 @@ export default class SelectionObserver extends Observer {

		domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );

		setInterval( () => this._clearInfiniteLoop(), 2000 );

T/629b Alternative fix infinite selection loop. #671

T/629b Alternative fix infinite selection loop. #671

Conversation

scofalik commented Nov 2, 2016 • edited by pomek Loading

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

scofalik Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

scofalik Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Nov 3, 2016

scofalik commented Nov 4, 2016 • edited Loading

Reinmar commented Nov 4, 2016

scofalik commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Nov 9, 2016 • edited Loading

Reinmar commented Nov 9, 2016

Reinmar commented Nov 9, 2016

Reinmar commented Nov 10, 2016

Reinmar commented Nov 10, 2016

Reinmar commented Nov 10, 2016

Choose a reason for hiding this comment

scofalik commented Nov 15, 2016

Choose a reason for hiding this comment

Reinmar commented Nov 15, 2016

scofalik commented Nov 16, 2016

scofalik commented Nov 2, 2016 •

edited by pomek

Loading

scofalik Nov 16, 2016 •

edited

Loading

scofalik Nov 16, 2016 •

edited

Loading

scofalik commented Nov 4, 2016 •

edited

Loading

Reinmar commented Nov 9, 2016 •

edited

Loading