From 9014fdd36352a07f4bef535859564c04444aeb1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Fri, 2 Mar 2018 14:30:39 +0100 Subject: [PATCH 1/6] Added tests checking 1267 issue. --- tests/tickets/1267.js | 75 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 tests/tickets/1267.js 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">[]' ); + } ); +} ); From 1178020e5030698ca02713acbe5a875e1279554d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Fri, 2 Mar 2018 17:53:38 +0100 Subject: [PATCH 2/6] DocumentSelection attributes update fix WIP. --- src/model/documentselection.js | 65 ++++++++++++-------------------- tests/model/documentselection.js | 13 ++----- 2 files changed, 28 insertions(+), 50 deletions(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 58fc7447b..34f007cd5 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:'; /** @@ -531,29 +527,13 @@ class LiveSelection extends Selection { } } ); - this.listenTo( this._model, 'applyOperation', ( evt, args ) => { - const operation = args[ 0 ]; - - if ( !operation.isDocumentOperation ) { - return; - } + this.listenTo( this._document, 'change', ( evt, batch ) => { + // Update selection's attributes. + this._updateAttributes( false ); - // 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; - - // 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 ) { @@ -1019,24 +999,27 @@ function getAttrsIfCharacter( node ) { } // Removes selection attributes from element which is not empty anymore. -function clearAttributesStoredInElement( operation, model, batch ) { - let changeParent = null; +function clearAttributesStoredInElement( model, batch ) { + // Clear attributes stored in selection; + const differ = model.document.differ; - if ( operation.type == 'insert' ) { - changeParent = operation.position.parent; - } else if ( operation.type == 'move' || operation.type == 'reinsert' || operation.type == 'remove' ) { - changeParent = operation.getMovedRangeStart().parent; - } + for ( const entry of differ.getChanges() ) { + if ( entry.type != 'insert' || !entry.position.parent ) { + 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 11f3bef20..adc51f215 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -1056,15 +1056,10 @@ describe( 'DocumentSelection', () => { expect( data.attributeKeys ).to.deep.equal( [ 'foo' ] ); } ); - model.applyOperation( wrapInDelta( - new AttributeOperation( - new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 5 ] ) ), - 'foo', - null, - 'bar', - doc.version - ) - ) ); + model.change( writer => { + const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 5 ] ) ); + writer.setAttribute( 'foo', 'bar', range ); + } ); expect( selection.getAttribute( 'foo' ) ).to.equal( 'bar' ); expect( spyAttribute.calledOnce ).to.be.true; From 9a614aacc12d454ad7708b1563c3befd36804f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Tue, 6 Mar 2018 16:43:43 +0100 Subject: [PATCH 3/6] Improved docs in DocumentSelection. --- src/model/documentselection.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 34f007cd5..69c78fe14 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -999,8 +999,11 @@ function getAttrsIfCharacter( node ) { } // Removes selection attributes from element which is not empty anymore. +// +// @private +// @param {module:engine/model/model~Model} model +// @param {module:engine/model/batch~Batch} batch function clearAttributesStoredInElement( model, batch ) { - // Clear attributes stored in selection; const differ = model.document.differ; for ( const entry of differ.getChanges() ) { From 1be70963e96848d3c9b930f05d6ac441d30f4d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Wed, 7 Mar 2018 10:28:03 +0100 Subject: [PATCH 4/6] Small doc fix. --- src/model/documentselection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 69c78fe14..8a15b3f75 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -531,7 +531,7 @@ class LiveSelection extends Selection { // Update selection's attributes. this._updateAttributes( false ); - // Clear selection attributes from element if no longer empty, + // Clear selection attributes from element if no longer empty. clearAttributesStoredInElement( this._model, batch ); } ); From 19bf4a3878d8260e6a4bf738ea40022d6b7604e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 8 Mar 2018 12:37:32 +0100 Subject: [PATCH 5/6] Removing non-exitent attributes from DocumentSelection prevents from adding them during attribute auto-update. --- src/model/documentselection.js | 9 ++++++--- tests/model/documentselection.js | 31 +++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 8a15b3f75..ba93b2f45 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -801,6 +801,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 @@ -815,6 +818,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; @@ -822,9 +828,6 @@ class LiveSelection extends Selection { this._attrs.delete( key ); - // Update priorities map. - this._attributePriority.set( key, priority ); - return true; } diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index adc51f215..097e44250 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()', () => { @@ -1056,10 +1071,18 @@ describe( 'DocumentSelection', () => { expect( data.attributeKeys ).to.deep.equal( [ 'foo' ] ); } ); - model.change( writer => { - const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 5 ] ) ); - writer.setAttribute( 'foo', 'bar', range ); - } ); + model.applyOperation( wrapInDelta( + new AttributeOperation( + new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 5 ] ) ), + 'foo', + null, + 'bar', + doc.version + ) + ) ); + + // Attributes are auto updated on document change. + model.change( () => {} ); expect( selection.getAttribute( 'foo' ) ).to.equal( 'bar' ); expect( spyAttribute.calledOnce ).to.be.true; From d90354f8000eb75c4913a03eeaf90f274bae883f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 8 Mar 2018 14:09:18 +0100 Subject: [PATCH 6/6] Removed unnecessary check for position parent. --- src/model/documentselection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 44dc7ee50..0441e1e2a 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -1012,7 +1012,7 @@ function clearAttributesStoredInElement( model, batch ) { const differ = model.document.differ; for ( const entry of differ.getChanges() ) { - if ( entry.type != 'insert' || !entry.position.parent ) { + if ( entry.type != 'insert' ) { continue; }