jenkins-bot has submitted this change and it was merged.

Change subject: Further fixes to DOM Fragment encapsulation + unpacking
......................................................................


Further fixes to DOM Fragment encapsulation + unpacking

* Fixed figure expansion reuse to go through the same helper
  function (encapsulateExpansionHTML) like all other code.

* Added isForeignContent flag for reused expanses (transclusions
  and images). This is also used to discard excess span wrappers
  on non-foreign content dom fragments, and fix about-ids on
  foreign content dom fragments.

* These fixes now eliminate about-id manipulation during fragment
  unpacking. This fixes regressions on fr:Vasco_Evtimov and other
  pages that showed regressions in RT testing.

* Fixed DOMFragmentBuilder to use the refactored helper in
  Util.js for processing content in a sub-pipeline.

* Added parser test to capture the last scenario.

Change-Id: I348f80817dfcd60768171b98d698a0733d4e6e79
---
M js/lib/dom.t.unpackDOMFragments.js
M js/lib/ext.core.DOMFragmentBuilder.js
M js/lib/ext.core.LinkHandler.js
M js/lib/ext.core.TemplateHandler.js
M js/lib/mediawiki.DOMUtils.js
M js/tests/parserTests.txt
6 files changed, 98 insertions(+), 66 deletions(-)

Approvals:
  GWicke: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/js/lib/dom.t.unpackDOMFragments.js 
b/js/lib/dom.t.unpackDOMFragments.js
index 7b1c604..a213d1f 100644
--- a/js/lib/dom.t.unpackDOMFragments.js
+++ b/js/lib/dom.t.unpackDOMFragments.js
@@ -107,8 +107,7 @@
                                DU.deleteNode(sibling);
                        }
 
-                       var contentNode = dummyNode.firstChild,
-                               haveOrigType = 
Util.hasParsoidTypeOf(contentNode.getAttribute("typeof"));
+                       var contentNode = dummyNode.firstChild;
 
                        // Update DSR
                        //
@@ -139,23 +138,32 @@
 
                        }
 
-                       // Set about and drop wrapper-spans
-                       var n = dummyNode.firstChild;
+                       var isForeignContent = 
node.data.parsoid.tmp.isForeignContent,
+                               aboutIdMap = {},
+                               n = dummyNode.firstChild;
                        while (n) {
-                               var next = n.nextSibling;
+                               var next = n.nextSibling,
+                                       nAbout = n.getAttribute("about");
 
-                               // Transfer the about attribute so that it is 
still unique in the page
-                               if (haveOrigType && about !== null) {
-                                       n.setAttribute('about', about);
-                               }
-
-                               // Discard unnecessary span wrappers
-                               DU.loadDataParsoid(n);
-                               if (n.data.parsoid.tmp.wrapper &&
-                                       !haveOrigType && 
!n.getAttribute("typeof"))
-                               {
-                                       DU.migrateChildren(n, n.parentNode, n);
-                                       DU.deleteNode(n);
+                               if (isForeignContent && nAbout) {
+                                       // Replace old about-id with new 
about-id that is
+                                       // unique to the global page 
environment object
+                                       var newAbout = aboutIdMap[nAbout];
+                                       if (!newAbout) {
+                                               newAbout = env.newAboutId();
+                                               aboutIdMap[nAbout] = newAbout;
+                                       }
+                                       n.setAttribute("about", newAbout);
+                               } else {
+                                       // Discard unnecessary span wrappers.
+                                       //
+                                       // If the node has an about-id on it, 
it is part of
+                                       // transclusion or other generated 
content and is required.
+                                       DU.loadDataParsoid(n);
+                                       if (n.data.parsoid.tmp.wrapper && 
!nAbout) {
+                                               DU.migrateChildren(n, 
n.parentNode, n);
+                                               DU.deleteNode(n);
+                                       }
                                }
 
                                n = next;
diff --git a/js/lib/ext.core.DOMFragmentBuilder.js 
b/js/lib/ext.core.DOMFragmentBuilder.js
index 167073d..3d77c34 100644
--- a/js/lib/ext.core.DOMFragmentBuilder.js
+++ b/js/lib/ext.core.DOMFragmentBuilder.js
@@ -68,20 +68,26 @@
                // Source offsets of content
                var srcOffsets = scopeToken.getAttribute("srcOffsets");
 
-               var pipeline = 
this.manager.pipeFactory.getPipeline("tokens/x-mediawiki/expanded", {
-                       inBlockToken: true,
-                       noPre: scopeToken.getAttribute('noPre'),
-                       // Without source offsets for the content, it is not 
possible to compute DSR
-                       // and template wrapping in content. So, users of 
mw:dom-fragment-token should
-                       // always set offsets on content that comes from the 
top-level document.
-                       wrapTemplates: !!srcOffsets
-               });
-
-               if (srcOffsets) {
-                       pipeline.setSourceOffsets(srcOffsets[0], srcOffsets[1]);
-               }
-               pipeline.addListener('document', 
this.wrapDOMFragment.bind(this, cb, scopeToken));
-               pipeline.process(content.concat([new EOFTk()]));
+               // Process tokens
+               Util.processContentInPipeline(
+                       this.manager,
+                       // Append EOF
+                       content.concat([new EOFTk()]),
+                       {
+                               pipelineType: "tokens/x-mediawiki/expanded",
+                               pipelineOpts: {
+                                       inBlockToken: true,
+                                       noPre: scopeToken.getAttribute('noPre'),
+                                       // Without source offsets for the 
content, it isn't possible to
+                                       // compute DSR and template wrapping in 
content. So, users of
+                                       // mw:dom-fragment-token should always 
set offsets on content
+                                       // that comes from the top-level 
document.
+                                       wrapTemplates: !!srcOffsets
+                               },
+                               srcOffsets: srcOffsets,
+                               documentCB: this.wrapDOMFragment.bind(this, cb, 
scopeToken)
+                       }
+               );
        }
 };
 
