Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Commit 4126359

Browse files
ReinmarReinmar
authored
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.
2 parents b3f5da3 + 3ebfe84 commit 4126359

File tree

4 files changed

+168
-86
lines changed

4 files changed

+168
-86
lines changed

‎src/view/downcastwriter.js

Lines changed: 78 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -866,27 +866,6 @@ export default class DowncastWriter {
866866

867867
// Break attributes at range start and end.
868868
const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
869-
870-
// Range around one element - check if AttributeElement can be unwrapped partially when it's not similar.
871-
// For example:
872-
// <b class="foo bar" title="baz"></b> unwrap with: <b class="foo"></p> result: <b class"bar" title="baz"></b>
873-
if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
874-
const node = breakStart.nodeAfter;
875-
876-
// Unwrap single attribute element.
877-
if ( !attribute.isSimilar( node ) && node instanceof AttributeElement && this._unwrapAttributeElement( attribute, node ) ) {
878-
const start = this.mergeAttributes( breakStart );
879-
880-
if ( !start.isEqual( breakStart ) ) {
881-
breakEnd.offset--;
882-
}
883-
884-
const end = this.mergeAttributes( breakEnd );
885-
886-
return new Range( start, end );
887-
}
888-
}
889-
890869
const parentContainer = breakStart.parent;
891870

892871
// Unwrap children located between break points.
@@ -1085,16 +1064,16 @@ export default class DowncastWriter {
10851064
}
10861065

