jenkins-bot has submitted this change and it was merged.
Change subject: (Bug 51678) Fixed bug in dom-diff algorithm
......................................................................
(Bug 51678) Fixed bug in dom-diff algorithm
* The dom-diff algorithm was occasionally recursing into
template transclusion HTML. Since transclusion HTML can never
be directly modified by editors and template updates are
transmitted via the data-mw object in the wrapper, it is not
necessary to compare transclusion content. It can in fact
introduce bugs as in the itwp examples in the bug report because
DOM-diff might insert deleted-content-markers and break the
continuity in template about markers (which is important to
skip over transclusion content during serialization correctly).
This bug also only surfaces because of the peculiarities of how
updated template HTML is modified in VE (see bug report for
more details).
* Added some documentation to dom-util functions.
* Tested this with HTML (old and edited) dumped from VE via
Chrome's console. Verified bug without patch and a fix with
the patch.
Currently, we don't have direct dom-diff tests, only indirect
tests via selser.
Even if we did have, it would be hard to write a test for this
bug fix becuase it depends on template content HTML getting
deleted on edit (see bug report for details). But, it would be
possible with a jasmine testing framework which we could consider
resurrecting.
To cut a long story short: no change in parsertest results,
and no new tests added.
Change-Id: I886161d981a0c79b8300d88c5281514b4248cff2
---
M js/lib/mediawiki.DOMDiff.js
M js/lib/mediawiki.DOMUtils.js
M js/lib/mediawiki.WikitextSerializer.js
3 files changed, 51 insertions(+), 16 deletions(-)
Approvals:
Cscott: Looks good to me, approved
jenkins-bot: Verified
diff --git a/js/lib/mediawiki.DOMDiff.js b/js/lib/mediawiki.DOMDiff.js
index 5d4814e..39a90c4 100644
--- a/js/lib/mediawiki.DOMDiff.js
+++ b/js/lib/mediawiki.DOMDiff.js
@@ -231,31 +231,48 @@
if (origNode.nodeName === baseNode.nodeName) {
// Identical wrapper-type, but modified.
// Mark as modified, and recurse.
+ this.debug("--found diff:
modified-wrapper--");
this.markNode(origNode,
'modified-wrapper');
- this.doDOMDiff(baseNode, origNode);
+ if (!DU.isTplElementNode(this.env,
baseNode) && !DU.isTplElementNode(this.env, origNode)) {
+ // Dont recurse into
template-like-content
+ this.doDOMDiff(baseNode,
origNode);
+ }
} else {
// Mark the sub-tree as modified since
// we have two entirely different nodes
here
+ this.debug("--found diff: modified--");
this.markNode(origNode, 'modified');
}
}
// Record the fact that direct children changed in the
parent node
+ this.debug("--found diff: modified-children--");
this.markNode(newParentNode, 'modified-children');
foundDiffOverall = true;
- } else if (!DU.isTplElementNode(this.env, newNode)) {
+ } else if (!DU.isTplElementNode(this.env, baseNode) &&
!DU.isTplElementNode(this.env, newNode)) {
+ this.debug("--equal: recursing--");
// Recursively diff subtrees if not template-like
content
var subtreeDiffers = this.doDOMDiff(baseNode, newNode);
if (subtreeDiffers) {
+ this.debug("--found diff: subtree-changed--");
this.markNode(newNode, 'subtree-changed');
}
foundDiffOverall = subtreeDiffers || foundDiffOverall;
}
- // And move on to the next pair
- baseNode = baseNode.nextSibling;
- newNode = newNode.nextSibling;
+ // And move on to the next pair (skipping over template HTML)
+ if (DU.isTplElementNode(this.env, baseNode)) {
+ baseNode = DU.skipOverEncapsulatedContent(baseNode);
+ } else {
+ baseNode = baseNode.nextSibling;
+ }
+
+ if (DU.isTplElementNode(this.env, newNode)) {
+ newNode = DU.skipOverEncapsulatedContent(newNode);
+ } else {
+ newNode = newNode.nextSibling;
+ }
}
// mark extra new nodes as modified
diff --git a/js/lib/mediawiki.DOMUtils.js b/js/lib/mediawiki.DOMUtils.js
index 0dd729e..b081938 100644
--- a/js/lib/mediawiki.DOMUtils.js
+++ b/js/lib/mediawiki.DOMUtils.js
@@ -4,6 +4,7 @@
* General DOM utilities
*/
+require('./core-upgrade.js');
var Util = require('./mediawiki.Util.js').Util,
Node = require('./mediawiki.wikitext.constants.js').Node,
pd = require('./mediawiki.parser.defines.js');
@@ -665,6 +666,18 @@
}
},
+ /**
+ * Gets all siblings that follow 'node' that have an 'about' as their
about id.
+ *
+ * This is used to fetch transclusion/extension content by using the
about-id
+ * as the key. This works because transclusion/extension content is a
forest
+ * of dom-trees formed by adjacent dom-nodes. This is the contract that
+ * templace encapsulation, dom-reuse, and VE code all have to abide by.
+ *
+ * The only exception to this adjacency rule is IEW nodes in fosterable
+ * positions (in tables) which are not span-wrapped to prevent them
from getting
+ * fostered out.
+ **/
getAboutSiblings: function(node, about) {
var nodes = [node];
@@ -682,6 +695,20 @@
},
/**
+ * If 'node' has about-id, it is assumed that it is generated by
templates or
+ * extensions. This function skips over all following content nodes
and returns
+ * the first non-template node that follows it.
+ */
+ skipOverEncapsulatedContent: function(node) {
+ var about = node.getAttribute('about');
+ if (!about) {
+ return node;
+ }
+
+ return this.getAboutSiblings(node, about).last().nextSibling;
+ },
+
+ /**
* Extract transclusion and extension expansions from a DOM, and return
* them in a structure like this:
* {
diff --git a/js/lib/mediawiki.WikitextSerializer.js
b/js/lib/mediawiki.WikitextSerializer.js
index bcf736b..651752e 100644
--- a/js/lib/mediawiki.WikitextSerializer.js
+++ b/js/lib/mediawiki.WikitextSerializer.js
@@ -3109,15 +3109,6 @@
return srcParts.join('');
};
-WSP.skipOverEncapsulatedContent = function(node) {
- var about = node.getAttribute('about');
- if (!about) {
- return node;
- }
-
- return DU.getAboutSiblings(node, about).last().nextSibling;
-};
-
/**
* Get a DOM-based handler for an element node
*/
@@ -3168,7 +3159,7 @@
if (src !== undefined) {
self.emitWikitext(src, state, cb, node);
- return
self.skipOverEncapsulatedContent(node);
+ return
DU.skipOverEncapsulatedContent(node);
} else {
console.error("ERROR: No handler for: "
+ node.outerHTML);
console.error("Serializing as HTML.");
@@ -3944,7 +3935,7 @@
// Skip over encapsulated content since
it has already been serialized
var typeOf = node.getAttribute(
'typeof' ) || '';
if
(/\bmw:(?:Transclusion\b|Param\b|Extension\/[^\s]+)/.test(typeOf)) {
- nextNode =
this.skipOverEncapsulatedContent(node);
+ nextNode =
DU.skipOverEncapsulatedContent(node);
}
} else if (DU.onlySubtreeChanged(node,
this.env) &&
hasValidTagWidths(dp.dsr) &&
--
To view, visit https://gerrit.wikimedia.org/r/77357
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I886161d981a0c79b8300d88c5281514b4248cff2
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/Parsoid
Gerrit-Branch: master
Gerrit-Owner: Subramanya Sastry <[email protected]>
Gerrit-Reviewer: Cscott <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
