Merge 9811bdc into 1128cb8

ckeditor · Jun 26, 2018 · ceb4326 · ceb4326
2 parents 1128cb8 + 9811bdc
commit ceb4326
Show file tree

Hide file tree

Showing 11 changed files with 131 additions and 42 deletions.
diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js
@@ -12,7 +12,7 @@ import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer';
 /**
  * Observes all new images added to the {@link module:engine/view/document~Document},
  * fires {@link module:engine/view/document~Document#event:imageLoaded} and
- * {@link module:engine/view/document~Document#layoutChanged} event every time when the new image
+ * {@link module:engine/view/document~Document#event:layoutChanged} event every time when the new image
  * has been loaded.
  *
  * **Note:** This event is not fired for images that has been added to the document and rendered as `complete` (already loaded).
@@ -82,7 +82,7 @@ export default class ImageLoadObserver extends Observer {
 	}
 
 	/**
-	 * Fires {@link module:engine/view/view/document~Document#event:layoutChanged} and
+	 * Fires {@link module:engine/view/document~Document#event:layoutChanged} and
 	 * {@link module:engine/view/document~Document#event:imageLoaded}
 	 * if observer {@link #isEnabled is enabled}.
 	 *
@@ -110,7 +110,7 @@ export default class ImageLoadObserver extends Observer {
  *
  * Introduced by {@link module:image/image/imageloadobserver~ImageLoadObserver}.
  *
- * @see image/image/imageloadobserver~ImageLoadObserver
- * @event module:engine/view/document~Document#event:imageLoaded
+ * @see module:image/image/imageloadobserver~ImageLoadObserver
+ * @event imageLoaded
  * @param {module:engine/view/observer/domeventdata~DomEventData} data Event data.
  */
diff --git a/src/imagetextalternative/imagetextalternativeui.js b/src/imagetextalternative/imagetextalternativeui.js
@@ -115,7 +115,7 @@ export default class ImageTextAlternativeUI extends Plugin {
 		} );
 
 		// Reposition the balloon or hide the form if an image widget is no longer selected.
