From d5a86cf762d3294cd90e2657cf04ab988765cdd1 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 10 Jun 2020 16:13:28 +0200 Subject: [PATCH] Refactoring in the logic responsible for deleting content around widgets. --- packages/ckeditor5-table/src/tablekeyboard.js | 7 +- packages/ckeditor5-widget/src/widget.js | 10 +- .../src/widgettypearound/widgettypearound.js | 114 +++++++----------- .../widgettypearound/widgettypearound.js | 30 ++++- 4 files changed, 78 insertions(+), 83 deletions(-) diff --git a/packages/ckeditor5-table/src/tablekeyboard.js b/packages/ckeditor5-table/src/tablekeyboard.js index fffbc6043db..6fdf2680228 100644 --- a/packages/ckeditor5-table/src/tablekeyboard.js +++ b/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 } ); } diff --git a/packages/ckeditor5-widget/src/widget.js b/packages/ckeditor5-widget/src/widget.js index ed417188f41..da035fefd04 100644 --- a/packages/ckeditor5-widget/src/widget.js +++ b/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(); diff --git a/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js b/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js index 5d2a4dcc34f..0681781963f 100644 --- a/packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js +++ b/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. - // - // [] - // - // 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. - // - // bar[] -> ba[] - // - 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. - // - // [] - // - // 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. - // - // []bar -> []ar - // - 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 ``: +// Returns the ancestor of an element closest to the root which is empty. For instance, +// for ``: // -// abc[] +// abc // // it returns ``. // // @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; } diff --git a/packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js b/packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js index 73a6919e18a..bf2fd85e9d9 100644 --- a/packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js +++ b/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, '
' + 'foo' + @@ -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( '
' + - 'foo' + + 'foo[]' + '
' + - '[]' + '' ); - 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, '[]' + '
' + @@ -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( - '[]' + + '' + '
' + - 'foo' + + '[]foo' + '
' ); - 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 );