Selaa lähdekoodia

Refactored to listen model selection change:range event.

Oskar Wróbel 8 vuotta sitten
vanhempi
sitoutus
fc247ba50f

+ 23 - 18
packages/ckeditor5-ui/src/toolbar/contextualtoolbar.js

@@ -11,6 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/contextualballoon';
 import ToolbarView from './toolbarview';
 import BalloonPanelView from '../panel/balloon/balloonpanelview.js';
+import debounce from '@ckeditor/ckeditor5-utils/src/lib/lodash/debounce';
 
 /**
  * The contextual toolbar.
@@ -73,7 +74,7 @@ export default class ContextualToolbar extends Plugin {
 		const editor = this.editor;
 
 		// Hide the panel View when editor loses focus but no the other way around.
-		this.toolbarView.listenTo( editor.ui.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => {
+		this.listenTo( editor.ui.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => {
 			if ( this._balloon.visibleView === this.toolbarView && !isFocused ) {
 				this._hidePanel();
 			}
@@ -81,20 +82,32 @@ export default class ContextualToolbar extends Plugin {
 	}
 
 	/**
-	 * Handles {@link module:core/editor/editor~Editor#editing} selection change
-	 * and show or hide toolbar.
+	 * Handles {@link modules:engine/model/document#selection} change and show or hide toolbar.
+	 *
+	 * Note that in this case it's better to listen to {@link modules:engine/model/document modelDocument}
+	 * selection instead of {@link modules:engine/view/document viewDocument} selection because the first one
+	 * is not fired when text changes styles like bold or italic and toolbar doesn't blink.
 	 *
 	 * @private
 	 */
 	_handleSelectionChange() {
-		const toolbarView = this.toolbarView;
-		const editingView = this.editor.editing.view;
+		const selection = this.editor.document.selection;
+
+		// This is internal plugin event which is fired 200 ms after selection last change (lodash#debounce).
+		// This is to makes easy test debounced action without need to use `setTimeout`.
+		// Because lodash keeps time related stuff in a closure it's not possible to override it
+		// by sinon fake timers.
+		const fireChangeDoneDebounced = debounce( () => this.fire( '_selectionChangeDone' ), 200 );
+
+		this.listenTo( selection, 'change:range', () => {
+			// Hide the toolbar when the selection starts changing.
+			this._hidePanel();
 
-		// Hide panel when selection is changing.
-		toolbarView.listenTo( editingView, 'selectionChange', () => this._hidePanel() );
+			// Show the toolbar attached to the selection when the selection stops changing.
+			fireChangeDoneDebounced();
+		} );
 
-		// Display panel attached to the selection when selection stops changing.
-		toolbarView.listenTo( editingView, 'selectionChangeDone', () => this._showPanel() );
+		this.on( '_selectionChangeDone', () => this._showPanel() );
 	}
 
 	/**
@@ -105,7 +118,7 @@ export default class ContextualToolbar extends Plugin {
 	_showPanel() {
 		const editingView = this.editor.editing.view;
 
-		// Do not add panel to the balloon stack twice.
+		// Do not add toolbar to the balloon stack twice.
 		if ( this._balloon.hasView( this.toolbarView ) ) {
 			return;
 		}
@@ -136,13 +149,6 @@ export default class ContextualToolbar extends Plugin {
 					[ positions.forwardSelection, positions.forwardSelectionAlternative ],
 			}
 		} );
-
-		// Update panel position when editor content has changed.
-		this.toolbarView.listenTo( editingView, 'render', () => {
-			if ( this._balloon.visibleView === this.toolbarView ) {
-				this._balloon.updatePosition();
-			}
-		} );
 	}
 
 	/**
@@ -153,7 +159,6 @@ export default class ContextualToolbar extends Plugin {
 	_hidePanel() {
 		if ( this._balloon.hasView( this.toolbarView ) ) {
 			this._balloon.remove( this.toolbarView );
-			this.toolbarView.stopListening( this.editor.editing.view, 'render' );
 		}
 	}
 }

+ 70 - 61
packages/ckeditor5-ui/tests/toolbar/contextualtoolbar.js

@@ -6,7 +6,6 @@ import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictest
 import ContextualToolbar from '../../src/toolbar/contextualtoolbar';
 import ContextualBalloon from '../../src/contextualballoon';
 import ToolbarView from '../../src/toolbar/toolbarview';
-import ViewSelection from '@ckeditor/ckeditor5-engine/src/view/selection';
 import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
 import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
@@ -14,7 +13,7 @@ import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 
 import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js';
 
-/* global document, window */
+/* global document, window, setTimeout */
 
 describe( 'ContextualToolbar', () => {
 	let sandbox, editor, contextualToolbar, balloon, editorElement;
@@ -64,10 +63,68 @@ describe( 'ContextualToolbar', () => {
 		expect( contextualToolbar.toolbarView.items ).to.length( 2 );
 	} );
 
+	it( 'should fire internal `_selectionChangeDone` event 200 ms after last selection change', () => {
+		// This test uses setTimeout to test lodash#debounce because sinon fake timers
+		// doesn't work with lodash. Lodash keeps time related stuff in a closure
+		// and sinon is not able to override it.
+
+		const spy = sandbox.spy();
+		setData( editor.document, '<paragraph>[bar]</paragraph>' );
+		contextualToolbar.on( '_selectionChangeDone', spy );
+
+		editor.document.selection.fire( 'change:range' );
+
+		// Not yet.
+		sinon.assert.notCalled( spy );
+
+		// Lets wait 100 ms.
+		setTimeout( () => {
+			// Still not yet.
+			sinon.assert.notCalled( spy );
+
+			// Fire event one more time.
+			editor.document.selection.fire( 'change:range' );
+
+			// Another 100 ms waiting.
+			setTimeout( () => {
+				// Still not yet.
+				sinon.assert.notCalled( spy );
+
+				// Another 100 ms waiting.
+				setTimeout( () => {
+					// And here it is.
+					sinon.assert.calledOnce( spy );
+				}, 100 );
+			}, 100 );
+		}, 100 );
+	} );
+
+	it( 'should open when selection stops changing', () => {
+		setData( editor.document, '<paragraph>[bar]</paragraph>' );
+
+		expect( balloon.visibleView ).to.null;
+
+		contextualToolbar.fire( '_selectionChangeDone' );
+
+		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
+	} );
+
+	it( 'should close when selection starts changing', () => {
+		setData( editor.document, '<paragraph>[bar]</paragraph>' );
+
+		contextualToolbar.fire( '_selectionChangeDone' );
+
+		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
+
+		editor.document.selection.fire( 'change:range' );
+
+		expect( balloon.visibleView ).to.null;
+	} );
+
 	it( 'should open below if the selection is forward', () => {
 		setData( editor.document, '<paragraph>[bar]</paragraph>' );
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 		expect( balloon.view.top ).to.be.above( 310 );
@@ -84,7 +141,7 @@ describe( 'ContextualToolbar', () => {
 			height: 310
 		} );
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 		expect( balloon.view.top ).to.be.below( 310 );
@@ -93,7 +150,7 @@ describe( 'ContextualToolbar', () => {
 	it( 'should open above if the selection is backward', () => {
 		setData( editor.document, '<paragraph>[bar]</paragraph>', { lastRangeBackward: true } );
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 		expect( balloon.view.top ).to.be.below( 100 );
@@ -110,7 +167,7 @@ describe( 'ContextualToolbar', () => {
 			height: 905
 		} );
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 		expect( balloon.view.top ).to.be.above( 100 );
@@ -119,16 +176,13 @@ describe( 'ContextualToolbar', () => {
 	it( 'should not open if the collapsed selection is moving', () => {
 		setData( editor.document, '<paragraph>ba[]r</paragraph>' );
 
-		const oldSelection = editor.editing.view.selection;
-		const newSelection = new ViewSelection();
-
-		editor.editing.view.fire( 'selectionChange', { oldSelection, newSelection } );
-		editor.editing.view.fire( 'selectionChangeDone' );
+		editor.document.selection.fire( 'change:range' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		setData( editor.document, '<paragraph>b[]ar</paragraph>' );
 
-		editor.editing.view.fire( 'selectionChange', { oldSelection, newSelection } );
-		editor.editing.view.fire( 'selectionChangeDone' );
+		editor.document.selection.fire( 'change:range' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.null;
 	} );
@@ -137,7 +191,7 @@ describe( 'ContextualToolbar', () => {
 		setData( editor.document, '<paragraph>[bar]</paragraph>' );
 		editor.ui.focusTracker.isFocused = true;
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 
@@ -146,63 +200,18 @@ describe( 'ContextualToolbar', () => {
 		expect( balloon.visibleView ).to.null;
 	} );
 
-	it( 'should hide if the selection is changing', () => {
-		setData( editor.document, '<paragraph>[bar]</paragraph>' );
-
-		editor.editing.view.fire( 'selectionChangeDone' );
-
-		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
-
-		const oldSelection = editor.editing.view.selection;
-		const newSelection = new ViewSelection();
-
-		editor.editing.view.fire( 'selectionChange', { oldSelection, newSelection } );
-
-		expect( balloon.visibleView ).to.null;
-	} );
-
 	it( 'should do nothing when panel is being added to balloon stack twice', () => {
 		setData( editor.document, '<paragraph>[bar]</paragraph>' );
 
-		editor.editing.view.fire( 'selectionChangeDone' );
+		contextualToolbar.fire( '_selectionChangeDone' );
 
 		expect( balloon.visibleView ).to.equal( contextualToolbar.toolbarView );
 
 		expect( () => {
-			editor.editing.view.fire( 'selectionChangeDone' );
+			contextualToolbar.fire( '_selectionChangeDone' );
 		} ).to.not.throw();
 	} );
 
-	it( 'should update toolbar position when is visible and editor content has changed', () => {
-		const spy = sandbox.spy( balloon, 'updatePosition' );
-
-		setData( editor.document, '<paragraph>ba[r]</paragraph>' );
-
-		editor.editing.view.fire( 'selectionChangeDone' );
-
-		sinon.assert.notCalled( spy );
-
-		editor.editing.view.fire( 'render' );
-
-		sinon.assert.calledOnce( spy );
-	} );
-
-	it( 'should not update toolbar position when is added to the balloon stack but is not visible and editor content has changed', () => {
-		const spy = sandbox.spy( balloon, 'updatePosition' );
-
-		setData( editor.document, '<paragraph>ba[r]</paragraph>' );
-
-		editor.editing.view.fire( 'selectionChangeDone' );
-
-		const viewMock = { destroy: () => {} };
-
-		balloon.add( { view: viewMock } );
-
-		editor.editing.view.fire( 'render' );
-
-		sinon.assert.notCalled( spy );
-	} );
-
 	function stubClientRects() {
 		const editingView = editor.editing.view;
 		const originalViewRangeToDom = editingView.domConverter.viewRangeToDom;