From 662880605c9b30dae8eb7e545f647ad155432263 Mon Sep 17 00:00:00 2001 From: Catrope Date: Mon, 19 Nov 2012 20:26:54 -0800 Subject: [PATCH] (bug 42119) Handle alienation in wrapping mode properly When alienating in wrapping mode, we need to look at the type of tag to decide whether to create a wrapped alienInline, or to interrupt the paragraph for an alienBlock. This was being done just fine for the general alienation case (unrecognized tag), but not for the special cases (mw:unrecognized, about groups). * Centralize the logic for ending a wrapper in stopWrapping() * Move the wrapping-contingent block/inline detection logic into createAlien() * Simplify the terrible if statement to decide whether a future decision requires us to stop wrapping. Instead, detect the cases in each code path separately and call stopWrapping() as appropriate * Add tests Change-Id: I4054584ae05e7d5daa71edead3e6a6588cf5d3bb --- modules/ve/dm/ve.dm.Converter.js | 77 ++++++++++++++--------------- modules/ve/test/dm/ve.dm.example.js | 46 +++++++++++++++++ 2 files changed, 82 insertions(+), 41 deletions(-) diff --git a/modules/ve/dm/ve.dm.Converter.js b/modules/ve/dm/ve.dm.Converter.js index 65d5f1f277..8ba61b9a0e 100644 --- a/modules/ve/dm/ve.dm.Converter.js +++ b/modules/ve/dm/ve.dm.Converter.js @@ -207,8 +207,11 @@ ve.dm.Converter.prototype.getDomElementFromDataAnnotation = function ( dataAnnot * @returns {Array} Linear model data */ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, dataElement, path, alreadyWrapped ) { - function createAlien( domElement, isInline, isWrapper ) { - var type = isInline ? 'alienInline' : 'alienBlock', html; + function createAlien( domElement, branchIsContent, isWrapper ) { + // If we're in wrapping mode, we don't know if this alien is supposed to be block + // or inline, so detect it based on the HTML tag name. + var isInline = wrapping ? !ve.isBlockElement( domElement ) : branchIsContent, + type = isInline ? 'alienInline' : 'alienBlock', html; if ( isWrapper ) { html = $( domElement ).html(); } else { @@ -248,6 +251,18 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d nextWhitespace = ''; } } + function stopWrapping() { + if ( wrappedWhitespace !== '' ) { + // Remove wrappedWhitespace from data + data.splice( -wrappedWhitespace.length, wrappedWhitespace.length ); + addWhitespace( wrappingParagraph, 3, wrappedWhitespace ); + nextWhitespace = wrappedWhitespace; + } + data.push( { 'type': '/paragraph' } ); + wrappingParagraph = undefined; + wrapping = false; + wrappingIsOurs = false; + } /** * Helper function to group adjacent child elements with the same about attribute together. @@ -334,6 +349,9 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d // Alienate about groups if ( childDomElement.hasAttribute( 'data-ve-aboutgroup' ) ) { alien = createAlien( childDomElement, branchIsContent, true ); + if ( wrapping && alien[0].type === 'alienBlock' ) { + stopWrapping(); + } data = data.concat( alien ); processNextWhitespace( alien[0] ); prevElement = alien[0]; @@ -391,6 +409,9 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d !rdfaType.match( /^mw:Entity/ ) ) { alien = createAlien( childDomElement, branchIsContent ); + if ( wrapping && alien[0].type === 'alienBlock' ) { + stopWrapping(); + } data = data.concat( alien ); processNextWhitespace( alien[0] ); prevElement = alien[0]; @@ -426,32 +447,12 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d // Look up child element type childDataElement = this.getDataElementFromDomElement( childDomElement ); - // End auto-wrapping of bare content from a previously processed node - // but only if childDataElement is a non-content element or if - // we are about to produce a block alien - if ( - wrapping && ( - ( - childDataElement && - !ve.dm.nodeFactory.isNodeContent( childDataElement.type ) - ) || ( - !childDataElement && - ve.isBlockElement( childDomElement ) - ) - ) - ) { - if ( wrappedWhitespace !== '' ) { - // Remove wrappedWhitespace from data - data.splice( -wrappedWhitespace.length, wrappedWhitespace.length ); - addWhitespace( wrappingParagraph, 3, wrappedWhitespace ); - nextWhitespace = wrappedWhitespace; - } - data.push( { 'type': '/paragraph' } ); - wrappingParagraph = undefined; - wrapping = false; - wrappingIsOurs = false; - } if ( childDataElement ) { + // End auto-wrapping of bare content from a previously processed node + // but only if childDataElement is a non-content element + if ( wrapping && !ve.dm.nodeFactory.isNodeContent( childDataElement.type ) ) { + stopWrapping(); + } if ( ve.dm.nodeFactory.canNodeHaveChildren( childDataElement.type ) ) { // Append child element data data = data.concat( @@ -473,12 +474,12 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d break; } // We don't know what this is, fall back to alien. - // If we're in wrapping mode, we don't know if this alien is - // supposed to be block or inline, so detect it based on the HTML - // tag name. - alien = createAlien( childDomElement, wrapping ? - !ve.isBlockElement( childDomElement ) : branchIsContent - ); + alien = createAlien( childDomElement, branchIsContent ); + // End auto-wrapping of bare content from a previously processed node + // but only if we produced an alienBlock + if ( wrapping && alien[0].type === 'alienBlock' ) { + stopWrapping(); + } data = data.concat( alien ); processNextWhitespace( alien[0] ); prevElement = alien[0]; @@ -627,15 +628,9 @@ ve.dm.Converter.prototype.getDataFromDom = function ( domElement, annotations, d } // End auto-wrapping of bare content if ( wrapping && wrappingIsOurs ) { - if ( wrappedWhitespace !== '' ) { - // Remove wrappedWhitespace from data - data.splice( -wrappedWhitespace.length, wrappedWhitespace.length ); - addWhitespace( wrappingParagraph, 3, wrappedWhitespace ); - nextWhitespace = wrappedWhitespace; - } - data.push( { 'type': '/paragraph' } ); + stopWrapping(); // Don't set wrapping = false here because it's checked below - wrappingParagraph = undefined; + wrapping = true; } // If we're closing a node that doesn't have any children, but could contain a paragraph, diff --git a/modules/ve/test/dm/ve.dm.example.js b/modules/ve/test/dm/ve.dm.example.js index 6c6147f068..3bb65b78c8 100644 --- a/modules/ve/test/dm/ve.dm.example.js +++ b/modules/ve/test/dm/ve.dm.example.js @@ -694,6 +694,52 @@ ve.dm.example.domToDataCases = { { 'type': '/paragraph' } ] }, + 'wrapping of bare content with mw:unrecognized inline alien': { + 'html': '1baz2', + 'data': [ + { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } }, + '1', + { + 'type': 'alienInline', + 'attributes': { 'html': 'baz' } + }, + { 'type': '/alienInline' }, + '2', + { 'type': '/paragraph' } + ] + }, + 'wrapping of bare content with mw:unrecognized block alien': { + 'html': '1
baz
2', + 'data': [ + { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } }, + '1', + { 'type': '/paragraph' }, + { + 'type': 'alienBlock', + 'attributes': { 'html': '
baz
' } + }, + { 'type': '/alienBlock' }, + { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } }, + '2', + { 'type': '/paragraph' } + ] + }, + 'wrapping of bare content with about group': { + 'html': '1foobar2', + 'data': [ + { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } }, + '1', + { 'type': '/paragraph' }, + { + 'type': 'alienBlock', + 'attributes': { 'html': 'foobar' } + }, + { 'type': '/alienBlock' }, + { 'type': 'paragraph', 'internal': { 'generated': 'wrapper' } }, + '2', + { 'type': '/paragraph' } + ] + }, 'wrapping of bare content between structural nodes': { 'html': '
abc
', 'data': [