Merge pull request #215 from ckeditor/t/142

Fix: The UI should update once the image is loaded. Closes #142. 

Used the `EditorUI#update` event instead of `View#render` to attach the UI components (see ckeditor/ckeditor5-core#130).

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
+ * @see module:image/image/imageloadobserver~ImageLoadObserver
  * @event module:engine/view/document~Document#event:imageLoaded
  * @param {module:engine/view/observer/domeventdata~DomEventData} data Event data.
  */

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 ) {

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
+			} );
 		}
 	}
 

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 );

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,7 +174,6 @@ 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() );
 				} );

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' );
 			} );
 	} );
@@ -112,17 +111,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 => {
@@ -136,12 +136,13 @@ describe( 'ImageToolbar', () => {
 
 			// 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>]' );
+
 			expect( balloon.visibleView ).to.equal( toolbar );
 
 			const lastView = new View();
@@ -156,11 +157,12 @@ 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>]' );
 
 			expect( balloon.visibleView ).to.equal( toolbar );
@@ -176,7 +178,7 @@ describe( 'ImageToolbar', () => {
 
 			// 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;
 		} );
 	} );

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>

tests/manual/tickets/142/1.js

@@ -0,0 +1,43 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* global document, console, window, setTimeout */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import Enter from '@ckeditor/ckeditor5-enter/src/enter';
+import Typing from '@ckeditor/ckeditor5-typing/src/typing';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Link from '@ckeditor/ckeditor5-link/src/link';
+import BlockToolbar from '@ckeditor/ckeditor5-ui/src/toolbar/block/blocktoolbar';
+import Image from '../../../../src/image';
+import ImageCaption from '../../../../src/imagecaption';
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ Enter, Typing, Paragraph, Link, Image, ImageCaption, BlockToolbar ],
+		toolbar: [],
+		blockToolbar: [ 'Link' ]
+	} )
+	.then( editor => {
+		window.editor = editor;
+
+		const doc = editor.model.document;
+
+		document.querySelector( '.start' ).addEventListener( 'click', () => {
+			let image;
+
+			editor.model.change( writer => {
+				image = writer.createElement( 'image', { src: 'sample-small.jpg' } );
+				writer.insert( image, doc.getRoot().getChild( 0 ), 'after' );
+			} );
+
+			setTimeout( () => {
+				editor.ui.view.element.querySelector( 'img' ).src = '../../sample.jpg';
+			}, 3000 );
+		} );
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

tests/manual/tickets/142/1.md

@@ -0,0 +1,4 @@
+### Update the UI after image load [#142](https://github.com/ckeditor/ckeditor5-image/issues/142)
+
+1. Click **Insert image** then quickly select the link in the content (the link editing balloon should show up).
+2. Observe if the link balloon and block toolbar remains properly positioned after replacing image source to the biggest one (replacing source is made using DOM API).