From 63a917dbd3fad9a8ca34f0afbb69a713597c5264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 26 Jun 2017 11:11:33 +0200 Subject: [PATCH 1/3] Live selection should properly set its properties in case of non-collapsed default range. --- src/model/liveselection.js | 4 +- tests/model/document/document.js | 131 ++++++++++++++++++++++++------- tests/model/liveselection.js | 80 ++++++++++++++++--- tests/tickets/699.js | 56 +++++++++++++ 4 files changed, 230 insertions(+), 41 deletions(-) create mode 100644 tests/tickets/699.js diff --git a/src/model/liveselection.js b/src/model/liveselection.js index 538d935cf..51332fff4 100644 --- a/src/model/liveselection.js +++ b/src/model/liveselection.js @@ -88,7 +88,7 @@ export default class LiveSelection extends Selection { get isCollapsed() { const length = this._ranges.length; - return length === 0 ? true : super.isCollapsed; + return length === 0 ? this._document._getDefaultRange().isCollapsed : super.isCollapsed; } /** @@ -102,7 +102,7 @@ export default class LiveSelection extends Selection { * @inheritDoc */ get focus() { - return super.focus || this._document._getDefaultRange().start; + return super.focus || this._document._getDefaultRange().end; } /** diff --git a/tests/model/document/document.js b/tests/model/document/document.js index 4f623ea36..de0b4dcd6 100644 --- a/tests/model/document/document.js +++ b/tests/model/document/document.js @@ -280,12 +280,18 @@ describe( 'Document', () => { beforeEach( () => { doc.schema.registerItem( 'paragraph', '$block' ); + doc.schema.registerItem( 'emptyBlock' ); doc.schema.allow( { name: 'emptyBlock', inside: '$root' } ); - doc.schema.registerItem( 'widget', '$block' ); + + doc.schema.registerItem( 'widget' ); doc.schema.allow( { name: 'widget', inside: '$root' } ); doc.schema.objects.add( 'widget' ); + doc.schema.registerItem( 'blockWidget', '$block' ); + doc.schema.allow( { name: 'blockWidget', inside: '$root' } ); + doc.schema.objects.add( 'blockWidget' ); + doc.createRoot(); selection = doc.selection; } ); @@ -373,34 +379,6 @@ describe( 'Document', () => { null ); - test( - 'should select nearest object - both', - '[]', - 'both', - '[]' - ); - - test( - 'should select nearest object - forward', - '[]', - 'forward', - '[]' - ); - - test( - 'should select nearest object - forward', - '[]', - 'forward', - '[]' - ); - - test( - 'should select nearest object - backward', - '[]', - 'backward', - '[]' - ); - test( 'should move forward when placed at root start', '[]', @@ -415,6 +393,101 @@ describe( 'Document', () => { '[]' ); + describe( 'in case of objects which do not allow text inside', () => { + test( + 'should select nearest object (o[]o) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object (o[]o) - forward', + '[]', + 'forward', + '[]' + ); + + test( + 'should select nearest object (o[]o) - backward', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object (p[]o) - forward', + '[]', + 'forward', + '[]' + ); + + test( + 'should select nearest object (o[]p) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object (o[]p) - backward', + '[]', + 'backward', + '[]' + ); + + test( + 'should select nearest object ([]o) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object ([]o) - forward', + '[]', + 'forward', + '[]' + ); + + test( + 'should select nearest object (o[]) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object (o[]) - backward', + '[]', + 'both', + '[]' + ); + } ); + + describe( 'in case of objects which allow text inside', () => { + test( + 'should select nearest object which allows text (o[]o) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object (o[]p) - both', + '[]', + 'both', + '[]' + ); + + test( + 'should select nearest object which allows text ([]o) - both', + '[]', + 'both', + '[]' + ); + } ); + function test( testName, data, direction, expected ) { it( testName, () => { setData( doc, data ); diff --git a/tests/model/liveselection.js b/tests/model/liveselection.js index 5cd35c675..02461a364 100644 --- a/tests/model/liveselection.js +++ b/tests/model/liveselection.js @@ -43,6 +43,9 @@ describe( 'LiveSelection', () => { selection = doc.selection; doc.schema.registerItem( 'p', '$block' ); + doc.schema.registerItem( 'widget' ); + doc.schema.objects.add( 'widget' ); + liveRange = new LiveRange( new Position( root, [ 0 ] ), new Position( root, [ 1 ] ) ); range = new Range( new Position( root, [ 2 ] ), new Position( root, [ 2, 2 ] ) ); } ); @@ -99,9 +102,66 @@ describe( 'LiveSelection', () => { } ); describe( 'isCollapsed', () => { - it( 'should return true for default range', () => { + it( 'should be true for the default range (in text position)', () => { expect( selection.isCollapsed ).to.be.true; } ); + + it( 'should be false for the default range (object selection) ', () => { + root.insertChildren( 0, new Element( 'widget' ) ); + + expect( selection.isCollapsed ).to.be.false; + } ); + + it( 'should back off to the default algorithm if selection has ranges', () => { + selection.addRange( range ); + + expect( selection.isCollapsed ).to.be.false; + } ); + } ); + + describe( 'anchor', () => { + it( 'should equal the default range\'s start (in text position)', () => { + const expectedPos = new Position( root, [ 0, 0 ] ); + + expect( selection.anchor.isEqual( expectedPos ) ).to.be.true; + } ); + + it( 'should equal the default range\'s start (object selection)', () => { + root.insertChildren( 0, new Element( 'widget' ) ); + + const expectedPos = new Position( root, [ 0 ] ); + + expect( selection.anchor.isEqual( expectedPos ) ).to.be.true; + } ); + + it( 'should back off to the default algorithm if selection has ranges', () => { + selection.addRange( range ); + + expect( selection.anchor.isEqual( range.start ) ).to.be.true; + } ); + } ); + + describe( 'focus', () => { + it( 'should equal the default range\'s end (in text position)', () => { + const expectedPos = new Position( root, [ 0, 0 ] ); + + expect( selection.focus.isEqual( expectedPos ) ).to.be.true; + } ); + + it( 'should equal the default range\'s end (object selection)', () => { + root.insertChildren( 0, new Element( 'widget' ) ); + + const expectedPos = new Position( root, [ 1 ] ); + + expect( selection.focus.isEqual( expectedPos ) ).to.be.true; + expect( selection.focus.isEqual( selection.getFirstRange().end ) ).to.be.true; + } ); + + it( 'should back off to the default algorithm if selection has ranges', () => { + selection.addRange( range ); + + expect( selection.focus.isEqual( range.end ) ).to.be.true; + } ); } ); describe( 'rangeCount', () => { @@ -118,7 +178,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'addRange', () => { + describe( 'addRange()', () => { it( 'should convert added Range to LiveRange', () => { selection.addRange( range ); @@ -149,7 +209,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'collapse', () => { + describe( 'collapse()', () => { it( 'detaches all existing ranges', () => { selection.addRange( range ); selection.addRange( liveRange ); @@ -161,7 +221,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'destroy', () => { + describe( 'destroy()', () => { it( 'should unbind all events', () => { selection.addRange( liveRange ); selection.addRange( range ); @@ -181,7 +241,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'setFocus', () => { + describe( 'setFocus()', () => { it( 'modifies default range', () => { const startPos = selection.getFirstPosition(); const endPos = Position.createAt( root, 'end' ); @@ -206,7 +266,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'removeAllRanges', () => { + describe( 'removeAllRanges()', () => { let spy, ranges; beforeEach( () => { @@ -249,7 +309,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'setRanges', () => { + describe( 'setRanges()', () => { it( 'should throw an error when range is invalid', () => { expect( () => { selection.setRanges( [ { invalid: 'range' } ] ); @@ -295,7 +355,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'getFirstRange', () => { + describe( 'getFirstRange()', () => { it( 'should return default range if no ranges were added', () => { const firstRange = selection.getFirstRange(); @@ -304,7 +364,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'getLastRange', () => { + describe( 'getLastRange()', () => { it( 'should return default range if no ranges were added', () => { const lastRange = selection.getLastRange(); @@ -313,7 +373,7 @@ describe( 'LiveSelection', () => { } ); } ); - describe( 'createFromSelection', () => { + describe( 'createFromSelection()', () => { it( 'should return a LiveSelection instance', () => { selection.addRange( range, true ); diff --git a/tests/tickets/699.js b/tests/tickets/699.js new file mode 100644 index 000000000..085a9fd1e --- /dev/null +++ b/tests/tickets/699.js @@ -0,0 +1,56 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/* globals document */ + +import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; + +import buildViewConverter from '../../src/conversion/buildviewconverter'; +import buildModelConverter from '../../src/conversion/buildmodelconverter'; + +import { getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + +describe( 'Bug ckeditor5-engine#699', () => { + let element; + + beforeEach( () => { + element = document.createElement( 'div' ); + + document.body.appendChild( element ); + } ); + + afterEach( () => { + element.remove(); + } ); + + it( 'the engine sets the initial selection on the first widget', () => { + return ClassicTestEditor + .create( element, { plugins: [ Paragraph, WidgetPlugin ] } ) + .then( editor => { + editor.setData( '

foo

' ); + + expect( getData( editor.document ) ).to.equal( '[]foo' ); + + return editor.destroy(); + } ); + } ); +} ); + +function WidgetPlugin( editor ) { + const schema = editor.document.schema; + + schema.registerItem( 'widget' ); + schema.allow( { name: 'widget', inside: '$root' } ); + schema.objects.add( 'widget' ); + + buildModelConverter().for( editor.data.modelToView, editor.editing.modelToView ) + .fromElement( 'widget' ) + .toElement( 'widget' ); + + buildViewConverter().for( editor.data.viewToModel ) + .fromElement( 'widget' ) + .toElement( 'widget' ); +} From 9b0cb5c0f05060ca50a51d1d53719513d4408ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 26 Jun 2017 11:15:43 +0200 Subject: [PATCH 2/3] Added direct view check even though the model check did verify the issue too (because dev-utils use model->view conversion :D). --- tests/tickets/699.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/tickets/699.js b/tests/tickets/699.js index 085a9fd1e..4221ed3eb 100644 --- a/tests/tickets/699.js +++ b/tests/tickets/699.js @@ -11,7 +11,8 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import buildViewConverter from '../../src/conversion/buildviewconverter'; import buildModelConverter from '../../src/conversion/buildmodelconverter'; -import { getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import { getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; describe( 'Bug ckeditor5-engine#699', () => { let element; @@ -32,7 +33,8 @@ describe( 'Bug ckeditor5-engine#699', () => { .then( editor => { editor.setData( '

foo

' ); - expect( getData( editor.document ) ).to.equal( '[]foo' ); + expect( getModelData( editor.document ) ).to.equal( '[]foo' ); + expect( getViewData( editor.editing.view ) ).to.equal( '[]

foo

' ); return editor.destroy(); } ); From 5abc5837cb768e88d2adf22bd90f1dbb89029b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 26 Jun 2017 11:19:55 +0200 Subject: [PATCH 3/3] Fixed import paths in tests. --- tests/tickets/699.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tickets/699.js b/tests/tickets/699.js index 4221ed3eb..3ffac4ef6 100644 --- a/tests/tickets/699.js +++ b/tests/tickets/699.js @@ -11,8 +11,8 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import buildViewConverter from '../../src/conversion/buildviewconverter'; import buildModelConverter from '../../src/conversion/buildmodelconverter'; -import { getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; -import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; +import { getData as getModelData } from '../../src/dev-utils/model'; +import { getData as getViewData } from '../../src/dev-utils/view'; describe( 'Bug ckeditor5-engine#699', () => { let element;