Diff-Bugfix: be more consistent about marking up block elements by 'inserted' instead of using <ins>-tags

This commit is contained in:
Tobias Hößl 2018-11-17 16:26:43 +01:00 committed by Emanuel Schütze
parent 34498af9c5
commit 31d8258750
3 changed files with 70 additions and 61 deletions

View File

@ -15,8 +15,7 @@ Bugfixes:
- Fixed missing output of special poll values (for motions and elections) [#3932]. - Fixed missing output of special poll values (for motions and elections) [#3932].
- Fixed amendment csv export (added missing submitters and recommendation, removed - Fixed amendment csv export (added missing submitters and recommendation, removed
html tags for old and new text) [#3942]. html tags for old and new text) [#3942].
- Fixed motion/amendment diff bug: does not delete a paragraph/list item - Fixed motion/amendment diff bug [#3943, #3946, #4020].
before an new one was inserted [#3943, #3946].
- Allow to hide internal items in agenda sort view [#3992]. - Allow to hide internal items in agenda sort view [#3992].

View File

@ -1337,6 +1337,32 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
return node.innerHTML; return node.innerHTML;
}; };
/**
* Add the CSS-class to the existing "class"-attribute, or add one.
* Works on strings, not nodes
*
* @param {string} tagStr
* @param {string} className
* @returns {string}
* @private
*/
this._addClassToHtmlTag = function (tagStr, className) {
return tagStr.replace(/<(\w+)( [^>]*)?>/gi, function(whole, tag, tagArguments) {
tagArguments = (tagArguments ? tagArguments : '');
if (tagArguments.match(/class="/gi)) {
// class="someclass" => class="someclass insert"
tagArguments = tagArguments.replace(/(class\s*=\s*)(["'])([^\2]*)\2/gi,
function (classWhole, attr, para, content) {
return attr + para + content + ' ' + className + para;
}
);
} else {
tagArguments += ' class="' + className + '"';
}
return '<' + tag + tagArguments + '>';
});
};
/** /**
* This function removes color-Attributes from the styles of this node or a descendant, * This function removes color-Attributes from the styles of this node or a descendant,
* as they interfer with the green/red color in HTML and PDF * as they interfer with the green/red color in HTML and PDF
@ -1469,36 +1495,9 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
} }
); );
// Merging individual insert/delete statements into bigger blocks
diffUnnormalized = diffUnnormalized.replace(/<\/ins><ins>/gi, '').replace(/<\/del><del>/gi, ''); diffUnnormalized = diffUnnormalized.replace(/<\/ins><ins>/gi, '').replace(/<\/del><del>/gi, '');
// Move whitespaces around inserted P's out of the INS-tag
diffUnnormalized = diffUnnormalized.replace(
/<ins>(\s*)(<p( [^>]*)?>[\s\S]*?<\/p>)(\s*)<\/ins>/gim,
function(match, whiteBefore, inner, tagInner, whiteAfter) {
return whiteBefore +
inner
.replace(/<p( [^>]*)?>/gi, function(match) {
return match + "<ins>";
})
.replace(/<\/p>/gi, "</ins></p>") +
whiteAfter;
}
);
// Fixes HTML produced by the diff like this:
// from: <del></P></del><ins> Inserted Text</P>\n<P>More inserted text</P></ins>
// into: <ins> Inserted Text</ins></P>\n<P>More inserted text</ins></P>
diffUnnormalized = diffUnnormalized.replace(
/<del><\/p><\/del><ins>([\s\S]*?)<\/p><\/ins>/gim,
"<ins>$1</ins></p>"
);
diffUnnormalized = diffUnnormalized.replace(
/<ins>[\s\S]*?<\/ins>/gim,
function(match) {
return match.replace(/(<\/p>\s*<p>)/gi, "</ins>$1<ins>");
}
);
// If only a few characters of a word have changed, don't display this as a replacement of the whole word, // If only a few characters of a word have changed, don't display this as a replacement of the whole word,
// but only of these specific characters // but only of these specific characters
diffUnnormalized = diffUnnormalized.replace(/<del>([a-z0-9,_-]* ?)<\/del><ins>([a-z0-9,_-]* ?)<\/ins>/gi, function (found, oldText, newText) { diffUnnormalized = diffUnnormalized.replace(/<del>([a-z0-9,_-]* ?)<\/del><ins>([a-z0-9,_-]* ?)<\/ins>/gi, function (found, oldText, newText) {
@ -1546,33 +1545,36 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
} }
); );
// <ins><li>...</li></ins> => <li class="insert">...</li> // If larger inserted HTML text contains block elements, we separate the inserted text into
diffUnnormalized = diffUnnormalized.replace( // inline <ins> elements and "insert"-class-based block elements.
/<(ins|del)><(p|div|blockquote|li)([^>]*)>([\s\S]*)<\/\2><\/\1>/gi, // <ins>...<div>...</div>...</ins> => <ins>...</ins><div class="insert">...</div><ins>...</ins>
function(whole, insDel, block, blockArguments, content) { var diffService = this;
// Prevent accidental matches like <ins><p>...</p>...</ins><ins>...<p></p></ins> diffUnnormalized = diffUnnormalized.replace(/<(ins|del)>([\s\S]*?)<\/\1>/gi, function(whole, insDel) {
if (content.match(/<(ins|del)>/gi)) {
return whole;
}
// Add the CSS-class to the existing "class"-attribute, or add one
var newArguments = blockArguments;
var modificationClass = (insDel.toLowerCase() === 'ins' ? 'insert' : 'delete'); var modificationClass = (insDel.toLowerCase() === 'ins' ? 'insert' : 'delete');
if (newArguments.match(/class="/gi)) { return whole.replace(/(<(p|div|blockquote|li)[^>]*>)([\s\S]*?)(<\/\2>)/gi, function(whole2, opening, blockTag, content, closing) {
// class="someclass" => class="someclass insert" var modifiedTag = diffService._addClassToHtmlTag(opening, modificationClass);
newArguments = newArguments.replace(/(class\s*=\s*)(["'])([^\2]*)\2/gi, return '</' + insDel + '>' + modifiedTag + content + closing + '<' + insDel + '>';
function(classWhole, attr, para, content) { });
return attr + para + content + ' ' + modificationClass + para; });
}
);
} else {
newArguments += ' class="' + modificationClass + '"';
}
return '<' + block + newArguments + '>' + content + '</' + block + '>'; // Cleanup leftovers from the operation above, when <ins></ins>-tags ore <ins> </ins>-tags are left
// around block tags. It should be safe to remove them and just leave the whitespaces.
diffUnnormalized = diffUnnormalized.replace(/<(ins|del)>(\s*)<\/\1>/gi, function (whole, insDel, space) {
return space;
});
// <del></p><ins> Added text</p></ins> -> <ins> Added text</ins></p>
diffUnnormalized = diffUnnormalized.replace(
/<del><\/(p|div|blockquote|li)><\/del><ins>([\s\S]*?)<\/\1>(\s*)<\/ins>/gi,
function(whole, blockTag, content, space) {
return '<ins>' + content + '</ins></' + blockTag + '>' + space;
} }
); );
// </p> </ins> -> </ins></p>
diffUnnormalized = diffUnnormalized.replace(/(<\/(p|div|blockquote|li)>)(\s*)<\/(ins|del)>/gi, function (whole, ending, blockTag, space, insdel) {
return '</' + insdel + '>' + ending + space;
});
if (diffUnnormalized.substr(0, workaroundPrepend.length) === workaroundPrepend) { if (diffUnnormalized.substr(0, workaroundPrepend.length) === workaroundPrepend) {
diffUnnormalized = diffUnnormalized.substring(workaroundPrepend.length); diffUnnormalized = diffUnnormalized.substring(workaroundPrepend.length);

View File

@ -454,7 +454,7 @@ describe('linenumbering', function () {
"<p>Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu.</p>"; "<p>Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu.</p>";
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe("<p><span class=\"line-number-5 os-line-number\" contenteditable=\"false\" data-line-number=\"5\">&nbsp;</span>Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</p>\n" + expect(diff).toBe("<p><span class=\"line-number-5 os-line-number\" contenteditable=\"false\" data-line-number=\"5\">&nbsp;</span>Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</p>\n" +
"<p><ins>Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu.</ins></p>"); "<p class=\"insert\">Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu.</p>");
}); });
it('does not result in separate paragraphs when only the first word has changed', function () { it('does not result in separate paragraphs when only the first word has changed', function () {
@ -527,9 +527,10 @@ describe('linenumbering', function () {
after = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch</p>" + after = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch</p>" +
"<p>Test 123</p>", "<p>Test 123</p>",
expected = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid<ins> Noch</ins></p>" + expected = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid<ins> Noch</ins></p>" +
"<p><ins>Test 123</ins></p>"; "<p class=\"insert\">Test 123</p>";
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe(expected); expect(diff).toBe(expected);
}); });
@ -540,7 +541,7 @@ describe('linenumbering', function () {
"\n" + "\n" +
"<p>Stet clita kasd gubergren, no sea takimata sanctus est.</p>", "<p>Stet clita kasd gubergren, no sea takimata sanctus est.</p>",
expected = "<p><span class=\"line-number-1 os-line-number\" contenteditable=\"false\" data-line-number=\"1\">&nbsp;</span>Lorem ipsum dolor sit amet, consetetur sadipscing elitr,<ins> sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum.</ins></p>\n" + expected = "<p><span class=\"line-number-1 os-line-number\" contenteditable=\"false\" data-line-number=\"1\">&nbsp;</span>Lorem ipsum dolor sit amet, consetetur sadipscing elitr,<ins> sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum.</ins></p>\n" +
"<p class=\"os-split-after\"><ins>Stet clita kasd gubergren, no sea takimata sanctus est.</ins></p>"; "<p class=\"insert os-split-after\">Stet clita kasd gubergren, no sea takimata sanctus est.</p>";
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe(expected); expect(diff).toBe(expected);
@ -552,8 +553,8 @@ describe('linenumbering', function () {
'<p style="text-align: justify;"><span style="color: #000000;">Inserting this line should not make any troubles, especially not affect the first line</span></p>' + '<p style="text-align: justify;"><span style="color: #000000;">Inserting this line should not make any troubles, especially not affect the first line</span></p>' +
'<p style="text-align: justify;"><span style="color: #000000;">Neither should this line</span></p>', '<p style="text-align: justify;"><span style="color: #000000;">Neither should this line</span></p>',
expected = "<p>This is a random first line that remains unchanged.</p>" + expected = "<p>This is a random first line that remains unchanged.</p>" +
'<p style="text-align: justify;"><ins><span style="color: #000000;">Inserting this line should not make any troubles, especially not affect the first line</span></ins></p>' + '<p style="text-align: justify;" class="insert"><span style="color: #000000;">Inserting this line should not make any troubles, especially not affect the first line</span></p>' +
'<p style="text-align: justify;"><ins><span style="color: #000000;">Neither should this line</span></ins></p>'; '<p style="text-align: justify;" class="insert"><span style="color: #000000;">Neither should this line</span></p>';
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe(expected); expect(diff).toBe(expected);
@ -653,6 +654,13 @@ describe('linenumbering', function () {
expect(diff).toBe('<p>Lorem ipsum dolor sit amet, consetetur <del><br></del>sadipscing elitr.<ins> Sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua..</ins><br>Bavaria ipsum dolor sit amet oha wea nia ausgähd<br><del>kummt nia hoam i hob di narrisch gean</del><ins>Autonomie erfährt ihre Grenzen</ins></p>'); expect(diff).toBe('<p>Lorem ipsum dolor sit amet, consetetur <del><br></del>sadipscing elitr.<ins> Sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua..</ins><br>Bavaria ipsum dolor sit amet oha wea nia ausgähd<br><del>kummt nia hoam i hob di narrisch gean</del><ins>Autonomie erfährt ihre Grenzen</ins></p>');
}); });
it('works with multiple inserted paragraphs', function() {
var before = '<p>This is the text before</p>',
after = "<p>This is the text before</p>\n<p>This is one added line</p>\n<p>Another added line</p>";
var diff = diffService.diff(before, after);
expect(diff).toBe("<p>This is the text before</p>\n<p class=\"insert\">This is one added line</p>\n<p class=\"insert\">Another added line</p>");
});
it('does not a change in a very specific case', function() { it('does not a change in a very specific case', function() {
// See diff._fixWrongChangeDetection // See diff._fixWrongChangeDetection
var inHtml = '<p>Test 123<br>wir strikt ab. lehnen wir ' + brMarkup(1486) + 'ab.<br>' + noMarkup(1487) + 'Gegenüber</p>', var inHtml = '<p>Test 123<br>wir strikt ab. lehnen wir ' + brMarkup(1486) + 'ab.<br>' + noMarkup(1487) + 'Gegenüber</p>',
@ -696,7 +704,7 @@ describe('linenumbering', function () {
before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2); before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2);
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" + expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" +
"<p><ins>NEW PARAGRAPH 2.</ins></p>"); "<p class=\"insert\">NEW PARAGRAPH 2.</p>");
}); });
it('works with two inserted paragraphs', function () { it('works with two inserted paragraphs', function () {
@ -711,8 +719,8 @@ describe('linenumbering', function () {
before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2); before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2);
var diff = diffService.diff(before, after); var diff = diffService.diff(before, after);
expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" + expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" +
"<p><ins>NEW PARAGRAPH 1.</ins></p>\n" + "<p class=\"insert\">NEW PARAGRAPH 1.</p>\n" +
"<p><ins>NEW PARAGRAPH 2.</ins></p>\n" + "<p class=\"insert\">NEW PARAGRAPH 2.</p>\n" +
"<div>" + noMarkup(4) + "Go on</div>" "<div>" + noMarkup(4) + "Go on</div>"
); );
}); });