diff --git a/js/lib/ext.core.LinkHandler.js b/js/lib/ext.core.LinkHandler.js
index fe639b4..f7d9e42 100644
--- a/js/lib/ext.core.LinkHandler.js
+++ b/js/lib/ext.core.LinkHandler.js
@@ -987,21 +987,13 @@
 
        // First check if we have a cached copy of this image expansion, and
        // avoid any further processing if we have a cache hit.
-       var cachedFile = this.manager.env.fileCache[token.dataAttribs.src];
+       var env = this.manager.env,
+               cachedFile = env.fileCache[token.dataAttribs.src];
        if (cachedFile) {
-               // Use the cached result.
-               // mw:DOMFragment wrapping is simplified as we know that we are
-               // dealing with a single subtree rooted either at a figure or a 
span.
-               var wrapperTokens = DU.getWrapperTokens(cachedFile.nodes),
+               var opts = { isForeignContent: true, noAboutId: true },
+                       wrapperTokens = DU.encapsulateExpansionHTML(env, token, 
cachedFile, opts),
                        firstWrapperToken = wrapperTokens[0];
-               DU.addTypeOf(firstWrapperToken, 'mw:DOMFragment');
-               firstWrapperToken.dataAttribs.html = cachedFile.html;
 
-               // Transfer tsr to the first token
-               firstWrapperToken.dataAttribs.tsr = token.dataAttribs.tsr;
-
-               // SSS FIXME: This could be deleted
-               //
                // Capture the delta between the old/new wikitext start posn.
                // 'tsr' values are stripped in the original DOM and won't be
                // present.  Since dsr[0] is identical to tsr[0] in this case,
@@ -1016,10 +1008,9 @@
                return;
        }
 
-       var env = this.manager.env,
-               // distinguish media types
-               // if image: parse options
-               content = buildLinkAttrs(token.attribs, true, null, null 
).contentKVs;
+       // distinguish media types
+       // if image: parse options
+       var content = buildLinkAttrs(token.attribs, true, null, null 
).contentKVs;
 
        // extract options
        // TODO gwicke: abstract out!
diff --git a/js/lib/ext.core.TemplateHandler.js 
b/js/lib/ext.core.TemplateHandler.js
index 0205248..7fa6448 100644
--- a/js/lib/ext.core.TemplateHandler.js
+++ b/js/lib/ext.core.TemplateHandler.js
@@ -108,7 +108,8 @@
                                // cache hit: reuse the expansion DOM
                                //console.log('cache hit for', 
JSON.stringify(text.substr(0, 50)));
                                var expansion = env.transclusionCache[text],
