diff --git a/packages/react-router/test/base/src/App.tsx b/packages/react-router/test/base/src/App.tsx index cc22a9e158d..09e61e940bd 100644 --- a/packages/react-router/test/base/src/App.tsx +++ b/packages/react-router/test/base/src/App.tsx @@ -70,6 +70,7 @@ import { RouterLinkModifierClick, RouterLinkModifierClickTarget } from './pages/ import { NavigateRootPageA, NavigateRootPageB, NavigateRootPageC } from './pages/navigate-root/NavigateRoot'; import SuspenseOutlet from './pages/suspense-outlet/SuspenseOutlet'; import { PropsUpdateDirect, PropsUpdateRoutesWrapper } from './pages/props-update/PropsUpdate'; +import DisabledButton from './pages/disabled-button/DisabledButton'; setupIonicReact(); @@ -81,6 +82,7 @@ const App: React.FC = () => { } /> + } /> } /> } /> } /> diff --git a/packages/react-router/test/base/src/pages/disabled-button/DisabledButton.tsx b/packages/react-router/test/base/src/pages/disabled-button/DisabledButton.tsx new file mode 100644 index 00000000000..5da5c808974 --- /dev/null +++ b/packages/react-router/test/base/src/pages/disabled-button/DisabledButton.tsx @@ -0,0 +1,45 @@ +import { IonButton, IonContent, IonHeader, IonPage, IonTitle, IonToolbar } from '@ionic/react'; +import React, { useState } from 'react'; + +/** + * FW-7393: `` must not render a `disabled="false"` + * attribute on the host. ion-button is a routing-wrapped component + * (createRoutingComponent), so this validates the fix end-to-end with a real + * component in a real browser. + */ +const DisabledButton: React.FC = () => { + const [toggleDisabled, setToggleDisabled] = useState(false); + + return ( + + + + Disabled Button + + + + + disabled false + + + + disabled true + + + + aria-expanded false + + + + toggle target + + + setToggleDisabled((v) => !v)}> + toggle + + + + ); +}; + +export default DisabledButton; diff --git a/packages/react-router/test/base/tests/e2e/playwright/disabled-button.spec.ts b/packages/react-router/test/base/tests/e2e/playwright/disabled-button.spec.ts new file mode 100644 index 00000000000..ce6d14f82b9 --- /dev/null +++ b/packages/react-router/test/base/tests/e2e/playwright/disabled-button.spec.ts @@ -0,0 +1,53 @@ +import { test, expect } from '@playwright/test'; + +import { ionPageVisible, withTestingMode } from './utils/test-utils'; + +/** + * FW-7393: `` must not leave a `disabled="false"` + * attribute on the host. ion-button is routing-wrapped (createRoutingComponent), + * so this is the real end-to-end check with an actual @ionic/react component. + */ +test.describe('IonButton disabled boolean attribute (FW-7393)', () => { + test.beforeEach(async ({ page }) => { + await page.goto(withTestingMode('/disabled-button')); + await ionPageVisible(page, 'disabled-button'); + }); + + test('disabled={false} should not render a disabled attribute', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/27930', + }); + + const button = page.locator('#btn-false'); + await expect(button).toHaveJSProperty('disabled', false); + await expect(button).not.toHaveAttribute('disabled', /.*/); + // The inner native button must be interactive. + await expect(button.locator('button')).not.toBeDisabled(); + }); + + test('disabled={true} should render a disabled, non-interactive button', async ({ page }) => { + const button = page.locator('#btn-true'); + await expect(button).toHaveJSProperty('disabled', true); + await expect(button).toHaveAttribute('disabled', ''); + await expect(button.locator('button')).toBeDisabled(); + }); + + test('aria-expanded={false} should be preserved (meaningful, not a boolean attribute)', async ({ page }) => { + // ion-button relocates aria-* from the host onto its inner native button, so + // the wrapper must NOT strip aria-expanded="false" (unlike disabled="false"): + // it has to survive on the host long enough to be inherited here. + const nativeButton = page.locator('#btn-aria button'); + await expect(nativeButton).toHaveAttribute('aria-expanded', 'false'); + }); + + test('toggling disabled false -> true should disable the button', async ({ page }) => { + const button = page.locator('#btn-toggle'); + await expect(button).not.toHaveAttribute('disabled', /.*/); + + await page.locator('#btn-do-toggle').click(); + + await expect(button).toHaveAttribute('disabled', ''); + await expect(button).toHaveJSProperty('disabled', true); + }); +}); diff --git a/packages/react/src/components/__tests__/createRoutingComponent.spec.tsx b/packages/react/src/components/__tests__/createRoutingComponent.spec.tsx new file mode 100644 index 00000000000..618f92b0d16 --- /dev/null +++ b/packages/react/src/components/__tests__/createRoutingComponent.spec.tsx @@ -0,0 +1,104 @@ +// Jest can't import Ionic's ESM-only custom elements; stub the values the +// wrapper's util chain reaches. +jest.mock('@ionic/core/components', () => ({ + getPlatforms: () => [], + isPlatform: () => false, + componentOnReady: (_el: HTMLElement, cb: () => void) => cb(), +})); + +import { act, render } from '@testing-library/react'; + +import { createRoutingComponent } from '../createRoutingComponent'; + +/** + * FW-7393: routing-wrapped components (ion-button, ion-card, ion-fab-button, + * ion-item-option, ion-breadcrumb, ion-router-link) go through + * `createRoutingComponent`, which is not covered by the @lit/react runtime that + * fixes the generated components on v9. `disabled={false}` must not leave a + * stray `disabled="false"` on the host, since presence of an HTML boolean + * attribute means "true" to assistive tech. + */ +const RoutingEl = createRoutingComponent('fake-routing-el'); + +describe('createRoutingComponent boolean attributes (FW-7393)', () => { + it('should not leave a disabled="false" attribute when disabled={false}', () => { + const { container } = render(x); + const el = container.querySelector('fake-routing-el')!; + + expect(el.hasAttribute('disabled')).toBe(false); + }); + + it('should preserve aria-* attributes set to false (meaningful, not boolean attributes)', () => { + const { container } = render(x); + const el = container.querySelector('fake-routing-el')!; + + // aria-expanded="false" means "collapsed", distinct from the attribute being absent. + expect(el.getAttribute('aria-expanded')).toBe('false'); + }); + + it('should preserve enumerated attributes set to false (e.g. draggable)', () => { + const { container } = render(x); + const el = container.querySelector('fake-routing-el')!; + + // draggable="false" explicitly disables dragging, distinct from absence. + expect(el.getAttribute('draggable')).toBe('false'); + }); + + it('should preserve camelCased enumerated attributes set to false (e.g. spellCheck)', () => { + const { container } = render(x); + const el = container.querySelector('fake-routing-el')!; + + // The wrapper dash-cases React prop names, so spellCheck renders as + // spell-check; spell-check="false" is meaningful and must survive. + expect(el.getAttribute('spell-check')).toBe('false'); + }); + + it('should keep the disabled attribute when disabled={true}', () => { + const { container } = render(x); + const el = container.querySelector('fake-routing-el')!; + + expect(el.hasAttribute('disabled')).toBe(true); + }); + + it('should disable the element when toggling disabled false -> true', () => { + const { container, rerender } = render(x); + const el = container.querySelector('fake-routing-el')!; + expect(el.hasAttribute('disabled')).toBe(false); + + act(() => { + rerender(x); + }); + + expect(el.hasAttribute('disabled')).toBe(true); + }); + + it('should drop the attribute when toggling disabled true -> false', () => { + const { container, rerender } = render(x); + const el = container.querySelector('fake-routing-el')!; + + act(() => { + rerender(x); + }); + + expect(el.hasAttribute('disabled')).toBe(false); + }); + + it('should not re-add disabled="false" after an unrelated re-render', () => { + const { container, rerender } = render( + + x + + ); + const el = container.querySelector('fake-routing-el')!; + + act(() => { + rerender( + + x + + ); + }); + + expect(el.hasAttribute('disabled')).toBe(false); + }); +}); diff --git a/packages/react/src/components/__tests__/utils.spec.ts b/packages/react/src/components/__tests__/utils.spec.ts index 3714aa2a859..76c3daa682e 100644 --- a/packages/react/src/components/__tests__/utils.spec.ts +++ b/packages/react/src/components/__tests__/utils.spec.ts @@ -51,3 +51,41 @@ describe('attachProps', () => { expect(Object.keys((div as any).__events)).toEqual(['ionClick']); }); }); + +describe('attachProps boolean attributes (FW-7393)', () => { + it('should strip a stray disabled="false" attribute when the prop is false', () => { + const div = document.createElement('div'); + div.setAttribute('disabled', 'false'); + + utils.attachProps(div, { disabled: false }); + + expect(div.hasAttribute('disabled')).toBe(false); + }); + + it('should preserve aria-* attributes set to false', () => { + const div = document.createElement('div'); + div.setAttribute('aria-expanded', 'false'); + + utils.attachProps(div, { 'aria-expanded': false }); + + expect(div.getAttribute('aria-expanded')).toBe('false'); + }); + + it('should preserve data-* attributes set to false', () => { + const div = document.createElement('div'); + div.setAttribute('data-active', 'false'); + + utils.attachProps(div, { 'data-active': false }); + + expect(div.getAttribute('data-active')).toBe('false'); + }); + + it('should preserve enumerated attributes set to false (e.g. draggable)', () => { + const div = document.createElement('div'); + div.setAttribute('draggable', 'false'); + + utils.attachProps(div, { draggable: false }); + + expect(div.getAttribute('draggable')).toBe('false'); + }); +}); 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 new file mode 100644 index 00000000000..ee328cea816 --- /dev/null +++ b/packages/react/src/components/react-component-lib/__tests__/createComponent.spec.tsx @@ -0,0 +1,46 @@ +import { act, render } from '@testing-library/react'; + +import { createReactComponent } from '../createComponent'; + +/** + * FW-7393: components built with createReactComponent (e.g. IonBackButton, + * IonTabButton via inner-proxies) render attributes directly and sync props + * through attachProps, so they must get the same `disabled="false"` stripping as + * routing-wrapped components. Presence of an HTML boolean attribute means "true" + * to assistive tech, so a false-valued boolean must not leave a stray attribute. + */ +const ReactEl = createReactComponent('fake-react-el'); + +describe('createReactComponent boolean attributes (FW-7393)', () => { + it('should not leave a disabled="false" attribute when disabled={false}', () => { + const { container } = render(x); + const el = container.querySelector('fake-react-el')!; + + expect(el.hasAttribute('disabled')).toBe(false); + }); + + it('should preserve aria-* attributes set to false', () => { + const { container } = render(x); + const el = container.querySelector('fake-react-el')!; + + expect(el.getAttribute('aria-expanded')).toBe('false'); + }); + + it('should keep the disabled attribute when disabled={true}', () => { + const { container } = render(x); + const el = container.querySelector('fake-react-el')!; + + expect(el.hasAttribute('disabled')).toBe(true); + }); + + it('should drop the attribute when toggling disabled true -> false', () => { + const { container, rerender } = render(x); + const el = container.querySelector('fake-react-el')!; + + act(() => { + rerender(x); + }); + + expect(el.hasAttribute('disabled')).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 9a1825f54f3..809d0a1b3c5 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,25 @@ import { camelToDashCase } from './case'; +// Enumerated attributes where the literal string "false" is meaningful and +// differs from the attribute being absent, so they must not be stripped like +// HTML boolean attributes. These are the dash-cased attribute names produced by +// camelToDashCase (e.g. the `spellCheck` prop renders as `spell-check`), so the +// entries must match that form. aria-* and data-* are handled by prefix below. +const NON_BOOLEAN_FALSE_ATTRIBUTES = new Set(['draggable', 'translate', 'spell-check', 'content-editable']); + +/** + * React serializes a boolean prop set to `false` (e.g. `disabled={false}`) as + * the string attribute `disabled="false"`. For HTML boolean attributes the mere + * presence means "true", so assistive tech treats the element as + * disabled/readonly even though Ionic renders it as interactive. The @lit/react + * runtime fixes this for the generated components on v9, but the hand-rolled + * wrappers (createReactComponent, createRoutingComponent, ...) render attributes + * directly and sync props through attachProps, so we strip the stray attribute + * here after the property has been assigned. See FW-7393. + */ +const isStaleFalseBooleanAttribute = (attribute: string) => + !attribute.startsWith('aria-') && !attribute.startsWith('data-') && !NON_BOOLEAN_FALSE_ATTRIBUTES.has(attribute); + 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) { @@ -32,6 +52,11 @@ export const attachProps = (node: HTMLElement, newProps: any, oldProps: any = {} const propType = typeof newProps[name]; if (propType === 'string') { node.setAttribute(camelToDashCase(name), newProps[name]); + } else if (newProps[name] === false) { + const attribute = camelToDashCase(name); + if (isStaleFalseBooleanAttribute(attribute)) { + node.removeAttribute(attribute); + } } } });