From 4200dbcb1bb3a279f9fc05ccc0590dfa3279e4ff Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 2 Dec 2020 16:56:00 +0100 Subject: [PATCH 1/4] Block quote should be split on backspace key press. Empty block element at the beginning of the limit element should be converted to a paragraph on backspace key press. --- .../src/blockquoteediting.js | 53 ++++++++---- .../tests/integration.js | 47 ++++++++++- packages/ckeditor5-list/src/listediting.js | 3 +- .../ckeditor5-typing/src/deletecommand.js | 80 ++++++++++++++++++- .../ckeditor5-typing/tests/deletecommand.js | 66 +++++++++++++++ 5 files changed, 226 insertions(+), 23 deletions(-) diff --git a/packages/ckeditor5-block-quote/src/blockquoteediting.js b/packages/ckeditor5-block-quote/src/blockquoteediting.js index cfb0472b420..bbe4047a280 100644 --- a/packages/ckeditor5-block-quote/src/blockquoteediting.js +++ b/packages/ckeditor5-block-quote/src/blockquoteediting.js @@ -8,6 +8,7 @@ */ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; +import priorities from '@ckeditor/ckeditor5-utils/src/priorities'; import BlockQuoteCommand from './blockquotecommand'; @@ -102,31 +103,49 @@ export default class BlockQuoteEditing extends Plugin { return false; } ); - } - /** - * @inheritDoc - */ - afterInit() { - const editor = this.editor; - const command = editor.commands.get( 'blockQuote' ); + const viewDocument = this.editor.editing.view.document; + const selection = editor.model.document.selection; + const blockQuoteCommand = editor.commands.get( 'blockQuote' ); // Overwrite default Enter key behavior. // If Enter key is pressed with selection collapsed in empty block inside a quote, break the quote. - // This listener is added in afterInit in order to register it after list's feature listener. - // We can't use a priority for this, because 'low' is already used by the enter feature, unless - // we'd use numeric priority in this case. - this.listenTo( this.editor.editing.view.document, 'enter', ( evt, data ) => { - const doc = this.editor.model.document; - const positionParent = doc.selection.getLastPosition().parent; + // + // Priority normal - 10 to override default handler but not list's feature listener. + this.listenTo( viewDocument, 'enter', ( evt, data ) => { + if ( !selection.isCollapsed || !blockQuoteCommand.value ) { + return; + } - if ( doc.selection.isCollapsed && positionParent.isEmpty && command.value ) { - this.editor.execute( 'blockQuote' ); - this.editor.editing.view.scrollToTheSelection(); + const positionParent = selection.getLastPosition().parent; + + if ( positionParent.isEmpty ) { + editor.execute( 'blockQuote' ); + editor.editing.view.scrollToTheSelection(); data.preventDefault(); evt.stop(); } - } ); + }, { priority: priorities.normal - 10 } ); + + // Overwrite default Backspace key behavior. + // If Backspace key is pressed with selection collapsed in first empty block inside a quote, break the quote. + // + // Priority high + 5 to override widget's feature listener but not list's feature listener. + this.listenTo( viewDocument, 'delete', ( evt, data ) => { + if ( data.direction != 'backward' || !selection.isCollapsed || !blockQuoteCommand.value ) { + return; + } + + const positionParent = selection.getLastPosition().parent; + + if ( positionParent.isEmpty && !positionParent.previousSibling ) { + editor.execute( 'blockQuote' ); + editor.editing.view.scrollToTheSelection(); + + data.preventDefault(); + evt.stop(); + } + }, { priority: priorities.high + 5 } ); } } diff --git a/packages/ckeditor5-block-quote/tests/integration.js b/packages/ckeditor5-block-quote/tests/integration.js index 3905303fcdf..91555ff4beb 100644 --- a/packages/ckeditor5-block-quote/tests/integration.js +++ b/packages/ckeditor5-block-quote/tests/integration.js @@ -281,7 +281,7 @@ describe( 'BlockQuote integration', () => { ); } ); - it( 'removes empty quote when merging into another quote', () => { + it( 'unwraps empty quote when the backspace key pressed in the first empty paragraph in a quote', () => { const data = fakeEventData(); setModelData( model, @@ -295,12 +295,13 @@ describe( 'BlockQuote integration', () => { expect( getModelData( model ) ).to.equal( 'x' + - '
a[]
' + + '
a
' + + '[]' + 'y' ); } ); - it( 'removes empty quote when merging into a paragraph', () => { + it( 'unwraps empty quote when the backspace key pressed in the empty paragraph that is the only content of quote', () => { const data = fakeEventData(); setModelData( model, @@ -312,7 +313,45 @@ describe( 'BlockQuote integration', () => { viewDocument.fire( 'delete', data ); expect( getModelData( model ) ).to.equal( - 'x[]' + + 'x' + + '[]' + + 'y' + ); + } ); + + it( 'unwraps quote from the first paragraph when the backspace key pressed', () => { + const data = fakeEventData(); + + setModelData( model, + 'x' + + '
[]foo
' + + 'y' + ); + + viewDocument.fire( 'delete', data ); + + expect( getModelData( model ) ).to.equal( + 'x' + + '[]' + + '
foo
' + + 'y' + ); + } ); + + it( 'merges paragraphs in a quote when the backspace key pressed not in the first paragraph', () => { + const data = fakeEventData(); + + setModelData( model, + 'x' + + '
[]
' + + 'y' + ); + + viewDocument.fire( 'delete', data ); + + expect( getModelData( model ) ).to.equal( + 'x' + + '
[]
' + 'y' ); } ); diff --git a/packages/ckeditor5-list/src/listediting.js b/packages/ckeditor5-list/src/listediting.js index bb9c6e18671..a77321d4d80 100644 --- a/packages/ckeditor5-list/src/listediting.js +++ b/packages/ckeditor5-list/src/listediting.js @@ -12,6 +12,7 @@ import IndentCommand from './indentcommand'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import priorities from '@ckeditor/ckeditor5-utils/src/priorities'; import { cleanList, @@ -168,7 +169,7 @@ export default class ListEditing extends Plugin { data.preventDefault(); evt.stop(); - }, { priority: 'high' } ); + }, { priority: priorities.high + 10 } ); const getCommandExecuter = commandName => { return ( data, cancel ) => { diff --git a/packages/ckeditor5-typing/src/deletecommand.js b/packages/ckeditor5-typing/src/deletecommand.js index fc19e45acad..17b85e70c01 100644 --- a/packages/ckeditor5-typing/src/deletecommand.js +++ b/packages/ckeditor5-typing/src/deletecommand.js @@ -77,6 +77,7 @@ export default class DeleteCommand extends Command { this._buffer.lock(); const selection = writer.createSelection( options.selection || doc.selection ); + const sequence = options.sequence || 1; // Do not replace the whole selected content if selection was collapsed. // This prevents such situation: @@ -91,12 +92,20 @@ export default class DeleteCommand extends Command { } // Check if deleting in an empty editor. See #61. - if ( this._shouldEntireContentBeReplacedWithParagraph( options.sequence || 1 ) ) { + if ( this._shouldEntireContentBeReplacedWithParagraph( sequence ) ) { this._replaceEntireContentWithParagraph( writer ); return; } + // Check if deleting in the first empty block. + // See https://github.com/ckeditor/ckeditor5/issues/8137. + if ( this._shouldReplaceFirstBlockWithParagraph( selection, sequence ) ) { + this._replaceFirstBlockWithParagraph( selection, writer ); + + return; + } + // If selection is still collapsed, then there's nothing to delete. if ( selection.isCollapsed ) { return; @@ -180,6 +189,7 @@ export default class DeleteCommand extends Command { * The entire content is replaced with the paragraph. Selection is moved inside the paragraph. * * @private + * @param {module:engine/model/writer~Writer} writer The model writer. */ _replaceEntireContentWithParagraph( writer ) { const model = this.editor.model; @@ -193,4 +203,72 @@ export default class DeleteCommand extends Command { writer.setSelection( paragraph, 0 ); } + + /** + * Checks if the selection is inside an empty element that is the first child of the limit element + * and should be replaced with a paragraph. + * + * @private + * @param {module:engine/model/selection~Selection} selection The selection. + * @param {Number} sequence A number describing which subsequent delete event it is without the key being released. + * @returns {Boolean} + */ + _shouldReplaceFirstBlockWithParagraph( selection, sequence ) { + const model = this.editor.model; + + // Does nothing if user pressed and held the "Backspace" key or it was a "Delete" button. + if ( sequence > 1 || this.direction != 'backward' ) { + return false; + } + + if ( !selection.isCollapsed ) { + return false; + } + + const position = selection.getFirstPosition(); + const limitElement = model.schema.getLimitElement( position ); + const limitElementFirstChild = limitElement.getChild( 0 ); + + // Only elements that are direct children of the limit element can be replaced. + // Unwrapping from a block quote should be handled in a dedicated feature. + if ( position.parent != limitElementFirstChild ) { + return false; + } + + // A block should be replaced only if it was empty. + if ( !selection.containsEntireContent( limitElementFirstChild ) ) { + return false; + } + + // Replace with a paragraph only if it's allowed there. + if ( !model.schema.checkChild( limitElement, 'paragraph' ) ) { + return false; + } + + // Does nothing if the limit element already contains only a paragraph. + if ( limitElementFirstChild.name == 'paragraph' ) { + return false; + } + + return true; + } + + /** + * The first child the limit element is replaced by a paragraph. + * + * @private + * @param {module:engine/model/selection~Selection} selection The selection. + * @param {module:engine/model/writer~Writer} writer The model writer. + */ + _replaceFirstBlockWithParagraph( selection, writer ) { + const model = this.editor.model; + + const limitElement = model.schema.getLimitElement( selection ); + const paragraph = writer.createElement( 'paragraph' ); + + writer.remove( limitElement.getChild( 0 ) ); + writer.insert( paragraph, limitElement ); + + writer.setSelection( paragraph, 0 ); + } } diff --git a/packages/ckeditor5-typing/tests/deletecommand.js b/packages/ckeditor5-typing/tests/deletecommand.js index c6866f8087c..a8fa7f2769f 100644 --- a/packages/ckeditor5-typing/tests/deletecommand.js +++ b/packages/ckeditor5-typing/tests/deletecommand.js @@ -315,5 +315,71 @@ describe( 'DeleteCommand', () => { expect( getData( model ) ).to.equal( '[]' ); } ); + + describe( 'with the empty first block', () => { + it( 'replaces the first empty block with paragraph', () => { + setData( model, '[]foo' ); + + editor.execute( 'delete' ); + + expect( getData( model ) ).to.equal( '[]foo' ); + } ); + + it( 'does not replace an element when Backspace key is held', () => { + setData( model, 'foo[]bar' ); + + for ( let sequence = 1; sequence < 10; ++sequence ) { + editor.execute( 'delete', { sequence } ); + } + + expect( getData( model ) ).to.equal( '[]bar' ); + } ); + + it( 'does not replace with paragraph in another paragraph already occurs in limit element', () => { + setData( model, '[]foo' ); + + const element = doc.getRoot().getNodeByPath( [ 0 ] ); + + editor.execute( 'delete' ); + + expect( element ).is.equal( doc.getRoot().getNodeByPath( [ 0 ] ) ); + expect( getData( model ) ).to.equal( '[]foo' ); + } ); + + it( 'does not replace an element if a paragraph is not allowed in current position', () => { + model.schema.addChildCheck( ( ctx, childDef ) => { + if ( ctx.endsWith( '$root' ) && childDef.name == 'paragraph' ) { + return false; + } + } ); + + setData( model, '[]foo' ); + + editor.execute( 'delete' ); + + expect( getData( model ) ).to.equal( '[]foo' ); + } ); + + it( 'does not replace an element if it\'s not empty', () => { + setData( model, '[]foobar' ); + + editor.execute( 'delete' ); + + expect( getData( model ) ).to.equal( '[]foobar' ); + } ); + + it( 'does not replace an element if it\'s wrapped with some other element', () => { + model.schema.register( 'blockQuote', { + allowWhere: '$block', + allowContentOf: '$root' + } ); + + setData( model, '
[]
bar' ); + + editor.execute( 'delete' ); + + expect( getData( model ) ).to.equal( '
[]
bar' ); + } ); + } ); } ); } ); From 3341e49c601b177582aeb899d7bafb2737fc119a Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 2 Dec 2020 17:20:12 +0100 Subject: [PATCH 2/4] Added test for the list feature delete listener priority. --- packages/ckeditor5-list/tests/listediting.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/ckeditor5-list/tests/listediting.js b/packages/ckeditor5-list/tests/listediting.js index b780478d83b..26813e9c5d1 100644 --- a/packages/ckeditor5-list/tests/listediting.js +++ b/packages/ckeditor5-list/tests/listediting.js @@ -291,6 +291,21 @@ describe( 'ListEditing', () => { sinon.assert.calledWithExactly( editor.execute, 'outdentList' ); } ); + + it( 'should outdent empty list when list is nested in block quote', () => { + const domEvtDataStub = { preventDefault() {}, direction: 'backward' }; + + sinon.spy( editor, 'execute' ); + + setModelData( + model, + 'x
[]
' + ); + + editor.editing.view.document.fire( 'delete', domEvtDataStub ); + + sinon.assert.calledWithExactly( editor.execute, 'outdentList' ); + } ); } ); describe( 'tab key handling callback', () => { From 77805199d5b8f3091547edf2a6a3d919785f2d9c Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 3 Dec 2020 11:08:46 +0100 Subject: [PATCH 3/4] Using 'paragraph' command for easier integration with TC. --- .../ckeditor5-typing/src/deletecommand.js | 21 +------------------ .../ckeditor5-typing/tests/deletecommand.js | 11 +++++++--- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/packages/ckeditor5-typing/src/deletecommand.js b/packages/ckeditor5-typing/src/deletecommand.js index 17b85e70c01..1b5a4b2e8d0 100644 --- a/packages/ckeditor5-typing/src/deletecommand.js +++ b/packages/ckeditor5-typing/src/deletecommand.js @@ -101,7 +101,7 @@ export default class DeleteCommand extends Command { // Check if deleting in the first empty block. // See https://github.com/ckeditor/ckeditor5/issues/8137. if ( this._shouldReplaceFirstBlockWithParagraph( selection, sequence ) ) { - this._replaceFirstBlockWithParagraph( selection, writer ); + this.editor.execute( 'paragraph', { selection } ); return; } @@ -252,23 +252,4 @@ export default class DeleteCommand extends Command { return true; } - - /** - * The first child the limit element is replaced by a paragraph. - * - * @private - * @param {module:engine/model/selection~Selection} selection The selection. - * @param {module:engine/model/writer~Writer} writer The model writer. - */ - _replaceFirstBlockWithParagraph( selection, writer ) { - const model = this.editor.model; - - const limitElement = model.schema.getLimitElement( selection ); - const paragraph = writer.createElement( 'paragraph' ); - - writer.remove( limitElement.getChild( 0 ) ); - writer.insert( paragraph, limitElement ); - - writer.setSelection( paragraph, 0 ); - } } diff --git a/packages/ckeditor5-typing/tests/deletecommand.js b/packages/ckeditor5-typing/tests/deletecommand.js index a8fa7f2769f..f59e37b2494 100644 --- a/packages/ckeditor5-typing/tests/deletecommand.js +++ b/packages/ckeditor5-typing/tests/deletecommand.js @@ -3,14 +3,17 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; -import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor'; import DeleteCommand from '../src/deletecommand'; import Delete from '../src/delete'; import ChangeBuffer from '../src/utils/changebuffer'; -import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + +import ParagraphCommand from '@ckeditor/ckeditor5-paragraph/src/paragraphcommand'; +import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; +import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; +import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + describe( 'DeleteCommand', () => { let editor, model, doc; @@ -26,6 +29,8 @@ describe( 'DeleteCommand', () => { const command = new DeleteCommand( editor, 'backward' ); editor.commands.add( 'delete', command ); + editor.commands.add( 'paragraph', new ParagraphCommand( editor ) ); + model.schema.register( 'paragraph', { inheritAllFrom: '$block' } ); model.schema.register( 'heading1', { inheritAllFrom: '$block' } ); } ); From 782a5a0528bcad552f2d4e38864cda801d82f060 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 10 Dec 2020 14:41:19 +0100 Subject: [PATCH 4/4] Added code comment. --- packages/ckeditor5-list/src/listediting.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/ckeditor5-list/src/listediting.js b/packages/ckeditor5-list/src/listediting.js index a77321d4d80..9df189d6b03 100644 --- a/packages/ckeditor5-list/src/listediting.js +++ b/packages/ckeditor5-list/src/listediting.js @@ -135,6 +135,8 @@ export default class ListEditing extends Plugin { // Overwrite default Backspace key behavior. // If Backspace key is pressed with selection collapsed on first position in first list item, outdent it. #83 + // + // Priority high + 10 to override widget and blockquote feature listener. this.listenTo( viewDocument, 'delete', ( evt, data ) => { // Check conditions from those that require less computations like those immediately available. if ( data.direction !== 'backward' ) {