diff --git a/projects/js-packages/charts/AGENTS.md b/projects/js-packages/charts/AGENTS.md index 7c9ddbdd00a1..4c27538cfbb5 100644 --- a/projects/js-packages/charts/AGENTS.md +++ b/projects/js-packages/charts/AGENTS.md @@ -33,6 +33,36 @@ The package is migrating to WordPress UI and Theme as its defaults. When adding `--wpds-*` mappings). Charts reference `--a8c-charts-*` roles with the mapped `var(--wpds-*, )` as the inline fallback; there is no runtime emission yet (that is CHARTS-203). +- **Two consumption paths — this changes what a charts change can break.** + `@wordpress/build` apps (premium-analytics, publicize, podcast, videopress) + consume the Rolldown output in `dist/` and load it as a **WordPress Script + Module** — native browser ESM, where `require` does not exist. Webpack apps + (My Jetpack and friends) resolve source through the `jetpack:src` export + condition instead. A change that only affects `dist/` can therefore break + four packages this one does not import. +- **Never `deps.alwaysBundle` a package that transitively requires an external.** + Pre-bundling is safe only for dependencies that require nothing themselves — + `fast-deep-equal` qualifies, which is why `tsdown.config.ts` still lists it. + Pre-bundle anything that reaches a CommonJS module requiring an external + (`react`, above all) and Rolldown emits a dynamic-`require` shim, because it + cannot rewrite a runtime `require` into a static ESM import. That shim throws + during module evaluation in Script Module consumers, taking down every widget + on the page rather than just the feature that pulled it in. + `tools/assert-no-dynamic-require.ts` fails the build when such a shim reaches + the ESM output; never suppress it. +- **`@wordpress/ui` is external in `dist`, and no build check can prove that is + safe.** Correctness depends on `@wordpress/build` *bundling* `@wordpress/ui` + rather than externalising it to `window.wp.ui`. It used to externalise it, + which is what CHARTS-163 worked around by pre-bundling; it now bundles any + `@wordpress/*` package that declares neither `wpScriptModuleExports` nor + `wpScript`, and `@wordpress/ui` declares `wpScript: false`. If a future + version reverts, `dist/index.js` keeps its clean `import … from + "@wordpress/ui"`, the guard passes, the build passes, and every Script Module + consumer breaks at runtime on `wp.ui` being undefined — the same blast radius + as CHARTS-237, with no build-time signal. Verified against `@wordpress/build` + 0.18.0 (publicize, podcast, videopress) and 0.19.1-next (premium-analytics). + Check a major bump by loading a charts screen in wp-admin, not by trusting a + green build. ## Documentation Workflow diff --git a/projects/js-packages/charts/changelog/charts-247-externalize-wordpress-ui b/projects/js-packages/charts/changelog/charts-247-externalize-wordpress-ui new file mode 100644 index 000000000000..b60d43bbd739 --- /dev/null +++ b/projects/js-packages/charts/changelog/charts-247-externalize-wordpress-ui @@ -0,0 +1,4 @@ +Significance: major +Type: changed + +Zoom: Restore the accessible tooltip on the reset control. `@wordpress/ui` is no longer bundled into the package output, so each consumer's bundler now resolves it. It remains a dependency and resolves from node_modules by default, but a bundler that externalizes `@wordpress/*` to `window.wp.*` must bundle `@wordpress/ui` instead — `window.wp.ui` does not exist. diff --git a/projects/js-packages/charts/src/charts/area-chart/stories/index.docs.mdx b/projects/js-packages/charts/src/charts/area-chart/stories/index.docs.mdx index 604cd28627df..6e744be7c1aa 100644 --- a/projects/js-packages/charts/src/charts/area-chart/stories/index.docs.mdx +++ b/projects/js-packages/charts/src/charts/area-chart/stories/index.docs.mdx @@ -95,7 +95,7 @@ Zoom behaviour: - Only the X axis rescales; the Y axis is unaffected. - A selection rectangle follows the pointer while dragging. - Drags shorter than 6px are ignored, so a click never zooms. -- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space. +- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space. A "Reset zoom" tooltip shows on hover and on keyboard focus, and Escape dismisses it. - Areas are clipped to the plot area whenever `zoomable` is set, keeping the zoom-out animation within the axes. - `zoomable` chains with your own `onPointerDown`/`onPointerMove`/`onPointerUp` handlers rather than replacing them. diff --git a/projects/js-packages/charts/src/charts/line-chart/stories/index.docs.mdx b/projects/js-packages/charts/src/charts/line-chart/stories/index.docs.mdx index 0bafe97cab0c..fd639b38c393 100644 --- a/projects/js-packages/charts/src/charts/line-chart/stories/index.docs.mdx +++ b/projects/js-packages/charts/src/charts/line-chart/stories/index.docs.mdx @@ -527,7 +527,7 @@ Zoom behaviour: - Only the X axis rescales; the Y axis is unaffected. - A selection rectangle follows the pointer while dragging. - Drags shorter than 6px are ignored, so a click never zooms. -- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. +- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space. A "Reset zoom" tooltip shows on hover and on keyboard focus, and Escape dismisses it. - Series are clipped to the plot area while zoomed, so lines never overflow the axes. - `zoomable` chains with your own `onPointerDown`/`onPointerMove`/`onPointerUp` handlers rather than replacing them. diff --git a/projects/js-packages/charts/src/charts/private/x-zoom/test/x-zoom.test.tsx b/projects/js-packages/charts/src/charts/private/x-zoom/test/x-zoom.test.tsx index 8e32db8c7ada..8b19e04b5fcd 100644 --- a/projects/js-packages/charts/src/charts/private/x-zoom/test/x-zoom.test.tsx +++ b/projects/js-packages/charts/src/charts/private/x-zoom/test/x-zoom.test.tsx @@ -1,6 +1,6 @@ -import { act, render, renderHook, screen } from '@testing-library/react'; +import { act, render, renderHook, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { useRef } from 'react'; +import { useCallback, useRef, useState } from 'react'; import { useXZoom, ZoomResetButton } from '../index'; import type { SingleChartRef } from '../../single-chart-context'; import type { EventHandlerParams } from '@visx/xychart'; @@ -147,17 +147,35 @@ const preventDefaultKeydown = ( event: ReactKeyboardEvent< HTMLDivElement > ) => event.preventDefault(); describe( 'ZoomResetButton', () => { - test( 'renders a labelled button with a hover tooltip', () => { + test( 'renders a labelled button without a native title tooltip', () => { const noop = jest.fn(); render( ); const button = screen.getByTestId( 'chart-zoom-reset' ); expect( button.tagName ).toBe( 'BUTTON' ); expect( button ).toHaveClass( 'x-zoom__reset' ); expect( button ).toHaveAccessibleName( 'Reset zoom' ); - // WPDS `IconButton` would supply a tooltip of its own, but it pulls in a - // CommonJS dependency that breaks Script Module consumers (see - // ZoomResetButton). `title` restores the hover hint on plain `Button`. - expect( button ).toHaveAttribute( 'title', 'Reset zoom' ); + // IconButton renders a real tooltip, so the `title` fallback is gone — + // `title` is invisible to keyboard users and cannot be dismissed. + expect( button ).not.toHaveAttribute( 'title' ); + } ); + + test( 'shows a tooltip on keyboard focus', async () => { + const noop = jest.fn(); + render( ); + await userEvent.tab(); + expect( screen.getByTestId( 'chart-zoom-reset' ) ).toHaveFocus(); + // The button's only text is the tooltip's — its own label is an + // `aria-label` attribute, so this cannot match the trigger. + await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible(); + } ); + + test( 'dismisses the tooltip on Escape', async () => { + const noop = jest.fn(); + render( ); + await userEvent.tab(); + await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible(); + await userEvent.keyboard( '{Escape}' ); + await waitFor( () => expect( screen.queryByText( 'Reset zoom' ) ).not.toBeInTheDocument() ); } ); test( 'fires onClick when activated', async () => { @@ -167,6 +185,33 @@ describe( 'ZoomResetButton', () => { expect( onClick ).toHaveBeenCalledTimes( 1 ); } ); + test( 'leaves no orphaned tooltip when activation unmounts the button', async () => { + // Resetting the zoom unmounts this control while its tooltip is open. + // The tooltip renders in a portal outside the container, so a missed + // cleanup would strand it on the page rather than remove it with the + // button. + /** + * Mirrors the host charts: the reset control exists only while zoomed. + * + * @return JSX element or null. + */ + function Host() { + const [ zoomed, setZoomed ] = useState( true ); + const unzoom = useCallback( () => setZoomed( false ), [] ); + return zoomed ? : null; + } + render( ); + await userEvent.tab(); + await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible(); + + await userEvent.keyboard( '{Enter}' ); + + await waitFor( () => + expect( screen.queryByTestId( 'chart-zoom-reset' ) ).not.toBeInTheDocument() + ); + expect( document.body ).not.toHaveTextContent( 'Reset zoom' ); + } ); + test( 'keyboard activation survives the chart wrapper keydown handler', async () => { const onClick = jest.fn(); // Mirrors the chart's grid wrapper, whose keyboard-navigation handler diff --git a/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.module.scss b/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.module.scss index 6ec9eafd5c1e..18285a1581fb 100644 --- a/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.module.scss +++ b/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.module.scss @@ -11,12 +11,12 @@ pointer-events: none; } - // Overlay placement and icon-only geometry — the visual treatment (border, - // hover/focus, sizing) comes from the WPDS Button. The elevation shadow is - // deliberately kept for separation from chart content (CHARTS-237 - // design call, carrying over CHARTS-200's tokenization). `@wordpress/theme` - // 1.0.0 dropped the `--wpds-elevation-*` group, so this role carries the - // former token's spec value directly rather than nesting a `--wpds-*` var. + // The elevation shadow is deliberately kept for separation + // from chart content (CHARTS-237 design call, carrying + // over CHARTS-200's tokenization). `@wordpress/theme` 1.0.0 + // dropped the `--wpds-elevation-*` group, so this role carries + // the former token's spec value directly rather than nesting + // a `--wpds-*` var. &__reset { position: absolute; top: var(--wpds-dimension-gap-sm, 8px); @@ -24,16 +24,6 @@ z-index: 2; box-shadow: var(--a8c-charts-elevation-xs, 0 1px 1px 0 #00000008, 0 1px 2px 0 #00000005, 0 3px 3px 0 #00000005, 0 4px 4px 0 #00000003); - // `Button` is sized for a text label. These are the same three custom - // properties @wordpress/ui's own IconButton composition sets to make a - // square icon-only control; we set them directly because IconButton - // itself cannot be used here — it renders a Base UI tooltip, whose - // CommonJS `use-sync-external-store` dependency makes Rolldown emit a - // dynamic `require()` that throws in Script Module consumers. - --wp-ui-button-aspect-ratio: 1; - --wp-ui-button-padding-inline: 0; - --wp-ui-button-min-width: unset; - // WPDS outline-neutral is transparent-bodied at rest; give this // floating control an opaque body so chart content doesn't show // through it. Scoped to the rest state so the WPDS diff --git a/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.tsx b/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.tsx index dfe68335c62a..4b916051dda7 100644 --- a/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.tsx +++ b/projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.tsx @@ -1,6 +1,6 @@ import { DataContext } from '@visx/xychart'; import { __ } from '@wordpress/i18n'; -import { Button } from '@wordpress/ui'; +import { IconButton } from '@wordpress/ui'; import { useCallback, useContext, useMemo, useState } from 'react'; import styles from './x-zoom.module.scss'; import type { SingleChartRef } from '../single-chart-context'; @@ -172,14 +172,8 @@ export function ZoomClip( { /** * Visible icon-only reset control rendered as an HTML overlay on top of the - * chart container, using the WPDS `Button`. The host should wrap its SVG in a - * `position: relative` container so the button anchors correctly. - * - * `IconButton` would be the natural fit, but it renders a Base UI tooltip whose - * CommonJS `use-sync-external-store` dependency makes Rolldown emit a dynamic - * `require()` into `dist`, which throws on evaluation in WordPress Script - * Module consumers. `Button` gives the same treatment without that dependency; - * the tooltip is replaced by `aria-label` + `title`. + * chart container, using the WPDS `IconButton`. The host should wrap its SVG in + * a `position: relative` container so the button anchors correctly. * * @param props - Props. * @param props.onClick - Click handler. Typically the `reset` from `useXZoom`. @@ -194,44 +188,39 @@ export function ZoomResetButton( { onClick }: { onClick: () => void } ) { event.stopPropagation(); } }, [] ); - const label = __( 'Reset zoom', 'jetpack-charts' ); return ( - + icon={ + + } + /> ); } diff --git a/projects/js-packages/charts/tsdown.config.ts b/projects/js-packages/charts/tsdown.config.ts index 7d7cfeb14247..c3bc346c95e2 100644 --- a/projects/js-packages/charts/tsdown.config.ts +++ b/projects/js-packages/charts/tsdown.config.ts @@ -45,7 +45,7 @@ export default defineConfig( { '.png': 'asset', }, deps: { - alwaysBundle: [ '@wordpress/ui', /^fast-deep-equal/ ], + alwaysBundle: [ /^fast-deep-equal/ ], }, css: { fileName: 'index.css',