Restore old behavior for empty `href` props on anchor tags (#28124)
Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in https://github.com/facebook/react/pull/18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of https://github.com/facebook/react/pull/21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.
Sebastian Silbermann committed
Jan 31, 2024 at 00:43 UTC
f3ce87ab650f07774e1df9bc3f8033e023973d10
5 files changed
+108
-3
fixtures/attribute-behavior/AttributeTableSnapshot.md
+1
-1
@@ -5077,7 +5077,7 @@
5077
| Test Case | Flags | Result |
5078
| --- | --- | --- |
5079
| `href=(string)`| (changed)| `"https://reactjs.com/"` |
5080
-| `href=(empty string)`| (initial, warning)| `<empty string>` |
5080
+| `href=(empty string)`| (changed)| `"http://localhost:3000/"` |
5081
| `href=(array with string)`| (changed)| `"https://reactjs.com/"` |
5082
| `href=(empty array)`| (changed)| `"http://localhost:3000/"` |
5083
| `href=(object)`| (changed)| `"http://localhost:3000/result%20of%20toString()"` |
packages/react-dom-bindings/src/client/ReactDOMComponent.js
+10
-2
@@ -434,7 +434,11 @@ function setProp(
434
case 'src':
435
case 'href': {
436
if (enableFilterEmptyStringAttributesDOM) {
437
- if (value === '') {
437
+ if (
438
+ value === '' &&
439
+ // <a href=""> is fine for "reload" links.
440
+ !(tag === 'a' && key === 'href')
441
+ ) {
442
if (__DEV__) {
443
if (key === 'src') {
444
console.error(
@@ -2350,7 +2354,11 @@ function diffHydratedGenericElement(
2354
case 'src':
2355
case 'href':
2356
if (enableFilterEmptyStringAttributesDOM) {
2353
- if (value === '') {
2357
+ if (
2358
+ value === '' &&
2359
+ // <a href=""> is fine for "reload" links.
2360
+ !(tag === 'a' && propKey === 'href')
2361
+ ) {
2362
if (__DEV__) {
2363
if (propKey === 'src') {
2364
console.error(
packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
+55
@@ -1503,6 +1503,54 @@ function checkSelectProp(props: any, propName: string) {
1503
}
1504
}
1505
1506
+function pushStartAnchor(
1507
+ target: Array<Chunk | PrecomputedChunk>,
1508
+ props: Object,
1509
+): ReactNodeList {
1510
+ target.push(startChunkForTag('a'));
1511
+
1512
+ let children = null;
1513
+ let innerHTML = null;
1514
+ for (const propKey in props) {
1515
+ if (hasOwnProperty.call(props, propKey)) {
1516
+ const propValue = props[propKey];
1517
+ if (propValue == null) {
1518
+ continue;
1519
+ }
1520
+ switch (propKey) {
1521
+ case 'children':
1522
+ children = propValue;
1523
+ break;
1524
+ case 'dangerouslySetInnerHTML':
1525
+ innerHTML = propValue;
1526
+ break;
1527
+ case 'href':
1528
+ if (propValue === '') {
1529
+ // Empty `href` is special on anchors so we're short-circuiting here.
1530
+ // On other tags it should trigger a warning
1531
+ pushStringAttribute(target, 'href', '');
1532
+ } else {
1533
+ pushAttribute(target, propKey, propValue);
1534
+ }
1535
+ break;
1536
+ default:
1537
+ pushAttribute(target, propKey, propValue);
1538
+ break;
1539
+ }
1540
+ }
1541
+ }
1542
+
1543
+ target.push(endOfStartTag);
1544
+ pushInnerHTML(target, innerHTML, children);
1545
+ if (typeof children === 'string') {
1546
+ // Special case children as a string to avoid the unnecessary comment.
1547
+ // TODO: Remove this special case after the general optimization is in place.
1548
+ target.push(stringToChunk(encodeHTMLTextNode(children)));
1549
+ return null;
1550
+ }
1551
+ return children;
1552
+}
1553
+
1554
function pushStartSelect(
1555
target: Array<Chunk | PrecomputedChunk>,
1556
props: Object,
@@ -3535,7 +3583,14 @@ export function pushStartInstance(
3583
case 'span':
3584
case 'svg':
3585
case 'path':
3586
+ // Fast track very common tags
3587
+ break;
3588
case 'a':
3589
+ if (enableFilterEmptyStringAttributesDOM) {
3590
+ return pushStartAnchor(target, props);
3591
+ } else {
3592
+ break;
3593
+ }
3594
case 'g':
3595
case 'p':
3596
case 'li':
packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+10
@@ -640,6 +640,16 @@ describe('ReactDOMComponent', () => {
640
expect(node.hasAttribute('href')).toBe(false);
641
});
642
643
+ it('should allow an empty href attribute on anchors', async () => {
644
+ const container = document.createElement('div');
645
+ const root = ReactDOMClient.createRoot(container);
646
+ await act(() => {
647
+ root.render(<a href="" />);
648
+ });
649
+ const node = container.firstChild;
650
+ expect(node.getAttribute('href')).toBe('');
651
+ });
652
+
653
it('should allow an empty action attribute', async () => {
654
const container = document.createElement('div');
655
const root = ReactDOMClient.createRoot(container);
packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js
+32
@@ -54,6 +54,38 @@ describe('ReactDOMServerIntegration', () => {
54
expect(e.getAttribute('width')).toBe('30');
55
});
56
57
+ itRenders('empty src on img', async render => {
58
+ const e = await render(
59
+ <img src="" />,
60
+ ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
61
+ );
62
+ expect(e.getAttribute('src')).toBe(
63
+ ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
64
+ );
65
+ });
66
+
67
+ itRenders('empty href on anchor', async render => {
68
+ const e = await render(<a href="" />);
69
+ expect(e.getAttribute('href')).toBe('');
70
+ });
71
+
72
+ itRenders('empty href on other tags', async render => {
73
+ const e = await render(
74
+ // <link href="" /> would be more sensible.
75
+ // However, that results in a hydration warning as well.
76
+ // Our test helpers do not support different error counts for initial
77
+ // server render and hydration.
78
+ // The number of errors on the server need to be equal to the number of
79
+ // errors during hydration.
80
+ // So we use a <div> instead.
81
+ <div href="" />,
82
+ ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
83
+ );
84
+ expect(e.getAttribute('href')).toBe(
85
+ ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
86
+ );
87
+ });
88
+
89
itRenders('no string prop with true value', async render => {
90
const e = await render(<a href={true} />, 1);
91
expect(e.hasAttribute('href')).toBe(false);