Browse Source

Merge branch 'master' into t/ckeditor5-undo/97

Maciej Gołaszewski 6 years ago
parent
commit
663fe5289c

+ 1 - 3
packages/ckeditor5-engine/.travis.yml

@@ -28,6 +28,4 @@ after_script:
 - ckeditor5-dev-tests-notify-travis-status
 env:
   global:
-  - secure: CSFKXnTb2oQW8YIjh+SWXYSYzx1jcIg1wGcl6jWrbaOU06JHEO77YLKlhMxLfRniFtilBEksK4ugIcn+lihykhHleMxI27+kifILdLLZKvwWlZiDrtpJSzLk08SpRNDwwthSlHVKrxbHVFE1K9SYpX4s3qTpxullDMPgc13OP4c=
-  - secure: WGdwDEavTOIE/yw8pdpWmx3Cehm2/GN8gZMimuot8sWPbOZRuPh+2lnVWpASLe25qJUKKeLWYLRHccpMLKbyweiordSEBwzNrOGOFyUuaDPITNZZrp5ekh2vHBp1+mwhRpRekdyWdzzVsm01RkFrnJqQlaXxX9i/r9Tu4EsK54M=
-  - secure: BKoFnOWcfKn8zaTvilMn9UORF+gR1zBqetqai5FKfSKraJXqiBrz1AI3ecytCjJOKS1g6HoRtZHwVS4a+uG3JjK3bRERQF2NyHiyjbrsL+2HaKaRnVmox8PC/59m1PJlcRKAV9RTYJqL/oqa7N8OBegt4LpIVnEcil8WCErCF6Q=
+  - secure: GMesvAe0tAa8QfdhGApzmzjhf9mpfjL6jMx8aFqpRli5tuyX9JsjNjFELnU/MMlNDyySPwXAuQVrvcWkCFy9a3n705tGt/VdmStCrTdwV2oRNmNKqGgFovmiK3an+RAIts2PX2iguDjwiogqkAW0FstWAMdG0Yk4AVUxKC1niIo=

+ 11 - 0
packages/ckeditor5-engine/CHANGELOG.md

@@ -1,6 +1,17 @@
 Changelog
 =========
 
