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..6345dc2a095 --- /dev/null +++ b/packages/react/src/components/__tests__/createComponent.spec.tsx @@ -0,0 +1,53 @@ +/** + * `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'; + +// 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; + +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 3714aa2a859..b4361592cc3 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -50,4 +50,59 @@ 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('testprop' in div).toBe(false); + }); + + 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); + }); + + 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 9a1825f54f3..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,5 +1,15 @@ import { camelToDashCase } from './case'; +/** + * 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) { @@ -28,10 +38,45 @@ 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]); + node.setAttribute(camelToDashCase(name), value); } } });