Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/61: Improved the typing when the whole content is selected #111

Merged
merged 26 commits into from
Aug 22, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c3ef549
Improved the typing – when the user held the Backspace or pressed the…
Jul 19, 2017
22bc2ab
Added more TCs to manual tests. [skip ci]
Jul 21, 2017
3380d32
Improved docs. [skip ci]
Jul 24, 2017
6558d59
Simplified the private API of `DeleteCommand#_shouldEntireContentBeRe…
Jul 24, 2017
ef6a30e
Simplified the code in order to avoid repeating the code with the eng…
Jul 24, 2017
74e77f2
Merge branch 'master' into t/61
Jul 24, 2017
60bccf5
Revert: ef6a30e. It was not good idea to follow this way.
Jul 25, 2017
bcd3e32
Merge branch 'master' into t/61
Jul 28, 2017
291f0db
Removed "getLimitElement()" function because it is already available …
Jul 31, 2017
825466a
Merge branch 'master' into t/61
Jul 31, 2017
f0c2230
Merge branch 'master' into t/61
Aug 9, 2017
723ff29
Merge branch 'master' into t/61
Aug 9, 2017
52cde47
Simplified the code (used "Selection#isEntireContentSelected()").
Aug 10, 2017
b20e509
Merge branch 'master' into t/61
Aug 16, 2017
4b364b9
Code style.
Reinmar Aug 16, 2017
5e1608b
Improved docs.
Reinmar Aug 17, 2017
2b254e7
Code refactoring – fixed semantics.
Reinmar Aug 17, 2017
66e9310
Merge branch 'master' into t/61
Reinmar Aug 17, 2017
45e9beb
Aligned Selection usage to changes in the engine.
Reinmar Aug 17, 2017
54b4ad8
Improved the docs and adjusted to changes in engine (Selection API).
Aug 17, 2017
3347a7d
Removed additional options from "getData()" function.
Aug 18, 2017
e1d027a
Removed invalid comment. [skip ci]
Aug 18, 2017
e798b83
Merge branch 'master' into t/61
Reinmar Aug 22, 2017
7c03010
Code style.
Reinmar Aug 22, 2017
7bf6fed
Fixed tests.
Reinmar Aug 22, 2017
9f57e6f
Improved comments.
Reinmar Aug 22, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/delete.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default class Delete extends Plugin {
editor.commands.add( 'delete', new DeleteCommand( editor, 'backward' ) );

this.listenTo( editingView, 'delete', ( evt, data ) => {
editor.execute( data.direction == 'forward' ? 'forwardDelete' : 'delete', { unit: data.unit } );
editor.execute( data.direction == 'forward' ? 'forwardDelete' : 'delete', { unit: data.unit, sequence: data.sequence } );
data.preventDefault();
} );
}
Expand Down
90 changes: 88 additions & 2 deletions src/deletecommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

import Command from '@ckeditor/ckeditor5-core/src/command';
import Selection from '@ckeditor/ckeditor5-engine/src/model/selection';
import Element from '@ckeditor/ckeditor5-engine/src/model/element';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';
import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import ChangeBuffer from './changebuffer';
import count from '@ckeditor/ckeditor5-utils/src/count';

Expand Down Expand Up @@ -54,8 +57,8 @@ export default class DeleteCommand extends Command {
*
* @fires execute
* @param {Object} [options] The command options.
* @param {'character'} [options.unit='character'] See {@link module:engine/controller/modifyselection~modifySelection}'s
* options.
* @param {'character'} [options.unit='character'] See {@link module:engine/controller/modifyselection~modifySelection}'s options.
* @param {Number} [options.sequence=1] See the {@link module:engine/view/document~Document#event:delete} event data.
*/
execute( options = {} ) {
const doc = this.editor.document;
Expand All @@ -73,6 +76,12 @@ export default class DeleteCommand extends Command {

// If selection is still collapsed, then there's nothing to delete.
if ( selection.isCollapsed ) {
const sequence = options.sequence || 1;

if ( this._shouldEntireContentBeReplacedWithParagraph( { sequence } ) ) {
this._replaceEntireContentWithParagraph();
}

return;
}

Expand All @@ -92,4 +101,81 @@ export default class DeleteCommand extends Command {
this._buffer.unlock();
} );
}

/**
* If the user keeps <kbd>Backspace</kbd> or <kbd>Delete</kbd> key, we do nothing because the user can clear
* the whole element without removing them.
*
* But, if the user pressed and released the key, we want to replace the entire content with a paragraph if:
* - the entire content is selected,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line before a list in MD. And start items from * (without additional indentation).

* - the paragraph is allowed in the common ancestor,
* - other paragraph does not occur in the editor.
*
* @private
* @param {Object} options
* @param {Number} options.sequence A number that describes which sequence of the same event is fired.
* @returns {Boolean}
*/
_shouldEntireContentBeReplacedWithParagraph( options ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a private method so just pass the sequence as its direct param. We don't need the options object right now here and since it's private we can do whatever we want with the params in the future.

// Does nothing if user pressed and held the "Backspace" or "Delete" key.
if ( options.sequence > 1 ) {
return false;
}

const document = this.editor.document;
const selection = document.selection;
const limitElement = getLimitElement( document.schema, selection );
const limitStartPosition = Position.createAt( limitElement );
const limitEndPosition = Position.createAt( limitElement, 'end' );

if (
!limitStartPosition.isTouching( selection.getFirstPosition() ) ||
!limitEndPosition.isTouching( selection.getLastPosition() )
) {
return false;
}

if ( !document.schema.check( { name: 'paragraph', inside: limitElement.name } ) ) {
return false;
}

// Does nothing if editor contains an empty paragraph.
if ( selection.getFirstRange().getCommonAncestor().name === 'paragraph' ) {
return false;
}

return true;
}

/**
* The entire content is replaced with the paragraph. Selection is moved inside the paragraph.
*
* @private
*/
_replaceEntireContentWithParagraph() {
const document = this.editor.document;
const selection = document.selection;
const limitElement = getLimitElement( document.schema, selection );
const paragraph = new Element( 'paragraph' );

this._buffer.batch.remove( Range.createIn( limitElement ) );
this._buffer.batch.insert( Position.createAt( limitElement ), paragraph );

selection.collapse( paragraph );
}
}

// Returns the lowest limit element defined in `Schema.limits` for passed selection.
function getLimitElement( schema, selection ) {
let element = selection.getFirstRange().getCommonAncestor();

while ( !schema.limits.has( element.name ) ) {
if ( element.parent ) {
element = element.parent;
} else {
break;
}
}

return element;
}
11 changes: 11 additions & 0 deletions src/deleteobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ export default class DeleteObserver extends Observer {
constructor( document ) {
super( document );

let sequence = 0;

document.on( 'keyup', ( evt, data ) => {
if ( data.keyCode == keyCodes.delete || data.keyCode == keyCodes.backspace ) {
sequence = 0;
}
} );

document.on( 'keydown', ( evt, data ) => {
const deleteData = {};

Expand All @@ -34,6 +42,7 @@ export default class DeleteObserver extends Observer {
}

deleteData.unit = data.altKey ? 'word' : deleteData.unit;
deleteData.sequence = ++sequence;

document.fire( 'delete', new DomEventData( document, data.domEvent, deleteData ) );
} );
Expand All @@ -55,4 +64,6 @@ export default class DeleteObserver extends Observer {
* @param {module:engine/view/observer/domeventdata~DomEventData} data
* @param {'forward'|'delete'} data.direction The direction in which the deletion should happen.
* @param {'character'|'word'} data.unit The "amount" of content that should be deleted.
* @param {Number} data.sequence A number that describes which sequence of the same event is fired.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A number describing which subsequent delete event it is without the key being released. If it's 2 or more it means that the key was pressed and hold.

* It helps detect the key was pressed and held.
*/
10 changes: 6 additions & 4 deletions tests/delete.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,23 @@ describe( 'Delete feature', () => {

view.fire( 'delete', new DomEventData( editingView, domEvt, {
direction: 'forward',
unit: 'character'
unit: 'character',
sequence: 1
} ) );

expect( spy.calledOnce ).to.be.true;
expect( spy.calledWithMatch( 'forwardDelete', { unit: 'character' } ) ).to.be.true;
expect( spy.calledWithMatch( 'forwardDelete', { unit: 'character', sequence: 1 } ) ).to.be.true;

expect( domEvt.preventDefault.calledOnce ).to.be.true;

view.fire( 'delete', new DomEventData( editingView, getDomEvent(), {
direction: 'backward',
unit: 'character'
unit: 'character',
sequence: 5
} ) );

expect( spy.calledTwice ).to.be.true;
expect( spy.calledWithMatch( 'delete', { unit: 'character' } ) ).to.be.true;
expect( spy.calledWithMatch( 'delete', { unit: 'character', sequence: 5 } ) ).to.be.true;
} );

function getDomEvent() {
Expand Down
106 changes: 94 additions & 12 deletions tests/deletecommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ describe( 'DeleteCommand', () => {
const command = new DeleteCommand( editor, 'backward' );
editor.commands.add( 'delete', command );

doc.schema.registerItem( 'p', '$block' );
doc.schema.registerItem( 'paragraph', '$block' );
doc.schema.registerItem( 'heading1', '$block' );
} );
} );

Expand All @@ -38,7 +39,7 @@ describe( 'DeleteCommand', () => {

describe( 'execute()', () => {
it( 'uses enqueueChanges', () => {
setData( doc, '<p>foo[]bar</p>' );
setData( doc, '<paragraph>foo[]bar</paragraph>' );

const spy = testUtils.sinon.spy( doc, 'enqueueChanges' );

Expand All @@ -48,7 +49,7 @@ describe( 'DeleteCommand', () => {
} );

it( 'locks buffer when executing', () => {
setData( doc, '<p>foo[]bar</p>' );
setData( doc, '<paragraph>foo[]bar</paragraph>' );

const buffer = editor.commands.get( 'delete' )._buffer;
const lockSpy = testUtils.sinon.spy( buffer, 'lock' );
Expand All @@ -61,46 +62,46 @@ describe( 'DeleteCommand', () => {
} );

it( 'deletes previous character when selection is collapsed', () => {
setData( doc, '<p>foo[]bar</p>' );
setData( doc, '<paragraph>foo[]bar</paragraph>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<p>fo[]bar</p>' );
expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>fo[]bar</paragraph>' );
} );

it( 'deletes selection contents', () => {
setData( doc, '<p>fo[ob]ar</p>' );
setData( doc, '<paragraph>fo[ob]ar</paragraph>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<p>fo[]ar</p>' );
expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>fo[]ar</paragraph>' );
} );

it( 'merges elements', () => {
setData( doc, '<p>foo</p><p>[]bar</p>' );
setData( doc, '<paragraph>foo</paragraph><paragraph>[]bar</paragraph>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<p>foo[]bar</p>' );
expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>foo[]bar</paragraph>' );
} );

it( 'does not try to delete when selection is at the boundary', () => {
const spy = sinon.spy();

editor.data.on( 'deleteContent', spy );
setData( doc, '<p>[]foo</p>' );
setData( doc, '<paragraph>[]foo</paragraph>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<p>[]foo</p>' );
expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>[]foo</paragraph>' );
expect( spy.callCount ).to.equal( 0 );
} );

it( 'passes options to modifySelection', () => {
const spy = sinon.spy();

editor.data.on( 'modifySelection', spy );
setData( doc, '<p>foo[]bar</p>' );
setData( doc, '<paragraph>foo[]bar</paragraph>' );

editor.commands.get( 'delete' ).direction = 'forward';

Expand All @@ -112,5 +113,86 @@ describe( 'DeleteCommand', () => {
expect( modifyOpts ).to.have.property( 'direction', 'forward' );
expect( modifyOpts ).to.have.property( 'unit', 'word' );
} );

it( 'leaves an empty paragraph after removing the whole content from editor', () => {
setData( doc, '<heading1>[Header 1</heading1><paragraph>Some text.]</paragraph>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>[]</paragraph>' );
} );

it( 'leaves an empty paragraph after removing the whole content inside limit element', () => {
doc.schema.registerItem( 'section', '$root' );
doc.schema.limits.add( 'section' );
doc.schema.allow( { name: 'section', inside: '$root' } );

setData( doc,
'<heading1>Foo</heading1>' +
'<section>' +
'<heading1>[Header 1</heading1>' +
'<paragraph>Some text.]</paragraph>' +
'</section>' +
'<paragraph>Bar.</paragraph>'
);

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal(
'<heading1>Foo</heading1>' +
'<section>' +
'<paragraph>[]</paragraph>' +
'</section>' +
'<paragraph>Bar.</paragraph>'
);
} );

it( 'leaves an empty paragraph after removing the whole content when root element was not added as Schema.limits', () => {
doc.schema.limits.delete( '$root' );

setData( doc, '<heading1>[]</heading1>' );

editor.execute( 'delete' );

expect( getData( doc ) ).to.equal( '<paragraph>[]</paragraph>' );
} );

it( 'replaces an empty element with paragraph', () => {
setData( doc, '<heading1>[]</heading1>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<paragraph>[]</paragraph>' );
} );

it( 'does not replace an element when Backspace or Delete key is held', () => {
setData( doc, '<heading1>Bar[]</heading1>' );

for ( let sequence = 1; sequence < 10; ++sequence ) {
editor.execute( 'delete', { sequence } );
}

expect( getData( doc, { selection: true } ) ).to.equal( '<heading1>[]</heading1>' );
} );

it( 'does not replace an element if a paragraph is a common ancestor', () => {
setData( doc, '<paragraph>[]</paragraph>' );

const element = doc.selection.getFirstRange().getCommonAncestor();

editor.execute( 'delete' );

expect( element ).is.equal( doc.selection.getFirstRange().getCommonAncestor() );
} );

it( 'does not replace an element if a paragraph is not allowed in current position', () => {
doc.schema.disallow( { name: 'paragraph', inside: '$root' } );

setData( doc, '<heading1>[]</heading1>' );

editor.execute( 'delete' );

expect( getData( doc, { selection: true } ) ).to.equal( '<heading1>[]</heading1>' );
} );
} );
} );
Loading