瀏覽代碼

Merge pull request #896 from ckeditor/t/887

Fix: Renderer should not change the native selection if the one it's about to render is visually similar to the current one. Closes #887. Closes #880.
Piotrek Koszuliński 8 年之前
父節點
當前提交
ad5a7663f0

+ 45 - 0
packages/ckeditor5-engine/src/view/renderer.js

@@ -9,12 +9,14 @@
 
 import ViewText from './text';
 import ViewPosition from './position';
+import Selection from './selection';
 import { INLINE_FILLER, INLINE_FILLER_LENGTH, startsWithFiller, isInlineFiller, isBlockFiller } from './filler';
 
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import diff from '@ckeditor/ckeditor5-utils/src/diff';
 import insertAt from '@ckeditor/ckeditor5-utils/src/dom/insertat';
 import remove from '@ckeditor/ckeditor5-utils/src/dom/remove';
+import log from '@ckeditor/ckeditor5-utils/src/log';
 import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
@@ -580,6 +582,17 @@ export default class Renderer {
 			return;
 		}
 
+		if ( oldViewSelection && areSimilarSelections( oldViewSelection, this.selection ) ) {
+			const data = {
+				oldSelection: oldViewSelection,
+				currentSelection: this.selection
+			};
+
+			log.warn( 'renderer-skipped-selection-rendering: The selection was not rendered due to its similarity to the current one.', data );
+
+			return;
+		}
+
 		// Multi-range selection is not available in most browsers, and, at least in Chrome, trying to
 		// set such selection, that is not continuous, throws an error. Because of that, we will just use anchor
 		// and focus of view selection.
@@ -642,3 +655,35 @@ export default class Renderer {
 }
 
 mix( Renderer, ObservableMixin );
+
+// Checks if two given selections are similar. Selections are considered similar if they are non-collapsed
+// and their trimmed (see {@link #_trimSelection}) representations are equal.
+//
+// @private
+// @param {module:engine/view/selection~Selection} selection1
+// @param {module:engine/view/selection~Selection} selection2
+// @returns {Boolean}
+function areSimilarSelections( selection1, selection2 ) {
+	return !selection1.isCollapsed && trimSelection( selection1 ).isEqual( trimSelection( selection2 ) );
+}
+
+// Creates a copy of a given selection with all of its ranges
+// trimmed (see {@link module:engine/view/range~Range#getTrimmed getTrimmed}).
+//
+// @private
+// @param {module:engine/view/selection~Selection} selection
+// @returns {module:engine/view/selection~Selection} Selection copy with all ranges trimmed.
+function trimSelection( selection ) {
+	const newSelection = Selection.createFromSelection( selection );
+	const ranges = newSelection.getRanges();
+
+	let trimmedRanges = [];
+
+	for ( let range of ranges ) {
+		trimmedRanges.push( range.getTrimmed() );
+	}
+
+	newSelection.setRanges( trimmedRanges, newSelection.isBackward );
+
+	return newSelection;
+}

+ 3 - 0
packages/ckeditor5-engine/tests/manual/tickets/880/1.html

@@ -0,0 +1,3 @@
+<div id="editor">
+	<p>This is an <strong>editor</strong> instance.</p>
+</div>

+ 27 - 0
packages/ckeditor5-engine/tests/manual/tickets/880/1.js

@@ -0,0 +1,27 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classic';
+import EssentialsPreset from '@ckeditor/ckeditor5-presets/src/essentials';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
+
+ClassicEditor.create( document.querySelector( '#editor' ), {
+	plugins: [ EssentialsPreset, Paragraph, Bold ],
+	toolbar: [ 'undo', 'redo' ]
+} )
+.then( editor => {
+	window.editor = editor;
+
+	editor.editing.view.on( 'selectionChange', () => {
+		editor.document.enqueueChanges( () => {} );
+		console.log( 'selectionChange', ( new Date() ).getTime() );
+	} );
+} )
+.catch( err => {
+	console.error( err.stack );
+} );

+ 14 - 0
packages/ckeditor5-engine/tests/manual/tickets/880/1.md

@@ -0,0 +1,14 @@
+## Renderer - infinite `selectionChange` event
+
+Place the selection like this:
+
+```
+This is {an <strong>editor}</strong> instance.
+```
+
+(it must end at the end of the inline style)
+
+**Expected**:
+
+* Every time selection is changed, console log with `selectionChange` is printed once.
+* While creating selection from **2.**, there might be `renderer-selection-similar` warning visible in the console.

+ 3 - 0
packages/ckeditor5-engine/tests/manual/tickets/887/1.html

