From bc47872933d5ff4bce259cc707cbe2d9f82b58b2 Mon Sep 17 00:00:00 2001 From: Patrick Kenny Date: Tue, 11 Aug 2026 17:00:42 +0900 Subject: [PATCH 1/5] attachProps should drop undefined; assistance by opus --- .../src/components/__tests__/utils.spec.ts | 18 +++++++++++++ .../react-component-lib/utils/attachProps.ts | 26 ++++++++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index 3714aa2a859..3a7c67b762f 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -50,4 +50,22 @@ describe('attachProps', () => { expect(div).toHaveStyle(`display: block;`); expect(Object.keys((div as any).__events)).toEqual(['ionClick']); }); + + it('should not write undefined props to a dom node', () => { + var div = document.createElement('div'); + utils.attachProps(div, { id: undefined, title: undefined, testprop: undefined }); + + expect(div.hasAttribute('id')).toEqual(false); + expect(div.hasAttribute('title')).toEqual(false); + expect((div as any).testprop).toEqual(undefined); + }); + + it('should clear a prop that no longer has a value', () => { + var 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')).toEqual(false); + expect((div as any).testprop).toEqual(undefined); + }); }); 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 9a1825f54f3..ec705103ce0 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/packages/react/src/components/react-component-lib/utils/attachProps.ts @@ -28,10 +28,30 @@ 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]; + if (value === undefined) { + /** + * An undefined prop must never be assigned to the element. 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"`. `render()` already omits + * undefined props for the same reason. + * + * A prop that had a value and no longer does is a removal, so clear + * it the way React clears a removed attribute: reset the property + * first, for props with no attribute to mirror, then drop the + * attribute that a reflected property left behind. + */ + if (oldProps[name] !== undefined) { + (node as any)[name] = undefined; + node.removeAttribute(camelToDashCase(name)); + } + return; + } + (node as any)[name] = value; + const propType = typeof value; if (propType === 'string') { - node.setAttribute(camelToDashCase(name), newProps[name]); + node.setAttribute(camelToDashCase(name), value); } } }); From 2c2e1a277dcadf27a8298c894069ca24ef41d78f Mon Sep 17 00:00:00 2001 From: ptmkenny <1451472+ptmkenny@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:01:23 +0900 Subject: [PATCH 2/5] Update packages/react/src/components/__tests__/utils.spec.ts Co-authored-by: Shane --- packages/react/src/components/__tests__/utils.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index 3a7c67b762f..05de56a0463 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -57,7 +57,7 @@ describe('attachProps', () => { expect(div.hasAttribute('id')).toEqual(false); expect(div.hasAttribute('title')).toEqual(false); - expect((div as any).testprop).toEqual(undefined); + expect('testprop' in div).toBe(false); }); it('should clear a prop that no longer has a value', () => { From f7776478423f04c398bd1d3f4b1b0c0c1aa9eb73 Mon Sep 17 00:00:00 2001 From: ptmkenny <1451472+ptmkenny@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:01:38 +0900 Subject: [PATCH 3/5] Update packages/react/src/components/react-component-lib/utils/attachProps.ts Co-authored-by: Shane --- .../react-component-lib/utils/attachProps.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) 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 ec705103ce0..9e233266489 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/packages/react/src/components/react-component-lib/utils/attachProps.ts @@ -31,16 +31,14 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {} const value = newProps[name]; if (value === undefined) { /** - * An undefined prop must never be assigned to the element. 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"`. `render()` already omits - * undefined props for the same reason. + * 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. * - * A prop that had a value and no longer does is a removal, so clear - * it the way React clears a removed attribute: reset the property - * first, for props with no attribute to mirror, then drop the - * attribute that a reflected property left behind. + * A prop that had a value and no longer does is a removal: reset the + * property, which covers props with no attribute to mirror, then drop + * the attribute a reflected property left behind. */ if (oldProps[name] !== undefined) { (node as any)[name] = undefined; From 604d64ebb6c41549df13caba3667d457562b57aa Mon Sep 17 00:00:00 2001 From: Patrick Kenny Date: Thu, 13 Aug 2026 00:41:25 +0900 Subject: [PATCH 4/5] handle null and improve tests --- .../__tests__/createComponent.spec.tsx | 51 ++++++++++++++++++ .../src/components/__tests__/utils.spec.ts | 37 +++++++++++++ .../react-component-lib/utils/attachProps.ts | 53 ++++++++++++++++--- 3 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 packages/react/src/components/__tests__/createComponent.spec.tsx diff --git a/packages/react/src/components/__tests__/createComponent.spec.tsx b/packages/react/src/components/__tests__/createComponent.spec.tsx new file mode 100644 index 00000000000..a3db0cc27a9 --- /dev/null +++ b/packages/react/src/components/__tests__/createComponent.spec.tsx @@ -0,0 +1,51 @@ +import { render } from '@testing-library/react'; + +import { createReactComponent } from '../react-component-lib/createComponent'; + +/** + * `createReactComponent` pulls nothing from `@ionic/core`, so the generated + * wrapper can be driven directly. The bug these tests cover only appears at this + * level: `render()` omits an undefined prop, so React emits no attribute, and + * `componentDidUpdate` then writes it back through `attachProps`. + */ +const IonToggle = createReactComponent('ion-toggle') as any; + +const getToggle = () => document.querySelector('ion-toggle') as HTMLElement; + +afterEach(() => { + document.body.innerHTML = ''; +}); + +describe('createReactComponent: nullish props', () => { + it('should not render an attribute for a prop passed as undefined', () => { + render(); + + expect(getToggle().hasAttribute('id')).toEqual(false); + expect(getToggle().hasAttribute('title')).toEqual(false); + }); + + it('should not render an attribute for a prop passed as null', () => { + render(); + + expect(getToggle().hasAttribute('id')).toEqual(false); + expect(getToggle().hasAttribute('title')).toEqual(false); + }); + + it('should remove the attribute when a prop becomes undefined', () => { + const { rerender } = render(); + expect(getToggle().getAttribute('id')).toEqual('my-id'); + + rerender(); + + expect(getToggle().hasAttribute('id')).toEqual(false); + }); + + it('should remove the attribute when a prop becomes null', () => { + const { rerender } = render(); + expect(getToggle().getAttribute('id')).toEqual('my-id'); + + rerender(); + + expect(getToggle().hasAttribute('id')).toEqual(false); + }); +}); diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index 05de56a0463..b4361592cc3 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -68,4 +68,41 @@ describe('attachProps', () => { expect(div.hasAttribute('id')).toEqual(false); expect((div as any).testprop).toEqual(undefined); }); + + it('should not write null native props to a dom node', () => { + var div = document.createElement('div'); + utils.attachProps(div, { id: null, title: null, slot: null }); + + expect(div.hasAttribute('id')).toEqual(false); + expect(div.hasAttribute('title')).toEqual(false); + expect(div.hasAttribute('slot')).toEqual(false); + }); + + it('should clear a native prop set to null', () => { + var div = document.createElement('div'); + utils.attachProps(div, { id: 'my-id' }); + utils.attachProps(div, { id: null }, { id: 'my-id' }); + + expect(div.hasAttribute('id')).toEqual(false); + }); + + it('should treat null as a value for a prop the element does not natively have', () => { + var div = document.createElement('div'); + utils.attachProps(div, { value: 'my-value' }); + utils.attachProps(div, { value: null }, { value: 'my-value' }); + + expect((div as any).value).toEqual(null); + }); + + it('should clear both attribute spellings of a camel cased native prop', () => { + var 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')).toEqual(false); + expect(div.hasAttribute('access-key')).toEqual(false); + expect(div.hasAttribute('tabindex')).toEqual(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 9e233266489..d72ee68993b 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/packages/react/src/components/react-component-lib/utils/attachProps.ts @@ -1,5 +1,24 @@ import { camelToDashCase } from './case'; +/** + * A prop name that already exists on a plain element is a native property: it + * mirrors an attribute the element owns, and assigning to it stringifies + * (`node.id = undefined` leaves `id="undefined"`, `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. + */ +let nativePropertyProbe: HTMLElement | undefined; +const isNativeElementProperty = (name: string) => { + if (typeof document === 'undefined') { + return false; + } + if (nativePropertyProbe === undefined) { + nativePropertyProbe = document.createElement('div'); + } + return name in nativePropertyProbe; +}; + 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) { @@ -29,20 +48,38 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {} } } else { const value = newProps[name]; - if (value === undefined) { + 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. + * undefined value. `null` stringifies the same way, but only native + * properties are treated as empty here: a component prop may accept + * `null` as a value. * - * A prop that had a value and no longer does is a removal: reset the - * property, which covers props with no attribute to mirror, then drop - * the attribute a reflected property left behind. + * A prop that had a value and no longer does is a removal. For a + * native property that means dropping the attribute, never assigning, + * since assigning coerces again (`node.tabIndex = undefined` leaves + * `tabindex="0"`). Both spellings have to go: the one the property + * reflects to (`accesskey`) and the dash-cased one `render()` emits + * (`access-key`). Other props are reset on the property, which covers + * props with no attribute to mirror, then have the attribute the + * string branch set removed. */ - if (oldProps[name] !== undefined) { - (node as any)[name] = undefined; - node.removeAttribute(camelToDashCase(name)); + 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; } From 421588b7dc93eb91dcc253a3e763de30200a90de Mon Sep 17 00:00:00 2001 From: Patrick Kenny Date: Thu, 13 Aug 2026 00:45:22 +0900 Subject: [PATCH 5/5] rewrite for consistency --- .../__tests__/createComponent.spec.tsx | 14 ++++--- .../react-component-lib/utils/attachProps.ts | 40 +++++++------------ 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/packages/react/src/components/__tests__/createComponent.spec.tsx b/packages/react/src/components/__tests__/createComponent.spec.tsx index a3db0cc27a9..6345dc2a095 100644 --- a/packages/react/src/components/__tests__/createComponent.spec.tsx +++ b/packages/react/src/components/__tests__/createComponent.spec.tsx @@ -1,13 +1,15 @@ +/** + * `createReactComponent` reaches nothing in `@ionic/core`, so the generated + * wrapper can be driven directly, with no module to mock. These cases only fail + * at the wrapper level: `render()` omits a nullish prop, so React emits no + * attribute, and `componentDidUpdate` then writes one back through + * `attachProps`. + */ import { render } from '@testing-library/react'; import { createReactComponent } from '../react-component-lib/createComponent'; -/** - * `createReactComponent` pulls nothing from `@ionic/core`, so the generated - * wrapper can be driven directly. The bug these tests cover only appears at this - * level: `render()` omits an undefined prop, so React emits no attribute, and - * `componentDidUpdate` then writes it back through `attachProps`. - */ +// Mirror how IonToggle is generated: a plain wrapper with no context or delegate. const IonToggle = createReactComponent('ion-toggle') as any; const getToggle = () => document.querySelector('ion-toggle') as HTMLElement; 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 d72ee68993b..a7868fbca8a 100644 --- a/packages/react/src/components/react-component-lib/utils/attachProps.ts +++ b/packages/react/src/components/react-component-lib/utils/attachProps.ts @@ -1,23 +1,14 @@ import { camelToDashCase } from './case'; /** - * A prop name that already exists on a plain element is a native property: it - * mirrors an attribute the element owns, and assigning to it stringifies - * (`node.id = undefined` leaves `id="undefined"`, `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 + * 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. */ -let nativePropertyProbe: HTMLElement | undefined; -const isNativeElementProperty = (name: string) => { - if (typeof document === 'undefined') { - return false; - } - if (nativePropertyProbe === undefined) { - nativePropertyProbe = document.createElement('div'); - } - return name in nativePropertyProbe; -}; +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 @@ -54,18 +45,17 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {} * 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 native - * properties are treated as empty here: a component prop may accept + * 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. For a - * native property that means dropping the attribute, never assigning, - * since assigning coerces again (`node.tabIndex = undefined` leaves - * `tabindex="0"`). Both spellings have to go: the one the property - * reflects to (`accesskey`) and the dash-cased one `render()` emits - * (`access-key`). Other props are reset on the property, which covers - * props with no attribute to mirror, then have the attribute the - * string branch set removed. + * 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) {