Restore old behavior for empty href props on anchor tags (#28124)

eps1lon · web-flow · commit f3ce87ab650f · 2024-01-31T00:43:40.000+01:00
Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #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 #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.
diff --git a/fixtures/attribute-behavior/AttributeTableSnapshot.md b/fixtures/attribute-behavior/AttributeTableSnapshot.md
@@ -5077,7 +5077,7 @@
 | Test Case | Flags | Result |
 | --- | --- | --- |
 | `href=(string)`| (changed)| `"https://reactjs.com/"` |
-| `href=(empty string)`| (initial, warning)| `<empty string>` |
+| `href=(empty string)`| (changed)| `"http://localhost:3000/"` |
 | `href=(array with string)`| (changed)| `"https://reactjs.com/"` |
 | `href=(empty array)`| (changed)| `"http://localhost:3000/"` |
 | `href=(object)`| (changed)| `"http://localhost:3000/result%20of%20toString()"` |
diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js
@@ -434,7 +434,11 @@ function setProp(
     case 'src':
     case 'href': {
       if (enableFilterEmptyStringAttributesDOM) {
-        if (value === '') {
+        if (
+          value === '' &&
+          // <a href=""> is fine for "reload" links.
+          !(tag === 'a' && key === 'href')
+        ) {
           if (__DEV__) {
             if (key === 'src') {
               console.error(
@@ -2350,7 +2354,11 @@ function diffHydratedGenericElement(
       case 'src':
       case 'href':
         if (enableFilterEmptyStringAttributesDOM) {
-          if (value === '') {
+          if (
+            value === '' &&
+            // <a href=""> is fine for "reload" links.
+            !(tag === 'a' && propKey === 'href')
+          ) {
             if (__DEV__) {
               if (propKey === 'src') {
                 console.error(
diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
@@ -1503,6 +1503,54 @@ function checkSelectProp(props: any, propName: string) {
   }
 }
 
+function pushStartAnchor(
+  target: Array<Chunk | PrecomputedChunk>,
+  props: Object,
+): ReactNodeList {
+  target.push(startChunkForTag('a'));
+
+  let children = null;
+  let innerHTML = null;
+  for (const propKey in props) {
+    if (hasOwnProperty.call(props, propKey)) {
+      const propValue = props[propKey];
+      if (propValue == null) {
+        continue;
+      }
+      switch (propKey) {
+        case 'children':
+          children = propValue;
+          break;
+        case 'dangerouslySetInnerHTML':
+          innerHTML = propValue;
+          break;
+        case 'href':
+          if (propValue === '') {
+            // Empty `href` is special on anchors so we're short-circuiting here.
+            // On other tags it should trigger a warning
+            pushStringAttribute(target, 'href', '');
+          } else {
+            pushAttribute(target, propKey, propValue);
+          }
+          break;
+        default:
+          pushAttribute(target, propKey, propValue);
+          break;
+      }
+    }
+  }
+
+  target.push(endOfStartTag);
+  pushInnerHTML(target, innerHTML, children);
+  if (typeof children === 'string') {
+    // Special case children as a string to avoid the unnecessary comment.
+    // TODO: Remove this special case after the general optimization is in place.
+    target.push(stringToChunk(encodeHTMLTextNode(children)));
+    return null;
+  }
+  return children;
+}
+
 function pushStartSelect(
   target: Array<Chunk | PrecomputedChunk>,
   props: Object,
@@ -3535,7 +3583,14 @@ export function pushStartInstance(
     case 'span':
     case 'svg':
     case 'path':
+      // Fast track very common tags
+      break;
     case 'a':
+      if (enableFilterEmptyStringAttributesDOM) {
+        return pushStartAnchor(target, props);
+      } else {
+        break;
+      }
     case 'g':
     case 'p':
     case 'li':
diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -640,6 +640,16 @@ describe('ReactDOMComponent', () => {
         expect(node.hasAttribute('href')).toBe(false);
       });
 
+      it('should allow an empty href attribute on anchors', async () => {
+        const container = document.createElement('div');
+        const root = ReactDOMClient.createRoot(container);
+        await act(() => {
+          root.render(<a href="" />);
+        });
+        const node = container.firstChild;
+        expect(node.getAttribute('href')).toBe('');
+      });
+
       it('should allow an empty action attribute', async () => {
         const container = document.createElement('div');
         const root = ReactDOMClient.createRoot(container);
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js
@@ -54,6 +54,38 @@ describe('ReactDOMServerIntegration', () => {
         expect(e.getAttribute('width')).toBe('30');
       });
 
+      itRenders('empty src on img', async render => {
+        const e = await render(
+          <img src="" />,
+          ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
+        );
+        expect(e.getAttribute('src')).toBe(
+          ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
+        );
+      });
+
+      itRenders('empty href on anchor', async render => {
+        const e = await render(<a href="" />);
+        expect(e.getAttribute('href')).toBe('');
+      });
+
+      itRenders('empty href on other tags', async render => {
+        const e = await render(
+          // <link href="" /> would be more sensible.
+          // However, that results in a hydration warning as well.
+          // Our test helpers do not support different error counts for initial
+          // server render and hydration.
+          // The number of errors on the server need to be equal to the number of
+          // errors during hydration.
+          // So we use a <div> instead.
+          <div href="" />,
+          ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? 1 : 0,
+        );
+        expect(e.getAttribute('href')).toBe(
+          ReactFeatureFlags.enableFilterEmptyStringAttributesDOM ? null : '',
+        );
+      });
+
       itRenders('no string prop with true value', async render => {
         const e = await render(<a href={true} />, 1);
         expect(e.hasAttribute('href')).toBe(false);
