Skip to content

Commit

Permalink
Merge pull request #7378 from ckeditor/i/6443
Browse files Browse the repository at this point in the history
Fix (ui): The `BalloonToolbar` should not show up when multiple objects (for instance, table cells) are selected at a time. Closes #6443.
  • Loading branch information
oleq committed Jun 5, 2020
2 parents 669d54f + 81525ba commit 6036d4a
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 2 deletions.
1 change: 1 addition & 0 deletions packages/ckeditor5-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"@ckeditor/ckeditor5-list": "^19.0.1",
"@ckeditor/ckeditor5-mention": "^19.0.1",
"@ckeditor/ckeditor5-paragraph": "^19.1.0",
"@ckeditor/ckeditor5-horizontal-line": "^19.0.1",
"@ckeditor/ckeditor5-typing": "^19.0.1"
},
"engines": {
Expand Down
30 changes: 29 additions & 1 deletion packages/ckeditor5-ui/src/toolbar/balloon/balloontoolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,14 +209,22 @@ export default class BalloonToolbar extends Plugin {
*/
show() {
const editor = this.editor;
const selection = editor.model.document.selection;
const schema = editor.model.schema;

// Do not add the toolbar to the balloon stack twice.
if ( this._balloon.hasView( this.toolbarView ) ) {
return;
}

// Do not show the toolbar when the selection is collapsed.
if ( editor.model.document.selection.isCollapsed ) {
if ( selection.isCollapsed ) {
return;
}

// Do not show the toolbar when there is more than one range in the selection and they fully contain object elements.
// See https://github.com/ckeditor/ckeditor5/issues/6443.
if ( selectionContainsOnlyMultipleObjects( selection, schema ) ) {
return;
}

Expand Down Expand Up @@ -356,6 +364,26 @@ function getBalloonPositions( isBackward ) {
];
}

// Returns "true" when the selection has multiple ranges and each range contains an object
// and nothing else.
//
// @private
// @param {module:engine/model/selection~Selection} selection
// @param {module:engine/model/schema~Schema} schema
// @returns {Boolean}
function selectionContainsOnlyMultipleObjects( selection, schema ) {
// It doesn't contain multiple objects if there is only one range.
if ( selection.rangeCount === 1 ) {
return false;
}

return [ ...selection.getRanges() ].every( range => {
const element = range.getContainedElement();

return element && schema.isObject( element );
} );
}

/**
* Contextual toolbar configuration. Used by the {@link module:ui/toolbar/balloon/balloontoolbar~BalloonToolbar}
* feature.
Expand Down
12 changes: 11 additions & 1 deletion packages/ckeditor5-ui/tests/toolbar/balloon/balloontoolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic';
import Underline from '@ckeditor/ckeditor5-basic-styles/src/underline';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import HorizontalLine from '@ckeditor/ckeditor5-horizontal-line/src/horizontalline';
import global from '@ckeditor/ckeditor5-utils/src/dom/global';
import ResizeObserver from '@ckeditor/ckeditor5-utils/src/dom/resizeobserver';

Expand Down Expand Up @@ -55,7 +56,7 @@ describe( 'BalloonToolbar', () => {

return ClassicTestEditor
.create( editorElement, {
plugins: [ Paragraph, Bold, Italic, BalloonToolbar ],
plugins: [ Paragraph, Bold, Italic, BalloonToolbar, HorizontalLine ],
balloonToolbar: [ 'bold', 'italic' ]
} )
.then( newEditor => {
Expand Down Expand Up @@ -374,6 +375,15 @@ describe( 'BalloonToolbar', () => {
sinon.assert.notCalled( balloonAddSpy );
} );

// https://github.com/ckeditor/ckeditor5/issues/6443
it( 'should not add the #toolbarView to the #_balloon when the selection contains more than one fully contained object', () => {
// This is for multi cell selection in tables.
setData( model, '[<horizontalLine></horizontalLine>]<paragraph>foo</paragraph>[<horizontalLine></horizontalLine>]' );

balloonToolbar.show();
sinon.assert.notCalled( balloonAddSpy );
} );

it( 'should not add #toolbarView to the #_balloon when all components inside #toolbarView are disabled', () => {
Array.from( balloonToolbar.toolbarView.items ).forEach( item => {
item.isEnabled = false;
Expand Down

0 comments on commit 6036d4a

Please sign in to comment.