Skip to content

Commit

Permalink
Support arbitrary attributes on elements with dashes in the tag name.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jimfb
jimfb committed Feb 6, 2015
1 parent c7e4f55 commit c841cf6
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 6 deletions.
6 changes: 3 additions & 3 deletions src/browser/ui/ReactDOMComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ var omittedCloseTags = {
// We accept any tag to be rendered but since this gets injected into abitrary
// HTML, we want to make sure that it's a safe tag.
// http://www.w3.org/TR/REC-xml/#NT-Name

var VALID_TAG_REGEX = /^[a-zA-Z][a-zA-Z:_\.\-\d]*$/; // Simplified subset
var validatedTagCache = {};

var hasOwnProperty = {}.hasOwnProperty;

function validateDangerousTag(tag) {
Expand Down Expand Up @@ -195,7 +195,7 @@ ReactDOMComponent.Mixin = {
assertValidProps(this._currentElement.props);
var closeTag = omittedCloseTags[this._tag] ? '' : '</' + this._tag + '>';
return (
this._createOpenTagMarkupAndPutListeners(transaction) +
this._createOpenTagMarkupAndPutListeners(transaction) +
this._createContentMarkup(transaction, context) +
closeTag
);
Expand Down Expand Up @@ -235,7 +235,7 @@ ReactDOMComponent.Mixin = {
propValue = CSSPropertyOperations.createMarkupForStyles(propValue);
}
var markup =
DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
DOMPropertyOperations.createMarkupForProperty(this._tag, propKey, propValue);
if (markup) {
ret += ' ' + markup;
}
Expand Down
37 changes: 34 additions & 3 deletions src/browser/ui/dom/DOMPropertyOperations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,17 @@
var DOMProperty = require('DOMProperty');

var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var invariant = require('invariant');
var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
var warning = require('warning');

// We accept any attribute name to be rendered but since this gets injected into abitrary
// HTML, we want to make sure that it's a safe attribute.
// Validate attribute names. We (temporarily) disallow colon until we decide the
// desired namespace semantics. For attribute name rules: http://www.w3.org/TR/xml/#NT-Name
var VALID_ATTRIBUTENAME_REGEX = /^[a-zA-Z_][a-zA-Z_\.\-\d]*$/; // Simplified subset
var validatedAttributeNameCache = {};

function shouldIgnoreValue(name, value) {
return value == null ||
(DOMProperty.hasBooleanValue[name] && !value) ||
Expand All @@ -26,6 +34,16 @@ function shouldIgnoreValue(name, value) {
(DOMProperty.hasOverloadedBooleanValue[name] && value === false);
}

function validateDangerousAttributeName(attributeName) {
if (!hasOwnProperty.call(validatedAttributeNameCache, attributeName)) {
invariant(VALID_ATTRIBUTENAME_REGEX.test(attributeName),
'Invalid attribute name: `%s`',
attributeName);
validatedAttributeNameCache[attributeName] = true;
}
return attributeName;
}

if (__DEV__) {
var reactProps = {
children: true,
Expand Down Expand Up @@ -84,11 +102,12 @@ var DOMPropertyOperations = {
/**
* Creates markup for a property.
*
* @param {string} tagName
* @param {string} name
* @param {*} value
* @return {?string} Markup string, or null if the property was invalid.
*/
createMarkupForProperty: function(name, value) {
createMarkupForProperty: function(tagName, name, value) {
if (DOMProperty.isStandardName.hasOwnProperty(name) &&
DOMProperty.isStandardName[name]) {
if (shouldIgnoreValue(name, value)) {
Expand All @@ -104,7 +123,12 @@ var DOMPropertyOperations = {
if (value == null) {
return '';
}
return name + '=' + quoteAttributeValueForBrowser(value);
return validateDangerousAttributeName(name) + '=' + quoteAttributeValueForBrowser(value);
} else if (tagName != null && tagName.indexOf('-') >= 0) {
if (value == null) {
return '';
}
return validateDangerousAttributeName(name) + '=' + quoteAttributeValueForBrowser(value);
} else if (__DEV__) {
warnUnknownProperty(name);
}
Expand All @@ -119,7 +143,14 @@ var DOMPropertyOperations = {
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
if (DOMProperty.isStandardName.hasOwnProperty(name) &&
if (node.tagName.indexOf('-') >= 0) {
if (value == null) {
node.removeAttribute(name);
} else {
node.setAttribute(name, '' + value);
}
}
else if (DOMProperty.isStandardName.hasOwnProperty(name) &&
DOMProperty.isStandardName[name]) {
var mutationMethod = DOMProperty.getMutationMethod[name];
if (mutationMethod) {
Expand Down
83 changes: 83 additions & 0 deletions src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,27 @@ describe('DOMPropertyOperations', function() {

it('should create markup for simple properties', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'name',
'simple'
)).toBe('name="simple"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'name',
false
)).toBe('name="false"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'name',
null
)).toBe('');
});

it('should work with the id attribute', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'id',
'simple'
)).toBe('id="simple"');
Expand All @@ -57,6 +61,7 @@ describe('DOMPropertyOperations', function() {
it('should warn about incorrect casing', function() {
spyOn(console, 'warn');
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'tabindex',
'1'
)).toBe(null);
Expand All @@ -67,6 +72,7 @@ describe('DOMPropertyOperations', function() {
it('should warn about class', function() {
spyOn(console, 'warn');
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'class',
'muffins'
)).toBe(null);
Expand All @@ -76,102 +82,144 @@ describe('DOMPropertyOperations', function() {

it('should create markup for boolean properties', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'checked',
'simple'
)).toBe('checked');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'checked',
true
)).toBe('checked');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'checked',
false
)).toBe('');
});

it('should create markup for booleanish properties', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
'simple'
)).toBe('download="simple"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
true
)).toBe('download');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
'true'
)).toBe('download="true"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
false
)).toBe('');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
'false'
)).toBe('download="false"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
undefined
)).toBe('');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
null
)).toBe('');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'download',
0
)).toBe('download="0"');
});

