浏览代码

Merge branch 'master' into t/ckeditor5/1096

Maciej Gołaszewski 6 年之前
父节点
当前提交
9a76f44339

+ 8 - 4
packages/ckeditor5-engine/package.json

@@ -40,7 +40,7 @@
     "@ckeditor/ckeditor5-widget": "^10.3.1",
     "eslint": "^5.5.0",
     "eslint-config-ckeditor5": "^1.0.9",
-    "husky": "^0.14.3",
+    "husky": "^1.3.1",
     "lint-staged": "^7.0.0"
   },
   "engines": {
@@ -61,8 +61,7 @@
     "theme"
   ],
   "scripts": {
-    "lint": "eslint --quiet '**/*.js'",
-    "precommit": "lint-staged"
+    "lint": "eslint --quiet '**/*.js'"
   },
   "lint-staged": {
     "**/*.js": [
@@ -72,5 +71,10 @@
   "eslintIgnore": [
     "src/lib/**",
     "packages/**"
-  ]
+  ],
+  "husky": {
+    "hooks": {
+      "pre-commit": "lint-staged"
+    }
+  }
 }

+ 4 - 2
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -79,9 +79,11 @@ export default class EditingController {
 			this.view._renderingDisabled = true;
 		}, { priority: 'highest' } );
 
