Refactor DOM attribute code (#11804)
* Harden tests around init/addition/update/removal of aliased attributes I noticed some patterns weren't being tested. * Call setValueForProperty() for null and undefined The branching before the call is unnecessary because setValueForProperty() already has an internal branch that delegates to deleteValueForProperty() for null and undefined through the shouldIgnoreValue() check. The goal is to start unifying these methods because their separation doesn't reflect the current behavior (e.g. for unknown properties) anymore, and obscures what actually happens with different inputs. * Inline deleteValueForProperty() into setValueForProperty() Now we don't read propertyInfo twice in this case. I also dropped a few early returns. I added them a while ago when we had Stack-only tracking of DOM operations, and some operations were being counted twice because of how this code is structured. This isn't a problem anymore (both because we don't track operations, and because I've just inlined this method call). * Inline deleteValueForAttribute() into setValueForAttribute() The special cases for null and undefined already exist in setValueForAttribute(). * Delete some dead code * Make setValueForAttribute() a branch of setValueForProperty() Their naming is pretty confusing by now. For example setValueForProperty() calls setValueForAttribute() when shouldSetAttribute() is false (!). I want to refactor (as in, inline and then maybe factor it out differently) the relation between them. For now, I'm consolidating the callers to use setValueForProperty(). * Make it more obvious where we skip and when we reset attributes The naming of these methods is still very vague and conflicting in some cases. Will need further work. * Rewrite setValueForProperty() with early exits This makes the flow clearer in my opinion. * Move shouldIgnoreValue() into DOMProperty It was previously duplicated. It's also suspiciously similar in purpose to shouldTreatAttributeValueAsNull() so I want to see if there is a way to unify them. * Use more specific methods for testing validity * Unify shouldTreatAttributeValueAsNull() and shouldIgnoreValue() * Remove shouldSetAttribute() Its naming was confusing and it was used all over the place instead of more specific checks. Now that we only have one call site, we might as well inline and get rid of it. * Remove unnecessary condition * Remove another unnecessary condition * Add Flow coverage * Oops * Fix lint (ESLint complains about Flow suppression)
Dan Abramov committed
Dec 8, 2017 at 20:42 UTC
47783e878d62ed96ea27290d1cdbb90b83c417c4
7 files changed
+305
-194
packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+110
@@ -442,6 +442,97 @@ describe('ReactDOMComponent', () => {
442
expect(container.firstChild.className).toEqual('');
443
});
444
445
+ it('should not set null/undefined attributes', () => {
446
+ var container = document.createElement('div');
447
+ // Initial render.
448
+ ReactDOM.render(<img src={null} data-foo={undefined} />, container);
449
+ var node = container.firstChild;
450
+ expect(node.hasAttribute('src')).toBe(false);
451
+ expect(node.hasAttribute('data-foo')).toBe(false);
452
+ // Update in one direction.
453
+ ReactDOM.render(<img src={undefined} data-foo={null} />, container);
454
+ expect(node.hasAttribute('src')).toBe(false);
455
+ expect(node.hasAttribute('data-foo')).toBe(false);
456
+ // Update in another direction.
457
+ ReactDOM.render(<img src={null} data-foo={undefined} />, container);
458
+ expect(node.hasAttribute('src')).toBe(false);
459
+ expect(node.hasAttribute('data-foo')).toBe(false);
460
+ // Removal.
461
+ ReactDOM.render(<img />, container);
462
+ expect(node.hasAttribute('src')).toBe(false);
463
+ expect(node.hasAttribute('data-foo')).toBe(false);
464
+ // Addition.
465
+ ReactDOM.render(<img src={undefined} data-foo={null} />, container);
466
+ expect(node.hasAttribute('src')).toBe(false);
467
+ expect(node.hasAttribute('data-foo')).toBe(false);
468
+ });
469
+
470
+ it('should apply React-specific aliases to HTML elements', () => {
471
+ var container = document.createElement('div');
472
+ ReactDOM.render(<form acceptCharset="foo" />, container);
473
+ var node = container.firstChild;
474
+ // Test attribute initialization.
475
+ expect(node.getAttribute('accept-charset')).toBe('foo');
476
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
477
+ // Test attribute update.
478
+ ReactDOM.render(<form acceptCharset="boo" />, container);
479
+ expect(node.getAttribute('accept-charset')).toBe('boo');
480
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
481
+ // Test attribute removal by setting to null.
482
+ ReactDOM.render(<form acceptCharset={null} />, container);
483
+ expect(node.hasAttribute('accept-charset')).toBe(false);
484
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
485
+ // Restore.
486
+ ReactDOM.render(<form acceptCharset="foo" />, container);
487
+ expect(node.getAttribute('accept-charset')).toBe('foo');
488
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
489
+ // Test attribute removal by setting to undefined.
490
+ ReactDOM.render(<form acceptCharset={undefined} />, container);
491
+ expect(node.hasAttribute('accept-charset')).toBe(false);
492
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
493
+ // Restore.
494
+ ReactDOM.render(<form acceptCharset="foo" />, container);
495
+ expect(node.getAttribute('accept-charset')).toBe('foo');
496
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
497
+ // Test attribute removal.
498
+ ReactDOM.render(<form />, container);
499
+ expect(node.hasAttribute('accept-charset')).toBe(false);
500
+ expect(node.hasAttribute('acceptCharset')).toBe(false);
501
+ });
502
+
503
+ it('should apply React-specific aliases to SVG elements', () => {
504
+ var container = document.createElement('div');
505
+ ReactDOM.render(<svg arabicForm="foo" />, container);
506
+ var node = container.firstChild;
507
+ // Test attribute initialization.
508
+ expect(node.getAttribute('arabic-form')).toBe('foo');
509
+ expect(node.hasAttribute('arabicForm')).toBe(false);
510
+ // Test attribute update.
511
+ ReactDOM.render(<svg arabicForm="boo" />, container);
512
+ expect(node.getAttribute('arabic-form')).toBe('boo');
513
+ expect(node.hasAttribute('arabicForm')).toBe(false);
514
+ // Test attribute removal by setting to null.
515
+ ReactDOM.render(<svg arabicForm={null} />, container);
516
+ expect(node.hasAttribute('arabic-form')).toBe(false);
517
+ expect(node.hasAttribute('arabicForm')).toBe(false);
518
+ // Restore.
519
+ ReactDOM.render(<svg arabicForm="foo" />, container);
520
+ expect(node.getAttribute('arabic-form')).toBe('foo');
521
+ expect(node.hasAttribute('arabicForm')).toBe(false);
522
+ // Test attribute removal by setting to undefined.
523
+ ReactDOM.render(<svg arabicForm={undefined} />, container);
524
+ expect(node.hasAttribute('arabic-form')).toBe(false);
525
+ expect(node.hasAttribute('arabicForm')).toBe(false);
526
+ // Restore.
527
+ ReactDOM.render(<svg arabicForm="foo" />, container);
528
+ expect(node.getAttribute('arabic-form')).toBe('foo');
529
+ expect(node.hasAttribute('arabicForm')).toBe(false);
530
+ // Test attribute removal.
531
+ ReactDOM.render(<svg />, container);
532
+ expect(node.hasAttribute('arabic-form')).toBe(false);
533
+ expect(node.hasAttribute('arabicForm')).toBe(false);
534
+ });
535
+
536
it('should properly update custom attributes on custom elements', () => {
537
const container = document.createElement('div');
538
ReactDOM.render(<some-custom-element foo="bar" />, container);
@@ -451,6 +542,25 @@ describe('ReactDOMComponent', () => {
542
expect(node.getAttribute('bar')).toBe('buzz');
543
});
544
545
+ it('should not apply React-specific aliases to custom elements', () => {
546
+ var container = document.createElement('div');
547
+ ReactDOM.render(<some-custom-element arabicForm="foo" />, container);
548
+ var node = container.firstChild;
549
+ // Should not get transformed to arabic-form as SVG would be.
550
+ expect(node.getAttribute('arabicForm')).toBe('foo');
551
+ expect(node.hasAttribute('arabic-form')).toBe(false);
552
+ // Test attribute update.
553
+ ReactDOM.render(<some-custom-element arabicForm="boo" />, container);
554
+ expect(node.getAttribute('arabicForm')).toBe('boo');
555
+ // Test attribute removal and addition.
556
+ ReactDOM.render(<some-custom-element acceptCharset="buzz" />, container);
557
+ // Verify the previous attribute was removed.
558
+ expect(node.hasAttribute('arabicForm')).toBe(false);
559
+ // Should not get transformed to accept-charset as HTML would be.
560
+ expect(node.getAttribute('acceptCharset')).toBe('buzz');
561
+ expect(node.hasAttribute('accept-charset')).toBe(false);
562
+ });
563
+
564
it('should clear a single style prop when changing `style`', () => {
565
let styles = {display: 'none', color: 'red'};
566
const container = document.createElement('div');
packages/react-dom/src/client/DOMPropertyOperations.js
+71
-105
@@ -3,51 +3,33 @@
3
*
4
* This source code is licensed under the MIT license found in the
5
* LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @flow
8
*/
9
10
import {
9
- ID_ATTRIBUTE_NAME,
10
- ROOT_ATTRIBUTE_NAME,
11
getPropertyInfo,
12
- shouldSetAttribute,
12
+ shouldSkipAttribute,
13
+ shouldTreatAttributeValueAsNull,
14
isAttributeNameSafe,
15
} from '../shared/DOMProperty';
16
16
-// shouldIgnoreValue() is currently duplicated in DOMMarkupOperations.
17
-// TODO: Find a better place for this.
18
-function shouldIgnoreValue(propertyInfo, value) {
19
- return (
20
- value == null ||
21
- (propertyInfo.hasBooleanValue && !value) ||
22
- (propertyInfo.hasNumericValue && isNaN(value)) ||
23
- (propertyInfo.hasPositiveNumericValue && value < 1) ||
24
- (propertyInfo.hasOverloadedBooleanValue && value === false)
25
- );
26
-}
27
-
28
-/**
29
- * Operations for dealing with DOM properties.
30
- */
31
-
32
-export function setAttributeForID(node, id) {
33
- node.setAttribute(ID_ATTRIBUTE_NAME, id);
34
-}
35
-
36
-export function setAttributeForRoot(node) {
37
- node.setAttribute(ROOT_ATTRIBUTE_NAME, '');
38
-}
39
-
17
/**
18
* Get the value for a property on a node. Only used in DEV for SSR validation.
19
* The "expected" argument is used as a hint of what the expected value is.
20
* Some properties have multiple equivalent values.
21
*/
45
-export function getValueForProperty(node, name, expected) {
22
+export function getValueForProperty(
23
+ node: Element,
24
+ name: string,
25
+ expected: mixed,
26
+): mixed {
27
if (__DEV__) {
28
const propertyInfo = getPropertyInfo(name);
29
if (propertyInfo) {
30
if (propertyInfo.mustUseProperty) {
50
- return node[propertyInfo.propertyName];
31
+ const {propertyName} = propertyInfo;
32
+ return (node: any)[propertyName];
33
} else {
34
const attributeName = propertyInfo.attributeName;
35
@@ -59,16 +41,16 @@ export function getValueForProperty(node, name, expected) {
41
if (value === '') {
42
return true;
43
}
62
- if (shouldIgnoreValue(propertyInfo, expected)) {
44
+ if (shouldTreatAttributeValueAsNull(name, expected, false)) {
45
return value;
46
}
65
- if (value === '' + expected) {
47
+ if (value === '' + (expected: any)) {
48
return expected;
49
}
50
return value;
51
}
52
} else if (node.hasAttribute(attributeName)) {
71
- if (shouldIgnoreValue(propertyInfo, expected)) {
53
+ if (shouldTreatAttributeValueAsNull(name, expected, false)) {
54
// We had an attribute but shouldn't have had one, so read it
55
// for the error message.
56
return node.getAttribute(attributeName);
@@ -85,9 +67,9 @@ export function getValueForProperty(node, name, expected) {
67
stringValue = node.getAttribute(attributeName);
68
}
69
88
- if (shouldIgnoreValue(propertyInfo, expected)) {
70
+ if (shouldTreatAttributeValueAsNull(name, expected, false)) {
71
return stringValue === null ? expected : stringValue;
90
- } else if (stringValue === '' + expected) {
72
+ } else if (stringValue === '' + (expected: any)) {
73
return expected;
74
} else {
75
return stringValue;
@@ -102,7 +84,11 @@ export function getValueForProperty(node, name, expected) {
84
* The third argument is used as a hint of what the expected value is. Some
85
* attributes have multiple equivalent values.
86
*/
105
-export function getValueForAttribute(node, name, expected) {
87
+export function getValueForAttribute(
88
+ node: Element,
89
+ name: string,
90
+ expected: mixed,
91
+): mixed {
92
if (__DEV__) {
93
if (!isAttributeNameSafe(name)) {
94
return;
@@ -111,7 +97,7 @@ export function getValueForAttribute(node, name, expected) {
97
return expected === undefined ? undefined : null;
98
}
99
const value = node.getAttribute(name);
114
- if (value === '' + expected) {
100
+ if (value === '' + (expected: any)) {
101
return expected;
102
}
103
return value;
@@ -125,84 +111,64 @@ export function getValueForAttribute(node, name, expected) {
111
* @param {string} name
112
* @param {*} value
113
*/
128
-export function setValueForProperty(node, name, value) {
129
- const propertyInfo = getPropertyInfo(name);
130
-
131
- if (propertyInfo && shouldSetAttribute(name, value)) {
132
- if (shouldIgnoreValue(propertyInfo, value)) {
133
- deleteValueForProperty(node, name);
134
- return;
135
- } else if (propertyInfo.mustUseProperty) {
136
- // Contrary to `setAttribute`, object properties are properly
137
- // `toString`ed by IE8/9.
138
- node[propertyInfo.propertyName] = value;
139
- } else {
140
- const attributeName = propertyInfo.attributeName;
141
- const namespace = propertyInfo.attributeNamespace;
142
- // `setAttribute` with objects becomes only `[object]` in IE8/9,
143
- // ('' + value) makes it output the correct toString()-value.
144
- if (namespace) {
145
- node.setAttributeNS(namespace, attributeName, '' + value);
146
- } else if (
147
- propertyInfo.hasBooleanValue ||
148
- (propertyInfo.hasOverloadedBooleanValue && value === true)
149
- ) {
150
- node.setAttribute(attributeName, '');
114
+export function setValueForProperty(
115
+ node: Element,
116
+ name: string,
117
+ value: mixed,
118
+ isCustomComponentTag: boolean,
119
+) {
120
+ if (shouldSkipAttribute(name, isCustomComponentTag)) {
121
+ return;
122
+ }
123
+ const propertyInfo = isCustomComponentTag ? null : getPropertyInfo(name);
124
+ if (shouldTreatAttributeValueAsNull(name, value, isCustomComponentTag)) {
125
+ value = null;
126
+ }
127
+ // If the prop isn't in the special list, treat it as a simple attribute.
128
+ if (!propertyInfo) {
129
+ if (isAttributeNameSafe(name)) {
130
+ const attributeName = name;
131
+ if (value == null) {
132
+ node.removeAttribute(attributeName);
133
} else {
152
- node.setAttribute(attributeName, '' + value);
134
+ node.setAttribute(attributeName, '' + (value: any));
135
}
136
}
155
- } else {
156
- setValueForAttribute(
157
- node,
158
- name,
159
- shouldSetAttribute(name, value) ? value : null,
160
- );
137
return;
138
}
163
-}
164
-
165
-export function setValueForAttribute(node, name, value) {
166
- if (!isAttributeNameSafe(name)) {
139
+ const {
140
+ hasBooleanValue,
141
+ hasOverloadedBooleanValue,
142
+ mustUseProperty,
143
+ } = propertyInfo;
144
+ if (mustUseProperty) {
145
+ const {propertyName} = propertyInfo;
146
+ if (value === null) {
147
+ (node: any)[propertyName] = hasBooleanValue ? false : '';
148
+ } else {
149
+ // Contrary to `setAttribute`, object properties are properly
150
+ // `toString`ed by IE8/9.
151
+ (node: any)[propertyName] = value;
152
+ }
153
return;
154
}
169
- if (value == null) {
170
- node.removeAttribute(name);
155
+ // The rest are treated as attributes with special cases.
156
+ const {attributeName, attributeNamespace} = propertyInfo;
157
+ if (value === null) {
158
+ node.removeAttribute(attributeName);
159
} else {
172
- node.setAttribute(name, '' + value);
173
- }
174
-}
175
-
176
-/**
177
- * Deletes an attributes from a node.
178
- *
179
- * @param {DOMElement} node
180
- * @param {string} name
181
- */
182
-export function deleteValueForAttribute(node, name) {
183
- node.removeAttribute(name);
184
-}
185
-
186
-/**
187
- * Deletes the value for a property on a node.
188
- *
189
- * @param {DOMElement} node
190
- * @param {string} name
191
- */
192
-export function deleteValueForProperty(node, name) {
193
- const propertyInfo = getPropertyInfo(name);
194
- if (propertyInfo) {
195
- if (propertyInfo.mustUseProperty) {
196
- const propName = propertyInfo.propertyName;
197
- if (propertyInfo.hasBooleanValue) {
198
- node[propName] = false;
199
- } else {
200
- node[propName] = '';
201
- }
160
+ let attributeValue;
161
+ if (hasBooleanValue || (hasOverloadedBooleanValue && value === true)) {
162
+ attributeValue = '';
163
+ } else {
164
+ // `setAttribute` with objects becomes only `[object]` in IE8/9,
165
+ // ('' + value) makes it output the correct toString()-value.
166
+ attributeValue = '' + (value: any);
167
+ }
168
+ if (attributeNamespace) {
169
+ node.setAttributeNS(attributeNamespace, attributeName, attributeValue);
170
} else {
203
- node.removeAttribute(propertyInfo.attributeName);
171
+ node.setAttribute(attributeName, attributeValue);
172
}
205
- } else {
206
- node.removeAttribute(name);
173
}
174
}
packages/react-dom/src/client/ReactDOMFiberComponent.js
+30
-25
@@ -24,7 +24,11 @@ import setTextContent from './setTextContent';
24
import {listenTo, trapBubbledEvent} from '../events/ReactBrowserEventEmitter';
25
import * as CSSPropertyOperations from '../shared/CSSPropertyOperations';
26
import {Namespaces, getIntrinsicNamespace} from '../shared/DOMNamespaces';
27
-import {getPropertyInfo, shouldSetAttribute} from '../shared/DOMProperty';
27
+import {
28
+ getPropertyInfo,
29
+ shouldSkipAttribute,
30
+ shouldTreatAttributeValueAsNull,
31
+} from '../shared/DOMProperty';
32
import assertValidProps from '../shared/assertValidProps';
33
import {DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE} from '../shared/HTMLNodeType';
34
import isCustomComponent from '../shared/isCustomComponent';
@@ -314,13 +318,13 @@ function setInitialDOMProperties(
318
}
319
ensureListeningTo(rootContainerElement, propKey);
320
}
317
- } else if (isCustomComponentTag) {
318
- DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp);
321
} else if (nextProp != null) {
320
- // If we're updating to null or undefined, we should remove the property
321
- // from the DOM node instead of inadvertently setting to a string. This
322
- // brings us in line with the same behavior we have on initial render.
323
- DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp);
322
+ DOMPropertyOperations.setValueForProperty(
323
+ domElement,
324
+ propKey,
325
+ nextProp,
326
+ isCustomComponentTag,
327
+ );
328
}
329
}
330
}
@@ -341,23 +345,13 @@ function updateDOMProperties(
345
setInnerHTML(domElement, propValue);
346
} else if (propKey === CHILDREN) {
347
setTextContent(domElement, propValue);
344
- } else if (isCustomComponentTag) {
345
- if (propValue != null) {
346
- DOMPropertyOperations.setValueForAttribute(
347
- domElement,
348
- propKey,
349
- propValue,
350
- );
351
- } else {
352
- DOMPropertyOperations.deleteValueForAttribute(domElement, propKey);
353
- }
354
- } else if (propValue != null) {
355
- DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue);
348
} else {
357
- // If we're updating to null or undefined, we should remove the property
358
- // from the DOM node instead of inadvertently setting to a string. This
359
- // brings us in line with the same behavior we have on initial render.
360
- DOMPropertyOperations.deleteValueForProperty(domElement, propKey);
349
+ DOMPropertyOperations.setValueForProperty(
350
+ domElement,
351
+ propKey,
352
+ propValue,
353
+ isCustomComponentTag,
354
+ );
355
}
356
}
357
}
@@ -965,7 +959,11 @@ export function diffHydratedProperties(
959
}
960
ensureListeningTo(rootContainerElement, propKey);
961
}
968
- } else if (__DEV__) {
962
+ } else if (
963
+ __DEV__ &&
964
+ // Convince Flow we've calculated it (it's DEV-only in this method.)
965
+ typeof isCustomComponentTag === 'boolean'
966
+ ) {
967
// Validate that the properties correspond to their expected values.
968
let serverValue;
969
let propertyInfo;
@@ -1010,7 +1008,14 @@ export function diffHydratedProperties(
1008
if (nextProp !== serverValue) {
1009
warnForPropDifference(propKey, serverValue, nextProp);
1010
}
1013
- } else if (shouldSetAttribute(propKey, nextProp)) {
1011
+ } else if (
1012
+ !shouldSkipAttribute(propKey, isCustomComponentTag) &&
1013
+ !shouldTreatAttributeValueAsNull(
1014
+ propKey,
1015
+ nextProp,
1016
+ isCustomComponentTag,
1017
+ )
1018
+ ) {
1019
if ((propertyInfo = getPropertyInfo(propKey))) {
1020
// $FlowFixMe - Should be inferred as not undefined.
1021
extraAttributeNames.delete(propertyInfo.attributeName);
packages/react-dom/src/client/ReactDOMFiberInput.js
+1
-1
@@ -133,7 +133,7 @@ export function updateChecked(element: Element, props: Object) {
133
const node = ((element: any): InputWithWrapperState);
134
const checked = props.checked;
135
if (checked != null) {
136
- DOMPropertyOperations.setValueForProperty(node, 'checked', checked);
136
+ DOMPropertyOperations.setValueForProperty(node, 'checked', checked, false);
137
}
138
}
139
packages/react-dom/src/server/DOMMarkupOperations.js
+19
-30
@@ -3,30 +3,20 @@
3
*
4
* This source code is licensed under the MIT license found in the
5
* LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @flow
8
*/
9
10
import {
11
ID_ATTRIBUTE_NAME,
12
ROOT_ATTRIBUTE_NAME,
13
getPropertyInfo,
12
- shouldAttributeAcceptBooleanValue,
13
- shouldSetAttribute,
14
isAttributeNameSafe,
15
+ shouldSkipAttribute,
16
+ shouldTreatAttributeValueAsNull,
17
} from '../shared/DOMProperty';
18
import quoteAttributeValueForBrowser from './quoteAttributeValueForBrowser';
19
18
-// shouldIgnoreValue() is currently duplicated in DOMPropertyOperations.
19
-// TODO: Find a better place for this.
20
-function shouldIgnoreValue(propertyInfo, value) {
21
- return (
22
- value == null ||
23
- (propertyInfo.hasBooleanValue && !value) ||
24
- (propertyInfo.hasNumericValue && isNaN(value)) ||
25
- (propertyInfo.hasPositiveNumericValue && value < 1) ||
26
- (propertyInfo.hasOverloadedBooleanValue && value === false)
27
- );
28
-}
29
-
20
/**
21
* Operations for dealing with DOM properties.
22
*/
@@ -37,11 +27,11 @@ function shouldIgnoreValue(propertyInfo, value) {
27
* @param {string} id Unescaped ID.
28
* @return {string} Markup string.
29
*/
40
-export function createMarkupForID(id) {
30
+export function createMarkupForID(id: string): string {
31
return ID_ATTRIBUTE_NAME + '=' + quoteAttributeValueForBrowser(id);
32
}
33
44
-export function createMarkupForRoot() {
34
+export function createMarkupForRoot(): string {
35
return ROOT_ATTRIBUTE_NAME + '=""';
36
}
37
@@ -52,31 +42,27 @@ export function createMarkupForRoot() {
42
* @param {*} value
43
* @return {?string} Markup string, or null if the property was invalid.
44
*/
55
-export function createMarkupForProperty(name, value) {
45
+export function createMarkupForProperty(name: string, value: mixed): string {
46
+ if (name !== 'style' && shouldSkipAttribute(name, false)) {
47
+ return '';
48
+ }
49
+ if (shouldTreatAttributeValueAsNull(name, value, false)) {
50
+ return '';
51
+ }
52
const propertyInfo = getPropertyInfo(name);
53
if (propertyInfo) {
58
- if (shouldIgnoreValue(propertyInfo, value)) {
59
- return '';
60
- }
54
const attributeName = propertyInfo.attributeName;
55
if (
56
propertyInfo.hasBooleanValue ||
57
(propertyInfo.hasOverloadedBooleanValue && value === true)
58
) {
59
return attributeName + '=""';
67
- } else if (
68
- typeof value !== 'boolean' ||
69
- shouldAttributeAcceptBooleanValue(name)
70
- ) {
60
+ } else {
61
return attributeName + '=' + quoteAttributeValueForBrowser(value);
62
}
73
- } else if (shouldSetAttribute(name, value)) {
74
- if (value == null) {
75
- return '';
76
- }
63
+ } else {
64
return name + '=' + quoteAttributeValueForBrowser(value);
65
}
79
- return null;
66
}
67
68
/**
@@ -86,7 +72,10 @@ export function createMarkupForProperty(name, value) {
72
* @param {*} value
73
* @return {string} Markup string, or empty string if the property was invalid.
74
*/
89
-export function createMarkupForCustomAttribute(name, value) {
75
+export function createMarkupForCustomAttribute(
76
+ name: string,
77
+ value: mixed,
78
+): string {
79
if (!isAttributeNameSafe(name) || value == null) {
80
return '';
81
}
packages/react-dom/src/shared/DOMProperty.js
+71
-29
@@ -3,10 +3,24 @@
3
*
4
* This source code is licensed under the MIT license found in the
5
* LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @flow
8
*/
9
10
import warning from 'fbjs/lib/warning';
11
12
+type PropertyInfo = {|
13
+ attributeName: string,
14
+ attributeNamespace: string | null,
15
+ propertyName: string,
16
+ mustUseProperty: boolean,
17
+ hasBooleanValue: boolean,
18
+ hasNumericValue: boolean,
19
+ hasPositiveNumericValue: boolean,
20
+ hasOverloadedBooleanValue: boolean,
21
+ hasStringBooleanValue: boolean,
22
+|};
23
+
24
// These attributes should be all lowercase to allow for
25
// case insensitive checks
26
const RESERVED_PROPS = {
@@ -54,7 +68,7 @@ function injectDOMPropertyConfig(domPropertyConfig) {
68
const lowerCased = propName.toLowerCase();
69
const propConfig = Properties[propName];
70
57
- const propertyInfo = {
71
+ const propertyInfo: PropertyInfo = {
72
attributeName: lowerCased,
73
attributeNamespace: null,
74
propertyName: propName,
@@ -118,7 +132,7 @@ export const VALID_ATTRIBUTE_NAME_REGEX = new RegExp(
132
const illegalAttributeNameCache = {};
133
const validatedAttributeNameCache = {};
134
121
-export function isAttributeNameSafe(attributeName) {
135
+export function isAttributeNameSafe(attributeName: string): boolean {
136
if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
137
return true;
138
}
@@ -163,12 +177,14 @@ export function isAttributeNameSafe(attributeName) {
177
*/
178
export const properties = {};
179
166
-/**
167
- * Checks whether a property name is a writeable attribute.
168
- * @method
169
- */
170
-export function shouldSetAttribute(name, value) {
180
+export function shouldSkipAttribute(
181
+ name: string,
182
+ isCustomComponentTag: boolean,
183
+): boolean {
184
if (isReservedProp(name)) {
185
+ return true;
186
+ }
187
+ if (isCustomComponentTag) {
188
return false;
189
}
190
if (
@@ -176,43 +192,69 @@ export function shouldSetAttribute(name, value) {
192
(name[0] === 'o' || name[0] === 'O') &&
193
(name[1] === 'n' || name[1] === 'N')
194
) {
179
- return false;
180
- }
181
- if (value === null) {
195
return true;
196
}
197
+ return false;
198
+}
199
+
200
+export function isBadlyTypedAttributeValue(
201
+ name: string,
202
+ value: mixed,
203
+ isCustomComponentTag: boolean,
204
+): boolean {
205
+ if (isReservedProp(name)) {
206
+ return false;
207
+ }
208
switch (typeof value) {
185
- case 'boolean':
186
- return shouldAttributeAcceptBooleanValue(name);
187
- case 'undefined':
188
- case 'number':
189
- case 'string':
190
- case 'object':
209
+ case 'function':
210
+ // $FlowIssue symbol is perfectly valid here
211
+ case 'symbol': // eslint-disable-line
212
return true;
213
+ case 'boolean':
214
+ break;
215
default:
193
- // function, symbol
216
return false;
217
}
196
-}
197
-
198
-export function getPropertyInfo(name) {
199
- return properties.hasOwnProperty(name) ? properties[name] : null;
200
-}
201
-
202
-export function shouldAttributeAcceptBooleanValue(name) {
203
- if (isReservedProp(name)) {
204
- return true;
218
+ if (isCustomComponentTag) {
219
+ return false;
220
}
221
let propertyInfo = getPropertyInfo(name);
222
if (propertyInfo) {
208
- return (
223
+ return !(
224
propertyInfo.hasBooleanValue ||
225
propertyInfo.hasStringBooleanValue ||
226
propertyInfo.hasOverloadedBooleanValue
227
);
228
}
229
const prefix = name.toLowerCase().slice(0, 5);
215
- return prefix === 'data-' || prefix === 'aria-';
230
+ return prefix !== 'data-' && prefix !== 'aria-';
231
+}
232
+
233
+export function shouldTreatAttributeValueAsNull(
234
+ name: string,
235
+ value: mixed,
236
+ isCustomComponentTag: boolean,
237
+): boolean {
238
+ if (value === null || typeof value === 'undefined') {
239
+ return true;
240
+ }
241
+ const propertyInfo = getPropertyInfo(name);
242
+ if (propertyInfo) {
243
+ if (propertyInfo.hasBooleanValue) {
244
+ return !value;
245
+ } else if (propertyInfo.hasOverloadedBooleanValue) {
246
+ return value === false;
247
+ } else if (propertyInfo.hasNumericValue && isNaN(value)) {
248
+ return true;
249
+ } else if (propertyInfo.hasPositiveNumericValue && (value: any) < 1) {
250
+ return true;
251
+ }
252
+ }
253
+ return isBadlyTypedAttributeValue(name, value, isCustomComponentTag);
254
+}
255
+
256
+export function getPropertyInfo(name: string): PropertyInfo | null {
257
+ return properties.hasOwnProperty(name) ? properties[name] : null;
258
}
259
260
/**
@@ -224,7 +266,7 @@ export function shouldAttributeAcceptBooleanValue(name) {
266
* @param {string} name
267
* @return {boolean} If the name is within reserved props
268
*/
227
-export function isReservedProp(name) {
269
+export function isReservedProp(name: string): boolean {
270
return RESERVED_PROPS.hasOwnProperty(name);
271
}
272
packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
+3
-4
@@ -15,8 +15,7 @@ import warning from 'fbjs/lib/warning';
15
import {
16
ATTRIBUTE_NAME_CHAR,
17
isReservedProp,
18
- shouldAttributeAcceptBooleanValue,
19
- shouldSetAttribute,
18
+ isBadlyTypedAttributeValue,
19
} from './DOMProperty';
20
import isCustomComponent from './isCustomComponent';
21
import possibleStandardNames from './possibleStandardNames';
@@ -191,7 +190,7 @@ if (__DEV__) {
190
191
if (
192
typeof value === 'boolean' &&
194
- !shouldAttributeAcceptBooleanValue(name)
193
+ isBadlyTypedAttributeValue(name, value, false)
194
) {
195
if (value) {
196
warning(
@@ -235,7 +234,7 @@ if (__DEV__) {
234
}
235
236
// Warn when a known attribute is a bad type
238
- if (!shouldSetAttribute(name, value)) {
237
+ if (isBadlyTypedAttributeValue(name, value, false)) {
238
warnedProperties[name] = true;
239
return false;
240
}