Przeglądaj źródła

Merge pull request #1719 from ckeditor/t/1716

Fix: `view.DowncastWriter` will now correctly wrap and unwrap nested attribute elements. Closes #1716. Closes ckeditor/ckeditor5-font#30.
Piotrek Koszuliński 6 lat temu
rodzic
commit
65b4cf35a6

+ 78 - 84
packages/ckeditor5-engine/src/view/downcastwriter.js

@@ -866,27 +866,6 @@ export default class DowncastWriter {
 
 		// Break attributes at range start and end.
 		const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
-
-		// Range around one element - check if AttributeElement can be unwrapped partially when it's not similar.
-		// For example:
-		// <b class="foo bar" title="baz"></b> unwrap with:	<b class="foo"></p> result: <b class"bar" title="baz"></b>
-		if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
-			const node = breakStart.nodeAfter;
-
-			// Unwrap single attribute element.
-			if ( !attribute.isSimilar( node ) && node instanceof AttributeElement && this._unwrapAttributeElement( attribute, node ) ) {
-				const start = this.mergeAttributes( breakStart );
-
-				if ( !start.isEqual( breakStart ) ) {
-					breakEnd.offset--;
-				}
-
-				const end = this.mergeAttributes( breakEnd );
-
-				return new Range( start, end );
-			}
-		}
-
 		const parentContainer = breakStart.parent;
 
 		// Unwrap children located between break points.
@@ -1085,16 +1064,16 @@ export default class DowncastWriter {
 	}
 
 	/**
-	 * Wraps children with provided `attribute`. Only children contained in `parent` element between
+	 * Wraps children with provided `wrapElement`. Only children contained in `parent` element between
 	 * `startOffset` and `endOffset` will be wrapped.
 	 *
 	 * @private
 	 * @param {module:engine/view/element~Element} parent
 	 * @param {Number} startOffset
 	 * @param {Number} endOffset
-	 * @param {module:engine/view/element~Element} attribute
+	 * @param {module:engine/view/element~Element} wrapElement
 	 */
