Browse Source

Merge pull request #7445 from ckeditor/i/7438-insertion-position

Internal (widget): The `findOptimalInsertionPosition()` helper should respect the "fake widget caret" to provide the best UX. Closes #7438.
Kuba Niegowski 5 years ago
parent
commit
970013189c

+ 13 - 2
packages/ckeditor5-widget/src/utils.js

@@ -14,6 +14,7 @@ import BalloonPanelView from '@ckeditor/ckeditor5-ui/src/panel/balloon/balloonpa
 import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 
 import dragHandleIcon from '../theme/icons/drag-handle.svg';
+import { getTypeAroundFakeCaretPosition } from './widgettypearound/utils';
 
 /**
  * CSS class added to each widget element.
@@ -256,8 +257,18 @@ export function toWidgetEditable( editable, writer ) {
 export function findOptimalInsertionPosition( selection, model ) {
 	const selectedElement = selection.getSelectedElement();
 
-	if ( selectedElement && model.schema.isBlock( selectedElement ) ) {
-		return model.createPositionAfter( selectedElement );
+	if ( selectedElement ) {
+		const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( selection );
+
+		// If the WidgetTypeAround "fake caret" is displayed, use its position for the insertion
+		// to provide the most predictable UX (https://github.com/ckeditor/ckeditor5/issues/7438).
+		if ( typeAroundFakeCaretPosition ) {
+			return model.createPositionAt( selectedElement, typeAroundFakeCaretPosition );
+		}
+
+		if ( model.schema.isBlock( selectedElement ) ) {
+			return model.createPositionAfter( selectedElement );
+		}
 	}
 
 	const firstBlock = selection.getSelectedBlocks().next().value;

+ 19 - 1
packages/ckeditor5-widget/src/widgettypearound/utils.js

@@ -10,6 +10,12 @@
 import { isWidget } from '../utils';
 
 /**
+ * The name of the type around model selection attribute responsible for
+ * displaying a "fake caret" next to a selected widget.
+ */
+export const TYPE_AROUND_SELECTION_ATTRIBUTE = 'widget-type-around';
+
+/**
  * Checks if an element is a widget that qualifies to get the type around UI.
  *
  * @param {module:engine/view/element~Element} viewElement
@@ -37,7 +43,7 @@ export function getClosestTypeAroundDomButton( domElement ) {
  * clicked by the user.
  *
  * @param {HTMLElement} domElement
- * @returns {String} Either `'before'` or `'after'`.
+ * @returns {'before'|'after'} Position of the button.
  */
 export function getTypeAroundButtonPosition( domElement ) {
 	return domElement.classList.contains( 'ck-widget__type-around__button_before' ) ? 'before' : 'after';
@@ -55,3 +61,15 @@ export function getClosestWidgetViewElement( domElement, domConverter ) {
 
 	return domConverter.mapDomToView( widgetDomElement );
 }
+
+/**
+ * For the passed selection instance, it returns the position of the "fake caret" displayed next to a widget.
+ *
+ * **Note**: If the "fake caret" is not currently displayed, `null` is returned.
+ *
+ * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
+ * @returns {'before'|'after'|null} Position of the fake caret or `null` when none is preset.
+ */
+export function getTypeAroundFakeCaretPosition( selection ) {
+	return selection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+}

+ 25 - 25
packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js

@@ -22,7 +22,9 @@ import {
 	isTypeAroundWidget,
 	getClosestTypeAroundDomButton,
 	getTypeAroundButtonPosition,
-	getClosestWidgetViewElement
+	getClosestWidgetViewElement,
+	getTypeAroundFakeCaretPosition,
+	TYPE_AROUND_SELECTION_ATTRIBUTE
 } from './utils';
 
 import {
@@ -37,8 +39,6 @@ const POSSIBLE_INSERTION_POSITIONS = [ 'before', 'after' ];
 // Do the SVG parsing once and then clone the result <svg> DOM element for each new button.
 const RETURN_ARROW_ICON_ELEMENT = new DOMParser().parseFromString( returnIcon, 'image/svg+xml' ).firstChild;
 
-const TYPE_AROUND_SELECTION_ATTRIBUTE = 'widget-type-around';
-
 /**
  * A plugin that allows users to type around widgets where normally it is impossible to place the caret due
  * to limitations of web browsers. These "tight spots" occur, for instance, before (or after) a widget being
@@ -122,7 +122,7 @@ export default class WidgetTypeAround extends Plugin {
 	/**
 	 * Similar to {@link #_insertParagraph}, this method inserts a paragraph except that it
 	 * does not expect a position but it performs the insertion next to a selected widget
-	 * according to the "widget-type-around" model selection attribute value.
+	 * according to the "widget-type-around" model selection attribute value ("fake caret" position).
 	 *
 	 * Because this method requires the "widget-type-around" attribute to be set,
 	 * the insertion can only happen when the widget "fake caret" is active (e.g. activated
@@ -131,19 +131,19 @@ export default class WidgetTypeAround extends Plugin {
 	 * @private
 	 * @returns {Boolean} Returns `true` when the paragraph was inserted (the attribute was present) and `false` otherwise.
 	 */
-	_insertParagraphAccordingToSelectionAttribute() {
+	_insertParagraphAccordingToFakeCaretPosition() {
 		const editor = this.editor;
 		const model = editor.model;
 		const modelSelection = model.document.selection;
-		const typeAroundSelectionAttributeValue = modelSelection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+		const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( modelSelection );
 
-		if ( !typeAroundSelectionAttributeValue ) {
+		if ( !typeAroundFakeCaretPosition ) {
 			return false;
 		}
 
 		const selectedModelElement = modelSelection.getSelectedElement();
 
-		this._insertParagraph( selectedModelElement, typeAroundSelectionAttributeValue );
+		this._insertParagraph( selectedModelElement, typeAroundFakeCaretPosition );
 
 		return true;
 	}
@@ -283,13 +283,13 @@ export default class WidgetTypeAround extends Plugin {
 				return;
 			}
 
-			const typeAroundSelectionAttribute = data.selection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+			const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( data.selection );
 
-			if ( !typeAroundSelectionAttribute ) {
+			if ( !typeAroundFakeCaretPosition ) {
 				return;
 			}
 
-			writer.addClass( positionToWidgetCssClass( typeAroundSelectionAttribute ), selectedViewElement );
+			writer.addClass( positionToWidgetCssClass( typeAroundFakeCaretPosition ), selectedViewElement );
 
 			// Remember the view widget that got the "fake-caret" CSS class. This class should be removed ASAP when the
 			// selection changes
@@ -367,12 +367,12 @@ export default class WidgetTypeAround extends Plugin {
 		const editor = this.editor;
 		const model = editor.model;
 		const modelSelection = model.document.selection;
-		const typeAroundSelectionAttribute = modelSelection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+		const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( modelSelection );
 
 		return model.change( writer => {
-			// If the selection already has the attribute...
-			if ( typeAroundSelectionAttribute ) {
-				const isLeavingWidget = typeAroundSelectionAttribute === ( isForward ? 'after' : 'before' );
+			// If the fake caret is displayed...
+			if ( typeAroundFakeCaretPosition ) {
+				const isLeavingWidget = typeAroundFakeCaretPosition === ( isForward ? 'after' : 'before' );
 
 				// If the keyboard arrow works against the value of the selection attribute...
 				// then remove the selection attribute but prevent default DOM actions
@@ -388,7 +388,7 @@ export default class WidgetTypeAround extends Plugin {
 					return true;
 				}
 			}
-			// If the selection didn't have the attribute, let's set it now according to the direction of the arrow
+			// If the fake caret wasn't displayed, let's set it now according to the direction of the arrow
 			// key press. This also means we cannot let the Widget plugin listener move the selection.
 			else {
 				writer.setSelectionAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE, isForward ? 'after' : 'before' );
@@ -495,7 +495,7 @@ export default class WidgetTypeAround extends Plugin {
 
 			// First check if the widget is selected and there's a type around selection attribute associated
 			// with the "fake caret" that would tell where to insert a new paragraph.
-			if ( this._insertParagraphAccordingToSelectionAttribute() ) {
+			if ( this._insertParagraphAccordingToFakeCaretPosition() ) {
 				wasHandled = true;
 			}
 			// Then, if there is no selection attribute associated with the "fake caret", check if the widget
@@ -546,7 +546,7 @@ export default class WidgetTypeAround extends Plugin {
 		editingView.document.on( 'keydown', ( evt, domEventData ) => {
 			// Don't handle enter/backspace/delete here. They are handled in dedicated listeners.
 			if ( !keyCodesHandledSomewhereElse.includes( domEventData.keyCode ) && !isNonTypingKeystroke( domEventData ) ) {
-				this._insertParagraphAccordingToSelectionAttribute();
+				this._insertParagraphAccordingToFakeCaretPosition();
 			}
 		}, { priority: priorities.get( 'high' ) + 1 } );
 	}
@@ -569,17 +569,17 @@ export default class WidgetTypeAround extends Plugin {
 
 		// Note: The priority must precede the default Widget class delete handler.
 		this.listenTo( editingView.document, 'delete', ( evt, domEventData ) => {
-			const typeAroundSelectionAttributeValue = model.document.selection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+			const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( model.document.selection );
 
 			// This listener handles only these cases when the "fake caret" is active.
-			if ( !typeAroundSelectionAttributeValue ) {
+			if ( !typeAroundFakeCaretPosition ) {
 				return;
 			}
 
 			const direction = domEventData.direction;
 			const selectedModelWidget = model.document.selection.getSelectedElement();
 
-			const isFakeCaretBefore = typeAroundSelectionAttributeValue === 'before';
+			const isFakeCaretBefore = typeAroundFakeCaretPosition === 'before';
 			const isForwardDelete = direction == 'forward';
 			const shouldDeleteEntireWidget = isFakeCaretBefore === isForwardDelete;
 
@@ -589,7 +589,7 @@ export default class WidgetTypeAround extends Plugin {
 				} );
 			} else {
 				const range = schema.getNearestSelectionRange(
-					model.createPositionAt( selectedModelWidget, typeAroundSelectionAttributeValue ),
+					model.createPositionAt( selectedModelWidget, typeAroundFakeCaretPosition ),
 					direction
 				);
 
@@ -652,9 +652,9 @@ export default class WidgetTypeAround extends Plugin {
 				return;
 			}
 
-			const typeAroundSelectionAttributeValue = documentSelection.getAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE );
+			const typeAroundFakeCaretPosition = getTypeAroundFakeCaretPosition( documentSelection );
 
-			if ( !typeAroundSelectionAttributeValue ) {
+			if ( !typeAroundFakeCaretPosition ) {
 				return;
 			}
 
@@ -662,7 +662,7 @@ export default class WidgetTypeAround extends Plugin {
 
 			return model.change( writer => {
 				const selectedElement = documentSelection.getSelectedElement();
-				const position = model.createPositionAt( selectedElement, typeAroundSelectionAttributeValue );
+				const position = model.createPositionAt( selectedElement, typeAroundFakeCaretPosition );
 				const selection = writer.createSelection( position );
 
 				const result = model.insertContent( content, selection );

+ 72 - 0
packages/ckeditor5-widget/tests/utils.js

@@ -327,6 +327,11 @@ describe( 'widget utils', () => {
 				isBlock: true
 			} );
 
+			model.schema.register( 'horizontalLine', {
+				isObject: true,
+				allowWhere: '$block'
+			} );
+
 			model.schema.extend( 'span', { allowIn: 'paragraph' } );
 			model.schema.extend( '$text', { allowIn: 'span' } );
 		} );
@@ -410,6 +415,73 @@ describe( 'widget utils', () => {
 
 			expect( pos.path ).to.deep.equal( [ 3 ] );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/7438
+		describe( 'integration with the WidgetTypeAround feature ("widget-type-around" model selection attribute)', () => {
+			it( 'should respect the attribute value when a widget (block and an object) is selected ("fake caret" before a widget)', () => {
+				setData( model, '<paragraph>x</paragraph>[<image></image>]<paragraph>y</paragraph>' );
+
+				model.change( writer => {
+					writer.setSelectionAttribute( 'widget-type-around', 'before' );
+				} );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 1 ] );
+			} );
+
+			it( 'should respect the attribute value when a widget (block and an object) is selected ("fake caret" after a widget)', () => {
+				setData( model, '<paragraph>x</paragraph>[<image></image>]<paragraph>y</paragraph>' );
+
+				model.change( writer => {
+					writer.setSelectionAttribute( 'widget-type-around', 'after' );
+				} );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 2 ] );
+			} );
+
+			it( 'should return a position after a selected widget (block and an object) ("fake caret" not displayed)', () => {
+				setData( model, '<paragraph>x</paragraph>[<image></image>]<paragraph>y</paragraph>' );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 2 ] );
+			} );
+
+			it( 'should respect the attribute value when a widget (an object) is selected ("fake caret" before a widget)', () => {
+				setData( model, '<paragraph>x</paragraph>[<horizontalLine></horizontalLine>]<paragraph>y</paragraph>' );
+
+				model.change( writer => {
+					writer.setSelectionAttribute( 'widget-type-around', 'before' );
+				} );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 1 ] );
+			} );
+
+			it( 'should respect the attribute value when a widget (an object) is selected ("fake caret" after a widget)', () => {
+				setData( model, '<paragraph>x</paragraph>[<horizontalLine></horizontalLine>]<paragraph>y</paragraph>' );
+
+				model.change( writer => {
+					writer.setSelectionAttribute( 'widget-type-around', 'after' );
+				} );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 2 ] );
+			} );
+
+			it( 'should return a position after a selected widget (an object) ("fake caret" not displayed)', () => {
+				setData( model, '<paragraph>x</paragraph>[<horizontalLine></horizontalLine>]<paragraph>y</paragraph>' );
+
+				const pos = findOptimalInsertionPosition( doc.selection, model );
+
+				expect( pos.path ).to.deep.equal( [ 2 ] );
+			} );
+		} );
 	} );
 
 	describe( 'viewToModelPositionOutsideModelElement()', () => {

+ 36 - 0
packages/ckeditor5-widget/tests/widgettypearound/utils.js

@@ -0,0 +1,36 @@
+/**
+ * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+import Selection from '@ckeditor/ckeditor5-engine/src/model/selection';
+import {
+	TYPE_AROUND_SELECTION_ATTRIBUTE,
+	getTypeAroundFakeCaretPosition
+} from '../../src/widgettypearound/utils';
+
+describe( 'widget type around utils', () => {
+	let selection;
+
+	beforeEach( () => {
+		selection = new Selection();
+	} );
+
+	describe( 'getTypeAroundFakeCaretPosition()', () => {
+		it( 'should return "before" if the model selection attribute is "before"', () => {
+			selection.setAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE, 'before' );
+
+			expect( getTypeAroundFakeCaretPosition( selection ) ).to.equal( 'before' );
+		} );
+
+		it( 'should return "after" if the model selection attribute is "after"', () => {
+			selection.setAttribute( TYPE_AROUND_SELECTION_ATTRIBUTE, 'after' );
+
+			expect( getTypeAroundFakeCaretPosition( selection ) ).to.equal( 'after' );
+		} );
+
+		it( 'should return undefined if the model selection attribute is not set', () => {
+			expect( getTypeAroundFakeCaretPosition( selection ) ).to.be.undefined;
+		} );
+	} );
+} );