-
Notifications
You must be signed in to change notification settings - Fork 40
T/629b Alternative fix infinite selection loop. #671
Changes from 5 commits
f4c0d25
b9d6bc2
1b39bfb
d2d1e73
a656829
d80b367
317d3dc
f220109
a83c2ab
6b6da0c
3768f04
3dc704c
a8e53ca
dcf65b8
0de2bf7
25b8ba2
a3690d5
b1e0e1f
adcdbdc
6a2418b
7ccaa13
bf9fc9f
bf87840
6a9306e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,25 +125,29 @@ export default class Selection { | |
} | ||
|
||
/** | ||
* Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions. | ||
* Checks whether this selection is equal to given selection. Selections are equal if they have same directions, | ||
* same number of ranges and all ranges from one selection equal to a range from other selection. | ||
* | ||
* @param {engine.model.Selection} otherSelection Selection to compare with. | ||
* @returns {Boolean} `true` if selections are equal, `false` otherwise. | ||
*/ | ||
isEqual( otherSelection ) { | ||
const rangeCount = this.rangeCount; | ||
|
||
if ( rangeCount != otherSelection.rangeCount ) { | ||
if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) { | ||
return false; | ||
} | ||
|
||
for ( let i = 0; i < this.rangeCount; i++ ) { | ||
if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) { | ||
return false; | ||
} | ||
if ( this.rangeCount != otherSelection.rangeCount ) { | ||
return false; | ||
} | ||
|
||
return this.isBackward === otherSelection.isBackward; | ||
// Every range from this selection... | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return rangeA.isEqual( rangeB ); | ||
} ); | ||
} ); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,9 @@ export default class SelectionObserver extends Observer { | |
} | ||
|
||
domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) ); | ||
domDocument.addEventListener( 'keydown', () => this._clearInfiniteLoop() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I commented in https://github.com/ckeditor/ckeditor5-engine/issues/535#issuecomment-258168263, I prefer the other solution. |
||
domDocument.addEventListener( 'mousemove', () => this._clearInfiniteLoop() ); | ||
domDocument.addEventListener( 'mousedown', () => this._clearInfiniteLoop() ); | ||
|
||
this._documents.add( domDocument ); | ||
} | ||
|
@@ -151,10 +154,6 @@ export default class SelectionObserver extends Observer { | |
newSelection: newViewSelection, | ||
domSelection: domSelection | ||
} ); | ||
|
||
// If nothing changes on `selectionChange` event, at this point we have "dirty DOM" (changed) and de-synched | ||
// view (which has not been changed). In order to "reset DOM" we render the view again. | ||
this.document.render(); | ||
} | ||
|
||
/** | ||
|
@@ -185,6 +184,17 @@ export default class SelectionObserver extends Observer { | |
|
||
return false; | ||
} | ||
|
||
/** | ||
* Clears `SelectionObserver` internal properties connected with preventing infinite loop. | ||
* | ||
* @private | ||
*/ | ||
_clearInfiniteLoop() { | ||
this._lastSelection = null; | ||
this._lastButOneSelection = null; | ||
this._loopbackCounter = 0; | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,33 +283,39 @@ export default class Selection { | |
} | ||
|
||
/** | ||
* Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions. | ||
* Checks whether, this selection is equal to given selection. Selections are equal if they have same directions, | ||
* same number of ranges and all ranges from one selection equal to a range from other selection. | ||
* | ||
* @param {engine.view.Selection} otherSelection Selection to compare with. | ||
* @returns {Boolean} `true` if selections are equal, `false` otherwise. | ||
*/ | ||
isEqual( otherSelection ) { | ||
const rangeCount = this.rangeCount; | ||
|
||
if ( rangeCount != otherSelection.rangeCount ) { | ||
if ( this.isFake != otherSelection.isFake ) { | ||
return false; | ||
} | ||
|
||
if ( this.isFake != otherSelection.isFake ) { | ||
if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) { | ||
return false; | ||
} | ||
|
||
if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) { | ||
if ( this.rangeCount != otherSelection.rangeCount ) { | ||
return false; | ||
} else if ( this.rangeCount === 0 ) { | ||
return true; | ||
} | ||
|
||
for ( let i = 0; i < this.rangeCount; i++ ) { | ||
if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) { | ||
return false; | ||
} | ||
if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) { | ||
return false; | ||
} | ||
|
||
return this._lastRangeBackward === otherSelection._lastRangeBackward; | ||
// Every range from this selection... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I see correctly that the ranges can be in a different order? Why so? Then, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Backward/forward selection check is indirectly implemented when we check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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.
So, instead one operation we have three? And it is me who get complaints about premature optimization? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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). |
||
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. | ||
return rangeA.isEqual( rangeB ); | ||
} ); | ||
} ); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/* globals setTimeout, Range, document */ | ||
/* globals setTimeout, Range, document, KeyboardEvent, MouseEvent */ | ||
/* bender-tags: view, browser-only */ | ||
|
||
import ViewRange from '/ckeditor5/engine/view/range.js'; | ||
|
@@ -162,71 +162,90 @@ describe( 'SelectionObserver', () => { | |
} ); | ||
|
||
it( 'should not be treated as an infinite loop if the position is different', ( done ) => { | ||
let counter = 30; | ||
|
||
const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); | ||
viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); | ||
|
||
let counter = 0; | ||
|
||
const spy = testUtils.sinon.spy( log, 'warn' ); | ||
|
||
listenter.listenTo( viewDocument, 'selectionChange', () => { | ||
counter--; | ||
counter++; | ||
|
||
if ( counter > 0 ) { | ||
setTimeout( () => changeCollapsedDomSelection( counter ) ); | ||
} else { | ||
done(); | ||
if ( counter < 15 ) { | ||
setTimeout( changeCollapsedDomSelection, 100 ); | ||
} | ||
} ); | ||
|
||
changeCollapsedDomSelection( counter ); | ||
} ); | ||
changeCollapsedDomSelection(); | ||
|
||
it( 'should not be treated as an infinite loop if it is less then 3 times', ( done ) => { | ||
let counter = 3; | ||
setTimeout( () => { | ||
expect( spy.called ).to.be.false; | ||
done(); | ||
}, 1500 ); | ||
} ); | ||
|
||
it( 'should not be treated as an infinite loop if selection is changed only few times', ( done ) => { | ||
const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); | ||
viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); | ||
|
||
listenter.listenTo( viewDocument, 'selectionChange', () => { | ||
counter--; | ||
const spy = testUtils.sinon.spy( log, 'warn' ); | ||
|
||
if ( counter > 0 ) { | ||
setTimeout( () => changeDomSelection() ); | ||
} else { | ||
done(); | ||
} | ||
} ); | ||
for ( let i = 0; i < 4; i++ ) { | ||
changeDomSelection(); | ||
} | ||
|
||
changeDomSelection(); | ||
setTimeout( () => { | ||
expect( spy.called ).to.be.false; | ||
done(); | ||
}, 1500 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's a chance to avoid (or shorten) such tests, then please do. |
||
} ); | ||
|
||
it( 'should call render after selection change which reset selection if it was not changed', ( done ) => { | ||
const viewBar = viewDocument.getRoot().getChild( 1 ).getChild( 0 ); | ||
viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewBar, 0, viewBar, 1 ) ); | ||
const events = { | ||
keydown: KeyboardEvent, | ||
mousedown: MouseEvent, | ||
mousemove: MouseEvent | ||
}; | ||
|
||
listenter.listenTo( viewDocument, 'selectionChange', () => { | ||
setTimeout( () => { | ||
const domSelection = document.getSelection(); | ||
for ( let event in events ) { | ||
it( 'should not be treated as an infinite loop if change is triggered by ' + event + ' event', ( done ) => { | ||
let counter = 0; | ||
|
||
expect( domSelection.rangeCount ).to.equal( 1 ); | ||
const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); | ||
viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); | ||
|
||
const domRange = domSelection.getRangeAt( 0 ); | ||
const domBar = document.getElementById( 'main' ).childNodes[ 1 ].childNodes[ 0 ]; | ||
const spy = testUtils.sinon.spy( log, 'warn' ); | ||
|
||
expect( domRange.startContainer ).to.equal( domBar ); | ||
expect( domRange.startOffset ).to.equal( 0 ); | ||
expect( domRange.endContainer ).to.equal( domBar ); | ||
expect( domRange.endOffset ).to.equal( 1 ); | ||
listenter.listenTo( viewDocument, 'selectionChange', () => { | ||
counter++; | ||
|
||
done(); | ||
if ( counter < 15 ) { | ||
setTimeout( () => { | ||
document.dispatchEvent( new events[ event ]( event ) ); | ||
changeDomSelection(); | ||
}, 100 ); | ||
} | ||
} ); | ||
} ); | ||
|
||
changeDomSelection(); | ||
} ); | ||
setTimeout( () => { | ||
expect( spy.called ).to.be.false; | ||
done(); | ||
}, 1000 ); | ||
|
||
document.dispatchEvent( new events[ event ]( event ) ); | ||
changeDomSelection(); | ||
} ); | ||
} | ||
} ); | ||
|
||
function changeCollapsedDomSelection( pos = 1 ) { | ||
function changeCollapsedDomSelection() { | ||
const domSelection = document.getSelection(); | ||
const pos = domSelection.anchorOffset + 1; | ||
|
||
if ( pos > 20 ) { | ||
return; | ||
} | ||
|
||
domSelection.removeAllRanges(); | ||
const domFoo = document.getElementById( 'main' ).childNodes[ 0 ].childNodes[ 0 ]; | ||
const domRange = new Range(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ... has
:P