From 6bca2d99281cf7a60d473a8b1432dc18406105c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 16 Sep 2019 17:59:30 +0200 Subject: [PATCH 1/4] Properly remove label from todo list item when changing list item type. --- src/todolistconverters.js | 22 ++++++++++++++++++++-- src/todolistediting.js | 2 +- tests/manual/todo-list.js | 5 +++-- tests/todolistediting.js | 39 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/todolistconverters.js b/src/todolistconverters.js index dfbcbf9..327e6d7 100644 --- a/src/todolistconverters.js +++ b/src/todolistconverters.js @@ -233,7 +233,7 @@ export function dataViewModelCheckmarkInsertion( evt, data, conversionApi ) { * @param {Function} onCheckedChange Callback fired after clicking the checkbox UI element. * @returns {Function} Returns a conversion callback. */ -export function modelViewChangeType( onCheckedChange ) { +export function modelViewChangeType( onCheckedChange, view ) { return ( evt, data, conversionApi ) => { const viewItem = conversionApi.mapper.toViewElement( data.item ); const viewWriter = conversionApi.writer; @@ -246,7 +246,12 @@ export function modelViewChangeType( onCheckedChange ) { viewWriter.insert( viewWriter.createPositionAt( viewItem, 0 ), checkmarkElement ); } else if ( data.attributeOldValue == 'todo' ) { viewWriter.removeClass( 'todo-list', viewItem.parent ); - viewWriter.remove( viewItem.getChild( 0 ) ); + + const label = findLabel( viewItem, view ); + + if ( label ) { + viewWriter.remove( label ); + } } }; } @@ -321,3 +326,16 @@ function createCheckmarkElement( modelItem, viewWriter, isChecked, onChange ) { return uiElement; } + +// Helper method to find label element inside li. +function findLabel( viewItem, view ) { + const range = view.createRangeIn( viewItem ); + + let label; + for ( const value of range ) { + if ( value.item.is( 'uiElement', 'label' ) ) { + label = value.item; + } + } + return label; +} diff --git a/src/todolistediting.js b/src/todolistediting.js index c203137..a14e370 100644 --- a/src/todolistediting.js +++ b/src/todolistediting.js @@ -79,7 +79,7 @@ export default class TodoListEditing extends Plugin { editing.downcastDispatcher.on( 'attribute:listType:listItem', - modelViewChangeType( listItem => this._handleCheckmarkChange( listItem ) ) + modelViewChangeType( listItem => this._handleCheckmarkChange( listItem ), editing.view ) ); editing.downcastDispatcher.on( 'attribute:todoListChecked:listItem', diff --git a/tests/manual/todo-list.js b/tests/manual/todo-list.js index b78cc9d..f9ce0a7 100644 --- a/tests/manual/todo-list.js +++ b/tests/manual/todo-list.js @@ -17,12 +17,13 @@ import Undo from '@ckeditor/ckeditor5-undo/src/undo'; import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard'; import List from '../../src/list'; import TodoList from '../../src/todolist'; +import Link from '../../../ckeditor5-link/src/link'; ClassicEditor .create( document.querySelector( '#editor' ), { - plugins: [ Enter, Typing, Heading, Highlight, Table, Bold, Paragraph, Undo, List, TodoList, Clipboard ], + plugins: [ Enter, Typing, Heading, Highlight, Table, Bold, Paragraph, Undo, List, TodoList, Clipboard, Link ], toolbar: [ - 'heading', '|', 'bulletedList', 'numberedList', 'todoList', '|', 'bold', 'highlight', 'insertTable', '|', 'undo', 'redo' + 'heading', '|', 'bulletedList', 'numberedList', 'todoList', '|', 'bold', 'link', 'highlight', 'insertTable', '|', 'undo', 'redo' ], table: { contentToolbar: [ diff --git a/tests/todolistediting.js b/tests/todolistediting.js index 74bd0c4..2f60a28 100644 --- a/tests/todolistediting.js +++ b/tests/todolistediting.js @@ -12,6 +12,7 @@ import ListCommand from '../src/listcommand'; import TodoListCheckCommand from '../src/todolistcheckcommand'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; import InlineEditableUIView from '@ckeditor/ckeditor5-ui/src/editableui/inline/inlineeditableuiview'; +import LinkEditing from '@ckeditor/ckeditor5-link/src/linkediting'; import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; @@ -27,7 +28,7 @@ describe( 'TodoListEditing', () => { beforeEach( () => { return VirtualTestEditor .create( { - plugins: [ TodoListEditing, Typing, BoldEditing, BlockQuoteEditing ] + plugins: [ TodoListEditing, Typing, BoldEditing, BlockQuoteEditing, LinkEditing ] } ) .then( newEditor => { editor = newEditor; @@ -287,6 +288,42 @@ describe( 'TodoListEditing', () => { ); } ); + it( 'should properly convert list type change - inner text with attribute', () => { + setModelData( model, + '1[.0' + + '<$text bold="true">2.0' + + '3.]0' + ); + + editor.execute( 'bulletedList' ); + + expect( getViewData( view ) ).to.equal( + '' + ); + } ); + + it( 'should properly convert list type change - inner text with many attributes', () => { + setModelData( model, + '1[.0' + + '<$text bold="true" linkHref="foo">2.0' + + '3.]0' + ); + + editor.execute( 'bulletedList' ); + + expect( getViewData( view ) ).to.equal( + '' + ); + } ); + it( 'should convert todoListChecked attribute change', () => { setModelData( model, '1.0' ); From 4ad9d4b7a864eda458b0746d542ed168b827ef73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 16 Sep 2019 18:27:32 +0200 Subject: [PATCH 2/4] Fix typing inside a text node with attributes at the beginning of a todo list item. --- src/todolistconverters.js | 55 ++++++++++++++++++++++----------------- src/todolistediting.js | 2 +- tests/todolistediting.js | 44 +++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/todolistconverters.js b/src/todolistconverters.js index 327e6d7..07095e5 100644 --- a/src/todolistconverters.js +++ b/src/todolistconverters.js @@ -9,7 +9,7 @@ /* global document */ -import { generateLiInUl, injectViewList, findInRange } from './utils'; +import { findInRange, generateLiInUl, injectViewList } from './utils'; import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement'; /** @@ -71,27 +71,39 @@ export function modelViewInsertion( model, onCheckboxChecked ) { * @param {Object} data Additional information about the change. * @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi Conversion interface. */ -export function modelViewTextInsertion( evt, data, conversionApi ) { - const parent = data.range.start.parent; +export function modelViewTextInsertion( view ) { + return ( evt, data, conversionApi ) => { + const parent = data.range.start.parent; - if ( parent.name != 'listItem' || parent.getAttribute( 'listType' ) != 'todo' ) { - return; - } + if ( parent.name != 'listItem' || parent.getAttribute( 'listType' ) != 'todo' ) { + return; + } - if ( !conversionApi.consumable.consume( data.item, 'insert' ) ) { - return; - } + if ( !conversionApi.consumable.consume( data.item, 'insert' ) ) { + return; + } - const viewWriter = conversionApi.writer; - const viewPosition = conversionApi.mapper.toViewPosition( data.range.start ); - const viewText = viewWriter.createText( data.item.data ); + const viewWriter = conversionApi.writer; + const viewPosition = conversionApi.mapper.toViewPosition( data.range.start ); + const viewText = viewWriter.createText( data.item.data ); + + // Be sure text is created after the UIElement, so if it is a first text node inside a `listItem` element + // it has to be moved after the first node in the view list item. + // + // model: [foo] + // view:
  • ^
    • ->
  • foo
    • + const textInsertionPosition = viewPosition.offset ? viewPosition : viewPosition.getShiftedBy( 1 ); + + const label = findLabel( viewPosition.parent, view ); + + if ( label && label.parent !== viewPosition.parent && viewPosition.offset === 0 ) { + viewWriter.insert( view.createPositionAfter( label ), viewText ); - // Be sure text is created after the UIElement, so if it is a first text node inside a `listItem` element - // it has to be moved after the first node in the view list item. - // - // model: [foo] - // view:
  • ^
    • ->
  • foo
    • - viewWriter.insert( viewPosition.offset ? viewPosition : viewPosition.getShiftedBy( 1 ), viewText ); + return; + } + + viewWriter.insert( textInsertionPosition, viewText ); + }; } /** @@ -246,12 +258,7 @@ export function modelViewChangeType( onCheckedChange, view ) { viewWriter.insert( viewWriter.createPositionAt( viewItem, 0 ), checkmarkElement ); } else if ( data.attributeOldValue == 'todo' ) { viewWriter.removeClass( 'todo-list', viewItem.parent ); - - const label = findLabel( viewItem, view ); - - if ( label ) { - viewWriter.remove( label ); - } + viewWriter.remove( findLabel( viewItem, view ) ); } }; } diff --git a/src/todolistediting.js b/src/todolistediting.js index a14e370..750f946 100644 --- a/src/todolistediting.js +++ b/src/todolistediting.js @@ -73,7 +73,7 @@ export default class TodoListEditing extends Plugin { modelViewInsertion( model, listItem => this._handleCheckmarkChange( listItem ) ), { priority: 'high' } ); - editing.downcastDispatcher.on( 'insert:$text', modelViewTextInsertion, { priority: 'high' } ); + editing.downcastDispatcher.on( 'insert:$text', modelViewTextInsertion( editing.view ), { priority: 'high' } ); data.downcastDispatcher.on( 'insert:listItem', dataModelViewInsertion( model ), { priority: 'high' } ); data.downcastDispatcher.on( 'insert:$text', dataModelViewTextInsertion, { priority: 'high' } ); diff --git a/tests/todolistediting.js b/tests/todolistediting.js index 2f60a28..bca8b3a 100644 --- a/tests/todolistediting.js +++ b/tests/todolistediting.js @@ -411,6 +411,50 @@ describe( 'TodoListEditing', () => { model.change( writer => writer.setAttribute( 'todoListChecked', true, modelRoot.getChild( 0 ) ) ); expect( getViewData( view ) ).to.equal( '{}Foo' ); } ); + + it( 'should properly handle typing inside text node with attribute', () => { + setModelData( model, '<$text bold="true">[]foo' ); + + editor.execute( 'input', { text: 'b' } ); + + expect( getModelData( model ) ).to.equal( + '<$text bold="true">b[]foo' + ); + + expect( getViewData( view ) ).to.equal( + '' + ); + } ); + + it( 'should properly handle typing inside text node with many attributes', () => { + setModelData( model, + '<$text bold="true" linkHref="foo">[]foo' + ); + + editor.execute( 'input', { text: 'b' } ); + + expect( getModelData( model ) ).to.equal( + '<$text bold="true" linkHref="foo">b[]foo' + ); + + expect( getViewData( view ) ).to.equal( + '' + ); + } ); } ); describe( 'data pipeline m -> v', () => { From f84a6a139b573fd4980d532427d8d4e6cffb4f5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 16 Sep 2019 18:32:53 +0200 Subject: [PATCH 3/4] Fix link import in manual test. --- tests/manual/todo-list.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/manual/todo-list.js b/tests/manual/todo-list.js index f9ce0a7..6158d09 100644 --- a/tests/manual/todo-list.js +++ b/tests/manual/todo-list.js @@ -11,13 +11,13 @@ import Typing from '@ckeditor/ckeditor5-typing/src/typing'; import Heading from '@ckeditor/ckeditor5-heading/src/heading'; import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold'; import Highlight from '@ckeditor/ckeditor5-highlight/src/highlight'; +import Link from '@ckeditor/ckeditor5-link/src/link'; import Table from '@ckeditor/ckeditor5-table/src/table'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import Undo from '@ckeditor/ckeditor5-undo/src/undo'; import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard'; import List from '../../src/list'; import TodoList from '../../src/todolist'; -import Link from '../../../ckeditor5-link/src/link'; ClassicEditor .create( document.querySelector( '#editor' ), { From cc87dbac4e0e6f8757198fd5decef3cc1964aff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 16 Sep 2019 19:04:13 +0200 Subject: [PATCH 4/4] Handle todo list checkbox by fixing modelToViewPosition mapping. --- src/todolistconverters.js | 53 ++------------------------------------- src/todolistediting.js | 27 +++++++++++++++++--- tests/todolistediting.js | 13 +++------- 3 files changed, 30 insertions(+), 63 deletions(-) diff --git a/src/todolistconverters.js b/src/todolistconverters.js index 07095e5..2fd1b18 100644 --- a/src/todolistconverters.js +++ b/src/todolistconverters.js @@ -59,53 +59,6 @@ export function modelViewInsertion( model, onCheckboxChecked ) { }; } -/** - * A model-to-view converter for the model `$text` element inside a to-do list item. - * - * It takes care of creating text after the {@link module:engine/view/uielement~UIElement checkbox UI element}. - * - * It is used by {@link module:engine/controller/editingcontroller~EditingController}. - * - * @see module:engine/conversion/downcastdispatcher~DowncastDispatcher#event:insert - * @param {module:utils/eventinfo~EventInfo} evt An object containing information about the fired event. - * @param {Object} data Additional information about the change. - * @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi Conversion interface. - */ -export function modelViewTextInsertion( view ) { - return ( evt, data, conversionApi ) => { - const parent = data.range.start.parent; - - if ( parent.name != 'listItem' || parent.getAttribute( 'listType' ) != 'todo' ) { - return; - } - - if ( !conversionApi.consumable.consume( data.item, 'insert' ) ) { - return; - } - - const viewWriter = conversionApi.writer; - const viewPosition = conversionApi.mapper.toViewPosition( data.range.start ); - const viewText = viewWriter.createText( data.item.data ); - - // Be sure text is created after the UIElement, so if it is a first text node inside a `listItem` element - // it has to be moved after the first node in the view list item. - // - // model: [foo] - // view:
  • ^
    • ->
  • foo
    • - const textInsertionPosition = viewPosition.offset ? viewPosition : viewPosition.getShiftedBy( 1 ); - - const label = findLabel( viewPosition.parent, view ); - - if ( label && label.parent !== viewPosition.parent && viewPosition.offset === 0 ) { - viewWriter.insert( view.createPositionAfter( label ), viewText ); - - return; - } - - viewWriter.insert( textInsertionPosition, viewText ); - }; -} - /** * A model-to-view converter for the `listItem` model element insertion. * @@ -335,14 +288,12 @@ function createCheckmarkElement( modelItem, viewWriter, isChecked, onChange ) { } // Helper method to find label element inside li. -function findLabel( viewItem, view ) { +export function findLabel( viewItem, view ) { const range = view.createRangeIn( viewItem ); - let label; for ( const value of range ) { if ( value.item.is( 'uiElement', 'label' ) ) { - label = value.item; + return value.item; } } - return label; } diff --git a/src/todolistediting.js b/src/todolistediting.js index 750f946..f6bd0a1 100644 --- a/src/todolistediting.js +++ b/src/todolistediting.js @@ -15,12 +15,12 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import { modelViewInsertion, - modelViewTextInsertion, dataModelViewInsertion, dataModelViewTextInsertion, dataViewModelCheckmarkInsertion, modelViewChangeChecked, - modelViewChangeType + modelViewChangeType, + findLabel } from './todolistconverters'; import { findInRange } from './utils'; @@ -73,7 +73,6 @@ export default class TodoListEditing extends Plugin { modelViewInsertion( model, listItem => this._handleCheckmarkChange( listItem ) ), { priority: 'high' } ); - editing.downcastDispatcher.on( 'insert:$text', modelViewTextInsertion( editing.view ), { priority: 'high' } ); data.downcastDispatcher.on( 'insert:listItem', dataModelViewInsertion( model ), { priority: 'high' } ); data.downcastDispatcher.on( 'insert:$text', dataModelViewTextInsertion, { priority: 'high' } ); @@ -86,6 +85,28 @@ export default class TodoListEditing extends Plugin { modelViewChangeChecked( listItem => this._handleCheckmarkChange( listItem ) ) ); + // Fix view position on 0 offset. + editing.mapper.on( 'modelToViewPosition', ( evt, data ) => { + const view = editing.view; + const modelPosition = data.modelPosition; + + if ( modelPosition.parent.is( 'listItem' ) && modelPosition.parent.getAttribute( 'listType' ) == 'todo' ) { + const viewLi = editing.mapper.toViewElement( data.modelPosition.parent ); + + if ( modelPosition.offset === 0 ) { + const label = findLabel( viewLi, view ); + + if ( label ) { + if ( label.nextSibling ) { + data.viewPosition = view.createPositionAt( label.nextSibling, 0 ); + } else { + data.viewPosition = view.createPositionAfter( label ); + } + } + } + } + }, { priority: 'low' } ); + data.upcastDispatcher.on( 'element:input', dataViewModelCheckmarkInsertion, { priority: 'high' } ); // Collect all view nodes that have changed and use it to check if the checkbox UI element is going to diff --git a/tests/todolistediting.js b/tests/todolistediting.js index bca8b3a..eb18912 100644 --- a/tests/todolistediting.js +++ b/tests/todolistediting.js @@ -424,9 +424,8 @@ describe( 'TodoListEditing', () => { expect( getViewData( view ) ).to.equal( '
      ' + '
    • ' + - '' + - 'b{}foo' + - '' + + '' + + 'b{}foo' + '
      • ' + '
    ' ); @@ -446,11 +445,8 @@ describe( 'TodoListEditing', () => { expect( getViewData( view ) ).to.equal( '' ); @@ -822,7 +818,6 @@ describe( 'TodoListEditing', () => { model: 'highlight', view: { classes: 'highlight' } } ); - model.change( writer => { writer.addMarker( 'element1', { range: writer.createRangeIn( writer.createPositionAt( modelRoot.getChild( 0 ), 0 ) ),