it('should create markup for custom attributes', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'aria-label',
'simple'
)).toBe('aria-label="simple"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'aria-label',
false
)).toBe('aria-label="false"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'aria-label',
null
)).toBe('');
});

it('should create markup for numeric properties', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'start',
5
)).toBe('start="5"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'start',
0
)).toBe('start="0"');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'size',
0
)).toBe('');

expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'size',
1
)).toBe('size="1"');
});

it('should allow custom properties on web components', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'x-my-webcomponent',
'awesomeness',
5
)).toBe('awesomeness="5"');

expect(DOMPropertyOperations.createMarkupForProperty(
'x-my-webcomponent',
'dev',
'jim'
)).toBe('dev="jim"');
});

it('should reject haxors', function() {
expect(function(){
DOMPropertyOperations.createMarkupForProperty(
'x-my-webcomponent',
'blah" onclick="beevil" noise="hi',
5
);
}).toThrow('Invariant Violation: Invalid attribute name: `blah" onclick="beevil" noise="hi`');
});

});

describe('setValueForProperty', function() {
Expand Down Expand Up @@ -289,19 +337,50 @@ describe('DOMPropertyOperations', function() {
// some browsers)
expect(stubNode.className).toBe('');
});

it('should work with tags containing dashes', function() {
var element = document.createElement('x-foo-component');

expect(element.getAttribute('foobar')).toBe(null);
DOMPropertyOperations.setValueForProperty(
element,
'foobar',
'selected'
);
expect(element.getAttribute('foobar')).toBe('selected');
DOMPropertyOperations.setValueForProperty(
element,
'foobar',
null
);
expect(element.getAttribute('foobar')).toBe(null);
});

it('should reject haxors', function() {
var element = document.createElement('x-foo-component');

expect(function(){
DOMPropertyOperations.setValueForProperty(
element,
'blah" onclick="beevil" noise="hi',
'selected'
);
}).toThrow();
});
});

describe('injectDOMPropertyConfig', function() {
it('should support custom attributes', function() {
// foobar does not exist yet
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'foobar',
'simple'
)).toBe(null);

// foo-* does not exist yet
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'foo-xyz',
'simple'
)).toBe(null);
Expand All @@ -316,22 +395,26 @@ describe('DOMPropertyOperations', function() {

// Ensure old attributes still work
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'name',
'simple'
)).toBe('name="simple"');
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'data-name',
'simple'
)).toBe('data-name="simple"');

// foobar should work
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'foobar',
'simple'
)).toBe('foobar="simple"');

// foo-* should work
expect(DOMPropertyOperations.createMarkupForProperty(
'div',
'foo-xyz',
'simple'
)).toBe('foo-xyz="simple"');
Expand Down

0 comments on commit c841cf6

Please sign in to comment.