jenkins-bot has submitted this change and it was merged.
Change subject: Prevent deletion of FocusableNodes from a collapsed selection
......................................................................
Prevent deletion of FocusableNodes from a collapsed selection
Instead select the node and require the user to press delete
again if they really meant to delete the node.
Also test cases!
Bug: 55336
Change-Id: I66520e18740e78ce6313f9b31bb575d06b91bea8
---
M VisualEditor.hooks.php
A modules/ve-mw/test/ce/ve.ce.Surface.test.js
M modules/ve-mw/test/dm/ve.dm.mwExample.js
M modules/ve/ce/ve.ce.Surface.js
M modules/ve/test/ce/ve.ce.Surface.test.js
5 files changed, 167 insertions(+), 50 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/VisualEditor.hooks.php b/VisualEditor.hooks.php
index 53c7908..ab501fc 100644
--- a/VisualEditor.hooks.php
+++ b/VisualEditor.hooks.php
@@ -356,6 +356,7 @@
've/test/ce/ve.ce.Document.test.js',
've/test/ce/ve.ce.Surface.test.js',
've-mw/test/ce/ve.ce.Document.test.js',
+ 've-mw/test/ce/ve.ce.Surface.test.js',
've/test/ce/ve.ce.NodeFactory.test.js',
've/test/ce/ve.ce.Node.test.js',
've/test/ce/ve.ce.BranchNode.test.js',
diff --git a/modules/ve-mw/test/ce/ve.ce.Surface.test.js
b/modules/ve-mw/test/ce/ve.ce.Surface.test.js
new file mode 100644
index 0000000..1eb190c
--- /dev/null
+++ b/modules/ve-mw/test/ce/ve.ce.Surface.test.js
@@ -0,0 +1,60 @@
+/*!
+ * VisualEditor ContentEditable Surface tests.
+ *
+ * @copyright 2011-2013 VisualEditor Team and others; see AUTHORS.txt
+ * @license The MIT License (MIT); see LICENSE.txt
+ */
+
+QUnit.module( 've.ce.Surface' );
+
+/* Tests */
+
+QUnit.test( 'handleDelete', function ( assert ) {
+ var i,
+ cases = [
+ {
+ 'html':
+ '<p>Foo</p>' +
+
ve.dm.mwExample.MWTransclusion.blockOpen +
ve.dm.mwExample.MWTransclusion.blockContent +
+ '<p>Bar</p>',
+ 'range': new ve.Range( 4 ),
+ 'operations': ['delete'],
+ 'expectedData': function () {},
+ 'expectedRange': new ve.Range( 5, 7 ),
+ 'msg': 'Block transclusion is focused not
deleted'
+ },
+ {
+ 'html':
+ '<p>Foo</p>' +
+
ve.dm.mwExample.MWTransclusion.blockOpen +
ve.dm.mwExample.MWTransclusion.blockContent +
+ '<p>Bar</p>',
+ 'range': new ve.Range( 4 ),
+ 'operations': ['delete', 'delete'],
+ 'expectedData': function ( data ) {
+ data.splice( 5, 2 );
+ },
+ 'expectedRange': new ve.Range( 5, 5 ),
+ 'msg': 'Block transclusion is deleted with two
keypresses'
+ },
+ {
+ 'html':
+ '<p>Foo</p>' +
+ ve.dm.mwExample.MWBlockImage.html +
+ '<p>Bar</p>',
+ 'range': new ve.Range( 4 ),
+ 'operations': ['delete'],
+ 'expectedData': function () { },
+ 'expectedRange': new ve.Range( 5, 14 ),
+ 'msg': 'Block image is focused not deleted'
+ }
+ ];
+
+ QUnit.expect( cases.length * 2 );
+
+ for ( i = 0; i < cases.length; i++ ) {
+ ve.test.utils.runSurfaceHandleDeleteTest(
+ assert, cases[i].html, cases[i].range,
cases[i].operations,
+ cases[i].expectedData, cases[i].expectedRange,
cases[i].msg
+ );
+ }
+} );
diff --git a/modules/ve-mw/test/dm/ve.dm.mwExample.js
b/modules/ve-mw/test/dm/ve.dm.mwExample.js
index a37350b..fc6939a 100644
--- a/modules/ve-mw/test/dm/ve.dm.mwExample.js
+++ b/modules/ve-mw/test/dm/ve.dm.mwExample.js
@@ -126,6 +126,37 @@
'value': $( ve.dm.mwExample.MWTransclusion.mixed ).toArray()
};
+ve.dm.mwExample.MWBlockImage = {
+ 'html':
+ '<figure typeof="mw:Image/Thumb" class="mw-halign-right
foobar">' +
+ '<a href="Foo"><img src="Bar" width="1" height="2"
resource="FooBar"></a>' +
+ '<figcaption>abc</figcaption>' +
+ '</figure>',
+ 'data': [
+ {
+ 'type': 'mwBlockImage',
+ 'attributes': {
+ 'type': 'thumb',
+ 'align': 'right',
+ 'href': 'Foo',
+ 'src': 'Bar',
+ 'width': '1',
+ 'height': '2',
+ 'resource': 'FooBar',
+ 'originalClasses': 'mw-halign-right foobar',
+ 'unrecognizedClasses': ['foobar']
+ }
+ },
+ { 'type': 'mwImageCaption' },
+ { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } },
+ 'a', 'b', 'c',
+ { 'type': '/paragraph' },
+ { 'type': '/mwImageCaption' },
+ { 'type': '/mwBlockImage' }
+ ]
+};
+
+
ve.dm.mwExample.MWReference = {
'referenceList':
'<ol class="references" typeof="mw:Extension/references"
about="#mwt7" data-parsoid="{}"' +
@@ -1613,31 +1644,11 @@
]
},
'thumb image': {
- 'html': '<body><figure typeof="mw:Image/Thumb"
class="mw-halign-right foobar"><a href="Foo"><img src="Bar" width="1"
height="2" resource="FooBar"></a><figcaption>abc</figcaption></figure></body>',
- 'data': [
- {
- 'type': 'mwBlockImage',
- 'attributes': {
- 'type': 'thumb',
- 'align': 'right',
- 'href': 'Foo',
- 'src': 'Bar',
- 'width': '1',
- 'height': '2',
- 'resource': 'FooBar',
- 'originalClasses': 'mw-halign-right
foobar',
- 'unrecognizedClasses': ['foobar']
- }
- },
- { 'type': 'mwImageCaption' },
- { 'type': 'paragraph', 'internal': { 'generated':
'wrapper' } },
- 'a', 'b', 'c',
- { 'type': '/paragraph' },
- { 'type': '/mwImageCaption' },
- { 'type': '/mwBlockImage' },
+ 'html': '<body>' + ve.dm.mwExample.MWBlockImage.html +
'</body>',
+ 'data': ve.dm.mwExample.MWBlockImage.data.concat( [
{ 'type': 'internalList' },
{ 'type': '/internalList' }
- ]
+ ] )
},
'attribute preservation does not crash due to text node split': {
'html':
diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js
index 46aad2d..be9245f 100644
--- a/modules/ve/ce/ve.ce.Surface.js
+++ b/modules/ve/ce/ve.ce.Surface.js
@@ -1512,7 +1512,8 @@
*/
ve.ce.Surface.prototype.handleDelete = function ( e, backspace ) {
var rangeToRemove = this.model.getSelection(),
- tx, startNode, endNode, endNodeData, nodeToDelete;
+ offset = 0,
+ docLength, tx, startNode, endNode, endNodeData, nodeToDelete;
if ( rangeToRemove.isCollapsed() ) {
// In case when the range is collapsed use the same logic that
is used for cursor left and
@@ -1523,6 +1524,20 @@
( e.altKey === true || e.ctrlKey === true ) ? 'word' :
'character',
true
);
+ offset = rangeToRemove.start;
+ docLength = this.model.getDocument().data.getLength();
+ if ( offset < docLength ) {
+ while ( offset < docLength &&
this.model.getDocument().data.isCloseElementData( offset ) ) {
+ offset++;
+ }
+ // If the user tries to delete a focusable node from a
collapsed selection,
+ // just select the node and cancel the deletion.
+ startNode =
this.documentView.getDocumentNode().getNodeFromOffset( offset + 1 );
+ if ( ve.isMixedIn( startNode, ve.ce.FocusableNode ) ) {
+ this.model.change( null,
startNode.getModel().getOuterRange() );
+ return;
+ }
+ }
if ( rangeToRemove.isCollapsed() ) {
// For instance beginning or end of the document.
return;
diff --git a/modules/ve/test/ce/ve.ce.Surface.test.js
b/modules/ve/test/ce/ve.ce.Surface.test.js
index 13093eb..bb51859 100644
--- a/modules/ve/test/ce/ve.ce.Surface.test.js
+++ b/modules/ve/test/ce/ve.ce.Surface.test.js
@@ -9,19 +9,41 @@
/* Tests */
-QUnit.test( 'handleDelete', function ( assert ) {
- var i,
- surface = ve.test.utils.createSurfaceFromHtml(
ve.dm.example.html ),
- view = surface.getView(),
- model = surface.getModel(),
- data = ve.copy( model.getDocument().getFullData() ),
- originalData = ve.copy( data ),
+ve.test.utils.runSurfaceHandleDeleteTest = function( assert, html, range,
operations, expectedData, expectedRange, msg ) {
+ var i, args,
+ selection,
deleteArgs = {
'backspace': [ {}, true ],
'delete': [ {}, false ],
'modifiedBackspace': [ { 'ctrlKey': true }, true ],
'modifiedDelete': [ { 'ctrlKey': true }, false ]
},
+ surface = ve.test.utils.createSurfaceFromHtml( html ||
ve.dm.example.html ),
+ view = surface.getView(),
+ model = surface.getModel(),
+ data = ve.copy( model.getDocument().getFullData() );
+
+ // TODO: model.getSelection() should be consistent after it has been
+ // changed but appears to behave differently depending on the browser.
+ // The selection from the change event is still consistent.
+ model.on( 'change', function( tx, s ) {
+ selection = s;
+ } );
+
+ model.change( null, range );
+ for ( i = 0; i < operations.length; i++ ) {
+ args = deleteArgs[operations[i]];
+ view.handleDelete( args[0], args[1] );
+ }
+ expectedData( data );
+
+ assert.deepEqualWithDomElements( model.getDocument().getFullData(),
data, msg + ': data' );
+ assert.deepEqual( selection, expectedRange, msg + ': range' );
+ surface.destroy();
+};
+
+QUnit.test( 'handleDelete', function ( assert ) {
+ var i,
cases = [
{
'range': new ve.Range( 2 ),
@@ -85,31 +107,39 @@
},
'expectedRange': new ve.Range( 1 ),
'msg': 'Empty node deleted by delete'
+ },
+ {
+ 'range': new ve.Range( 41 ),
+ 'operations': ['backspace'],
+ 'expectedData': function () {},
+ 'expectedRange': new ve.Range( 39, 41 ),
+ 'msg': 'Focusable node selected but not deleted
by backspace'
+ },
+ {
+ 'range': new ve.Range( 39 ),
+ 'operations': ['delete'],
+ 'expectedData': function () {},
+ 'expectedRange': new ve.Range( 39, 41 ),
+ 'msg': 'Focusable node selected but not deleted
by delete'
+ },
+ {
+ 'range': new ve.Range( 39, 41 ),
+ 'operations': ['delete'],
+ 'expectedData': function ( data ) {
+ data.splice( 39, 2 );
+ },
+ 'expectedRange': new ve.Range( 39 ),
+ 'msg': 'Focusable node deleted if selected
first'
}
];
-
- function testRunner( range, operations, expectedData, expectedRange,
msg ) {
- var i, args, data = ve.copy( originalData );
- model.change( null, range );
- for ( i = 0; i < operations.length; i++ ) {
- args = deleteArgs[operations[i]];
- view.handleDelete( args[0], args[1] );
- }
- expectedData( data );
-
- assert.deepEqual( model.getDocument().getFullData(), data, msg
+ ': data' );
- assert.deepEqual( model.getSelection(), expectedRange, msg + ':
range' );
-
- // Roll back the test Surface
- while ( model.undo() ) {
- /*jshint noempty:false */
- }
- }
QUnit.expect( cases.length * 2 );
for ( i = 0; i < cases.length; i++ ) {
- testRunner( cases[i].range, cases[i].operations,
cases[i].expectedData, cases[i].expectedRange, cases[i].msg );
+ ve.test.utils.runSurfaceHandleDeleteTest(
+ assert, cases[i].html, cases[i].range,
cases[i].operations,
+ cases[i].expectedData, cases[i].expectedRange,
cases[i].msg
+ );
}
} );
--
To view, visit https://gerrit.wikimedia.org/r/87666
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I66520e18740e78ce6313f9b31bb575d06b91bea8
Gerrit-PatchSet: 12
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Esanders <[email protected]>
Gerrit-Reviewer: Jforrester <[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
