Переглянути джерело

Merge pull request #114 from ckeditor/t/113

Fix: Link balloon should update its position upon external document changes. Closes #113.
Oskar Wróbel 8 роки тому
батько
коміт
4440690249

+ 46 - 25
packages/ckeditor5-link/src/link.js

@@ -192,21 +192,6 @@ export default class Link extends Plugin {
 			if ( viewSelection.isCollapsed && parentLink ) {
 				// Then show panel but keep focus inside editor editable.
 				this._showPanel();
-
-				// Avoid duplication of the same listener.
-				this.stopListening( viewDocument, 'render' );
-
-				// Start listen to view document changes and close the panel when selection will be moved
-				// out of the actual link element.
-				this.listenTo( viewDocument, 'render', () => {
-					const currentParentLink = this._getSelectedLinkElement();
-
-					if ( !viewSelection.isCollapsed || parentLink !== currentParentLink ) {
-						this._hidePanel();
-					} else {
-						this._balloon.updatePosition();
-					}
-				} );
 			}
 		} );
 
@@ -244,8 +229,42 @@ export default class Link extends Plugin {
 	 * @return {Promise} A promise resolved when the {@link #formView} {@link module:ui/view~View#init} is done.
 	 */
 	_showPanel( focusInput ) {
+		const editing = this.editor.editing;
+		const showViewDocument = editing.view;
+		const showIsCollapsed = showViewDocument.selection.isCollapsed;
+		const showSelectedLink = this._getSelectedLinkElement();
+
 		// https://github.com/ckeditor/ckeditor5-link/issues/53
-		this.formView.unlinkButtonView.isVisible = !!this._getSelectedLinkElement();
+		this.formView.unlinkButtonView.isVisible = !!showSelectedLink;
+
+		this.listenTo( showViewDocument, 'render', () => {
+			const renderSelectedLink = this._getSelectedLinkElement();
+			const renderIsCollapsed = showViewDocument.selection.isCollapsed;
+			const hasSellectionExpanded = showIsCollapsed && !renderIsCollapsed;
+
+			// Hide the panel if:
+			//   * the selection went out of the original link element
+			//     (e.g. paragraph containing the link was removed),
+			//   * the selection has expanded
+			// upon the #render event.
+			if ( hasSellectionExpanded || showSelectedLink !== renderSelectedLink ) {
+				this._hidePanel( true );
+			}
+			// Update the position of the panel when:
+			//  * the selection remains in the original link element,
+			//  * there was no link element in the first place, i.e. creating a new link
+			else {
+				// If still in a link element, simply update the position of the balloon.
+				if ( renderSelectedLink ) {
+					this._balloon.updatePosition();
+				}
+				// If there was no link, upon #render, the balloon must be moved
+				// to the new position in the editing view (a new native DOM range).
+				else {
+					this._balloon.updatePosition( this._getBalloonPositionData() );
+				}
+			}
+		} );
 
 		if ( this._balloon.hasView( this.formView ) ) {
 			// Check if formView should be focused and focus it if is visible.
@@ -254,16 +273,16 @@ export default class Link extends Plugin {
 			}
 
 			return Promise.resolve();
+		} else {
+			return this._balloon.add( {
+					view: this.formView,
+					position: this._getBalloonPositionData()
+				} ).then( () => {
+					if ( focusInput ) {
+						this.formView.urlInputView.select();
+					}
+				} );
 		}
-
-		return this._balloon.add( {
-				view: this.formView,
-				position: this._getBalloonPositionData()
-			} ).then( () => {
-				if ( focusInput ) {
-					this.formView.urlInputView.select();
-				}
-			} );
 	}
 
 	/**
@@ -275,6 +294,8 @@ export default class Link extends Plugin {
 	 * @param {Boolean} [focusEditable=false] When `true`, editable focus will be restored on panel hide.
 	 */
 	_hidePanel( focusEditable ) {
+		this.stopListening( this.editor.editing.view, 'render' );
+
 		if ( !this._balloon.hasView( this.formView ) ) {
 			return;
 		}

+ 138 - 118
packages/ckeditor5-link/tests/link.js

@@ -213,6 +213,144 @@ describe( 'Link', () => {
 						} );
 				} );
 		} );