-	_wrapChildren( parent, startOffset, endOffset, attribute ) {
+	_wrapChildren( parent, startOffset, endOffset, wrapElement ) {
 		let i = startOffset;
 		const wrapPositions = [];
 
@@ -1105,10 +1084,27 @@ export default class DowncastWriter {
 			const isEmpty = child.is( 'emptyElement' );
 			const isUI = child.is( 'uiElement' );
 
-			// Wrap text, empty elements, ui elements or attributes with higher or equal priority.
-			if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( attribute, child ) ) ) {
+			//
+			// (In all examples, assume that `wrapElement` is `<span class="foo">` element.)
+			//
+			// Check if `wrapElement` can be joined with the wrapped element. One of requirements is having same name.
+			// If possible, join elements.
+			//
+			// <p><span class="bar">abc</span></p>  -->  <p><span class="foo bar">abc</span></p>
+			//
+			if ( isAttribute && this._wrapAttributeElement( wrapElement, child ) ) {
+				wrapPositions.push(	new Position( parent, i ) );
+			}
+			//
+			// Wrap the child if it is not an attribute element or if it is an attribute element that should be inside
+			// `wrapElement` (due to priority).
+			//
+			// <p>abc</p>                   -->  <p><span class="foo">abc</span></p>
+			// <p><strong>abc</strong></p>  -->  <p><span class="foo"><strong>abc</strong></span></p>
+			//
+			else if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( wrapElement, child ) ) ) {
 				// Clone attribute.
-				const newAttribute = attribute._clone();
+				const newAttribute = wrapElement._clone();
 
 				// Wrap current node with new attribute.
 				child._remove();
@@ -1119,9 +1115,13 @@ export default class DowncastWriter {
 
 				wrapPositions.push(	new Position( parent, i ) );
 			}
-			// If other nested attribute is found start wrapping there.
+			//
+			// If other nested attribute is found and it wasn't wrapped (see above), continue wrapping inside it.
+			//
+			// <p><a href="foo.html">abc</a></p>  -->  <p><a href="foo.html"><span class="foo">abc</span></a></p>
+			//
 			else if ( isAttribute ) {
-				this._wrapChildren( child, 0, child.childCount, attribute );
+				this._wrapChildren( child, 0, child.childCount, wrapElement );
 			}
 
 			i++;
@@ -1151,25 +1151,40 @@ export default class DowncastWriter {
 	}
 
 	/**
-	 * Unwraps children from provided `attribute`. Only children contained in `parent` element between
+	 * Unwraps children from provided `unwrapElement`. Only children contained in `parent` element between
 	 * `startOffset` and `endOffset` will be unwrapped.
 	 *
 	 * @private
 	 * @param {module:engine/view/element~Element} parent
 	 * @param {Number} startOffset
 	 * @param {Number} endOffset
-	 * @param {module:engine/view/element~Element} attribute
+	 * @param {module:engine/view/element~Element} unwrapElement
 	 */
-	_unwrapChildren( parent, startOffset, endOffset, attribute ) {
+	_unwrapChildren( parent, startOffset, endOffset, unwrapElement ) {
 		let i = startOffset;
 		const unwrapPositions = [];
 
 		// Iterate over each element between provided offsets inside parent.
+		// We don't use tree walker or range iterator because we will be removing and merging potentially multiple nodes,
+		// so it could get messy. It is safer to it manually in this case.
 		while ( i < endOffset ) {
 			const child = parent.getChild( i );
 
-			// If attributes are the similar, then unwrap.
-			if ( child.isSimilar( attribute ) ) {
+			// Skip all text nodes. There should be no container element's here either.
+			if ( !child.is( 'attributeElement' ) ) {
+				i++;
+
+				continue;
+			}
+
+			//
+			// (In all examples, assume that `unwrapElement` is `<span class="foo">` element.)
+			//
+			// If the child is similar to the given attribute element, unwrap it - it will be completely removed.
+			//
+			// <p><span class="foo">abc</span>xyz</p>  -->  <p>abcxyz</p>
+			//
+			if ( child.isSimilar( unwrapElement ) ) {
 				const unwrapped = child.getChildren();
 				const count = child.childCount;
 
@@ -1185,18 +1200,39 @@ export default class DowncastWriter {
 					new Position( parent, i + count )
 				);
 
-				// Skip elements that were unwrapped. Assuming that there won't be another element to unwrap in child
-				// elements.
+				// Skip elements that were unwrapped. Assuming there won't be another element to unwrap in child elements.
 				i += count;
 				endOffset += count - 1;
-			} else {
-				// If other nested attribute is found start unwrapping there.
-				if ( child.is( 'attributeElement' ) ) {
-					this._unwrapChildren( child, 0, child.childCount, attribute );
-				}
+
+				continue;
+			}
+
+			//
+			// If the child is not similar but is an attribute element, try partial unwrapping - remove the same attributes/styles/classes.
+			// Partial unwrapping will happen only if the elements have the same name.
+			//
+			// <p><span class="foo bar">abc</span>xyz</p>  -->  <p><span class="bar">abc</span>xyz</p>
+			// <p><i class="foo">abc</i>xyz</p>            -->  <p><i class="foo">abc</i>xyz</p>
+			//
+			if ( this._unwrapAttributeElement( unwrapElement, child ) ) {
+				unwrapPositions.push(
+					new Position( parent, i ),
+					new Position( parent, i + 1 )
+				);
 
 				i++;
+
+				continue;
 			}
+
+			//
+			// If other nested attribute is found, look through it's children for elements to unwrap.
+			//
+			// <p><i><span class="foo">abc</span></i><p>  -->  <p><i>abc</i><p>
+			//
+			this._unwrapChildren( child, 0, child.childCount, unwrapElement );
+
+			i++;
 		}
 
 		// Merge at each unwrap.
@@ -1235,43 +1271,12 @@ export default class DowncastWriter {
 	 * @returns {module:engine/view/range~Range} New range after wrapping, spanning over wrapping attribute element.
 	 */
 	_wrapRange( range, attribute ) {
-		// Range is inside single attribute and spans on all children.
-		if ( rangeSpansOnAllChildren( range ) && this._wrapAttributeElement( attribute, range.start.parent ) ) {
-			const parent = range.start.parent;
-
-			const end = this.mergeAttributes( Position._createAfter( parent ) );
-			const start = this.mergeAttributes( Position._createBefore( parent ) );
-
-			return new Range( start, end );
-		}
-
 		// Break attributes at range start and end.
 		const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
-
-		// Range around one element.
-		if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
-			const node = breakStart.nodeAfter;
-
-			if ( node instanceof AttributeElement && this._wrapAttributeElement( attribute, node ) ) {
-				const start = this.mergeAttributes( breakStart );
-
-				if ( !start.isEqual( breakStart ) ) {
-					breakEnd.offset--;
-				}
-
-				const end = this.mergeAttributes( breakEnd );
-
-				return new Range( start, end );
-			}
-		}
-
 		const parentContainer = breakStart.parent;
 
-		// Unwrap children located between break points.
-		const unwrappedRange = this._unwrapChildren( parentContainer, breakStart.offset, breakEnd.offset, attribute );
-
 		// Wrap all children with attribute.
-		const newRange = this._wrapChildren( parentContainer, unwrappedRange.start.offset, unwrappedRange.end.offset, attribute );
+		const newRange = this._wrapChildren( parentContainer, breakStart.offset, breakEnd.offset, attribute );
 
 		// Merge attributes at the both ends and return a new range.
 		const start = this.mergeAttributes( newRange.start );
@@ -1805,17 +1810,6 @@ function mergeTextNodes( t1, t2 ) {
 	return new Position( t1, nodeBeforeLength );
 }
 
-// Returns `true` if range is located in same {@link module:engine/view/attributeelement~AttributeElement AttributeElement}
-// (`start` and `end` positions are located inside same {@link module:engine/view/attributeelement~AttributeElement AttributeElement}),
-// starts on 0 offset and ends after last child node.
-//
-// @param {module:engine/view/range~Range} Range
-// @returns {Boolean}
-function rangeSpansOnAllChildren( range ) {
-	return range.start.parent == range.end.parent && range.start.parent.is( 'attributeElement' ) &&
-		range.start.offset === 0 && range.end.offset === range.start.parent.childCount;
-}
-
 // Checks if provided nodes are valid to insert. Checks if each node is an instance of
 // {@link module:engine/view/text~Text Text} or {@link module:engine/view/attributeelement~AttributeElement AttributeElement},
 // {@link module:engine/view/containerelement~ContainerElement ContainerElement},

+ 2 - 2
packages/ckeditor5-engine/src/view/treewalker.js

@@ -240,7 +240,7 @@ export default class TreeWalker {
 				let charactersCount = node.data.length;
 				let item;
 
-				// If text stick out of walker range, we need to cut it and wrap by TextProxy.
+				// If text stick out of walker range, we need to cut it and wrap in TextProxy.
 				if ( node == this._boundaryEndParent ) {
 					charactersCount = this.boundaries.end.offset;
 					item = new TextProxy( node, 0, charactersCount );
@@ -352,7 +352,7 @@ export default class TreeWalker {
 				let charactersCount = node.data.length;
 				let item;
 
-				// If text stick out of walker range, we need to cut it and wrap by TextProxy.
+				// If text stick out of walker range, we need to cut it and wrap in TextProxy.
 				if ( node == this._boundaryStartParent ) {
 					const offset = this.boundaries.start.offset;
 

+ 50 - 0
packages/ckeditor5-engine/tests/view/downcastwriter/unwrap.js

@@ -144,6 +144,22 @@ describe( 'DowncastWriter', () => {
 			);
 		} );
 
+		it( 'should unwrap a part of a nested attribute', () => {
+			test(
+				'<container:p>' +
+					'<attribute:u view-priority="1"><attribute:b view-priority="1">fo{ob}ar</attribute:b></attribute:u>' +
+				'</container:p>',
+				'<attribute:b view-priority="1"></attribute:b>',
+				'<container:p>' +
+					'<attribute:u view-priority="1">' +
+						'<attribute:b view-priority="1">fo</attribute:b>' +
+						'[ob]' +
+						'<attribute:b view-priority="1">ar</attribute:b>' +
+					'</attribute:u>' +
+				'</container:p>'
+			);
+		} );
+
 		it( 'should merge unwrapped nodes #1', () => {
 			test(
 				'<container:p>foo[<attribute:b view-priority="1">bar</attribute:b>]baz</container:p>',
@@ -313,6 +329,40 @@ describe( 'DowncastWriter', () => {
 			);
 		} );
 
+		it( 'should partially unwrap a nested attribute', () => {
+			test(
+				'<container:p>' +
+					'[<attribute:i view-priority="1">' +
+						'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px;">test</attribute:b>' +
+					'</attribute:i>]' +
+				'</container:p>',
+				'<attribute:b view-priority="1" style="position: absolute;"></attribute:b>',
+				'<container:p>' +
+					'[<attribute:i view-priority="1">' +
+						'<attribute:b view-priority="1" style="color:red;top:10px">test</attribute:b>' +
+					'</attribute:i>]' +
+				'</container:p>'
+			);
+		} );
+
+		it( 'should partially unwrap a part of a nested attribute', () => {
+			test(
+				'<container:p>' +
+					'<attribute:i view-priority="1">' +
+						'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px;">t{es}t</attribute:b>' +
+					'</attribute:i>' +
+				'</container:p>',
+				'<attribute:b view-priority="1" style="position: absolute;"></attribute:b>',
+				'<container:p>' +
+					'<attribute:i view-priority="1">' +
+						'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px">t</attribute:b>' +
+						'[<attribute:b view-priority="1" style="color:red;top:10px">es</attribute:b>]' +
+						'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px">t</attribute:b>' +
+					'</attribute:i>' +
+				'</container:p>'
+			);
+		} );
+
 		it( 'should be merged after being partially unwrapped', () => {
 			test(
 				'<container:p>' +

+ 38 - 0
packages/ckeditor5-engine/tests/view/downcastwriter/wrap.js

@@ -119,6 +119,44 @@ describe( 'DowncastWriter', () => {
 				);
 			} );
 
+			it( 'should join wrapping element with a nested attribute element', () => {
+				test(
+					'<container:p>' +
+						'<attribute:u view-priority="1">' +
+							'<attribute:b view-priority="2" class="bar">{foo}</attribute:b>' +
+						'</attribute:u>' +
+					'</container:p>',
+
+					'<attribute:b class="foo" view-priority="2"></attribute:b>',
+
+					'<container:p>' +
+						'[<attribute:u view-priority="1">' +
+							'<attribute:b view-priority="2" class="bar foo">foo</attribute:b>' +
+						'</attribute:u>]' +
+					'</container:p>'
+				);
+			} );
+
+			it( 'should join wrapping element with a part of a nested attribute element', () => {
+				test(
+					'<container:p>' +
+						'<attribute:i view-priority="1">' +
+							'<attribute:b view-priority="2" class="bar">fo{ob}ar</attribute:b>' +
+						'</attribute:i>' +
+					'</container:p>',
+
+					'<attribute:b class="foo" view-priority="2"></attribute:b>',
+
+					'<container:p>' +
+						'<attribute:i view-priority="1">' +
+							'<attribute:b view-priority="2" class="bar">fo</attribute:b>' +
+							'[<attribute:b view-priority="2" class="bar foo">ob</attribute:b>]' +
+							'<attribute:b view-priority="2" class="bar">ar</attribute:b>' +
+						'</attribute:i>' +
+					'</container:p>'
+				);
+			} );
+
 			it( 'wraps part of a single text node #3', () => {
 				test(
 					'<container:p>foo{bar}</container:p>',