-		this.listenTo( this.model, '_afterChanges', () => {
+		this.listenTo( this.model, '_afterChanges', ( evt, { hasModelDocumentChanged } ) => {
 			this.view._renderingDisabled = false;
-			this.view.render();
+			if ( hasModelDocumentChanged ) {
+				this.view.render();
+			}
 		}, { priority: 'lowest' } );
 
 		// Whenever model document is changed, convert those changes to the view (using model.Document#differ).

+ 17 - 0
packages/ckeditor5-engine/src/model/differ.js

@@ -293,6 +293,23 @@ export default class Differ {
 	}
 
 	/**
+	 * Returns all markers which changed.
+	 *
+	 * @returns {Array.<Object>}
+	 */
+	getChangedMarkers() {
+		return Array.from( this._changedMarkers ).map( item => (
+			{
+				name: item[ 0 ],
+				data: {
+					oldRange: item[ 1 ].oldRange,
+					newRange: item[ 1 ].newRange
+				}
+			}
+		) );
+	}
+
+	/**
 	 * Checks whether some of the buffered changes affect the editor data.
 	 *
 	 * Types of changes which affect the editor data:

+ 55 - 29
packages/ckeditor5-engine/src/model/document.js

@@ -46,7 +46,7 @@ export default class Document {
 		 * The {@link module:engine/model/model~Model model} that the document is a part of.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/model~Model}
+		 * @type {module:engine/model/model~Model}
 		 */
 		this.model = model;
 
@@ -58,7 +58,7 @@ export default class Document {
 		 * a {@link module:utils/ckeditorerror~CKEditorError model-document-applyOperation-wrong-version} error is thrown.
 		 *
 		 * @readonly
-		 * @member {Number}
+		 * @type {Number}
 		 */
 		this.version = 0;
 
@@ -66,7 +66,7 @@ export default class Document {
 		 * The document's history.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/history~History}
+		 * @type {module:engine/model/history~History}
 		 */
 		this.history = new History( this );
 
@@ -74,7 +74,7 @@ export default class Document {
 		 * The selection in this document.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/documentselection~DocumentSelection}
+		 * @type {module:engine/model/documentselection~DocumentSelection}
 		 */
 		this.selection = new DocumentSelection( this );
 
@@ -83,7 +83,7 @@ export default class Document {
 		 * {@link #getRoot} to manipulate it.
 		 *
 		 * @readonly
-		 * @member {module:utils/collection~Collection}
+		 * @type {module:utils/collection~Collection}
 		 */
 		this.roots = new Collection( { idProperty: 'rootName' } );
 
@@ -91,7 +91,7 @@ export default class Document {
 		 * The model differ object. Its role is to buffer changes done on the model document and then calculate a diff of those changes.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/differ~Differ}
+		 * @type {module:engine/model/differ~Differ}
 		 */
 		this.differ = new Differ( model.markers );
 
@@ -99,10 +99,18 @@ export default class Document {
 		 * Post-fixer callbacks registered to the model document.
 		 *
 		 * @private
-		 * @member {Set}
+		 * @type {Set.<Function>}
 		 */
 		this._postFixers = new Set();
 
+		/**
+		 * A boolean indicates whether the selection has changed until
+		 *
+		 * @private
+		 * @type {Boolean}
+		 */
+		this._hasSelectionChangedFromTheLastChangeBlock = false;
+
 		// Graveyard tree root. Document always have a graveyard root, which stores removed nodes.
 		this.createRoot( '$root', graveyardName );
 
@@ -144,29 +152,8 @@ export default class Document {
 		}, { priority: 'low' } );
 
 		// Listen to selection changes. If selection changed, mark it.
-		let hasSelectionChanged = false;
-
 		this.listenTo( this.selection, 'change', () => {
-			hasSelectionChanged = true;
-		} );
-
-		// Wait for `_change` event from model, which signalizes that outermost change block has finished.
-		// When this happens, check if there were any changes done on document, and if so, call post-fixers,
-		// fire `change` event for features and conversion and then reset the differ.
-		// Fire `change:data` event when at least one operation or buffered marker changes the data.
-		this.listenTo( model, '_change', ( evt, writer ) => {
-			if ( !this.differ.isEmpty || hasSelectionChanged ) {
-				this._callPostFixers( writer );
-
-				if ( this.differ.hasDataChanges() ) {
-					this.fire( 'change:data', writer.batch );
-				} else {
-					this.fire( 'change', writer.batch );
-				}
-
-				this.differ.reset();
-				hasSelectionChanged = false;
-			}
+			this._hasSelectionChangedFromTheLastChangeBlock = true;
 		} );
 
 		// Buffer marker changes.
@@ -307,6 +294,44 @@ export default class Document {
 	}
 
 	/**
+	 * Check if there were any changes done on document, and if so, call post-fixers,
+	 * fire `change` event for features and conversion and then reset the differ.
+	 * Fire `change:data` event when at least one operation or buffered marker changes the data.
+	 *
+	 * @protected
+	 * @fires change
+	 * @fires change:data
+	 * @param {module:engine/model/writer~Writer} writer The writer on which post-fixers will be called.
+	 */
+	_handleChangeBlock( writer ) {
+		if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
+			this._callPostFixers( writer );
+
+			if ( this.differ.hasDataChanges() ) {
+				this.fire( 'change:data', writer.batch );
+			} else {
+				this.fire( 'change', writer.batch );
+			}
+
+			this.differ.reset();
+		}
+
+		this._hasSelectionChangedFromTheLastChangeBlock = false;
+	}
+
+	/**
+	 * Returns whether there is a buffered change or if the selection has changed from the last
+	 * {@link module:engine/model/model~Model#enqueueChange `enqueueChange()` block}
+	 * or {@link module:engine/model/model~Model#change `change()` block}.
+	 *
+	 * @protected
+	 * @returns {Boolean} Returns `true` if document has changed from the last `change()` or `enqueueChange()` block.
+	 */
+	_hasDocumentChangedFromTheLastChangeBlock() {
+		return !this.differ.isEmpty || this._hasSelectionChangedFromTheLastChangeBlock;
+	}
+
+	/**
 	 * Returns the default root for this document which is either the first root that was added to the document using
 	 * {@link #createRoot} or the {@link #graveyard graveyard root} if no other roots were created.
 	 *
@@ -359,6 +384,7 @@ export default class Document {
 	 * Performs post-fixer loops. Executes post-fixer callbacks as long as none of them has done any changes to the model.
 	 *
 	 * @private
+	 * @param {module:engine/model/writer~Writer} writer The writer on which post-fixer callbacks will be called.
 	 */
 	_callPostFixers( writer ) {
 		let wasFixed = false;

+ 12 - 2
packages/ckeditor5-engine/src/model/model.js

@@ -684,6 +684,7 @@ export default class Model {
 	 */
 	_runPendingChanges() {
 		const ret = [];
+		let hasModelDocumentChanged = false;
 
 		this.fire( '_beforeChanges' );
 
@@ -696,14 +697,19 @@ export default class Model {
 			const callbackReturnValue = this._pendingChanges[ 0 ].callback( this._currentWriter );
 			ret.push( callbackReturnValue );
 
-			// Fire internal `_change` event.
+			// Collect an information whether the model document has changed during from the last pending change.
+			hasModelDocumentChanged = hasModelDocumentChanged || this.document._hasDocumentChangedFromTheLastChangeBlock();
+
+			// Fire '_change' event before resetting differ.
 			this.fire( '_change', this._currentWriter );
 
+			this.document._handleChangeBlock( this._currentWriter );
+
 			this._pendingChanges.shift();
 			this._currentWriter = null;
 		}
 
-		this.fire( '_afterChanges' );
+		this.fire( '_afterChanges', { hasModelDocumentChanged } );
 
 		return ret;
 	}
@@ -714,6 +720,7 @@ export default class Model {
 	 *
 	 * **Note:** This is an internal event! Use {@link module:engine/model/document~Document#event:change} instead.
 	 *
+	 * @deprecated
 	 * @protected
 	 * @event _change
 	 * @param {module:engine/model/writer~Writer} writer `Writer` instance that has been used in the change block.
@@ -733,6 +740,9 @@ export default class Model {
 	 *
 	 * @protected
 	 * @event _afterChanges
+	 * @param {Object} options
+	 * @param {Boolean} options.hasModelDocumentChanged `true` if the model document has changed during the
+	 * {@link module:engine/model/model~Model#change} or {@link module:engine/model/model~Model#enqueueChange} blocks.
 	 */
 
 	/**

+ 25 - 7
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -541,7 +541,7 @@ class ContextFactory {
 						if ( ( affectedLeft || affectedRight ) && !movedRange.containsRange( markerRange ) ) {
 							this._setRelation( opA, opB, {
 								side: affectedLeft ? 'left' : 'right',
-								offset: affectedLeft ? markerRange.start.offset : markerRange.end.offset
+								path: affectedLeft ? markerRange.start.path.slice() : markerRange.end.path.slice()
 							} );
 						}
 
@@ -550,10 +550,17 @@ class ContextFactory {
 
 					case MergeOperation: {
 						const wasInLeftElement = markerRange.start.isEqual( opB.targetPosition );
+						const wasStartBeforeMergedElement = markerRange.start.isEqual( opB.deletionPosition );
+						const wasEndBeforeMergedElement = markerRange.end.isEqual( opB.deletionPosition );
 						const wasInRightElement = markerRange.end.isEqual( opB.sourcePosition );
 
-						if ( wasInLeftElement || wasInRightElement ) {
-							this._setRelation( opA, opB, { wasInLeftElement, wasInRightElement } );
+						if ( wasInLeftElement || wasStartBeforeMergedElement || wasEndBeforeMergedElement || wasInRightElement ) {
+							this._setRelation( opA, opB, {
+								wasInLeftElement,
+								wasStartBeforeMergedElement,
+								wasEndBeforeMergedElement,
+								wasInRightElement
+							} );
 						}
 
 						break;
@@ -1122,13 +1129,16 @@ setTransformation( MarkerOperation, MoveOperation, ( a, b, context ) => {
 
 	if ( a.newRange ) {
 		if ( context.abRelation ) {
+			const aNewRange = Range._createFromRanges( a.newRange._getTransformedByMoveOperation( b ) );
+
 			if ( context.abRelation.side == 'left' && b.targetPosition.isEqual( a.newRange.start ) ) {
-				a.newRange.start.offset = context.abRelation.offset;
-				a.newRange.end.offset += b.howMany;
+				a.newRange.start.path = context.abRelation.path;
+				a.newRange.end = aNewRange.end;
 
 				return [ a ];
 			} else if ( context.abRelation.side == 'right' && b.targetPosition.isEqual( a.newRange.end ) ) {
-				a.newRange.end.offset = context.abRelation.offset;
+				a.newRange.start = aNewRange.start;
+				a.newRange.end.path = context.abRelation.path;
 
 				return [ a ];
 			}
@@ -1147,12 +1157,20 @@ setTransformation( MarkerOperation, SplitOperation, ( a, b, context ) => {
 
 	if ( a.newRange ) {
 		if ( context.abRelation ) {
-			if ( a.newRange.start.isEqual( b.splitPosition ) && !context.abRelation.wasInLeftElement ) {
+			const aNewRange = a.newRange._getTransformedBySplitOperation( b );
+
+			if ( a.newRange.start.isEqual( b.splitPosition ) && context.abRelation.wasStartBeforeMergedElement ) {
+				a.newRange.start = Position._createAt( b.insertionPosition );
+			} else if ( a.newRange.start.isEqual( b.splitPosition ) && !context.abRelation.wasInLeftElement ) {
 				a.newRange.start = Position._createAt( b.moveTargetPosition );
 			}
 
 			if ( a.newRange.end.isEqual( b.splitPosition ) && context.abRelation.wasInRightElement ) {
 				a.newRange.end = Position._createAt( b.moveTargetPosition );
+			} else if ( a.newRange.end.isEqual( b.splitPosition ) && context.abRelation.wasEndBeforeMergedElement ) {
+				a.newRange.end = Position._createAt( b.insertionPosition );
+			} else {
+				a.newRange.end = aNewRange.end;
 			}
 
 			return [ a ];

+ 3 - 4
packages/ckeditor5-engine/src/model/writer.js

@@ -504,13 +504,12 @@ export default class Writer {
 		this._assertWriterUsedCorrectly();
 
 		const rangeToRemove = itemOrRange instanceof Range ? itemOrRange : Range._createOn( itemOrRange );
-
-		// If part of the marker is removed, create additional marker operation for undo purposes.
-		this._addOperationForAffectedMarkers( 'move', rangeToRemove );
-
 		const ranges = rangeToRemove.getMinimalFlatRanges().reverse();
 
 		for ( const flat of ranges ) {
+			// If part of the marker is removed, create additional marker operation for undo purposes.
+			this._addOperationForAffectedMarkers( 'move', flat );
+
 			applyRemoveOperation( flat.start, flat.end.offset - flat.start.offset, this.batch, this.model );
 		}
 	}

+ 73 - 0
packages/ckeditor5-engine/tests/model/differ.js

@@ -1403,6 +1403,16 @@ describe( 'Differ', () => {
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [
 				{ name: 'name', range }
 			] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: null,
+						newRange: range
+					}
+				}
+			] );
 		} );
 
 		it( 'remove marker', () => {
@@ -1413,6 +1423,16 @@ describe( 'Differ', () => {
 			] );
 
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: range,
+						newRange: null
+					}
+				}
+			] );
 		} );
 
 		it( 'change marker\'s range', () => {
@@ -1425,6 +1445,16 @@ describe( 'Differ', () => {
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [
 				{ name: 'name', range: rangeB }
 			] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: range,
+						newRange: rangeB
+					}
+				}
+			] );
 		} );
 
 		it( 'add marker not affecting data', () => {
@@ -1445,6 +1475,8 @@ describe( 'Differ', () => {
 
 			expect( differ.getMarkersToRemove() ).to.deep.equal( [] );
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [] );
+			expect( differ.getChangedMarkers() ).to.deep.equal( [] );
+
 			expect( differ.hasDataChanges() ).to.be.false;
 		} );
 
@@ -1457,6 +1489,16 @@ describe( 'Differ', () => {
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [
 				{ name: 'name', range: rangeB }
 			] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: null,
+						newRange: rangeB
+					}
+				}
+			] );
 		} );
 
 		it( 'change marker to not affecting data', () => {
@@ -1475,6 +1517,17 @@ describe( 'Differ', () => {
 			] );
 
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: range,
+						newRange: null
+					}
+				}
+			] );
+
 			expect( differ.hasDataChanges() ).to.be.true;
 		} );
 
@@ -1489,6 +1542,16 @@ describe( 'Differ', () => {
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [
 				{ name: 'name', range }
 			] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: range,
+						newRange: range
+					}
+				}
+			] );
 		} );
 
 		it( 'change marker to the same range', () => {
@@ -1501,6 +1564,16 @@ describe( 'Differ', () => {
 			expect( differ.getMarkersToAdd() ).to.deep.equal( [
 				{ name: 'name', range }
 			] );
+
+			expect( differ.getChangedMarkers() ).to.deep.equal( [
+				{
+					name: 'name',
+					data: {
+						oldRange: range,
+						newRange: range
+					}
+				}
+			] );
 		} );
 	} );
 

+ 64 - 0
packages/ckeditor5-engine/tests/model/operation/transform/undo.js

@@ -463,4 +463,68 @@ describe( 'transform', () => {
 
 		expectClients( '<paragraph><m1:start></m1:start>Foo<m1:end></m1:end>bar</paragraph><paragraph></paragraph>' );
 	} );
+
+	it( 'marker on closing and opening tag - remove multiple elements #1', () => {
+		john.setData(
+			'<paragraph>Abc</paragraph>' +
+			'<paragraph>Foo[</paragraph>' +
+			'<paragraph>]Bar</paragraph>'
+		);
+
+		john.setMarker( 'm1' );
+		john.setSelection( [ 0, 1 ], [ 2, 2 ] );
+		john._processExecute( 'delete' );
+
+		expectClients( '<paragraph>A<m1:start></m1:start>r</paragraph>' );
+
+		john.undo();
+
+		expectClients(
+			'<paragraph>Abc</paragraph>' +
+			'<paragraph>Foo<m1:start></m1:start></paragraph>' +
+			'<paragraph><m1:end></m1:end>Bar</paragraph>'
+		);
+	} );
+
+	it( 'marker on closing and opening tag - remove multiple elements #2', () => {
+		john.setData(
+			'<paragraph>Foo[</paragraph>' +
+			'<paragraph>]Bar</paragraph>' +
+			'<paragraph>Xyz</paragraph>'
+		);
+
+		john.setMarker( 'm1' );
+		john.setSelection( [ 0, 1 ], [ 2, 2 ] );
+		john._processExecute( 'delete' );
+
+		expectClients( '<paragraph>F<m1:start></m1:start>z</paragraph>' );
+
+		john.undo();
+
+		expectClients(
+			'<paragraph>Foo<m1:start></m1:start></paragraph>' +
+			'<paragraph><m1:end></m1:end>Bar</paragraph>' +
+			'<paragraph>Xyz</paragraph>'
+		);
+	} );
+
+	it( 'marker on closing and opening tag + some text - merge elements + remove text', () => {
+		john.setData(
+			'<paragraph>Foo[</paragraph>' +
+			'<paragraph>B]ar</paragraph>'
+		);
+
+		john.setMarker( 'm1' );
+		john.setSelection( [ 0, 1 ], [ 1, 2 ] );
+		john._processExecute( 'delete' );
+
+		expectClients( '<paragraph>F<m1:start></m1:start>r</paragraph>' );
+
+		john.undo();
+
+		expectClients(
+			'<paragraph>Foo<m1:start></m1:start></paragraph>' +
+			'<paragraph>B<m1:end></m1:end>ar</paragraph>'
+		);
+	} );
 } );

+ 34 - 0
packages/ckeditor5-engine/tests/tickets/1653.js

@@ -0,0 +1,34 @@
+/**
+ * @license Copyright (c) 2003-2019, 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';
+
+describe( 'Bug ckeditor5-engine#1653', () => {
+	it( '`DataController.parse()` should not invoke `editing.view.render()`', () => {
+		let editor;
+
+		const element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		return ClassicTestEditor
+			.create( element, { plugins: [ Paragraph ] } )
+			.then( newEditor => {
+				editor = newEditor;
+
+				const spy = sinon.spy( editor.editing.view, 'render' );
+				editor.data.parse( '<p></p>' );
+
+				sinon.assert.notCalled( spy );
+			} )
+			.then( () => {
+				element.remove();
+
+				return editor.destroy();
+			} );
+	} );
+} );

+ 1 - 1
packages/ckeditor5-engine/tests/tickets/ckeditor5-692.js

@@ -29,7 +29,7 @@ describe( 'Bug ckeditor5#692', () => {
 				editor = newEditor;
 				view = editor.editing.view;
 				mutationObserver = view.getObserver( MutationObserver );
-				domEditor = editor.ui.view.editableElement;
+				domEditor = editor.ui.getEditableElement();
 			} );
 	} );