+
+		describe( 'when the document is rendering', () => {
+			it( 'should not duplicate #render listeners', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph>f[]oo</paragraph>' );
+
+				const spy = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						viewDocument.render();
+						linkFeature._hidePanel();
+
+						return linkFeature._showPanel()
+							.then( () => {
+								viewDocument.render();
+								sinon.assert.calledTwice( spy );
+							} );
+					} );
+			} );
+
+			//https://github.com/ckeditor/ckeditor5-link/issues/113
+			it( 'updates the position of the panel – editing a link, then the selection remains in the link upon #render', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph><$text linkHref="url">f[]oo</$text></paragraph>' );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						const spy = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+
+						const root = viewDocument.getRoot();
+						const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
+
+						// Move selection to foo[].
+						viewDocument.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 3, text, 3 ) ], true );
+						viewDocument.render();
+
+						sinon.assert.calledOnce( spy );
+						sinon.assert.calledWithExactly( spy );
+					} );
+			} );
+
+			//https://github.com/ckeditor/ckeditor5-link/issues/113
+			it( 'updates the position of the panel – creating a new link, then the selection moved upon #render', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph>f[]oo</paragraph>' );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						const spy = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+
+						// Fires #render.
+						const root = viewDocument.getRoot();
+						const text = root.getChild( 0 ).getChild( 0 );
+
+						viewDocument.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 3, text, 3 ) ], true );
+						viewDocument.render();
+
+						sinon.assert.calledOnce( spy );
+						sinon.assert.calledWithExactly( spy, {
+							target: editorElement.ownerDocument.getSelection().getRangeAt( 0 ),
+							limiter: editorElement
+						} );
+					} );
+			} );
+
+			//https://github.com/ckeditor/ckeditor5-link/issues/113
+			it( 'hides of the panel – editing a link, then the selection moved out of the link upon #render', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph><$text linkHref="url">f[]oo</$text>bar</paragraph>' );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						const spyUpdate = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+						const spyHide = testUtils.sinon.spy( linkFeature, '_hidePanel' );
+
+						const root = viewDocument.getRoot();
+						const text = root.getChild( 0 ).getChild( 1 );
+
+						// Move selection to b[]ar.
+						viewDocument.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 1 ) ], true );
+						viewDocument.render();
+
+						sinon.assert.calledOnce( spyHide );
+						sinon.assert.notCalled( spyUpdate );
+					} );
+			} );
+
+			//https://github.com/ckeditor/ckeditor5-link/issues/113
+			it( 'hides of the panel – editing a link, then the selection moved to another link upon #render', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph><$text linkHref="url">f[]oo</$text>bar<$text linkHref="url">b[]az</$text></paragraph>' );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						const spyUpdate = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+						const spyHide = testUtils.sinon.spy( linkFeature, '_hidePanel' );
+
+						const root = viewDocument.getRoot();
+						const text = root.getChild( 0 ).getChild( 2 ).getChild( 0 );
+
+						// Move selection to b[]az.
+						viewDocument.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 1 ) ], true );
+						viewDocument.render();
+
+						sinon.assert.calledOnce( spyHide );
+						sinon.assert.notCalled( spyUpdate );
+					} );
+			} );
+
+			//https://github.com/ckeditor/ckeditor5-link/issues/113
+			it( 'hides the panel – editing a link, then the selection expands upon #render', () => {
+				const viewDocument = editor.editing.view;
+
+				setModelData( editor.document, '<paragraph><$text linkHref="url">f[]oo</$text></paragraph>' );
+
+				return linkFeature._showPanel()
+					.then( () => {
+						const spyUpdate = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
+						const spyHide = testUtils.sinon.spy( linkFeature, '_hidePanel' );
+
+						const root = viewDocument.getRoot();
+						const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
+
+						// Move selection to f[o]o.
+						viewDocument.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 2 ) ], true );
+						viewDocument.render();
+
+						sinon.assert.calledOnce( spyHide );
+						sinon.assert.notCalled( spyUpdate );
+					} );
+			} );
+		} );
 	} );
 
 	describe( '_hidePanel()', () => {
@@ -462,124 +600,6 @@ describe( 'Link', () => {
 				sinon.assert.calledWithExactly( spy );
 			} );
 
-			it( 'should keep open and update position until collapsed selection stay inside the same link element', () => {
-				// Method is stubbed because it returns internal promise which can't be returned in test.
-				const showSpy = testUtils.sinon.stub( linkFeature, '_showPanel', () => {} );
-				const hideSpy = testUtils.sinon.stub( linkFeature, '_hidePanel' );
-				const updatePositionSpy = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
-
-				editor.document.schema.allow( { name: '$text', inside: '$root' } );
-				setModelData( editor.document, '<$text linkHref="url">b[]ar</$text>' );
-
-				const root = editor.editing.view.getRoot();
-				const text = root.getChild( 0 ).getChild( 0 );
-
-				observer.fire( 'click', { target: document.body } );
-
-				// Panel is shown.
-				sinon.assert.calledOnce( showSpy );
-
-				// Move selection.
-				editor.editing.view.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 1 ) ], true );
-				editor.editing.view.render();
-
-				// Check if balloon is still opened (wasn't hide).
-				sinon.assert.notCalled( hideSpy );
-				// And position was updated
-				sinon.assert.calledOnce( updatePositionSpy );
-			} );
-
-			it( 'should not duplicate `render` listener on `ViewDocument`', () => {
-				const updatePositionSpy = testUtils.sinon.stub( balloon, 'updatePosition', () => {} );
-
-				// Method is stubbed because it returns internal promise which can't be returned in test.
-				testUtils.sinon.stub( linkFeature, '_showPanel', () => {} );
-
-				editor.document.schema.allow( { name: '$text', inside: '$root' } );
-				setModelData( editor.document, '<$text linkHref="url">b[]ar</$text>' );
-
-				// Click at the same link more than once.
-				observer.fire( 'click', { target: document.body } );
-				observer.fire( 'click', { target: document.body } );
-				observer.fire( 'click', { target: document.body } );
-
-				sinon.assert.notCalled( updatePositionSpy );
-
-				const root = editor.editing.view.getRoot();
-				const text = root.getChild( 0 ).getChild( 0 );
-
-				// Move selection.
-				editor.editing.view.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 1 ) ], true );
-				editor.editing.view.render();
-
-				// Position should be updated only once.
-				sinon.assert.calledOnce( updatePositionSpy );
-			} );
-
-			it( 'should close when selection goes outside the link element', () => {
-				const hideSpy = testUtils.sinon.stub( linkFeature, '_hidePanel' );
-
-				// Method is stubbed because it returns internal promise which can't be returned in test.
-				testUtils.sinon.stub( linkFeature, '_showPanel', () => {} );
-
-				editor.document.schema.allow( { name: '$text', inside: '$root' } );
-				setModelData( editor.document, 'foo <$text linkHref="url">b[]ar</$text>' );
-
-				const root = editor.editing.view.getRoot();
-				const text = root.getChild( 0 );
-
-				observer.fire( 'click', { target: document.body } );
-
-				sinon.assert.notCalled( hideSpy );
-
-				editor.editing.view.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 3, text, 3 ) ], true );
-				editor.editing.view.render();
-
-				sinon.assert.calledOnce( hideSpy );
-			} );
-
-			it( 'should close when selection goes to the other link element with the same href', () => {
-				const hideSpy = testUtils.sinon.stub( linkFeature, '_hidePanel' );
-
-				// Method is stubbed because it returns internal promise which can't be returned in test.
-				testUtils.sinon.stub( linkFeature, '_showPanel', () => {} );
-
-				editor.document.schema.allow( { name: '$text', inside: '$root' } );
-				setModelData( editor.document, '<$text linkHref="url">f[]oo</$text> bar <$text linkHref="url">biz</$text>' );
-
-				const root = editor.editing.view.getRoot();
-				const text = root.getChild( 2 ).getChild( 0 );
-
-				observer.fire( 'click', { target: document.body } );
-
-				sinon.assert.notCalled( hideSpy );
-
-				editor.editing.view.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 1 ) ], true );
-				editor.editing.view.render();
-
-				sinon.assert.calledOnce( hideSpy );
-			} );
-
-			it( 'should close when selection becomes non-collapsed', () => {
-				const hideSpy = testUtils.sinon.stub( linkFeature, '_hidePanel' );
-
-				// Method is stubbed because it returns internal promise which can't be returned in test.
-				testUtils.sinon.stub( linkFeature, '_showPanel', () => {} );
-
-				editor.document.schema.allow( { name: '$text', inside: '$root' } );
-				setModelData( editor.document, '<$text linkHref="url">f[]oo</$text>' );
-
-				const root = editor.editing.view.getRoot();
-				const text = root.getChild( 0 ).getChild( 0 );
-
-				observer.fire( 'click', { target: {} } );
-
-				editor.editing.view.selection.setRanges( [ Range.createFromParentsAndOffsets( text, 1, text, 2 ) ] );
-				editor.editing.view.render();
-
-				sinon.assert.calledOnce( hideSpy );
-			} );
-
 			it( 'should not open when selection is not inside link element', () => {
 				const showSpy = testUtils.sinon.stub( linkFeature, '_showPanel' );
 

+ 13 - 0
packages/ckeditor5-link/tests/manual/tickets/113/1.html

@@ -0,0 +1,13 @@
+<h2>External changes (insert/edit)</h2>
+<p><button id="button-insert">Start external changes</button></p>
+<div id="editor-insert">
+	<p>EXTERNAL CHANGES WILL START HERE -> This specification defines the 5th <a href="http://ckeditor.com">major revision</a> of the core language of the [Select this text]: the Hypertext Markup Language (HTML).</p>
+</div>
+
+<h2>External changes (delete)</h2>
+<p><button id="button-delete">Start external changes</button></p>
+<div id="editor-delete">
+	<p>A first paragraph.</p>
+	<p>THIS PARAGRAPH WILL BE REMOVED <a href="http://ckeditor.com">major revision</a>. [Select this text].</p>
+	<p>A third paragraph.</p>
+</div>

+ 114 - 0
packages/ckeditor5-link/tests/manual/tickets/113/1.js

@@ -0,0 +1,114 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console:false, document, setTimeout */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classic';
+import ArticlePresets from '@ckeditor/ckeditor5-presets/src/article';
+
+import Element from '@ckeditor/ckeditor5-engine/src/model/element';
+import Text from '@ckeditor/ckeditor5-engine/src/model/text';
+import Position from '@ckeditor/ckeditor5-engine/src/model/position';
+import Range from '@ckeditor/ckeditor5-engine/src/model/range';
+
+// Editor for the external insert.
+ClassicEditor.create( document.querySelector( '#editor-insert' ), {
+	plugins: [ ArticlePresets ],
+	toolbar: [ 'undo', 'redo', 'link' ]
+} )
+.then( editor => {
+	const element = document.querySelector( '#button-insert' );
+
+	element.addEventListener( 'click', () => {
+		element.disabled = true;
+		startExternalInsert( editor );
+	} );
+} )
+.catch( err => console.error( err.stack ) );
+
+// Editor for the external delete.
+ClassicEditor.create( document.querySelector( '#editor-delete' ), {
+	plugins: [ ArticlePresets ],
+	toolbar: [ 'undo', 'redo', 'link' ]
+} )
+.then( editor => {
+	const element = document.querySelector( '#button-delete' );
+
+	element.addEventListener( 'click', () => {
+		element.disabled = true;
+		startExternalDelete( editor );
+	} );
+} )
+.catch( err => console.error( err.stack ) );
+
+function wait( delay ) {
+	return new Promise( ( resolve ) => {
+		setTimeout( () => resolve(), delay );
+	} );
+}
+
+function startExternalInsert( editor ) {
+	const document = editor.document;
+	const bath = document.batch( 'transparent' );
+
+	function type( path, text ) {
+		return new Promise( ( resolve ) => {
+			let position = new Position( document.getRoot(), path );
+			let index = 0;
+
+			function typing() {
+				wait( 40 ).then( () => {
+					document.enqueueChanges( () => {
+						bath.insert( position, new Text( text[ index ] ) );
+						position = position.getShiftedBy( 1 );
+
+						let nextLetter = text[ ++index ];
+
+						if ( nextLetter ) {
+							typing( nextLetter );
+						} else {
+							index = 0;
+							resolve();
+						}
+					} );
+				} );
+			}
+
+			typing();
+		} );
+	}
+
+	function insertNewLine( path ) {
+		return wait( 200 ).then( () => {
+			document.enqueueChanges( () => {
+				bath.insert( new Position( document.getRoot(), path ), new Element( 'paragraph' ) );
+			} );
+
+			return Promise.resolve();
+		} );
+	}
+
+	wait( 3000 )
+		.then( () => type( [ 0, 36 ], `This specification defines the 5th major revision of the core language of the World Wide Web. ` ) )
+		.then( () => insertNewLine( [ 0 ] ) )
+		.then( () => type( [ 0, 0 ], 'a' ) )
+		.then( () => insertNewLine( [ 1 ] ) )
+		.then( () => type( [ 1, 0 ], 'b' ) )
+		.then( () => insertNewLine( [ 2 ] ) )
+		.then( () => type( [ 2, 0 ], 'c' ) )
+		.then( () => insertNewLine( [ 0 ] ) )
+		.then( () => type( [ 0, 0 ], 'DONE :)' ) );
+}
+
+function startExternalDelete( editor ) {
+	const document = editor.document;
+	const bath = document.batch( 'transparent' );
+
+	wait( 3000 ).then( () => {
+		document.enqueueChanges( () => {
+			bath.remove( Range.createFromPositionAndShift( new Position( document.getRoot(), [ 1 ] ), 1 ) );
+		} );
+	} );
+}

+ 23 - 0
packages/ckeditor5-link/tests/manual/tickets/113/1.md

@@ -0,0 +1,23 @@
+# Issue [#113](https://github.com/ckeditor/ckeditor5-ui/issues/198) manual test.
+
+## Position of the link panel should be updated on external changes.
+
+### Edit (existing link):
+
+1. Click **Start external changes** then quickly select the link in the content (the link editing balloon should show up).
+2. Observe if the balloon remains attached to the link.
+
+### Create (a new link):
+
+1. Click **Start external changes** then quickly select some text which is not a link yet and press the Link button in the toolbar (the link editing balloon should show up).
+2. Observe if the balloon remains attached the selection.
+
+### Delete (existing link):
+
+1. Click **Start external changes** then quickly select the link in the content (the link editing balloon should show up).
+2. Check if the balloon hides and there are no errors in the browser console.
+
+### Delete (a new link):
+
+1. Click **Start external changes** then quickly select some text in the **second paragraph** which is not a link yet and press the Link button in the toolbar (the link editing balloon should show up).
+2. Observe if the balloon remains attached to the the selection after the removal.