From 38a9b8b3012b347a8f2516733aee18048b8e2274 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 15:54:16 -0700 Subject: [PATCH 1/5] Fix typo in selectNodes() Change-Id: I320728bafa289290c4c2a91fee736a8afb17be80 --- modules/ve2/ve.Document.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 6ca70bc2a7..27609798c6 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -80,7 +80,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { 'node': doc, 'range': new ve.Range( start, end ), 'index': 0, - 'parentRange': new ve.Range( 0, doc.getLength() ) + 'nodeRange': new ve.Range( 0, doc.getLength() ) } ]; } // TODO maybe we could find the start more efficiently using the offset map From ec449d168a3f476b6b086e1cb762a26b97617760 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 15:55:19 -0700 Subject: [PATCH 2/5] Fix startOffset computation in selectNodes() * Needs to be initialized to 1, not 0 * Needs to be stored *after* left is incremented to account for an opening Change-Id: I7978ae241578a8a17120e494684e6e93626a8529 --- modules/ve2/ve.Document.js | 40 +++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 27609798c6..2d247c11ef 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -47,7 +47,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { stack = [ { 'node': doc, 'index': 0, - 'startOffset': 0 + 'startOffset': 1 } ], node, prevNode, @@ -117,15 +117,15 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( mode == 'leaves' && node.children && node.children.length ) { // Descend into node + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - if ( node.children[0].isWrapped() ) { - left++; - } startFound = true; continue; } else { @@ -141,16 +141,16 @@ ve.Document.prototype.selectNodes = function( range, mode ) { } else if ( startInside && endInside ) { if ( node.children && node.children.length ) { // Descend into node + // If the first child of node has an opening, skip over it + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - // If the first child of node has an opening, skip over it - if ( node.children[0].isWrapped() ) { - left++; - } continue; } else { // node is a leaf node and the range is entirely inside it @@ -165,15 +165,15 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( mode == 'leaves' && node.children && node.children.length ) { // node is a branch node and the start is inside it // Descend into it + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - if ( node.children[0].isWrapped() ) { - left++; - } continue; } else { // node is a leaf node and the start is inside it @@ -194,15 +194,15 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( mode == 'leaves' && node.children && node.children.length ) { // Descend into node + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - if ( node.children[0].isWrapped() ) { - left++; - } continue; } else { // All of node is covered @@ -218,15 +218,15 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( mode == 'leaves' && node.children && node.children.length ) { // node is a branch node and the end is inside it // Descend into it + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - if ( node.children[0].isWrapped() ) { - left++; - } continue; } else { // node is a leaf node and the end is inside it @@ -247,15 +247,15 @@ ve.Document.prototype.selectNodes = function( range, mode ) { if ( mode == 'leaves' && node.children && node.children.length ) { // Descend into node + if ( node.children[0].isWrapped() ) { + left++; + } currentFrame = { 'node': node, 'index': 0, 'startOffset': left }; stack.push( currentFrame ); - if ( node.children[0].isWrapped() ) { - left++; - } continue; } else { // All of node is covered From f79e56b4f1870fe3cc792d64075647cc62708e65 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 15:57:58 -0700 Subject: [PATCH 3/5] Document the stack properties used in selectNodes() Change-Id: Ibaf178830a0b7b99a4ab1c0ec09283bd3dd552e0 --- modules/ve2/ve.Document.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 2d247c11ef..984c3be2c8 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -45,8 +45,12 @@ ve.Document.prototype.selectNodes = function( range, mode ) { start = range.start, end = range.end, stack = [ { + // Node we are currently stepping through + // Note each iteration visits a child of node, not node itself 'node': doc, + // Index of the child in node we're visiting 'index': 0, + // First offset inside node 'startOffset': 1 } ], node, From 9b6402801a7320b74984324557957354e9ffe7f0 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 16:08:15 -0700 Subject: [PATCH 4/5] Fix the selectNodes() bug that Inez reported Selecting a zero-length range at the start or end of a text node (e.g. (1,1)) would return the text node's parent instead of the text node Change-Id: I7fe089bf66b93185dd3415eff53aa7e04e3ffdb2 --- modules/ve2/ve.Document.js | 12 ++++++++++-- tests/ve2/ve.example.js | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 984c3be2c8..026fd52410 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -104,7 +104,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { // Is the end between node and nextNode or between node and the parent's closing? endBetween = node.isWrapped() ? end == right + 1 : end == right; - if ( start == end && ( startBetween || endBetween ) ) { + if ( start == end && ( startBetween || endBetween ) && node.isWrapped() ) { // Empty range in the parent, outside of any child parentFrame = stack[stack.length - 2]; return [ { @@ -132,7 +132,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { stack.push( currentFrame ); startFound = true; continue; - } else { + } else if ( !endInside ) { // All of node is covered retval.push( { 'node': node, @@ -141,6 +141,14 @@ ve.Document.prototype.selectNodes = function( range, mode ) { 'nodeRange': new ve.Range( left, right ) } ); startFound = true; + } else { + // Part of node is covered + return [ { + 'node': node, + 'range': new ve.Range( start, end ), + 'index': currentFrame.index, + 'nodeRange': new ve.Range( left, right ) + } ]; } } else if ( startInside && endInside ) { if ( node.children && node.children.length ) { diff --git a/tests/ve2/ve.example.js b/tests/ve2/ve.example.js index b4688f2a58..d29ddbb944 100644 --- a/tests/ve2/ve.example.js +++ b/tests/ve2/ve.example.js @@ -115,6 +115,20 @@ ve.example.getSelectNodesCases = function( doc ) { 'nodeRange': new ve.Range( 42, 52 ) } ] + }, + // Zero-length range at the edge of a text node returns that text node rather than + // its parent + { + 'actual': doc.selectNodes( new ve.Range( 1, 1 ), 'leaves' ), + 'expected': [ + // heading/text + { + 'node': lookup( documentNode, 0, 0 ), + 'range': new ve.Range( 1, 1 ), + 'index': 0, + 'nodeRange': new ve.Range( 1, 4 ) + } + ] } ]; }; From 8834145f9e975114fe2555dfd1d3d07fe423707b Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 10 May 2012 16:42:16 -0700 Subject: [PATCH 5/5] Fix nodeSelectionEqual() so it doesn't explode when a and b have unequal lengths Change-Id: Idb84bc1238576c711a201cdbf05f63a7b603a819 --- modules/ve2/ve.Document.js | 2 +- tests/ve2/ve.example.js | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/modules/ve2/ve.Document.js b/modules/ve2/ve.Document.js index 026fd52410..f9664adb0a 100644 --- a/modules/ve2/ve.Document.js +++ b/modules/ve2/ve.Document.js @@ -168,7 +168,7 @@ ve.Document.prototype.selectNodes = function( range, mode ) { // node is a leaf node and the range is entirely inside it return [ { 'node': node, - 'range': new ve.Range( left, right ), + 'range': new ve.Range( start, end ), 'index': currentFrame.index, 'nodeRange': new ve.Range( left, right ) } ]; diff --git a/tests/ve2/ve.example.js b/tests/ve2/ve.example.js index d29ddbb944..5a6fa1cb76 100644 --- a/tests/ve2/ve.example.js +++ b/tests/ve2/ve.example.js @@ -129,6 +129,31 @@ ve.example.getSelectNodesCases = function( doc ) { 'nodeRange': new ve.Range( 1, 4 ) } ] + }, + { + 'actual': doc.selectNodes( new ve.Range( 4, 4 ), 'leaves' ), + 'expected': [ + // heading/text + { + 'node': lookup( documentNode, 0, 0 ), + 'range': new ve.Range( 4, 4 ), + 'index': 0, + 'nodeRange': new ve.Range( 1, 4 ) + } + ] + }, + // Range entirely within one leaf node + { + 'actual': doc.selectNodes( new ve.Range( 2, 3 ), 'leaves' ), + 'expected': [ + // heading/text + { + 'node': lookup( documentNode, 0, 0 ), + 'range': new ve.Range( 2, 3 ), + 'index': 0, + 'nodeRange': new ve.Range( 1, 4 ) + } + ] } ]; }; @@ -161,8 +186,9 @@ ve.example.nodeTreeEqual = function( a, b ) { * @method */ ve.example.nodeSelectionEqual = function( a, b ) { + var minLength = a.length < b.length ? a.length : b.length; equal( a.length, b.length, 'length match' ); - for ( var i = 0; i < a.length; i++ ) { + for ( var i = 0; i < minLength; i++ ) { ok( a[i].node === b[i].node, 'node match' ); if ( a[i].range && b[i].range ) { deepEqual( a[i].range, b[i].range, 'range match' );