fix(react): clear reflected attributes for nullish props

packages/react/src/components/__tests__/utils.spec.ts

@@ -52,6 +52,78 @@ describe('attachProps', () => {
   });
 });
 
+// Fixes https://github.com/ionic-team/ionic-framework/issues/31344
+// jsdom's `HTMLElement.prototype` is missing props Chrome has, so a prop like `role` takes
+// the component branch here and the native branch in a browser.
+describe('attachProps nullish reflected props', () => {
+  it('should not write an attribute for a reflected prop passed as undefined', () => {
+    const div = document.createElement('div');
+
+    utils.attachProps(div, { id: undefined, title: undefined, slot: undefined });
+
+    expect(div.hasAttribute('id')).toBe(false);
+    expect(div.hasAttribute('title')).toBe(false);
+    expect(div.hasAttribute('slot')).toBe(false);
+  });
+
+  it('should not write an attribute for a reflected prop passed as null', () => {
+    const div = document.createElement('div');
+
+    utils.attachProps(div, { id: null, title: null });
+
+    expect(div.hasAttribute('id')).toBe(false);
+    expect(div.hasAttribute('title')).toBe(false);
+  });
+
+  it('should remove the attribute when a reflected prop becomes undefined', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { id: 'real-id' });
+    expect(div.getAttribute('id')).toBe('real-id');
+
+    utils.attachProps(div, { id: undefined }, { id: 'real-id' });
+
+    expect(div.hasAttribute('id')).toBe(false);
+  });
+
+  it('should remove both attribute spellings of a camel cased reflected prop', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { accessKey: 'k' });
+
+    utils.attachProps(div, { accessKey: undefined }, { accessKey: 'k' });
+
+    expect(div.hasAttribute('accesskey')).toBe(false);
+    expect(div.hasAttribute('access-key')).toBe(false);
+  });
+
+  it('should not leave a stringified value for a nullish enumerated prop', () => {
+    const div = document.createElement('div');
+
+    // Assigning `undefined` gives `draggable="false"` and `translate="no"`, which look like real values.
+    utils.attachProps(div, { draggable: undefined, translate: undefined });
+
+    expect(div.hasAttribute('draggable')).toBe(false);
+    expect(div.hasAttribute('translate')).toBe(false);
+  });
+
+  it('should still assign a component prop set to null', () => {
+    const div = document.createElement('div');
+
+    // Passing `spinner={null}` to `ion-loading` means no spinner, while `undefined` gets the mode default.
+    utils.attachProps(div, { spinner: null });
+
+    expect((div as any).spinner).toBe(null);
+  });
+
+  it('should not remove the attribute of a component prop set to null', () => {
+    const div = document.createElement('div');
+    utils.attachProps(div, { spinner: 'bubbles' });
+
+    utils.attachProps(div, { spinner: null }, { spinner: 'bubbles' });
+
+    expect(div.getAttribute('spinner')).toBe('bubbles');
+  });
+});
+
 describe('attachProps boolean attributes', () => {
   it('should strip a stray disabled="false" attribute when the prop is false', () => {
     const div = document.createElement('div');

packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx

@@ -44,3 +44,28 @@ describe('createReactComponent boolean attributes', () => {
     expect(el.hasAttribute('disabled')).toBe(false);
   });
 });
+
+// Fixes https://github.com/ionic-team/ionic-framework/issues/31344
+describe('createReactComponent nullish reflected props', () => {
+  it('should not render an attribute for an unset optional prop', () => {
+    const Wrapper = ({ id }: { id?: string }) => <ReactEl id={id}>x</ReactEl>;
+    const { container } = render(<Wrapper />);
+    const el = container.querySelector('fake-react-el')!;
+
+    expect(el.hasAttribute('id')).toBe(false);
+    expect(el.outerHTML).not.toContain('undefined');
+  });
+
+  it('should remove the attribute when the prop goes back to undefined', () => {
+    const Wrapper = ({ id }: { id?: string }) => <ReactEl id={id}>x</ReactEl>;
+    const { container, rerender } = render(<Wrapper id="real-id" />);
+    const el = container.querySelector('fake-react-el')!;
+    expect(el.getAttribute('id')).toBe('real-id');
+
+    act(() => {
+      rerender(<Wrapper />);
+    });
+
+    expect(el.hasAttribute('id')).toBe(false);
+  });
+});

packages/react/src/components/react-component-lib/utils/attachProps.ts

@@ -24,6 +24,14 @@ const NON_BOOLEAN_FALSE_ATTRIBUTES = new Set(['draggable', 'translate', 'spell-c
 const isStaleFalseBooleanAttribute = (attribute: string) =>
   !attribute.startsWith('aria-') && !attribute.startsWith('data-') && !NON_BOOLEAN_FALSE_ATTRIBUTES.has(attribute);
 
+/**
+ * Assigning `undefined` or `null` to a reflected prop like `id` writes the stringified value, so
+ * the element ends up with `id="undefined"` or `id="null"`. Only native properties are cleaned up
+ * here: a nullish component prop still has to reach the component, and Stencil clears the
+ * attribute for props declared `reflect: true`.
+ */
+const isNativeElementProperty = (name: string) => name in HTMLElement.prototype;
+
 export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {}) => {
   // some test frameworks don't render DOM elements, so we test here to make sure we are dealing with DOM first
   if (node instanceof Element) {
@@ -52,11 +60,18 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {}
           syncEvent(node, eventNameLc, newProps[name]);
         }
       } else {
-        (node as any)[name] = newProps[name];
-        const propType = typeof newProps[name];
+        const value = newProps[name];
+        (node as any)[name] = value;
+        if ((value === undefined || value === null) && isNativeElementProperty(name)) {
+          // Remove both spellings: the property reflects `accesskey`, and dash-casing writes `access-key`.
+          node.removeAttribute(name);
+          node.removeAttribute(camelToDashCase(name));
+          return;
+        }
+        const propType = typeof value;
         if (propType === 'string') {
-          node.setAttribute(camelToDashCase(name), newProps[name]);
-        } else if (newProps[name] === false) {
+          node.setAttribute(camelToDashCase(name), value);
+        } else if (value === false) {
           const attribute = camelToDashCase(name);
           if (isStaleFalseBooleanAttribute(attribute)) {
             node.removeAttribute(attribute);