C. Scott Ananian has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/363743 )
Change subject: Fix infinite loop in ve.BranchNode#getNodeFromOffset ...................................................................... Fix infinite loop in ve.BranchNode#getNodeFromOffset This is a minor clean up of the refactoring done in 1cbcc2f0bf66fd824c255a7e6ca1e87fdacd46e9. If the document is empty (consisting only of ve.ce.InternalListNode) then it is possible for `currentNode.children.length > 0` and `currentNode.children[ 0 ] instanceof ve.ce.InternalListNode`. This sets up an infinite loop, unless `offset === nodeOffset`. This was being triggered when an empty document consisting only of `<paragraph>, </paragraph>, <internalList>, </internalList>` was p-unwrapped; for example, prior to paste in ve.ui.MWWikitextStringTransferHandler.js or prior to update of a `-{}-` construct by ve.ui.MWLanguageVariantInspector. Change-Id: I933bf28637f3b3c235aa4b9f8d96d1d09a7b3085 --- M src/ve.BranchNode.js M src/ve.Document.js M tests/ce/ve.ce.Document.test.js 3 files changed, 37 insertions(+), 19 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor refs/changes/43/363743/1 diff --git a/src/ve.BranchNode.js b/src/ve.BranchNode.js index c2404db..477bb4b 100644 --- a/src/ve.BranchNode.js +++ b/src/ve.BranchNode.js @@ -155,14 +155,14 @@ while ( currentNode.children.length ) { for ( i = 0, length = currentNode.children.length; i < length; i++ ) { childNode = currentNode.children[ i ]; - if ( childNode instanceof ve.ce.InternalListNode ) { - break; - } if ( offset === nodeOffset ) { // The requested offset is right before childNode, so it's not // inside any of currentNode's children, but is inside currentNode return currentNode; } + if ( childNode instanceof ve.ce.InternalListNode ) { + break SIBLINGS; + } nodeLength = childNode.getOuterLength(); if ( offset >= nodeOffset && offset < nodeOffset + nodeLength ) { if ( !shallow && childNode.hasChildren() && childNode.getChildren().length ) { diff --git a/src/ve.Document.js b/src/ve.Document.js index 58d7218..5e909c4 100644 --- a/src/ve.Document.js +++ b/src/ve.Document.js @@ -39,7 +39,7 @@ }; /** - * Get a node a an offset. + * Get a node at an offset. * * @method * @param {number} offset Offset to get node at diff --git a/tests/ce/ve.ce.Document.test.js b/tests/ce/ve.ce.Document.test.js index 4984c98..c1eebc8 100644 --- a/tests/ce/ve.ce.Document.test.js +++ b/tests/ce/ve.ce.Document.test.js @@ -35,7 +35,7 @@ // TODO: getDirectionFromRange QUnit.test( 'getNodeAndOffset', function ( assert ) { - var tests, i, iLen, test, parts, view, data, ceDoc, rootNode, offsetCount, offset, j, jLen, node; + var tests, i, iLen, test, parts, view, data, dmDoc, ceDoc, rootNode, offsetCount, offset, j, jLen, node, ex; // Each test below has the following: // html: an input document @@ -63,6 +63,14 @@ html: '<div><p>x</p></div>', data: [ '<div>', '<paragraph>', 'x', '</paragraph>', '</div>' ], positions: "<div class='ve-ce-branchNode ve-ce-documentNode'>|<div class='ve-ce-branchNode-slug ve-ce-branchNode-blockSlug'></div><div class='ve-ce-branchNode'>|<p class='ve-ce-branchNode ve-ce-contentBranchNode ve-ce-paragraphNode'><#text>|x|</#text></p>|</div><div class='ve-ce-branchNode-slug ve-ce-branchNode-blockSlug'></div></div>" + }, + { + title: 'Empty document', + html: '<p></p>', + unwrap: [ { type: 'paragraph' } ], + data: [], + positions: "", + dies: [ 1 ] }, { title: 'Slugless emptied paragraph', @@ -121,7 +129,20 @@ test = tests[ i ]; parts = test.positions.split( /[|]/ ); view = ve.test.utils.createSurfaceViewFromHtml( test.html ); - data = view.getModel().getDocument().data.data + dmDoc = view.getModel().getDocument(); + if ( test.unwrap ) { + new ve.dm.Surface( dmDoc ).change( + ve.dm.TransactionBuilder.static.newFromWrap( + dmDoc, + new ve.Range( 0, dmDoc.data.countNonInternalElements() ), + [], + [], + test.unwrap, + [] + ) + ); + } + data = dmDoc.data.data .slice( 0, -2 ) .map( showModelItem ); ceDoc = view.documentView; @@ -147,19 +168,6 @@ } for ( offset = 0; offset < offsetCount; offset++ ) { - try { - if ( test.dies && test.dies.indexOf( offset ) !== -1 ) { - assert.ok( false, test.title + ' (' + offset + ') does not die' ); - continue; - } - } catch ( ex ) { - assert.ok( - test.dies && test.dies.indexOf( offset ) !== -1, - test.title + ' (' + offset + ') dies' - ); - continue; - } - assert.strictEqual( ve.test.utils.serializePosition( rootNode, @@ -174,6 +182,16 @@ test.title + ' (' + offset + ')' ); } + for ( j = 0; test.dies && j < test.dies.length; j++ ) { + offset = test.dies[ j ]; + ex = null; + try { + ceDoc.getNodeAndOffset( offset, test.outsideNails ); + } catch ( e ) { + ex = e; + } + assert.ok( ex !== null, test.title + ' (' + offset + ') dies' ); + } view.destroy(); } } ); -- To view, visit https://gerrit.wikimedia.org/r/363743 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I933bf28637f3b3c235aa4b9f8d96d1d09a7b3085 Gerrit-PatchSet: 1 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: master Gerrit-Owner: C. Scott Ananian <canan...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits