From 13ba9b7de87e6cf17260d4a5960dc0a5c265fbb6 Mon Sep 17 00:00:00 2001 From: Catrope Date: Thu, 5 Jul 2012 12:55:52 -0700 Subject: [PATCH] (bug 37980) Cutting all text and repasting it breaks the editor This was caused by a bug in fixupInsertion that caused it to believe that inserting something like "a

b

c" into the middle of an empty paragraph was invalid. This commit fixes the fixupInsertion bug, which fixes the select-all-cut-paste behavior in Chrome. It's still broken in Firefox because of selection-related issues, but I'll split that out into a different bug report. Change-Id: I767f5d37ec7e511778ae9ca8283ec4b26c728298 --- modules/ve/dm/ve.dm.Document.js | 90 ++++++++++++++------------- tests/ve/dm/ve.dm.Transaction.test.js | 25 ++++++++ 2 files changed, 71 insertions(+), 44 deletions(-) diff --git a/modules/ve/dm/ve.dm.Document.js b/modules/ve/dm/ve.dm.Document.js index 4471ea2d57..5e58cfbaa9 100644 --- a/modules/ve/dm/ve.dm.Document.js +++ b/modules/ve/dm/ve.dm.Document.js @@ -922,7 +922,7 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { // Array of element openings (object). Openings in data are pushed onto this stack // when they are encountered and popped off when they are closed openingStack = [], - // Array of element types (strings). Closings in data that close nodes that were + // Array of node objects. Closings in data that close nodes that were // not opened in data (i.e. were already in the document) are pushed onto this stack // and popped off when balanced out by an opening in data closingStack = [], @@ -977,18 +977,23 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { */ function writeElement( element, index ) { var expectedType; + if ( element.type !== undefined ) { + // Content, do nothing if ( element.type.charAt( 0 ) !== '/' ) { // Opening - // Check if this opening balances an earlier closing of a node - // that was already in the document. This is only the case if - // openingStack is empty (otherwise we still have unclosed nodes from - // within data) and if this opening matches the top of closingStack + // Check if this opening balances an earlier closing of a node that was already in + // the document. This is only the case if openingStack is empty (otherwise we still + // have unclosed nodes from within data) and if this opening matches the top of + // closingStack if ( openingStack.length === 0 && closingStack.length > 0 && - closingStack[closingStack.length - 1] === element.type + closingStack[closingStack.length - 1].getType() === element.type ) { // The top of closingStack is now balanced out, so remove it - closingStack.pop(); + // Also restore parentNode from closingStack. While this is technically not + // entirely accurate (the current node is a new node that's a sibling of this + // node), it's good enough for the purposes of this algorithm + parentNode = closingStack.pop(); } else { // This opens something new, put it on openingStack openingStack.push( element ); @@ -998,29 +1003,28 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { // Closing // Make sure that this closing matches the currently opened node if ( openingStack.length > 0 ) { - // The opening was on openingStack, so we're closing - // a node that was opened within data. Don't track - // that on closingStack + // The opening was on openingStack, so we're closing a node that was opened + // within data. Don't track that on closingStack expectedType = openingStack.pop().type; } else { - // openingStack is empty, so we're closing a node that - // was already in the document. This means we have to - // reopen it later, so track this on closingStack + // openingStack is empty, so we're closing a node that was already in the + // document. This means we have to reopen it later, so track this on + // closingStack expectedType = parentNode.getType(); - closingStack.push( expectedType ); + closingStack.push( parentNode ); parentNode = parentNode.getParent(); if ( !parentNode ) { throw 'Inserted data is trying to close the root node ' + '(at index ' + index + ')'; } - } - parentType = expectedType; + parentType = expectedType; - // Validate - // FIXME this breaks certain input, should fix it up, not scream and die - if ( element.type !== '/' + expectedType ) { - throw 'Type mismatch, expected /' + expectedType + - ' but got ' + element.type + ' (at index ' + index + ')'; + // Validate + // FIXME this breaks certain input, should fix it up, not scream and die + if ( element.type !== '/' + expectedType ) { + throw 'Type mismatch, expected /' + expectedType + + ' but got ' + element.type + ' (at index ' + index + ')'; + } } } } @@ -1056,11 +1060,11 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { closings = []; reopenElements = []; // Opening or content - // Make sure that opening this element here does not violate the - // parent/children/content rules. If it does, insert stuff to fix it + // Make sure that opening this element here does not violate the parent/children/content + // rules. If it does, insert stuff to fix it - // If this node is content, check that the containing node can contain - // content. If not, wrap in a paragraph + // If this node is content, check that the containing node can contain content. If not, + // wrap in a paragraph if ( ve.dm.nodeFactory.isNodeContent( childType ) && !ve.dm.nodeFactory.canNodeContainContent( parentType ) ) { @@ -1068,8 +1072,8 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { openings.unshift ( { 'type': 'paragraph' } ); } - // Check that this node is allowed to have the containing node as its - // parent. If not, wrap it until it's fixed + // Check that this node is allowed to have the containing node as its parent. If not, + // wrap it until it's fixed do { allowedParents = ve.dm.nodeFactory.getParentNodeTypes( childType ); parentsOK = allowedParents === null || @@ -1086,14 +1090,14 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { } } while ( !parentsOK ); - // Check that the containing node can have this node as its child. If not, - // close nodes until it's fixed + // Check that the containing node can have this node as its child. If not, close nodes + // until it's fixed do { allowedChildren = ve.dm.nodeFactory.getChildNodeTypes( parentType ); childrenOK = allowedChildren === null || $.inArray( childType, allowedChildren ) !== -1; - // Also check if we're trying to insert structure into a node that - // has to contain content + // Also check if we're trying to insert structure into a node that has to contain + // content childrenOK = childrenOK && !( !ve.dm.nodeFactory.isNodeContent( childType ) && ve.dm.nodeFactory.canNodeContainContent( parentType ) @@ -1106,14 +1110,13 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { popped = openingStack.pop(); parentType = popped.type; reopenElements.push( ve.copyObject( popped ) ); - // The opening was on openingStack, so we're closing - // a node that was opened within data. Don't track - // that on closingStack + // The opening was on openingStack, so we're closing a node that was opened + // within data. Don't track that on closingStack } else { - // openingStack is empty, so we're closing a node that - // was already in the document. This means we have to - // reopen it later, so track this on closingStack - closingStack.push( parentType ); + // openingStack is empty, so we're closing a node that was already in the + // document. This means we have to reopen it later, so track this on + // closingStack + closingStack.push( parentNode ); reopenElements.push( parentNode.getClonedElement() ); parentNode = parentNode.getParent(); if ( !parentNode ) { @@ -1127,9 +1130,8 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { } while( !childrenOK ); for ( j = 0; j < closings.length; j++ ) { - // writeElement() would update openingStack/closingStack, but - // we've already done that for closings - //writeElement( closings[j], i ); + // writeElement() would update openingStack/closingStack, but we've already done + // that for closings newData.push( closings[j] ); } for ( j = 0; j < openings.length; j++ ) { @@ -1148,8 +1150,8 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { 'reopenElements': reopenElements } ); } - // If we didn't wrap the text node, then the node we're inserting - // into can have content, so we couldn't have closed anything + // If we didn't wrap the text node, then the node we're inserting into can have + // content, so we couldn't have closed anything } else { fixupStack.push( { 'expectedType': '/' + data[i].type, @@ -1213,7 +1215,7 @@ ve.dm.Document.prototype.fixupInsertion = function( data, offset ) { popped = closingStack[closingStack.length - 1]; // writeElement() will perform the actual pop() that removes // popped from closingStack - writeElement( { 'type': popped }, i ); + writeElement( { 'type': popped.getType() }, i ); } return newData; diff --git a/tests/ve/dm/ve.dm.Transaction.test.js b/tests/ve/dm/ve.dm.Transaction.test.js index bb5e68ccd7..968aa5a328 100644 --- a/tests/ve/dm/ve.dm.Transaction.test.js +++ b/tests/ve/dm/ve.dm.Transaction.test.js @@ -42,6 +42,7 @@ ve.dm.Transaction.runConstructorTests = function( constructor, cases ) { test( 'newFromInsertion', function() { var doc = new ve.dm.Document( ve.dm.example.data ), + doc2 = new ve.dm.Document( [ { 'type': 'paragraph' }, { 'type': '/paragraph' } ] ), cases = { 'paragraph before first element': { 'args': [doc, 0, [{ 'type': 'paragraph' }, '1', { 'type': '/paragraph' }]], @@ -173,6 +174,30 @@ test( 'newFromInsertion', function() { }, { 'type': 'retain', 'length': 27 } ] + }, + 'inserting two paragraphs into a document with just an empty paragraph': { + 'args': [doc2, 1, ['F', 'O', 'O', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'R']], + 'ops': [ + { 'type': 'retain', 'length': 1 }, + { + 'type': 'replace', + 'remove': [], + 'insert': ['F', 'O', 'O', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'R'] + }, + { 'type': 'retain', 'length': 1 } + ] + }, + 'inserting three paragraphs into a document with just an empty paragraph': { + 'args': [doc2, 1, ['F', 'O', 'O', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'R', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'Z']], + 'ops': [ + { 'type': 'retain', 'length': 1 }, + { + 'type': 'replace', + 'remove': [], + 'insert': ['F', 'O', 'O', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'R', { 'type': '/paragraph' }, { 'type': 'paragraph' }, 'B', 'A', 'Z'] + }, + { 'type': 'retain', 'length': 1 } + ] } // TODO test cases for unclosed openings // TODO test cases for (currently failing) unopened closings use case