jenkins-bot has submitted this change and it was merged. 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(+), 28 deletions(-) Approvals: Esanders: Looks good to me, approved jenkins-bot: Verified diff --git a/modules/ve/ce/ve.ce.js b/modules/ve/ce/ve.ce.js index 804e47e..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,19 +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(); - // \u2603 is the snowman character: ☃ - 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/149920 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ibf64fc0d3d03c639b2ad0f434949e75a42cef9ef Gerrit-PatchSet: 6 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: master Gerrit-Owner: Catrope <roan.katt...@gmail.com> Gerrit-Reviewer: Catrope <roan.katt...@gmail.com> Gerrit-Reviewer: Esanders <esand...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits