8000 TINY-10869: Improve merging inserted nested inline elements (#9658) · tinymce/tinymce@465fbbe · GitHub
[go: up one dir, main page]
Skip to content

Commit 465fbbe

Browse files
MitchC1999MitchC1999
authored
TINY-10869: Improve merging inserted nested inline elements (#9658)
* TINY-10869: Recursively trim style-matched nodes on insert * TINY-10869: Add tests for merging spans * TINY-10869: Recursively check all parents for identical node before removing * TINY-10869: Add more tests * TINY-10869: Add changelog entry * TINY-10869: Review recommendations * TINY-10869: Improve tests * TINY-10869: Add more tests * TINY-10869: Improve test names * TINY-10869: Add more test runs * TINY-10869: Fix wrong test runs * TINY-10869: Improve merge conditions * TINY-10869: Add tests for non-inheritable styles and multiple children * TINY-10869: Improve changelog entry
1 parent 647845a commit 465fbbe

File tree

5 files changed

+452
-37
lines changed

5 files changed

+452
-37
lines changed

‎.changes/unreleased/tinymce-TINY-10869-2024-06-04.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
project: tinymce
2+
kind: Improved
3+
body: Improve merging of inserted inline elements by removing nodes with redundant
4+
inheritable styles.
5+
time: 2024-06-04T17:59:27.418577+10:00
6+
custom:
7+
Issue: TINY-10869

‎modules/tinymce/src/core/main/ts/api/html/StyleUtils.ts

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const nonInheritableStyles: Set<string> = new Set();
99
'margin', 'margin-left', 'margin-right', 'margin-top', 'margin-bottom',
1010
'padding', 'padding-left', 'padding-right', 'padding-top', 'padding-bottom',
1111
'border', 'border-width', 'border-style', 'border-color',
12-
'background', 'background-attachment', 'background-clip', 'background-color',
12+
'background', 'background-attachment', 'background-clip',
1313
'background-image', 'background-origin', 'background-position', 'background-repeat', 'background-size',
1414
'float', 'position', 'left', 'right', 'top', 'bottom',
1515
'z-index', 'display', 'transform',
@@ -22,17 +22,34 @@ const nonInheritableStyles: Set<string> = new Set();
2222
});
2323
})();
2424

25+
const conditionalNonInheritableStyles: Set<string> = new Set();
26+
(() => {
27+
// These styles are only noninheritable when applied to an element with a noninheritable style
28+ // For example, background-color is visible on an element with padding, even when children have background-color;
29+
// however, when the element has no padding, background-color is either visible or overridden by children
30+
const conditionalNonInheritableStylesArr = [
31+
'background-color'
32+
];
33+
Arr.each(conditionalNonInheritableStylesArr, (style) => {
34+
conditionalNonInheritableStyles.add(style);
35+
});
36+
})();
37+
2538
// TODO: TINY-7326 Figure out what else should be added to the shorthandStyleProps list
2639
// Does not include non-inherited shorthand style properties
2740
const shorthandStyleProps = [ 'font', 'text-decoration', 'text-emphasis' ];
2841

29-
const getStyleProps = (dom: DOMUtils, node: Element) =>
30-
Obj.keys(dom.parseStyle(dom.getAttrib(node, 'style')));
42+
const getStyles = (dom: DOMUtils, node: Element): Record<string, string> => dom.parseStyle(dom.getAttrib(node, 'style'));
43+
const getStyleProps = (dom: DOMUtils, node: Element): string[] => Obj.keys(getStyles(dom, node));
3144

3245
const isNonInheritableStyle = (style: string) => nonInheritableStyles.has(style);
46+
const isConditionalNonInheritableStyle = (style: string) => conditionalNonInheritableStyles.has(style);
47+
48+
const hasNonInheritableStyles = (dom: DOMUtils, node: Element): boolean =>
49+
Arr.exists(getStyleProps(dom, node), (style) => isNonInheritableStyle(style));
3350