+## [13.1.1](https://github.com/ckeditor/ckeditor5-engine/compare/v13.1.0...v13.1.1) (2019-06-05)
+
+### Bug fixes
+
+* Prevented from losing selection attributes between operations (fixes a bug with text composition). Closes https://github.com/ckeditor/ckeditor5-typing/issues/188. ([42dcb25](https://github.com/ckeditor/ckeditor5-engine/commit/42dcb25))
+
+### Other changes
+
+* Added more cases of affected markers on merging in `model.Writer`. Closes [#1738](https://github.com/ckeditor/ckeditor5-engine/issues/1738). ([01ff6e6](https://github.com/ckeditor/ckeditor5-engine/commit/01ff6e6))
+
+
 ## [13.1.0](https://github.com/ckeditor/ckeditor5-engine/compare/v13.0.0...v13.1.0) (2019-04-10)
 
 ### Features

+ 1 - 2
packages/ckeditor5-engine/README.md

@@ -4,7 +4,6 @@ CKEditor 5 editing engine
 [![Join the chat at https://gitter.im/ckeditor/ckeditor5](https://badges.gitter.im/ckeditor/ckeditor5.svg)](https://gitter.im/ckeditor/ckeditor5?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
 [![npm version](https://badge.fury.io/js/%40ckeditor%2Fckeditor5-engine.svg)](https://www.npmjs.com/package/@ckeditor/ckeditor5-engine)
 [![Build Status](https://travis-ci.org/ckeditor/ckeditor5-engine.svg?branch=master)](https://travis-ci.org/ckeditor/ckeditor5-engine)
-[![BrowserStack Status](https://automate.browserstack.com/automate/badge.svg?badge_key=d3hvenZqQVZERFQ5d09FWXdyT0ozVXhLaVltRFRjTTUyZGpvQWNmWVhUUT0tLUZqNlJ1YWRUd0RvdEVOaEptM1B2Q0E9PQ==--c9d3dee40b9b4471ff3fb516d9ecf8d09292c7e0)](https://automate.browserstack.com/public-build/d3hvenZqQVZERFQ5d09FWXdyT0ozVXhLaVltRFRjTTUyZGpvQWNmWVhUUT0tLUZqNlJ1YWRUd0RvdEVOaEptM1B2Q0E9PQ==--c9d3dee40b9b4471ff3fb516d9ecf8d09292c7e0)
 [![Coverage Status](https://coveralls.io/repos/github/ckeditor/ckeditor5-engine/badge.svg?branch=master)](https://coveralls.io/github/ckeditor/ckeditor5-engine?branch=master)
 <br>
 [![Dependency Status](https://david-dm.org/ckeditor/ckeditor5-engine/status.svg)](https://david-dm.org/ckeditor/ckeditor5-engine)
@@ -32,4 +31,4 @@ Additionally, refer to the [`@ckeditor/ckeditor5-engine` package](https://ckedit
 
 ## License
 
-Licensed under the terms of [GNU General Public License Version 2 or later](http://www.gnu.org/licenses/gpl.html). For full details about the license, please check the `LICENSE.md` file.
+Licensed under the terms of [GNU General Public License Version 2 or later](http://www.gnu.org/licenses/gpl.html). For full details about the license, please check the `LICENSE.md` file or [https://ckeditor.com/legal/ckeditor-oss-license](https://ckeditor.com/legal/ckeditor-oss-license).

+ 16 - 16
packages/ckeditor5-engine/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@ckeditor/ckeditor5-engine",
-  "version": "13.1.0",
+  "version": "13.1.1",
   "description": "The editing engine of CKEditor 5 – the best browser-based rich text editor.",
   "keywords": [
     "wysiwyg",
@@ -21,24 +21,24 @@
     "ckeditor 5"
   ],
   "dependencies": {
-    "@ckeditor/ckeditor5-utils": "^12.1.0",
+    "@ckeditor/ckeditor5-utils": "^12.1.1",
     "lodash-es": "^4.17.10"
   },
   "devDependencies": {
-    "@ckeditor/ckeditor5-basic-styles": "^11.1.0",
-    "@ckeditor/ckeditor5-block-quote": "^11.0.1",
-    "@ckeditor/ckeditor5-core": "^12.1.0",
-    "@ckeditor/ckeditor5-editor-classic": "^12.1.0",
-    "@ckeditor/ckeditor5-enter": "^11.0.1",
-    "@ckeditor/ckeditor5-essentials": "^11.0.1",
-    "@ckeditor/ckeditor5-heading": "^11.0.1",
-    "@ckeditor/ckeditor5-link": "^11.0.1",
-    "@ckeditor/ckeditor5-list": "^12.0.1",
-    "@ckeditor/ckeditor5-paragraph": "^11.0.1",
-    "@ckeditor/ckeditor5-theme-lark": "^13.0.1",
-    "@ckeditor/ckeditor5-typing": "^12.0.1",
-    "@ckeditor/ckeditor5-undo": "^11.0.1",
-    "@ckeditor/ckeditor5-widget": "^11.0.1",
+    "@ckeditor/ckeditor5-basic-styles": "^11.1.1",
+    "@ckeditor/ckeditor5-block-quote": "^11.1.0",
+    "@ckeditor/ckeditor5-core": "^12.1.1",
+    "@ckeditor/ckeditor5-editor-classic": "^12.1.1",
+    "@ckeditor/ckeditor5-enter": "^11.0.2",
+    "@ckeditor/ckeditor5-essentials": "^11.0.2",
+    "@ckeditor/ckeditor5-heading": "^11.0.2",
+    "@ckeditor/ckeditor5-link": "^11.0.2",
+    "@ckeditor/ckeditor5-list": "^12.0.2",
+    "@ckeditor/ckeditor5-paragraph": "^11.0.2",
+    "@ckeditor/ckeditor5-theme-lark": "^14.0.0",
+    "@ckeditor/ckeditor5-typing": "^12.0.2",
+    "@ckeditor/ckeditor5-undo": "^11.0.2",
+    "@ckeditor/ckeditor5-widget": "^11.0.2",
     "eslint": "^5.5.0",
     "eslint-config-ckeditor5": "^1.0.11",
     "husky": "^1.3.1",

+ 1 - 0
packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -297,6 +297,7 @@ export default class DataController {
 	 */
 	set( data ) {
 		let newData = {};
+
 		if ( typeof data === 'string' ) {
 			newData.main = data; // Default root is 'main'. To set data on a different root, object should be passed.
 		} else {

+ 8 - 1
packages/ckeditor5-engine/src/model/utils/insertcontent.js

@@ -561,8 +561,15 @@ class Insertion {
 				this.position = this.writer.createPositionBefore( parent );
 
 				// Special case – parent is empty (<p>^</p>).
+				//
+				// 1. parent.isEmpty
 				// We can remove the element after moving insertion position out of it.
-				if ( parent.isEmpty ) {
+				//
+				// 2. parent.parent === allowedIn
+				// However parent should remain in place when allowed element is above limit element in document tree.
+				// For example there shouldn't be allowed to remove empty paragraph from tableCell, when is pasted
+				// content allowed in $root.
+				if ( parent.isEmpty && parent.parent === allowedIn ) {
 					this.writer.remove( parent );
 				}
 			} else if ( this.position.isAtEnd ) {

+ 42 - 7
packages/ckeditor5-engine/src/model/writer.js

@@ -177,8 +177,16 @@ export default class Writer {
 			// If it isn't the same root.
 			else {
 				if ( item.root.document ) {
-					// It is forbidden to move a node that was already in a document outside of it.
-					throw new Error( 'model-writer-insert-forbidden-move: Cannot move a node from a document to a different tree.' );
+					/**
+					 * Cannot move a node from a document to a different tree.
+					 * It is forbidden to move a node that was already in a document outside of it.
+					 *
+					 * @error model-writer-insert-forbidden-move
+					 */
+					throw new CKEditorError(
+						'model-writer-insert-forbidden-move: ' +
+						'Cannot move a node from a document to a different tree. ' +
+						'It is forbidden to move a node that was already in a document outside of it.' );
 				} else {
 					// Move between two different document fragments or from document fragment to a document is possible.
 					// In that case, remove the item from it's original parent.
@@ -208,7 +216,13 @@ export default class Writer {
 					markerRange.end._getCombined( rangeRootPosition, position )
 				);
 
-				this.addMarker( markerName, { range, usingOperation: true, affectsData: true } );
+				const options = { range, usingOperation: true, affectsData: true };
+
+				if ( this.model.markers.has( markerName ) ) {
+					this.updateMarker( markerName, options );
+				} else {
+					this.addMarker( markerName, options );
+				}
 			}
 		}
 	}
@@ -1349,10 +1363,31 @@ export default class Writer {
 				const elementBefore = positionOrRange.nodeBefore;
 				const elementAfter = positionOrRange.nodeAfter;
 
-				const affectedOnLeft = markerRange.start.parent == elementBefore && markerRange.start.isAtEnd;
-				const affectedOnRight = markerRange.end.parent == elementAfter && markerRange.end.offset == 0;
-
-				isAffected = affectedOnLeft || affectedOnRight;
+				//               Start:  <p>Foo[</p><p>Bar]</p>
+				//         After merge:  <p>Foo[Bar]</p>
+				// After undoing split:  <p>Foo</p><p>[Bar]</p>     <-- incorrect, needs remembering for undo.
+				//
+				const affectedInLeftElement = markerRange.start.parent == elementBefore && markerRange.start.isAtEnd;
+
+				//               Start:  <p>[Foo</p><p>]Bar</p>
+				//         After merge:  <p>[Foo]Bar</p>
+				// After undoing split:  <p>[Foo]</p><p>Bar</p>     <-- incorrect, needs remembering for undo.
+				//
+				const affectedInRightElement = markerRange.end.parent == elementAfter && markerRange.end.offset == 0;
+
+				//               Start:  <p>[Foo</p>]<p>Bar</p>
+				//         After merge:  <p>[Foo]Bar</p>
+				// After undoing split:  <p>[Foo]</p><p>Bar</p>     <-- incorrect, needs remembering for undo.
+				//
+				const affectedAfterLeftElement = markerRange.end.nodeAfter == elementAfter;
+
+				//               Start:  <p>Foo</p>[<p>Bar]</p>
+				//         After merge:  <p>Foo[Bar]</p>
+				// After undoing split:  <p>Foo</p><p>[Bar]</p>     <-- incorrect, needs remembering for undo.
+				//
+				const affectedBeforeRightElement = markerRange.start.nodeAfter == elementAfter;
+
+				isAffected = affectedInLeftElement || affectedInRightElement || affectedAfterLeftElement || affectedBeforeRightElement;
 			}
 
 			if ( isAffected ) {

+ 18 - 0
packages/ckeditor5-engine/tests/controller/datacontroller.js

@@ -336,6 +336,24 @@ describe( 'DataController', () => {
 
 			expect( getData( model, { withoutSelection: true } ) ).to.equal( 'foo' );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5-engine/issues/1721.
+		it( 'should not throw when setting the data with markers that already exist in the editor', () => {
+			schema.extend( '$text', { allowIn: '$root' } );
+
+			data.set( 'foo' );
+
+			downcastHelpers.markerToElement( { model: 'marker', view: 'marker' } );
+			upcastHelpers.elementToMarker( { view: 'marker', model: 'marker' } );
+
+			model.change( writer => {
+				writer.addMarker( 'marker', { range: writer.createRangeIn( modelDocument.getRoot() ), usingOperation: true } );
+			} );
+
+			expect( () => {
+				data.set( data.get() );
+			} ).not.to.throw();
+		} );
 	} );
 
 	describe( 'get()', () => {

+ 37 - 0
packages/ckeditor5-engine/tests/model/utils/insertcontent.js

@@ -1254,6 +1254,43 @@ describe( 'DataController utils', () => {
 			expect( getData( model ) ).to.equal( '<limit>ab[]foo</limit>' );
 			expect( stringify( root, affectedRange ) ).to.equal( '<limit>[ab]foo</limit>' );
 		} );
+
+		describe( 'when allowed element is above limit element in document tree', () => {
+			// $root > table > tableRow > tableCell > paragraph
+			// After inserting new table ( allowed in root ), empty paragraph shouldn't be removed from current tableCell.
+			beforeEach( () => {
+				const schema = model.schema;
+
+				schema.register( 'wrapper', {
+					isLimit: true,
+					isBlock: true,
+					isObject: true,
+					allowWhere: '$block'
+				} );
+
+				schema.extend( 'paragraph', { allowIn: 'limit' } );
+				schema.extend( 'limit', { allowIn: 'wrapper' } );
+			} );
+
+			it( 'should not remove empty elements when not-allowed element is paste', () => {
+				setData( model, '<wrapper><limit><paragraph>[]</paragraph></limit></wrapper>' );
+
+				// pasted content is forbidden in current selection
+				const affectedRange = insertHelper( '<wrapper><limit><paragraph>foo</paragraph></limit></wrapper>' );
+
+				expect( getData( model ) ).to.equal( '<wrapper><limit>[]<paragraph></paragraph></limit></wrapper>' );
+				expect( stringify( root, affectedRange ) ).to.equal( '<wrapper><limit><paragraph>[]</paragraph></limit></wrapper>' );
+			} );
+
+			it( 'should correctly paste allowed nodes', () => {
+				setData( model, '<wrapper><limit><paragraph>[]</paragraph></limit></wrapper>' );
+
+				const affectedRange = insertHelper( '<paragraph>foo</paragraph>' );
+
+				expect( getData( model ) ).to.equal( '<wrapper><limit><paragraph>foo</paragraph>[]</limit></wrapper>' );
+				expect( stringify( root, affectedRange ) ).to.equal( '<wrapper><limit>[<paragraph>foo</paragraph>]</limit></wrapper>' );
+			} );
+		} );
 	} );
 
 	// Helper function that parses given content and inserts it at the cursor position.

+ 72 - 18
packages/ckeditor5-engine/tests/model/writer.js

@@ -302,15 +302,15 @@ describe( 'Writer', () => {
 
 			expect( () => {
 				insert( node, docFrag );
-			} ).to.throw();
+			} ).to.throw( CKEditorError, /^model-writer-insert-forbidden-move/ );
 		} );
 
 		it( 'should transfer markers from given DocumentFragment', () => {
 			const root = doc.createRoot();
-
 			const docFrag = createDocumentFragment();
 
 			appendText( 'abcd', root );
+
 			appendElement( 'p', docFrag );
 			insertText( 'foo bar', new Position( docFrag, [ 0, 0 ] ) );
 
@@ -331,6 +331,39 @@ describe( 'Writer', () => {
 			expect( modelMarker.affectsData ).to.be.true;
 		} );
 
+		// https://github.com/ckeditor/ckeditor5-engine/issues/1721.
+		it( 'should update a marker if DocumentFragment has a marker that is already in the model (markers have the same name)', () => {
+			const root = doc.createRoot();
+			const docFrag = createDocumentFragment();
+
+			// <root><p></p><p>[ab]cd</p></root>.
+			appendText( 'abcd', root );
+
+			// <docFrag><p>f[oo b]ar</p></docFrag>.
+			appendElement( 'p', docFrag );
+			insertText( 'foo bar', new Position( docFrag, [ 0, 0 ] ) );
+
+			model.change( writer => {
+				const range = new Range( new Position( root, [ 1, 0 ] ), new Position( root, [ 1, 2 ] ) );
+
+				writer.addMarker( 'marker', { range, usingOperation: true } );
+			} );
+
+			docFrag.markers.set( 'marker', new Range( new Position( docFrag, [ 0, 1 ] ), new Position( docFrag, [ 0, 5 ] ) ) );
+
+			insert( docFrag, new Position( root, [ 2 ] ) );
+
+			expect( Array.from( model.markers ).length ).to.equal( 1 );
+
+			const modelMarker = model.markers.get( 'marker' );
+			const range = modelMarker.getRange();
+			expect( range.root ).to.equal( root );
+			expect( range.start.path ).to.deep.equal( [ 2, 1 ] );
+			expect( range.end.path ).to.deep.equal( [ 2, 5 ] );
+			expect( modelMarker.managedUsingOperations ).to.be.true;
+			expect( modelMarker.affectsData ).to.be.true;
+		} );
+
 		it( 'should throw when trying to use detached writer', () => {
 			const writer = new Writer( model, batch );
 			const root = doc.createRoot();
@@ -716,7 +749,7 @@ describe( 'Writer', () => {
 
 			expect( () => {
 				append( node, docFrag );
-			} ).to.throw();
+			} ).to.throw( CKEditorError, /^model-writer-insert-forbidden-move/ );
 		} );
 	} );
 
@@ -1425,29 +1458,50 @@ describe( 'Writer', () => {
 			expect( docFrag.getChild( 0 ).getChild( 0 ).data ).to.equal( 'foobar' );
 		} );
 
-		it( 'should create a marker operation if a marker was affected', () => {
-			const markerRange = new Range( Position._createAt( p2, 0 ), Position._createAt( p2, 0 ) );
+		describe( 'should create a marker operation if a marker was affected', () => {
+			it( '<p>Foo[</p><p>Bar]</p>', () => {
+				test( p1, 'end', p2, 0 );
+			} );
 
-			addMarker( 'name', {
-				range: markerRange,
-				usingOperation: true
+			it( '<p>[Foo</p><p>]Bar</p>', () => {
+				test( p1, 0, p2, 0 );
 			} );
 
-			const documentVersion = model.document.version;
+			it( '<p>[Foo</p>]<p>Bar</p>', () => {
+				test( p1, 0, root, 1 );
+			} );
 
-			merge( Position._createAfter( p1 ) );
+			it( '<p>Foo</p>[<p>Bar]</p>', () => {
+				test( root, 1, p2, 'end' );
+			} );
 
-			const history = model.document.history;
+			function test( startElement, startOffset, endElement, endOffset ) {
+				const markerRange = new Range(
+					Position._createAt( startElement, startOffset ),
+					Position._createAt( endElement, endOffset )
+				);
 
-			const lastOperation = history._operations[ history._operations.length - 1 ];
-			const secondLastOperation = history._operations[ history._operations.length - 2 ];
+				addMarker( 'name', {
+					range: markerRange,
+					usingOperation: true
+				} );
 
-			expect( secondLastOperation.type ).to.equal( 'marker' );
-			expect( secondLastOperation.oldRange.isEqual( markerRange ) );
-			expect( secondLastOperation.newRange.isEqual( markerRange ) );
+				const documentVersion = model.document.version;
 
-			expect( lastOperation.type ).to.equal( 'merge' );
-			expect( model.document.version ).to.equal( documentVersion + 2 );
+				merge( Position._createAfter( p1 ) );
+
+				const history = model.document.history;
+
+				const lastOperation = history._operations[ history._operations.length - 1 ];
+				const secondLastOperation = history._operations[ history._operations.length - 2 ];
+
+				expect( secondLastOperation.type ).to.equal( 'marker' );
+				expect( secondLastOperation.oldRange.isEqual( markerRange ) );
+				expect( secondLastOperation.newRange.isEqual( markerRange ) );
+
+				expect( lastOperation.type ).to.equal( 'merge' );
+				expect( model.document.version ).to.equal( documentVersion + 2 );
+			}
 		} );
 
 		it( 'should not create a marker operation if affected marker was not using operations', () => {