Skip to content

Commit

Permalink
Refactoring in the logic responsible for deleting content around widg…
Browse files Browse the repository at this point in the history 
…ets.
  • Loading branch information
@niegowski @oleq
niegowski authored and oleq committed Jun 10, 2020
1 parent 73c2272 commit d5a86cf
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 83 deletions.
7 changes: 3 additions & 4 deletions packages/ckeditor5-table/src/tablekeyboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 } );
}

Expand Down
10 changes: 5 additions & 5 deletions packages/ckeditor5-widget/src/widget.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,9 @@ export default class Widget extends Plugin {

domEventData.preventDefault();
eventInfo.stop();

return;
}

return;
}

// If selection is next to object element.
Expand All @@ -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();
Expand Down
114 changes: 46 additions & 68 deletions packages/ckeditor5-widget/src/widgettypearound/widgettypearound.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -93,7 +93,7 @@ export default class WidgetTypeAround extends Plugin {
* @inheritDoc
*/
destroy() {
this._currentFakeCaretModelWidget = null;
this._currentFakeCaretModelElement = null;
}

/**
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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 ) => {
Expand Down Expand Up @@ -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
} );
}
}
}
}
Expand Down Expand Up @@ -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;
}
Expand Down
30 changes: 24 additions & 6 deletions packages/ckeditor5-widget/tests/widgettypearound/widgettypearound.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>' +
Expand All @@ -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 );
Expand Down Expand Up @@ -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>' +
Expand All @@ -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 );
Expand Down

0 comments on commit d5a86cf

Please sign in to comment.