From e08334de542f944dd0106adafc7dff02cc404078 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 10 Apr 2018 12:04:55 +0200 Subject: [PATCH] Fix: view.Writer should deeply add and remove attribute elements to/from their clone groups. --- src/view/writer.js | 34 +++++++++++++-- tests/view/writer/writer.js | 84 +++++++++++++++++++++++++++++++++---- 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/src/view/writer.js b/src/view/writer.js index 2ac2450b8..b624d92bb 100644 --- a/src/view/writer.js +++ b/src/view/writer.js @@ -941,12 +941,13 @@ export default class Writer { if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( attribute, child ) ) ) { // Clone attribute. const newAttribute = attribute._clone(); - this._addToClonedElementsGroup( newAttribute ); // Wrap current node with new attribute. child._remove(); newAttribute._appendChild( child ); + parent._insertChild( i, newAttribute ); + this._addToClonedElementsGroup( newAttribute ); wrapPositions.push( new Position( parent, i ) ); } @@ -1006,10 +1007,10 @@ export default class Writer { // Replace wrapper element with its children child._remove(); - this._removeFromClonedElementsGroup( child ); - parent._insertChild( i, unwrapped ); + this._removeFromClonedElementsGroup( child ); + // Save start and end position of moved items. unwrapPositions.push( new Position( parent, i ), @@ -1415,10 +1416,10 @@ export default class Writer { // Break element. const clonedNode = positionParent._clone(); - this._addToClonedElementsGroup( clonedNode ); // Insert cloned node to position's parent node. positionParent.parent._insertChild( offsetAfter, clonedNode ); + this._addToClonedElementsGroup( clonedNode ); // Get nodes to move. const count = positionParent.childCount - positionOffset; @@ -1447,6 +1448,19 @@ export default class Writer { * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to save. */ _addToClonedElementsGroup( element ) { + // Add only if the element is in document tree. + if ( !element.root.is( 'rootElement' ) ) { + return; + } + + // Traverse the element's children recursively to find other attribute elements that also might got inserted. + // The loop is at the beginning so we can make fast returns later in the code. + if ( element.is( 'element' ) ) { + for ( const child of element.getChildren() ) { + this._addToClonedElementsGroup( child ); + } + } + const id = element.id; if ( !id ) { @@ -1477,6 +1491,14 @@ export default class Writer { * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to remove. */ _removeFromClonedElementsGroup( element ) { + // Traverse the element's children recursively to find other attribute elements that also got removed. + // The loop is at the beginning so we can make fast returns later in the code. + if ( element.is( 'element' ) ) { + for ( const child of element.getChildren() ) { + this._removeFromClonedElementsGroup( child ); + } + } + const id = element.id; if ( !id ) { @@ -1485,6 +1507,10 @@ export default class Writer { const group = this._cloneGroups.get( id ); + if ( !group ) { + return; + } + group.delete( element ); // Not removing group from element on purpose! // If other parts of code have reference to this element, they will be able to get references to other elements from the group. diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js index 58a29b1e5..853de8811 100644 --- a/tests/view/writer/writer.js +++ b/tests/view/writer/writer.js @@ -13,7 +13,7 @@ import createViewRoot from '../_utils/createroot'; describe( 'Writer', () => { let writer, attributes, root, doc; - before( () => { + beforeEach( () => { attributes = { foo: 'bar', baz: 'quz' }; doc = new Document(); root = createViewRoot( doc ); @@ -43,6 +43,9 @@ describe( 'Writer', () => { describe( 'setSelectionFocus()', () => { it( 'should use selection._setFocus method internally', () => { + const position = ViewPosition.createAt( root ); + writer.setSelection( position ); + const spy = sinon.spy( writer.document.selection, '_setFocus' ); writer.setSelectionFocus( root, 0 ); @@ -255,10 +258,9 @@ describe( 'Writer', () => { describe( 'manages AttributeElement#_clonesGroup', () => { it( 'should return all clones of a broken attribute element with id', () => { - const container = writer.createContainerElement( 'div' ); const text = writer.createText( 'abccccde' ); - writer.insert( ViewPosition.createAt( container, 0 ), text ); + writer.insert( ViewPosition.createAt( root, 0 ), text ); const span = writer.createAttributeElement( 'span', null, { id: 'foo' } ); span._priority = 20; @@ -271,8 +273,8 @@ describe( 'Writer', () => { //
abcccde
writer.wrap( ViewRange.createFromParentsAndOffsets( - container.getChild( 0 ), 1, - container.getChild( 1 ).getChild( 0 ), 1 + root.getChild( 0 ), 1, + root.getChild( 1 ).getChild( 0 ), 1 ), i ); @@ -280,14 +282,14 @@ describe( 'Writer', () => { //
abccccde
writer.wrap( ViewRange.createFromParentsAndOffsets( - container.getChild( 2 ).getChild( 0 ), 1, - container.getChild( 3 ), 1 + root.getChild( 2 ).getChild( 0 ), 1, + root.getChild( 3 ), 1 ), i ); // Find all spans. - const allSpans = Array.from( ViewRange.createIn( container ).getItems() ).filter( element => element.is( 'span' ) ); + const allSpans = Array.from( ViewRange.createIn( root ).getItems() ).filter( element => element.is( 'span' ) ); // For each of the spans created above... for ( const oneOfAllSpans of allSpans ) { @@ -302,6 +304,72 @@ describe( 'Writer', () => { expect( brokenArray.length ).to.equal( allSpans.length ); } } ); + + it( 'should not create groups for attribute elements that are not created in document root', () => { + const p = writer.createContainerElement( 'p' ); + const foo = writer.createText( 'foo' ); + writer.insert( ViewPosition.createAt( p, 0 ), foo ); + //

foo

+ + const span = writer.createAttributeElement( 'span', null, { id: 'span' } ); + + //

foo

+ writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 0, foo, 3 ), span ); + + // Find the span. + const createdSpan = p.getChild( 0 ); + + expect( createdSpan.getElementsWithSameId().size ).to.equal( 0 ); + } ); + + it( 'should add attribute elements to clone groups deeply', () => { + const p = writer.createContainerElement( 'p' ); + const foo = writer.createText( 'foo' ); + writer.insert( ViewPosition.createAt( p, 0 ), foo ); + //

foo

+ + const span = writer.createAttributeElement( 'span', null, { id: 'span' } ); + + //

foo

+ writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 0, foo, 3 ), span ); + + //