10871066
/**
1088-
* Wraps children with provided `attribute`. Only children contained in `parent` element between
1067+
* Wraps children with provided `wrapElement`. Only children contained in `parent` element between
10891068
* `startOffset` and `endOffset` will be wrapped.
10901069
*
10911070
* @private
10921071
* @param {module:engine/view/element~Element} parent
10931072
* @param {Number} startOffset
10941073
* @param {Number} endOffset
1095-
* @param {module:engine/view/element~Element} attribute
1074+
* @param {module:engine/view/element~Element} wrapElement
10961075
*/
1097-
_wrapChildren( parent, startOffset, endOffset, attribute ) {
1076+
_wrapChildren( parent, startOffset, endOffset, wrapElement ) {
10981077
let i = startOffset;
10991078
const wrapPositions = [];
11001079

@@ -1105,10 +1084,27 @@ export default class DowncastWriter {
11051084
const isEmpty = child.is( 'emptyElement' );
11061085
const isUI = child.is( 'uiElement' );
11071086

1108-
// Wrap text, empty elements, ui elements or attributes with higher or equal priority.
1109-
if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( attribute, child ) ) ) {
1087+
//
1088+
// (In all examples, assume that `wrapElement` is `<span class="foo">` element.)
1089+
//
1090+
// Check if `wrapElement` can be joined with the wrapped element. One of requirements is having same name.
1091+
// If possible, join elements.
1092+
//
1093+
// <p><span class="bar">abc</span></p> --> <p><span class="foo bar">abc</span></p>
1094+
//
1095+
if ( isAttribute && this._wrapAttributeElement( wrapElement, child ) ) {
1096+
wrapPositions.push( new Position( parent, i ) );
1097+
}
1098+
//
1099+
// Wrap the child if it is not an attribute element or if it is an attribute element that should be inside
1100+
// `wrapElement` (due to priority).
1101+
//
1102+
// <p>abc</p> --> <p><span class="foo">abc</span></p>
1103+
// <p><strong>abc</strong></p> --> <p><span class="foo"><strong>abc</strong></span></p>
1104+
//
1105+
else if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( wrapElement, child ) ) ) {
11101106
// Clone attribute.
1111-
const newAttribute = attribute._clone();
1107+
const newAttribute = wrapElement._clone();
11121108

11131109
// Wrap current node with new attribute.
11141110
child._remove();
@@ -1119,9 +1115,13 @@ export default class DowncastWriter {
11191115

11201116
wrapPositions.push( new Position( parent, i ) );
11211117
}
1122-
// If other nested attribute is found start wrapping there.
1118+
//
1119+
// If other nested attribute is found and it wasn't wrapped (see above), continue wrapping inside it.
1120+
//
1121+
// <p><a href="foo.html">abc</a></p> --> <p><a href="foo.html"><span class="foo">abc</span></a></p>
1122+
//
11231123
else if ( isAttribute ) {
1124-
this._wrapChildren( child, 0, child.childCount, attribute );
1124+
this._wrapChildren( child, 0, child.childCount, wrapElement );
11251125
}
11261126

11271127
i++;
@@ -1151,25 +1151,40 @@ export default class DowncastWriter {
11511151
}
11521152

11531153
/**
1154-
* Unwraps children from provided `attribute`. Only children contained in `parent` element between
1154+
* Unwraps children from provided `unwrapElement`. Only children contained in `parent` element between
11551155
* `startOffset` and `endOffset` will be unwrapped.
11561156
*
11571157
* @private
11581158
* @param {module:engine/view/element~Element} parent
11591159
* @param {Number} startOffset
11601160
* @param {Number} endOffset
1161-
* @param {module:engine/view/element~Element} attribute
1161+
* @param {module:engine/view/element~Element} unwrapElement
11621162
*/
1163-
_unwrapChildren( parent, startOffset, endOffset, attribute ) {
1163+
_unwrapChildren( parent, startOffset, endOffset, unwrapElement ) {
11641164
let i = startOffset;
11651165
const unwrapPositions = [];
11661166

11671167
// Iterate over each element between provided offsets inside parent.
1168+
// We don't use tree walker or range iterator because we will be removing and merging potentially multiple nodes,
1169+
// so it could get messy. It is safer to it manually in this case.
11681170
while ( i < endOffset ) {
11691171
const child = parent.getChild( i );
11701172

1171-
// If attributes are the similar, then unwrap.
1172-
if ( child.isSimilar( attribute ) ) {
1173+
// Skip all text nodes. There should be no container element's here either.
1174+
if ( !child.is( 'attributeElement' ) ) {
1175+
i++;
1176+
1177+
continue;
1178+
}
1179+
1180+
//
1181+
// (In all examples, assume that `unwrapElement` is `<span class="foo">` element.)
1182+
//
1183+
// If the child is similar to the given attribute element, unwrap it - it will be completely removed.
1184+
//
1185+
// <p><span class="foo">abc</span>xyz</p> --> <p>abcxyz</p>
1186+
//
1187+
if ( child.isSimilar( unwrapElement ) ) {
11731188
const unwrapped = child.getChildren();
11741189
const count = child.childCount;
11751190

@@ -1185,18 +1200,39 @@ export default class DowncastWriter {
11851200
new Position( parent, i + count )
11861201
);
11871202

1188-
// Skip elements that were unwrapped. Assuming that there won't be another element to unwrap in child
1189-
// elements.
1203+
// Skip elements that were unwrapped. Assuming there won't be another element to unwrap in child elements.
11901204
i += count;
11911205
endOffset += count - 1;
1192-
} else {
1193-
// If other nested attribute is found start unwrapping there.
1194-
if ( child.is( 'attributeElement' ) ) {
1195-
this._unwrapChildren( child, 0, child.childCount, attribute );
1196-
}
1206+
1207+
continue;
1208+
}
1209+
1210+
//
1211+
// If the child is not similar but is an attribute element, try partial unwrapping - remove the same attributes/styles/classes.
1212+
// Partial unwrapping will happen only if the elements have the same name.
1213+
//
1214+
// <p><span class="foo bar">abc</span>xyz</p> --> <p><span class="bar">abc</span>xyz</p>
1215+
// <p><i class="foo">abc</i>xyz</p> --> <p><i class="foo">abc</i>xyz</p>
1216+
//
1217+
if ( this._unwrapAttributeElement( unwrapElement, child ) ) {
1218+
unwrapPositions.push(
1219+
new Position( parent, i ),
1220+
new Position( parent, i + 1 )
1221+
);
11971222

11981223
i++;
1224+
1225+
continue;
11991226
}
1227+
1228+
//
1229+
// If other nested attribute is found, look through it's children for elements to unwrap.
1230+
//
1231+
// <p><i><span class="foo">abc</span></i><p> --> <p><i>abc</i><p>
1232+
//
1233+
this._unwrapChildren( child, 0, child.childCount, unwrapElement );
1234+
1235+
i++;
12001236
}
12011237

12021238
// Merge at each unwrap.
@@ -1235,43 +1271,12 @@ export default class DowncastWriter {
12351271
* @returns {module:engine/view/range~Range} New range after wrapping, spanning over wrapping attribute element.
12361272
*/
12371273
_wrapRange( range, attribute ) {
1238-
// Range is inside single attribute and spans on all children.
1239-
if ( rangeSpansOnAllChildren( range ) && this._wrapAttributeElement( attribute, range.start.parent ) ) {
1240-
const parent = range.start.parent;
1241-
1242-
const end = this.mergeAttributes( Position._createAfter( parent ) );
1243-
const start = this.mergeAttributes( Position._createBefore( parent ) );
1244-
1245-
return new Range( start, end );
1246-
}
1247-
12481274
// Break attributes at range start and end.
12491275
const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
1250-
1251-
// Range around one element.
1252-
if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
1253-
const node = breakStart.nodeAfter;
1254-
1255-
if ( node instanceof AttributeElement && this._wrapAttributeElement( attribute, node ) ) {
1256-
const start = this.mergeAttributes( breakStart );
1257-
1258-
if ( !start.isEqual( breakStart ) ) {
1259-
breakEnd.offset--;
1260-
}
1261-
1262-
const end = this.mergeAttributes( breakEnd );
1263-
1264-
return new Range( start, end );
1265-
}
1266-
}
1267-
12681276
const parentContainer = breakStart.parent;
12691277

1270-
// Unwrap children located between break points.
1271-
const unwrappedRange = this._unwrapChildren( parentContainer, breakStart.offset, breakEnd.offset, attribute );
1272-
12731278
// Wrap all children with attribute.
1274-
const newRange = this._wrapChildren( parentContainer, unwrappedRange.start.offset, unwrappedRange.end.offset, attribute );
1279+
const newRange = this._wrapChildren( parentContainer, breakStart.offset, breakEnd.offset, attribute );
12751280

12761281
// Merge attributes at the both ends and return a new range.
12771282
const start = this.mergeAttributes( newRange.start );
@@ -1805,17 +1810,6 @@ function mergeTextNodes( t1, t2 ) {
18051810
return new Position( t1, nodeBeforeLength );
18061811
}
18071812

1808-
// Returns `true` if range is located in same {@link module:engine/view/attributeelement~AttributeElement AttributeElement}
1809-
// (`start` and `end` positions are located inside same {@link module:engine/view/attributeelement~AttributeElement AttributeElement}),
1810-
// starts on 0 offset and ends after last child node.
1811-
//
1812-
// @param {module:engine/view/range~Range} Range
1813-
// @returns {Boolean}
1814-
function rangeSpansOnAllChildren( range ) {
1815-
return range.start.parent == range.end.parent && range.start.parent.is( 'attributeElement' ) &&
1816-
range.start.offset === 0 && range.end.offset === range.start.parent.childCount;
1817-
}
1818-
18191813
// Checks if provided nodes are valid to insert. Checks if each node is an instance of
18201814
// {@link module:engine/view/text~Text Text} or {@link module:engine/view/attributeelement~AttributeElement AttributeElement},
18211815
// {@link module:engine/view/containerelement~ContainerElement ContainerElement},

‎src/view/treewalker.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ export default class TreeWalker {
240240
let charactersCount = node.data.length;
241241
let item;
242242

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

355-
// If text stick out of walker range, we need to cut it and wrap by TextProxy.
355+
// If text stick out of walker range, we need to cut it and wrap in TextProxy.
356356
if ( node == this._boundaryStartParent ) {
357357
const offset = this.boundaries.start.offset;
358358

‎tests/view/downcastwriter/unwrap.js

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,22 @@ describe( 'DowncastWriter', () => {
144144
);
145145
} );
146146

147+
it( 'should unwrap a part of a nested attribute', () => {
148+
test(
149+
'<container:p>' +
150+
'<attribute:u view-priority="1"><attribute:b view-priority="1">fo{ob}ar</attribute:b></attribute:u>' +
151+
'</container:p>',
152+
'<attribute:b view-priority="1"></attribute:b>',
153+
'<container:p>' +
154+
'<attribute:u view-priority="1">' +
155+
'<attribute:b view-priority="1">fo</attribute:b>' +
156+
'[ob]' +
157+
'<attribute:b view-priority="1">ar</attribute:b>' +
158+
'</attribute:u>' +
159+
'</container:p>'
160+
);
161+
} );
162+
147163
it( 'should merge unwrapped nodes #1', () => {
148164
test(
149165
'<container:p>foo[<attribute:b view-priority="1">bar</attribute:b>]baz</container:p>',
@@ -313,6 +329,40 @@ describe( 'DowncastWriter', () => {
313329
);
314330
} );
315331

332+
it( 'should partially unwrap a nested attribute', () => {
333+
test(
334+
'<container:p>' +
335+
'[<attribute:i view-priority="1">' +
336+
'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px;">test</attribute:b>' +
337+
'</attribute:i>]' +
338+
'</container:p>',
339+
'<attribute:b view-priority="1" style="position: absolute;"></attribute:b>',
340+
'<container:p>' +
341+
'[<attribute:i view-priority="1">' +
342+
'<attribute:b view-priority="1" style="color:red;top:10px">test</attribute:b>' +
343+
'</attribute:i>]' +
344+
'</container:p>'
345+
);
346+
} );
347+
348+
it( 'should partially unwrap a part of a nested attribute', () => {
349+
test(
350+
'<container:p>' +
351+
'<attribute:i view-priority="1">' +
352+
'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px;">t{es}t</attribute:b>' +
353+
'</attribute:i>' +
354+
'</container:p>',
355+
'<attribute:b view-priority="1" style="position: absolute;"></attribute:b>',
356+
'<container:p>' +
357+
'<attribute:i view-priority="1">' +
358+
'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px">t</attribute:b>' +
359+
'[<attribute:b view-priority="1" style="color:red;top:10px">es</attribute:b>]' +
360+
'<attribute:b view-priority="1" style="color:red;position:absolute;top:10px">t</attribute:b>' +
361+
'</attribute:i>' +
362+
'</container:p>'
363+
);
364+
} );
365+
316366
it( 'should be merged after being partially unwrapped', () => {
317367
test(
318368
'<container:p>' +

‎tests/view/downcastwriter/wrap.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,44 @@ describe( 'DowncastWriter', () => {
119119
);
120120
} );
121121

122+
it( 'should join wrapping element with a nested attribute element', () => {
123+
test(
124+
'<container:p>' +
125+
'<attribute:u view-priority="1">' +
126+
'<attribute:b view-priority="2" class="bar">{foo}</attribute:b>' +
127+
'</attribute:u>' +
128+
'</container:p>',
129+
130+
'<attribute:b class="foo" view-priority="2"></attribute:b>',
131+
132+
'<container:p>' +
133+
'[<attribute:u view-priority="1">' +
134+
'<attribute:b view-priority="2" class="bar foo">foo</attribute:b>' +
135+
'</attribute:u>]' +
136+
'</container:p>'
137+
);
138+
} );
139+
140+
it( 'should join wrapping element with a part of a nested attribute element', () => {
141+
test(
142+
'<container:p>' +
143+
'<attribute:i view-priority="1">' +
144+
'<attribute:b view-priority="2" class="bar">fo{ob}ar</attribute:b>' +
145+
'</attribute:i>' +
146+
'</container:p>',
147+
148+
'<attribute:b class="foo" view-priority="2"></attribute:b>',
149+
150+
'<container:p>' +
151+
'<attribute:i view-priority="1">' +
152+
'<attribute:b view-priority="2" class="bar">fo</attribute:b>' +
153+
'[<attribute:b view-priority="2" class="bar foo">ob</attribute:b>]' +
154+
'<attribute:b view-priority="2" class="bar">ar</attribute:b>' +
155+
'</attribute:i>' +
156+
'</container:p>'
157+
);
158+
} );
159+
122160
it( 'wraps part of a single text node #3', () => {
123161
test(
124162
'<container:p>foo{bar}</container:p>',

0 commit comments

Comments
 (0)