Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #114 from ckeditor/t/113
Browse files Browse the repository at this point in the history 
Fix: Link balloon should update its position upon external document changes. Closes #113.
  • Loading branch information
@oskarwrobel
oskarwrobel committed Apr 27, 2017
2 parents 866fa49 + f0adbbd commit 18a5b90
Show file tree
Hide file tree
Showing 5 changed files with 334 additions and 143 deletions.
71 changes: 46 additions & 25 deletions src/link.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
} );
}
} );

Expand Down Expand Up @@ -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.
Expand All @@ -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();
}
} );
}

/**
Expand All @@ -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;
}
Expand Down
256 changes: 138 additions & 118 deletions tests/link.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand Down Expand Up @@ -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' );

Expand Down
13 changes: 13 additions & 0 deletions tests/manual/tickets/113/1.html
View file
Original file line number Diff line number Diff line change
@@ -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>
Loading

0 comments on commit 18a5b90

Please sign in to comment.