From 444b37c0f3097a2859be7cf06f21539f1b115915 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 19 Jun 2018 12:21:53 +0200 Subject: [PATCH 01/12] Feature: Introduced ImageLoadObserver. --- src/image/imageloadobserver.js | 101 +++++++++++++++++++++++++++++++ tests/image/imageloadobserver.js | 96 +++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 src/image/imageloadobserver.js create mode 100644 tests/image/imageloadobserver.js diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js new file mode 100644 index 00000000..6de9f841 --- /dev/null +++ b/src/image/imageloadobserver.js @@ -0,0 +1,101 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module image/image/imageloadobserver + */ + +import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver'; +import DOMEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; + +/** + * Observes all images added to the {@link module:engine/view/document~Document}, + * fires {@link module:engine/view/document~Document#event:imageLoad}` event and + * {@link module:engine/view/view~View#render renders} the View every time when new image + * has been loaded. + * + * **Note:** This event is not fired for images that has been added to the document as `complete` (already loaded). + * + * @extends module:engine/view/observer/domeventobserver~DomEventObserver + */ +export default class ImageLoadObserver extends DomEventObserver { + constructor( view ) { + super( view ); + + /** + * List of img DOM elements that are observed by this observer. + * + * @private + * @type {Set.} + */ + this._observedElements = new Set(); + + /** + * DOM emitter used for listening images `load` event. + * + * @private + * @type {module:utils/dom/emittermixin~DomEmitterMixin} + */ + this._domEmitter = Object.create( DOMEmitterMixin ); + } + + /** + * @inheritDoc + */ + observe( domRoot, name ) { + const viewRoot = this.document.getRoot( name ); + + viewRoot.on( 'change:children', ( evt, node ) => { + this.view.once( 'render', () => { + if ( !node.is( 'element' ) || node.is( 'attributeElement' ) ) { + return; + } + + const domNode = this.view.domConverter.mapViewToDom( node ); + + for ( const domElement of domNode.querySelectorAll( 'img' ) ) { + if ( !this._observedElements.has( domElement ) ) { + this._domEmitter.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); + this._observedElements.add( domElement ); + } + } + + // Clean up the list of observed elements from elements that has been removed from the root. + for ( const domElement of this._observedElements ) { + if ( !domRoot.contains( domElement ) ) { + this._domEmitter.stopListening( domElement ); + this._observedElements.delete( domElement ); + } + } + } ); + } ); + } + + /** + * @inheritDoc + */ + onDomEvent( domEvent ) { + this.view.render(); + this.fire( 'imageLoad', domEvent ); + } + + /** + * @inheritDoc + */ + destroy() { + this._domEmitter.stopListening(); + super.destroy(); + } +} + +/** + * Fired when img element has been loaded in one of the editable. + * + * Introduced by {@link module:image/image/imageloadobserver~ImageLoadObserver}. + * + * @see image/image/imageloadobserver~ImageLoadObserver + * @event module:engine/view/document~Document#event:imageLoad + * @param {module:engine/view/observer/domeventdata~DomEventData} data Event data. + */ diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js new file mode 100644 index 00000000..6ee39c81 --- /dev/null +++ b/tests/image/imageloadobserver.js @@ -0,0 +1,96 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/* globals document, Event */ + +import ImageLoadObserver from '../../src/image/imageloadobserver'; +import View from '@ckeditor/ckeditor5-engine/src/view/view'; +import Position from '@ckeditor/ckeditor5-engine/src/view/position'; +import Range from '@ckeditor/ckeditor5-engine/src/view/range'; +import createViewRoot from '@ckeditor/ckeditor5-engine/tests/view/_utils/createroot'; +import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; + +describe( 'ClickObserver', () => { + let view, viewDocument, observer, domRoot, viewRoot; + + beforeEach( () => { + view = new View(); + viewDocument = view.document; + observer = view.addObserver( ImageLoadObserver ); + + viewRoot = createViewRoot( viewDocument ); + domRoot = document.createElement( 'div' ); + view.attachDomRoot( domRoot ); + } ); + + afterEach( () => { + view.destroy(); + } ); + + it( 'should fire `loadImage` event for images in the document that are loaded with a delay', () => { + const spy = sinon.spy(); + + viewDocument.on( 'imageLoad', spy ); + + setData( view, '' ); + + sinon.assert.notCalled( spy ); + + domRoot.querySelector( 'img' ).dispatchEvent( new Event( 'load' ) ); + + sinon.assert.calledOnce( spy ); + } ); + + it( 'should not fire `loadImage` event for images removed from document', () => { + const spy = sinon.spy(); + + viewDocument.on( 'imageLoad', spy ); + + setData( view, '' ); + + sinon.assert.notCalled( spy ); + + const img = domRoot.querySelector( 'img' ); + + setData( view, '' ); + + img.dispatchEvent( new Event( 'load' ) ); + + sinon.assert.notCalled( spy ); + } ); + + it( 'should do nothing with an image when changes are in the other parent', () => { + setData( view, 'foo' ); + + const viewP = viewRoot.getChild( 0 ); + const viewDiv = viewRoot.getChild( 1 ); + + const mapSpy = sinon.spy( view.domConverter, 'mapViewToDom' ); + + // Change only the paragraph. + view.change( writer => { + const text = writer.createText( 'foo', { b: true } ); + + writer.insert( Position.createAt( viewRoot.getChild( 0 ).getChild( 0 ) ), text ); + writer.wrap( Range.createOn( text ), writer.createAttributeElement( 'b' ) ); + } ); + + sinon.assert.calledWith( mapSpy, viewP ); + sinon.assert.neverCalledWith( mapSpy, viewDiv ); + } ); + + it( 'should call `view#render` along with `imageLoad` event', () => { + const renderSpy = sinon.spy(); + const imageLoadSpy = sinon.spy(); + + view.on( 'render', renderSpy ); + view.document.on( 'imageLoad', imageLoadSpy ); + + observer.onDomEvent( {} ); + + sinon.assert.calledOnce( renderSpy ); + sinon.assert.calledOnce( imageLoadSpy ); + } ); +} ); From 2758893f9fc704cf14f70c3eb744ae8da69a53d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 19 Jun 2018 12:25:50 +0200 Subject: [PATCH 02/12] Registered ImageLoadObserver. --- src/image/imageediting.js | 4 ++++ tests/image/imageediting.js | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/src/image/imageediting.js b/src/image/imageediting.js index 77e6d781..c4067190 100644 --- a/src/image/imageediting.js +++ b/src/image/imageediting.js @@ -8,6 +8,7 @@ */ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; +import ImageLoadObserver from './imageloadobserver'; import { viewFigureToModel, @@ -39,6 +40,9 @@ export default class ImageEditing extends Plugin { const t = editor.t; const conversion = editor.conversion; + // See https://github.com/ckeditor/ckeditor5-image/issues/142. + editor.editing.view.addObserver( ImageLoadObserver ); + // Configure schema. schema.register( 'image', { isObject: true, diff --git a/tests/image/imageediting.js b/tests/image/imageediting.js index c715f7df..30fc34a7 100644 --- a/tests/image/imageediting.js +++ b/tests/image/imageediting.js @@ -5,6 +5,7 @@ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; 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'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import { isImageWidget } from '../../src/image/utils'; @@ -43,6 +44,10 @@ describe( 'ImageEditing', () => { expect( model.schema.checkChild( [ '$root', '$block' ], 'image' ) ).to.be.false; } ); + it( 'should register ImageLoadObserver', () => { + expect( view.getObserver( ImageLoadObserver ) ).to.be.instanceOf( ImageLoadObserver ); + } ); + describe( 'conversion in data pipeline', () => { describe( 'model to view', () => { it( 'should convert', () => { From 98c48bc684777e009b518eb4a8fe1cda850641b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 22 Jun 2018 14:42:53 +0200 Subject: [PATCH 03/12] Renamed imageLoad event to imageLoaded. --- src/image/imageloadobserver.js | 6 +++--- tests/image/imageloadobserver.js | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 6de9f841..5337a2fa 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -12,7 +12,7 @@ import DOMEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /** * Observes all images added to the {@link module:engine/view/document~Document}, - * fires {@link module:engine/view/document~Document#event:imageLoad}` event and + * fires {@link module:engine/view/document~Document#event:imageLoaded}` event and * {@link module:engine/view/view~View#render renders} the View every time when new image * has been loaded. * @@ -78,7 +78,7 @@ export default class ImageLoadObserver extends DomEventObserver { */ onDomEvent( domEvent ) { this.view.render(); - this.fire( 'imageLoad', domEvent ); + this.fire( 'imageLoaded', domEvent ); } /** @@ -96,6 +96,6 @@ export default class ImageLoadObserver extends DomEventObserver { * Introduced by {@link module:image/image/imageloadobserver~ImageLoadObserver}. * * @see image/image/imageloadobserver~ImageLoadObserver - * @event module:engine/view/document~Document#event:imageLoad + * @event module:engine/view/document~Document#event:imageLoaded * @param {module:engine/view/observer/domeventdata~DomEventData} data Event data. */ diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js index 6ee39c81..43609e22 100644 --- a/tests/image/imageloadobserver.js +++ b/tests/image/imageloadobserver.js @@ -32,7 +32,7 @@ describe( 'ClickObserver', () => { it( 'should fire `loadImage` event for images in the document that are loaded with a delay', () => { const spy = sinon.spy(); - viewDocument.on( 'imageLoad', spy ); + viewDocument.on( 'imageLoaded', spy ); setData( view, '' ); @@ -46,7 +46,7 @@ describe( 'ClickObserver', () => { it( 'should not fire `loadImage` event for images removed from document', () => { const spy = sinon.spy(); - viewDocument.on( 'imageLoad', spy ); + viewDocument.on( 'imageLoaded', spy ); setData( view, '' ); @@ -81,16 +81,16 @@ describe( 'ClickObserver', () => { sinon.assert.neverCalledWith( mapSpy, viewDiv ); } ); - it( 'should call `view#render` along with `imageLoad` event', () => { + it( 'should call `view#render` along with `imageLoaded` event', () => { const renderSpy = sinon.spy(); - const imageLoadSpy = sinon.spy(); + const imageLoadedSpy = sinon.spy(); view.on( 'render', renderSpy ); - view.document.on( 'imageLoad', imageLoadSpy ); + view.document.on( 'imageLoaded', imageLoadedSpy ); observer.onDomEvent( {} ); sinon.assert.calledOnce( renderSpy ); - sinon.assert.calledOnce( imageLoadSpy ); + sinon.assert.calledOnce( imageLoadedSpy ); } ); } ); From ad00dd727f2de409e7fafb9dd69cbd30aa94e926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 22 Jun 2018 14:45:20 +0200 Subject: [PATCH 04/12] Docs: Minor change. --- src/image/imageloadobserver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 5337a2fa..0f68cb81 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -91,7 +91,7 @@ export default class ImageLoadObserver extends DomEventObserver { } /** - * Fired when img element has been loaded in one of the editable. + * Fired when an DOM element has been loaded in the DOM root. * * Introduced by {@link module:image/image/imageloadobserver~ImageLoadObserver}. * From 261f1327091570d4641c483b5d50258077013739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 22 Jun 2018 14:48:10 +0200 Subject: [PATCH 05/12] Fired document#layoutChanged instead of rendering the view. --- src/image/imageloadobserver.js | 2 +- tests/image/imageloadobserver.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 0f68cb81..c414818e 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -77,7 +77,7 @@ export default class ImageLoadObserver extends DomEventObserver { * @inheritDoc */ onDomEvent( domEvent ) { - this.view.render(); + this.document.fire( 'layoutChanged' ); this.fire( 'imageLoaded', domEvent ); } diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js index 43609e22..717094d7 100644 --- a/tests/image/imageloadobserver.js +++ b/tests/image/imageloadobserver.js @@ -81,16 +81,16 @@ describe( 'ClickObserver', () => { sinon.assert.neverCalledWith( mapSpy, viewDiv ); } ); - it( 'should call `view#render` along with `imageLoaded` event', () => { - const renderSpy = sinon.spy(); + it( 'should call `layoutChanged` along with `imageLoaded` event', () => { + const layoutChangedSpy = sinon.spy(); const imageLoadedSpy = sinon.spy(); - view.on( 'render', renderSpy ); + view.document.on( 'layoutChanged', layoutChangedSpy ); view.document.on( 'imageLoaded', imageLoadedSpy ); observer.onDomEvent( {} ); - sinon.assert.calledOnce( renderSpy ); + sinon.assert.calledOnce( layoutChangedSpy ); sinon.assert.calledOnce( imageLoadedSpy ); } ); } ); From 8300dc0f4d55242dec96484dae882336005e8917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 25 Jun 2018 16:52:17 +0200 Subject: [PATCH 06/12] Docs: Some improvements. --- src/image/imageloadobserver.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index c414818e..0b193922 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -11,12 +11,12 @@ import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domev import DOMEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /** - * Observes all images added to the {@link module:engine/view/document~Document}, - * fires {@link module:engine/view/document~Document#event:imageLoaded}` event and - * {@link module:engine/view/view~View#render renders} the View every time when new image + * 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 * has been loaded. * - * **Note:** This event is not fired for images that has been added to the document as `complete` (already loaded). + * **Note:** This event is not fired for images that has been added to the document and rendered as `complete` (already loaded). * * @extends module:engine/view/observer/domeventobserver~DomEventObserver */ From 79f37d453050d3de1a752f8625a244ce2547a7cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 25 Jun 2018 18:23:13 +0200 Subject: [PATCH 07/12] Fixed bug when try to map element that is removed from DOM. --- src/image/imageloadobserver.js | 5 +++++ tests/image/imageloadobserver.js | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 0b193922..73aa37a7 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -55,6 +55,11 @@ export default class ImageLoadObserver extends DomEventObserver { const domNode = this.view.domConverter.mapViewToDom( node ); + // If there is no `domNode` it means that it was removed from the DOM in the meanwhile. + if ( !domNode ) { + return; + } + for ( const domElement of domNode.querySelectorAll( 'img' ) ) { if ( !this._observedElements.has( domElement ) ) { this._domEmitter.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js index 717094d7..c7a5dabd 100644 --- a/tests/image/imageloadobserver.js +++ b/tests/image/imageloadobserver.js @@ -81,6 +81,22 @@ describe( 'ClickObserver', () => { sinon.assert.neverCalledWith( mapSpy, viewDiv ); } ); + it( 'should not throw when synced child was removed in the meanwhile', () => { + let viewDiv; + + const mapSpy = sinon.spy( view.domConverter, 'mapViewToDom' ); + + view.change( writer => { + viewDiv = writer.createContainerElement( 'div' ); + viewRoot.fire( 'change:children', viewDiv ); + } ); + + expect( () => { + view._renderer.render(); + sinon.assert.calledWith( mapSpy, viewDiv ); + } ).to.not.throw(); + } ); + it( 'should call `layoutChanged` along with `imageLoaded` event', () => { const layoutChangedSpy = sinon.spy(); const imageLoadedSpy = sinon.spy(); From ec5dc6bb234365b86349f12b475e50cd8ee8ca81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 25 Jun 2018 18:50:11 +0200 Subject: [PATCH 08/12] Cleared collection with observed elements on observer destroy. --- src/image/imageloadobserver.js | 1 + tests/image/imageloadobserver.js | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 73aa37a7..a393d914 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -91,6 +91,7 @@ export default class ImageLoadObserver extends DomEventObserver { */ destroy() { this._domEmitter.stopListening(); + this._observedElements.clear(); super.destroy(); } } diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js index c7a5dabd..34243280 100644 --- a/tests/image/imageloadobserver.js +++ b/tests/image/imageloadobserver.js @@ -109,4 +109,18 @@ describe( 'ClickObserver', () => { sinon.assert.calledOnce( layoutChangedSpy ); sinon.assert.calledOnce( imageLoadedSpy ); } ); + + it( 'should stop observing images on destroy', () => { + const spy = sinon.spy(); + + viewDocument.on( 'imageLoaded', spy ); + + setData( view, '' ); + + observer.destroy(); + + domRoot.querySelector( 'img' ).dispatchEvent( new Event( 'load' ) ); + + sinon.assert.notCalled( spy ); + } ); } ); From b139df71ac269a2bf4e753dab0b26ffc5ce590b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 26 Jun 2018 14:04:33 +0200 Subject: [PATCH 09/12] Renamed _domEmitter property to _domObserver. --- src/image/imageloadobserver.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index a393d914..944d465f 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -38,7 +38,7 @@ export default class ImageLoadObserver extends DomEventObserver { * @private * @type {module:utils/dom/emittermixin~DomEmitterMixin} */ - this._domEmitter = Object.create( DOMEmitterMixin ); + this._domObserver = Object.create( DOMEmitterMixin ); } /** @@ -62,7 +62,7 @@ export default class ImageLoadObserver extends DomEventObserver { for ( const domElement of domNode.querySelectorAll( 'img' ) ) { if ( !this._observedElements.has( domElement ) ) { - this._domEmitter.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); + this._domObserver.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); this._observedElements.add( domElement ); } } @@ -70,7 +70,7 @@ export default class ImageLoadObserver extends DomEventObserver { // Clean up the list of observed elements from elements that has been removed from the root. for ( const domElement of this._observedElements ) { if ( !domRoot.contains( domElement ) ) { - this._domEmitter.stopListening( domElement ); + this._domObserver.stopListening( domElement ); this._observedElements.delete( domElement ); } } @@ -90,7 +90,7 @@ export default class ImageLoadObserver extends DomEventObserver { * @inheritDoc */ destroy() { - this._domEmitter.stopListening(); + this._domObserver.stopListening(); this._observedElements.clear(); super.destroy(); } From 96455860f2c71d724e68622cab84ad23f7eb6951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 26 Jun 2018 14:19:47 +0200 Subject: [PATCH 10/12] Improved code readability. --- src/image/imageloadobserver.js | 60 ++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index 944d465f..bea40a75 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -47,35 +47,47 @@ export default class ImageLoadObserver extends DomEventObserver { observe( domRoot, name ) { const viewRoot = this.document.getRoot( name ); + // When there is a change in one of the view element + // we need to check if there are any new `` elements to observe. viewRoot.on( 'change:children', ( evt, node ) => { - this.view.once( 'render', () => { - if ( !node.is( 'element' ) || node.is( 'attributeElement' ) ) { - return; - } + // Wait for the render to be sure that `` elements are rendered in the DOM root. + this.view.once( 'render', () => this._updateObservedElements( domRoot, node ) ); + } ); + } + + /** + * Updates the list of observed `` elements. + * + * @private + * @param {HTMLElement} domRoot DOM root element. + * @param {module:engine/view/element~Element} viewNode View element where children have changed. + */ + _updateObservedElements( domRoot, viewNode ) { + if ( !viewNode.is( 'element' ) || viewNode.is( 'attributeElement' ) ) { + return; + } - const domNode = this.view.domConverter.mapViewToDom( node ); + const domNode = this.view.domConverter.mapViewToDom( viewNode ); - // If there is no `domNode` it means that it was removed from the DOM in the meanwhile. - if ( !domNode ) { - return; - } + // If there is no `domNode` it means that it was removed from the DOM in the meanwhile. + if ( !domNode ) { + return; + } - for ( const domElement of domNode.querySelectorAll( 'img' ) ) { - if ( !this._observedElements.has( domElement ) ) { - this._domObserver.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); - this._observedElements.add( domElement ); - } - } + for ( const domElement of domNode.querySelectorAll( 'img' ) ) { + if ( !this._observedElements.has( domElement ) ) { + this._domObserver.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); + this._observedElements.add( domElement ); + } + } - // Clean up the list of observed elements from elements that has been removed from the root. - for ( const domElement of this._observedElements ) { - if ( !domRoot.contains( domElement ) ) { - this._domObserver.stopListening( domElement ); - this._observedElements.delete( domElement ); - } - } - } ); - } ); + // Clean up the list of observed elements from elements that has been removed from the root. + for ( const domElement of this._observedElements ) { + if ( !domRoot.contains( domElement ) ) { + this._domObserver.stopListening( domElement ); + this._observedElements.delete( domElement ); + } + } } /** From e09343dcab718f65d8d426b360ba04e80a6350c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 26 Jun 2018 16:19:28 +0200 Subject: [PATCH 11/12] Used domEmitter interface instead of creating new instance. --- src/image/imageloadobserver.js | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index bea40a75..e4d242e7 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -8,7 +8,6 @@ */ import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver'; -import DOMEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /** * Observes all new images added to the {@link module:engine/view/document~Document}, @@ -31,14 +30,6 @@ export default class ImageLoadObserver extends DomEventObserver { * @type {Set.} */ this._observedElements = new Set(); - - /** - * DOM emitter used for listening images `load` event. - * - * @private - * @type {module:utils/dom/emittermixin~DomEmitterMixin} - */ - this._domObserver = Object.create( DOMEmitterMixin ); } /** @@ -76,7 +67,7 @@ export default class ImageLoadObserver extends DomEventObserver { for ( const domElement of domNode.querySelectorAll( 'img' ) ) { if ( !this._observedElements.has( domElement ) ) { - this._domObserver.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); + this.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); this._observedElements.add( domElement ); } } @@ -84,7 +75,7 @@ export default class ImageLoadObserver extends DomEventObserver { // Clean up the list of observed elements from elements that has been removed from the root. for ( const domElement of this._observedElements ) { if ( !domRoot.contains( domElement ) ) { - this._domObserver.stopListening( domElement ); + this.stopListening( domElement ); this._observedElements.delete( domElement ); } } @@ -102,7 +93,6 @@ export default class ImageLoadObserver extends DomEventObserver { * @inheritDoc */ destroy() { - this._domObserver.stopListening(); this._observedElements.clear(); super.destroy(); } From a1ed796dc606c4566be343bb27832576c60176aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 26 Jun 2018 16:53:50 +0200 Subject: [PATCH 12/12] Changed ImageLoadObserver from DomEventObserver to Observer. --- src/image/imageloadobserver.js | 23 +++++++++------ tests/image/imageloadobserver.js | 48 ++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/image/imageloadobserver.js b/src/image/imageloadobserver.js index e4d242e7..c7e4f66e 100644 --- a/src/image/imageloadobserver.js +++ b/src/image/imageloadobserver.js @@ -7,7 +7,7 @@ * @module image/image/imageloadobserver */ -import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver'; +import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer'; /** * Observes all new images added to the {@link module:engine/view/document~Document}, @@ -17,9 +17,9 @@ import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domev * * **Note:** This event is not fired for images that has been added to the document and rendered as `complete` (already loaded). * - * @extends module:engine/view/observer/domeventobserver~DomEventObserver + * @extends module:engine/view/observer/observer~Observer */ -export default class ImageLoadObserver extends DomEventObserver { +export default class ImageLoadObserver extends Observer { constructor( view ) { super( view ); @@ -67,7 +67,7 @@ export default class ImageLoadObserver extends DomEventObserver { for ( const domElement of domNode.querySelectorAll( 'img' ) ) { if ( !this._observedElements.has( domElement ) ) { - this.listenTo( domElement, 'load', ( evt, domEvt ) => this.onDomEvent( domEvt ) ); + this.listenTo( domElement, 'load', ( evt, domEvt ) => this._fireEvents( domEvt ) ); this._observedElements.add( domElement ); } } @@ -82,11 +82,18 @@ export default class ImageLoadObserver extends DomEventObserver { } /** - * @inheritDoc + * Fires {@link module:engine/view/view/document~Document#event:layoutChanged} and + * {@link module:engine/view/document~Document#event:imageLoaded} + * if observer {@link #isEnabled is enabled}. + * + * @protected + * @param {Event} domEvent The DOM event. */ - onDomEvent( domEvent ) { - this.document.fire( 'layoutChanged' ); - this.fire( 'imageLoaded', domEvent ); + _fireEvents( domEvent ) { + if ( this.isEnabled ) { + this.document.fire( 'layoutChanged' ); + this.document.fire( 'imageLoaded', domEvent ); + } } /** diff --git a/tests/image/imageloadobserver.js b/tests/image/imageloadobserver.js index 34243280..75889b3f 100644 --- a/tests/image/imageloadobserver.js +++ b/tests/image/imageloadobserver.js @@ -6,13 +6,14 @@ /* globals document, Event */ import ImageLoadObserver from '../../src/image/imageloadobserver'; +import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer'; import View from '@ckeditor/ckeditor5-engine/src/view/view'; import Position from '@ckeditor/ckeditor5-engine/src/view/position'; import Range from '@ckeditor/ckeditor5-engine/src/view/range'; import createViewRoot from '@ckeditor/ckeditor5-engine/tests/view/_utils/createroot'; import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; -describe( 'ClickObserver', () => { +describe( 'ImageLoadObserver', () => { let view, viewDocument, observer, domRoot, viewRoot; beforeEach( () => { @@ -29,6 +30,10 @@ describe( 'ClickObserver', () => { view.destroy(); } ); + it( 'should extend Observer', () => { + expect( observer ).instanceof( Observer ); + } ); + it( 'should fire `loadImage` event for images in the document that are loaded with a delay', () => { const spy = sinon.spy(); @@ -43,6 +48,34 @@ describe( 'ClickObserver', () => { sinon.assert.calledOnce( spy ); } ); + it( 'should fire `layoutChanged` along with `imageLoaded` event', () => { + const layoutChangedSpy = sinon.spy(); + const imageLoadedSpy = sinon.spy(); + + view.document.on( 'layoutChanged', layoutChangedSpy ); + view.document.on( 'imageLoaded', imageLoadedSpy ); + + observer._fireEvents( {} ); + + sinon.assert.calledOnce( layoutChangedSpy ); + sinon.assert.calledOnce( imageLoadedSpy ); + } ); + + it( 'should not fire events when observer is disabled', () => { + const layoutChangedSpy = sinon.spy(); + const imageLoadedSpy = sinon.spy(); + + view.document.on( 'layoutChanged', layoutChangedSpy ); + view.document.on( 'imageLoaded', imageLoadedSpy ); + + observer.isEnabled = false; + + observer._fireEvents( {} ); + + sinon.assert.notCalled( layoutChangedSpy ); + sinon.assert.notCalled( imageLoadedSpy ); + } ); + it( 'should not fire `loadImage` event for images removed from document', () => { const spy = sinon.spy(); @@ -97,19 +130,6 @@ describe( 'ClickObserver', () => { } ).to.not.throw(); } ); - it( 'should call `layoutChanged` along with `imageLoaded` event', () => { - const layoutChangedSpy = sinon.spy(); - const imageLoadedSpy = sinon.spy(); - - view.document.on( 'layoutChanged', layoutChangedSpy ); - view.document.on( 'imageLoaded', imageLoadedSpy ); - - observer.onDomEvent( {} ); - - sinon.assert.calledOnce( layoutChangedSpy ); - sinon.assert.calledOnce( imageLoadedSpy ); - } ); - it( 'should stop observing images on destroy', () => { const spy = sinon.spy();