-                                       toks = DU.encapsulateExpansionHTML(env, 
token, expansion);
+                                       opts = {isForeignContent: true },
+                                       toks = DU.encapsulateExpansionHTML(env, 
token, expansion, opts);
 
                                cb({ tokens: toks });
                        } else {
diff --git a/js/lib/mediawiki.DOMUtils.js b/js/lib/mediawiki.DOMUtils.js
index 5c9f7ae..0c102c0 100644
--- a/js/lib/mediawiki.DOMUtils.js
+++ b/js/lib/mediawiki.DOMUtils.js
@@ -1091,10 +1091,7 @@
 
 
                function doExtractExpansions (node) {
-                       var nodes, expAccum,
-                               outerHTML = function (n) {
-                                       return n.outerHTML;
-                               };
+                       var nodes, expAccum;
 
                        while (node) {
                                if (node.nodeType === node.ELEMENT_NODE) {
@@ -1126,7 +1123,9 @@
                                                if (key) {
                                                        expAccum[key] = {
                                                                nodes: nodes,
-                                                               html: 
nodes.map(outerHTML).join('')
+                                                               html: 
nodes.map(function(node) {
+                                                                       return 
node.outerHTML;
+                                                               }).join('')
                                                        };
                                                }
                                                node = nodes.last();
@@ -1284,27 +1283,43 @@
 
        },
 
-       encapsulateExpansionHTML: function(env, token, expansion, aboutId) {
+       encapsulateExpansionHTML: function(env, token, expansion, opts) {
+               opts = opts || {};
+
                // Get placeholder tokens to get our subdom through the token 
processing
                // stages. These will be finally unwrapped on the DOM.
                var toks = this.getWrapperTokens(expansion.nodes),
-                       about = aboutId || env.newAboutId();
+                       firstWrapperToken = toks[0];
 
+               // Add the DOMFragment type so that we get unwrapped later.
+               firstWrapperToken.setAttribute('typeof', 'mw:DOMFragment');
                // Assign the HTML fragment to the data-parsoid.html on the 
first wrapper token.
-               toks[0].dataAttribs.html = expansion.html;
-               // Add the DOMFragment type so that we get unwrapped later
-               toks[0].setAttribute('typeof', 'mw:DOMFragment');
+               firstWrapperToken.dataAttribs.html = expansion.html;
 
-               // Add the about to all wrapper tokens
-               toks.forEach(function(tok) {
-                       tok.setAttribute('about', about);
-               });
+               // Set foreign content flag.
+               if (opts.isForeignContent) {
+                       if (!firstWrapperToken.dataAttribs.tmp) {
+                               firstWrapperToken.dataAttribs.tmp = {};
+                       }
+                       firstWrapperToken.dataAttribs.tmp.isForeignContent = 
true;
+               }
 
-               // Transfer the tsr. The first token gets the full width, the 
following
-               // tokens zero width.
+               // Add about to all wrapper tokens, if necessary.
+               var about = opts.aboutId;
+               if (!about && !opts.noAboutId) {
+                       about = env.newAboutId();
+               }
+               if (about) {
+                       toks.forEach(function(tok) {
+                               tok.setAttribute('about', about);
+                       });
+               }
+
+               // Transfer the tsr.
+               // The first token gets the full width, the following tokens 
zero width.
                var tokenTsr = token.dataAttribs ? token.dataAttribs.tsr : null;
                if (tokenTsr) {
-                       toks[0].dataAttribs.tsr = tokenTsr;
+                       firstWrapperToken.dataAttribs.tsr = tokenTsr;
                        var endTsr = [tokenTsr[1],tokenTsr[1]];
                        for (var i = 1; i < toks.length; i++) {
                                toks[i].dataAttribs.tsr = endTsr;
@@ -1346,7 +1361,8 @@
                                nodes: nodes,
                                html: nodes.map(function(n) { return 
n.outerHTML; }).join('')
                        },
-                       aboutId);
+                       { aboutId: aboutId }
+               );
        },
 
        /**
diff --git a/js/tests/parserTests.txt b/js/tests/parserTests.txt
index a6dbcc2..abcfefc 100644
--- a/js/tests/parserTests.txt
+++ b/js/tests/parserTests.txt
@@ -5096,6 +5096,16 @@
 </p>
 !! end
 
+!! test
+Parsoid: Scoped parsing should handle mixed transclusions and plain text
+!! options
+parsoid
+!! input
+[[Foo|{{echo|a}} b {{echo|c}}]]
+!! result
+<p data-parsoid='{"dsr":[0,20,0,0]}'><a rel="mw:WikiLink" href="Foo"><span 
about="#mwt2" typeof="mw:Transclusion" 
data-mw='{"parts":[{"template":{"target":{"wt":"echo","href":"./Template:Echo"},"params":{"1":{"wt":"a"}},"i":0}}]}'>a</span>
 b <span about="#mwt3" typeof="mw:Transclusion" 
data-mw='{"parts":[{"template":{"target":{"wt":"echo","href":"./Template:Echo"},"params":{"1":{"wt":"c"}},"i":0}}]}'>c</span></a></p>
+!! end
+
 ###
 ### Interwiki links (see maintenance/interwiki.sql)
 ###

-- 
To view, visit https://gerrit.wikimedia.org/r/88521
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I348f80817dfcd60768171b98d698a0733d4e6e79
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/Parsoid
Gerrit-Branch: master
Gerrit-Owner: Subramanya Sastry <[email protected]>
Gerrit-Reviewer: GWicke <[email protected]>
Gerrit-Reviewer: Subramanya Sastry <[email protected]>
Gerrit-Reviewer: jenkins-bot

_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to