@@ -0,0 +1,3 @@
+<div id="editor">
+	<p>This is an <strong>editor</strong> instance.</p>
+</div>

+ 22 - 0
packages/ckeditor5-engine/tests/manual/tickets/887/1.js

@@ -0,0 +1,22 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classic';
+import EssentialsPreset from '@ckeditor/ckeditor5-presets/src/essentials';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
+
+ClassicEditor.create( document.querySelector( '#editor' ), {
+	plugins: [ EssentialsPreset, Paragraph, Bold ],
+	toolbar: [ 'undo', 'redo' ]
+} )
+.then( editor => {
+	window.editor = editor;
+} )
+.catch( err => {
+	console.error( err.stack );
+} );

+ 10 - 0
packages/ckeditor5-engine/tests/manual/tickets/887/1.md

@@ -0,0 +1,10 @@
+## Renderer - MacOS accent balloon on inline element boundary
+
+1. Place the selection like this:
+
+   `This is an <strong>editor{}</strong> instance.`
+2. Open MacOS accent balloon (e.g. long `a` press).
+3. Navigate through the balloon panel using arrow keys.
+
+**Expected**: It is possible to navigate with arrow keys inside the MacOS balloon panel. While navigating, there
+might be `renderer-selection-similar` warning visible in the console.

+ 293 - 0
packages/ckeditor5-engine/tests/view/renderer.js

@@ -8,6 +8,7 @@
 import ViewElement from '../../src/view/element';
 import ViewText from '../../src/view/text';
 import ViewRange from '../../src/view/range';
+import ViewPosition from '../../src/view/position';
 import Selection from '../../src/view/selection';
 import DomConverter from '../../src/view/domconverter';
 import Renderer from '../../src/view/renderer';
@@ -16,6 +17,7 @@ import { parse } from '../../src/dev-utils/view';
 import { INLINE_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, BR_FILLER } from '../../src/view/filler';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
+import log from '@ckeditor/ckeditor5-utils/src/log';
 
 testUtils.createSinonSandbox();
 
@@ -1349,6 +1351,297 @@ describe( 'Renderer', () => {
 				expect( bindSelection.isEqual( selection ) ).to.be.true;
 			} );
 		} );
