Subramanya Sastry has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/372896 )
Change subject: Linter: Tweak tidy-whitespace-bug output
......................................................................
Linter: Tweak tidy-whitespace-bug output
* Before this patch, we were reporting all instances of affected
whitespace. However, with short strings, there is no real issue
with rendering unless the viewport is small. So, in this patch,
I am implementing a heuristic that triggers this linter issue
only if the affected string is of a particular length.
* Updated mocha tests.
Change-Id: I15d401e9a9b649723ae3bfc3598d02e8a619a9ec
---
M lib/config/ParsoidConfig.js
M lib/wt2html/pp/handlers/linter.js
M tests/mocha/linter.js
3 files changed, 94 insertions(+), 57 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/parsoid
refs/changes/96/372896/1
diff --git a/lib/config/ParsoidConfig.js b/lib/config/ParsoidConfig.js
index 7addddd..4958257 100644
--- a/lib/config/ParsoidConfig.js
+++ b/lib/config/ParsoidConfig.js
@@ -249,6 +249,18 @@
*/
ParsoidConfig.prototype.linterAPISampling = 1;
+ParsoidConfig.prototype.linter = {
+ /**
+ * @property {number} tidyWhiteSpaceBugMaxLength
+ *
+ * Max length of content covered by 'white-space:nowrap' CSS
+ * that we consider "safe" when Tidy is replaced. Beyond that,
+ * wikitext will have to be fixed up to manually insert whitespace
+ * at the right places.
+ */
+ tidyWhitespaceBugMaxLength: 100,
+};
+
/**
* @property {Function} loggerBackend
* The logger output function.
diff --git a/lib/wt2html/pp/handlers/linter.js
b/lib/wt2html/pp/handlers/linter.js
index d4e2e7d..b6fc76c 100644
--- a/lib/wt2html/pp/handlers/linter.js
+++ b/lib/wt2html/pp/handlers/linter.js
@@ -433,66 +433,79 @@
}
}
-function logTidyWhitespaceBug(env, node, dp, tplInfo) {
- if (DU.isBlockNode(node)) {
- // Nothing to worry
- return;
- }
-
- if (!hasNoWrapCSS(node)) {
- // no CSS property that affects whitespcae
- return;
- }
-
- // Find next non-comment sibling of 'node'
- var next = node.nextSibling;
- while (next && DU.isComment(next)) {
- next = next.nextSibling;
- }
-
- if (!next || DU.isBlockNode(next) || (DU.isText(next) &&
/^\s/.test(next.data))) {
- // All good! No text/inline-node sibling
- return;
- }
-
+function lastNonCommentNode(node) {
// Find last non-comment child of 'node'
var last = node.lastChild;
while (last && DU.isComment(last)) {
last = last.previousSibling;
}
+}
- if (!last || !DU.isText(last) || !/\s$/.test(last.data)) {
- // All good! No whitespace for Tidy to hoist out
- return;
+function logTidyWhitespaceBug(env, node, dp, tplInfo) {
+ // FIXME: Potential for O(n^2) behavior in really bad cases
+
+ var nowrapNodes = [];
+ var startNode = node;
+ while (node && !DU.isBlockNode(node)) {
+ if (DU.isText(node) || !hasNoWrapCSS(node)) {
+ // No CSS property that affects whitespace.
+ if (nowrapNodes.length > 0 &&
!/^\s/.test(node.textContent)) {
+ nowrapNodes.push(node);
+ }
+ break;
+ }
+
+ // Find last non-comment child of 'node'
+ var last = node.lastChild;
+ while (last && DU.isComment(last)) {
+ last = last.previousSibling;
+ }
+
+ if (!last || !DU.isText(last) || !/\s$/.test(last.data)) {
+ // All good! No whitespace for Tidy to hoist out
+ break;
+ }
+
+ // In this scenario, when Tidy hoists the whitespace to
+ // after the node, that whitespace is not subject to the
+ // nowrap CSS => browsers can break content there.
+ //
+ // But, non-Tidy libraries won't hoist the whitespace.
+ // So, browsers don't have a place to break content.
+ //
+ // Track the nodes that are subject to this problem.
+ nowrapNodes.push(node);
+
+ node = node.nextSibling;
+
+ // Skip comments
+ while (node && DU.isComment(node)) {
+ node = node.nextSibling;
+ }
}
- // So, we have:
- // * two inline siblings without whitespace between then,
- // * the left sibling has nowrap CSS
- // * the left sibling has trailing whitespace
- //
- // In this scenario, when Tidy hoists the whitespace to
- // after the left sibling, that whitespace is not subject
- // to the nowrap CSS => browsers can break content there.
- //
- // But, non-Tidy libraries won't hoist the whitespace.
- // So, the content will extend indefinitely.
- // Flag this pattern for editors to fix.
-
-
- var dsr, templateInfo;
- if (tplInfo) {
- templateInfo = findEnclosingTemplateName(env, tplInfo);
- dsr = tplInfo.dsr;
- } else {
- dsr = dp.dsr;
+ // Find length of affected content
+ var len = nowrapNodes.length < 2 ? 0 : nowrapNodes.reduce(function(n,
node) { return n + node.textContent.length; }, 0);
+ if (len && len > env.conf.parsoid.linter.tidyWhitespaceBugMaxLength) {
+ var lastNode = nowrapNodes[nowrapNodes.length - 1];
+ var lastNodeDSR = DU.getDataParsoid(lastNode);
+ var dsr, templateInfo;
+ if (tplInfo) {
+ templateInfo = findEnclosingTemplateName(env, tplInfo);
+ dsr = tplInfo.dsr;
+ if (DU.getAttribute(lastNode, 'about') !==
tplInfo.about) {
+ dsr = [dsr[0], (lastNodeDSR.dsr || {})[1]];
+ }
+ } else {
+ dsr = [dp.dsr[0], (lastNodeDSR.dsr || {})[1]];
+ }
+ var lintObj = {
+ dsr: dsr,
+ templateInfo: templateInfo,
+ params: { node: startNode.nodeName, sibling:
lastNode.nodeName },
+ };
+ env.log('lint/tidy-whitespace-bug', lintObj);
}
- var lintObj = {
- dsr: dsr,
- templateInfo: templateInfo,
- params: { node: node.nodeName, sibling: next.nodeName },
- };
- env.log('lint/tidy-whitespace-bug', lintObj);
}
function logWikitextFixups(node, env, atTopLevel, tplInfo) {
diff --git a/tests/mocha/linter.js b/tests/mocha/linter.js
index 81c9ef8..c573407 100644
--- a/tests/mocha/linter.js
+++ b/tests/mocha/linter.js
@@ -17,14 +17,14 @@
// Parsing the `[[file:...]]` tags below may also require running the
// mock API to answer imageinfo queries.
var parsoidConfig = new ParsoidConfig(null, { defaultWiki: 'enwiki',
loadWMF: true, linting: true });
- var parseWT = function(wt) {
- return helpers.parse(parsoidConfig, wt).then(function(ret) {
+ var parseWT = function(wt, opts) {
+ return helpers.parse(parsoidConfig, wt,
opts).then(function(ret) {
return ret.env.lintLogger.buffer;
});
};
- var expectEmptyResults = function(wt) {
- return parseWT(wt).then(function(result) {
+ var expectEmptyResults = function(wt, opts) {
+ return parseWT(wt, opts).then(function(result) {
return result.should.be.empty;
});
};
@@ -478,7 +478,10 @@
"<!--boo-->",
"<span>x</span>",
].join('');
- return parseWT(wt).then(function(result) {
+ var tweakEnv = function(env) {
+
env.conf.parsoid.linter.tidyWhitespaceBugMaxLength = 0;
+ }
+ return parseWT(wt, { tweakEnv: tweakEnv }
).then(function(result) {
result.should.have.length(5);
result.forEach(function(r) {
r.should.have.a.property('type',
'tidy-whitespace-bug');
@@ -491,6 +494,12 @@
result[4].params.should.have.a.property("sibling", "SPAN");
// skipping dsr tests
});
+
+ // Nothing to trigger here
+ tweakEnv = function(env) {
+
env.conf.parsoid.linter.tidyWhitespaceBugMaxLength = 100;
+ };
+ return expectEmptyResults(wt, { tweakEnv: tweakEnv });
});
it('should not flag tidy whitespace bug (1)' , function() {
var wt = [
@@ -531,7 +540,10 @@
"a ",
"</span>",
].join('');
- return expectEmptyResults(wt);
+ var tweakEnv = function(env) {
+
env.conf.parsoid.linter.tidyWhitespaceBugMaxLength = 0;
+ }
+ return expectEmptyResults(wt, { tweakEnv: tweakEnv });
});
});
});
--
To view, visit https://gerrit.wikimedia.org/r/372896
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I15d401e9a9b649723ae3bfc3598d02e8a619a9ec
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/parsoid
Gerrit-Branch: master
Gerrit-Owner: Subramanya Sastry <ssas...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
