Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I/6115: Table cells selection with keyboard. #6728

Merged
merged 6 commits into from
May 7, 2020
Merged

I/6115: Table cells selection with keyboard. #6728

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
deb7de1
Table cells selection with keyboard.
niegowski May 4, 2020
9b46900
Tests (cloud-services-core): Added missing test. Closes #6715.
Reinmar May 4, 2020
1e6af30
Merge branch 'i/6641' into i/6115
niegowski May 4, 2020
5fe8406
Update packages/ckeditor5-table/tests/tableselection.js
niegowski May 5, 2020
a4c69f4
Applied review comments.
niegowski May 6, 2020
7aa2f44
Apply suggestions from code review
niegowski May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 44 additions & 19 deletions packages/ckeditor5-table/src/tablenavigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
* @module table/tablenavigation
*/

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import { getSelectedTableCells, getTableCellsContainingSelection } from './utils';
import { findAncestor } from './commands/utils';
import TableSelection from './tableselection';
import TableWalker from './tablewalker';
import { findAncestor } from './commands/utils';
import { getSelectedTableCells, getTableCellsContainingSelection } from './utils';

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';

/**
* This plugin enables keyboard navigation for tables.
Expand All @@ -29,6 +31,13 @@ export default class TableNavigation extends Plugin {
return 'TableNavigation';
}

/**
* @inheritDoc
*/
static get requires() {
return [ TableSelection ];
}

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -188,9 +197,16 @@ export default class TableNavigation extends Plugin {
const selectedCells = getSelectedTableCells( selection );

if ( selectedCells.length ) {
const tableCell = isForward ? selectedCells[ selectedCells.length - 1 ] : selectedCells[ 0 ];
if ( expandSelection ) {
const tableSelection = this.editor.plugins.get( 'TableSelection' );
const focusCell = tableSelection.getFocusCell();

this._navigateFromCellInDirection( focusCell, direction, expandSelection );
jodator marked this conversation as resolved.
Show resolved Hide resolved
} else {
const tableCell = isForward ? selectedCells[ selectedCells.length - 1 ] : selectedCells[ 0 ];

this._navigateFromCellInDirection( tableCell, direction );
this._navigateFromCellInDirection( tableCell, direction, expandSelection );
}

return true;
}
Expand All @@ -206,7 +222,7 @@ export default class TableNavigation extends Plugin {

// Let's check if the selection is at the beginning/end of the cell.
if ( this._isSelectionAtCellEdge( selection, isForward ) ) {
this._navigateFromCellInDirection( tableCell, direction );
this._navigateFromCellInDirection( tableCell, direction, expandSelection );

return true;
}
Expand All @@ -228,7 +244,7 @@ export default class TableNavigation extends Plugin {
const textRange = this._findTextRangeFromSelection( cellRange, selection, isForward );

if ( !textRange ) {
this._navigateFromCellInDirection( tableCell, direction );
this._navigateFromCellInDirection( tableCell, direction, expandSelection );

return true;
}
Expand Down Expand Up @@ -426,18 +442,19 @@ export default class TableNavigation extends Plugin {
/**
* Moves the selection from the given table cell in the specified direction.
*
* @private
* @param {module:engine/model/element~Element} tableCell The table cell to start the selection navigation.
* @protected
niegowski marked this conversation as resolved.
Show resolved Hide resolved
* @param {module:engine/model/element~Element} focusCell The table cell that is current multi-cell selection focus.
* @param {'left'|'up'|'right'|'down'} direction Direction in which selection should move.
* @param {Boolean} expandSelection If the current selection should be expanded.
*/
_navigateFromCellInDirection( tableCell, direction ) {
_navigateFromCellInDirection( focusCell, direction, expandSelection ) {
jodator marked this conversation as resolved.
Show resolved Hide resolved
jodator marked this conversation as resolved.
Show resolved Hide resolved
const model = this.editor.model;

const table = findAncestor( 'table', tableCell );
const table = findAncestor( 'table', focusCell );
const tableMap = [ ...new TableWalker( table, { includeSpanned: true } ) ];
const { row: lastRow, column: lastColumn } = tableMap[ tableMap.length - 1 ];

const currentCellInfo = tableMap.find( ( { cell } ) => cell == tableCell );
const currentCellInfo = tableMap.find( ( { cell } ) => cell == focusCell );
let { row, column } = currentCellInfo;

switch ( direction ) {
Expand Down Expand Up @@ -474,20 +491,28 @@ export default class TableNavigation extends Plugin {
}

if ( column < 0 ) {
column = lastColumn;
column = expandSelection ? 0 : lastColumn;
row--;
} else if ( column > lastColumn ) {
column = 0;
column = expandSelection ? lastColumn : 0;
row++;
}

const cellToSelect = tableMap.find( cellInfo => cellInfo.row == row && cellInfo.column == column ).cell;
const isForward = [ 'right', 'down' ].includes( direction );
const positionToSelect = model.createPositionAt( cellToSelect, isForward ? 0 : 'end' );

model.change( writer => {
writer.setSelection( positionToSelect );
} );
if ( expandSelection ) {
const tableSelection = this.editor.plugins.get( 'TableSelection' );
const anchorCell = tableSelection.getAnchorCell() || focusCell;

tableSelection.setCellSelection( anchorCell, cellToSelect );
} else {
const positionToSelect = model.createPositionAt( cellToSelect, isForward ? 0 : 'end' );

model.change( writer => {
writer.setSelection( positionToSelect );
} );
}
}
}

Expand Down
79 changes: 52 additions & 27 deletions packages/ckeditor5-table/src/tableselection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import first from '@ckeditor/ckeditor5-utils/src/first';

import TableWalker from './tablewalker';
import TableUtils from './tableutils';
Expand Down Expand Up @@ -181,7 +182,7 @@ export default class TableSelection extends Plugin {

if ( targetCell && haveSameTableParent( anchorCell, targetCell ) ) {
blockSelectionChange = true;
this._setCellSelection( anchorCell, targetCell );
this.setCellSelection( anchorCell, targetCell );

domEventData.preventDefault();
}
Expand Down Expand Up @@ -272,7 +273,7 @@ export default class TableSelection extends Plugin {
}

blockSelectionChange = true;
this._setCellSelection( anchorCell, targetCell );
this.setCellSelection( anchorCell, targetCell );

domEventData.preventDefault();
} );
Expand Down Expand Up @@ -367,11 +368,10 @@ 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 the backward flag.
*
* @protected
* @param {module:engine/model/element~Element} anchorCell
* @param {module:engine/model/element~Element} targetCell
*/
_setCellSelection( anchorCell, targetCell ) {
setCellSelection( anchorCell, targetCell ) {
jodator marked this conversation as resolved.
Show resolved Hide resolved
niegowski marked this conversation as resolved.
Show resolved Hide resolved
const cellsToSelect = this._getCellsToSelect( anchorCell, targetCell );

this.editor.model.change( writer => {
Expand All @@ -382,6 +382,40 @@ export default class TableSelection extends Plugin {
} );
}

/**
* Returns the focus cell from the current selection.
*
* @returns {module:engine/model/element~Element}
*/
getFocusCell() {
niegowski marked this conversation as resolved.
Show resolved Hide resolved
niegowski marked this conversation as resolved.
Show resolved Hide resolved
const selection = this.editor.model.document.selection;
const focusCellRange = [ ...selection.getRanges() ].pop();
const element = focusCellRange.getContainedElement();

if ( element && element.is( 'tableCell' ) ) {
return element;
}

return null;
}

/**
* Returns the anchor cell from the current selection.
*
* @returns {module:engine/model/element~Element} anchorCell
*/
getAnchorCell() {
const selection = this.editor.model.document.selection;
const anchorCellRange = first( selection.getRanges() );
const element = anchorCellRange.getContainedElement();

if ( element && element.is( 'tableCell' ) ) {
return element;
}

return null;
}

/**
* Returns the model table cell element based on the target element of the passed DOM event.
*
Expand Down Expand Up @@ -425,39 +459,30 @@ export default class TableSelection extends Plugin {
const startColumn = Math.min( startLocation.column, endLocation.column );
const endColumn = Math.max( startLocation.column, endLocation.column );

const cells = [];
const selectionMap = new Array( endRow - startRow + 1 ).fill( null ).map( () => [] );
niegowski marked this conversation as resolved.
Show resolved Hide resolved
jodator marked this conversation as resolved.
Show resolved Hide resolved

for ( const cellInfo of new TableWalker( findAncestor( 'table', anchorCell ), { startRow, endRow } ) ) {
if ( cellInfo.column >= startColumn && cellInfo.column <= endColumn ) {
cells.push( cellInfo.cell );
selectionMap[ cellInfo.row - startRow ].push( cellInfo.cell );
}
}

// A selection started in the bottom right corner and finished in the upper left corner
// should have it ranges returned in the reverse order.
// However, this is only half of the story because the selection could be made to the left (then the left cell is a focus)
// or to the right (then the right cell is a focus), while being a forward selection in both cases
// (because it was made from top to bottom). This isn't handled.
// This method would need to be smarter, but the ROI is microscopic, so I skip this.
if ( checkIsBackward( startLocation, endLocation ) ) {
return { cells: cells.reverse(), backward: true };
}
const flipVertically = endLocation.row < startLocation.row;
const flipHorizontally = endLocation.column < startLocation.column;

return { cells, backward: false };
}
}
if ( flipVertically ) {
selectionMap.reverse();
}

// Naively check whether the selection should be backward or not. See the longer explanation where this function is used.
function checkIsBackward( startLocation, endLocation ) {
if ( startLocation.row > endLocation.row ) {
return true;
}
if ( flipHorizontally ) {
selectionMap.forEach( row => row.reverse() );
}

if ( startLocation.row == endLocation.row && startLocation.column > endLocation.column ) {
return true;
return {
cells: selectionMap.flat(),
backward: flipVertically || flipHorizontally
};
}

return false;
}

function haveSameTableParent( cellA, cellB ) {
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-table/tests/commands/insertcolumncommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe( 'InsertColumnCommand', () => {
const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();

tableSelection._setCellSelection(
tableSelection.setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 1, 1 ] )
);
Expand Down Expand Up @@ -251,7 +251,7 @@ describe( 'InsertColumnCommand', () => {
const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();

tableSelection._setCellSelection(
tableSelection.setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 1, 1 ] )
);
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-table/tests/commands/insertrowcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe( 'InsertRowCommand', () => {
const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();

tableSelection._setCellSelection(
tableSelection.setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 1, 1 ] )
);
Expand Down Expand Up @@ -328,7 +328,7 @@ describe( 'InsertRowCommand', () => {
const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();

tableSelection._setCellSelection(
tableSelection.setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 1, 1 ] )
);
Expand Down
22 changes: 11 additions & 11 deletions packages/ckeditor5-table/tests/commands/mergecellscommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11' ]
], { headingRows: 1 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 1, 0 ] )
);
Expand Down Expand Up @@ -230,7 +230,7 @@ describe( 'MergeCellsCommand', () => {

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
tableSelection.setCellSelection(
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
);
Expand All @@ -244,7 +244,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11', '12', '13' ]
], { headingColumns: 2 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 1, 1 ] )
);
Expand All @@ -258,7 +258,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11', '12', '13' ]
], { headingColumns: 2 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 1, 2 ] )
);
Expand All @@ -272,7 +272,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11', '12', '13' ]
], { headingColumns: 2, headingRows: 1 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 0, 1 ] )
);
Expand All @@ -286,7 +286,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11', '12', '13' ]
], { headingColumns: 2, headingRows: 1 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 0, 2 ] )
);
Expand All @@ -300,7 +300,7 @@ describe( 'MergeCellsCommand', () => {
[ '10', '11', '12', '13' ]
], { headingColumns: 2, headingRows: 1 } ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 1, 2 ] )
);
Expand All @@ -315,7 +315,7 @@ describe( 'MergeCellsCommand', () => {
[ '[]00', '01' ]
] ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 0, 0 ] ),
root.getNodeByPath( [ 0, 0, 1 ] )
);
Expand All @@ -334,7 +334,7 @@ describe( 'MergeCellsCommand', () => {
[ '20', '22' ]
] ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 1, 0 ] ),
root.getNodeByPath( [ 0, 2, 1 ] )
);
Expand All @@ -358,7 +358,7 @@ describe( 'MergeCellsCommand', () => {
[ '20', '22' ]
] ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 2, 1 ] ),
root.getNodeByPath( [ 0, 1, 0 ] )
);
Expand All @@ -383,7 +383,7 @@ describe( 'MergeCellsCommand', () => {
[ '30', '31', '32', '33' ]
] ) );

tableSelection._setCellSelection(
tableSelection.setCellSelection(
root.getNodeByPath( [ 0, 2, 1 ] ),
root.getNodeByPath( [ 0, 1, 2 ] )
);
Expand Down
Loading