From fe31554be0d445440c9d6b367a9900b3318b3c6b Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 14 Mar 2018 11:54:52 +0100 Subject: [PATCH] Changed: Refactored how `view.Writer` stores cloned attribute elements groups and how `conversion.Mapper` uses this information. --- src/conversion/downcast-converters.js | 22 ++-- src/conversion/mapper.js | 23 ++++- src/view/writer.js | 138 ++++++++++++++++++-------- tests/conversion/mapper.js | 2 +- tests/view/writer/writer.js | 47 +++++++-- 5 files changed, 165 insertions(+), 67 deletions(-) diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js index ab05563c3..2a6d5cf31 100644 --- a/src/conversion/downcast-converters.js +++ b/src/conversion/downcast-converters.js @@ -810,6 +810,10 @@ export function highlightText( highlightDescriptor ) { for ( const element of rangeAfterWrap.getItems() ) { if ( element.is( 'attributeElement' ) && element.isSimilar( viewElement ) ) { conversionApi.mapper.bindElementToMarker( element, data.markerName ); + + // One attribute element is enough, because all of them are bound together by the view writer. + // Mapper uses this binding to get all the elements no matter how many of them are registered in the mapper. + break; } } } @@ -925,23 +929,9 @@ export function removeHighlight( highlightDescriptor ) { for ( const element of elements ) { if ( element.is( 'attributeElement' ) ) { - // If the bound element is an `AttributeElement`, get all other `AttributeElement`s that were created "from it" - // (when view.Writer broke it when handling other `AttributeElement`s). - const allAttributeElements = conversionApi.writer.getAllBrokenSiblings( element ); - - // Handle all those elements. - for ( const attributeElement of allAttributeElements ) { - // Filter out elements which got removed. For example, when converting from model to view, - // converter might have created two `AttributeElement`s, split by some other element (for - // example another marker). Then, that splitting element might got removed and the marker parts - // might got merged. In this case, previously bound element might got removed and now has to be filtered. - if ( attributeElement.parent ) { - // If the element is still in the tree, unwrap it. - conversionApi.writer.unwrap( ViewRange.createOn( attributeElement ), viewHighlightElement ); - } - } + conversionApi.writer.unwrap( ViewRange.createOn( element ), viewHighlightElement ); } else { - // If the bound element is a ContainerElement, just use `removeHighlight` function on it. + // if element.is( 'containerElement' ). element.getCustomProperty( 'removeHighlight' )( element, descriptor.id, conversionApi.writer ); } } diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js index 22b955d43..3c530763e 100644 --- a/src/conversion/mapper.js +++ b/src/conversion/mapper.js @@ -280,10 +280,29 @@ export default class Mapper { * Gets all view elements bound to the given marker name. * * @param {String} name Marker name. - * @returns {Set.} View elements bound with given marker name. + * @returns {Set.|null} View elements bound with given marker name or `null` + * if no elements are bound to given marker name. */ markerNameToElements( name ) { - return this._markerNameToElements.get( name ); + const boundElements = this._markerNameToElements.get( name ); + + if ( !boundElements ) { + return null; + } + + const elements = new Set(); + + for ( const element of boundElements ) { + if ( element.is( 'attributeElement' ) ) { + for ( const clone of element.getCustomProperty( 'clonedElements' ) ) { + elements.add( clone ); + } + } else { + elements.add( element ); + } + } + + return elements; } /** diff --git a/src/view/writer.js b/src/view/writer.js index 4aab8bf76..0701269b7 100644 --- a/src/view/writer.js +++ b/src/view/writer.js @@ -32,6 +32,15 @@ export default class Writer { * @type {module:engine/view/document~Document} */ this.document = document; + + /** + * Holds references to the attribute groups that share the same {@link module:engine/view/attributeelement~AttributeElement#id id}. + * The keys are `id`s, the values are `Set`s holding {@link module:engine/view/attributeelement~AttributeElement}s. + * + * @private + * @type {Map} + */ + this._cloneGroups = new Map(); } /** @@ -503,9 +512,9 @@ export default class Writer { if ( positionParent.is( 'attributeElement' ) && positionParent.childCount === 0 ) { const parent = positionParent.parent; const offset = positionParent.index; - positionParent._remove(); - this._unlinkBrokenAttributeElement( positionParent ); + positionParent._remove(); + this._removeFromClonedElementsGroup( positionParent ); return this.mergeAttributes( new Position( parent, offset ) ); } @@ -529,7 +538,7 @@ export default class Writer { nodeBefore._appendChildren( nodeAfter.getChildren() ); nodeAfter._remove(); - this._unlinkBrokenAttributeElement( nodeAfter ); + this._removeFromClonedElementsGroup( nodeAfter ); // New position is located inside the first node, before new nodes. // Call this method recursively to merge again if needed. @@ -617,8 +626,12 @@ export default class Writer { } const insertionPosition = this._breakAttributes( position, true ); - const length = container._insertChildren( insertionPosition.offset, nodes ); + + for ( const node of nodes ) { + this._addToClonedElementsGroup( node ); + } + const endPosition = insertionPosition.getShiftedBy( length ); const start = this.mergeAttributes( insertionPosition ); @@ -666,7 +679,7 @@ export default class Writer { const removed = parentContainer._removeChildren( breakStart.offset, count ); for ( const node of removed ) { - this._unlinkBrokenAttributeElement( node ); + this._removeFromClonedElementsGroup( node ); } // Merge after removing. @@ -912,21 +925,22 @@ export default class Writer { } /** - * For the given attribute element, returns that element and all the elements that were cloned from the given element - * when the element was broken by `Writer`. + * For given {@link module:engine/view/attributeelement~AttributeElement attribute element}, returns all other + * attribute elements with the same {@link module:engine/view/attributeelement~AttributeElement#id id} that + * were inserted by `Writer` to the view tree. * * @param {module:engine/view/attributeelement~AttributeElement} element - * @returns {Array.} + * @returns {Set.|null} Set containing all the attribute elements + * with the same `id` or `null` if given element had no `id`. */ - getAllBrokenSiblings( element ) { - const original = element.getCustomProperty( 'originalAttributeElement' ) || element; - const clones = original.getCustomProperty( 'clonedAttributeElements' ); + getAllClonedElements( element ) { + const id = element.id; - if ( clones ) { - return Array.from( clones ).concat( original ); - } else { - return [ element ]; + if ( !id ) { + return null; } + + return this._cloneGroups.get( id ); } /** @@ -954,6 +968,7 @@ 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(); @@ -1018,7 +1033,7 @@ export default class Writer { // Replace wrapper element with its children child._remove(); - this._unlinkBrokenAttributeElement( child ); + this._removeFromClonedElementsGroup( child ); parent._insertChildren( i, unwrapped ); @@ -1314,6 +1329,15 @@ export default class Writer { return true; } + /** + * Helper function used by other `Writer` methods. Breaks attribute elements at the boundaries of given range. + * + * @private + * @param {module:engine/view/range~Range} range Range which `start` and `end` positions will be used to break attributes. + * @param {Boolean} [forceSplitText=false] If set to `true`, will break text nodes even if they are directly in container element. + * This behavior will result in incorrect view state, but is needed by other view writing methods which then fixes view state. + * @returns {module:engine/view/range~Range} New range with located at break positions. + */ _breakAttributesRange( range, forceSplitText = false ) { const rangeStart = range.start; const rangeEnd = range.end; @@ -1337,6 +1361,21 @@ export default class Writer { return new Range( breakStart, breakEnd ); } + /** + * Helper function used by other `Writer` methods. Breaks attribute elements at given position. + * + * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-empty-element` when break position + * is placed inside {@link module:engine/view/emptyelement~EmptyElement EmptyElement}. + * + * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-ui-element` when break position + * is placed inside {@link module:engine/view/uielement~UIElement UIElement}. + * + * @private + * @param {module:engine/view/position~Position} position Position where to break attributes. + * @param {Boolean} [forceSplitText=false] If set to `true`, will break text nodes even if they are directly in container element. + * This behavior will result in incorrect view state, but is needed by other view writing methods which then fixes view state. + * @returns {module:engine/view/position~Position} New position after breaking the attributes. + */ _breakAttributes( position, forceSplitText = false ) { const positionOffset = position.offset; const positionParent = position.parent; @@ -1403,9 +1442,7 @@ export default class Writer { // Break element. const clonedNode = positionParent._clone(); - - // Save that the `clonedNode` was created from `positionParent`. - this._linkBrokenAttributeElements( clonedNode, positionParent ); + this._addToClonedElementsGroup( clonedNode ); // Insert cloned node to position's parent node. positionParent.parent._insertChildren( offsetAfter, clonedNode ); @@ -1425,42 +1462,65 @@ export default class Writer { } /** - * Links `clonedNode` element to `clonedFrom` element. Saves information that `clonedNode` was created as a clone - * of `clonedFrom` when `clonedFrom` was broken into two elements by the `Writer`. + * Stores the information that an {@link module:engine/view/attributeelement~AttributeElement attribute element} was + * added to the tree. Saves the reference to the group in the given element and updates the group, so other elements + * from the group now keep a reference to the given attribute element. + * + * The clones group can be accessed from the element, under `clonedElements` custom property: + * + * attributeElement.getCustomProperty( 'clonedElements' ); + * + * Does nothing if added element has no {@link module:engine/view/attributeelement~AttributeElement#id id}. * * @private - * @param {module:engine/view/attributeelement~AttributeElement} clonedNode - * @param {module:engine/view/attributeelement~AttributeElement} clonedFrom + * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to save. */ - _linkBrokenAttributeElements( clonedNode, clonedFrom ) { - const original = clonedFrom.getCustomProperty( 'originalAttributeElement' ) || clonedFrom; - const clones = original.getCustomProperty( 'clonedAttributeElements' ) || new Set(); + _addToClonedElementsGroup( element ) { + const id = element.id; - this.setCustomProperty( 'originalAttributeElement', original, clonedNode ); + if ( !id ) { + return; + } + + let group = this._cloneGroups.get( id ); + + if ( !group ) { + group = new Set(); + this._cloneGroups.set( id, group ); + } - clones.add( clonedNode ); - this.setCustomProperty( 'clonedAttributeElements', clones, original ); + group.add( element ); + this.setCustomProperty( 'clonedElements', group, element ); } /** - * Unlinks `element`. Removes information about how `element` was broken by the `Writer` from `element` and from - * its original element (the element it was cloned from). + * Removes all the information about the given {@link module:engine/view/attributeelement~AttributeElement attribute element} + * from its clones group. + * + * Keep in mind, that the element will still keep a reference to the group (but the group will not keep a reference to it). + * This allows to reference the whole group even if the element was already removed from the tree. + * + * Does nothing if the element has no {@link module:engine/view/attributeelement~AttributeElement#id id}. * * @private - * @param {module:engine/view/attributeelement~AttributeElement} element + * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to remove. */ - _unlinkBrokenAttributeElement( element ) { - if ( !element.is( 'element' ) ) { + _removeFromClonedElementsGroup( element ) { + const id = element.id; + + if ( !id ) { return; } - const original = element.getCustomProperty( 'originalAttributeElement' ); + const group = this._cloneGroups.get( id ); - if ( original ) { - this.removeCustomProperty( 'originalAttributeElement', element ); + 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. + // If all other elements are removed from the set, everything will be garbage collected. - const clones = original.getCustomProperty( 'clonedAttributeElements' ); - clones.delete( element ); + if ( group.size === 0 ) { + this._cloneGroups.delete( id ); } } } diff --git a/tests/conversion/mapper.js b/tests/conversion/mapper.js index 9414bcbc2..0c35aa078 100644 --- a/tests/conversion/mapper.js +++ b/tests/conversion/mapper.js @@ -656,7 +656,7 @@ describe( 'Mapper', () => { const elements = mapper.markerNameToElements( 'marker' ); - expect( elements ).to.be.undefined; + expect( elements ).to.be.null; } ); } ); diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js index 776122444..96c9f6f28 100644 --- a/tests/view/writer/writer.js +++ b/tests/view/writer/writer.js @@ -263,14 +263,14 @@ describe( 'Writer', () => { } ); } ); - describe( 'getAllBrokenSiblings()', () => { - it( 'should return all clones of a broken attribute element', () => { + describe( 'getAllClonedElements()', () => { + 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 ); - const span = writer.createAttributeElement( 'span' ); + const span = writer.createAttributeElement( 'span', null, { id: 'foo' } ); span._priority = 20; //
abccccde
@@ -287,7 +287,7 @@ describe( 'Writer', () => { i ); - //
abcccde
+ //
abccccde
writer.wrap( ViewRange.createFromParentsAndOffsets( container.getChild( 2 ).getChild( 0 ), 1, @@ -301,11 +301,8 @@ describe( 'Writer', () => { // For each of the spans created above... for ( const oneOfAllSpans of allSpans ) { - // Find all broken siblings of that span... - const brokenArray = writer.getAllBrokenSiblings( oneOfAllSpans ); - - // Convert to set because we don't care about order. - const brokenSet = new Set( brokenArray ); + const brokenSet = writer.getAllClonedElements( oneOfAllSpans ); + const brokenArray = Array.from( brokenSet ); // Check if all spans are included. for ( const s of allSpans ) { @@ -315,6 +312,38 @@ describe( 'Writer', () => { expect( brokenArray.length ).to.equal( allSpans.length ); } } ); + + it( 'should return null if an element without id is given (even if it was broken)', () => { + const container = writer.createContainerElement( 'div' ); + const text = writer.createText( 'abccccde' ); + + writer.insert( ViewPosition.createAt( container, 0 ), text ); + + const span = writer.createAttributeElement( 'span' ); + span._priority = 20; + + //
abccccde
+ writer.wrap( ViewRange.createFromParentsAndOffsets( text, 2, text, 6 ), span ); + + const i = writer.createAttributeElement( 'i' ); + + //
abcccde
+ writer.wrap( + ViewRange.createFromParentsAndOffsets( + container.getChild( 0 ), 1, + container.getChild( 1 ).getChild( 0 ), 1 + ), + i + ); + + const spanFromView = container.getChild( 2 ); + + // Make sure a proper element was taken. + expect( spanFromView.is( 'span' ) ).to.be.true; + expect( spanFromView.id ).to.be.null; + + expect( writer.getAllClonedElements( spanFromView ) ).to.be.null; + } ); } ); function assertElementAttributes( element, attributes ) {