#6693: Keyboard support for the widget type around feature

packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js

@@ -82,7 +82,14 @@ function selectionPostFixer( writer, model ) {
 		// Those ranges might overlap but will be corrected later.
 		const correctedRange = tryFixingRange( modelRange, schema );
 
-		if ( correctedRange ) {
+		// "Selection fixing" algorithms sometimes get lost. In consequence, it may happen
+		// that a new range is returned but, in fact, it has the same positions as the original
+		// range anyway. If this range is not discarded, a new selection will be set and that,
+		// for instance, would destroy the selection attributes. Let's make sure that the post-fixer
+		// actually worked first before setting a new selection.
+		//
+		// https://github.com/ckeditor/ckeditor5/issues/6693
+		if ( correctedRange && !correctedRange.isEqual( modelRange ) ) {
 			ranges.push( correctedRange );
 			wasFixed = true;
 		} else {

packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js

@@ -835,6 +835,35 @@ describe( 'Selection post-fixer', () => {
 					'</table>]'
 				);
 			} );
+
+			it( 'should not reset the selection if the final range is the same as the initial one', () => {
+				setModelData( model,
+					'<table>' +
+						'<tableRow>' +
+							'<tableCell>[<image></image>]</tableCell>' +
+						'</tableRow>' +
+					'</table>'
+				);
+
+				// Setting a selection attribute will trigger the post-fixer. However, because this
+				// action does not affect ranges, the post-fixer should not set a new selection and,
+				// in consequence, should not clear the selection attribute (like it normally would when
+				// a new selection is set).
+				// https://github.com/ckeditor/ckeditor5/issues/6693
+				model.change( writer => {
+					writer.setSelectionAttribute( 'foo', 'bar' );
+				} );
+
+				assertEqualMarkup( getModelData( model ),
+					'<table>' +
+						'<tableRow>' +
+							'<tableCell>[<image></image>]</tableCell>' +
+						'</tableRow>' +
+					'</table>'
+				);
+
+				expect( model.document.selection.hasAttribute( 'foo' ) ).to.be.true;
+			} );
 		} );
 
 		describe( 'non-collapsed selection - image scenarios', () => {

packages/ckeditor5-image/tests/image.js

@@ -64,7 +64,6 @@ describe( 'Image', () => {
 			expect( getViewData( view ) ).to.equal(
 				'[<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'ck-widget_selected image" contenteditable="false"' +
 				'>' +
 					'<img alt="alt text" src="/assets/sample.png"></img>' +
@@ -82,7 +81,6 @@ describe( 'Image', () => {
 			expect( getViewData( view ) ).to.equal(
 				'[<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'ck-widget_selected image" contenteditable="false"' +
 				'>' +
 					'<img alt="" src="/assets/sample.png"></img>' +
@@ -103,15 +101,13 @@ describe( 'Image', () => {
 			expect( getViewData( view ) ).to.equal(
 				'[<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'ck-widget_selected image" contenteditable="false"' +
 				'>' +
 					'<img alt="alt text" src="/assets/sample.png"></img>' +
 					'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
 				'</figure>]' +
 				'<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'image" contenteditable="false"' +
 				'>' +
 					'<img alt="alt text" src="/assets/sample.png"></img>' +
@@ -127,15 +123,13 @@ describe( 'Image', () => {
 			expect( getViewData( view ) ).to.equal(
 				'<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'image" contenteditable="false"' +
 				'>' +
 					'<img alt="alt text" src="/assets/sample.png"></img>' +
 					'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
 				'</figure>' +
 				'[<figure class="' +
 					'ck-widget ' +
-					'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
 					'ck-widget_selected image" contenteditable="false"' +
 				'>' +
 					'<img alt="alt text" src="/assets/sample.png"></img>' +

packages/ckeditor5-paragraph/src/insertparagraphcommand.js

@@ -18,6 +18,10 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
  *			position: editor.model.createPositionBefore( element )
  *		} );
  *
+ * If a paragraph is disallowed in the context of the specific position, the command
+ * will attempt to split position ancestors to find a place where it is possible
+ * to insert a paragraph.
+ *
  * **Note**: This command moves the selection to the inserted paragraph.
  *
  * @extends module:core/command~Command
@@ -33,15 +37,24 @@ export default class InsertParagraphCommand extends Command {
 	 */
 	execute( options ) {
 		const model = this.editor.model;
-
-		if ( !model.schema.checkChild( options.position, 'paragraph' ) ) {
-			return;
-		}
+		let position = options.position;
 
 		model.change( writer => {
 			const paragraph = writer.createElement( 'paragraph' );
 
-			model.insertContent( paragraph, options.position );
+			if ( !model.schema.checkChild( position.parent, paragraph ) ) {
+				const allowedParent = model.schema.findAllowedParent( position, paragraph );
+
+				// It could be there's no ancestor limit that would allow paragraph.
+				// In theory, "paragraph" could be disallowed even in the "$root".
+				if ( !allowedParent ) {
+					return;
+				}
+
+				position = writer.split( position, allowedParent ).position;
+			}
+
+			model.insertContent( paragraph, position );
 
 			writer.setSelection( paragraph, 'in' );
 		} );

packages/ckeditor5-paragraph/tests/insertparagraphcommand.js

@@ -23,7 +23,9 @@ describe( 'InsertParagraphCommand', () => {
 			editor.commands.add( 'insertParagraph', command );
 			schema.register( 'paragraph', { inheritAllFrom: '$block' } );
 			schema.register( 'heading1', { inheritAllFrom: '$block', allowIn: 'headersOnly' } );
-			schema.register( 'headersOnly', { inheritAllFrom: '$block' } );
+			schema.register( 'allowP', { inheritAllFrom: '$block' } );
+			schema.register( 'disallowP', { inheritAllFrom: '$block', allowIn: [ 'allowP' ] } );
+			model.schema.extend( 'paragraph', { allowIn: [ 'allowP' ] } );
 		} );
 	} );
 
@@ -42,18 +44,38 @@ describe( 'InsertParagraphCommand', () => {
 			expect( getData( model ) ).to.equal( '<paragraph>[]</paragraph><heading1>foo</heading1>' );
 		} );
 
-		it( 'should do nothing if the paragraph is not allowed at the provided position', () => {
-			setData( model, '<headersOnly><heading1>foo[]</heading1></headersOnly>' );
+		it( 'should split ancestors down to a limit where a paragraph is allowed', () => {
+			setData( model, '<allowP><disallowP>foo</disallowP></allowP>' );
 
 			command.execute( {
-				position: model.createPositionBefore( root.getChild( 0 ).getChild( 0 ) )
+				// fo[]o
+				position: model.createPositionAt( root.getChild( 0 ).getChild( 0 ), 2 )
 			} );
 
+			expect( getData( model ) ).to.equal(
+				'<allowP>' +
+					'<disallowP>fo</disallowP>' +
+					'<paragraph>[]</paragraph>' +
+					'<disallowP>o</disallowP>' +
+				'</allowP>'
+			);
+		} );
+
+		it( 'should do nothing if the paragraph is not allowed at the provided position', () => {
+			// Create a situation where "paragraph" is disallowed even in the "root".
+			schema.addChildCheck( ( context, childDefinition ) => {
+				if ( context.endsWith( '$root' ) && childDefinition.name == 'paragraph' ) {
+					return false;
+				}
+			} );
+
+			setData( model, '<heading1>foo[]</heading1>' );
+
 			command.execute( {
-				position: model.createPositionAfter( root.getChild( 0 ).getChild( 0 ) )
+				position: model.createPositionBefore( root.getChild( 0 ) )
 			} );
 
-			expect( getData( model ) ).to.equal( '<headersOnly><heading1>foo[]</heading1></headersOnly>' );
+			expect( getData( model ) ).to.equal( '<heading1>foo[]</heading1>' );
 		} );
 
 		describe( 'interation with existing paragraphs in the content', () => {

packages/ckeditor5-table/src/tablekeyboard.js

@@ -13,7 +13,10 @@ import TableWalker from './tablewalker';
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
 import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
-import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
+import {
+	isArrowKeyCode,
+	getLocalizedArrowKeyCodeDirection
+} from '@ckeditor/ckeditor5-utils/src/keyboard';
 import { getSelectedTableCells, getTableCellsContainingSelection } from './utils/selection';
 import { findAncestor } from './utils/common';
 
@@ -163,13 +166,14 @@ export default class TableKeyboard extends Plugin {
 	 * @param {module:engine/view/observer/domeventdata~DomEventData} domEventData
 	 */
 	_onKeydown( eventInfo, domEventData ) {
+		const editor = this.editor;
 		const keyCode = domEventData.keyCode;
 
 		if ( !isArrowKeyCode( keyCode ) ) {
 			return;
 		}
 
-		const direction = getDirectionFromKeyCode( keyCode, this.editor.locale.contentLanguageDirection );
+		const direction = getLocalizedArrowKeyCodeDirection( keyCode, editor.locale.contentLanguageDirection );
 		const wasHandled = this._handleArrowKeys( direction, domEventData.shiftKey );
 
 		if ( wasHandled ) {
@@ -515,38 +519,3 @@ export default class TableKeyboard extends Plugin {
 	}
 }
 
-// Returns `true` if the provided key code represents one of the arrow keys.
-//
-// @private
-// @param {Number} keyCode
-// @returns {Boolean}
-function isArrowKeyCode( keyCode ) {
-	return keyCode == keyCodes.arrowright ||
-		keyCode == keyCodes.arrowleft ||
-		keyCode == keyCodes.arrowup ||
-		keyCode == keyCodes.arrowdown;
-}
-
-// Returns the direction name from `keyCode`.
-//
-// @private
-// @param {Number} keyCode
-// @param {String} contentLanguageDirection The content language direction.
-// @returns {'left'|'up'|'right'|'down'} Arrow direction.
-function getDirectionFromKeyCode( keyCode, contentLanguageDirection ) {
-	const isLtrContent = contentLanguageDirection === 'ltr';
-
-	switch ( keyCode ) {
-		case keyCodes.arrowleft:
-			return isLtrContent ? 'left' : 'right';
-
-		case keyCodes.arrowright:
-			return isLtrContent ? 'right' : 'left';
-
-		case keyCodes.arrowup:
-			return 'up';
-
-		case keyCodes.arrowdown:
-			return 'down';
-	}
-}

packages/ckeditor5-table/tests/converters/upcasttable.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import { getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import ImageEditing from '@ckeditor/ckeditor5-image/src/image/imageediting';
@@ -17,8 +17,8 @@ describe( 'upcastTable()', () => {
 	let editor, model;
 
 	beforeEach( () => {
-		return VirtualTestEditor
-			.create( {
+		return ClassicTestEditor
+			.create( '', {
 				plugins: [ TableEditing, Paragraph, ImageEditing, Widget ]
 			} )
 			.then( newEditor => {
@@ -31,6 +31,10 @@ describe( 'upcastTable()', () => {
 			} );
 	} );
 
+	afterEach( () => {
+		editor.destroy();
+	} );
+
 	function expectModel( data ) {
 		assertEqualMarkup( getModelData( model, { withoutSelection: true } ), data );
 	}

packages/ckeditor5-table/tests/table-integration.js

@@ -11,7 +11,7 @@ import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
 import ListEditing from '@ckeditor/ckeditor5-list/src/listediting';
 import BlockQuoteEditing from '@ckeditor/ckeditor5-block-quote/src/blockquoteediting';
 import Typing from '@ckeditor/ckeditor5-typing/src/typing';
-import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import {
 	getData as getModelData,
 	setData as setModelData
@@ -27,14 +27,18 @@ describe( 'Table feature – integration', () => {
 		let editor, clipboard;
 
 		beforeEach( () => {
-			return VirtualTestEditor
-				.create( { plugins: [ Paragraph, TableEditing, ListEditing, BlockQuoteEditing, Widget, Clipboard ] } )
+			return ClassicTestEditor
+				.create( '', { plugins: [ Paragraph, TableEditing, ListEditing, BlockQuoteEditing, Widget, Clipboard ] } )
 				.then( newEditor => {
 					editor = newEditor;
 					clipboard = editor.plugins.get( 'Clipboard' );
 				} );
 		} );
 
+		afterEach( () => {
+			editor.destroy();
+		} );
+
 		it( 'pastes td as p when pasting into the table', () => {
 			setModelData( editor.model, modelTable( [ [ 'foo[]' ] ] ) );
 
@@ -86,15 +90,19 @@ describe( 'Table feature – integration', () => {
 		let editor, doc, root;
 
 		beforeEach( () => {
-			return VirtualTestEditor
-				.create( { plugins: [ Paragraph, TableEditing, Widget, UndoEditing ] } )
+			return ClassicTestEditor
+				.create( '', { plugins: [ Paragraph, TableEditing, Widget, UndoEditing ] } )
 				.then( newEditor => {
 					editor = newEditor;
 					doc = editor.model.document;
 					root = doc.getRoot();
 				} );
 		} );
 
+		afterEach( () => {
+			editor.destroy();
+		} );
+
 		it( 'fixing empty roots should be transparent to undo', () => {
 			expect( editor.getData( { trim: 'none' } ) ).to.equal( '<p>&nbsp;</p>' );
 			expect( editor.commands.get( 'undo' ).isEnabled ).to.be.false;
@@ -155,13 +163,17 @@ describe( 'Table feature – integration', () => {
 		let editor;
 
 		beforeEach( () => {
-			return VirtualTestEditor
-				.create( { plugins: [ Paragraph, TableEditing, ListEditing, BlockQuoteEditing, Widget, Typing ] } )
+			return ClassicTestEditor
+				.create( '', { plugins: [ Paragraph, TableEditing, ListEditing, BlockQuoteEditing, Widget, Typing ] } )
 				.then( newEditor => {
 					editor = newEditor;
 				} );
 		} );
 
+		afterEach( () => {
+			editor.destroy();
+		} );
+
 		it( 'merges elements without throwing errors', () => {
 			setModelData( editor.model, modelTable( [
 				[ '<blockQuote><paragraph>Foo</paragraph></blockQuote><paragraph>[]Bar</paragraph>' ]

packages/ckeditor5-table/tests/tablekeyboard.js

@@ -2769,6 +2769,9 @@ describe( 'TableKeyboard', () => {
 							[ '20', '21', '22' ]
 						] ) );
 
+						// Note: Two keydowns are necessary because the first one is handled by the WidgetTypeAround plugin
+						// to activate the "fake caret".
+						editor.editing.view.document.fire( 'keydown', downArrowDomEvtDataStub );
 						editor.editing.view.document.fire( 'keydown', downArrowDomEvtDataStub );
 
 						assertEqualMarkup( getModelData( model ), modelTable( [