diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index d5f506ce0e9..86114b82526 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/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'); diff --git a/packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx b/packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx index 55001169f95..72c68ae84ab 100644 --- a/packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx +++ b/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 }) => x; + const { container } = render(); + 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 }) => x; + const { container, rerender } = render(); + const el = container.querySelector('fake-react-el')!; + expect(el.getAttribute('id')).toBe('real-id'); + + act(() => { + rerender(); + }); + + expect(el.hasAttribute('id')).toBe(false); + }); +}); diff --git a/packages/react/src/components/react-component-lib/utils/attachProps.ts b/packages/react/src/components/react-component-lib/utils/attachProps.ts index ade256c2438..9b344897bd4 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/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);