From 8ed6cc971e8f1da9f2cb1513a3f5d3c5df4de7c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 8 Sep 2017 19:46:35 +0200 Subject: [PATCH 1/7] Added util for checking if image widged is currently selected. --- src/image/utils.js | 12 ++++++++++++ tests/image/utils.js | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/image/utils.js b/src/image/utils.js index 27903acf..1614de98 100644 --- a/src/image/utils.js +++ b/src/image/utils.js @@ -44,6 +44,18 @@ export function isImageWidget( viewElement ) { return !!viewElement.getCustomProperty( imageSymbol ) && isWidget( viewElement ); } +/** + * Checks image widget is a selected element. + * + * @param {module:engine/view/selection~Selection} viewSelection + * @returns {Boolean} + */ +export function isImageWidgetSelected( viewSelection ) { + const viewElement = viewSelection.getSelectedElement(); + + return viewElement && isImageWidget( viewElement ); +} + /** * Checks if the provided model element is an instance of {@link module:engine/model/element~Element Element} and its name * is `image`. diff --git a/tests/image/utils.js b/tests/image/utils.js index e2c4f4ef..af65975a 100644 --- a/tests/image/utils.js +++ b/tests/image/utils.js @@ -4,8 +4,11 @@ */ import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; +import ViewSelection from '@ckeditor/ckeditor5-engine/src/view/selection'; +import ViewDocumentFragment from '@ckeditor/ckeditor5-engine/src/view/documentfragment'; +import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; -import { toImageWidget, isImageWidget, isImage } from '../../src/image/utils'; +import { toImageWidget, isImageWidget, isImageWidgetSelected, isImage } from '../../src/image/utils'; import { isWidget, getLabel } from '@ckeditor/ckeditor5-widget/src/utils'; describe( 'image widget utils', () => { @@ -49,6 +52,40 @@ describe( 'image widget utils', () => { } ); } ); + describe( 'isImageWidgetSelected()', () => { + let frag; + + it( 'should return true when image widget is the only element in the selection', () => { + // We need to create a container for the element to be able to create a Range on this element. + frag = new ViewDocumentFragment( [ element ] ); + + const selection = new ViewSelection( [ ViewRange.createOn( element ) ] ); + + expect( isImageWidgetSelected( selection ) ).to.be.true; + } ); + + it( 'should return false when non-widgetized elements is the only element in the selection', () => { + const notWidgetizedElement = new ViewElement( 'p' ); + + // We need to create a container for the element to be able to create a Range on this element. + frag = new ViewDocumentFragment( [ notWidgetizedElement ] ); + + const selection = new ViewSelection( [ ViewRange.createOn( notWidgetizedElement ) ] ); + + expect( isImageWidgetSelected( selection ) ).to.be.false; + } ); + + it( 'should return false when widget element is not the only element in the selection', () => { + const notWidgetizedElement = new ViewElement( 'p' ); + + frag = new ViewDocumentFragment( [ notWidgetizedElement, notWidgetizedElement ] ); + + const selection = new ViewSelection( [ ViewRange.createIn( frag ) ] ); + + expect( isImageWidgetSelected( selection ) ).to.be.false; + } ); + } ); + describe( 'isImage', () => { it( 'should return true for image element', () => { const image = new ModelElement( 'image' ); From daf2d5f2fe55bd0df2db1200140177c88ce9cb79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 8 Sep 2017 19:48:34 +0200 Subject: [PATCH 2/7] Simplified checking if selected element is an image widget. --- src/image.js | 6 ++---- src/image/ui/utils.js | 6 ++---- src/imagetoolbar.js | 7 ++----- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/image.js b/src/image.js index 96da6626..147dad33 100644 --- a/src/image.js +++ b/src/image.js @@ -11,7 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import ImageEngine from './image/imageengine'; import Widget from '@ckeditor/ckeditor5-widget/src/widget'; import ImageTextAlternative from './imagetextalternative'; -import { isImageWidget } from './image/utils'; +import { isImageWidgetSelected } from './image/utils'; import '../theme/theme.scss'; @@ -49,9 +49,7 @@ export default class Image extends Plugin { // https://github.com/ckeditor/ckeditor5-image/issues/110 if ( contextualToolbar ) { this.listenTo( contextualToolbar, 'show', evt => { - const selectedElement = editor.editing.view.selection.getSelectedElement(); - - if ( selectedElement && isImageWidget( selectedElement ) ) { + if ( isImageWidgetSelected( editor.editing.view.selection ) ) { evt.stop(); } }, { priority: 'high' } ); diff --git a/src/image/ui/utils.js b/src/image/ui/utils.js index 55649fbf..65edf118 100644 --- a/src/image/ui/utils.js +++ b/src/image/ui/utils.js @@ -8,7 +8,7 @@ */ import BalloonPanelView from '@ckeditor/ckeditor5-ui/src/panel/balloon/balloonpanelview'; -import { isImageWidget } from '../utils'; +import { isImageWidgetSelected } from '../utils'; /** * A helper utility which positions the @@ -18,11 +18,9 @@ import { isImageWidget } from '../utils'; * @param {module:core/editor/editor~Editor} editor The editor instance. */ export function repositionContextualBalloon( editor ) { - const editingView = editor.editing.view; const balloon = editor.plugins.get( 'ContextualBalloon' ); - const selectedElement = editingView.selection.getSelectedElement(); - if ( selectedElement && isImageWidget( selectedElement ) ) { + if ( isImageWidgetSelected( editor.editing.view.selection ) ) { const position = getBalloonPositionData( editor ); balloon.updatePosition( position ); diff --git a/src/imagetoolbar.js b/src/imagetoolbar.js index 9e72af3c..98b8062e 100644 --- a/src/imagetoolbar.js +++ b/src/imagetoolbar.js @@ -11,7 +11,7 @@ import Template from '@ckeditor/ckeditor5-ui/src/template'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import ToolbarView from '@ckeditor/ckeditor5-ui/src/toolbar/toolbarview'; import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon'; -import { isImageWidget } from './image/utils'; +import { isImageWidgetSelected } from './image/utils'; import { repositionContextualBalloon, getBalloonPositionData } from './image/ui/utils'; const balloonClassName = 'ck-toolbar-container ck-editor-toolbar-container'; @@ -103,10 +103,7 @@ export default class ImageToolbar extends Plugin { if ( !editor.ui.focusTracker.isFocused ) { this._hideToolbar(); } else { - const editingView = editor.editing.view; - const selectedElement = editingView.selection.getSelectedElement(); - - if ( selectedElement && isImageWidget( selectedElement ) ) { + if ( isImageWidgetSelected( editor.editing.view.selection ) ) { this._showToolbar(); } else { this._hideToolbar(); From 1125c2f89e776f8b5a529b90dba2d166c4db6169 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 8 Sep 2017 20:02:38 +0200 Subject: [PATCH 3/7] Hide ImageTextAlternative balloon when image element has been removed by external change. --- src/imagetextalternative.js | 9 ++++++--- tests/imagetextalternative.js | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/imagetextalternative.js b/src/imagetextalternative.js index 56f94b13..c001da53 100644 --- a/src/imagetextalternative.js +++ b/src/imagetextalternative.js @@ -15,6 +15,7 @@ import TextAlternativeFormView from './imagetextalternative/ui/textalternativefo import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon'; import textAlternativeIcon from '@ckeditor/ckeditor5-core/theme/icons/low-vision.svg'; import { repositionContextualBalloon, getBalloonPositionData } from './image/ui/utils'; +import { isImageWidgetSelected } from './image/utils'; import '../theme/imagetextalternative/theme.scss'; @@ -119,9 +120,11 @@ export default class ImageTextAlternative extends Plugin { cancel(); } ); - // Reposition the balloon upon #render. + // Reposition the balloon or hide the form if image widget is no longer selected. this.listenTo( editingView, 'render', () => { - if ( this._isVisible ) { + if ( !isImageWidgetSelected( editingView.selection ) ) { + this._hideForm(); + } else if ( this._isVisible ) { repositionContextualBalloon( editor ); } }, { priority: 'low' } ); @@ -176,7 +179,7 @@ export default class ImageTextAlternative extends Plugin { /** * Removes the {@link #_form} from the {@link #_balloon}. * - * @param {Boolean} focusEditable Controls whether the editing view is focused afterwards. + * @param {Boolean} [focusEditable=false] Controls whether the editing view is focused afterwards. * @private */ _hideForm( focusEditable ) { diff --git a/tests/imagetextalternative.js b/tests/imagetextalternative.js index 034be927..2b362f3b 100644 --- a/tests/imagetextalternative.js +++ b/tests/imagetextalternative.js @@ -168,6 +168,20 @@ describe( 'ImageTextAlternative', () => { editingView.fire( 'render' ); sinon.assert.calledOnce( spy ); } ); + + it( 'should hide the form when image widget is no longer selected', () => { + setData( doc, '[]' ); + button.fire( 'execute' ); + + const spy = sinon.spy( balloon, 'remove' ); + + // EnqueueChanges automatically calls #render event. + doc.enqueueChanges( () => { + doc.batch( 'transparent' ).remove( doc.selection.getFirstRange() ); + } ); + + sinon.assert.calledWithExactly( spy, form ); + } ); } ); describe( 'integration with the editor focus', () => { From 90c8152a01363a25ac620715c637798b300a737a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Fri, 8 Sep 2017 20:09:51 +0200 Subject: [PATCH 4/7] Improved docs. --- src/image/utils.js | 2 +- src/imagetextalternative.js | 2 +- tests/imagetextalternative.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/image/utils.js b/src/image/utils.js index 1614de98..257406c5 100644 --- a/src/image/utils.js +++ b/src/image/utils.js @@ -45,7 +45,7 @@ export function isImageWidget( viewElement ) { } /** - * Checks image widget is a selected element. + * Checks if an image widget is the only selected element. * * @param {module:engine/view/selection~Selection} viewSelection * @returns {Boolean} diff --git a/src/imagetextalternative.js b/src/imagetextalternative.js index c001da53..fc1e03d8 100644 --- a/src/imagetextalternative.js +++ b/src/imagetextalternative.js @@ -120,7 +120,7 @@ export default class ImageTextAlternative extends Plugin { cancel(); } ); - // Reposition the balloon or hide the form if image widget is no longer selected. + // Reposition the balloon or hide the form if an image widget is no longer selected. this.listenTo( editingView, 'render', () => { if ( !isImageWidgetSelected( editingView.selection ) ) { this._hideForm(); diff --git a/tests/imagetextalternative.js b/tests/imagetextalternative.js index 2b362f3b..d6da21be 100644 --- a/tests/imagetextalternative.js +++ b/tests/imagetextalternative.js @@ -169,7 +169,7 @@ describe( 'ImageTextAlternative', () => { sinon.assert.calledOnce( spy ); } ); - it( 'should hide the form when image widget is no longer selected', () => { + it( 'should hide the form when image widget has been removed by external change', () => { setData( doc, '[]' ); button.fire( 'execute' ); From 344f8a0602a4cd8ddd656a786125d756bb8d9226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 11 Sep 2017 16:53:04 +0200 Subject: [PATCH 5/7] Changed type of value returned by isImageWidgetSelected util. --- src/image/utils.js | 2 +- tests/image/utils.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/image/utils.js b/src/image/utils.js index 257406c5..e6cf8e78 100644 --- a/src/image/utils.js +++ b/src/image/utils.js @@ -53,7 +53,7 @@ export function isImageWidget( viewElement ) { export function isImageWidgetSelected( viewSelection ) { const viewElement = viewSelection.getSelectedElement(); - return viewElement && isImageWidget( viewElement ); + return !!( viewElement && isImageWidget( viewElement ) ); } /** diff --git a/tests/image/utils.js b/tests/image/utils.js index af65975a..74fe1b24 100644 --- a/tests/image/utils.js +++ b/tests/image/utils.js @@ -78,7 +78,7 @@ describe( 'image widget utils', () => { it( 'should return false when widget element is not the only element in the selection', () => { const notWidgetizedElement = new ViewElement( 'p' ); - frag = new ViewDocumentFragment( [ notWidgetizedElement, notWidgetizedElement ] ); + frag = new ViewDocumentFragment( [ element, notWidgetizedElement ] ); const selection = new ViewSelection( [ ViewRange.createIn( frag ) ] ); From 643d9a34c34f089386eed8293e3daf82e0cc4fe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 12 Sep 2017 11:01:46 +0200 Subject: [PATCH 6/7] Added one more test case for ImageTextAlternative balloon manual test. --- tests/manual/tickets/106/1.js | 9 +++++++-- tests/manual/tickets/106/1.md | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/manual/tickets/106/1.js b/tests/manual/tickets/106/1.js index 1e19ab08..91196389 100644 --- a/tests/manual/tickets/106/1.js +++ b/tests/manual/tickets/106/1.js @@ -108,9 +108,14 @@ function startExternalDelete( editor ) { const document = editor.document; const bath = document.batch( 'transparent' ); - wait( 3000 ).then( () => { + function removeSecondBlock() { document.enqueueChanges( () => { bath.remove( Range.createFromPositionAndShift( new Position( document.getRoot(), [ 1 ] ), 1 ) ); } ); - } ); + } + + wait( 3000 ) + .then( () => removeSecondBlock() ) + .then( () => wait( 3000 ) ) + .then( () => removeSecondBlock() ); } diff --git a/tests/manual/tickets/106/1.md b/tests/manual/tickets/106/1.md index 55f73c06..86a0e7e6 100644 --- a/tests/manual/tickets/106/1.md +++ b/tests/manual/tickets/106/1.md @@ -11,3 +11,4 @@ 1. Click **Start external changes** then quickly select the image in the content, then from the toolbar open the **text alternative** editing balloon. 2. Observe if the balloon remains attached to the image as the content is deleted. +3. Wait a bit more and observe if the balloon is removed together with an image. From 12be7a93aa222e3ee8b4d2995d29b6f0a0700aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 12 Sep 2017 14:25:45 +0200 Subject: [PATCH 7/7] Added focusing editable when ImageTextAlternative balloon has been removed by an external change. --- src/imagetextalternative.js | 2 +- tests/imagetextalternative.js | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/imagetextalternative.js b/src/imagetextalternative.js index fc1e03d8..ee89ac7f 100644 --- a/src/imagetextalternative.js +++ b/src/imagetextalternative.js @@ -123,7 +123,7 @@ export default class ImageTextAlternative extends Plugin { // Reposition the balloon or hide the form if an image widget is no longer selected. this.listenTo( editingView, 'render', () => { if ( !isImageWidgetSelected( editingView.selection ) ) { - this._hideForm(); + this._hideForm( true ); } else if ( this._isVisible ) { repositionContextualBalloon( editor ); } diff --git a/tests/imagetextalternative.js b/tests/imagetextalternative.js index d6da21be..b1f11409 100644 --- a/tests/imagetextalternative.js +++ b/tests/imagetextalternative.js @@ -169,18 +169,20 @@ describe( 'ImageTextAlternative', () => { sinon.assert.calledOnce( spy ); } ); - it( 'should hide the form when image widget has been removed by external change', () => { + it( 'should hide the form and focus editable when image widget has been removed by external change', () => { setData( doc, '[]' ); button.fire( 'execute' ); - const spy = sinon.spy( balloon, 'remove' ); + const remveSpy = sinon.spy( balloon, 'remove' ); + const focusSpy = sinon.spy( editor.editing.view, 'focus' ); - // EnqueueChanges automatically calls #render event. + // EnqueueChanges automatically fires #render event. doc.enqueueChanges( () => { doc.batch( 'transparent' ).remove( doc.selection.getFirstRange() ); } ); - sinon.assert.calledWithExactly( spy, form ); + sinon.assert.calledWithExactly( remveSpy, form ); + sinon.assert.calledOnce( focusSpy ); } ); } );