Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/150274
Change subject: Generate only one pair of snowmen for multi-sibling nodes
......................................................................
Generate only one pair of snowmen for multi-sibling nodes
If you had a leaf node rendered as 3 siblings (i.e. $element.length = 3),
then ve.ce.getDomText() would return 6 snowman characters, but the
correct number is 2. Put in a guard against double-snowmanning these nodes.
Also clean up the surrounding code a bit:
* Use Node.FOO_NODE constants directly
* Get rid of check for Node.CDATA_SECTION_NODE (who uses that?!)
Add unit tests, combining the tests for getDomHash and getDomText, and
including cases to test all code paths, including this one.
Bug: 67992
Bug: 68147
Bug: 68151
Bug: 68733
Bug: 68740
Change-Id: Ibf64fc0d3d03c639b2ad0f434949e75a42cef9ef
---
M modules/ve/ce/ve.ce.js
M modules/ve/tests/ce/ve.ce.test.js
2 files changed, 61 insertions(+), 27 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/74/150274/1
diff --git a/modules/ve/ce/ve.ce.js b/modules/ve/ce/ve.ce.js
index e6f08bd..e999752 100644
--- a/modules/ve/ce/ve.ce.js
+++ b/modules/ve/ce/ve.ce.js
@@ -32,8 +32,8 @@
* Gets the plain text of a DOM element (that is a node canContainContent ===
true)
*
* In the returned string only the contents of text nodes are included, and
the contents of
- * non-editable elements are excluded (but replaced with the appropriate
number of characters
- * so the offsets match up with the linear model).
+ * non-editable elements are excluded (but replaced with the appropriate
number of snowman
+ * characters so the offsets match up with the linear model).
*
* @method
* @param {HTMLElement} element DOM element to get text of
@@ -42,15 +42,16 @@
ve.ce.getDomText = function ( element ) {
// Inspired by jQuery.text / Sizzle.getText
var func = function ( element ) {
- var nodeType = element.nodeType,
- text = '',
- numChars,
- $element = $( element );
+ var viewNode,
+ nodeType = element.nodeType,
+ $element = $( element ),
+ text = '';
- // Node.ELEMENT_NODE = 1
- // Node.DOCUMENT_NODE = 9
- // Node.DOCUMENT_FRAGMENT_NODE = 11
- if ( nodeType === 1 || nodeType === 9 || nodeType === 11 ) {
+ if (
+ nodeType === Node.ELEMENT_NODE ||
+ nodeType === Node.DOCUMENT_NODE ||
+ nodeType === Node.DOCUMENT_FRAGMENT_NODE
+ ) {
if ( $element.hasClass( 've-ce-branchNode-slug' ) ) {
// Slugs are not represented in the model at
all, but they do
// contain a single nbsp/FEFF character in the
DOM, so make sure
@@ -58,18 +59,23 @@
return '';
} else if ( $element.hasClass( 've-ce-leafNode' ) ) {
// For leaf nodes, don't return the content,
but return
- // the right amount of characters so the
offsets match up
- numChars = $element.data( 'view'
).getOuterLength();
- return new Array( numChars + 1 ).join( '\u2603'
);
+ // the right number of placeholder characters
so the offsets match up.
+ viewNode = $element.data( 'view' );
+ // Only return snowmen for the first element in
a sibling group: otherwise
+ // we'll double-count this node
+ if ( viewNode && element ===
viewNode.$element[0] ) {
+ // \u2603 is the snowman character: ☃
+ return new Array(
viewNode.getOuterLength() + 1 ).join( '\u2603' );
+ }
+ // Second or subsequent sibling, don't
double-count
+ return '';
} else {
// Traverse its children
for ( element = element.firstChild; element;
element = element.nextSibling ) {
text += func( element );
}
}
- // Node.TEXT_NODE = 3
- // Node.CDATA_SECTION_NODE = 4 (historical, unused in HTML5)
- } else if ( nodeType === 3 || nodeType === 4 ) {
+ } else if ( nodeType === Node.TEXT_NODE ) {
return element.data;
}
return text;
diff --git a/modules/ve/tests/ce/ve.ce.test.js
b/modules/ve/tests/ce/ve.ce.test.js
index bfb8188..862b562 100644
--- a/modules/ve/tests/ce/ve.ce.test.js
+++ b/modules/ve/tests/ce/ve.ce.test.js
@@ -16,18 +16,46 @@
assert.equal( 'ab'.match( ve.ce.whitespacePattern ), null, 'does not
match non-whitespace' );
} );
-QUnit.test( 'getDomText', 1, function ( assert ) {
- assert.equal( ve.ce.getDomText(
- $( '<span>a<b><a
href="#">b</a></b><span></span><i>c</i>d</span>' )[0] ),
- 'abcd'
- );
-} );
+QUnit.test( 'getDomHash/getDomText', function ( assert ) {
+ var i, surface, documentView,
+ cases = [
+ {
+ msg: 'Nested annotations',
+ html: '<p><span>a<b><a href="#">b</a></b><span>
</span><i>c</i>d</span></p>',
+ hash:
'<DIV><P><SPAN>#<B><A>#</A></B><SPAN>#</SPAN><I>#</I>#</SPAN></P></DIV>',
+ text: 'ab cd'
+ },
+ {
+ msg: 'Inline nodes produce snowmen',
+ html: '<p>Foo <span rel="ve:Alien">Alien</span>
bar</p>',
+ hash: '<DIV><P>#<SPAN>#</SPAN>#</P></DIV>',
+ text: 'Foo ☃☃ bar'
+ },
+ {
+ msg: 'About grouped aliens produce one pair of
snowmen',
+ html: '<p>Foo ' +
+ '<span about="g1"
rel="ve:Alien">Alien</span>' +
+ '<span about="g1"
rel="ve:Alien">Aliens</span>' +
+ '<span about="g1"
rel="ve:Alien">Alien³</span> bar</p>',
+ hash:
'<DIV><P>#<SPAN>#</SPAN><SPAN>#</SPAN><SPAN>#</SPAN>#</P></DIV>',
+ text: 'Foo ☃☃ bar'
+ },
+ {
+ msg: 'Branch slugs are ignored',
+ html: '<table><tr><td>Foo</td></tr></table>',
+ hash:
'<DIV><DIV><P>#</P></DIV><TABLE><TBODY><TR><TD><P>#</P></TD></TR></TBODY></TABLE><DIV><P>#</P></DIV></DIV>',
+ text: 'Foo'
+ }
+ ];
-QUnit.test( 'getDomHash', 1, function ( assert ) {
- assert.equal(
- ve.ce.getDomHash( $( '<span>a<b><a
href="#">b</a></b><span></span><i>c</i>d</span>' )[0] ),
- '<SPAN>#<B><A>#</A></B><SPAN></SPAN><I>#</I>#</SPAN>'
- );
+ QUnit.expect( cases.length * 2 );
+
+ for ( i = 0; i < cases.length; i++ ) {
+ surface = ve.test.utils.createSurfaceFromHtml( cases[i].html );
+ documentView = surface.getView().getDocument();
+ assert.equal( ve.ce.getDomHash(
documentView.getDocumentNode().$element[0] ), cases[i].hash, 'getDomHash: ' +
cases[i].msg );
+ assert.equal( ve.ce.getDomText(
documentView.getDocumentNode().$element[0] ), cases[i].text, 'getDomText: ' +
cases[i].msg );
+ }
} );
QUnit.test( 'getOffset', function ( assert ) {
--
To view, visit https://gerrit.wikimedia.org/r/150274
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf64fc0d3d03c639b2ad0f434949e75a42cef9ef
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: wmf/1.24wmf15
Gerrit-Owner: Catrope <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