+
+		// #887
+		describe( 'similar selection', () => {
+			// Use spies to check selection updates. Some selection positions are not achievable in some
+			// browsers (e.g. <p>Foo<b>{}Bar</b></p> in Chrome) so asserting dom selection after rendering would fail.
+			let selectionCollapseSpy, selectionExtendSpy, logWarnStub;
+
+			before( () => {
+				logWarnStub = sinon.stub( log, 'warn' );
+			} );
+
+			afterEach( () => {
+				if ( selectionCollapseSpy ) {
+					selectionCollapseSpy.restore();
+					selectionCollapseSpy = null;
+				}
+
+				if ( selectionExtendSpy ) {
+					selectionExtendSpy.restore();
+					selectionExtendSpy = null;
+				}
+				logWarnStub.reset();
+			} );
+
+			after( () => {
+				logWarnStub.restore();
+			} );
+
+			it( 'should always render collapsed selection even if it is similar', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>foo{}<attribute:b>bar</attribute:b></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+				const viewB = viewRoot.getChild( 0 ).getChild( 1 );
+
+				expect( domSelection.isCollapsed ).to.true;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 3 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domP.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 3 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>foo<attribute:b>{}bar</attribute:b></container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewB.getChild( 0 ), 0 ), new ViewPosition( viewB.getChild( 0 ), 0 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.calledOnce ).to.true;
+				expect( selectionCollapseSpy.calledWith( domB.childNodes[ 0 ], 0 ) ).to.true;
+				expect( selectionExtendSpy.calledOnce ).to.true;
+				expect( selectionExtendSpy.calledWith( domB.childNodes[ 0 ], 0 ) ).to.true;
+				expect( logWarnStub.notCalled ).to.true;
+			} );
+
+			it( 'should always render collapsed selection even if it is similar (with empty element)', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>foo<attribute:b>[]</attribute:b></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+
+				expect( domSelection.isCollapsed ).to.true;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( INLINE_FILLER_LENGTH );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( INLINE_FILLER_LENGTH );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>foo{}<attribute:b></attribute:b></container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewP.getChild( 0 ), 3 ), new ViewPosition( viewP.getChild( 0 ), 3 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.calledOnce ).to.true;
+				expect( selectionCollapseSpy.calledWith( domP.childNodes[ 0 ], 3 ) ).to.true;
+				expect( selectionExtendSpy.calledOnce ).to.true;
+				expect( selectionExtendSpy.calledWith( domP.childNodes[ 0 ], 3 ) ).to.true;
+				expect( logWarnStub.notCalled ).to.true;
+			} );
+
+			it( 'should always render non-collapsed selection if it not is similar', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>fo{o}<attribute:b>bar</attribute:b></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+				const viewB = viewRoot.getChild( 0 ).getChild( 1 );
+
+				expect( domSelection.isCollapsed ).to.false;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 2 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domP.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 3 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>fo{o<attribute:b>b}ar</attribute:b></container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewP.getChild( 0 ), 2 ), new ViewPosition( viewB.getChild( 0 ), 1 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.calledOnce ).to.true;
+				expect( selectionCollapseSpy.calledWith( domP.childNodes[ 0 ], 2 ) ).to.true;
+				expect( selectionExtendSpy.calledOnce ).to.true;
+				expect( selectionExtendSpy.calledWith( domB.childNodes[ 0 ], 1 ) ).to.true;
+				expect( logWarnStub.notCalled ).to.true;
+			} );
+
+			it( 'should not render non-collapsed selection it is similar (element start)', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>foo<attribute:b>{ba}r</attribute:b></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+				const viewB = viewRoot.getChild( 0 ).getChild( 1 );
+
+				expect( domSelection.isCollapsed ).to.false;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 0 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 2 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>foo{<attribute:b>ba}r</attribute:b></container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewP.getChild( 0 ), 3 ), new ViewPosition( viewB.getChild( 0 ), 2 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.notCalled ).to.true;
+				expect( selectionExtendSpy.notCalled ).to.true;
+				expect( logWarnStub.called ).to.true;
+			} );
+
+			it( 'should not render non-collapsed selection it is similar (element end)', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>foo<attribute:b>b{ar}</attribute:b>baz</container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+				const viewB = viewRoot.getChild( 0 ).getChild( 1 );
+
+				expect( domSelection.isCollapsed ).to.false;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domB.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 3 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>foo<attribute:b>b{ar</attribute:b>}baz</container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewB.getChild( 0 ), 1 ), new ViewPosition( viewP.getChild( 2 ), 0 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.notCalled ).to.true;
+				expect( selectionExtendSpy.notCalled ).to.true;
+				expect( logWarnStub.called ).to.true;
+			} );
+
+			it( 'should not render non-collapsed selection it is similar (element start - nested)', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>foo<attribute:b><attribute:i>{ba}r</attribute:i></attribute:b></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+				const viewI = viewRoot.getChild( 0 ).getChild( 1 ).getChild( 0 );
+
+				expect( domSelection.isCollapsed ).to.false;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domB.childNodes[ 0 ].childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 0 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domB.childNodes[ 0 ].childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 2 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>foo{<attribute:b><attribute:i>ba}r</attribute:i></attribute:b></container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewP.getChild( 0 ), 3 ), new ViewPosition( viewI.getChild( 0 ), 2 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.notCalled ).to.true;
+				expect( selectionExtendSpy.notCalled ).to.true;
+				expect( logWarnStub.called ).to.true;
+			} );
+
+			it( 'should not render non-collapsed selection it is similar (element end - nested)', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse(
+					'<container:p>f{oo<attribute:b><attribute:i>bar}</attribute:i></attribute:b>baz</container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const domP = domRoot.childNodes[ 0 ];
+				const domB = domP.childNodes[ 1 ];
+
+				expect( domSelection.isCollapsed ).to.false;
+				expect( domSelection.rangeCount ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP.childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 1 );
+				expect( domSelection.getRangeAt( 0 ).endContainer ).to.equal( domB.childNodes[ 0 ].childNodes[ 0 ] );
+				expect( domSelection.getRangeAt( 0 ).endOffset ).to.equal( 3 );
+
+				selectionCollapseSpy = sinon.spy( window.Selection.prototype, 'collapse' );
+				selectionExtendSpy = sinon.spy( window.Selection.prototype, 'extend' );
+
+				// <container:p>f{oo<attribute:b><attribute:i>bar</attribute:i></attribute:b>}baz</container:p>
+				selection.setRanges( [ new ViewRange( new ViewPosition( viewP.getChild( 0 ), 1 ), new ViewPosition( viewP.getChild( 2 ), 0 ) ) ] );
+
+				renderer.markToSync( 'children', viewP );
+				renderer.render();
+
+				expect( selectionCollapseSpy.notCalled ).to.true;
+				expect( selectionExtendSpy.notCalled ).to.true;
+				expect( logWarnStub.called ).to.true;
+			} );
+		} );
 	} );
 } );