diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index d5f506ce0e9..115f5be1c58 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -52,6 +52,69 @@ describe('attachProps', () => { }); }); +describe('attachProps nullish props', () => { + it('should not write undefined props to a dom node', () => { + const div = document.createElement('div'); + + utils.attachProps(div, { id: undefined, title: undefined, testprop: undefined }); + + expect(div.hasAttribute('id')).toBe(false); + expect(div.hasAttribute('title')).toBe(false); + expect('testprop' in div).toBe(false); + }); + + it('should not write null native props to a dom node', () => { + const div = document.createElement('div'); + + utils.attachProps(div, { id: null, title: null, slot: null }); + + expect(div.hasAttribute('id')).toBe(false); + expect(div.hasAttribute('title')).toBe(false); + expect(div.hasAttribute('slot')).toBe(false); + }); + + it('should clear a prop that no longer has a value', () => { + const div = document.createElement('div'); + utils.attachProps(div, { id: 'my-id', testprop: ['red'] }); + + utils.attachProps(div, { id: undefined, testprop: undefined }, { id: 'my-id', testprop: ['red'] }); + + expect(div.hasAttribute('id')).toBe(false); + expect((div as any).testprop).toBe(undefined); + }); + + it('should clear a native prop set to null', () => { + const div = document.createElement('div'); + utils.attachProps(div, { id: 'my-id' }); + + utils.attachProps(div, { id: null }, { id: 'my-id' }); + + expect(div.hasAttribute('id')).toBe(false); + }); + + it('should treat null as a value for a prop the element does not natively have', () => { + const div = document.createElement('div'); + utils.attachProps(div, { value: 'my-value' }); + + utils.attachProps(div, { value: null }, { value: 'my-value' }); + + expect((div as any).value).toBe(null); + }); + + it('should clear both attribute spellings of a camel cased native prop', () => { + const div = document.createElement('div'); + // The property write reflects to `accesskey` while the dash-cased write + // adds `access-key`, so both attributes end up on the element. + utils.attachProps(div, { accessKey: 'k', tabIndex: 2 }); + + utils.attachProps(div, { accessKey: undefined, tabIndex: undefined }, { accessKey: 'k', tabIndex: 2 }); + + expect(div.hasAttribute('accesskey')).toBe(false); + expect(div.hasAttribute('access-key')).toBe(false); + expect(div.hasAttribute('tabindex')).toBe(false); + }); +}); + 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..92bf2f90a59 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,56 @@ describe('createReactComponent boolean attributes', () => { expect(el.hasAttribute('disabled')).toBe(false); }); }); + +/** + * These only fail at the wrapper level: render() omits a nullish prop, so React + * emits no attribute, and componentDidUpdate then writes one back through + * attachProps. A direct attachProps call cannot see that interaction. + */ +describe('createReactComponent nullish props', () => { + it('should not render an attribute for a prop passed as undefined', () => { + const { container } = render( + + x + + ); + const el = container.querySelector('fake-react-el')!; + + expect(el.hasAttribute('id')).toBe(false); + expect(el.hasAttribute('title')).toBe(false); + }); + + it('should not render an attribute for a prop passed as null', () => { + const { container } = render( + + x + + ); + const el = container.querySelector('fake-react-el')!; + + expect(el.hasAttribute('id')).toBe(false); + expect(el.hasAttribute('title')).toBe(false); + }); + + it('should drop the attribute when a prop becomes undefined', () => { + const { container, rerender } = render(x); + const el = container.querySelector('fake-react-el')!; + + act(() => { + rerender(x); + }); + + expect(el.hasAttribute('id')).toBe(false); + }); + + it('should drop the attribute when a prop becomes null', () => { + const { container, rerender } = render(x); + const el = container.querySelector('fake-react-el')!; + + act(() => { + rerender(x); + }); + + 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 0fe56a4a412..651844b7055 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/packages/react/src/components/react-component-lib/utils/attachProps.ts @@ -25,6 +25,16 @@ 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); +/** + * A prop that every element already has is a native property: it mirrors an + * attribute the element owns, and assigning to it stringifies the value, so + * `node.id = undefined` leaves `id="undefined"` and `node.tabIndex = undefined` + * leaves `tabindex="0"`. Anything else is a component prop, where `null` can be + * a real value (`ion-input` declares `value?: string | number | null`), so it + * must still be assigned. + */ +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) { @@ -53,11 +63,46 @@ 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]; + const isNativeProperty = isNativeElementProperty(name); + if (value === undefined || (value === null && isNativeProperty)) { + /** + * Reflected properties such as `id`, `title` and `slot` stringify + * whatever they are given, so `node.id = undefined` leaves the element + * with the literal attribute `id="undefined"`. Never assign an + * undefined value. `null` stringifies the same way, but only a native + * property is treated as empty here, since a component prop may take + * `null` as a value. + * + * A prop that had a value and no longer does is a removal. A native + * property is cleared by dropping its attributes rather than by + * assigning, which would only coerce again, and it can carry two: the + * one it reflects to (`accesskey`) and the dash-cased one `render()` + * emits (`access-key`). Any other prop resets the property, which + * covers props with no attribute to mirror, then drops the attribute + * the string branch left behind. + */ + const oldValue = oldProps[name]; + if (oldValue !== undefined && oldValue !== null) { + const dashCasedName = camelToDashCase(name); + if (isNativeProperty) { + const reflectedName = name.toLowerCase(); + node.removeAttribute(reflectedName); + if (dashCasedName !== reflectedName) { + node.removeAttribute(dashCasedName); + } + } else { + (node as any)[name] = undefined; + node.removeAttribute(dashCasedName); + } + } + return; + } + (node as any)[name] = value; + 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);