34-
const hasInheritableStyles = (dom: DOMUtils, node: Element): boolean =>
35-
Arr.forall(getStyleProps(dom, node), (style) => !isNonInheritableStyle(style));
51+
const hasConditionalNonInheritableStyles = (dom: DOMUtils, node: Element): boolean => hasNonInheritableStyles(dom, node) &&
52+
Arr.exists(getStyleProps(dom, node), (style) => isConditionalNonInheritableStyle(style));
3653

3754
const getLonghandStyleProps = (styles: string[]): string[] =>
3855
Arr.filter(styles, (style) => Arr.exists(shorthandStyleProps, (prop) => Strings.startsWith(style, prop)));
@@ -61,6 +78,8 @@ const hasStyleConflict = (dom: DOMUtils, node: Element, 341A parentNode: Element): bo
6178
};
6279

6380
export {
64-
hasInheritableStyles,
81+
getStyleProps,
82+
hasNonInheritableStyles,
83+
hasConditionalNonInheritableStyles,
6584
hasStyleConflict
6685
};

‎modules/tinymce/src/core/main/ts/content/InsertContentImpl.ts

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -88,21 +88,28 @@ const reduceInlineTextElements = (editor: Editor, merge: boolean | undefined): v
8888
const root = editor.getBody();
8989
const elementUtils = ElementUtils(editor);
9090

