Skip to content

Refactor DOM attribute code #11804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Dec 8, 2017
Merged

Refactor DOM attribute code #11804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
24fa7e5
Harden tests around init/addition/update/removal of aliased attributes
gaearon Nov 26, 2017
91a32f2
Call setValueForProperty() for null and undefined
gaearon Nov 26, 2017
e88f4bd
Inline deleteValueForProperty() into setValueForProperty()
gaearon Nov 26, 2017
9370492
Inline deleteValueForAttribute() into setValueForAttribute()
gaearon Nov 26, 2017
171795b
Delete some dead code
gaearon Nov 26, 2017
1208a54
Make setValueForAttribute() a branch of setValueForProperty()
gaearon Nov 26, 2017
7022000
Make it more obvious where we skip and when we reset attributes
gaearon Nov 27, 2017
4018f7b
Rewrite setValueForProperty() with early exits
gaearon Nov 27, 2017
915b1c6
Move shouldIgnoreValue() into DOMProperty
gaearon Nov 27, 2017
22646c7
Use more specific methods for testing validity
gaearon Nov 27, 2017
ec83eac
Unify shouldTreatAttributeValueAsNull() and shouldIgnoreValue()
gaearon Nov 27, 2017
2484117
Remove shouldSetAttribute()
gaearon Nov 27, 2017
388498e
Remove unnecessary condition
gaearon Dec 8, 2017
632dabd
Remove another unnecessary condition
gaearon Dec 8, 2017
50c7624
Add Flow coverage
gaearon Dec 8, 2017
57d4169
Oops
gaearon Dec 8, 2017
463c4f5
Fix lint (ESLint complains about Flow suppression)
gaearon Dec 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,97 @@ 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 @@ -451,6 +542,25 @@ 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: 71 additions & 105 deletions packages/react-dom/src/client/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,33 @@
*
* 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,
shouldSetAttribute,
shouldSkipAttribute,
shouldTreatAttributeValueAsNull,
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, name, expected) {
export function getValueForProperty(
node: Element,
name: string,
expected: mixed,
): mixed {
if (__DEV__) {
const propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
return node[propertyInfo.propertyName];
const {propertyName} = propertyInfo;
return (node: any)[propertyName];
} else {
const attributeName = propertyInfo.attributeName;

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

if (shouldIgnoreValue(propertyInfo, expected)) {
if (shouldTreatAttributeValueAsNull(name, expected, false)) {
return stringValue === null ? expected : stringValue;
} else if (stringValue === '' + expected) {
} else if (stringValue === '' + (expected: any)) {
return expected;
} else {
return stringValue;
Expand All @@ -102,7 +84,11 @@ export function getValueForProperty(node, name, expected) {
* The third argument is used as a hint of what the expected value is. Some
* attributes have multiple equivalent values.
*/
export function getValueForAttribute(node, name, expected) {
export function getValueForAttribute(
node: Element,
name: string,
expected: mixed,
): mixed {
if (__DEV__) {
if (!isAttributeNameSafe(name)) {
return;
Expand All @@ -111,7 +97,7 @@ export function getValueForAttribute(node, name, expected) {
return expected === undefined ? undefined : null;
}
const value = node.getAttribute(name);
if (value === '' + expected) {
if (value === '' + (expected: any)) {
return expected;
}
return value;
Expand All @@ -125,84 +111,64 @@ export function getValueForAttribute(node, name, expected) {
* @param {string} name
* @param {*} value
*/
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, '');
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);
} else {
node.setAttribute(attributeName, '' + value);
node.setAttribute(attributeName, '' + (value: any));
}
}
} else {
setValueForAttribute(
node,
name,
shouldSetAttribute(name, value) ? value : null,
);
return;
}
}

export function setValueForAttribute(node, name, value) {
if (!isAttributeNameSafe(name)) {
const {
hasBooleanValue,
hasOverloadedBooleanValue,
mustUseProperty,
} = propertyInfo;
if (mustUseProperty) {
const {propertyName} = propertyInfo;
if (value === null) {
(node: any)[propertyName] = hasBooleanValue ? false : '';
Copy link
Contributor

@aweary aweary Dec 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All attributes that must use property are boolean values, so technically this check is unnecessary. We could add a new attribute type like HAS_BOOLEAN_PROPERTY to simplify this, and get rid of the bitmasks again (happy to do in a follow up)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow up. This diff is getting hard to deal with.

} else {
// Contrary to `setAttribute`, object properties are properly
// `toString`ed by IE8/9.
(node: any)[propertyName] = value;
}
return;
}
if (value == null) {
node.removeAttribute(name);
// The rest are treated as attributes with special cases.
const {attributeName, attributeNamespace} = propertyInfo;
if (value === null) {
node.removeAttribute(attributeName);
} else {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this test since deleteValueForProperty doesn't exist anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care? There's plenty of internal names, even in test files (e.g. ReactDOMComponent-test). I agree we need to clean it up by functionality rather than file, but don't think it's necessary to do in the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using names of internal APIs that exist is different. In this case, it's referrencing a function that no longer exists. If I was looking to contribute and encountered that, I would be confused that the function didn't exist anywhere in the codebase.

It's not critical, but since you remove it feels like it's in scope to fix any lingering references. If we're going to restructure the tests soon anyways, it probably doesn't matter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I'm referring to is that there are already plenty examples where internal names don't match tests. It used to be important for them to be in sync when unit test tested internals, but they don't anymore. I agree it's confusing, but IMO mismatches in test file names are even more confusing.

Conceptually those tests are about deleting. I wouldn't move them into set*ForProperty just because those code paths moved there. Instead I would've preferred to rename all tests to human readable descriptions (e.g. "deletion"). But then it's already inconsistent in the current test codebase so I think it's easier to do in one big cleanup later.

const propertyInfo = getPropertyInfo(name);
if (propertyInfo) {
if (propertyInfo.mustUseProperty) {
const propName = propertyInfo.propertyName;
if (propertyInfo.hasBooleanValue) {
node[propName] = false;
} else {
node[propName] = '';
}
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);
} else {
node.removeAttribute(propertyInfo.attributeName);
node.setAttribute(attributeName, attributeValue);
}
} else {
node.removeAttribute(name);
}
}
Loading