foo

+ writer.insert( ViewPosition.createAt( root, 0 ), p ); + + // Find the span. + const createdSpan = p.getChild( 0 ); + + expect( Array.from( createdSpan.getElementsWithSameId() ) ).to.deep.equal( [ createdSpan ] ); + } ); + + it( 'should remove attribute elements from clone groups deeply', () => { + const p1 = writer.createContainerElement( 'p' ); + const p2 = writer.createContainerElement( 'p' ); + const foo = writer.createText( 'foo' ); + const bar = writer.createText( 'bar' ); + + writer.insert( ViewPosition.createAt( root, 0 ), p1 ); + writer.insert( ViewPosition.createAt( root, 1 ), p2 ); + writer.insert( ViewPosition.createAt( p1, 0 ), foo ); + writer.insert( ViewPosition.createAt( p2, 0 ), bar ); + //

foo

bar

+ + const span = writer.createAttributeElement( 'span', null, { id: 'span' } ); + + //

foo

bar

+ writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 2, foo, 3 ), span ); + + //

foo

bar

+ writer.wrap( ViewRange.createFromParentsAndOffsets( bar, 0, bar, 1 ), span ); + + //

bar

+ writer.remove( ViewRange.createOn( p1 ) ); + + // Find the span. + const spanInTree = p2.getChild( 0 ); + + expect( Array.from( spanInTree.getElementsWithSameId() ) ).to.deep.equal( [ spanInTree ] ); + } ); } ); function assertElementAttributes( element, attributes ) {