jenkins-bot has submitted this change and it was merged.
Change subject: Fix range translation for surface fragments
......................................................................
Fix range translation for surface fragments
Remove all manual changes to SF ranges as these are not
undoable. Instead change translate range to default to
outer expand and build functionality around that behaviour
never changing.
As translate range is always outer I don't think we need to
check for start and end crossing over?
Added more undo tests to assert these selections are maintained
properly, and added the test case to 'update' for when and undo
point is overwritten.
Insert content now results in a selection over the inserted
content. Most usages were expecting this anyway and were
followed up with an adjustRange(-length,0) which is no longer
necessary.
Noticed that the link inspector case was never being triggered
as word boundary was always expanding to at least one char (mainly
for Hanzi selection). This doesn't make much sense as single
spaces get auto selected so removed this functionality.
Split collapseRange out into collapseRangeToStart and
collapseRangeToEnd as this may be required to get the old
behaviour (range moves to end after insert).
Change-Id: I3dc0b4d00d37bad1ca3076a69b41c5f0b3fa0570
---
M modules/ve/ce/ve.ce.Surface.js
M modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
M modules/ve/dm/ve.dm.SurfaceFragment.js
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
M modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
M modules/ve/test/dm/ve.dm.Transaction.test.js
M modules/ve/ui/inspectors/ve.ui.LinkInspector.js
8 files changed, 135 insertions(+), 103 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js
index 23093f6..7687d3e 100644
--- a/modules/ve/ce/ve.ce.Surface.js
+++ b/modules/ve/ce/ve.ce.Surface.js
@@ -326,8 +326,7 @@
originFragment.removeContent();
// Re-insert node at new location and re-select it
- targetFragment.insertContent( nodeData );
- targetFragment.adjustRange( -nodeData.length, 0
).select();
+ targetFragment.insertContent( nodeData ).select();
}, this ) );
}
diff --git a/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
b/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
index ddc2bf1..c6c83de 100644
--- a/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
+++ b/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
@@ -705,13 +705,7 @@
} else if ( !unicodeJS.wordbreak.isBreak( dataString, offset -
1 ) ) {
offset--;
} else {
- // just return one character to the right, unless we
are at the end
- // of the text, in which case the character to the left
- if ( dataString.read( offset ) !== null ) {
- return new ve.Range( offset, offset + 1 );
- } else {
- return new ve.Range( offset - 1, offset );
- }
+ return new ve.Range( offset );
}
}
diff --git a/modules/ve/dm/ve.dm.SurfaceFragment.js
b/modules/ve/dm/ve.dm.SurfaceFragment.js
index 5eaf446..9ab7f23 100644
--- a/modules/ve/dm/ve.dm.SurfaceFragment.js
+++ b/modules/ve/dm/ve.dm.SurfaceFragment.js
@@ -22,7 +22,7 @@
// Properties
this.surface = surface;
- this.setRange( range && range instanceof ve.Range ? range :
surface.getSelection() );
+ this.range = range && range instanceof ve.Range ? range :
surface.getSelection();
// Short-circuit for invalid range null fragment
if ( !this.range ) {
return this;
@@ -30,14 +30,14 @@
this.document = surface.getDocument();
this.noAutoSelect = !!noAutoSelect;
-
// Initialization
var length = this.document.data.getLength();
- this.setRange( new ve.Range(
+ this.range = new ve.Range(
// Clamp range to valid document offsets
Math.min( Math.max( this.range.from, 0 ), length ),
Math.min( Math.max( this.range.to, 0 ), length )
- ) );
+ );
+ this.historyPointer = this.getSurface().getCompleteHistoryLength();
};
/* Static Properties */
@@ -103,17 +103,6 @@
};
/**
- * Update the range of the transaction and reset the history pointer.
- *
- * @method
- * @param {ve.Range} range New range
- */
-ve.dm.SurfaceFragment.prototype.setRange = function ( range ) {
- this.historyPointer = this.getSurface().getCompleteHistoryLength();
- this.range = range;
-};
-
-/**
* Check if the fragment is null.
*
* @method
@@ -168,13 +157,29 @@
* @method
* @returns {ve.dm.SurfaceFragment} Collapsed fragment
*/
-ve.dm.SurfaceFragment.prototype.collapseRange = function () {
+ve.dm.SurfaceFragment.prototype.collapseRangeToStart = function () {
// Handle null fragment
if ( !this.surface ) {
return this;
}
return new ve.dm.SurfaceFragment(
this.surface, new ve.Range( this.getRange( true ).start ),
this.noAutoSelect
+ );
+};
+
+/**
+ * Get a new fragment with a zero-length selection at the end offset.
+ *
+ * @method
+ * @returns {ve.dm.SurfaceFragment} Collapsed fragment
+ */
+ve.dm.SurfaceFragment.prototype.collapseRangeToEnd = function () {
+ // Handle null fragment
+ if ( !this.surface ) {
+ return this;
+ }
+ return new ve.dm.SurfaceFragment(
+ this.surface, new ve.Range( this.getRange( true ).end ),
this.noAutoSelect
);
};
@@ -546,17 +551,6 @@
if ( this.getRange( true ).getLength() ) {
tx = ve.dm.Transaction.newFromRemoval( this.document,
this.getRange() );
this.surface.change( tx, !this.noAutoSelect &&
tx.translateRange( this.getRange() ) );
- // Check if the range didn't get collapsed automatically - this
will occur when removing
- // content across un-mergable nodes because the delete only
strips out content leaving
- // structure at the beginning and end of the range in place
- if ( this.getRange( true ).getLength() ) {
- // Collapse the range manually
- this.range = new ve.Range( this.getRange( true ).start
);
- if ( !this.noAutoSelect ) {
- // Update the surface selection
- this.surface.change( null, this.getRange() );
- }
- }
}
return this;
};
@@ -607,10 +601,8 @@
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
}
- var tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
[], [], [], wrapper ),
- newRange = new ve.Range( this.getRange( true ).start,
this.getRange( true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
- this.setRange( newRange );
+ var tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
[], [], [], wrapper );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -632,7 +624,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, innerUnwrapper = [], outerUnwrapper = [];
+ var i, tx, innerUnwrapper = [], outerUnwrapper = [];
if ( this.getRange( true ).end - this.getRange( true ).start <
innerDepth * 2 ) {
throw new Error( 'cannot unwrap by greater depth than maximum
theoretical depth of selection' );
@@ -646,10 +638,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
outerUnwrapper, [], innerUnwrapper, [] );
- newRange = new ve.Range( this.getRange( true ).start - outerDepth,
this.getRange( true ).end + outerDepth + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -680,7 +669,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, unwrapper = [];
+ var i, tx, unwrapper = [];
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
@@ -695,10 +684,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(), [],
[], unwrapper, wrapper );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -728,17 +714,14 @@
return this;
}
- var tx, newRange;
+ var tx;
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(), [],
wrapper, [], [] );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -766,7 +749,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, unwrapper = [],
+ var i, tx, unwrapper = [],
innerRange = new ve.Range( this.getRange( true ).start + depth,
this.getRange( true ).end - depth );
if ( !ve.isArray( wrapper ) ) {
@@ -782,10 +765,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, innerRange,
unwrapper, wrapper, [], [] );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -816,8 +796,7 @@
endSplitRequired = false,
startSplitNodes = [],
endSplitNodes = [],
- fragment = this,
- newRange = new ve.Range( this.getRange( true ).start,
this.getRange( true ).end );
+ fragment = this;
function createSplits( splitNodes, insertBefore ) {
var i, length,
@@ -833,8 +812,7 @@
}
tx = ve.dm.Transaction.newFromInsertion(
fragment.getDocument(), insertBefore ? startOffset : endOffset, data );
- newRange = new ve.Range( newRange.start, newRange.end +
tx.getLengthDifference() );
- fragment.surface.change( tx, !fragment.noAutoSelect && newRange
);
+ fragment.surface.change( tx, !fragment.noAutoSelect &&
tx.translateRange( fragment.getRange() ) );
startOffset += startOffsetChange;
endOffset += endOffsetChange;
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index b2a9a7d..7d9bffb 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -743,13 +743,8 @@
* @returns {ve.Range} Translated range, as it will be after processing
transaction
*/
ve.dm.Transaction.prototype.translateRange = function ( range, reversed ) {
- var start = this.translateOffset( range.start, reversed, false ),
- end = this.translateOffset( range.end, reversed, true );
- if ( range.start <= range.end && start > end ) {
- // translateOffset() has mapped end to jump over start
- // Disable the excludeInsertion behavior in this case
+ var start = this.translateOffset( range.start, reversed, true ),
end = this.translateOffset( range.end, reversed, false );
- }
return range.isBackwards() ? new ve.Range( end, start ) : new ve.Range(
start, end );
};
diff --git a/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
b/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
index 8327018..3082e25 100644
--- a/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
+++ b/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
@@ -1312,13 +1312,13 @@
'phrase': '维基百科',
'msg': 'Hanzi characters (cursor in middle)',
'offset': 2,
- 'expected': '百'
+ 'expected': ''
},
{
'phrase': '维基百科',
'msg': 'Hanzi characters (cursor at end)',
'offset': 4,
- 'expected': '科'
+ 'expected': ''
},
{
'phrase': 'Costs £1,234.00 each',
diff --git a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
index e71b952..e0108bc 100644
--- a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
+++ b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
@@ -26,27 +26,48 @@
assert.strictEqual( fragment.willAutoSelect(), false, 'noAutoSelect
values are boolean' );
} );
-QUnit.test( 'update', 2, function ( assert ) {
+QUnit.test( 'update', 3, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
- fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range(
1, 56 ) ),
- fragment2 = new ve.dm.SurfaceFragment( surface, new ve.Range(
2, 4 ) ),
- fragment3 = new ve.dm.SurfaceFragment( surface, new ve.Range(
2, 4 ) );
- fragment1.removeContent();
+ fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) ),
+ fragment2 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) ),
+ fragment3 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) );
+ fragment1.wrapNodes(
+ [{ 'type': 'list', 'attributes': { 'style': 'bullet' } }, {
'type': 'listItem' }]
+ );
assert.deepEqual(
fragment2.getRange(),
- new ve.Range( 1, 1 ),
- 'fragment range collapses after removeContent'
+ new ve.Range( 55, 69 ),
+ 'fragment range changes after wrapNodes'
);
surface.undo();
assert.deepEqual(
fragment3.getRange(),
- new ve.Range( 4, 4 ),
- 'fragment range moved after undo'
+ new ve.Range( 55, 61 ),
+ 'fragment range restored after undo'
);
+
+ fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 1 ) );
+ surface.breakpoint();
+
+ fragment1.insertContent( '01' );
+ surface.breakpoint();
+
+ fragment1 = fragment1.collapseRangeToEnd();
+ fragment1.insertContent( '234' );
+ fragment2 = fragment1.adjustRange();
+
+ surface.undo();
+ fragment1.insertContent( '5678' );
+ assert.deepEqual(
+ fragment2.getRange(),
+ new ve.Range( 3, 7 ),
+ 'Range created during truncated undo point still translates
correctly'
+ );
+
} );
-QUnit.test( 'adjustRange', 3, function ( assert ) {
+QUnit.test( 'adjustRange', 4, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range(
20, 21 ) ),
@@ -54,16 +75,24 @@
assert.ok( fragment !== adjustedFragment, 'adjustRange produces a new
fragment' );
assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
assert.deepEqual( adjustedFragment.getRange(), new ve.Range( 1, 56 ),
'new range is used' );
+
+ adjustedFragment = fragment.adjustRange();
+ assert.deepEqual( adjustedFragment, fragment, 'fragment is clone if no
parameters supplied' );
} );
-QUnit.test( 'collapseRange', 3, function ( assert ) {
+QUnit.test( 'collapseRangeToStart/End', 6, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range(
20, 21 ) ),
- collapsedFragment = fragment.collapseRange();
- assert.ok( fragment !== collapsedFragment, 'collapseRange produces a
new fragment' );
+ collapsedFragment = fragment.collapseRangeToStart();
+ assert.ok( fragment !== collapsedFragment, 'collapseRangeToStart
produces a new fragment' );
assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
assert.deepEqual( collapsedFragment.getRange(), new ve.Range( 20, 20 ),
'new range is used' );
+
+ collapsedFragment = fragment.collapseRangeToEnd();
+ assert.ok( fragment !== collapsedFragment, 'collapseRangeToEnd produces
a new fragment' );
+ assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
+ assert.deepEqual( collapsedFragment.getRange(), new ve.Range( 21, 21 ),
'range is at end when collapseToEnd is set' );
} );
QUnit.test( 'expandRange (closest)', 1, function ( assert ) {
@@ -112,13 +141,15 @@
}
} );
-QUnit.test( 'removeContent', 2, function ( assert ) {
+QUnit.test( 'removeContent', 6, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
+ originalDoc = ve.dm.example.createExampleDocument(),
+ expectedDoc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1,
56 ) ),
- expectedData = ve.copyArray( ve.dm.example.data.slice( 0, 1 ) )
- .concat( ve.copyArray( ve.dm.example.data.slice( 4, 5 )
) )
- .concat( ve.copyArray( ve.dm.example.data.slice( 55 ) )
);
+ expectedData = ve.copyArray( expectedDoc.data.slice( 0, 1 ) )
+ .concat( ve.copyArray( expectedDoc.data.slice( 4, 5 ) )
)
+ .concat( ve.copyArray( expectedDoc.data.slice( 55 ) ) );
ve.setProp( expectedData[0], 'internal', 'changed', 'content', 1 );
fragment.removeContent();
assert.deepEqual(
@@ -128,12 +159,39 @@
);
assert.deepEqual(
fragment.getRange(),
+ new ve.Range( 1, 3 ),
+ 'removing content results in a fragment covering just remaining
structure'
+ );
+ surface.undo();
+ assert.deepEqual(
+ doc.getData(),
+ originalDoc.getData(),
+ 'content restored after undo'
+ );
+ assert.deepEqual(
+ fragment.getRange(),
+ new ve.Range( 1, 56 ),
+ 'range restored after undo'
+ );
+
+ fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 4 ) );
+ fragment.removeContent();
+ assert.deepEqual(
+ doc.getData( new ve.Range( 0, 2 ) ),
+ [
+ { 'type': 'heading', 'attributes': { 'level': 1 },
'internal': { 'changed': { 'content' : 1 } } },
+ { 'type': '/heading'}
+ ],
+ 'removing content empties node'
+ );
+ assert.deepEqual(
+ fragment.getRange(),
new ve.Range( 1, 1 ),
- 'removing content results in a zero-length fragment'
+ 'removing content collapses range'
);
} );
-QUnit.test( 'insertContent', 3, function ( assert ) {
+QUnit.test( 'insertContent', 4, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1,
4 ) );
@@ -145,15 +203,26 @@
);
assert.deepEqual(
fragment.getRange(),
- new ve.Range( 4, 4 ),
- 'inserting content results in a zero-length fragment'
+ new ve.Range( 1, 4 ),
+ 'inserting content results in range around content'
);
+
+ surface.breakpoint();
+ fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 4 ) );
fragment.insertContent( '321' );
assert.deepEqual(
doc.getData( new ve.Range( 4, 7 ) ),
['3', '2', '1'],
'strings get converted into data when inserting content'
);
+
+ surface.undo();
+ fragment.getRange();
+ assert.deepEqual(
+ fragment.getRange(),
+ new ve.Range( 4 ),
+ 'range restored after undo'
+ );
} );
QUnit.test( 'wrapNodes/unwrapNodes', 10, function ( assert ) {
diff --git a/modules/ve/test/dm/ve.dm.Transaction.test.js
b/modules/ve/test/dm/ve.dm.Transaction.test.js
index 82769f4..bf2ec06 100644
--- a/modules/ve/test/dm/ve.dm.Transaction.test.js
+++ b/modules/ve/test/dm/ve.dm.Transaction.test.js
@@ -1006,8 +1006,8 @@
cases = [
{
'before': new ve.Range( 55, 61 ),
- 'after': new ve.Range( 57, 65 ),
- 'msg': 'Wrapped range is translated to inner range'
+ 'after': new ve.Range( 55, 67 ),
+ 'msg': 'Wrapped range is translated to outer range'
},
{
'before': new ve.Range( 54, 62 ),
@@ -1016,12 +1016,12 @@
},
{
'before': new ve.Range( 54, 61 ),
- 'after': new ve.Range( 54, 65 ),
+ 'after': new ve.Range( 54, 67 ),
'msg': 'Wrapped range plus one on the left'
},
{
'before': new ve.Range( 55, 62 ),
- 'after': new ve.Range( 57, 68 ),
+ 'after': new ve.Range( 55, 68 ),
'msg': 'wrapped range plus one on the right'
}
];
diff --git a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
index ff08744..b69c3c5 100644
--- a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
+++ b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
@@ -136,7 +136,7 @@
// Call parent method
ve.ui.Inspector.prototype.onClose.call( this, action );
- var i, len, annotations, selection, adjustedFragment,
+ var i, len, annotations, selection,
insert = false,
undo = false,
clear = false,
@@ -165,13 +165,10 @@
}
}
if ( insert ) {
- // Insert default text and select it
fragment.insertContent( target, false );
- adjustedFragment = fragment.adjustRange( -target.length, 0 );
- fragment = adjustedFragment;
- // Move cursor to the end of the inserted content
- selection = new ve.Range( this.initialSelection.start +
target.length );
+ // Move cursor to the end of the inserted content, even if back
button is used
+ this.previousSelection = new ve.Range(
this.initialSelection.start + target.length );
}
if ( undo ) {
// Go back to before we added an annotation
--
To view, visit https://gerrit.wikimedia.org/r/60973
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3dc0b4d00d37bad1ca3076a69b41c5f0b3fa0570
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Trevor Parscal <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
