Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/189507
Change subject: ve.utils: Apply whitelist in setDomAttributes() attribute
removal
......................................................................
ve.utils: Apply whitelist in setDomAttributes() attribute removal
Previously, the whitelist only affected setting of attributes.
Input that wants to remove attributes was always honoured, even
if the key was not whitelisted.
* Simplify test assertions.
* Test whitelist for attribute removal.
* Test case-insensitive nature of whitelist.
Change-Id: Id4ab9fb3d0609182f83724327b8b5b9258b1b86d
---
M src/ve.utils.js
M tests/ve.test.js
2 files changed, 52 insertions(+), 18 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/07/189507/1
diff --git a/src/ve.utils.js b/src/ve.utils.js
index 05fa63a..74b3d78 100644
--- a/src/ve.utils.js
+++ b/src/ve.utils.js
@@ -554,9 +554,11 @@
/**
* Set the attributes of a DOM element as an object with key/value pairs.
*
+ * Use the `null` or `undefined` value to ensure an attribute's absence.
+ *
* @param {HTMLElement} element DOM element to apply attributes to
* @param {Object} attributes Attributes to apply
- * @param {string[]} [whitelist] List of attributes to exclusively allow (all
lower case names)
+ * @param {string[]} [whitelist] List of attributes to exclusively allow (all
lowercase names)
*/
ve.setDomAttributes = function ( element, attributes, whitelist ) {
var key;
@@ -565,12 +567,12 @@
return;
}
for ( key in attributes ) {
+ if ( whitelist && whitelist.indexOf( key.toLowerCase() ) === -1
) {
+ continue;
+ }
if ( attributes[key] === undefined || attributes[key] === null
) {
element.removeAttribute( key );
} else {
- if ( whitelist && whitelist.indexOf( key.toLowerCase()
) === -1 ) {
- continue;
- }
element.setAttribute( key, attributes[key] );
}
}
diff --git a/tests/ve.test.js b/tests/ve.test.js
index d598ae5..f411252 100644
--- a/tests/ve.test.js
+++ b/tests/ve.test.js
@@ -88,28 +88,60 @@
QUnit.test( 'getDomAttributes', 1, function ( assert ) {
assert.deepEqual(
- ve.getDomAttributes( $( '<div foo="bar" baz quux=3></div>'
).get( 0 ) ),
- { foo: 'bar', baz: '', quux: '3' },
+ ve.getDomAttributes( $.parseHTML( '<div string="foo" empty
number="0"></div>' )[ 0 ] ),
+ { string: 'foo', empty: '', number: '0' },
'getDomAttributes() returns object with correct attributes'
);
} );
-QUnit.test( 'setDomAttributes', 3, function ( assert ) {
- var element = document.createElement( 'div' );
- ve.setDomAttributes( element, { foo: 'bar', baz: '', quux: 3 } );
+QUnit.test( 'setDomAttributes', 7, function ( assert ) {
+ var target,
+ sample = $.parseHTML( '<div foo="one" bar="two"
baz="three"></div>' )[ 0 ];
+
+ target = {};
+ ve.setDomAttributes( target, { add: 'foo' } );
+ assert.deepEqual( target, {}, 'ignore incompatible target object' );
+
+ target = document.createElement( 'div' );
+ ve.setDomAttributes( target, { string: 'foo', empty: '', number: 0 } );
assert.deepEqual(
- ve.getDomAttributes( element ),
- { foo: 'bar', baz: '', quux: '3' },
- 'setDomAttributes() sets attributes correctly'
+ ve.getDomAttributes( target ),
+ { string: 'foo', empty: '', number: '0' },
+ 'add attributes'
);
- ve.setDomAttributes( element, { foo: null, bar: 1, baz: undefined,
quux: 5, whee: 'yay' } );
+
+ target = sample.cloneNode();
+ ve.setDomAttributes( target, { foo: null, bar: 'update', baz:
undefined, add: 'yay' } );
assert.deepEqual(
- ve.getDomAttributes( element ),
- { bar: '1', quux: '5', whee: 'yay' },
- 'setDomAttributes() overwrites attributes, removes attributes,
and sets new attributes'
+ ve.getDomAttributes( target ),
+ { bar: 'update', add: 'yay' },
+ 'add, update, and remove attributes'
);
- ve.setDomAttributes( element, { onclick: 'alert(1);' }, ['foo', 'bar',
'baz', 'quux', 'whee'] );
- assert.ok( !element.hasAttribute( 'onclick' ), 'event attributes are
blocked when sanitizing' );
+
+ target = sample.cloneNode();
+ ve.setDomAttributes( target, { onclick: 'alert(1);', foo: 'update',
add: 'whee' }, ['foo', 'add'] );
+ assert.ok( !target.hasAttribute( 'onclick' ), 'whitelist affects
creating attributes' );
+ assert.deepEqual(
+ ve.getDomAttributes( target ),
+ { foo: 'update', bar: 'two', baz: 'three', add: 'whee' },
+ 'whitelist does not affect pre-existing attributes'
+ );
+
+ target = document.createElement( 'div' );
+ ve.setDomAttributes( target, { Foo: 'add', Bar: 'add' }, ['bar'] );
+ assert.deepEqual(
+ ve.getDomAttributes( target ),
+ { bar: 'add' },
+ 'whitelist is case-insensitive'
+ );
+
+ target = sample.cloneNode();
+ ve.setDomAttributes( target, { foo: 'update', bar: null }, ['bar',
'baz'] );
+ assert.propEqual(
+ ve.getDomAttributes( target ),
+ { foo: 'one', baz: 'three' },
+ 'whitelist affects removal/updating of attributes'
+ );
} );
QUnit.test( 'getHtmlAttributes', 7, function ( assert ) {
--
To view, visit https://gerrit.wikimedia.org/r/189507
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id4ab9fb3d0609182f83724327b8b5b9258b1b86d
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
