Skip to content

Commit

Permalink
Revert "Refactor DOM attribute code (#11804)" (#11814)
Browse files Browse the repository at this point in the history 
This reverts commit 47783e8.
  • Loading branch information
@gaearon
gaearon authored Dec 8, 2017
1 parent 47783e8 commit d9869a4
Show file tree
Hide file tree
Showing 7 changed files with 194 additions and 305 deletions.
110 changes: 0 additions & 110 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -442,97 +442,6 @@ describe('ReactDOMComponent', () => {
expect(container.firstChild.className).toEqual('');
});

it('should not set null/undefined attributes', () => {
var container = document.createElement('div');
// Initial render.
ReactDOM.render(<img src={null} data-foo={undefined} />, container);
var node = container.firstChild;
expect(node.hasAttribute('src')).toBe(false);
expect(node.hasAttribute('data-foo')).toBe(false);
// Update in one direction.
ReactDOM.render(<img src={undefined} data-foo={null} />, container);
expect(node.hasAttribute('src')).toBe(false);
expect(node.hasAttribute('data-foo')).toBe(false);
// Update in another direction.
ReactDOM.render(<img src={null} data-foo={undefined} />, container);
expect(node.hasAttribute('src')).toBe(false);
expect(node.hasAttribute('data-foo')).toBe(false);
// Removal.
ReactDOM.render(<img />, container);
expect(node.hasAttribute('src')).toBe(false);
expect(node.hasAttribute('data-foo')).toBe(false);
// Addition.
ReactDOM.render(<img src={undefined} data-foo={null} />, container);
expect(node.hasAttribute('src')).toBe(false);
expect(node.hasAttribute('data-foo')).toBe(false);
});

it('should apply React-specific aliases to HTML elements', () => {
var container = document.createElement('div');
ReactDOM.render(<form acceptCharset="foo" />, container);
var node = container.firstChild;
// Test attribute initialization.
expect(node.getAttribute('accept-charset')).toBe('foo');
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Test attribute update.
ReactDOM.render(<form acceptCharset="boo" />, container);
expect(node.getAttribute('accept-charset')).toBe('boo');
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Test attribute removal by setting to null.
ReactDOM.render(<form acceptCharset={null} />, container);
expect(node.hasAttribute('accept-charset')).toBe(false);
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Restore.
ReactDOM.render(<form acceptCharset="foo" />, container);
expect(node.getAttribute('accept-charset')).toBe('foo');
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Test attribute removal by setting to undefined.
ReactDOM.render(<form acceptCharset={undefined} />, container);
expect(node.hasAttribute('accept-charset')).toBe(false);
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Restore.
ReactDOM.render(<form acceptCharset="foo" />, container);
expect(node.getAttribute('accept-charset')).toBe('foo');
expect(node.hasAttribute('acceptCharset')).toBe(false);
// Test attribute removal.
ReactDOM.render(<form />, container);
expect(node.hasAttribute('accept-charset')).toBe(false);
expect(node.hasAttribute('acceptCharset')).toBe(false);
});

it('should apply React-specific aliases to SVG elements', () => {
var container = document.createElement('div');
ReactDOM.render(<svg arabicForm="foo" />, container);
var node = container.firstChild;
// Test attribute initialization.
expect(node.getAttribute('arabic-form')).toBe('foo');
expect(node.hasAttribute('arabicForm')).toBe(false);
// Test attribute update.
ReactDOM.render(<svg arabicForm="boo" />, container);
expect(node.getAttribute('arabic-form')).toBe('boo');
expect(node.hasAttribute('arabicForm')).toBe(false);
// Test attribute removal by setting to null.
ReactDOM.render(<svg arabicForm={null} />, container);
expect(node.hasAttribute('arabic-form')).toBe(false);
expect(node.hasAttribute('arabicForm')).toBe(false);
// Restore.
ReactDOM.render(<svg arabicForm="foo" />, container);
expect(node.getAttribute('arabic-form')).toBe('foo');
expect(node.hasAttribute('arabicForm')).toBe(false);
// Test attribute removal by setting to undefined.
ReactDOM.render(<svg arabicForm={undefined} />, container);
expect(node.hasAttribute('arabic-form')).toBe(false);
expect(node.hasAttribute('arabicForm')).toBe(false);
// Restore.
ReactDOM.render(<svg arabicForm="foo" />, container);
expect(node.getAttribute('arabic-form')).toBe('foo');
expect(node.hasAttribute('arabicForm')).toBe(false);
// Test attribute removal.
ReactDOM.render(<svg />, container);
expect(node.hasAttribute('arabic-form')).toBe(false);
expect(node.hasAttribute('arabicForm')).toBe(false);
});

it('should properly update custom attributes on custom elements', () => {
const container = document.createElement('div');
ReactDOM.render(<some-custom-element foo="bar" />, container);
Expand All @@ -542,25 +451,6 @@ describe('ReactDOMComponent', () => {
expect(node.getAttribute('bar')).toBe('buzz');
});

it('should not apply React-specific aliases to custom elements', () => {
var container = document.createElement('div');
ReactDOM.render(<some-custom-element arabicForm="foo" />, container);
var node = container.firstChild;
// Should not get transformed to arabic-form as SVG would be.
expect(node.getAttribute('arabicForm')).toBe('foo');
expect(node.hasAttribute('arabic-form')).toBe(false);
// Test attribute update.
ReactDOM.render(<some-custom-element arabicForm="boo" />, container);
expect(node.getAttribute('arabicForm')).toBe('boo');
// Test attribute removal and addition.
ReactDOM.render(<some-custom-element acceptCharset="buzz" />, container);
// Verify the previous attribute was removed.
expect(node.hasAttribute('arabicForm')).toBe(false);
// Should not get transformed to accept-charset as HTML would be.
expect(node.getAttribute('acceptCharset')).toBe('buzz');
expect(node.hasAttribute('accept-charset')).toBe(false);
});

it('should clear a single style prop when changing `style`', () => {
let styles = {display: 'none', color: 'red'};
const container = document.createElement('div');
Expand Down
176 changes: 105 additions & 71 deletions packages/react-dom/src/client/DOMPropertyOperations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,51 @@
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import {
ID_ATTRIBUTE_NAME,
ROOT_ATTRIBUTE_NAME,
getPropertyInfo,
shouldSkipAttribute,
shouldTreatAttributeValueAsNull,
shouldSetAttribute,
isAttributeNameSafe,
} from '../shared/DOMProperty';

// shouldIgnoreValue() is currently duplicated in DOMMarkupOperations.
// TODO: Find a better place for this.
function shouldIgnoreValue(propertyInfo, value) {
return (
value == null ||
(propertyInfo.hasBooleanValue && !value) ||
(propertyInfo.hasNumericValue && isNaN(value)) ||
(propertyInfo.hasPositiveNumericValue && value < 1) ||
(propertyInfo.hasOverloadedBooleanValue && value === false)
);
}

/**
* Operations for dealing with DOM properties.
*/

export function setAttributeForID(node, id) {
node.setAttribute(ID_ATTRIBUTE_NAME, id);
}

export function setAttributeForRoot(node) {
node.setAttribute(ROOT_ATTRIBUTE_NAME, '');
}

/**
* Get the value for a property on a node. Only used in DEV for SSR validation.
* The "expected" argument is used as a hint of what the expected value is.
* Some properties have multiple equivalent values.
*/
export function getValueForProperty(
node: Element,
name: string,
expected: mixed,
): mixed {
export function getValueForProperty(node, name, expected) {
if (__DEV__) {
const propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
const {propertyName} = propertyInfo;
return (node: any)[propertyName];
return node[propertyInfo.propertyName];
} else {
const attributeName = propertyInfo.attributeName;

Expand All @@ -41,16 +59,16 @@ export function getValueForProperty(
if (value === '') {
return true;
}
if (shouldTreatAttributeValueAsNull(name, expected, false)) {
if (shouldIgnoreValue(propertyInfo, expected)) {
return value;
}
if (value === '' + (expected: any)) {
if (value === '' + expected) {
return expected;
}
return value;
}
} else if (node.hasAttribute(attributeName)) {
if (shouldTreatAttributeValueAsNull(name, expected, false)) {
if (shouldIgnoreValue(propertyInfo, expected)) {
// We had an attribute but shouldn't have had one, so read it
// for the error message.
return node.getAttribute(attributeName);
Expand All @@ -67,9 +85,9 @@ export function getValueForProperty(
stringValue = node.getAttribute(attributeName);
}

if (shouldTreatAttributeValueAsNull(name, expected, false)) {
if (shouldIgnoreValue(propertyInfo, expected)) {
return stringValue === null ? expected : stringValue;
} else if (stringValue === '' + (expected: any)) {
} else if (stringValue === '' + expected) {
return expected;
} else {
return stringValue;
Expand All @@ -84,11 +102,7 @@ export function getValueForProperty(
* The third argument is used as a hint of what the expected value is. Some
* attributes have multiple equivalent values.
*/
export function getValueForAttribute(
node: Element,
name: string,
expected: mixed,
): mixed {
export function getValueForAttribute(node, name, expected) {
if (__DEV__) {
if (!isAttributeNameSafe(name)) {
return;
Expand All @@ -97,7 +111,7 @@ export function getValueForAttribute(
return expected === undefined ? undefined : null;
}
const value = node.getAttribute(name);
if (value === '' + (expected: any)) {
if (value === '' + expected) {
return expected;
}
return value;
Expand All @@ -111,64 +125,84 @@ export function getValueForAttribute(
* @param {string} name
* @param {*} value
*/
export function setValueForProperty(
node: Element,
name: string,
value: mixed,
isCustomComponentTag: boolean,
) {
if (shouldSkipAttribute(name, isCustomComponentTag)) {
return;
}
const propertyInfo = isCustomComponentTag ? null : getPropertyInfo(name);
if (shouldTreatAttributeValueAsNull(name, value, isCustomComponentTag)) {
value = null;
}
// If the prop isn't in the special list, treat it as a simple attribute.
if (!propertyInfo) {
if (isAttributeNameSafe(name)) {
const attributeName = name;
if (value == null) {
node.removeAttribute(attributeName);
export function setValueForProperty(node, name, value) {
const propertyInfo = getPropertyInfo(name);

if (propertyInfo && shouldSetAttribute(name, value)) {
if (shouldIgnoreValue(propertyInfo, value)) {
deleteValueForProperty(node, name);
return;
} else if (propertyInfo.mustUseProperty) {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
node[propertyInfo.propertyName] = value;
} else {
const attributeName = propertyInfo.attributeName;
const namespace = propertyInfo.attributeNamespace;
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
if (namespace) {
node.setAttributeNS(namespace, attributeName, '' + value);
} else if (
propertyInfo.hasBooleanValue ||
(propertyInfo.hasOverloadedBooleanValue && value === true)
) {
node.setAttribute(attributeName, '');
} else {
node.setAttribute(attributeName, '' + (value: any));
node.setAttribute(attributeName, '' + value);
}
}
} else {
setValueForAttribute(
node,
name,
shouldSetAttribute(name, value) ? value : null,
);
return;
}
const {
hasBooleanValue,
hasOverloadedBooleanValue,
mustUseProperty,
} = propertyInfo;
if (mustUseProperty) {
const {propertyName} = propertyInfo;
if (value === null) {
(node: any)[propertyName] = hasBooleanValue ? false : '';
} else {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
(node: any)[propertyName] = value;
}
}

export function setValueForAttribute(node, name, value) {
if (!isAttributeNameSafe(name)) {
return;
}
// The rest are treated as attributes with special cases.
const {attributeName, attributeNamespace} = propertyInfo;
if (value === null) {
node.removeAttribute(attributeName);
if (value == null) {
node.removeAttribute(name);
} else {
let attributeValue;
if (hasBooleanValue || (hasOverloadedBooleanValue && value === true)) {
attributeValue = '';
} else {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
attributeValue = '' + (value: any);
}
if (attributeNamespace) {
node.setAttributeNS(attributeNamespace, attributeName, attributeValue);
node.setAttribute(name, '' + value);
}
}

/**
* Deletes an attributes from a node.
*
* @param {DOMElement} node
* @param {string} name
*/
export function deleteValueForAttribute(node, name) {
node.removeAttribute(name);
}

/**
* Deletes the value for a property on a node.
*
* @param {DOMElement} node
* @param {string} name
*/
export function deleteValueForProperty(node, name) {
const propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
const propName = propertyInfo.propertyName;
if (propertyInfo.hasBooleanValue) {
node[propName] = false;
} else {
node[propName] = '';
}
} else {
node.setAttribute(attributeName, attributeValue);
node.removeAttribute(propertyInfo.attributeName);
}
} else {
node.removeAttribute(name);
}
}
Loading

0 comments on commit d9869a4

Please sign in to comment.