91-
Tools.each(dom.select('*[data-mce-fragment]'), (node) => {
92-
const isInline = Type.isNonNullable(textInlineElements[node.nodeName.toLowerCase()]);
93-
if (isInline && StyleUtils.hasInheritableStyles(dom, node)) {
94-
for (let parentNode = node.parentElement; Type.isNonNullable(parentNode) && parentNode !== root; parentNode = parentNode.parentElement) {
95-
// Check if the parent has a style conflict that would prevent the child node from being safely removed,
96-
// even if a exact node match could be found further up the tree
97-
const styleConflict = StyleUtils.hasStyleConflict(dom, node, parentNode);
98-
if (styleConflict) {
99-
break;
100-
}
101-
102-
if (elementUtils.compare(parentNode, node)) {
103-
dom.remove(node, true);
104-
break;
105-
}
91+
const fragmentSelector = '*[data-mce-fragment]';
92+
const fragments = dom.select(fragmentSelector);
93+
94+
Tools.each(fragments, (node) => {
95+
const isInline = (currentNode: Element) => Type.isNonNullable(textInlineElements[currentNode.nodeName.toLowerCase()]);
96+
const hasOneChild = (currentNode: Element) => currentNode.childNodes.length === 1;
97+
const hasNoNonInheritableStyles = (currentNode: Element) => !(StyleUtils.hasNonInheritableStyles(dom, currentNode) || StyleUtils.hasConditionalNonInheritableStyles(dom, currentNode));
98+
99+
if (hasNoNonInheritableStyles(node) && isInline(node) && hasOneChild(node)) {
100+
const styles = StyleUtils.getStyleProps(dom, node);
101+
const isOverridden = (oldStyles: string[], newStyles: string[]) => Arr.forall(oldStyles, (style) => Arr.contains(newStyles, style));
102+
const overriddenByAllChildren = (childNode: Element): boolean => hasOneChild(node) && dom.is(childNode, fragmentSelector) && isInline(childNode) &&
103+
(childNode.nodeName === node.nodeName && isOverridden(styles, StyleUtils.getStyleProps(dom, childNode)) || overriddenByAllChildren(childNode.children[0]));
104+
105+
const identicalToParent = (parentNode: Element | null): boolean => Type.isNonNullable(parentNode) && parentNode !== root
106+
&& (elementUtils.compare(node, parentNode) || identicalToParent(parentNode.parentElement));
107+
108+
const conflictWithInsertedParent = (parentNode: Element | null): boolean => Type.isNonNullable(parentNode) && parentNode !== root
109+
&& dom.is(parentNode, fragmentSelector) && (StyleUtils.hasStyleConflict(dom, node, parentNode) || conflictWithInsertedParent(parentNode.parentElement));
110+
111+
if (overriddenByAllChildren(node.children[0]) || (identicalToParent(node.parentElement) && !conflictWithInsertedParent(node.parentElement))) {
112+
dom.remove(node, true);
106113
}
107114
}
108115
});

‎modules/tinymce/src/core/test/ts/browser/content/insert/InsertContentTest.ts

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -383,11 +383,7 @@ describe('browser.tinymce.core.content.insert.InsertContentTest', () => {
383383
merge: true
384384
});
385385
TinyAssertions.assertContent(editor, '<p>' +
386-
'<span style="font-size: 9pt;">' +
387-
'<span style="font-size: 14pt;">' +
388386
'<span style="font-size: 9pt;">test</span>' +
389-
'</span>' +
390-
'</span>' +
391387
'</p>');
392388
});
393389

@@ -406,8 +402,8 @@ describe('browser.tinymce.core.content.insert.InsertContentTest', () => {
406402
merge: true
407403
});
408404
TinyAssertions.assertContent(editor, '<p>' +
409-
'<span style="color: red; font-size: 9pt;">' +
410-
'<span style="background-color: red; color: red;">test</span>' +
405+
'<span style="background-color: red; color: red;">' +
406+
'<span style="color: red; font-size: 9pt;">test</span>' +
411407
'</span>' +
412408
'</p>');
413409
});
@@ -435,11 +431,13 @@ describe('browser.tinymce.core.content.insert.InsertContentTest', () => {
435431
merge: true
436432
});
437433
TinyAssertions.assertContent(editor, '<p>' +
438-
'<span style="color: yellow;">' +
439434
'<span style="background-color: red;">' +
435+
'<span style="color: yellow;">' +
440436
'<span style="color: red;">red</span>' +
441437
'yellow' +
442-
'<span style="color: blue;"><strong>blue</strong></span>' +
438+
'<strong>' +
439+
'<span style="color: blue;">blue</span>' +
440+
'</strong>' +
443441
'</span>' +
444442
'</span>' +
445443
'</p>');
@@ -504,16 +502,14 @@ describe('browser.tinymce.core.content.insert.InsertContentTest', () => {
504502
merge: true
505503
});
506504
TinyAssertions.assertContent(editor, '<p>' +
507-
'<span style="font: italic 10px sans-serif;">' +
508-
'<span style="font-size: 10px;">test</span>' +
505+
'<span style="font-size: 10px;">' +
506+
'<span style="font: italic 10px sans-serif;">test</span>' +
509507
'</span>' +
510508
'</p>' +
511509
'<p>' +
512-
'<span style="font: italic 10px sans-serif;">' +
513510
'<span style="font-size: 12px;">' +
514511
'<span style="font: italic 10px sans-serif;">test</span>' +
515512
'</span>' +
516-
'</span>' +
517513
'</p>');
518514
});
519515

@@ -541,16 +537,14 @@ describe('browser.tinymce.core.content.insert.InsertContentTest', () => {
541537
merge: true
542538
});
543539
TinyAssertions.assertContent(editor, '<p>' +
544-
'<span style="font-style: italic;">' +
545-
'<span style="font: italic 12px sans-serif;">test</span>' +
540+
'<span style="font: italic 12px sans-serif;">' +
541+
'<span style="font-style: italic;">test</span>' +
546542
'</span>' +
547543
'</p>' +
548544
'<p>' +
549-
'<span style="font-size: 10px;">' +
550545
'<span style="font: italic 12px sans-serif;">' +
551546
'<span style="font-size: 10px;">test</span>' +
552547
'</span>' +
553-
'</span>' +
554548
'</p>');
555549
});
556550

0 commit comments

Comments
 (0)
0