From e3c567857a8391cf1843e291b55d60eabac25895 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 17 Sep 2020 15:58:33 +0200 Subject: [PATCH 01/23] Internal (image): Dirty PoC to mitigate image resize wrapper flicker. There's some extra code that I'll be removing now. --- .../src/imageresize/imageresizehandles.js | 49 ++++++++++++++++++- .../ckeditor5-image/theme/imageresize.css | 15 ++++++ packages/ckeditor5-widget/theme/widget.css | 2 +- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index c5e1725c60f..c01ed8cb27f 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -9,6 +9,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import WidgetResize from '@ckeditor/ckeditor5-widget/src/widgetresize'; +import ImageLoadObserver from './../image/imageloadobserver'; /** * The image resize by handles feature. @@ -39,12 +40,20 @@ export default class ImageResizeHandles extends Plugin { init() { const editor = this.editor; const command = editor.commands.get( 'imageResize' ); + const editingView = editor.editing.view; + + editingView.addObserver( ImageLoadObserver ); this.bind( 'isEnabled' ).to( command ); editor.editing.downcastDispatcher.on( 'insert:image', ( evt, data, conversionApi ) => { const widget = conversionApi.mapper.toViewElement( data.item ); + // @todo: check if can be cleaned up + editingView.change( writer => { + writer.addClass( 'image_resizer_loading', widget ); + } ); + const resizer = editor.plugins .get( WidgetResize ) .attachTo( { @@ -74,13 +83,51 @@ export default class ImageResizeHandles extends Plugin { resizer.on( 'updateSize', () => { if ( !widget.hasClass( 'image_resized' ) ) { - editor.editing.view.change( writer => { + editingView.change( writer => { writer.addClass( 'image_resized', widget ); } ); } } ); + this._hideResizerUntilImageIsLoaded( resizer, widget ); + resizer.bind( 'isEnabled' ).to( this ); }, { priority: 'low' } ); } + + /** + * @private + * @param {*} widget + */ + _hideResizerUntilImageIsLoaded( resizer, widget ) { + // Mitigation logic for #8088 (which is caused by a more complex problem #7548). + const editingView = this.editor.editing.view; + + editingView.change( writer => { + writer.addClass( 'image_resizer_loading', widget ); + } ); + + editingView.document.on( 'imageLoaded', imageLoadCallback ); + + function imageLoadCallback( evt, domEvent ) { + const handleHost = resizer._getHandleHost(); + + if ( domEvent.target.isSameNode( handleHost ) ) { + editingView.change( writer => { + writer.removeClass( 'image_resizer_loading', widget ); + resizer.redraw(); + writer.addClass( 'image_resizer_loaded', widget ); + } ); + + editingView.document.off( 'imageLoaded', imageLoadCallback ); // Listener is no longer needed. + } + } + + // setTimeout( () => { + // console.log( 'timeout1' ); + // setTimeout( () => { + // console.log( 'timeout2' ); + // }, 0 ); + // }, 0 ); + } } diff --git a/packages/ckeditor5-image/theme/imageresize.css b/packages/ckeditor5-image/theme/imageresize.css index 4c9c336c5e5..3ccc9b22651 100644 --- a/packages/ckeditor5-image/theme/imageresize.css +++ b/packages/ckeditor5-image/theme/imageresize.css @@ -24,6 +24,21 @@ } } +/* .ck-content .image .ck-widget__resizer { +} */ + +.ck-focused .ck-widget_with-resizer.ck-widget_selected.image > .ck-widget__resizer { + display: none; +} + +.ck-focused .ck-widget_with-resizer.ck-widget_selected.image.image_resizer_loaded > .ck-widget__resizer { + display: block; +} + +/* .ck-content .image.image_resizer_loaded .ck-widget__resizer { + display: block; +} */ + [dir="ltr"] .ck.ck-button.ck-button_with-text.ck-resize-image-button .ck-button__icon { margin-right: var(--ck-spacing-standard); } diff --git a/packages/ckeditor5-widget/theme/widget.css b/packages/ckeditor5-widget/theme/widget.css index d79e092a6ab..349a5a56071 100644 --- a/packages/ckeditor5-widget/theme/widget.css +++ b/packages/ckeditor5-widget/theme/widget.css @@ -5,7 +5,7 @@ :root { --ck-color-resizer: var(--ck-color-focus-border); - --ck-resizer-size: 10px; + --ck-resizer-size: 80px; --ck-resizer-border-width: 1px; --ck-resizer-border-radius: 2px; From 196db1d378d3d2b7bf24b6b01937349a6f0244ab Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 10:00:02 +0200 Subject: [PATCH 02/23] Internal (image): Minor cleanup of new image resizer logic. --- .../src/imageresize/imageresizehandles.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index c01ed8cb27f..b169898f23d 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -97,7 +97,8 @@ export default class ImageResizeHandles extends Plugin { /** * @private - * @param {*} widget + * @param {module:widget/widgetresize/resizer~Resizer} resizer + * @param {module:engine/view/containerelement~ContainerElement} widget */ _hideResizerUntilImageIsLoaded( resizer, widget ) { // Mitigation logic for #8088 (which is caused by a more complex problem #7548). @@ -114,20 +115,13 @@ export default class ImageResizeHandles extends Plugin { if ( domEvent.target.isSameNode( handleHost ) ) { editingView.change( writer => { - writer.removeClass( 'image_resizer_loading', widget ); resizer.redraw(); - writer.addClass( 'image_resizer_loaded', widget ); + writer.removeClass( 'image_resizer_loading', widget ); } ); - editingView.document.off( 'imageLoaded', imageLoadCallback ); // Listener is no longer needed. + // Remove image load listener for optimization, as it is no longer needed. + editingView.document.off( 'imageLoaded', imageLoadCallback ); } } - - // setTimeout( () => { - // console.log( 'timeout1' ); - // setTimeout( () => { - // console.log( 'timeout2' ); - // }, 0 ); - // }, 0 ); } } From 662aa04722ef177533b085d732a247bb79257ae1 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 10:42:53 +0200 Subject: [PATCH 03/23] Tests (image): Added unit test coverage. --- .../tests/imageresize/imageresizehandles.js | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 833c8f120fe..20c4c773c5f 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -238,6 +238,64 @@ describe( 'ImageResizeHandles', () => { } ); } ); + describe( 'image load detection', () => { + it( 'loading image is marked with a proper class', async () => { + editor = await createEditor(); + + setData( editor.model, `[]` ); + + const widget = viewDocument.getRoot().getChild( 0 ); + const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; + const image = domWidget.querySelector( 'img' ); + + // Before image is loaded there should be the loading class. + expect( Array.from( widget.getClassNames() ) ).to.include( 'image_resizer_loading' ); + + viewDocument.fire( 'imageLoaded', { target: image } ); + + // Now class should be removed. + expect( Array.from( widget.getClassNames() ) ).to.not.include( 'image_resizer_loading' ); + } ); + + it( 'loaded image has the loading class removed', async () => { + editor = await createEditor(); + + setData( editor.model, `[]` ); + + const widget = viewDocument.getRoot().getChild( 0 ); + const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; + const image = domWidget.querySelector( 'img' ); + + viewDocument.fire( 'imageLoaded', { target: image } ); + + // Now the class should be removed. + expect( Array.from( widget.getClassNames() ) ).to.not.include( 'image_resizer_loading' ); + } ); + + it( 'loading another image doesn\'t affect another', async () => { + editor = await createEditor(); + + setData( editor.model, `[]` ); + + const widgets = { + loading: viewDocument.getRoot().getChild( 0 ), + finished: viewDocument.getRoot().getChild( 1 ) + }; + + const domWidgets = { + loading: getWidgetDomParts( editor, widgets.loading, 'bottom-right' ).widget, + finished: getWidgetDomParts( editor, widgets.finished, 'bottom-right' ).widget + }; + + // Marking other image as loaded. + const finishedImage = domWidgets.finished.querySelector( 'img' ); + viewDocument.fire( 'imageLoaded', { target: finishedImage } ); + + // But the former is still considered as not loaded. + expect( Array.from( widgets.loading.getClassNames() ) ).to.include( 'image_resizer_loading' ); + } ); + } ); + describe( 'srcset integration', () => { // The image is 96x96 pixels. const imageBaseUrl = '/assets/sample.png'; From be992f62dab2d87b0badf25fb55146299543bdd3 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 10:43:48 +0200 Subject: [PATCH 04/23] Internal (image): Removed superfluous code. --- .../ckeditor5-image/src/imageresize/imageresizehandles.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index b169898f23d..4888e1f006b 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -49,11 +49,6 @@ export default class ImageResizeHandles extends Plugin { editor.editing.downcastDispatcher.on( 'insert:image', ( evt, data, conversionApi ) => { const widget = conversionApi.mapper.toViewElement( data.item ); - // @todo: check if can be cleaned up - editingView.change( writer => { - writer.addClass( 'image_resizer_loading', widget ); - } ); - const resizer = editor.plugins .get( WidgetResize ) .attachTo( { From e7d2cba4501da0a497304ed9a2cb13e6bff35ea4 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 12:55:08 +0200 Subject: [PATCH 05/23] Tests (image): Fixed integration test with undo. --- .../tests/imageresize/imageresizehandles.js | 4 ++++ packages/ckeditor5-image/theme/imageresize.css | 13 +------------ 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 20c4c773c5f..fd2655eb045 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -181,6 +181,10 @@ describe( 'ImageResizeHandles', () => { setData( editor.model, `foo[]` ); widget = viewDocument.getRoot().getChild( 1 ); + + const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; + + viewDocument.fire( 'imageLoaded', { target: domWidget.querySelector( 'img' ) } ); } ); it( 'has correct border size after undo', async () => { diff --git a/packages/ckeditor5-image/theme/imageresize.css b/packages/ckeditor5-image/theme/imageresize.css index 3ccc9b22651..fea881a964f 100644 --- a/packages/ckeditor5-image/theme/imageresize.css +++ b/packages/ckeditor5-image/theme/imageresize.css @@ -24,21 +24,10 @@ } } -/* .ck-content .image .ck-widget__resizer { -} */ - -.ck-focused .ck-widget_with-resizer.ck-widget_selected.image > .ck-widget__resizer { +.ck-focused .ck-widget_with-resizer.ck-widget_selected.image.image_resizer_loading > .ck-widget__resizer { display: none; } -.ck-focused .ck-widget_with-resizer.ck-widget_selected.image.image_resizer_loaded > .ck-widget__resizer { - display: block; -} - -/* .ck-content .image.image_resizer_loaded .ck-widget__resizer { - display: block; -} */ - [dir="ltr"] .ck.ck-button.ck-button_with-text.ck-resize-image-button .ck-button__icon { margin-right: var(--ck-spacing-standard); } From bf97d151e35caf9f19f3811a80fc4d7f5b525a46 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 12:56:18 +0200 Subject: [PATCH 06/23] Internal (ckeditor5): Undo unnecessary style change. --- packages/ckeditor5-widget/theme/widget.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-widget/theme/widget.css b/packages/ckeditor5-widget/theme/widget.css index 349a5a56071..d79e092a6ab 100644 --- a/packages/ckeditor5-widget/theme/widget.css +++ b/packages/ckeditor5-widget/theme/widget.css @@ -5,7 +5,7 @@ :root { --ck-color-resizer: var(--ck-color-focus-border); - --ck-resizer-size: 80px; + --ck-resizer-size: 10px; --ck-resizer-border-width: 1px; --ck-resizer-border-radius: 2px; From f835a333d65ed1237fb1ce6784824b0ea2cdfc7b Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 18 Sep 2020 13:02:29 +0200 Subject: [PATCH 07/23] Docs (image): Added some extra documentation on what the code is doing. --- .../ckeditor5-image/src/imageresize/imageresizehandles.js | 5 ++++- packages/ckeditor5-image/theme/imageresize.css | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 4888e1f006b..bcab1bd7230 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -91,12 +91,15 @@ export default class ImageResizeHandles extends Plugin { } /** + * Function to add listeners that will hide the resizer frame until the image is loaded, + * to avoid [resize frame flashing when the image is not yet loaded](https://github.com/ckeditor/ckeditor5/issues/8088). + * * @private * @param {module:widget/widgetresize/resizer~Resizer} resizer * @param {module:engine/view/containerelement~ContainerElement} widget */ _hideResizerUntilImageIsLoaded( resizer, widget ) { - // Mitigation logic for #8088 (which is caused by a more complex problem #7548). + // Given that #7548 is fixed, this logic might no longer be needed. const editingView = this.editor.editing.view; editingView.change( writer => { diff --git a/packages/ckeditor5-image/theme/imageresize.css b/packages/ckeditor5-image/theme/imageresize.css index fea881a964f..8ce764896df 100644 --- a/packages/ckeditor5-image/theme/imageresize.css +++ b/packages/ckeditor5-image/theme/imageresize.css @@ -25,6 +25,7 @@ } .ck-focused .ck-widget_with-resizer.ck-widget_selected.image.image_resizer_loading > .ck-widget__resizer { + /* Style to avoid resizer border flicker when the image is not yet uploaded (#8088). */ display: none; } From 646cd189f2b689dc48f526606b7216b12adff50d Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 22 Sep 2020 13:19:44 +0200 Subject: [PATCH 08/23] Internal (link): Redraw only visible resizers. This improves performance and fixes https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. --- .../src/imageresize/imageresizehandles.js | 9 ++++++-- .../tests/imageresize/imageresizehandles.js | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index bcab1bd7230..33bbfac5afe 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -100,7 +100,8 @@ export default class ImageResizeHandles extends Plugin { */ _hideResizerUntilImageIsLoaded( resizer, widget ) { // Given that #7548 is fixed, this logic might no longer be needed. - const editingView = this.editor.editing.view; + const editor = this.editor; + const editingView = editor.editing.view; editingView.change( writer => { writer.addClass( 'image_resizer_loading', widget ); @@ -113,7 +114,11 @@ export default class ImageResizeHandles extends Plugin { if ( domEvent.target.isSameNode( handleHost ) ) { editingView.change( writer => { - resizer.redraw(); + if ( editor.plugins.get( WidgetResize )._visibleResizer == resizer ) { + // Small optimization to redraw only a resizer that is visible/focused. Hidden resizers should remain + // unaffected. It also fixes https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. + resizer.redraw(); + } writer.removeClass( 'image_resizer_loading', widget ); } ); diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index fd2655eb045..1d80bd4ce7b 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -214,6 +214,29 @@ describe( 'ImageResizeHandles', () => { expect( shadowBoundingRect.width ).to.equal( 100 ); expect( shadowBoundingRect.height ).to.equal( 50 ); } ); + + it( 'doesn\'t show resizers when undoing to multiple images', async () => { + // Based on https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. + setData( editor.model, `[]` ); + + const paragraph = editor.model.change( writer => { + return writer.createElement( 'paragraph' ); + } ); + editor.model.insertContent( paragraph ); + + // Undo to go back to two, selected images. + editor.commands.get( 'undo' ).execute(); + + for ( let i = 0; i < 2; i++ ) { + widget = viewDocument.getRoot().getChild( i ); + const domImage = getWidgetDomParts( editor, widget, 'bottom-right' ).widget.querySelector( 'img' ); + viewDocument.fire( 'imageLoaded', { target: domImage } ); + + const domResizeWrapper = getWidgetDomParts( editor, widget, 'bottom-left' ).resizeWrapper; + + expect( domResizeWrapper.getBoundingClientRect().height ).to.equal( 0 ); + } + } ); } ); describe( 'table integration', () => { From d45bc876f9ede29da07dc7855bd860a7991a862e Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 13 Oct 2020 09:08:36 +0200 Subject: [PATCH 09/23] Adjusted import path. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Piotrek Koszuliński --- packages/ckeditor5-image/src/imageresize/imageresizehandles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 33bbfac5afe..40296a84c32 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -9,7 +9,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import WidgetResize from '@ckeditor/ckeditor5-widget/src/widgetresize'; -import ImageLoadObserver from './../image/imageloadobserver'; +import ImageLoadObserver from '../image/imageloadobserver'; /** * The image resize by handles feature. From 6eb1b6c70307b0302e814a10f2f18032177b1de7 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 13 Oct 2020 09:17:11 +0200 Subject: [PATCH 10/23] Other (widget): `WidgetResize#visibleResizer` is now a public property. --- .../src/imageresize/imageresizehandles.js | 2 +- packages/ckeditor5-widget/src/widgetresize.js | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 40296a84c32..57496e86ee9 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -114,7 +114,7 @@ export default class ImageResizeHandles extends Plugin { if ( domEvent.target.isSameNode( handleHost ) ) { editingView.change( writer => { - if ( editor.plugins.get( WidgetResize )._visibleResizer == resizer ) { + if ( editor.plugins.get( WidgetResize ).visibleResizer == resizer ) { // Small optimization to redraw only a resizer that is visible/focused. Hidden resizers should remain // unaffected. It also fixes https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. resizer.redraw(); diff --git a/packages/ckeditor5-widget/src/widgetresize.js b/packages/ckeditor5-widget/src/widgetresize.js index 4e9b4e9fd1e..beec7ee4229 100644 --- a/packages/ckeditor5-widget/src/widgetresize.js +++ b/packages/ckeditor5-widget/src/widgetresize.js @@ -41,11 +41,10 @@ export default class WidgetResize extends Plugin { /** * The currently visible resizer. * - * @protected * @observable - * @member {module:widget/widgetresize/resizer~Resizer|null} #_visibleResizer + * @member {module:widget/widgetresize/resizer~Resizer|null} #visibleResizer */ - this.set( '_visibleResizer', null ); + this.set( 'visibleResizer', null ); /** * References an active resizer. @@ -82,8 +81,8 @@ export default class WidgetResize extends Plugin { this._observer.listenTo( domDocument, 'mouseup', this._mouseUpListener.bind( this ) ); const redrawFocusedResizer = () => { - if ( this._visibleResizer ) { - this._visibleResizer.redraw(); + if ( this.visibleResizer ) { + this.visibleResizer.redraw(); } }; @@ -91,7 +90,7 @@ export default class WidgetResize extends Plugin { // Redraws occurring upon a change of visible resizer must not be throttled, as it is crucial for the initial // render. Without it the resizer frame would be misaligned with resizing host for a fraction of second. - this.on( 'change:_visibleResizer', redrawFocusedResizer ); + this.on( 'change:visibleResizer', redrawFocusedResizer ); // Redrawing on any change of the UI of the editor (including content changes). this.editor.ui.on( 'update', redrawFocusedResizerThrottled ); @@ -104,7 +103,7 @@ export default class WidgetResize extends Plugin { viewSelection.on( 'change', () => { const selectedElement = viewSelection.getSelectedElement(); - this._visibleResizer = this._getResizerByViewElement( selectedElement ) || null; + this.visibleResizer = this._getResizerByViewElement( selectedElement ) || null; } ); } From 83d407bd25e34deed5b6d7b2f2e764d589e5bfe0 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 13 Oct 2020 13:49:14 +0200 Subject: [PATCH 11/23] Internal (image): Refactored ImageResizeHandles plugin in a way that it attaches the resizer once the image is fully loaded. --- .../src/imageresize/imageresizehandles.js | 75 +++++++------------ 1 file changed, 26 insertions(+), 49 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 57496e86ee9..7f3b65596f8 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -38,24 +38,41 @@ export default class ImageResizeHandles extends Plugin { * @inheritDoc */ init() { + const command = this.editor.commands.get( 'imageResize' ); + this.bind( 'isEnabled' ).to( command ); + + this._addResizerCreationListener(); + } + + /** + * + * @private + */ + _addResizerCreationListener() { const editor = this.editor; - const command = editor.commands.get( 'imageResize' ); const editingView = editor.editing.view; editingView.addObserver( ImageLoadObserver ); - this.bind( 'isEnabled' ).to( command ); + editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { + const imageView = editor.editing.view.domConverter.domToView( domEvent.target ); + const widgetView = imageView.parent; + const mapper = editor.editing.mapper; - editor.editing.downcastDispatcher.on( 'insert:image', ( evt, data, conversionApi ) => { - const widget = conversionApi.mapper.toViewElement( data.item ); + // const imageModel = mapper.toModelElement( imageView ) || mapper.toModelElement( imageView.parent ); + const imageModel = mapper.toModelElement( widgetView ); + + if ( imageModel.name != 'image' ) { + return; + } const resizer = editor.plugins .get( WidgetResize ) .attachTo( { unit: editor.config.get( 'image.resizeUnit' ), - modelElement: data.item, - viewElement: widget, + modelElement: imageModel, + viewElement: widgetView, editor, getHandleHost( domWidgetElement ) { @@ -66,7 +83,7 @@ export default class ImageResizeHandles extends Plugin { }, // TODO consider other positions. isCentered() { - const imageStyle = data.item.getAttribute( 'imageStyle' ); + const imageStyle = imageModel.getAttribute( 'imageStyle' ); return !imageStyle || imageStyle == 'full' || imageStyle == 'alignCenter'; }, @@ -77,54 +94,14 @@ export default class ImageResizeHandles extends Plugin { } ); resizer.on( 'updateSize', () => { - if ( !widget.hasClass( 'image_resized' ) ) { + if ( !widgetView.hasClass( 'image_resized' ) ) { editingView.change( writer => { - writer.addClass( 'image_resized', widget ); + writer.addClass( 'image_resized', widgetView ); } ); } } ); - this._hideResizerUntilImageIsLoaded( resizer, widget ); - resizer.bind( 'isEnabled' ).to( this ); - }, { priority: 'low' } ); - } - - /** - * Function to add listeners that will hide the resizer frame until the image is loaded, - * to avoid [resize frame flashing when the image is not yet loaded](https://github.com/ckeditor/ckeditor5/issues/8088). - * - * @private - * @param {module:widget/widgetresize/resizer~Resizer} resizer - * @param {module:engine/view/containerelement~ContainerElement} widget - */ - _hideResizerUntilImageIsLoaded( resizer, widget ) { - // Given that #7548 is fixed, this logic might no longer be needed. - const editor = this.editor; - const editingView = editor.editing.view; - - editingView.change( writer => { - writer.addClass( 'image_resizer_loading', widget ); } ); - - editingView.document.on( 'imageLoaded', imageLoadCallback ); - - function imageLoadCallback( evt, domEvent ) { - const handleHost = resizer._getHandleHost(); - - if ( domEvent.target.isSameNode( handleHost ) ) { - editingView.change( writer => { - if ( editor.plugins.get( WidgetResize ).visibleResizer == resizer ) { - // Small optimization to redraw only a resizer that is visible/focused. Hidden resizers should remain - // unaffected. It also fixes https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. - resizer.redraw(); - } - writer.removeClass( 'image_resizer_loading', widget ); - } ); - - // Remove image load listener for optimization, as it is no longer needed. - editingView.document.off( 'imageLoaded', imageLoadCallback ); - } - } } } From a152ee2b04f1f3e3a07724eef663a6c5065574e0 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 13 Oct 2020 15:17:03 +0200 Subject: [PATCH 12/23] Other (widget): WidgetResize will now automatically set visibleResizer when calling WidgetResize.attachTo() if the corresponding resizer's element is focused during the call. --- .../src/imageresize/imageresizehandles.js | 3 +-- packages/ckeditor5-image/theme/imageresize.css | 5 ----- packages/ckeditor5-widget/src/widgetresize.js | 9 +++++++++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 7f3b65596f8..ba5a6a56200 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -56,10 +56,9 @@ export default class ImageResizeHandles extends Plugin { editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { const imageView = editor.editing.view.domConverter.domToView( domEvent.target ); - const widgetView = imageView.parent; + const widgetView = imageView.findAncestor( 'figure' ); const mapper = editor.editing.mapper; - // const imageModel = mapper.toModelElement( imageView ) || mapper.toModelElement( imageView.parent ); const imageModel = mapper.toModelElement( widgetView ); if ( imageModel.name != 'image' ) { diff --git a/packages/ckeditor5-image/theme/imageresize.css b/packages/ckeditor5-image/theme/imageresize.css index 8ce764896df..4c9c336c5e5 100644 --- a/packages/ckeditor5-image/theme/imageresize.css +++ b/packages/ckeditor5-image/theme/imageresize.css @@ -24,11 +24,6 @@ } } -.ck-focused .ck-widget_with-resizer.ck-widget_selected.image.image_resizer_loading > .ck-widget__resizer { - /* Style to avoid resizer border flicker when the image is not yet uploaded (#8088). */ - display: none; -} - [dir="ltr"] .ck.ck-button.ck-button_with-text.ck-resize-image-button .ck-button__icon { margin-right: var(--ck-spacing-standard); } diff --git a/packages/ckeditor5-widget/src/widgetresize.js b/packages/ckeditor5-widget/src/widgetresize.js index beec7ee4229..03962801600 100644 --- a/packages/ckeditor5-widget/src/widgetresize.js +++ b/packages/ckeditor5-widget/src/widgetresize.js @@ -148,6 +148,15 @@ export default class WidgetResize extends Plugin { this._resizers.set( options.viewElement, resizer ); + const viewSelection = this.editor.editing.view.document.selection; + const selectedElement = viewSelection.getSelectedElement(); + + // It could be that the element the resizer is created for is currently focused. In that + // case it should become visible. + if ( this._getResizerByViewElement( selectedElement ) == resizer ) { + this.visibleResizer = resizer; + } + return resizer; } From 64b69dabe3c6437ce6db3a51e9af0bcd522c5130 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Tue, 13 Oct 2020 15:19:27 +0200 Subject: [PATCH 13/23] Added API comment. --- packages/ckeditor5-image/src/imageresize/imageresizehandles.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index ba5a6a56200..32fe0c73656 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -45,6 +45,7 @@ export default class ImageResizeHandles extends Plugin { } /** + * Attaches the listeners responsible for creating a resizer for each editor image. * * @private */ From e519b50f4416f43ff9b82973ecdcfcf3b06604db Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Wed, 14 Oct 2020 09:40:24 +0200 Subject: [PATCH 14/23] Tests (image): Adjusted unit tests so that they wait for the images to be loaded. Looks that explicit editing.view.focus() is no longer needed. It was causing a major problem where the selection would be moved away from what was meant to be selected using `setData()`. --- .../tests/imageresize/imageresizehandles.js | 127 +++++++----------- .../tests/widgetresize/_utils/utils.js | 1 - 2 files changed, 52 insertions(+), 76 deletions(-) diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 1d80bd4ce7b..0e31c4c2e7a 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -30,6 +30,7 @@ import { } from '@ckeditor/ckeditor5-widget/tests/widgetresize/_utils/utils'; import WidgetResize from '@ckeditor/ckeditor5-widget/src/widgetresize'; +import ImageLoadObserver from '../../src/image/imageloadobserver'; describe( 'ImageResizeHandles', () => { // 100x50 black png image @@ -62,7 +63,7 @@ describe( 'ImageResizeHandles', () => { const attachToSpy = sinon.spy( localEditor.plugins.get( WidgetResize ), 'attachTo' ); - setData( localEditor.model, `[]` ); + await setModelAndWaitForImages( localEditor, `[]` ); expect( attachToSpy.args[ 0 ][ 0 ] ).to.have.a.property( 'unit', '%' ); @@ -79,7 +80,7 @@ describe( 'ImageResizeHandles', () => { it( 'uses the command on commit', async () => { const spy = sinon.spy( editor.commands.get( 'imageResize' ), 'execute' ); - setData( editor.model, `foo[]` ); + await setModelAndWaitForImages( editor, `foo[]` ); widget = viewDocument.getRoot().getChild( 1 ); const domParts = getWidgetDomParts( editor, widget, 'bottom-left' ); @@ -92,7 +93,7 @@ describe( 'ImageResizeHandles', () => { } ); it( 'disables the resizer if the command is disabled', async () => { - setData( editor.model, `foo[]` ); + await setModelAndWaitForImages( editor, `foo[]` ); const resizer = getSelectedImageResizer( editor ); @@ -124,6 +125,8 @@ describe( 'ImageResizeHandles', () => { editor.model.insertContent( writer.createElement( 'image', { src: IMAGE_SRC_FIXTURE } ) ); } ); + await waitForAllImagesLoaded( editor ); + const resizer = getSelectedImageResizer( editor ); const resizerWrapper = editor.ui.getEditableElement().querySelector( '.ck-widget__resizer' ); @@ -136,7 +139,8 @@ describe( 'ImageResizeHandles', () => { beforeEach( async () => { editor = await createEditor(); - setData( editor.model, `foo[]` ); + await setModelAndWaitForImages( editor, + `foo[]` ); widget = viewDocument.getRoot().getChild( 1 ); } ); @@ -178,13 +182,9 @@ describe( 'ImageResizeHandles', () => { beforeEach( async () => { editor = await createEditor(); - setData( editor.model, `foo[]` ); + await setModelAndWaitForImages( editor, `foo[]` ); widget = viewDocument.getRoot().getChild( 1 ); - - const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; - - viewDocument.fire( 'imageLoaded', { target: domWidget.querySelector( 'img' ) } ); } ); it( 'has correct border size after undo', async () => { @@ -204,9 +204,9 @@ describe( 'ImageResizeHandles', () => { // Toggle _visibleResizer to force synchronous redraw. Otherwise you'd need to wait ~200ms for // throttled redraw to take place, making tests slower. - const visibleResizer = plugin._visibleResizer; - plugin._visibleResizer = null; - plugin._visibleResizer = visibleResizer; + for ( const [ , resizer ] of plugin._resizers.entries() ) { + resizer.redraw(); + } const resizerWrapper = document.querySelector( '.ck-widget__resizer' ); const shadowBoundingRect = resizerWrapper.getBoundingClientRect(); @@ -217,7 +217,8 @@ describe( 'ImageResizeHandles', () => { it( 'doesn\'t show resizers when undoing to multiple images', async () => { // Based on https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-695949745. - setData( editor.model, `[]` ); + await setModelAndWaitForImages( editor, + `[]` ); const paragraph = editor.model.change( writer => { return writer.createElement( 'paragraph' ); @@ -243,7 +244,7 @@ describe( 'ImageResizeHandles', () => { it( 'works when resizing in a table', async () => { editor = await createEditor(); - setData( editor.model, + await setModelAndWaitForImages( editor, '' + `[]` + '
' @@ -265,64 +266,6 @@ describe( 'ImageResizeHandles', () => { } ); } ); - describe( 'image load detection', () => { - it( 'loading image is marked with a proper class', async () => { - editor = await createEditor(); - - setData( editor.model, `[]` ); - - const widget = viewDocument.getRoot().getChild( 0 ); - const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; - const image = domWidget.querySelector( 'img' ); - - // Before image is loaded there should be the loading class. - expect( Array.from( widget.getClassNames() ) ).to.include( 'image_resizer_loading' ); - - viewDocument.fire( 'imageLoaded', { target: image } ); - - // Now class should be removed. - expect( Array.from( widget.getClassNames() ) ).to.not.include( 'image_resizer_loading' ); - } ); - - it( 'loaded image has the loading class removed', async () => { - editor = await createEditor(); - - setData( editor.model, `[]` ); - - const widget = viewDocument.getRoot().getChild( 0 ); - const domWidget = getWidgetDomParts( editor, widget, 'bottom-right' ).widget; - const image = domWidget.querySelector( 'img' ); - - viewDocument.fire( 'imageLoaded', { target: image } ); - - // Now the class should be removed. - expect( Array.from( widget.getClassNames() ) ).to.not.include( 'image_resizer_loading' ); - } ); - - it( 'loading another image doesn\'t affect another', async () => { - editor = await createEditor(); - - setData( editor.model, `[]` ); - - const widgets = { - loading: viewDocument.getRoot().getChild( 0 ), - finished: viewDocument.getRoot().getChild( 1 ) - }; - - const domWidgets = { - loading: getWidgetDomParts( editor, widgets.loading, 'bottom-right' ).widget, - finished: getWidgetDomParts( editor, widgets.finished, 'bottom-right' ).widget - }; - - // Marking other image as loaded. - const finishedImage = domWidgets.finished.querySelector( 'img' ); - viewDocument.fire( 'imageLoaded', { target: finishedImage } ); - - // But the former is still considered as not loaded. - expect( Array.from( widgets.loading.getClassNames() ) ).to.include( 'image_resizer_loading' ); - } ); - } ); - describe( 'srcset integration', () => { // The image is 96x96 pixels. const imageBaseUrl = '/assets/sample.png'; @@ -357,11 +300,13 @@ describe( 'ImageResizeHandles', () => { ` ); + await waitForAllImagesLoaded( editor ); + widget = viewDocument.getRoot().getChild( 0 ); model = editor.model.document.getRoot().getChild( 0 ); } ); - it( 'works with images containing srcset', () => { + it( 'works with images containing srcset', async () => { const domParts = getWidgetDomParts( editor, widget, 'bottom-right' ); const initialPosition = getHandleCenterPoint( domParts.widget, 'bottom-right' ); const finalPointerPosition = initialPosition.clone().moveBy( -20, -20 ); @@ -374,7 +319,7 @@ describe( 'ImageResizeHandles', () => { expect( model.getAttribute( 'width' ) ).to.equal( '76px' ); } ); - it( 'retains width after removing srcset', () => { + it( 'retains width after removing srcset', async () => { const domParts = getWidgetDomParts( editor, widget, 'bottom-right' ); const initialPosition = getHandleCenterPoint( domParts.widget, 'bottom-right' ); const finalPointerPosition = initialPosition.clone().moveBy( -16, -16 ); @@ -424,7 +369,7 @@ describe( 'ImageResizeHandles', () => { } } ); - setData( editor.model, `foo[]` ); + await setModelAndWaitForImages( editor, `foo[]` ); widget = viewDocument.getRoot().getChild( 1 ); @@ -500,4 +445,36 @@ describe( 'ImageResizeHandles', () => { return newEditor; } ); } + + async function waitForAllImagesLoaded( editor ) { + // Returns a promise that waits for all the images in editor to be loaded. + // This is needed because resizers are created only after images are loaded. + const root = editor.model.document.getRoot(); + const editingView = editor.editing.view; + const images = new Set(); + + for ( const curModel of root.getChildren() ) { + if ( curModel.is( 'element', 'image' ) ) { + const imageView = editor.editing.mapper.toViewElement( curModel ); + images.add( editingView.domConverter.viewToDom( imageView ).querySelector( 'img' ) ); + } + } + + editingView.addObserver( ImageLoadObserver ); + + return new Promise( resolve => { + editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { + images.delete( domEvent.target ); + + if ( images.size === 0 ) { + resolve(); + } + } ); + } ); + } + + async function setModelAndWaitForImages( editor, data ) { + setData( editor.model, data ); + return waitForAllImagesLoaded( editor ); + } } ); diff --git a/packages/ckeditor5-widget/tests/widgetresize/_utils/utils.js b/packages/ckeditor5-widget/tests/widgetresize/_utils/utils.js index 9b9d62248da..222cc9383d9 100644 --- a/packages/ckeditor5-widget/tests/widgetresize/_utils/utils.js +++ b/packages/ckeditor5-widget/tests/widgetresize/_utils/utils.js @@ -152,6 +152,5 @@ export function getHandleCenterPoint( domWrapper, handlePosition ) { } export function focusEditor( editor ) { - editor.editing.view.focus(); editor.ui.focusTracker.isFocused = true; } From a6f90702a4fca2bace5a40b887e34540529bd7f2 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Wed, 14 Oct 2020 09:47:52 +0200 Subject: [PATCH 15/23] Loosen model verification. --- .../ckeditor5-image/src/imageresize/imageresizehandles.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 32fe0c73656..0b8d3bfb86d 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -62,10 +62,6 @@ export default class ImageResizeHandles extends Plugin { const imageModel = mapper.toModelElement( widgetView ); - if ( imageModel.name != 'image' ) { - return; - } - const resizer = editor.plugins .get( WidgetResize ) .attachTo( { From 25aebdf9236b94cc5b4c1bf27010b6b9d258cf8a Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Wed, 14 Oct 2020 12:17:59 +0200 Subject: [PATCH 16/23] Tests (image): Fixed a failing test by extracting common code. --- .../tests/imageresize/_utils/utils.js | 37 +++++++++++++++++++ .../tests/imageresize/imageresizeediting.js | 5 +-- .../tests/imageresize/imageresizehandles.js | 33 +---------------- .../tests/widgetresize-integration.js | 5 ++- 4 files changed, 43 insertions(+), 37 deletions(-) create mode 100644 packages/ckeditor5-image/tests/imageresize/_utils/utils.js diff --git a/packages/ckeditor5-image/tests/imageresize/_utils/utils.js b/packages/ckeditor5-image/tests/imageresize/_utils/utils.js new file mode 100644 index 00000000000..f4344aac024 --- /dev/null +++ b/packages/ckeditor5-image/tests/imageresize/_utils/utils.js @@ -0,0 +1,37 @@ +/** + * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +import ImageLoadObserver from '../../../src/image/imageloadobserver'; + +// A 100x50 black png image +export const IMAGE_SRC_FIXTURE = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGQAAAAyCAQAAAAAPLY1AAAAQklEQVR42u3PQREAAAgDoK1/' + + 'aM3g14MGNJMXKiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiJysRFNMgH0RpujAAAAAElFTkSuQmCC'; + +export async function waitForAllImagesLoaded( editor ) { + // Returns a promise that waits for all the images in editor to be loaded. + // This is needed because resizers are created only after images are loaded. + const root = editor.model.document.getRoot(); + const editingView = editor.editing.view; + const images = new Set(); + + for ( const curModel of root.getChildren() ) { + if ( curModel.is( 'element', 'image' ) ) { + const imageView = editor.editing.mapper.toViewElement( curModel ); + images.add( editingView.domConverter.viewToDom( imageView ).querySelector( 'img' ) ); + } + } + + editingView.addObserver( ImageLoadObserver ); + + return new Promise( resolve => { + editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { + images.delete( domEvent.target ); + + if ( images.size === 0 ) { + resolve(); + } + } ); + } ); +} diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizeediting.js b/packages/ckeditor5-image/tests/imageresize/imageresizeediting.js index 963782b9f2d..649f9bc0299 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizeediting.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizeediting.js @@ -18,12 +18,9 @@ import ImageStyle from '../../src/imagestyle'; import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { focusEditor } from '@ckeditor/ckeditor5-widget/tests/widgetresize/_utils/utils'; +import { IMAGE_SRC_FIXTURE } from './_utils/utils'; describe( 'ImageResizeEditing', () => { - // 100x50 black png image - const IMAGE_SRC_FIXTURE = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGQAAAAyCAQAAAAAPLY1AAAAQklEQVR42u3PQREAAAgDoK1/' + - 'aM3g14MGNJMXKiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiJysRFNMgH0RpujAAAAAElFTkSuQmCC'; - let editor, editorElement; beforeEach( () => { diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 0e31c4c2e7a..3061b7f3188 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -30,13 +30,9 @@ import { } from '@ckeditor/ckeditor5-widget/tests/widgetresize/_utils/utils'; import WidgetResize from '@ckeditor/ckeditor5-widget/src/widgetresize'; -import ImageLoadObserver from '../../src/image/imageloadobserver'; +import { IMAGE_SRC_FIXTURE, waitForAllImagesLoaded } from './_utils/utils'; describe( 'ImageResizeHandles', () => { - // 100x50 black png image - const IMAGE_SRC_FIXTURE = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAGQAAAAyCAQAAAAAPLY1AAAAQklEQVR42u3PQREAAAgDoK1/' + - 'aM3g14MGNJMXKiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiJysRFNMgH0RpujAAAAAElFTkSuQmCC'; - let widget, editor, view, viewDocument, editorElement; beforeEach( () => { @@ -446,33 +442,6 @@ describe( 'ImageResizeHandles', () => { } ); } - async function waitForAllImagesLoaded( editor ) { - // Returns a promise that waits for all the images in editor to be loaded. - // This is needed because resizers are created only after images are loaded. - const root = editor.model.document.getRoot(); - const editingView = editor.editing.view; - const images = new Set(); - - for ( const curModel of root.getChildren() ) { - if ( curModel.is( 'element', 'image' ) ) { - const imageView = editor.editing.mapper.toViewElement( curModel ); - images.add( editingView.domConverter.viewToDom( imageView ).querySelector( 'img' ) ); - } - } - - editingView.addObserver( ImageLoadObserver ); - - return new Promise( resolve => { - editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { - images.delete( domEvent.target ); - - if ( images.size === 0 ) { - resolve(); - } - } ); - } ); - } - async function setModelAndWaitForImages( editor, data ) { setData( editor.model, data ); return waitForAllImagesLoaded( editor ); diff --git a/packages/ckeditor5-widget/tests/widgetresize-integration.js b/packages/ckeditor5-widget/tests/widgetresize-integration.js index 6b52d006411..353d9a2d810 100644 --- a/packages/ckeditor5-widget/tests/widgetresize-integration.js +++ b/packages/ckeditor5-widget/tests/widgetresize-integration.js @@ -11,6 +11,7 @@ import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-util import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import Image from '@ckeditor/ckeditor5-image/src/image'; import ImageResize from '@ckeditor/ckeditor5-image/src/imageresize'; +import { waitForAllImagesLoaded } from '@ckeditor/ckeditor5-image/tests/imageresize/_utils/utils'; describe( 'WidgetResize - integration', () => { let editor, model, view, viewDocument, editorElement; @@ -36,11 +37,13 @@ describe( 'WidgetResize - integration', () => { return editor.destroy(); } ); - it( 'should not fire viewDocument#mousedown events after starting resizing', () => { + it( 'should not fire viewDocument#mousedown events after starting resizing', async () => { const eventSpy = sinon.spy().named( 'ViewDocument#mousedown' ); setModelData( model, '[]' ); + await waitForAllImagesLoaded( editor ); + const resizeSquareUI = [ ...viewDocument.getRoot().getChild( 0 ).getChildren() ] .find( element => element.hasClass( 'ck-widget__resizer' ) ); From a0fd43642c49700d7944deb538cd7a9a1f555bb7 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Wed, 14 Oct 2020 15:30:06 +0200 Subject: [PATCH 17/23] Internal (image): Fixed a case when multiple resizers could be created for a single widget. --- .../src/imageresize/imageresizehandles.js | 12 ++++++++++-- .../tests/imageresize/imageresizehandles.js | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 0b8d3bfb86d..8480fa87c3c 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -58,11 +58,19 @@ export default class ImageResizeHandles extends Plugin { editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { const imageView = editor.editing.view.domConverter.domToView( domEvent.target ); const widgetView = imageView.findAncestor( 'figure' ); - const mapper = editor.editing.mapper; + let resizer = this.editor.plugins.get( WidgetResize )._getResizerByViewElement( widgetView ); + + if ( resizer ) { + // There are rare cases when image will be triggered multiple times for the same widget, e.g. when + // image's src was changed after upload (https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-708302992). + resizer.redraw(); + return; + } + const mapper = editor.editing.mapper; const imageModel = mapper.toModelElement( widgetView ); - const resizer = editor.plugins + resizer = editor.plugins .get( WidgetResize ) .attachTo( { unit: editor.config.get( 'image.resizeUnit' ), diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 3061b7f3188..cb0e0f214f7 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -262,6 +262,23 @@ describe( 'ImageResizeHandles', () => { } ); } ); + it( 'doesn\'t create multiple resizers for a single image widget', async () => { + // https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-708302992 + editor = await createEditor(); + await setModelAndWaitForImages( editor, `[]` ); + widget = viewDocument.getRoot().getChild( 0 ); + + const domParts = getWidgetDomParts( editor, widget, 'bottom-right' ); + const alternativeImageFixture = + 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; + + // Change the image so that load event triggers for the same img element again. + domParts.widget.querySelector( 'img' ).src = alternativeImageFixture; + await waitForAllImagesLoaded( editor ); + + expect( domParts.widget.querySelectorAll( '.ck-widget__resizer' ).length ).to.equal( 1 ); + } ); + describe( 'srcset integration', () => { // The image is 96x96 pixels. const imageBaseUrl = '/assets/sample.png'; From fadf8db47969e7002c2ee0ce70a0fd27820437f3 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 15 Oct 2020 15:09:27 +0200 Subject: [PATCH 18/23] Tests (image): Added a TC ensuring that resizer is only created after image is loaded. --- .../tests/imageresize/imageresizehandles.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index cb0e0f214f7..3fa96a3c97c 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -279,6 +279,19 @@ describe( 'ImageResizeHandles', () => { expect( domParts.widget.querySelectorAll( '.ck-widget__resizer' ).length ).to.equal( 1 ); } ); + it( 'only creates a resizer after the image is loaded', async () => { + // https://github.com/ckeditor/ckeditor5/issues/8088 + editor = await createEditor(); + setData( editor.model, `[]` ); + widget = viewDocument.getRoot().getChild( 0 ); + const domParts = getWidgetDomParts( editor, widget, 'bottom-right' ); + + expect( domParts.widget.querySelectorAll( '.ck-widget__resizer' ).length ).to.equal( 0 ); + + await waitForAllImagesLoaded( editor ); + expect( domParts.widget.querySelectorAll( '.ck-widget__resizer' ).length ).to.equal( 1 ); + } ); + describe( 'srcset integration', () => { // The image is 96x96 pixels. const imageBaseUrl = '/assets/sample.png'; From 18ddae940155262731eae1dbb34d5031781a7b70 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 15 Oct 2020 15:44:06 +0200 Subject: [PATCH 19/23] Tests (widget): Added unit test ensuring that new resizer will be shown if associated widget element is focused prior resizer creation. --- .../ckeditor5-widget/tests/widgetresize.js | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/packages/ckeditor5-widget/tests/widgetresize.js b/packages/ckeditor5-widget/tests/widgetresize.js index f1020cd4e7f..7d12c7870c6 100644 --- a/packages/ckeditor5-widget/tests/widgetresize.js +++ b/packages/ckeditor5-widget/tests/widgetresize.js @@ -468,20 +468,43 @@ describe( 'WidgetResize', () => { } ); describe( 'attachTo()', () => { - it( 'works without WidgetToolbarRepository plugin', async () => { - const localEditorElement = createEditorElement(); - const localEditor = await ClassicEditor.create( localEditorElement, { + let localEditorElement, localEditor; + + beforeEach( async () => { + localEditorElement = createEditorElement(); + localEditor = await ClassicEditor.create( localEditorElement, { plugins: [ WidgetResize, simpleWidgetPlugin ] } ); + } ); + + afterEach( () => { + localEditorElement.remove(); + return localEditor.destroy(); + } ); + + it( 'works without WidgetToolbarRepository plugin', async () => { + setModelData( localEditor.model, '[]' ); + + localEditor.plugins.get( WidgetResize ).attachTo( gerResizerOptions( localEditor ) ); + // Nothing should be thrown. + } ); + it( 'sets the visible resizer if associated widget is already focused', async () => { setModelData( localEditor.model, '[]' ); - const resizerOptions = { - modelElement: localEditor.model.document.getRoot().getChild( 0 ), - viewElement: localEditor.editing.view.document.getRoot().getChild( 0 ), - editor: localEditor, + const widgetResizePlugin = localEditor.plugins.get( WidgetResize ); + const resizer = widgetResizePlugin.attachTo( gerResizerOptions( localEditor ) ); + + expect( widgetResizePlugin.visibleResizer ).to.eql( resizer ); + } ); + + function gerResizerOptions( editor ) { + return { + modelElement: editor.model.document.getRoot().getChild( 0 ), + viewElement: editor.editing.view.document.getRoot().getChild( 0 ), + editor, isCentered: () => false, getHandleHost( domWidgetElement ) { @@ -493,13 +516,7 @@ describe( 'WidgetResize', () => { onCommit: commitStub }; - - localEditor.plugins.get( WidgetResize ).attachTo( resizerOptions ); - // Nothing should be thrown. - // And clean up. - localEditorElement.remove(); - return localEditor.destroy(); - } ); + } } ); describe( 'init()', () => { From 8f26321247b28d8674a57c9e158437fe2a36f186 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 15 Oct 2020 15:59:17 +0200 Subject: [PATCH 20/23] Other (widget): `WidgetResize#getResizerByViewElement()` is now a public method. --- .../src/imageresize/imageresizehandles.js | 2 +- .../tests/imageresize/imageresizehandles.js | 2 +- packages/ckeditor5-widget/src/widgetresize.js | 25 +++++++++---------- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 8480fa87c3c..4206be71681 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -58,7 +58,7 @@ export default class ImageResizeHandles extends Plugin { editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { const imageView = editor.editing.view.domConverter.domToView( domEvent.target ); const widgetView = imageView.findAncestor( 'figure' ); - let resizer = this.editor.plugins.get( WidgetResize )._getResizerByViewElement( widgetView ); + let resizer = this.editor.plugins.get( WidgetResize ).getResizerByViewElement( widgetView ); if ( resizer ) { // There are rare cases when image will be triggered multiple times for the same widget, e.g. when diff --git a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js index 3fa96a3c97c..b99b62bcd40 100644 --- a/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js @@ -451,7 +451,7 @@ describe( 'ImageResizeHandles', () => { } function getSelectedImageResizer( editor ) { - return editor.plugins.get( 'WidgetResize' )._getResizerByViewElement( + return editor.plugins.get( 'WidgetResize' ).getResizerByViewElement( editor.editing.view.document.selection.getSelectedElement() ); } diff --git a/packages/ckeditor5-widget/src/widgetresize.js b/packages/ckeditor5-widget/src/widgetresize.js index 03962801600..a390041cca7 100644 --- a/packages/ckeditor5-widget/src/widgetresize.js +++ b/packages/ckeditor5-widget/src/widgetresize.js @@ -103,7 +103,7 @@ export default class WidgetResize extends Plugin { viewSelection.on( 'change', () => { const selectedElement = viewSelection.getSelectedElement(); - this.visibleResizer = this._getResizerByViewElement( selectedElement ) || null; + this.visibleResizer = this.getResizerByViewElement( selectedElement ) || null; } ); } @@ -153,13 +153,23 @@ export default class WidgetResize extends Plugin { // It could be that the element the resizer is created for is currently focused. In that // case it should become visible. - if ( this._getResizerByViewElement( selectedElement ) == resizer ) { + if ( this.getResizerByViewElement( selectedElement ) == resizer ) { this.visibleResizer = resizer; } return resizer; } + /** + * Returns a resizer created for a given view element (widget element). + * + * @param {module:engine/view/containerelement~ContainerElement} viewElement View element associated with the resizer. + * @returns {module:widget/widgetresize/resizer~Resizer/undefined} + */ + getResizerByViewElement( viewElement ) { + return this._resizers.get( viewElement ); + } + /** * Returns a resizer that contains a given resize handle. * @@ -175,17 +185,6 @@ export default class WidgetResize extends Plugin { } } - /** - * Returns a resizer created for a given view element (widget element). - * - * @protected - * @param {module:engine/view/containerelement~ContainerElement} viewElement - * @returns {module:widget/widgetresize/resizer~Resizer} - */ - _getResizerByViewElement( viewElement ) { - return this._resizers.get( viewElement ); - } - /** * @protected * @param {module:utils/eventinfo~EventInfo} event From c6277812437ff823115c87d36047bd521ac1720f Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 15 Oct 2020 16:16:46 +0200 Subject: [PATCH 21/23] Internal (image): Use context-aware event listener. --- packages/ckeditor5-image/src/imageresize/imageresizehandles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 4206be71681..59244fade1a 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -55,7 +55,7 @@ export default class ImageResizeHandles extends Plugin { editingView.addObserver( ImageLoadObserver ); - editingView.document.on( 'imageLoaded', ( evt, domEvent ) => { + this.listenTo( editingView.document, 'imageLoaded', ( evt, domEvent ) => { const imageView = editor.editing.view.domConverter.domToView( domEvent.target ); const widgetView = imageView.findAncestor( 'figure' ); let resizer = this.editor.plugins.get( WidgetResize ).getResizerByViewElement( widgetView ); From be3f9d67760163a77022190313240d8b484f9d0c Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 15 Oct 2020 16:23:10 +0200 Subject: [PATCH 22/23] Docs (widget): Fixed API docs type separator. --- packages/ckeditor5-widget/src/widgetresize.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-widget/src/widgetresize.js b/packages/ckeditor5-widget/src/widgetresize.js index a390041cca7..1094f299e23 100644 --- a/packages/ckeditor5-widget/src/widgetresize.js +++ b/packages/ckeditor5-widget/src/widgetresize.js @@ -164,7 +164,7 @@ export default class WidgetResize extends Plugin { * Returns a resizer created for a given view element (widget element). * * @param {module:engine/view/containerelement~ContainerElement} viewElement View element associated with the resizer. - * @returns {module:widget/widgetresize/resizer~Resizer/undefined} + * @returns {module:widget/widgetresize/resizer~Resizer|undefined} */ getResizerByViewElement( viewElement ) { return this._resizers.get( viewElement ); From 9e5317c546fc8647533f84e663b031657aaba729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 3 Nov 2020 13:46:14 +0100 Subject: [PATCH 23/23] Minor wording improvement. --- .../ckeditor5-image/src/imageresize/imageresizehandles.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js index 59244fade1a..be553d0d68f 100644 --- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.js +++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.js @@ -41,15 +41,15 @@ export default class ImageResizeHandles extends Plugin { const command = this.editor.commands.get( 'imageResize' ); this.bind( 'isEnabled' ).to( command ); - this._addResizerCreationListener(); + this._setupResizerCreator(); } /** - * Attaches the listeners responsible for creating a resizer for each editor image. + * Attaches the listeners responsible for creating a resizer for each image. * * @private */ - _addResizerCreationListener() { + _setupResizerCreator() { const editor = this.editor; const editingView = editor.editing.view; @@ -64,6 +64,7 @@ export default class ImageResizeHandles extends Plugin { // There are rare cases when image will be triggered multiple times for the same widget, e.g. when // image's src was changed after upload (https://github.com/ckeditor/ckeditor5/pull/8108#issuecomment-708302992). resizer.redraw(); + return; }