Merge pull request #1343 from ckeditor/t/1267

Fix: `model.DocumentSelection` should update it's attributes after each change, including external changes. Closes #1267.

Author: scofalik

src/model/documentselection.js

@@ -19,10 +19,6 @@ import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import log from '@ckeditor/ckeditor5-utils/src/log';
 
-const attrOpTypes = new Set(
-	[ 'addAttribute', 'removeAttribute', 'changeAttribute', 'addRootAttribute', 'removeRootAttribute', 'changeRootAttribute' ]
-);
-
 const storePrefix = 'selection:';
 
 /**
@@ -533,29 +529,13 @@ class LiveSelection extends Selection {
 			}
 		} );
 
-		this.listenTo( this._model, 'applyOperation', ( evt, args ) => {
-			const operation = args[ 0 ];
-
-			if ( !operation.isDocumentOperation ) {
-				return;
-			}
-
-			// Whenever attribute operation is performed on document, update selection attributes.
-			// This is not the most efficient way to update selection attributes, but should be okay for now.
-			if ( attrOpTypes.has( operation.type ) ) {
-				this._updateAttributes( false );
-			}
-
-			const batch = operation.delta.batch;
+		this.listenTo( this._document, 'change', ( evt, batch ) => {
+			// Update selection's attributes.
+			this._updateAttributes( false );
 
-			// Batch may not be passed to the document#change event in some tests.
-			// See https://github.com/ckeditor/ckeditor5-engine/issues/1001#issuecomment-314202352
-			if ( batch ) {
-				// Whenever element which had selection's attributes stored in it stops being empty,
-				// the attributes need to be removed.
-				clearAttributesStoredInElement( operation, this._model, batch );
-			}
-		}, { priority: 'low' } );
+			// Clear selection attributes from element if no longer empty.
+			clearAttributesStoredInElement( this._model, batch );
+		} );
 
 		this.listenTo( this._model, 'applyOperation', () => {
 			while ( this._fixGraveyardRangesData.length ) {
@@ -823,6 +803,9 @@ class LiveSelection extends Selection {
 	// Internal method for removing `LiveSelection` attribute. Supports attribute priorities (through `directChange`
 	// parameter).
 	//
+	// NOTE: Even if attribute is not present in the selection but is provided to this method, it's priority will
+	// be changed according to `directChange` parameter.
+	//
 	// @private
 	// @param {String} key Attribute key.
 	// @param {Boolean} [directChange=true] `true` if the change is caused by `Selection` API, `false` if change
@@ -837,16 +820,16 @@ class LiveSelection extends Selection {
 			return false;
 		}
 
+		// Update priorities map.
+		this._attributePriority.set( key, priority );
+
 		// Don't do anything if value has not changed.
 		if ( !super.hasAttribute( key ) ) {
 			return false;
 		}
 
 		this._attrs.delete( key );
 
-		// Update priorities map.
-		this._attributePriority.set( key, priority );
-
 		return true;
 	}
 
@@ -1021,24 +1004,30 @@ function getAttrsIfCharacter( node ) {
 }
 
 // Removes selection attributes from element which is not empty anymore.
-function clearAttributesStoredInElement( operation, model, batch ) {
-	let changeParent = null;
-
-	if ( operation.type == 'insert' ) {
-		changeParent = operation.position.parent;
-	} else if ( operation.type == 'move' || operation.type == 'reinsert' || operation.type == 'remove' ) {
-		changeParent = operation.getMovedRangeStart().parent;
-	}
+//
+// @private
+// @param {module:engine/model/model~Model} model
+// @param {module:engine/model/batch~Batch} batch
+function clearAttributesStoredInElement( model, batch ) {
+	const differ = model.document.differ;
+
+	for ( const entry of differ.getChanges() ) {
+		if ( entry.type != 'insert' ) {
+			continue;
+		}
 
-	if ( !changeParent || changeParent.isEmpty ) {
-		return;
-	}
+		const changeParent = entry.position.parent;
+		const isNoLongerEmpty = entry.length === changeParent.maxOffset;
 
-	model.enqueueChange( batch, writer => {
-		const storedAttributes = Array.from( changeParent.getAttributeKeys() ).filter( key => key.startsWith( storePrefix ) );
+		if ( isNoLongerEmpty ) {
+			model.enqueueChange( batch, writer => {
+				const storedAttributes = Array.from( changeParent.getAttributeKeys() )
+					.filter( key => key.startsWith( storePrefix ) );
 
-		for ( const key of storedAttributes ) {
-			writer.removeAttribute( key, changeParent );
+				for ( const key of storedAttributes ) {
+					writer.removeAttribute( key, changeParent );
+				}
+			} );
 		}
-	} );
+	}
 }

tests/model/documentselection.js

@@ -439,6 +439,21 @@ describe( 'DocumentSelection', () => {
 
 				expect( selection.getAttribute( 'foo' ) ).to.be.undefined;
 			} );
+
+			it( 'should prevent auto update of the attribute even if attribute is not preset yet', () => {
+				selection._setTo( new Position( root, [ 0, 1 ] ) );
+
+				// Remove "foo" attribute that is not present in selection yet.
+				expect( selection.hasAttribute( 'foo' ) ).to.be.false;
+				selection._removeAttribute( 'foo' );
+
+				// Trigger selecton auto update on document change. It should not get attribute from surrounding text;
+				model.change( writer => {
+					writer.setAttribute( 'foo', 'bar', Range.createIn( fullP ) );
+				} );
+
+				expect( selection.getAttribute( 'foo' ) ).to.be.undefined;
+			} );
 		} );
 
 		describe( '_getStoredAttributes()', () => {
@@ -1066,6 +1081,9 @@ describe( 'DocumentSelection', () => {
 					)
 				) );
 
+				// Attributes are auto updated on document change.
+				model.change( () => {} );
+
 				expect( selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( spyAttribute.calledOnce ).to.be.true;
 			} );

tests/tickets/1267.js

@@ -0,0 +1,75 @@
+/**
+ * @license Copyright (c) 2003-2018, 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 Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
+import Position from '../../src/model/position';
+import Range from '../../src/model/range';
+import { setData as setModelData, getData as getModelData } from '../../src/dev-utils/model';
+
+describe( 'Bug ckeditor5-engine#1267', () => {
+	let element, editor, model;
+
+	beforeEach( () => {
+		element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		return ClassicTestEditor
+			.create( element, { plugins: [ Paragraph, Bold ] } )
+			.then( newEditor => {
+				editor = newEditor;
+				model = editor.model;
+			} );
+	} );
+
+	afterEach( () => {
+		element.remove();
+
+		return editor.destroy();
+	} );
+
+	it( 'selection should not retain attributes after external change removal', () => {
+		setModelData( model,
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph>foo <$text bold="true">bar{}</$text> baz</paragraph>'
+		);
+
+		// Remove second paragraph where selection is placed.
+		model.enqueueChange( 'transparent', writer => {
+			writer.remove( Range.createFromPositionAndShift( new Position( model.document.getRoot(), [ 1 ] ), 1 ) );
+		} );
+
+		expect( getModelData( model ) ).to.equal( '<paragraph>foo bar baz[]</paragraph>' );
+	} );
+
+	it( 'selection should retain attributes set manually', () => {
+		setModelData( model,
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph>[]</paragraph>'
+		);
+
+		// Execute bold command when selection is inside empty paragraph.
+		editor.execute( 'bold' );
+		expect( getModelData( model ) ).to.equal(
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph selection:bold="true"><$text bold="true">[]</$text></paragraph>'
+		);
+
+		// Remove second paragraph.
+		model.enqueueChange( 'transparent', writer => {
+			writer.remove( Range.createFromPositionAndShift( new Position( model.document.getRoot(), [ 1 ] ), 1 ) );
+		} );
+
+		// Selection attributes set by command should stay as they were.
+		expect( getModelData( model ) ).to.equal(
+			'<paragraph>foo bar baz</paragraph>' +
+			'<paragraph selection:bold="true"><$text bold="true">[]</$text></paragraph>' );
+	} );
+} );