jenkins-bot has submitted this change and it was merged.
Change subject: AlienNode stores original DOM elements instead of HTML
......................................................................
AlienNode stores original DOM elements instead of HTML
As jQuery hash problems in some cases converting HTML, it is
easier just to store the original DOM elements.
The bulk of this commit is fixing the tests as although we have an
assertion for comparing DOM elements, we don't have one for comparing
objects or arrays which may contain DOM elements.
ve.js
* copyObject & copyArray fixed to run cloneNode(true) on any item
of type Node.
* Added callback function so an object/array can be copied with
modifications.
ve.qunit.js
* Added deepEqualWithDomElements: Using the new copyObect/Array
callback, we can copy the incoming object and convert any nodes
to node summaries, then just run the normal deep equal comparison.
ve.dm.AlienNodes.js
* Instead of storing HMTL we store cloned DOM elements which we can
send straight back to the converter without any processing.
ve.dm.example.js
* Updated tests to expect DOM elements instead of HTML.
ve.dm.Converter.test.js
* Updated tests to use deepEqualWithDomElements
Bug: 47737
Change-Id: I3df8f49b170c31da9610129d53cf8cb65dd5d5f8
---
M modules/ve/dm/nodes/ve.dm.AlienNode.js
M modules/ve/test/dm/ve.dm.Converter.test.js
M modules/ve/test/dm/ve.dm.example.js
M modules/ve/test/ve.qunit.js
M modules/ve/ve.js
5 files changed, 77 insertions(+), 61 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/dm/nodes/ve.dm.AlienNode.js
b/modules/ve/dm/nodes/ve.dm.AlienNode.js
index a17b29a..2cfe9c5 100644
--- a/modules/ve/dm/nodes/ve.dm.AlienNode.js
+++ b/modules/ve/dm/nodes/ve.dm.AlienNode.js
@@ -35,34 +35,18 @@
ve.dm.AlienNode.static.toDataElement = function ( domElements, converter ) {
var isInline = this.isHybridInline( domElements, converter ),
- type = isInline ? 'alienInline' : 'alienBlock',
- html = $( '<div>', domElements[0].ownerDocument ).append( $(
domElements ).clone() ).html();
+ type = isInline ? 'alienInline' : 'alienBlock';
return {
'type': type,
'attributes': {
- 'html': html
+ 'domElements': ve.copyArray( domElements )
}
};
};
-ve.dm.AlienNode.static.toDomElements = function ( dataElement, doc ) {
- var wrapper = doc.createElement( 'div' );
-
- // Filthy hack: Parsoid is currently sending us unescaped angle brackets
- // inside data-parsoid. For some reason FF picks this up as html and
tries
- // to sanitise it, converting <ref/> to <ref></span> (!?).
- // As a *very temporary* fix we can regex replace them here.
- $( wrapper ).html(
- dataElement.attributes.html.replace(
- /data-parsoid="([^"]+)"/g,
- function( r0, r1 ) {
- return 'data-parsoid="' + r1.replace( /</g,
'<' ).replace( />/g, '>' ) + '"';
- }
- )
- );
- // Convert wrapper.children to an array
- return Array.prototype.slice.call( wrapper.childNodes, 0 );
+ve.dm.AlienNode.static.toDomElements = function ( dataElement ) {
+ return dataElement.attributes.domElements;
};
/* Concrete subclasses */
diff --git a/modules/ve/test/dm/ve.dm.Converter.test.js
b/modules/ve/test/dm/ve.dm.Converter.test.js
index ca0c100..6dc1d88 100644
--- a/modules/ve/test/dm/ve.dm.Converter.test.js
+++ b/modules/ve/test/dm/ve.dm.Converter.test.js
@@ -63,7 +63,7 @@
store = new ve.dm.IndexValueStore();
internalList = new ve.dm.InternalList();
ve.dm.example.preprocessAnnotations( cases[msg].data,
store );
- assert.deepEqual(
+ assert.deepEqualWithDomElements(
ve.dm.converter.getDataFromDom(
ve.createDocumentFromHTML( cases[msg].html ), store, internalList ).getData(),
cases[msg].data,
msg
@@ -100,9 +100,8 @@
store.index( cases[msg].storeItems[i].value,
cases[msg].storeItems[i].hash );
}
}
- // functions won't be copied by ve.copyObject
- if( ve.dm.example.domToDataCases[msg].modify ) {
- ve.dm.example.domToDataCases[msg].modify(
cases[msg].data );
+ if( cases[msg].modify ) {
+ cases[msg].modify( cases[msg].data );
}
doc = new ve.dm.Document( ve.dm.example.preprocessAnnotations(
cases[msg].data, store ) );
originalData = ve.copyArray( doc.getFullData() );
@@ -111,6 +110,6 @@
ve.createDocumentFromHTML( cases[msg].normalizedHtml ||
cases[msg].html ),
msg
);
- assert.deepEqual( doc.getFullData(), originalData, msg + '
(data hasn\'t changed)' );
+ assert.deepEqualWithDomElements( doc.getFullData(),
originalData, msg + ' (data hasn\'t changed)' );
}
} );
diff --git a/modules/ve/test/dm/ve.dm.example.js
b/modules/ve/test/dm/ve.dm.example.js
index 01086f5..b0579e0 100644
--- a/modules/ve/test/dm/ve.dm.example.js
+++ b/modules/ve/test/dm/ve.dm.example.js
@@ -1121,7 +1121,7 @@
'a',
{
'type': 'alienInline',
- 'attributes': { 'html': '<tt
class="foo">b</tt>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<tt class="foo">b</tt>' ) ) }
},
{ 'type': '/alienInline' },
'c',
@@ -1136,7 +1136,7 @@
'b',
'c',
{ 'type': '/paragraph' },
- { 'type': 'alienBlock', 'attributes': { 'html':
'<figure>abc</figure>' } },
+ { 'type': 'alienBlock', 'attributes': { 'domElements':
$.makeArray( $( '<figure>abc</figure>' ) ) } },
{ 'type': '/alienBlock' },
{ 'type': 'paragraph' },
'd',
@@ -1153,7 +1153,7 @@
'a',
{
'type': 'alienInline',
- 'attributes': { 'html': '<tt
class="foo">b</tt>' },
+ 'attributes': { 'domElements': $.makeArray( $(
'<tt class="foo">b</tt>' ) ) },
'annotations': [ ve.dm.example.bold ]
},
{ 'type': '/alienInline' },
@@ -1217,7 +1217,7 @@
'1',
{
'type': 'alienInline',
- 'attributes': { 'html': '<tt
class="bar">baz</tt>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<tt class="bar">baz</tt>' ) ) }
},
{ 'type': '/alienInline' },
'2',
@@ -1232,7 +1232,7 @@
{ 'type': '/paragraph' },
{
'type': 'alienBlock',
- 'attributes': { 'html': '<figure
class="bar">baz</figure>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<figure class="bar">baz</figure>' ) ) }
},
{ 'type': '/alienBlock' },
{ 'type': 'paragraph', 'internal': { 'generated':
'wrapper' } },
@@ -1247,7 +1247,7 @@
'1',
{
'type': 'alienInline',
- 'attributes': { 'html': '<span
typeof="mw:Placeholder">baz</span>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<span typeof="mw:Placeholder">baz</span>' ) ) }
},
{ 'type': '/alienInline' },
'2',
@@ -1262,7 +1262,7 @@
{ 'type': '/paragraph' },
{
'type': 'alienBlock',
- 'attributes': { 'html': '<div
typeof="mw:Placeholder">baz</div>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<div typeof="mw:Placeholder">baz</div>' ) ) }
},
{ 'type': '/alienBlock' },
{ 'type': 'paragraph', 'internal': { 'generated':
'wrapper' } },
@@ -1276,7 +1276,7 @@
{ 'type': 'paragraph', 'internal': { 'generated':
'wrapper' } },
{
'type': 'alienInline',
- 'attributes': { 'html': '<span
typeof="mw:Placeholder">Foo</span>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<span typeof="mw:Placeholder">Foo</span>' ) ) }
},
{ 'type': '/alienInline' },
'B',
@@ -1294,7 +1294,7 @@
'o',
{
'type': 'alienInline',
- 'attributes': { 'html': '<span
typeof="mw:Placeholder">Bar</span>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<span typeof="mw:Placeholder">Bar</span>' ) ) }
},
{ 'type': '/alienInline' },
{ 'type': '/paragraph' }
@@ -1307,7 +1307,7 @@
'1',
{
'type': 'alienInline',
- 'attributes': { 'html': '<tt
about="#mwt1">foo</tt><tt about="#mwt1">bar</tt>' }
+ 'attributes': { 'domElements': $.makeArray( $(
'<tt about="#mwt1">foo</tt><tt about="#mwt1">bar</tt>' ) ) }
},
{ 'type': '/alienInline' },
'2',
@@ -1860,7 +1860,7 @@
{
'type': 'alienBlock',
'attributes': {
- 'html': '<p typeof="mw:Placeholder">
<br> </p>'
+ 'domElements': $.makeArray( $( '<p
typeof="mw:Placeholder"> <br> </p>' ) )
},
'internal': {
'whitespace': [ ' ', undefined,
undefined, ' ' ]
@@ -1873,7 +1873,7 @@
'o',
'\t',
'\t',
- { 'type': 'alienInline', 'attributes': { 'html':
'<tt>\t\t\tBar\t\t\t\t</tt>' } },
+ { 'type': 'alienInline', 'attributes': { 'domElements':
$.makeArray( $( '<tt>\t\t\tBar\t\t\t\t</tt>' ) ) } },
{ 'type': '/alienInline' },
'\n',
'B',
@@ -1884,7 +1884,7 @@
{
'type': 'alienInline',
'attributes': {
- 'html': '<span
typeof="mw:Placeholder">\n\n\nQuux\n\n\n\n</span>'
+ 'domElements': $.makeArray( $( '<span
typeof="mw:Placeholder">\n\n\nQuux\n\n\n\n</span>' ) )
}
},
{ 'type': '/alienInline' },
@@ -1898,7 +1898,7 @@
{
'type': 'alienBlock',
'attributes': {
- 'html': '<figure>\n\tYay \t </figure>'
+ 'domElements': $.makeArray( $(
'<figure>\n\tYay \t </figure>' ) )
},
'internal': {
'whitespace': [ '\t\n', undefined,
undefined, ' \n ' ]
@@ -2259,16 +2259,16 @@
{
'type': 'alienBlock',
'attributes': {
- 'html': '<div typeof="mw:Placeholder"
about="#mwt1">Foo</div>' +
- '<figure
typeof="mw:Placeholder" about="#mwt1">Bar</figure>'
+ 'domElements': $.makeArray( $( '<div
typeof="mw:Placeholder" about="#mwt1">Foo</div>' +
+ '<figure
typeof="mw:Placeholder" about="#mwt1">Bar</figure>' ) )
}
},
{ 'type': '/alienBlock' },
{
'type': 'alienBlock',
'attributes': {
- 'html': '<figure
typeof="mw:Placeholder" about="#mwt2">Baz</figure>' +
- '<span typeof="mw:Placeholder"
about="#mwt2">Quux</span>'
+ 'domElements': $.makeArray( $( '<figure
typeof="mw:Placeholder" about="#mwt2">Baz</figure>' +
+ '<span typeof="mw:Placeholder"
about="#mwt2">Quux</span>' ) )
}
},
{ 'type': '/alienBlock' },
@@ -2281,8 +2281,8 @@
{
'type': 'alienBlock',
'attributes': {
- 'html': '<span typeof="mw:Placeholder"
about="#mwt2">Yay</span>' +
- '<div typeof="mw:Placeholder"
about="#mwt2">Blah</div>'
+ 'domElements': $.makeArray( $( '<span
typeof="mw:Placeholder" about="#mwt2">Yay</span>' +
+ '<div typeof="mw:Placeholder"
about="#mwt2">Blah</div>' ) )
}
},
{ 'type': '/alienBlock' },
@@ -2290,7 +2290,7 @@
{
'type': 'alienInline',
'attributes': {
- 'html': '<span typeof="mw:Placeholder"
about="#mwt3">Meh</span>'
+ 'domElements': $.makeArray( $( '<span
typeof="mw:Placeholder" about="#mwt3">Meh</span>' ) )
}
},
{ 'type': '/alienInline' },
@@ -2304,8 +2304,8 @@
{
'type': 'alienBlock',
'attributes': {
- 'html': '<div typeof="mw:Placeholder"
about="#mwt1">\tFoo\t\t</div>\t\t\t' +
- '<div typeof="mw:Placeholder"
about="#mwt1"> Bar </div>'
+ 'domElements': $.makeArray( $( '<div
typeof="mw:Placeholder" about="#mwt1">\tFoo\t\t</div>\t\t\t' +
+ '<div typeof="mw:Placeholder"
about="#mwt1"> Bar </div>' ) )
},
'internal': {
'whitespace': [ ' ', undefined,
undefined, ' ' ]
@@ -2377,7 +2377,7 @@
{
'type': 'alienInline',
'attributes': {
- 'html': '<p>Bar</p>'
+ 'domElements': $.makeArray( $(
'<p>Bar</p>' ) )
},
'annotations': [ ve.dm.example.span ]
},
@@ -2395,7 +2395,7 @@
{
'type': 'alienInline',
'attributes': {
- 'html': '<p>Bar</p>'
+ 'domElements': $.makeArray( $(
'<p>Bar</p>' ) )
},
'annotations': [ ve.dm.example.span ]
},
@@ -2416,7 +2416,7 @@
{
'type': 'alienInline',
'attributes': {
- 'html': '<p>Bar</p>'
+ 'domElements': $.makeArray( $(
'<p>Bar</p>' ) )
},
'annotations': [ ve.dm.example.span ]
},
diff --git a/modules/ve/test/ve.qunit.js b/modules/ve/test/ve.qunit.js
index 88714f5..fc2dfc4 100644
--- a/modules/ve/test/ve.qunit.js
+++ b/modules/ve/test/ve.qunit.js
@@ -120,6 +120,20 @@
return summary;
}
+/**
+ * Callback for ve.copyArray/Object to convert nodes to a comparable summary
+ *
+ * @method
+ * @private
+ * @param {Object} value Value in the object/array
+ * @returns {Object} DOM element summary if value is a node, otherwise just
the value
+ */
+function convertDomElements( value ) {
+ if ( value instanceof Node ) {
+ return getDomElementSummary( value );
+ }
+ return value;
+}
/**
* Assertion helpers for VisualEditor test suite.
@@ -178,4 +192,17 @@
);
};
+/**
+ * Assert that two objects which may contain dom elements are equal.
+ * @method
+ * @static
+ */
+QUnit.assert.deepEqualWithDomElements = function ( actual, expected, message )
{
+ // Recursively copy objects or arrays, converting any dom elements
found to comparable summaries
+ actual = ve.isArray( actual ) ? ve.copyArray( actual,
convertDomElements ) : ve.copyObject( actual, convertDomElements );
+ expected = ve.isArray( expected ) ? ve.copyArray( expected,
convertDomElements ) : ve.copyObject( expected, convertDomElements );
+
+ QUnit.push( QUnit.equiv(actual, expected), actual, expected, message );
+};
+
}( QUnit ) );
diff --git a/modules/ve/ve.js b/modules/ve/ve.js
index e90120e..308e4cd 100644
--- a/modules/ve/ve.js
+++ b/modules/ve/ve.js
@@ -583,22 +583,25 @@
*
* @method
* @param {Array} source Array to copy
+ * @param {Function} [callback] Applied to leaf values before they
added to the clone
* @returns {Array} Copy of source array
*/
- ve.copyArray = function ( source ) {
+ ve.copyArray = function ( source, callback ) {
var i, sourceValue, sourceType,
destination = [];
for ( i = 0; i < source.length; i++ ) {
sourceValue = source[i];
sourceType = typeof sourceValue;
if ( ve.isPlainObject( sourceValue ) ) {
- destination.push( ve.copyObject( sourceValue )
);
+ destination.push( ve.copyObject( sourceValue,
callback ) );
} else if ( ve.isArray( sourceValue ) ) {
- destination.push( ve.copyArray( sourceValue ) );
+ destination.push( ve.copyArray( sourceValue,
callback ) );
} else if ( sourceValue && typeof sourceValue.clone ===
'function' ) {
- destination.push( sourceValue.clone() );
+ destination.push( callback ? callback(
sourceValue.clone() ) : sourceValue.clone() );
+ } else if ( sourceValue instanceof Node ) {
+ destination.push( callback ? callback(
sourceValue.cloneNode( true ) ) : sourceValue.cloneNode( true ) );
} else {
- destination.push( sourceValue );
+ destination.push( callback ? callback(
sourceValue ) : sourceValue );
}
}
return destination;
@@ -609,9 +612,10 @@
*
* @method
* @param {Object} source Object to copy
+ * @param {Function} [callback] Applied to leaf values before they
added to the clone
* @returns {Object} Copy of source object
*/
- ve.copyObject = function ( source ) {
+ ve.copyObject = function ( source, callback ) {
var key, sourceValue, sourceType,
destination = {};
if ( typeof source.clone === 'function' ) {
@@ -621,13 +625,15 @@
sourceValue = source[key];
sourceType = typeof sourceValue;
if ( ve.isPlainObject( sourceValue ) ) {
- destination[key] = ve.copyObject( sourceValue );
+ destination[key] = ve.copyObject( sourceValue,
callback );
} else if ( ve.isArray( sourceValue ) ) {
- destination[key] = ve.copyArray( sourceValue );
+ destination[key] = ve.copyArray( sourceValue,
callback );
} else if ( sourceValue && typeof sourceValue.clone ===
'function' ) {
- destination[key] = sourceValue.clone();
+ destination[key] = callback ? callback(
sourceValue.clone() ) : sourceValue.clone();
+ } else if ( sourceValue instanceof Node ) {
+ destination[key] = callback ? callback(
sourceValue.cloneNode( true ) ) : sourceValue.cloneNode( true );
} else {
- destination[key] = sourceValue;
+ destination[key] = callback ? callback(
sourceValue ) : sourceValue;
}
}
return destination;
--
To view, visit https://gerrit.wikimedia.org/r/61180
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3df8f49b170c31da9610129d53cf8cb65dd5d5f8
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <esand...@wikimedia.org>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
