Merge pull request #3952 from CatoTH/OpenSlides-3-Diff-Bugfixes

Diff bugfixes: port of #3946 & #3943 - fixes #3944

client/src/app/site/motions/services/diff.service.spec.ts

@@ -805,7 +805,7 @@ describe('DiffService', () => {
                 after = '';
             const diff = service.diff(before, after);
             expect(diff).toBe(
-                '<P class="delete">Ihr könnt ohne Sorge fortgehen.\'Da meckerte die Alte und machte sich getrost auf den Weg.</P>'
+                '<p class="delete">Ihr könnt ohne Sorge fortgehen.\'Da meckerte die Alte und machte sich getrost auf den Weg.</p>'
             );
         }));
 
@@ -953,6 +953,20 @@ describe('DiffService', () => {
                     'Gegenüber</p>'
             );
         }));
+
+        it('does not delete a paragraph before an inserted one', inject([DiffService], (service: DiffService) => {
+            const inHtml = '<ul class="os-split-before"><li>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.</li>\n' +
+                '</ul>',
+                outHtml = '<ul class="os-split-before">\n' +
+                    '<li>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.</li>\n' +
+                    '<li class="testclass">At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</li>\n' +
+                    '</ul>';
+            const diff = service.diff(inHtml, outHtml);
+            expect(diff).toBe('<ul class="os-split-before">' +
+                '<li>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.</li>' +
+                '<li class="testclass insert">At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</li>' +
+                '</ul>');
+        }));
     });
 
     describe('ignoring line numbers', () => {
@@ -1028,6 +1042,17 @@ describe('DiffService', () => {
             }
         ));
 
+        it('works with a replaced list item', inject([DiffService], (service: DiffService) => {
+            const before = "<ul><li>Lorem ipsum <strong>dolor sit amet</strong>, consetetur sadipscing elitr, sed diam nonumy eirmod tempor.</li></ul>",
+                after = "<ul>\n<li>\n<p>At vero eos et accusam et justo duo dolores et ea rebum.</p>\n</li>\n</ul>\n",
+                expected = '<UL class="delete"><LI>' + noMarkup(1) + 'Lorem ipsum <STRONG>dolor sit amet</STRONG>, consetetur sadipscing elitr, sed diam nonumy ' + brMarkup(2) + 'eirmod tempor.</LI></UL>' +
+                    "<UL class=\"insert\">\n<LI>\n<P>At vero eos et accusam et justo duo dolores et ea rebum.</P>\n</LI>\n</UL>";
+            const diff = service.diff(before, after, 80, 1);
+            const diffNormalized = service.normalizeHtmlForDiff(diff).toLowerCase();
+            const expectedNormalized = service.normalizeHtmlForDiff(expected).toLowerCase();
+            expect(diffNormalized).toBe(expectedNormalized);
+        }));
+
         it('detects broken HTML and lowercases class names', inject([DiffService], (service: DiffService) => {
             const before =
                     '<p><span class="line-number-3 os-line-number" data-line-number="3" contenteditable="false">&nbsp;</span>holen, da rief sie alle sieben herbei und sprach:</p>\n\n<p><span class="line-number-4 os-line-number" data-line-number="4" contenteditable="false">&nbsp;</span><span style="color: #000000;">"Liebe Kinder, ich will hinaus in den Wald, seid auf der Hut vor dem Wolf! Wenn er <br class="os-line-break"><span class="line-number-5 os-line-number" data-line-number="5" contenteditable="false">&nbsp;</span>hereinkommt, frisst er euch alle mit Haut und Haar. Der Bösewicht verstellt sich oft, aber <br class="os-line-break"><span class="line-number-6 os-line-number" data-line-number="6" contenteditable="false">&nbsp;</span>an der rauen Stimme und an seinen schwarzen Füßen werdet ihr ihn schon erkennen."</span></p>\n\n<p><span class="line-number-7 os-line-number" data-line-number="7" contenteditable="false">&nbsp;</span>Die Geißlein sagten: " Liebe Mutter, wir wollen uns schon in acht nehmen, du kannst ohne </p>',

client/src/app/site/motions/services/diff.service.ts

@@ -1709,18 +1709,14 @@ export class DiffService {
                 null,
                 firstLineNumber
             );
-            newTextWithBreaks = this.lineNumberingService.insertLineNumbersNode(
+            newText = this.lineNumberingService.insertLineBreaksWithoutNumbers(newText, lineLength);
-                newText,
-                lineLength,
-                null,
-                firstLineNumber
-            );
         } else {
             oldTextWithBreaks = document.createElement('div');
             oldTextWithBreaks.innerHTML = oldText;
-            newTextWithBreaks = document.createElement('div');
-            newTextWithBreaks.innerHTML = newText;
         }
+        newText = newText.replace(/^\s+/g, '').replace(/\s+$/g, '');
+        newTextWithBreaks = document.createElement('div');
+        newTextWithBreaks.innerHTML = newText;
 
         for (let i = 0; i < oldTextWithBreaks.childNodes.length; i++) {
             currChild = oldTextWithBreaks.childNodes[i];
@@ -1923,6 +1919,31 @@ export class DiffService {
             }
         );
 
+        // <ins><li>...</li></ins> => <li class="insert">...</li>
+        diffUnnormalized = diffUnnormalized.replace(
+            /<(ins|del)><(p|div|blockquote|li)([^>]*)>([\s\S]*)<\/\2><\/\1>/gi,
+            (whole: string, insDel: string, block: string, blockArguments: string, content: string): string => {
+                // Prevent accidental matches like <ins><p>...</p>...</ins><ins>...<p></p></ins>
+                if (content.match(/<(ins|del)>/gi)) {
+                    return whole;
+                }
+                // Add the CSS-class to the existing "class"-attribute, or add one
+                let newArguments = blockArguments;
+                const modificationClass = (insDel.toLowerCase() === 'ins' ? 'insert' : 'delete');
+                if (newArguments.match(/class="/gi)) {
+                    // class="someclass" => class="someclass insert"
+                    newArguments = newArguments.replace(/(class\s*=\s*)(["'])([^\2]*)\2/gi,
+                        (classWhole: string, attr: string, para: string, classContent: string): string => {
+                            return attr + para + classContent + ' ' + modificationClass + para;
+                        }
+                    );
+                } else {
+                    newArguments += ' class="' + modificationClass + '"';
+                }
+                return '<' + block + newArguments + '>' + content + '</' + block + '>';
+            }
+        );
+
         if (diffUnnormalized.substr(0, workaroundPrepend.length) === workaroundPrepend) {
             diffUnnormalized = diffUnnormalized.substring(workaroundPrepend.length);
         }
@@ -1931,49 +1952,6 @@ export class DiffService {
         if (this.diffDetectBrokenDiffHtml(diffUnnormalized)) {
             diff = this.diffParagraphs(htmlOld, htmlNew, lineLength, firstLineNumber);
         } else {
-            diffUnnormalized = diffUnnormalized.replace(
-                /<ins>.*?(\n.*?)*<\/ins>/gi,
-                (found: string): string => {
-                    found = found.replace(
-                        /<(div|p|li)[^>]*>/gi,
-                        (match: string): string => {
-                            return match + '<ins>';
-                        }
-                    );
-                    found = found.replace(
-                        /<\/(div|p|li)[^>]*>/gi,
-                        (match: string): string => {
-                            return '</ins>' + match;
-                        }
-                    );
-                    return found;
-                }
-            );
-            diffUnnormalized = diffUnnormalized.replace(
-                /<del>.*?(\n.*?)*<\/del>/gi,
-                (found: string): string => {
-                    found = found.replace(
-                        /<(div|p|li)[^>]*>/gi,
-                        (match: string): string => {
-                            return match + '<del>';
-                        }
-                    );
-                    found = found.replace(
-                        /<\/(div|p|li)[^>]*>/gi,
-                        (match: string): string => {
-                            return '</del>' + match;
-                        }
-                    );
-                    return found;
-                }
-            );
-            diffUnnormalized = diffUnnormalized.replace(
-                /^<del><p>(.*)<\/p><\/del>$/gi,
-                (match: string, inner: string): string => {
-                    return '<p>' + inner + '</p>';
-                }
-            );
-
             let node: Element = document.createElement('div');
             node.innerHTML = diffUnnormalized;
             diff = node.innerHTML;

client/src/app/site/motions/services/linenumbering.service.spec.ts

@@ -491,6 +491,16 @@ describe('LinenumberingService', () => {
             expect(service.insertLineBreaksWithoutNumbers(outHtml, 80)).toBe(outHtml);
         }));
 
+        it('does not count within .insert nodes', inject(
+            [LinenumberingService],
+            (service: LinenumberingService) => {
+            const inHtml = "<p>1234</p><ul class=\"insert\"><li>1234</li></ul><p>1234 1234</p>";
+            const outHtml = service.insertLineNumbers(inHtml, 10);
+            expect(outHtml).toBe('<p>' + noMarkup(1) + '1234</p><ul class="insert"><li>1234</li></ul><p>' + noMarkup(2) + '1234 1234</p>');
+            expect(service.stripLineNumbers(outHtml)).toBe(inHtml);
+            expect(service.insertLineBreaksWithoutNumbers(outHtml, 80)).toBe(outHtml);
+        }));
+
         it('does not create a new line for a trailing INS', inject(
             [LinenumberingService],
             (service: LinenumberingService) => {

client/src/app/site/motions/services/linenumbering.service.ts

@@ -511,6 +511,8 @@ export class LinenumberingService {
             return this.ignoreInsertedText;
         } else if (this.isOsLineNumberNode(element)) {
             return true;
+        } else if (element.classList && element.classList.contains('insert')) {
+            return true;
         } else {
             return false;
         }
@@ -808,7 +810,7 @@ export class LinenumberingService {
             throw new Error('This method may only be called for ELEMENT-nodes: ' + element.nodeValue);
         }
         if (this.isIgnoredByLineNumbering(element)) {
-            if (this.currentInlineOffset === 0 && this.currentLineNumber !== null) {
+            if (this.currentInlineOffset === 0 && this.currentLineNumber !== null && this.isInlineElement(element)) {
                 const lineNumberNode = this.createLineNumber();
                 if (lineNumberNode) {
                     element.insertBefore(lineNumberNode, element.firstChild);