DLynch has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/342847 )
Change subject: ElementLinearData: when removing original dom elements, remap
hashes
......................................................................
ElementLinearData: when removing original dom elements, remap hashes
Failure to do this breaks later assumptions that VE makes when action on
annotations, specifically that annotation objects fetched from the store will
always hash-by-value back into it. This manifested as not being able to toggle
annotations converted from pasted content.
This has been broken since 14b5fbc3 / T142943, which switched
originalDomElements to using the IV store.
Bug: T154123
Change-Id: I6a22de2d6b39fa45d57d1dbcedc5cc8aac6b893d
---
M src/dm/lineardata/ve.dm.ElementLinearData.js
M src/dm/ve.dm.IndexValueStore.js
M tests/dm/lineardata/ve.dm.ElementLinearData.test.js
3 files changed, 70 insertions(+), 3 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/47/342847/1
diff --git a/src/dm/lineardata/ve.dm.ElementLinearData.js
b/src/dm/lineardata/ve.dm.ElementLinearData.js
index f7bc569..cc96094 100644
--- a/src/dm/lineardata/ve.dm.ElementLinearData.js
+++ b/src/dm/lineardata/ve.dm.ElementLinearData.js
@@ -1064,6 +1064,31 @@
};
/**
+ * Remap an annotation index when it changes
+ *
+ * @param {string} oldIndex Old index to replace
+ * @param {string} newIndex New index to replace it with
+ */
+ve.dm.ElementLinearData.prototype.remapAnnotationIndex = function ( oldIndex,
newIndex ) {
+ var i, ilen, attrIndex;
+ for ( i = 0, ilen = this.data.length; i < ilen; i++ ) {
+ if ( this.data[ i ] === undefined || typeof this.data[ i ] ===
'string' ) {
+ // common case, cheap, avoid the isArray check
+ continue;
+ } else if ( Array.isArray( this.data[ i ] ) ) {
+ attrIndex = this.data[ i ][ 1 ].indexOf( oldIndex );
+ if ( attrIndex !== -1 ) {
+ if ( this.data[ i ][ 1 ].indexOf( newIndex )
=== -1 ) {
+ this.data[ i ][ 1 ].splice( attrIndex,
1, newIndex );
+ } else {
+ this.data[ i ][ 1 ].splice( attrIndex,
1, newIndex );
+ }
+ }
+ }
+ }
+};
+
+/**
* Sanitize data according to a set of rules.
*
* @param {Object} rules Sanitization rules
@@ -1077,7 +1102,7 @@
* @param {boolean} [rules.keepEmptyContentBranches] Preserve empty content
branch nodes
*/
ve.dm.ElementLinearData.prototype.sanitize = function ( rules ) {
- var i, len, annotations, emptySet, setToRemove, type,
+ var i, len, annotations, emptySet, setToRemove, type, oldHash, newHash,
canContainContent, contentElement, isOpen, nodeClass, ann,
elementStack = [],
store = this.getStore(),
@@ -1091,7 +1116,23 @@
for ( i = 0, len = allAnnotations.getLength(); i < len;
i++ ) {
ann = allAnnotations.get( i );
if ( ann.element.originalDomElementsIndex !==
undefined ) {
+ // This changes the hash of the value,
so we have to
+ // update that. If we don't do this,
other assumptions
+ // that values fetched from the store
are actually in the
+ // store will fail.
+ oldHash = store.indexOfValue( ann );
delete allAnnotations.get( i
).element.originalDomElementsIndex;
+ newHash = store.replaceHash( oldHash,
ann );
+ this.remapAnnotationIndex( oldHash,
newHash );
+ if (
allAnnotations.storeIndexes.indexOf( newHash ) !== -1 ) {
+ // New annotation-value was
already in the set, which
+ // just reduces the
effective-length of the set.
+
allAnnotations.storeIndexes.splice( i, 1 );
+ i--;
+ len--;
+ } else {
+
allAnnotations.storeIndexes.splice( i, 1, newHash );
+ }
}
}
}
diff --git a/src/dm/ve.dm.IndexValueStore.js b/src/dm/ve.dm.IndexValueStore.js
index d3af37f..cd1168f 100644
--- a/src/dm/ve.dm.IndexValueStore.js
+++ b/src/dm/ve.dm.IndexValueStore.js
@@ -108,6 +108,32 @@
return hash;
};
+ /**
+ * Replace a value's stored hash, e.g. if the value has changed and you want
to discard the old one.
+ *
+ * @param {string} oldHash The value's previously stored hash
+ * @param {Object|string|Array} value New value
+ * @throws {Error} Old hash not found
+ * @return {string} New hash
+ */
+ve.dm.IndexValueStore.prototype.replaceHash = function ( oldHash, value ) {
+ var newHash = this.indexOfValue( value ),
+ index = this.hashStore[ oldHash ];
+
+ if ( index === undefined ) {
+ throw new Error( 'Old hash not found: ' + oldHash );
+ }
+
+ delete this.hashStore[ oldHash ];
+
+ if ( this.hashStore[ newHash ] === undefined ) {
+ this.hashStore[ newHash ] = value;
+ this.hashes.splice( this.hashes.indexOf( oldHash ), 1, newHash
);
+ }
+
+ return newHash;
+};
+
/**
* Get the hash of a value without inserting it in the store
*
diff --git a/tests/dm/lineardata/ve.dm.ElementLinearData.test.js
b/tests/dm/lineardata/ve.dm.ElementLinearData.test.js
index d8e789e..e8aab53 100644
--- a/tests/dm/lineardata/ve.dm.ElementLinearData.test.js
+++ b/tests/dm/lineardata/ve.dm.ElementLinearData.test.js
@@ -1545,12 +1545,12 @@
html: '<p style="text-shadow: 0 0 1px
#000;">F<b style="color:blue;">o</b>o</p>',
data: [
{ type: 'paragraph' },
- 'F', [ 'o', [ 'h5dd47c3facf7b6a8' ] ],
'o',
+ 'F', [ 'o', [ 'h49981eab0f8056ff' ] ],
'o',
{ type: '/paragraph' },
{ type: 'internalList' },
{ type: '/internalList' }
],
- store: { h5dd47c3facf7b6a8: bold },
+ store: { h49981eab0f8056ff: bold },
rules: { removeOriginalDomElements: true },
msg: 'Original DOM elements removed'
},
--
To view, visit https://gerrit.wikimedia.org/r/342847
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a22de2d6b39fa45d57d1dbcedc5cc8aac6b893d
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: DLynch <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
