From 2bef2e9e95b43cf96a37ce7f61ea33647df677ab Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Fri, 13 Mar 2020 14:00:49 +0100 Subject: [PATCH 1/9] TableSelection plugin should collapse a multi-cell selection when it gets disabled. --- src/tableselection.js | 29 +++++++++++++++++++++++++++++ tests/tableselection.js | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/src/tableselection.js b/src/tableselection.js index 78858fd0..b1eef6f6 100644 --- a/src/tableselection.js +++ b/src/tableselection.js @@ -57,6 +57,7 @@ export default class TableSelection extends Plugin { this._defineSelectionConverter(); this._enableShiftClickSelection(); this._enableMouseDragSelection(); + this._enablePluginDisabling(); // sic! } /** @@ -291,6 +292,34 @@ export default class TableSelection extends Plugin { }, { priority: 'highest' } ); } + /** + * Creates a listener that reacts to changes in {@link #isEnabled} and, if the plugin was disabled, + * it collapses the multi-cell selection to a regular selection placed inside a table cell. + * + * This listener helps features that disable the table selection plugin bring the selection + * to a clear state they can work with (for instance, because they don't support multiple cell selection). + */ + _enablePluginDisabling() { + const editor = this.editor; + + this.on( 'change:isEnabled', () => { + if ( !this.isEnabled ) { + const selectedCells = this.getSelectedTableCells(); + + if ( !selectedCells ) { + return; + } + + editor.model.change( writer => { + const position = writer.createPositionAt( selectedCells[ 0 ], 0 ); + const range = editor.model.schema.getNearestSelectionRange( position ); + + writer.setSelection( range ); + } ); + } + } ); + } + /** * It overrides the default `model.deleteContent()` behavior over a selected table fragment. * diff --git a/tests/tableselection.js b/tests/tableselection.js index e60875e8..22bfacb6 100644 --- a/tests/tableselection.js +++ b/tests/tableselection.js @@ -37,6 +37,45 @@ describe( 'table selection', () => { afterEach( async () => { await editor.destroy(); } ); + + describe( 'init()', () => { + describe( 'plugin disabling support', () => { + let plugin; + + beforeEach( () => { + plugin = editor.plugins.get( TableSelection ); + } ); + + it( 'should collapse multi-cell selection when the plugin gets disabled', () => { + const firstCell = modelRoot.getNodeByPath( [ 0, 0, 0 ] ); + const lastCell = modelRoot.getNodeByPath( [ 0, 1, 1 ] ); + + tableSelection._setCellSelection( + firstCell, + lastCell + ); + + plugin.forceDisabled( 'foo' ); + + const ranges = [ ...model.document.selection.getRanges() ]; + + expect( ranges ).to.have.length( 1 ); + expect( ranges[ 0 ].isCollapsed ).to.be.true; + expect( ranges[ 0 ].start.path ).to.deep.equal( [ 0, 0, 0, 0, 0 ] ); + } ); + + it( 'should do nothing if there were no multi-cell selections', () => { + plugin.forceDisabled( 'foo' ); + + const ranges = [ ...model.document.selection.getRanges() ]; + + expect( ranges ).to.have.length( 1 ); + expect( ranges[ 0 ].isCollapsed ).to.be.true; + expect( ranges[ 0 ].start.path ).to.deep.equal( [ 0, 0, 0, 0, 2 ] ); + } ); + } ); + } ); + describe( 'selection by shift+click', () => { it( 'should...', () => { // tableSelection.startSelectingFrom( modelRoot.getNodeByPath( [ 0, 0, 0 ] ) ); From 9c705b7899fbbd564b2fe6558ec0444eb83565a7 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Tue, 24 Mar 2020 09:25:21 +0100 Subject: [PATCH 2/9] Tests for a table selection mechanism. --- src/tableselection.js | 2 + tests/tableselection.js | 635 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 598 insertions(+), 39 deletions(-) diff --git a/src/tableselection.js b/src/tableselection.js index 65185c14..0e8fe2ab 100644 --- a/src/tableselection.js +++ b/src/tableselection.js @@ -75,6 +75,7 @@ export default class TableSelection extends Plugin { } // This should never happen, but let's know if it ever happens. + // @if CK_DEBUG // /* istanbul ignore next */ // @if CK_DEBUG // if ( selectedCells.length != selection.rangeCount ) { // @if CK_DEBUG // console.warn( 'Mixed selection warning. The selection contains table cells and some other ranges.' ); // @if CK_DEBUG // } @@ -365,6 +366,7 @@ export default class TableSelection extends Plugin { const modelPosition = this.editor.editing.mapper.toModelPosition( viewPosition ); const modelElement = modelPosition.parent; + // TODO: Required? if ( !modelElement ) { return; } diff --git a/tests/tableselection.js b/tests/tableselection.js index e60875e8..5d893ab3 100644 --- a/tests/tableselection.js +++ b/tests/tableselection.js @@ -3,71 +3,545 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* globals document, console */ + import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; -import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; -import { setData as setModelData, stringify as stringifyModel } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; +import { + getData as getModelData, + setData as setModelData, + stringify as stringifyModel +} from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import TableEditing from '../src/tableediting'; import TableSelection from '../src/tableselection'; -// import { assertSelectedCells, modelTable, viewTable } from './_utils/utils'; -import { modelTable } from './_utils/utils'; -// import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; -// import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; +import { assertSelectedCells, modelTable } from './_utils/utils'; import DocumentFragment from '@ckeditor/ckeditor5-engine/src/model/documentfragment'; +import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata'; +import Typing from '@ckeditor/ckeditor5-typing/src/typing'; +import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; describe( 'table selection', () => { - let editor, model, tableSelection, modelRoot; - - beforeEach( async () => { - editor = await VirtualTestEditor.create( { - plugins: [ TableEditing, TableSelection, Paragraph ] - } ); - - model = editor.model; - modelRoot = model.document.getRoot(); - tableSelection = editor.plugins.get( TableSelection ); + let editorElement, editor, model, tableSelection, modelRoot, view, viewDocument; - setModelData( model, modelTable( [ - [ '11[]', '12', '13' ], - [ '21', '22', '23' ], - [ '31', '32', '33' ] - ] ) ); + beforeEach( () => { + editorElement = document.createElement( 'div' ); + document.body.appendChild( editorElement ); } ); afterEach( async () => { + editorElement.remove(); await editor.destroy(); } ); + describe( 'selection by shift+click', () => { - it( 'should...', () => { - // tableSelection.startSelectingFrom( modelRoot.getNodeByPath( [ 0, 0, 0 ] ) ); - // tableSelection.setSelectingTo( modelRoot.getNodeByPath( [ 0, 0, 1 ] ) ); + beforeEach( async () => { + // Disables attaching drag mouse events. + sinon.stub( TableSelection.prototype, '_enableMouseDragSelection' ); + + editor = await createEditor(); + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + tableSelection = editor.plugins.get( TableSelection ); + + setModelData( model, modelTable( [ + [ '11[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + afterEach( () => { + TableSelection.prototype._enableMouseDragSelection.restore(); + } ); + + it( 'should do nothing if the plugin is disabled', () => { + tableSelection.isEnabled = false; + + viewDocument.fire( 'mousedown', new DomEventData( view, {} ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should abort if Shift key was not pressed', () => { + viewDocument.fire( 'mousedown', new DomEventData( view, { + shiftKey: false + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should abort if started selecting elements outside the table', () => { + const preventDefault = sinon.spy(); + + model.change( writer => { + const paragraph = writer.createElement( 'paragraph' ); + const text = writer.createText( 'foo' ); + + writer.insert( text, paragraph ); + writer.insert( paragraph, model.document.getRoot(), 'end' ); + writer.setSelection( paragraph, 'end' ); + } ); + + viewDocument.fire( 'mousedown', new DomEventData( view, { + shiftKey: true, + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 1 ) + ), + preventDefault + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + + expect( preventDefault.called ).to.equal( false ); + } ); + + it( 'should abort if clicked a cell that belongs to another table', () => { + const preventDefault = sinon.spy(); + + setModelData( model, [ + modelTable( [ + [ '1.11[]', '1.12' ], + [ '1.21', '1.22' ] + ] ), + modelTable( [ + [ '2.11', '2.12' ], + [ '2.21', '2.22' ] + ] ) + ].join( '' ) ); + + const domEventDataMock = new DomEventData( view, { + shiftKey: true, + target: view.domConverter.mapViewToDom( + // The second table: figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 1 ).getChild( 1 ).getChild( 0 ).getChild( 1 ).getChild( 1 ) + ), + preventDefault + } ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 0, 0 ], + [ 0, 0 ] + ] ); + + expect( preventDefault.called ).to.equal( false ); + } ); + + it( 'should select all cells in first row', () => { + const preventDefault = sinon.spy(); + + const domEventDataMock = new DomEventData( view, { + shiftKey: true, + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 2 ) + ), + preventDefault + } ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 1, 1, 1 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); - // tableSelection.stopSelection(); + expect( preventDefault.called ).to.equal( true ); + } ); + + it( 'should ignore `selectionChange` event when selecting cells ', () => { + const consoleLog = sinon.stub( console, 'log' ); + const preventDefault = sinon.spy(); + const selectionChangeCallback = sinon.spy(); + + // Adding a new callback to check whether it will be executed (whether `evt.stop()` is being called). + viewDocument.on( 'selectionChange', selectionChangeCallback ); + + // No changes were made. + expect( selectionChangeCallback.called ).to.equal( false ); + + // Start selecting a cell. Disables listening to `selectionChange`. + viewDocument.fire( 'mousedown', new DomEventData( view, { + shiftKey: true, + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 2 ) + ), + preventDefault + } ) ); + + // The callback shouldn't be executed. + viewDocument.fire( 'selectionChange' ); + + expect( selectionChangeCallback.called ).to.equal( false ); + + // `selectionChange` event should be canceled. + expect( selectionChangeCallback.called ).to.equal( false ); - // assertSelectedCells( model, [ - // [ 1, 1, 0 ], - // [ 0, 0, 0 ], - // [ 0, 0, 0 ] - // ] ); + // Enables listening to `selectionChange` event. + viewDocument.fire( 'mouseup' ); + + viewDocument.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( selectionChangeCallback.called ).to.equal( true ); + expect( consoleLog.called ).to.equal( true ); + expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); + + consoleLog.restore(); } ); } ); describe( 'selection by mouse drag', () => { - it( 'should...', () => { - // tableSelection.startSelectingFrom( modelRoot.getNodeByPath( [ 0, 0, 0 ] ) ); - // tableSelection.setSelectingTo( modelRoot.getNodeByPath( [ 0, 0, 1 ] ) ); + let preventDefault; + + beforeEach( async () => { + // Disables attaching mouse+Shift events. + sinon.stub( TableSelection.prototype, '_enableShiftClickSelection' ); + + editor = await createEditor(); + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + tableSelection = editor.plugins.get( TableSelection ); + + setModelData( model, modelTable( [ + [ '11[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); - // tableSelection.stopSelection(); + preventDefault = sinon.spy(); + } ); - // assertSelectedCells( model, [ - // [ 1, 1, 0 ], - // [ 0, 0, 0 ], - // [ 0, 0, 0 ] - // ] ); + afterEach( () => { + TableSelection.prototype._enableShiftClickSelection.restore(); + } ); + + it( 'should do nothing if the plugin is disabled', () => { + tableSelection.isEnabled = false; + + const domEventDataMock = new DomEventData( view, {} ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should abort if Shift is pressed', () => { + const domEventDataMock = new DomEventData( view, { + shiftKey: true + } ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should abort if Ctrl is pressed', () => { + const domEventDataMock = new DomEventData( view, { + ctrlKey: true + } ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should abort if Alt is pressed', () => { + const domEventDataMock = new DomEventData( view, { + altKey: true + } ); + + viewDocument.fire( 'mousedown', domEventDataMock ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should do nothing if any of mouse buttons was not clicked', () => { + viewDocument.fire( 'mousemove', new DomEventData( view, { + buttons: 0 + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should do nothing if started dragging outside of table', () => { + model.change( writer => { + const paragraph = writer.createElement( 'paragraph' ); + const text = writer.createText( 'foo' ); + + writer.insert( text, paragraph ); + writer.insert( paragraph, model.document.getRoot(), 'end' ); + writer.setSelection( paragraph, 'end' ); + } ); + + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 1 ) + ) + } ) ); + + viewDocument.fire( 'mousemove', new DomEventData( view, { + buttons: 1 + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should do nothing if ended dragging outside of table', () => { + model.change( writer => { + const paragraph = writer.createElement( 'paragraph' ); + const text = writer.createText( 'foo' ); + + writer.insert( text, paragraph ); + writer.insert( paragraph, model.document.getRoot(), 'end' ); + writer.setSelection( paragraph, 'end' ); + } ); + + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ) + } ) ); + + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 1 ) + ), + buttons: 1 + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should do nothing if ended dragging inside another table', () => { + setModelData( model, [ + modelTable( [ + [ '1.11[]', '1.12' ], + [ '1.21', '1.22' ] + ] ), + modelTable( [ + [ '2.11', '2.12' ], + [ '2.21', '2.22' ] + ] ) + ].join( '' ) ); + + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ) + } ) ); + + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 1 ).getChild( 1 ).getChild( 0 ).getChild( 1 ).getChild( 1 ) + ), + buttons: 1 + } ) ); + + assertSelectedCells( model, [ + [ 0, 0 ], + [ 0, 0 ] + ] ); + } ); + + it( 'should do nothing if ended in the same cell', () => { + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ) + } ) ); + + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ), + buttons: 1 + } ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should select started and ended dragging in the same cell but went over its border', () => { + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ) + } ) ); + + // Select the next one. + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 1 ) + ), + buttons: 1, + preventDefault: sinon.spy() + } ) ); + + // And back to the "started" cell. + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ), + buttons: 1, + preventDefault: sinon.spy() + } ) ); + + viewDocument.fire( 'mouseup' ); + + assertSelectedCells( model, [ + [ 1, 0, 0 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + + it( 'should select all cells in first row', () => { + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 0 ) + ) + } ) ); + + viewDocument.fire( 'mousemove', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 2 ) + ), + buttons: 1, + preventDefault + } ) ); + + viewDocument.fire( 'mouseup' ); + + assertSelectedCells( model, [ + [ 1, 1, 1 ], + [ 0, 0, 0 ], + [ 0, 0, 0 ] + ] ); + + expect( preventDefault.called ).to.equal( true ); + } ); + + it( 'should ignore `selectionChange` event when selecting cells ', () => { + const consoleLog = sinon.stub( console, 'log' ); + const preventDefault = sinon.spy(); + const selectionChangeCallback = sinon.spy(); + + // Adding a new callback to check whether it will be executed (whether `evt.stop()` is being called). + viewDocument.on( 'selectionChange', selectionChangeCallback ); + + // No changes were made. + expect( selectionChangeCallback.called ).to.equal( false ); + + // Click on a cell. + viewDocument.fire( 'mousedown', new DomEventData( view, { + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 1 ) + ) + } ) ); + + // Then move the mouse to another cell. Disables listening to `selectionChange`. + viewDocument.fire( 'mousemove', new DomEventData( view, { + buttons: 1, + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 2 ) + ), + preventDefault + } ) ); + + // The callback shouldn't be executed. + viewDocument.fire( 'selectionChange' ); + + // `selectionChange` event should be canceled. + expect( selectionChangeCallback.called ).to.equal( false ); + + // Enables listening to `selectionChange` event. + viewDocument.fire( 'mouseup' ); + + viewDocument.fire( 'selectionChange', { + newSelection: view.document.selection + } ); + + expect( selectionChangeCallback.called ).to.equal( true ); + expect( consoleLog.called ).to.equal( true ); + expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); + + consoleLog.restore(); } ); } ); describe( 'getSelectedTableCells()', () => { + beforeEach( async () => { + editor = await createEditor(); + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + tableSelection = editor.plugins.get( TableSelection ); + + setModelData( model, modelTable( [ + [ '11[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + it( 'should return nothing if selection is not started', () => { expect( tableSelection.getSelectedTableCells() ).to.be.null; } ); @@ -138,6 +612,21 @@ describe( 'table selection', () => { } ); describe( 'getSelectionAsFragment()', () => { + beforeEach( async () => { + editor = await createEditor(); + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + tableSelection = editor.plugins.get( TableSelection ); + + setModelData( model, modelTable( [ + [ '11[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + it( 'should return undefined if no table cells are selected', () => { expect( tableSelection.getSelectionAsFragment() ).to.be.null; } ); @@ -177,4 +666,72 @@ describe( 'table selection', () => { ] ) ); } ); } ); + + describe( 'delete content', () => { + beforeEach( async () => { + editor = await createEditor(); + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + tableSelection = editor.plugins.get( TableSelection ); + + setModelData( model, modelTable( [ + [ '11[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + it( 'should put selection in the last selected cell after removing content (backward delete)', () => { + tableSelection._setCellSelection( + modelRoot.getChild( 0 ).getChild( 0 ).getChild( 0 ), + modelRoot.getChild( 0 ).getChild( 1 ).getChild( 1 ) + ); + + editor.execute( 'delete' ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '', '', '13' ], + [ '', '[]', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + it( 'should put selection in the last selected cell after removing content (forward delete)', () => { + tableSelection._setCellSelection( + modelRoot.getChild( 0 ).getChild( 0 ).getChild( 0 ), + modelRoot.getChild( 0 ).getChild( 1 ).getChild( 1 ) + ); + + editor.execute( 'delete' ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '', '', '13' ], + [ '', '[]', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + it( 'should clear single cell if selected', () => { + tableSelection._setCellSelection( + modelRoot.getChild( 0 ).getChild( 0 ).getChild( 0 ), + modelRoot.getChild( 0 ).getChild( 0 ).getChild( 0 ), + ); + + editor.execute( 'forwardDelete' ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '[]', '12', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + } ); + + function createEditor() { + return ClassicTestEditor.create( editorElement, { + plugins: [ TableEditing, TableSelection, Paragraph, Typing ] + } ); + } } ); From 154e582a93ae337233051969cef9ef3a1709ab89 Mon Sep 17 00:00:00 2001 From: Anna Tomanek Date: Tue, 24 Mar 2020 12:39:36 +0100 Subject: [PATCH 3/9] Docs: Table selection section added to the guide. [skip ci] --- docs/features/table.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/features/table.md b/docs/features/table.md index 8138ec90..a8590abc 100644 --- a/docs/features/table.md +++ b/docs/features/table.md @@ -31,6 +31,15 @@ Put the caret anywhere inside the table and click the **"Table properties"** but By default, table styling tools are not included in the {@link builds/guides/overview ready–to–use editor builds} and must be installed separately. See the [installation](#table-and-cell-styling-tools-2) section to learn how to enable them in your editor. +## Table selection + +The {@link module:table/tableselection~TableSelection} plugin introduces support for the custom selection system for tables that lets you: + +* Select an arbitrary rectangular table fragment — a few cells from different rows, a column (or a few of them) or a row (or multiple rows). +* Apply formatting or add a link to all selected cells at once. + +The table selection plugin is loaded automatically by the `Table` plugin and can be tested in the [demos above](#demos). + ## Installation ### Basic table features From 2f55c6a32ef9e0779a72ed0e38363d97dc24f57c Mon Sep 17 00:00:00 2001 From: Anna Tomanek Date: Tue, 24 Mar 2020 12:51:27 +0100 Subject: [PATCH 4/9] Docs: Table selection API docs corrected. [skip ci] --- src/tableselection.js | 20 ++++++++++---------- src/tableselection/croptable.js | 6 +++--- src/tableselection/mouseeventsobserver.js | 14 +++++++------- src/utils.js | 2 +- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/tableselection.js b/src/tableselection.js index 65185c14..8015c332 100644 --- a/src/tableselection.js +++ b/src/tableselection.js @@ -22,7 +22,7 @@ import cropTable from './tableselection/croptable'; import '../theme/tableselection.css'; /** - * This plugin enables the advanced table cells/rows/columns selection. + * This plugin enables the advanced table cells, rows and columns selection. * It is loaded automatically by the {@link module:table/table~Table} plugin. * * @extends module:core/plugin~Plugin @@ -61,7 +61,7 @@ export default class TableSelection extends Plugin { } /** - * Returns currently selected table cells or `null` if not a table cells selection. + * Returns the currently selected table cells or `null` if it is not a table cells selection. * * @returns {Array.|null} */ @@ -83,7 +83,7 @@ export default class TableSelection extends Plugin { } /** - * Returns a selected table fragment as a document fragment. + * Returns the selected table fragment as a document fragment. * * @returns {module:engine/model/documentfragment~DocumentFragment|null} */ @@ -105,10 +105,10 @@ export default class TableSelection extends Plugin { } /** - * Defines a selection converter which marks selected cells with a specific class. + * Defines a selection converter which marks the selected cells with a specific class. * * The real DOM selection is put in the last cell. Since the order of ranges is dependent on whether the - * selection is backward or not, the last cell with usually be close to the "focus" end of the selection + * selection is backward or not, the last cell will usually be close to the "focus" end of the selection * (a selection has anchor and focus). * * The real DOM selection is then hidden with CSS. @@ -151,8 +151,8 @@ export default class TableSelection extends Plugin { } /** - * Enables making cells selection by Shift+click. Creates a selection from the cell which previously hold - * the selection to the cell which was clicked (can be the same cell, in which case it selects a single cell). + * Enables making cells selection by Shift+click. Creates a selection from the cell which previously held + * the selection to the cell which was clicked. It can be the same cell, in which case it selects a single cell. * * @private */ @@ -217,7 +217,7 @@ export default class TableSelection extends Plugin { /** * Enables making cells selection by dragging. * - * The selection is made only on mousemove. We start tracking the mouse on mousedown. + * The selection is made only on mousemove. Mouse tracking is started on mousedown. * However, the cells selection is enabled only after the mouse cursor left the anchor cell. * Thanks to that normal text selection within one cell works just fine. However, you can still select * just one cell by leaving the anchor cell and moving back to it. @@ -293,7 +293,7 @@ export default class TableSelection extends Plugin { } /** - * It overrides the default `model.deleteContent()` behavior over a selected table fragment. + * Overrides the default `model.deleteContent()` behavior over a selected table fragment. * * @private * @param {module:utils/eventinfo~EventInfo} event @@ -334,7 +334,7 @@ export default class TableSelection extends Plugin { /** * Sets the model selection based on given anchor and target cells (can be the same cell). - * Takes care of setting backward flag. + * Takes care of setting the backward flag. * * @protected * @param {module:engine/model/element~Element} anchorCell diff --git a/src/tableselection/croptable.js b/src/tableselection/croptable.js index 050304b7..656c9b13 100644 --- a/src/tableselection/croptable.js +++ b/src/tableselection/croptable.js @@ -10,16 +10,16 @@ import { findAncestor } from '../commands/utils'; /** - * Returns cropped table from selected table cells. + * Returns a cropped table from the selected table cells. * - * This is to be used with table selection + * This function is to be used with the table selection. * * tableSelection.startSelectingFrom( startCell ) * tableSelection.setSelectingFrom( endCell ) * * const croppedTable = cropTable( tableSelection.getSelectedTableCells() ); * - * **Note**: This function is used also by {@link module:table/tableselection~TableSelection#getSelectionAsFragment} + * **Note**: This function is also used by {@link module:table/tableselection~TableSelection#getSelectionAsFragment}. * * @param {Iterable.} selectedTableCellsIterator * @param {module:table/tableutils~TableUtils} tableUtils diff --git a/src/tableselection/mouseeventsobserver.js b/src/tableselection/mouseeventsobserver.js index 47098c63..e8c3b713 100644 --- a/src/tableselection/mouseeventsobserver.js +++ b/src/tableselection/mouseeventsobserver.js @@ -10,18 +10,18 @@ import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver'; /** - * The mouse selection events observer. + * The mouse selection event observer. * - * It registers listeners for DOM events: + * It registers listeners for the following DOM events: * * - `'mousemove'` * - `'mouseup'` * - `'mouseleave'` * - * Note that this observer is disabled by default. To enable this observer it needs to be added to + * Note that this observer is disabled by default. To enable this observer, it needs to be added to * {@link module:engine/view/view~View} using the {@link module:engine/view/view~View#addObserver} method. * - * It is registered by the {@link module:table/tableselection~TableSelection} plugin. + * The observer is registered by the {@link module:table/tableselection~TableSelection} plugin. * * @extends module:engine/view/observer/domeventobserver~DomEventObserver */ @@ -48,7 +48,7 @@ export default class MouseEventsObserver extends DomEventObserver { * * Introduced by {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver}. * - * Note that this event is not available by default. To make it available + * Note that this event is not available by default. To make it available, * {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver} needs to be added * to {@link module:engine/view/view~View} using the {@link module:engine/view/view~View#addObserver} method. * @@ -62,7 +62,7 @@ export default class MouseEventsObserver extends DomEventObserver { * * Introduced by {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver}. * - * Note that this event is not available by default. To make it available + * Note that this event is not available by default. To make it available, * {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver} needs to be added * to {@link module:engine/view/view~View} using the {@link module:engine/view/view~View#addObserver} method. * @@ -76,7 +76,7 @@ export default class MouseEventsObserver extends DomEventObserver { * * Introduced by {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver}. * - * Note that this event is not available by default. To make it available + * Note that this event is not available by default. To make it available, * {@link module:table/tableselection/mouseeventsobserver~MouseEventsObserver} needs to be added * to {@link module:engine/view/view~View} using the {@link module:engine/view/view~View#addObserver} method. * diff --git a/src/utils.js b/src/utils.js index 2b2caa2f..408884bd 100644 --- a/src/utils.js +++ b/src/utils.js @@ -93,7 +93,7 @@ export function getSelectedTableCells( selection ) { } /** - * Returns all model table cells the provided model selection's ranges + * Returns all model table cells that the provided model selection's ranges * {@link module:engine/model/range~Range#start} inside. * * To obtain the cells selected from the outside, use From 4a2f4cfcefe7d205eab902cc5fd4aabdff653c9b Mon Sep 17 00:00:00 2001 From: panr Date: Tue, 24 Mar 2020 13:19:03 +0100 Subject: [PATCH 5/9] Add more buttons to the Table snippet toolbar --- docs/_snippets/features/build-table-source.js | 8 ++++---- docs/_snippets/features/table-styling-colors.js | 16 +++++++++++++++- docs/_snippets/features/table-styling.js | 16 +++++++++++++++- docs/_snippets/features/table.js | 16 +++++++++++++++- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/docs/_snippets/features/build-table-source.js b/docs/_snippets/features/build-table-source.js index b75aa557..980ec668 100644 --- a/docs/_snippets/features/build-table-source.js +++ b/docs/_snippets/features/build-table-source.js @@ -7,13 +7,13 @@ import ClassicEditor from '@ckeditor/ckeditor5-build-classic/src/ckeditor'; -import Table from '@ckeditor/ckeditor5-table/src/table'; -import TableToolbar from '@ckeditor/ckeditor5-table/src/tabletoolbar'; +import FontFamily from '@ckeditor/ckeditor5-font/src/fontfamily'; +import FontSize from '@ckeditor/ckeditor5-font/src/fontsize'; +import Alignment from '@ckeditor/ckeditor5-alignment/src/alignment'; import TableProperties from '@ckeditor/ckeditor5-table/src/tableproperties'; import TableCellProperties from '@ckeditor/ckeditor5-table/src/tablecellproperties'; -ClassicEditor.builtinPlugins.push( Table ); -ClassicEditor.builtinPlugins.push( TableToolbar ); +ClassicEditor.builtinPlugins.push( FontFamily, FontSize, Alignment ); window.ClassicEditor = ClassicEditor; window.CKEditorPlugins = { diff --git a/docs/_snippets/features/table-styling-colors.js b/docs/_snippets/features/table-styling-colors.js index e99cbb1b..51a4795e 100644 --- a/docs/_snippets/features/table-styling-colors.js +++ b/docs/_snippets/features/table-styling-colors.js @@ -99,7 +99,21 @@ ClassicEditor cloudServices: CS_CONFIG, toolbar: { items: [ - 'insertTable', '|', 'heading', '|', 'bold', 'italic', '|', 'undo', 'redo' + 'insertTable', + '|', + 'fontFamily', 'fontSize', + '|', + 'bold', 'italic', + '|', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + '|', + 'bulletedList', 'numberedList', + '|', + 'indent', 'outdent', + '|', + 'link', 'blockQuote', + '|', + 'undo', 'redo' ], viewportTopOffset: window.getViewportTopOffsetConfig() }, diff --git a/docs/_snippets/features/table-styling.js b/docs/_snippets/features/table-styling.js index 8ca4e93e..1ed9f017 100644 --- a/docs/_snippets/features/table-styling.js +++ b/docs/_snippets/features/table-styling.js @@ -100,7 +100,21 @@ ClassicEditor cloudServices: CS_CONFIG, toolbar: { items: [ - 'insertTable', '|', 'heading', '|', 'bold', 'italic', '|', 'undo', 'redo' + 'insertTable', + '|', + 'fontFamily', 'fontSize', + '|', + 'bold', 'italic', + '|', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + '|', + 'bulletedList', 'numberedList', + '|', + 'indent', 'outdent', + '|', + 'link', 'blockQuote', + '|', + 'undo', 'redo' ], viewportTopOffset: window.getViewportTopOffsetConfig() }, diff --git a/docs/_snippets/features/table.js b/docs/_snippets/features/table.js index bcd64066..ecc03173 100644 --- a/docs/_snippets/features/table.js +++ b/docs/_snippets/features/table.js @@ -12,7 +12,21 @@ ClassicEditor cloudServices: CS_CONFIG, toolbar: { items: [ - 'insertTable', '|', 'heading', '|', 'bold', 'italic', '|', 'undo', 'redo' + 'insertTable', + '|', + 'fontFamily', 'fontSize', + '|', + 'bold', 'italic', + '|', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + '|', + 'bulletedList', 'numberedList', + '|', + 'indent', 'outdent', + '|', + 'link', 'blockQuote', + '|', + 'undo', 'redo' ], viewportTopOffset: window.getViewportTopOffsetConfig() }, From 62da9fa746e393676a50bccd9865bf61cae1fe24 Mon Sep 17 00:00:00 2001 From: panr Date: Tue, 24 Mar 2020 13:24:18 +0100 Subject: [PATCH 6/9] Fix whitespaces --- docs/_snippets/features/table-styling-colors.js | 2 +- docs/_snippets/features/table-styling.js | 2 +- docs/_snippets/features/table.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/_snippets/features/table-styling-colors.js b/docs/_snippets/features/table-styling-colors.js index 51a4795e..639e73f4 100644 --- a/docs/_snippets/features/table-styling-colors.js +++ b/docs/_snippets/features/table-styling-colors.js @@ -105,7 +105,7 @@ ClassicEditor '|', 'bold', 'italic', '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', '|', 'bulletedList', 'numberedList', '|', diff --git a/docs/_snippets/features/table-styling.js b/docs/_snippets/features/table-styling.js index 1ed9f017..8f2ed21a 100644 --- a/docs/_snippets/features/table-styling.js +++ b/docs/_snippets/features/table-styling.js @@ -106,7 +106,7 @@ ClassicEditor '|', 'bold', 'italic', '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', '|', 'bulletedList', 'numberedList', '|', diff --git a/docs/_snippets/features/table.js b/docs/_snippets/features/table.js index ecc03173..8366b389 100644 --- a/docs/_snippets/features/table.js +++ b/docs/_snippets/features/table.js @@ -18,7 +18,7 @@ ClassicEditor '|', 'bold', 'italic', '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', '|', 'bulletedList', 'numberedList', '|', From 3c75814f2adcf3464f973b81c4070b0fc6e162cb Mon Sep 17 00:00:00 2001 From: panr Date: Wed, 25 Mar 2020 09:11:35 +0100 Subject: [PATCH 7/9] Add IndentBlock plugin & make editor config simpler --- docs/_snippets/features/build-table-source.js | 29 ++++++++++++++++++- .../features/table-styling-colors.js | 23 --------------- docs/_snippets/features/table-styling.js | 23 --------------- docs/_snippets/features/table.js | 23 --------------- 4 files changed, 28 insertions(+), 70 deletions(-) diff --git a/docs/_snippets/features/build-table-source.js b/docs/_snippets/features/build-table-source.js index 980ec668..ca75518f 100644 --- a/docs/_snippets/features/build-table-source.js +++ b/docs/_snippets/features/build-table-source.js @@ -10,10 +10,37 @@ import ClassicEditor from '@ckeditor/ckeditor5-build-classic/src/ckeditor'; import FontFamily from '@ckeditor/ckeditor5-font/src/fontfamily'; import FontSize from '@ckeditor/ckeditor5-font/src/fontsize'; import Alignment from '@ckeditor/ckeditor5-alignment/src/alignment'; +import IndentBlock from '@ckeditor/ckeditor5-indent/src/indentblock'; import TableProperties from '@ckeditor/ckeditor5-table/src/tableproperties'; import TableCellProperties from '@ckeditor/ckeditor5-table/src/tablecellproperties'; -ClassicEditor.builtinPlugins.push( FontFamily, FontSize, Alignment ); +import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; + +ClassicEditor.builtinPlugins.push( FontFamily, FontSize, Alignment, IndentBlock ); +ClassicEditor.defaultConfig = { + cloudServices: CS_CONFIG, + toolbar: { + items: [ + 'insertTable', + '|', + 'fontFamily', 'fontSize', + '|', + 'bold', 'italic', + '|', + 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', + '|', + 'bulletedList', 'numberedList', + '|', + 'indent', 'outdent', + '|', + 'link', 'blockQuote', + '|', + 'undo', 'redo' + ], + viewportTopOffset: window.getViewportTopOffsetConfig() + }, + indentBlock: { offset: 30, unit: 'px' } +}; window.ClassicEditor = ClassicEditor; window.CKEditorPlugins = { diff --git a/docs/_snippets/features/table-styling-colors.js b/docs/_snippets/features/table-styling-colors.js index 639e73f4..0d13f1a1 100644 --- a/docs/_snippets/features/table-styling-colors.js +++ b/docs/_snippets/features/table-styling-colors.js @@ -5,8 +5,6 @@ /* globals ClassicEditor, CKEditorPlugins, console, window, document */ -import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; - const customColorPalette = [ { color: 'hsl(4, 90%, 58%)', @@ -96,27 +94,6 @@ ClassicEditor CKEditorPlugins.TableProperties, CKEditorPlugins.TableCellProperties ], - cloudServices: CS_CONFIG, - toolbar: { - items: [ - 'insertTable', - '|', - 'fontFamily', 'fontSize', - '|', - 'bold', 'italic', - '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', - '|', - 'bulletedList', 'numberedList', - '|', - 'indent', 'outdent', - '|', - 'link', 'blockQuote', - '|', - 'undo', 'redo' - ], - viewportTopOffset: window.getViewportTopOffsetConfig() - }, table: { contentToolbar: [ 'tableColumn', 'tableRow', 'mergeTableCells', 'tableProperties', 'tableCellProperties' ], tableProperties: { diff --git a/docs/_snippets/features/table-styling.js b/docs/_snippets/features/table-styling.js index 8f2ed21a..c8f4a97e 100644 --- a/docs/_snippets/features/table-styling.js +++ b/docs/_snippets/features/table-styling.js @@ -5,8 +5,6 @@ /* globals ClassicEditor, CKEditorPlugins, console, window, document */ -import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; - const COLOR_PALETTE = [ { color: 'hsl(0, 0%, 0%)', @@ -97,27 +95,6 @@ ClassicEditor CKEditorPlugins.TableProperties, CKEditorPlugins.TableCellProperties ], - cloudServices: CS_CONFIG, - toolbar: { - items: [ - 'insertTable', - '|', - 'fontFamily', 'fontSize', - '|', - 'bold', 'italic', - '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', - '|', - 'bulletedList', 'numberedList', - '|', - 'indent', 'outdent', - '|', - 'link', 'blockQuote', - '|', - 'undo', 'redo' - ], - viewportTopOffset: window.getViewportTopOffsetConfig() - }, table: { contentToolbar: [ 'tableColumn', 'tableRow', 'mergeTableCells', 'tableProperties', 'tableCellProperties' ], tableProperties: { diff --git a/docs/_snippets/features/table.js b/docs/_snippets/features/table.js index 8366b389..74ccb4b0 100644 --- a/docs/_snippets/features/table.js +++ b/docs/_snippets/features/table.js @@ -5,31 +5,8 @@ /* globals ClassicEditor, console, window, document */ -import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config'; - ClassicEditor .create( document.querySelector( '#snippet-table' ), { - cloudServices: CS_CONFIG, - toolbar: { - items: [ - 'insertTable', - '|', - 'fontFamily', 'fontSize', - '|', - 'bold', 'italic', - '|', - 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify', - '|', - 'bulletedList', 'numberedList', - '|', - 'indent', 'outdent', - '|', - 'link', 'blockQuote', - '|', - 'undo', 'redo' - ], - viewportTopOffset: window.getViewportTopOffsetConfig() - }, table: { contentToolbar: [ 'tableColumn', 'tableRow', 'mergeTableCells' ] } From 4c66093f93e5168589b50da093d11e26b2016ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 25 Mar 2020 09:40:42 +0100 Subject: [PATCH 8/9] Minor improvements. --- tests/tableselection.js | 65 ++++++++++++----------------------------- 1 file changed, 18 insertions(+), 47 deletions(-) diff --git a/tests/tableselection.js b/tests/tableselection.js index cfbcbdcc..b43793b5 100644 --- a/tests/tableselection.js +++ b/tests/tableselection.js @@ -79,11 +79,8 @@ describe( 'table selection', () => { } ); } ); - describe( 'selection by shift+click', () => { + describe( 'selection by Shift+click', () => { beforeEach( async () => { - // Disables attaching drag mouse events. - sinon.stub( TableSelection.prototype, '_enableMouseDragSelection' ); - editor = await createEditor(); model = editor.model; modelRoot = model.document.getRoot(); @@ -98,10 +95,6 @@ describe( 'table selection', () => { ] ) ); } ); - afterEach( () => { - TableSelection.prototype._enableMouseDragSelection.restore(); - } ); - it( 'should do nothing if the plugin is disabled', () => { tableSelection.isEnabled = false; @@ -116,7 +109,11 @@ describe( 'table selection', () => { it( 'should abort if Shift key was not pressed', () => { viewDocument.fire( 'mousedown', new DomEventData( view, { - shiftKey: false + shiftKey: false, + target: view.domConverter.mapViewToDom( + // figure > table > tbody > tr > td + viewDocument.getRoot().getChild( 0 ).getChild( 1 ).getChild( 0 ).getChild( 0 ).getChild( 2 ) + ) } ) ); assertSelectedCells( model, [ @@ -126,7 +123,7 @@ describe( 'table selection', () => { ] ); } ); - it( 'should abort if started selecting elements outside the table', () => { + it( 'should abort if Shift+clicked an element outside a table', () => { const preventDefault = sinon.spy(); model.change( writer => { @@ -211,7 +208,7 @@ describe( 'table selection', () => { expect( preventDefault.called ).to.equal( true ); } ); - it( 'should ignore `selectionChange` event when selecting cells ', () => { + it( 'should ignore `selectionChange` event when selecting cells', () => { const consoleLog = sinon.stub( console, 'log' ); const preventDefault = sinon.spy(); const selectionChangeCallback = sinon.spy(); @@ -219,10 +216,7 @@ describe( 'table selection', () => { // Adding a new callback to check whether it will be executed (whether `evt.stop()` is being called). viewDocument.on( 'selectionChange', selectionChangeCallback ); - // No changes were made. - expect( selectionChangeCallback.called ).to.equal( false ); - - // Start selecting a cell. Disables listening to `selectionChange`. + // Shift+click a cell to create a selection. Should disable listening to `selectionChange`. viewDocument.fire( 'mousedown', new DomEventData( view, { shiftKey: true, target: view.domConverter.mapViewToDom( @@ -232,13 +226,16 @@ describe( 'table selection', () => { preventDefault } ) ); - // The callback shouldn't be executed. + // Due to browsers "fixing" the selection (e.g. moving it to text nodes), after we set a selection + // the browser fill fire native selectionchange, which triggers our selectionChange. We need to ignore it. + // See a broader explanation in tableselection.js. viewDocument.fire( 'selectionChange' ); - expect( selectionChangeCallback.called ).to.equal( false ); - + // The callback shouldn't be executed because // `selectionChange` event should be canceled. expect( selectionChangeCallback.called ).to.equal( false ); + expect( consoleLog.called ).to.equal( true ); + expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); // Enables listening to `selectionChange` event. viewDocument.fire( 'mouseup' ); @@ -248,8 +245,6 @@ describe( 'table selection', () => { } ); expect( selectionChangeCallback.called ).to.equal( true ); - expect( consoleLog.called ).to.equal( true ); - expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); consoleLog.restore(); } ); @@ -259,9 +254,6 @@ describe( 'table selection', () => { let preventDefault; beforeEach( async () => { - // Disables attaching mouse+Shift events. - sinon.stub( TableSelection.prototype, '_enableShiftClickSelection' ); - editor = await createEditor(); model = editor.model; modelRoot = model.document.getRoot(); @@ -278,10 +270,6 @@ describe( 'table selection', () => { preventDefault = sinon.spy(); } ); - afterEach( () => { - TableSelection.prototype._enableShiftClickSelection.restore(); - } ); - it( 'should do nothing if the plugin is disabled', () => { tableSelection.isEnabled = false; @@ -296,20 +284,6 @@ describe( 'table selection', () => { ] ); } ); - it( 'should abort if Shift is pressed', () => { - const domEventDataMock = new DomEventData( view, { - shiftKey: true - } ); - - viewDocument.fire( 'mousedown', domEventDataMock ); - - assertSelectedCells( model, [ - [ 0, 0, 0 ], - [ 0, 0, 0 ], - [ 0, 0, 0 ] - ] ); - } ); - it( 'should abort if Ctrl is pressed', () => { const domEventDataMock = new DomEventData( view, { ctrlKey: true @@ -529,9 +503,6 @@ describe( 'table selection', () => { // Adding a new callback to check whether it will be executed (whether `evt.stop()` is being called). viewDocument.on( 'selectionChange', selectionChangeCallback ); - // No changes were made. - expect( selectionChangeCallback.called ).to.equal( false ); - // Click on a cell. viewDocument.fire( 'mousedown', new DomEventData( view, { target: view.domConverter.mapViewToDom( @@ -550,11 +521,13 @@ describe( 'table selection', () => { preventDefault } ) ); - // The callback shouldn't be executed. + // See explanation why do we fire it in the similar test for Shift+click. viewDocument.fire( 'selectionChange' ); // `selectionChange` event should be canceled. expect( selectionChangeCallback.called ).to.equal( false ); + expect( consoleLog.called ).to.equal( true ); + expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); // Enables listening to `selectionChange` event. viewDocument.fire( 'mouseup' ); @@ -564,8 +537,6 @@ describe( 'table selection', () => { } ); expect( selectionChangeCallback.called ).to.equal( true ); - expect( consoleLog.called ).to.equal( true ); - expect( consoleLog.firstCall.args[ 0 ] ).to.equal( 'Blocked selectionChange to avoid breaking table cells selection.' ); consoleLog.restore(); } ); From edefe8a93a1d3ca2b2802b3dbdd88622e0ad1597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 25 Mar 2020 09:55:24 +0100 Subject: [PATCH 9/9] Complete code coverage. --- src/tableselection.js | 18 +++--- tests/tableselection-integration.js | 50 ----------------- tests/tableselection.js | 86 +++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 61 deletions(-) diff --git a/src/tableselection.js b/src/tableselection.js index 63c22ad0..3f951fd0 100644 --- a/src/tableselection.js +++ b/src/tableselection.js @@ -352,12 +352,13 @@ export default class TableSelection extends Plugin { const rangeToSelect = model.schema.getNearestSelectionRange( writer.createPositionAt( tableCellToSelect, 0 ) ); - if ( rangeToSelect ) { - if ( selection.is( 'documentSelection' ) ) { - writer.setSelection( rangeToSelect ); - } else { - selection.setTo( rangeToSelect ); - } + // Note: we ignore the case where rangeToSelect may be null because deleteContent() will always (unless someone broke it) + // create an empty paragraph to accommodate the selection. + + if ( selection.is( 'documentSelection' ) ) { + writer.setSelection( rangeToSelect ); + } else { + selection.setTo( rangeToSelect ); } } ); } @@ -395,11 +396,6 @@ export default class TableSelection extends Plugin { const modelPosition = this.editor.editing.mapper.toModelPosition( viewPosition ); const modelElement = modelPosition.parent; - // TODO: Required? - if ( !modelElement ) { - return; - } - if ( modelElement.is( 'tableCell' ) ) { return modelElement; } diff --git a/tests/tableselection-integration.js b/tests/tableselection-integration.js index 85606b8e..99c408cc 100644 --- a/tests/tableselection-integration.js +++ b/tests/tableselection-integration.js @@ -104,56 +104,6 @@ describe( 'TableSelection - integration', () => { [ '31', '32', '33' ] ] ) ); } ); - - it( 'should work with any arbitrary selection passed to Model#deleteContent() (delete backwards)', () => { - const selection = model.createSelection( [ - model.createRange( - model.createPositionFromPath( modelRoot, [ 0, 0, 0 ] ), - model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ) - ), - model.createRange( - model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ), - model.createPositionFromPath( modelRoot, [ 0, 0, 2 ] ) - ) - ] ); - - model.change( writer => { - model.deleteContent( selection ); - writer.setSelection( selection ); - } ); - - assertEqualMarkup( getModelData( model ), modelTable( [ - [ '', '[]', '13' ], - [ '21', '22', '23' ], - [ '31', '32', '33' ] - ] ) ); - } ); - - it( 'should work with any arbitrary selection passed to Model#deleteContent() (delete forwards)', () => { - const selection = model.createSelection( [ - model.createRange( - model.createPositionFromPath( modelRoot, [ 0, 0, 0 ] ), - model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ) - ), - model.createRange( - model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ), - model.createPositionFromPath( modelRoot, [ 0, 0, 2 ] ) - ) - ] ); - - model.change( writer => { - model.deleteContent( selection, { - direction: 'forward' - } ); - writer.setSelection( selection ); - } ); - - assertEqualMarkup( getModelData( model ), modelTable( [ - [ '[]', '', '13' ], - [ '21', '22', '23' ], - [ '31', '32', '33' ] - ] ) ); - } ); } ); describe( 'on user input', () => { diff --git a/tests/tableselection.js b/tests/tableselection.js index b43793b5..5301925b 100644 --- a/tests/tableselection.js +++ b/tests/tableselection.js @@ -67,6 +67,25 @@ describe( 'table selection', () => { expect( ranges[ 0 ].start.path ).to.deep.equal( [ 0, 0, 0, 0, 0 ] ); } ); + it( 'should reenable table selection when reenabling the plugin', () => { + tableSelection.forceDisabled( 'foo' ); + tableSelection.clearForceDisabled( 'foo' ); + + const firstCell = modelRoot.getNodeByPath( [ 0, 0, 0 ] ); + const lastCell = modelRoot.getNodeByPath( [ 0, 1, 1 ] ); + + tableSelection._setCellSelection( + firstCell, + lastCell + ); + + assertSelectedCells( model, [ + [ 1, 1, 0 ], + [ 1, 1, 0 ], + [ 0, 0, 0 ] + ] ); + } ); + it( 'should do nothing if there were no multi-cell selections', () => { tableSelection.forceDisabled( 'foo' ); @@ -743,6 +762,73 @@ describe( 'table selection', () => { [ '31', '32', '33' ] ] ) ); } ); + + it( 'should work with document selection passed to Model#deleteContent()', () => { + tableSelection._setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + model.change( () => { + model.deleteContent( model.document.selection ); + } ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '', '', '13' ], + [ '', '[]', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + it( 'should work with any arbitrary selection passed to Model#deleteContent() (delete backwards)', () => { + const selection = model.createSelection( [ + model.createRange( + model.createPositionFromPath( modelRoot, [ 0, 0, 0 ] ), + model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ) + ), + model.createRange( + model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ), + model.createPositionFromPath( modelRoot, [ 0, 0, 2 ] ) + ) + ] ); + + model.change( writer => { + model.deleteContent( selection ); + writer.setSelection( selection ); + } ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '', '[]', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); + + it( 'should work with any arbitrary selection passed to Model#deleteContent() (delete forwards)', () => { + const selection = model.createSelection( [ + model.createRange( + model.createPositionFromPath( modelRoot, [ 0, 0, 0 ] ), + model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ) + ), + model.createRange( + model.createPositionFromPath( modelRoot, [ 0, 0, 1 ] ), + model.createPositionFromPath( modelRoot, [ 0, 0, 2 ] ) + ) + ] ); + + model.change( writer => { + model.deleteContent( selection, { + direction: 'forward' + } ); + writer.setSelection( selection ); + } ); + + assertEqualMarkup( getModelData( model ), modelTable( [ + [ '[]', '', '13' ], + [ '21', '22', '23' ], + [ '31', '32', '33' ] + ] ) ); + } ); } ); function createEditor() {