瀏覽代碼

Refactoring in the logic responsible for deleting content around widgets.

Kuba Niegowski 5 年之前
父節點
當前提交
d5a86cf762

+ 3 - 4
packages/ckeditor5-table/src/tablekeyboard.js

@@ -53,10 +53,9 @@ export default class TableKeyboard extends Plugin {
 		this.editor.keystrokes.set( 'Tab', this._getTabHandler( true ), { priority: 'low' } );
 		this.editor.keystrokes.set( 'Shift+Tab', this._getTabHandler( false ), { priority: 'low' } );
 
-		// Note: This listener has the "high+1" priority because we would like to avoid collisions with other features
-		// (like Widgets), which take over the keydown events with the "high" priority. Table navigation takes precedence
-		// over Widgets in that matter (widget arrow handler stops propagation of event if object element was selected
-		// but getNearestSelectionRange didn't returned any range).
+		// Note: This listener has the "high-10" priority because it should allow the Widget plugin to handle the default
+		// behavior first ("high") but it should not be "prevent–defaulted" by the Widget plugin ("high-20") because of
+		// the fake selection retention on the fully selected widget.
 		this.listenTo( viewDocument, 'keydown', ( ...args ) => this._onKeydown( ...args ), { priority: priorities.get( 'high' ) - 10 } );
 	}
 

+ 5 - 5
packages/ckeditor5-widget/src/widget.js

@@ -221,9 +221,9 @@ export default class Widget extends Plugin {
 
 				domEventData.preventDefault();
 				eventInfo.stop();
-
-				return;
 			}
+
+			return;
 		}
 
 		// If selection is next to object element.
@@ -232,10 +232,10 @@ export default class Widget extends Plugin {
 			return;
 		}
 
-		const objectElement2 = this._getObjectElementNextToSelection( isForward );
+		const objectElementNextToSelection = this._getObjectElementNextToSelection( isForward );
 
