Merge pull request #257 from ckeditor/i/6114

Internal: Improved the look of multi-cell selection. Prevented the selection from starting in the fake selection container. Closes ckeditor/ckeditor5#6114. Closes ckeditor/ckeditor5#6353.

src/tablecellproperties/commands/tablecellpropertycommand.js

@@ -114,6 +114,8 @@ export default class TableCellPropertyCommand extends Command {
 }
 
 // Returns all selected table cells.
+// The implementation of this function is incorrect as it may return a single cell twice.
+// See https://github.com/ckeditor/ckeditor5/issues/6358.
 function getSelectedTableCells( model ) {
 	const selection = model.document.selection;
 

src/tableselection.js

@@ -229,6 +229,7 @@ export default class TableSelection extends Plugin {
 	 * @returns {Iterable.<module:engine/model/element~Element>}
 	 */
 	* getSelectedTableCells() {
+		// TODO: this function should be removed. See https://github.com/ckeditor/ckeditor5/issues/6358
 		if ( !this.hasMultiCellSelection ) {
 			return;
 		}

src/tableselection/mouseselectionhandler.js

@@ -40,10 +40,11 @@ export default class MouseSelectionHandler {
 		 * A flag indicating that the mouse selection is "active". A selection is "active" if it was started and not yet finished.
 		 * A selection can be "active", for instance, if a user moves a mouse over a table while holding a mouse button down.
 		 *
+		 * @private
 		 * @readonly
 		 * @member {Boolean}
 		 */
-		this.isSelecting = false;
+		this._isSelecting = false;
 
 		/**
 		 * Editing mapper.
@@ -77,12 +78,11 @@ export default class MouseSelectionHandler {
 
 		if ( !tableCell ) {
 			this._tableSelection.clearSelection();
-			this._tableSelection.stopSelection();
 
 			return;
 		}
 
-		this.isSelecting = true;
+		this._isSelecting = true;
 		this._tableSelection.startSelectingFrom( tableCell );
 	}
 
@@ -95,12 +95,15 @@ export default class MouseSelectionHandler {
 	 * @private
 	 */
 	_handleMouseMove( domEventData ) {
-		if ( !isButtonPressed( domEventData ) ) {
+		if ( !this._isSelecting || !isButtonPressed( domEventData ) ) {
 			this._tableSelection.stopSelection();
 
 			return;
 		}
 
+		// https://github.com/ckeditor/ckeditor5/issues/6114
+		domEventData.preventDefault();
+
 		const tableCell = this._getModelTableCellFromDomEvent( domEventData );
 
 		if ( !tableCell ) {
@@ -119,10 +122,12 @@ export default class MouseSelectionHandler {
 	 * @private
 	 */
 	_handleMouseUp( domEventData ) {
-		if ( !this.isSelecting ) {
+		if ( !this._isSelecting ) {
 			return;
 		}
 
+		this._isSelecting = false;
+
 		const tableCell = this._getModelTableCellFromDomEvent( domEventData );
 
 		// Selection can be stopped if table cell is undefined.

src/ui/utils.js

@@ -81,6 +81,7 @@ export function getBalloonTablePositionData( editor ) {
  * @returns {module:utils/dom/position~Options}
  */
 export function getBalloonCellPositionData( editor ) {
+	// This is a bit naive. See https://github.com/ckeditor/ckeditor5/issues/6357.
 	const modelTableCell = getTableCellAtPosition( editor.model.document.selection.getFirstPosition() );
 	const viewTableCell = editor.editing.mapper.toViewElement( modelTableCell );
 

tests/manual/tableselection.html

@@ -12,6 +12,11 @@
 	.external-source td, .external-source th {
 		border: solid 1px hsl(0, 0%, 85%);
 	}
+
+	/* It should not create entire viewport scroll. */
+	pre {
+		overflow: auto;
+	}
 </style>
 
 <h3>A "content" test editor</h3>

tests/tableselection/mouseselectionhandler.js

@@ -12,10 +12,13 @@ import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'
 import TableEditing from '../../src/tableediting';
 import TableSelection from '../../src/tableselection';
 import { assertSelectedCells, modelTable, viewTable } from '../_utils/utils';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 
 describe( 'table selection', () => {
 	let editor, model, view, viewDoc;
 
+	testUtils.createSinonSandbox();
+
 	beforeEach( async () => {
 		editor = await VirtualTestEditor.create( {
 			plugins: [ TableEditing, TableSelection, Paragraph ]
@@ -418,6 +421,41 @@ describe( 'table selection', () => {
 				[ '10', '11' ]
 			], { asWidget: true } ) + '<p>{}foo</p>' );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/6353
+		it( 'should do nothing on successive "mouseup" events beyond the table after the selection was fininished', () => {
+			const spy = testUtils.sinon.spy( editor.plugins.get( 'TableSelection' ), 'stopSelection' );
+
+			setModelData( model, modelTable( [
+				[ '[]00', '01' ],
+				[ '10', '11' ]
+			] ) );
+
+			pressMouseButtonOver( getTableCell( '00' ) );
+			movePressedMouseOver( getTableCell( '11' ) );
+			releaseMouseButtonOver( getTableCell( '11' ) );
+
+			sinon.assert.calledOnce( spy );
+
+			pressMouseButtonOver( viewDoc.getRoot() );
+			releaseMouseButtonOver( viewDoc.getRoot() );
+
+			sinon.assert.calledOnce( spy );
+		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/6114
+		it( 'should prevent default the "mousemove" event to prevent the native DOM selection from changing', () => {
+			setModelData( model, modelTable( [
+				[ '[]00', '01' ],
+				[ '10', '11' ]
+			] ) );
+
+			pressMouseButtonOver( getTableCell( '00' ) );
+			const domEvtData = movePressedMouseOver( getTableCell( '11' ) );
+			releaseMouseButtonOver( getTableCell( '11' ) );
+
+			sinon.assert.calledOnce( domEvtData.preventDefault );
+		} );
 	} );
 
 	function getTableCell( data ) {
@@ -437,15 +475,15 @@ describe( 'table selection', () => {
 	}
 
 	function movePressedMouseOver( target ) {
-		moveMouseOver( target, mouseButtonPressed );
+		return moveMouseOver( target, mouseButtonPressed );
 	}
 
 	function moveReleasedMouseOver( target ) {
 		moveMouseOver( target, mouseButtonReleased );
 	}
 
 	function moveMouseOver( target, ...decorators ) {
-		fireEvent( view, 'mousemove', addTarget( target ), ...decorators );
+		return fireEvent( view, 'mousemove', addTarget( target ), ...decorators );
 	}
 
 	function releaseMouseButtonOver( target ) {
@@ -481,5 +519,7 @@ describe( 'table selection', () => {
 		}
 
 		viewDoc.fire( eventName, domEvtDataStub );
+
+		return domEvtDataStub;
 	}
 } );

theme/table.css

@@ -26,7 +26,7 @@
 		& th {
 			min-width: 2em;
 			padding: .4em;
-			border-color: hsl(0, 0%, 85%);
+			border-color: hsl(0, 0%, 75%);
 		}
 
 		& th {

theme/tableselection.css

@@ -8,10 +8,3 @@
  * it acts as a message to the builder telling that it should look for the corresponding styles
  * **in the theme** when compiling the editor.
  */
-
-.ck.ck-editor__editable .table table {
-	& td.ck-editor__editable_selected,
-	& th.ck-editor__editable_selected {
-		box-shadow: inset 0 0 0 1px var(--ck-color-focus-border);
-	}
-}