-		this.listenTo( view, 'render', () => {
+		this.listenTo( editor.ui, 'update', () => {
 			if ( !isImageWidgetSelected( viewDocument.selection ) ) {
 				this._hideForm( true );
 			} else if ( this._isVisible ) {

diff --git a/src/imagetoolbar.js b/src/imagetoolbar.js
@@ -93,11 +93,11 @@ export default class ImageToolbar extends Plugin {
 		this._toolbar.fillFromConfig( toolbarConfig, editor.ui.componentFactory );
 
 		// Show balloon panel each time image widget is selected.
-		this.listenTo( editor.editing.view, 'render', () => {
+		this.listenTo( editor.ui, 'update', () => {
 			this._checkIsVisible();
 		} );
 
-		// There is no render method after focus is back in editor, we need to check if balloon panel should be visible.
+		// UI#update is not fired after focus is back in editor, we need to check if balloon panel should be visible.
 		this.listenTo( editor.ui.focusTracker, 'change:isFocused', () => {
 			this._checkIsVisible();
 		}, { priority: 'low' } );
@@ -111,14 +111,10 @@ export default class ImageToolbar extends Plugin {
 	_checkIsVisible() {
 		const editor = this.editor;
 
-		if ( !editor.ui.focusTracker.isFocused ) {
+		if ( !editor.ui.focusTracker.isFocused || !isImageWidgetSelected( editor.editing.view.document.selection ) ) {
 			this._hideToolbar();
 		} else {
-			if ( isImageWidgetSelected( editor.editing.view.document.selection ) ) {
-				this._showToolbar();
-			} else {
-				this._hideToolbar();
-			}
+			this._showToolbar();
 		}
 	}
 
@@ -132,14 +128,12 @@ export default class ImageToolbar extends Plugin {
 
 		if ( this._isVisible ) {
 			repositionContextualBalloon( editor );
-		} else {
-			if ( !this._balloon.hasView( this._toolbar ) ) {
-				this._balloon.add( {
-					view: this._toolbar,
-					position: getBalloonPositionData( editor ),
-					balloonClassName
-				} );
-			}
+		} else if ( !this._balloon.hasView( this._toolbar ) ) {
+			this._balloon.add( {
+				view: this._toolbar,
+				position: getBalloonPositionData( editor ),
+				balloonClassName
+			} );
 		}
 	}
 

diff --git a/tests/image/imageediting.js b/tests/image/imageediting.js
@@ -3,7 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
+/* global document, Event */
+
 import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import ImageEditing from '../../src/image/imageediting';
 import ImageLoadObserver from '../../src/image/imageloadobserver';
 import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
@@ -12,7 +15,7 @@ import { isImageWidget } from '../../src/image/utils';
 import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml';
 
 describe( 'ImageEditing', () => {
-	let editor, model, document, view, viewDocument;
+	let editor, model, doc, view, viewDocument;
 
 	beforeEach( () => {
 		return VirtualTestEditor
@@ -22,7 +25,7 @@ describe( 'ImageEditing', () => {
 			.then( newEditor => {
 				editor = newEditor;
 				model = editor.model;
-				document = model.document;
+				doc = model.document;
 				view = editor.editing.view;
 				viewDocument = view.document;
 			} );
@@ -48,6 +51,28 @@ describe( 'ImageEditing', () => {
 		expect( view.getObserver( ImageLoadObserver ) ).to.be.instanceOf( ImageLoadObserver );
 	} );
 
+	// See https://github.com/ckeditor/ckeditor5-image/issues/142.
+	it( 'should update the ui after image has been loaded in the DOM', () => {
+		const element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		ClassicTestEditor.create( element, {
+			plugins: [ ImageEditing ]
+		} ).then( editor => {
+			editor.data.set( '<figure class="image"><img src="foo.png" alt="bar" /></figure>' );
+
+			const spy = sinon.spy();
+
+			editor.ui.on( 'update', spy );
+
+			element.querySelector( 'img' ).dispatchEvent( new Event( 'load' ) );
+
+			sinon.assert.calledOnce( spy );
+
+			return editor.destroy().then( () => element.remove() );
+		} );
+	} );
+
 	describe( 'conversion in data pipeline', () => {
 		describe( 'model to view', () => {
 			it( 'should convert', () => {
@@ -120,7 +145,7 @@ describe( 'ImageEditing', () => {
 						'srcset=\'{ "foo":"bar" }\'>' +
 					'</image>' );
 
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 				model.change( writer => {
 					writer.removeAttribute( 'srcset', image );
 				} );
@@ -400,7 +425,7 @@ describe( 'ImageEditing', () => {
 
 			it( 'should convert attribute change', () => {
 				setModelData( model, '<image src="foo.png" alt="alt text"></image>' );
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 
 				model.change( writer => {
 					writer.setAttribute( 'alt', 'new text', image );
@@ -413,7 +438,7 @@ describe( 'ImageEditing', () => {
 
 			it( 'should convert attribute removal', () => {
 				setModelData( model, '<image src="foo.png" alt="alt text"></image>' );
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 
 				model.change( writer => {
 					writer.removeAttribute( 'alt', image );
@@ -425,7 +450,7 @@ describe( 'ImageEditing', () => {
 
 			it( 'should not convert change if is already consumed', () => {
 				setModelData( model, '<image src="foo.png" alt="alt text"></image>' );
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 
 				editor.editing.downcastDispatcher.on( 'attribute:alt:image', ( evt, data, conversionApi ) => {
 					conversionApi.consumable.consume( data.item, 'attribute:alt' );
@@ -463,7 +488,7 @@ describe( 'ImageEditing', () => {
 						'srcset=\'{ "foo":"bar" }\'>' +
 					'</image>' );
 
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 				model.change( writer => {
 					writer.removeAttribute( 'srcset', image );
 				} );
@@ -492,7 +517,7 @@ describe( 'ImageEditing', () => {
 
 			it( 'should remove sizes and srcsset attribute when srcset attribute is removed from model', () => {
 				setModelData( model, '<image src="foo.png" srcset=\'{ "data": "small.png 148w, big.png 1024w" }\'></image>' );
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 
 				model.change( writer => {
 					writer.removeAttribute( 'srcset', image );
@@ -512,7 +537,7 @@ describe( 'ImageEditing', () => {
 						'srcset=\'{ "data": "small.png 148w, big.png 1024w", "width": "1024" }\'>' +
 					'</image>'
 				);
-				const image = document.getRoot().getChild( 0 );
+				const image = doc.getRoot().getChild( 0 );
 
 				model.change( writer => {
 					writer.removeAttribute( 'srcset', image );

diff --git a/tests/imagetextalternative/imagetextalternativeui.js b/tests/imagetextalternative/imagetextalternativeui.js
@@ -156,14 +156,14 @@ describe( 'ImageTextAlternative', () => {
 			expect( balloon.visibleView ).to.equal( lastView );
 		} );
 
-		describe( 'integration with the editor selection (#render event)', () => {
+		describe( 'integration with the editor selection (ui#update event)', () => {
 			it( 'should re-position the form', () => {
 				setData( model, '[<image src=""></image>]' );
 				button.fire( 'execute' );
 
 				const spy = sinon.spy( balloon, 'updatePosition' );
 
-				editor.editing.view.fire( 'render' );
+				editor.ui.fire( 'update' );
 				sinon.assert.calledOnce( spy );
 			} );
 
@@ -174,11 +174,13 @@ describe( 'ImageTextAlternative', () => {
 				const removeSpy = sinon.spy( balloon, 'remove' );
 				const focusSpy = sinon.spy( editor.editing.view, 'focus' );
 
-				// EnqueueChange automatically fires #render event.
 				model.enqueueChange( 'transparent', writer => {
 					writer.remove( doc.selection.getFirstRange() );
 				} );
 
+				// Because the update event is throttled we need to "flush" it.
+				editor.ui.fire( 'update' );
+
 				sinon.assert.calledWithExactly( removeSpy, form );
 				sinon.assert.calledOnce( focusSpy );
 			} );

diff --git a/tests/imagetoolbar.js b/tests/imagetoolbar.js
@@ -17,7 +17,7 @@ import View from '@ckeditor/ckeditor5-ui/src/view';
 import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 
 describe( 'ImageToolbar', () => {
-	let editor, model, doc, editingView, plugin, toolbar, balloon, editorElement;
+	let editor, model, doc, plugin, toolbar, balloon, editorElement;
 
 	beforeEach( () => {
 		editorElement = global.document.createElement( 'div' );
@@ -36,7 +36,6 @@ describe( 'ImageToolbar', () => {
 				doc = model.document;
 				plugin = editor.plugins.get( ImageToolbar );
 				toolbar = plugin._toolbar;
-				editingView = editor.editing.view;
 				balloon = editor.plugins.get( 'ContextualBalloon' );
 			} );
 	} );
@@ -79,6 +78,9 @@ describe( 'ImageToolbar', () => {
 
 			setData( model, '[<image src=""></image>]' );
 
+			// "Flush" throttled update event.
+			editor.ui.fire( 'update' );
+
 			sinon.assert.calledWithMatch( spy, {
 				view: toolbar,
 				balloonClassName: 'ck-toolbar-container'
@@ -112,17 +114,18 @@ describe( 'ImageToolbar', () => {
 		} );
 	} );
 
-	describe( 'integration with the editor selection (#change event)', () => {
+	describe( 'integration with the editor selection', () => {
 		beforeEach( () => {
 			editor.ui.focusTracker.isFocused = true;
 		} );
 
-		it( 'should show the toolbar on render when the image is selected', () => {
+		it( 'should show the toolbar on ui#update when the image is selected', () => {
 			setData( model, '<paragraph>[foo]</paragraph><image src=""></image>' );
 
 			expect( balloon.visibleView ).to.be.null;
 
-			editingView.change( () => {} );
+			editor.ui.fire( 'update' );
+
 			expect( balloon.visibleView ).to.be.null;
 
 			model.change( writer => {
@@ -132,16 +135,22 @@ describe( 'ImageToolbar', () => {
 				);
 			} );
 
+			// "Flush" throttled update event.
+			editor.ui.fire( 'update' );
 			expect( balloon.visibleView ).to.equal( toolbar );
 
 			// Make sure successive change does not throw, e.g. attempting
 			// to insert the toolbar twice.
-			editingView.change( () => {} );
+			editor.ui.fire( 'update' );
 			expect( balloon.visibleView ).to.equal( toolbar );
 		} );
 
 		it( 'should not engage when the toolbar is in the balloon yet invisible', () => {
 			setData( model, '[<image src=""></image>]' );
+
+			// "Flush" throttled update event.
+			editor.ui.fire( 'update' );
+
 			expect( balloon.visibleView ).to.equal( toolbar );
 
 			const lastView = new View();
@@ -156,13 +165,17 @@ describe( 'ImageToolbar', () => {
 
 			expect( balloon.visibleView ).to.equal( lastView );
 
-			editingView.change( () => {} );
+			editor.ui.fire( 'update' );
+
 			expect( balloon.visibleView ).to.equal( lastView );
 		} );
 
-		it( 'should hide the toolbar on render if the image is de–selected', () => {
+		it( 'should hide the toolbar on ui#update if the image is de–selected', () => {
 			setData( model, '<paragraph>foo</paragraph>[<image src=""></image>]' );
 
+			// "Flush" throttled update event.
+			editor.ui.fire( 'update' );
+
 			expect( balloon.visibleView ).to.equal( toolbar );
 
 			model.change( writer => {
@@ -172,11 +185,13 @@ describe( 'ImageToolbar', () => {
 				);
 			} );
 
+			// "Flush" throttled update event.
+			editor.ui.fire( 'update' );
 			expect( balloon.visibleView ).to.be.null;
 
 			// Make sure successive change does not throw, e.g. attempting
 			// to remove the toolbar twice.
-			editingView.change( () => {} );
+			editor.ui.fire( 'update' );
 			expect( balloon.visibleView ).to.be.null;
 		} );
 	} );

diff --git a/tests/manual/tickets/142/1.html b/tests/manual/tickets/142/1.html
@@ -0,0 +1,6 @@
+<p><button class="start">Insert image</button></p>
+
+<div id="editor">
+	<p>lorem ipsum</p>
+	<p>foo <a href="foo">bar</a> biz</p>
+</div>