diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 72774827e..0441e1e2a 100644 --- a/src/model/documentselection.js +++ b/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,6 +820,9 @@ 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; @@ -844,9 +830,6 @@ class LiveSelection extends Selection { 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 ); + } + } ); } - } ); + } } diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index 5e3fea79d..84e59e0be 100644 --- a/tests/model/documentselection.js +++ b/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; } ); diff --git a/tests/tickets/1267.js b/tests/tickets/1267.js new file mode 100644 index 000000000..6f998aa71 --- /dev/null +++ b/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, + 'foo bar baz' + + 'foo <$text bold="true">bar{} baz' + ); + + // 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( 'foo bar baz[]' ); + } ); + + it( 'selection should retain attributes set manually', () => { + setModelData( model, + 'foo bar baz' + + 'foo bar baz' + + '[]' + ); + + // Execute bold command when selection is inside empty paragraph. + editor.execute( 'bold' ); + expect( getModelData( model ) ).to.equal( + 'foo bar baz' + + 'foo bar baz' + + '<$text bold="true">[]' + ); + + // 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( + 'foo bar baz' + + '<$text bold="true">[]' ); + } ); +} );