-		if ( !!objectElement2 && schema.isObject( objectElement2 ) ) {
-			this._setSelectionOverElement( objectElement2 );
+		if ( objectElementNextToSelection && schema.isObject( objectElementNextToSelection ) ) {
+			this._setSelectionOverElement( objectElementNextToSelection );
 
 			domEventData.preventDefault();
 			eventInfo.stop();

+ 46 - 68
packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js

@@ -67,14 +67,14 @@ export default class WidgetTypeAround extends Plugin {
 		super( editor );
 
 		/**
-		 * A reference to the editing model widget element that has the "fake caret" active
+		 * A reference to the model widget element that has the "fake caret" active
 		 * on either side of it. It is later used to remove CSS classes associated with the "fake caret"
-		 * when the widget no longer it.
+		 * when the widget no longer needs it.
 		 *
 		 * @private
 		 * @member {module:engine/model/element~Element|null}
 		 */
-		this._currentFakeCaretModelWidget = null;
+		this._currentFakeCaretModelElement = null;
 	}
 
 	/**
@@ -93,7 +93,7 @@ export default class WidgetTypeAround extends Plugin {
 	 * @inheritDoc
 	 */
 	destroy() {
-		this._currentFakeCaretModelWidget = null;
+		this._currentFakeCaretModelElement = null;
 	}
 
 	/**
@@ -218,7 +218,7 @@ export default class WidgetTypeAround extends Plugin {
 
 		// This is the main listener responsible for the "fake caret".
 		// Note: The priority must precede the default Widget class keydown handler ("high") and the
-		// TableKeyboard keydown handler ("high + 1").
+		// TableKeyboard keydown handler ("high-10").
 		editingView.document.on( 'keydown', ( evt, domEventData ) => {
 			if ( isArrowKeyCode( domEventData.keyCode ) ) {
 				this._handleArrowKeyPress( evt, domEventData );
@@ -254,14 +254,14 @@ export default class WidgetTypeAround extends Plugin {
 		editor.editing.downcastDispatcher.on( 'selection', ( evt, data, conversionApi ) => {
 			const writer = conversionApi.writer;
 
-			if ( this._currentFakeCaretModelWidget ) {
-				const selectedViewElement = conversionApi.mapper.toViewElement( this._currentFakeCaretModelWidget );
+			if ( this._currentFakeCaretModelElement ) {
+				const selectedViewElement = conversionApi.mapper.toViewElement( this._currentFakeCaretModelElement );
 
 				if ( selectedViewElement ) {
 					// Get rid of CSS classes associated with the active ("fake horizontal caret") mode from the view widget.
 					writer.removeClass( POSSIBLE_INSERTION_POSITIONS.map( positionToWidgetCssClass ), selectedViewElement );
 
-					this._currentFakeCaretModelWidget = null;
+					this._currentFakeCaretModelElement = null;
 				}
 			}
 
@@ -287,7 +287,7 @@ export default class WidgetTypeAround extends Plugin {
 
 			// Remember the view widget that got the "fake-caret" CSS class. This class should be removed ASAP when the
 			// selection changes
-			this._currentFakeCaretModelWidget = selectedModelElement;
+			this._currentFakeCaretModelElement = selectedModelElement;
 		} );
 
 		this.listenTo( editor.ui.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => {
@@ -582,58 +582,42 @@ export default class WidgetTypeAround extends Plugin {
 				editor.execute( 'delete', {
 					selection: model.createSelection( selectedModelWidget, 'on' )
 				} );
-			} else if ( !isForwardDelete ) {
-				const range = schema.getNearestSelectionRange( model.createPositionBefore( selectedModelWidget ), direction );
-
-				if ( range ) {
-					const deepestEmptyRangeAncestor = getDeepestEmptyPositionAncestor( schema, range.start );
-
-					// Handle a case when there's an empty document tree branch before the widget that should be deleted.
-					//
-					//		<foo><bar></bar></foo>[<widget></widget>]
-					//
-					// Note: Range is collapsed, so it does not matter if this is start or end.
-					if ( deepestEmptyRangeAncestor ) {
-						model.deleteContent( model.createSelection( deepestEmptyRangeAncestor, 'on' ), {
-							doNotAutoparagraph: true
-						} );
-					}
-					// Handle a case when there's a non-empty document tree branch before the widget.
-					//
-					//		<foo>bar</foo>[<widget></widget>] -> <foo>ba[]</foo><widget></widget>
-					//
-					else {
-						model.change( writer => {
-							writer.setSelection( range );
-							editor.execute( 'delete' );
-						} );
-					}
-				}
 			} else {
-				const range = schema.getNearestSelectionRange( model.createPositionAfter( selectedModelWidget ), direction );
+				const range = schema.getNearestSelectionRange(
+					model.createPositionAt( selectedModelWidget, typeAroundSelectionAttributeValue ),
+					direction
+				);
 
+				// If there is somewhere to move selection to, then there will be something to delete.
 				if ( range ) {
-					const deepestEmptyRangeAncestor = getDeepestEmptyPositionAncestor( schema, range.start );
-
-					// Handle a case when there's an empty document tree branch after the widget that should be deleted.
-					//
-					//		[<widget></widget>]<foo><bar></bar></foo>
-					//
-					// Note: Range is collapsed, so it does not matter if this is start or end.
-					if ( deepestEmptyRangeAncestor ) {
-						model.deleteContent( model.createSelection( deepestEmptyRangeAncestor, 'on' ), {
-							doNotAutoparagraph: true
-						} );
-					}
-					// Handle a case when there's a non-empty document tree branch after the widget.
-					//
-					//		[<widget></widget>]<foo>bar</foo> -> <widget></widget><foo>[]ar</foo>
-					//
-					else {
+					// If the range is NOT collapsed, then we know that the range contains an object (see getNearestSelectionRange() docs).
+					if ( !range.isCollapsed ) {
 						model.change( writer => {
 							writer.setSelection( range );
-							editor.execute( 'forwardDelete' );
+							editor.execute( isForwardDelete ? 'forwardDelete' : 'delete' );
 						} );
+					} else {
+						const probe = model.createSelection( range.start );
+						model.modifySelection( probe, { direction } );
+
+						// If the range is collapsed, let's see if a non-collapsed range exists that can could be deleted.
+						// If such range exists, use the editor command because it it safe for collaboration (it merges where it can).
+						if ( !probe.focus.isEqual( range.start ) ) {
+							model.change( writer => {
+								writer.setSelection( range );
+								editor.execute( isForwardDelete ? 'forwardDelete' : 'delete' );
+							} );
+						}
+						// If there is no non-collapsed range to be deleted then we are sure that there is an empty element
+						// next to a widget that should be removed. "delete" and "forwardDelete" commands cannot get rid of it
+						// so calling Model#deleteContent here manually.
+						else {
+							const deepestEmptyRangeAncestor = getDeepestEmptyElementAncestor( schema, range.start.parent );
+
+							model.deleteContent( model.createSelection( deepestEmptyRangeAncestor, 'on' ), {
+								doNotAutoparagraph: true
+							} );
+						}
 					}
 				}
 			}
@@ -709,26 +693,20 @@ function injectFakeCaret( wrapperDomElement ) {
 	wrapperDomElement.appendChild( caretTemplate.render() );
 }
 
-// Returns the ancestor of a position closest to the root which is empty. For instance,
-// for a position in `<baz>`:
+// Returns the ancestor of an element closest to the root which is empty. For instance,
+// for `<baz>`:
 //
-//		<foo>abc<bar><baz>[]</baz></bar></foo>
+//		<foo>abc<bar><baz></baz></bar></foo>
 //
 // it returns `<bar>`.
 //
 // @param {module:engine/model/schema~Schema} schema
-// @param {module:engine/model/position~Position} position
+// @param {module:engine/model/element~Element} element
 // @returns {module:engine/model/element~Element|null}
-function getDeepestEmptyPositionAncestor( schema, position ) {
-	const firstPositionParent = position.parent;
-
-	if ( !firstPositionParent.isEmpty ) {
-		return null;
-	}
-
-	let deepestEmptyAncestor = firstPositionParent;
+function getDeepestEmptyElementAncestor( schema, element ) {
+	let deepestEmptyAncestor = element;
 
-	for ( const ancestor of firstPositionParent.getAncestors( { parentFirst: true } ) ) {
+	for ( const ancestor of element.getAncestors( { parentFirst: true } ) ) {
 		if ( ancestor.childCount > 1 || schema.isLimit( ancestor ) ) {
 			break;
 		}

+ 24 - 6
packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js

@@ -989,6 +989,8 @@ describe( 'WidgetTypeAround', () => {
 				} );
 
 				it( 'should delete an empty document tree sub-branch before a widget if the "fake caret" is also before the widget', () => {
+					let operationType;
+
 					setModelData( editor.model,
 						'<blockQuote>' +
 							'<paragraph>foo</paragraph>' +
@@ -1008,14 +1010,21 @@ describe( 'WidgetTypeAround', () => {
 					);
 					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.equal( 'before' );
 
+					// Assert that the paragraph is merged rather than deleted because
+					// it is safer for collaboration.
+					model.on( 'applyOperation', ( evt, [ operation ] ) => {
+						operationType = operation.type;
+					} );
+
 					fireDeleteEvent();
 					expect( getModelData( model ) ).to.equal(
 						'<blockQuote>' +
-							'<paragraph>foo</paragraph>' +
+							'<paragraph>foo[]</paragraph>' +
 						'</blockQuote>' +
-						'[<blockWidget></blockWidget>]'
+						'<blockWidget></blockWidget>'
 					);
-					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.equal( 'before' );
+					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.be.undefined;
+					expect( operationType ).to.equal( 'merge' );
 
 					sinon.assert.calledOnce( eventInfoStub.stop );
 					sinon.assert.calledOnce( domEventDataStub.domEvent.preventDefault );
@@ -1142,6 +1151,8 @@ describe( 'WidgetTypeAround', () => {
 				} );
 
 				it( 'should delete an empty document tree sub-branch after a widget if the "fake caret" is also after the widget', () => {
+					let operationType;
+
 					setModelData( editor.model,
 						'[<blockWidget></blockWidget>]' +
 						'<blockQuote>' +
@@ -1161,14 +1172,21 @@ describe( 'WidgetTypeAround', () => {
 					);
 					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.equal( 'after' );
 
+					// Assert that the paragraph is merged rather than deleted because
+					// it is safer for collaboration.
+					model.on( 'applyOperation', ( evt, [ operation ] ) => {
+						operationType = operation.type;
+					} );
+
 					fireDeleteEvent( true );
 					expect( getModelData( model ) ).to.equal(
-						'[<blockWidget></blockWidget>]' +
+						'<blockWidget></blockWidget>' +
 						'<blockQuote>' +
-							'<paragraph>foo</paragraph>' +
+							'<paragraph>[]foo</paragraph>' +
 						'</blockQuote>'
 					);
-					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.equal( 'after' );
+					expect( modelSelection.getAttribute( 'widget-type-around' ) ).to.be.undefined;
+					expect( operationType ).to.equal( 'merge' );
 
 					sinon.assert.calledOnce( eventInfoStub.stop );
 					sinon.assert.calledOnce( domEventDataStub.domEvent.preventDefault );