From 870c1656ae021993308142e73b4a169f20a1c15e Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 10 Jul 2017 11:04:48 +0200 Subject: [PATCH 1/3] Other: Implemented public show and hide methods in the ContextualToolbar plugin. Closes #263. Extended enableToolbarKeyboardFocus helper with beforeFocus and afterBlur callbacks. --- src/toolbar/contextual/contextualtoolbar.js | 31 ++++--- src/toolbar/enabletoolbarkeyboardfocus.js | 14 ++- tests/toolbar/contextual/contextualtoolbar.js | 92 +++++++++++-------- tests/toolbar/enabletoolbarkeyboardfocus.js | 54 ++++++++++- 4 files changed, 134 insertions(+), 57 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index a002c168..bde000c5 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -13,6 +13,7 @@ import ContextualBalloon from '../../panel/balloon/contextualballoon'; import ToolbarView from '../toolbarview'; import BalloonPanelView from '../../panel/balloon/balloonpanelview.js'; import debounce from '@ckeditor/ckeditor5-utils/src/lib/lodash/debounce'; +import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect'; /** * The contextual toolbar. @@ -104,7 +105,7 @@ export default class ContextualToolbar extends Plugin { // Hide the panel View when editor loses focus but no the other way around. this.listenTo( editor.ui.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => { if ( this._balloon.visibleView === this.toolbarView && !isFocused ) { - this._hidePanel(); + this.hide(); } } ); } @@ -120,12 +121,13 @@ export default class ContextualToolbar extends Plugin { */ _handleSelectionChange() { const selection = this.editor.document.selection; + const editingView = this.editor.editing.view; this.listenTo( selection, 'change:range', ( evt, data ) => { // When the selection is not changed by a collaboration and when is not collapsed. if ( data.directChange || selection.isCollapsed ) { // Hide the toolbar when the selection starts changing. - this._hidePanel(); + this.hide(); } // Fire internal `_selectionChangeDebounced` when the selection stops changing. @@ -133,17 +135,20 @@ export default class ContextualToolbar extends Plugin { } ); // Hide the toolbar when the selection stops changing. - this.listenTo( this, '_selectionChangeDebounced', () => this._showPanel() ); + this.listenTo( this, '_selectionChangeDebounced', () => { + // This implementation assumes that only non–collapsed selections gets the contextual toolbar. + if ( editingView.isFocused && !editingView.selection.isCollapsed ) { + this.show(); + } + } ); } /** * Adds panel view to the {@link: #_balloon} and attaches panel to the selection. * * Fires {@link #event:beforeShow} event just before displaying the panel. - * - * @protected */ - _showPanel() { + show() { const editingView = this.editor.editing.view; let isStopped = false; @@ -185,11 +190,9 @@ export default class ContextualToolbar extends Plugin { } /** - * Removes panel from the {@link: #_balloon}. - * - * @private + * Removes the toolbar from the {@link: #_balloon} which hides it. */ - _hidePanel() { + hide() { if ( this._balloon.hasView( this.toolbarView ) ) { this.stopListening( this.editor.editing.view, 'render' ); this._balloon.remove( this.toolbarView ); @@ -215,13 +218,11 @@ export default class ContextualToolbar extends Plugin { // computed and hence, the target is defined as a function instead of a static value. // https://github.com/ckeditor/ckeditor5-ui/issues/195 target: () => { - // getBoundingClientRect() makes no sense when the selection spans across number - // of lines of text. Using getClientRects() allows us to browse micro–ranges - // that would normally make up the bounding client rect. - const rangeRects = editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ).getClientRects(); + const range = editingView.selection.getFirstRange(); + const rangeRects = Rect.getDomRangeRects( editingView.domConverter.viewRangeToDom( range ) ); // Select the proper range rect depending on the direction of the selection. - return isBackward ? rangeRects.item( 0 ) : rangeRects.item( rangeRects.length - 1 ); + return rangeRects[ isBackward ? 0 : rangeRects.length - 1 ]; }, limiter: this.editor.ui.view.editable.element, positions: getBalloonPositions( isBackward ) diff --git a/src/toolbar/enabletoolbarkeyboardfocus.js b/src/toolbar/enabletoolbarkeyboardfocus.js index 4e0a62c9..56a98540 100644 --- a/src/toolbar/enabletoolbarkeyboardfocus.js +++ b/src/toolbar/enabletoolbarkeyboardfocus.js @@ -24,7 +24,9 @@ export default function enableToolbarKeyboardFocus( { origin, originKeystrokeHandler, originFocusTracker, - toolbar + toolbar, + beforeFocus, + afterBlur } ) { // Because toolbar items can get focus, the overall state of the toolbar must // also be tracked. @@ -33,7 +35,12 @@ export default function enableToolbarKeyboardFocus( { // Focus the toolbar on the keystroke, if not already focused. originKeystrokeHandler.set( 'Alt+F10', ( data, cancel ) => { if ( originFocusTracker.isFocused && !toolbar.focusTracker.isFocused ) { + if ( beforeFocus ) { + beforeFocus(); + } + toolbar.focus(); + cancel(); } } ); @@ -42,6 +49,11 @@ export default function enableToolbarKeyboardFocus( { toolbar.keystrokes.set( 'Esc', ( data, cancel ) => { if ( toolbar.focusTracker.isFocused ) { origin.focus(); + + if ( afterBlur ) { + afterBlur(); + } + cancel(); } } ); diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 14488413..4957be6f 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -113,7 +113,7 @@ describe( 'ContextualToolbar', () => { } ); } ); - describe( '_showPanel()', () => { + describe( 'show()', () => { let balloonAddSpy, forwardSelectionRect, backwardSelectionRect; beforeEach( () => { @@ -127,9 +127,9 @@ describe( 'ContextualToolbar', () => { }; backwardSelectionRect = { - top: 100, + top: 200, height: 10, - bottom: 110, + bottom: 210, left: 200, width: 50, right: 250 @@ -146,13 +146,13 @@ describe( 'ContextualToolbar', () => { const defaultPositions = BalloonPanelView.defaultPositions; - contextualToolbar._showPanel(); + contextualToolbar.show(); - sinon.assert.calledWithExactly( balloonAddSpy, { + sinon.assert.calledWith( balloonAddSpy, { view: contextualToolbar.toolbarView, balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', position: { - target: sinon.match( value => value() == backwardSelectionRect ), + target: sinon.match.func, limiter: editor.ui.view.editable.element, positions: [ defaultPositions.southEastArrowNorth, @@ -164,6 +164,8 @@ describe( 'ContextualToolbar', () => { ] } } ); + + expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( backwardSelectionRect ); } ); it( 'should add #toolbarView to the #_balloon and attach the #_balloon to the selection for the backward selection', () => { @@ -171,13 +173,13 @@ describe( 'ContextualToolbar', () => { const defaultPositions = BalloonPanelView.defaultPositions; - contextualToolbar._showPanel(); + contextualToolbar.show(); sinon.assert.calledWithExactly( balloonAddSpy, { view: contextualToolbar.toolbarView, balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container', position: { - target: sinon.match( value => value() == forwardSelectionRect ), + target: sinon.match.func, limiter: editor.ui.view.editable.element, positions: [ defaultPositions.northWestArrowSouth, @@ -189,6 +191,8 @@ describe( 'ContextualToolbar', () => { ] } } ); + + expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( forwardSelectionRect ); } ); it( 'should update balloon position on ViewDocument#render event while balloon is added to the #_balloon', () => { @@ -198,7 +202,7 @@ describe( 'ContextualToolbar', () => { editor.editing.view.fire( 'render' ); - contextualToolbar._showPanel(); + contextualToolbar.show(); sinon.assert.notCalled( spy ); editor.editing.view.fire( 'render' ); @@ -208,28 +212,44 @@ describe( 'ContextualToolbar', () => { it( 'should not add #toolbarView to the #_balloon more than once', () => { setData( editor.document, 'b[a]r' ); - contextualToolbar._showPanel(); - contextualToolbar._showPanel(); + contextualToolbar.show(); + contextualToolbar.show(); sinon.assert.calledOnce( balloonAddSpy ); } ); - it( 'should not add #toolbarView to the #_balloon when editor is not focused', () => { - setData( editor.document, 'b[a]r' ); - editor.editing.view.isFocused = false; + describe( 'on #_selectionChangeDebounced event', () => { + let showSpy; - contextualToolbar._showPanel(); - sinon.assert.notCalled( balloonAddSpy ); - } ); + beforeEach( () => { + showSpy = sinon.spy( contextualToolbar, 'show' ); + } ); - it( 'should not add #toolbarView to the #_balloon when selection is collapsed', () => { - setData( editor.document, 'b[]ar' ); + it( 'should not be called when the editor is not focused', () => { + setData( editor.document, 'b[a]r' ); + editor.editing.view.isFocused = false; - contextualToolbar._showPanel(); - sinon.assert.notCalled( balloonAddSpy ); + contextualToolbar.fire( '_selectionChangeDebounced' ); + sinon.assert.notCalled( showSpy ); + } ); + + it( 'should not be called when the selection is collapsed', () => { + setData( editor.document, 'b[]ar' ); + + contextualToolbar.fire( '_selectionChangeDebounced' ); + sinon.assert.notCalled( showSpy ); + } ); + + it( 'should be called when the selection is not collapsed and editor is focused', () => { + setData( editor.document, 'b[a]r' ); + editor.editing.view.isFocused = true; + + contextualToolbar.fire( '_selectionChangeDebounced' ); + sinon.assert.calledOnce( showSpy ); + } ); } ); } ); - describe( '_hidePanel()', () => { + describe( 'hide()', () => { let removeBalloonSpy; beforeEach( () => { @@ -240,9 +260,9 @@ describe( 'ContextualToolbar', () => { it( 'should remove #toolbarView from the #_balloon', () => { setData( editor.document, 'b[a]r' ); - contextualToolbar._showPanel(); + contextualToolbar.show(); - contextualToolbar._hidePanel(); + contextualToolbar.hide(); sinon.assert.calledWithExactly( removeBalloonSpy, contextualToolbar.toolbarView ); } ); @@ -251,15 +271,15 @@ describe( 'ContextualToolbar', () => { const spy = sandbox.spy( balloon, 'updatePosition' ); - contextualToolbar._showPanel(); - contextualToolbar._hidePanel(); + contextualToolbar.show(); + contextualToolbar.hide(); editor.editing.view.fire( 'render' ); sinon.assert.notCalled( spy ); } ); it( 'should not remove #ttolbarView when is not added to the #_balloon', () => { - contextualToolbar._hidePanel(); + contextualToolbar.hide(); sinon.assert.notCalled( removeBalloonSpy ); } ); @@ -295,8 +315,8 @@ describe( 'ContextualToolbar', () => { beforeEach( () => { setData( editor.document, '[bar]' ); - showPanelSpy = sandbox.spy( contextualToolbar, '_showPanel' ); - hidePanelSpy = sandbox.spy( contextualToolbar, '_hidePanel' ); + showPanelSpy = sandbox.spy( contextualToolbar, 'show' ); + hidePanelSpy = sandbox.spy( contextualToolbar, 'hide' ); } ); it( 'should open when selection stops changing', () => { @@ -395,7 +415,7 @@ describe( 'ContextualToolbar', () => { contextualToolbar.on( 'beforeShow', spy ); setData( editor.document, 'b[a]r' ); - contextualToolbar._showPanel(); + contextualToolbar.show(); sinon.assert.calledOnce( spy ); } ); @@ -408,7 +428,7 @@ describe( 'ContextualToolbar', () => { stop(); } ); - contextualToolbar._showPanel(); + contextualToolbar.show(); sinon.assert.notCalled( balloonAddSpy ); } ); } ); @@ -421,14 +441,8 @@ describe( 'ContextualToolbar', () => { sandbox.stub( editingView.domConverter, 'viewRangeToDom', ( ...args ) => { const domRange = originalViewRangeToDom.apply( editingView.domConverter, args ); - sandbox.stub( domRange, 'getClientRects', () => { - return { - length: 2, - item( id ) { - return id === 0 ? forwardSelectionRect : backwardSelectionRect; - } - }; - } ); + sandbox.stub( domRange, 'getClientRects' ) + .returns( [ forwardSelectionRect, backwardSelectionRect ] ); return domRange; } ); diff --git a/tests/toolbar/enabletoolbarkeyboardfocus.js b/tests/toolbar/enabletoolbarkeyboardfocus.js index b4d9527d..1a2bef6c 100644 --- a/tests/toolbar/enabletoolbarkeyboardfocus.js +++ b/tests/toolbar/enabletoolbarkeyboardfocus.js @@ -28,7 +28,7 @@ describe( 'enableToolbarKeyboardFocus()', () => { toolbar } ); - return toolbar.init(); + toolbar.init(); } ); it( 'focuses the toolbar on Alt+F10', () => { @@ -66,7 +66,8 @@ describe( 'enableToolbarKeyboardFocus()', () => { it( 're–foucuses origin on Esc', () => { const spy = origin.focus = sinon.spy(); const toolbarFocusTracker = toolbar.focusTracker; - const keyEvtData = { keyCode: keyCodes.esc, + const keyEvtData = { + keyCode: keyCodes.esc, preventDefault: sinon.spy(), stopPropagation: sinon.spy() }; @@ -84,6 +85,55 @@ describe( 'enableToolbarKeyboardFocus()', () => { sinon.assert.calledOnce( keyEvtData.preventDefault ); sinon.assert.calledOnce( keyEvtData.stopPropagation ); } ); + + it( 'supports beforeFocus and afterBlur callbacks', () => { + const beforeFocus = sinon.spy(); + const afterBlur = sinon.spy(); + + origin = viewCreator(); + originFocusTracker = new FocusTracker(); + originKeystrokeHandler = new KeystrokeHandler(); + toolbar = new ToolbarView(); + + const toolbarFocusSpy = sinon.spy( toolbar, 'focus' ); + const originFocusSpy = origin.focus = sinon.spy(); + const toolbarFocusTracker = toolbar.focusTracker; + + enableToolbarKeyboardFocus( { + origin, + originFocusTracker, + originKeystrokeHandler, + toolbar, + beforeFocus, + afterBlur + } ); + + toolbar.init(); + + let keyEvtData = { + keyCode: keyCodes.f10, + altKey: true, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + toolbarFocusTracker.isFocused = false; + originFocusTracker.isFocused = true; + + originKeystrokeHandler.press( keyEvtData ); + sinon.assert.callOrder( beforeFocus, toolbarFocusSpy ); + + keyEvtData = { + keyCode: keyCodes.esc, + preventDefault: sinon.spy(), + stopPropagation: sinon.spy() + }; + + toolbarFocusTracker.isFocused = true; + + toolbar.keystrokes.press( keyEvtData ); + sinon.assert.callOrder( originFocusSpy, afterBlur ); + } ); } ); function viewCreator( name ) { From 3ee49d5c8a9e90b73968d7ddb75f4158b1c9559b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 10 Jul 2017 11:14:44 +0200 Subject: [PATCH 2/3] Removed obsolete condition already handled in _selectionChangeDebounced listener. --- src/toolbar/contextual/contextualtoolbar.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index bde000c5..135a2cc7 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -157,11 +157,6 @@ export default class ContextualToolbar extends Plugin { return; } - // This implementation assumes that only non–collapsed selections gets the contextual toolbar. - if ( !editingView.isFocused || editingView.selection.isCollapsed ) { - return; - } - // If `beforeShow` event is not stopped by any external code then panel will be displayed. this.once( 'beforeShow', () => { if ( isStopped ) { From 7a5e6de1662e9ebe130488192a595fe68fcc83f4 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Jul 2017 12:52:23 +0200 Subject: [PATCH 3/3] Docs: Fixed and extended documentation of enableToolbarKeyboardFocus() helper and ContextualToolbar plugin. --- src/toolbar/contextual/contextualtoolbar.js | 4 ++-- src/toolbar/enabletoolbarkeyboardfocus.js | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index 135a2cc7..40daf823 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -144,7 +144,7 @@ export default class ContextualToolbar extends Plugin { } /** - * Adds panel view to the {@link: #_balloon} and attaches panel to the selection. + * Shows the toolbar and attaches it to the selection. * * Fires {@link #event:beforeShow} event just before displaying the panel. */ @@ -185,7 +185,7 @@ export default class ContextualToolbar extends Plugin { } /** - * Removes the toolbar from the {@link: #_balloon} which hides it. + * Hides the toolbar. */ hide() { if ( this._balloon.hasView( this.toolbarView ) ) { diff --git a/src/toolbar/enabletoolbarkeyboardfocus.js b/src/toolbar/enabletoolbarkeyboardfocus.js index 56a98540..712f99a1 100644 --- a/src/toolbar/enabletoolbarkeyboardfocus.js +++ b/src/toolbar/enabletoolbarkeyboardfocus.js @@ -19,6 +19,10 @@ * for `options.origin`. * @param {module:ui/toolbar/toolbarview~ToolbarView} options.toolbar A toolbar which is to gain * focus when `Alt+F10` is pressed. + * @param {Function} [options.beforeFocus] A callback executed before the `options.toolbar` gains focus + * upon the `Alt+F10` keystroke. + * @param {Function} [options.afterBlur] A callback executed after `options.toolbar` loses focus upon + * `Esc` keystroke but before the focus goes back to `options.origin`. */ export default function enableToolbarKeyboardFocus( { origin,