diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a0689881ff9..7fcd6003f4e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## 10.1.6 + +- Manager: Do not display non-existing shortcuts in the settings page - [#32711](https://github.com/storybookjs/storybook/pull/32711), thanks @DKER2! +- Preview: Enforce inert body if manager is focus-trapped - [#33186](https://github.com/storybookjs/storybook/pull/33186), thanks @Sidnioulz! +- Telemetry: Await pending operations in getLastEvents to prevent race conditions - [#33285](https://github.com/storybookjs/storybook/pull/33285), thanks @valentinpalkovic! +- UI: Fix keyboard navigation bug for "reset" option in `Select` - [#33268](https://github.com/storybookjs/storybook/pull/33268), thanks @Sidnioulz! + ## 10.1.5 - Addon-Vitest: Isolate error reasons during postinstall - [#33295](https://github.com/storybookjs/storybook/pull/33295), thanks @valentinpalkovic! diff --git a/code/core/src/components/components/Modal/Modal.tsx b/code/core/src/components/components/Modal/Modal.tsx index 80b839bbfae2..b2f734ae30db 100644 --- a/code/core/src/components/components/Modal/Modal.tsx +++ b/code/core/src/components/components/Modal/Modal.tsx @@ -4,7 +4,12 @@ import { deprecate } from 'storybook/internal/client-logger'; import type { DecoratorFunction } from 'storybook/internal/csf'; import { FocusScope } from '@react-aria/focus'; -import { Overlay, UNSAFE_PortalProvider, useModalOverlay } from '@react-aria/overlays'; +import { + Overlay, + UNSAFE_PortalProvider, + ariaHideOutside, + useModalOverlay, +} from '@react-aria/overlays'; import { mergeProps } from '@react-aria/utils'; import { useOverlayTriggerState } from '@react-stately/overlays'; import type { KeyboardEvent as RAKeyboardEvent } from '@react-types/shared'; @@ -169,6 +174,12 @@ function BaseModal({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [isMounted]); + useEffect(() => { + if (isMounted && (open || defaultOpen) && overlayRef.current) { + return ariaHideOutside([overlayRef.current], { shouldUseInert: true }); + } + }, [isMounted, open, defaultOpen, overlayRef]); + if (!isMounted || status === 'exited' || status === 'unmounted') { return null; } diff --git a/code/core/src/components/components/Select/Select.stories.tsx b/code/core/src/components/components/Select/Select.stories.tsx index 18b232b3a7c5..0cf7721783a3 100644 --- a/code/core/src/components/components/Select/Select.stories.tsx +++ b/code/core/src/components/components/Select/Select.stories.tsx @@ -429,6 +429,47 @@ export const KeyboardSelectionMultiSS = meta.story({ play: kbMultiSelectionTest(' ', ' '), }); +export const FullArrowNavigation = meta.story({ + play: async ({ canvas, step }) => { + const selectButton = await canvas.findByRole('button'); + await step('Open select', async () => { + selectButton.focus(); + await userEvent.keyboard('{ArrowDown}'); + expect(selectButton).toHaveTextContent('Tadpole'); + }); + + await step('Navigate to option 2 with ArrowDown', async () => { + await userEvent.keyboard('{ArrowDown}'); + expect(selectButton).toHaveTextContent('Pollywog'); + }); + + await step('Navigate to option 3 with ArrowDown', async () => { + await userEvent.keyboard('{ArrowDown}'); + expect(selectButton).toHaveTextContent('Frog'); + }); + + await step('Loop back to option 1 with ArrowDown', async () => { + await userEvent.keyboard('{ArrowDown}'); + expect(selectButton).toHaveTextContent('Tadpole'); + }); + + await step('Navigate backwards with ArrowUp', async () => { + await userEvent.keyboard('{ArrowUp}'); + expect(selectButton).toHaveTextContent('Frog'); + }); + + await step('Navigate backwards with ArrowUp', async () => { + await userEvent.keyboard('{ArrowUp}'); + expect(selectButton).toHaveTextContent('Pollywog'); + }); + + await step('Navigate back to option 1 with ArrowUp', async () => { + await userEvent.keyboard('{ArrowUp}'); + expect(selectButton).toHaveTextContent('Tadpole'); + }); + }, +}); + export const MouseOpenNoAutoselect = meta.story({ name: 'AutoSelect - nothing selected on Mouse open (single)', play: async ({ canvas, args, step }) => { diff --git a/code/core/src/components/components/Select/Select.tsx b/code/core/src/components/components/Select/Select.tsx index 961e2990e9e6..119f2115d3f1 100644 --- a/code/core/src/components/components/Select/Select.tsx +++ b/code/core/src/components/components/Select/Select.tsx @@ -4,7 +4,7 @@ import React, { forwardRef, useCallback, useEffect, useMemo, useRef, useState } import { RefreshIcon } from '@storybook/icons'; import { useInteractOutside } from '@react-aria/interactions'; -import { Overlay, useOverlay, useOverlayPosition } from '@react-aria/overlays'; +import { Overlay, ariaHideOutside, useOverlay, useOverlayPosition } from '@react-aria/overlays'; import { useObjectRef } from '@react-aria/utils'; import { useOverlayTriggerState } from '@react-stately/overlays'; import { darken, transparentize } from 'polished'; @@ -142,6 +142,12 @@ const MinimalistPopover: FC<{ onInteractOutside: handleClose, }); + useEffect(() => { + if (popoverRef.current) { + return ariaHideOutside([popoverRef.current], { shouldUseInert: true }); + } + }, []); + const { overlayProps: positionProps } = useOverlayPosition({ targetRef: triggerRef, overlayRef: popoverRef, @@ -325,8 +331,10 @@ export const Select = forwardRef( return; } - const currentIndex = options.findIndex( - (option) => externalToValue(option.value) === activeOption.value + const currentIndex = options.findIndex((option) => + activeOption.type === 'reset' + ? 'type' in option && option.type === 'reset' + : externalToValue(option.value) === activeOption.value ); const nextIndex = currentIndex + step; @@ -349,8 +357,11 @@ export const Select = forwardRef( setActiveOption(options[Math.max(0, options.length - step)]); return; } - const currentIndex = options.findIndex( - (option) => externalToValue(option.value) === activeOption.value + + const currentIndex = options.findIndex((option) => + activeOption.type === 'reset' + ? 'type' in option && option.type === 'reset' + : externalToValue(option.value) === activeOption.value ); const nextIndex = currentIndex - step; diff --git a/code/core/src/core-events/index.ts b/code/core/src/core-events/index.ts index f2acd7739990..bcd990b30aab 100644 --- a/code/core/src/core-events/index.ts +++ b/code/core/src/core-events/index.ts @@ -91,6 +91,8 @@ enum events { // Open a file in the code editor OPEN_IN_EDITOR_REQUEST = 'openInEditorRequest', OPEN_IN_EDITOR_RESPONSE = 'openInEditorResponse', + // Emitted when the manager UI sets up a focus trap + MANAGER_INERT_ATTRIBUTE_CHANGED = 'managerInertAttributeChanged', } // Enables: `import Events from ...` @@ -159,6 +161,7 @@ export const { ARGTYPES_INFO_RESPONSE, OPEN_IN_EDITOR_REQUEST, OPEN_IN_EDITOR_RESPONSE, + MANAGER_INERT_ATTRIBUTE_CHANGED, } = events; export * from './data/create-new-story'; diff --git a/code/core/src/manager/App.tsx b/code/core/src/manager/App.tsx index 962fe6309b2b..cecbc87f4809 100644 --- a/code/core/src/manager/App.tsx +++ b/code/core/src/manager/App.tsx @@ -1,6 +1,7 @@ import type { ComponentProps } from 'react'; import React, { useEffect } from 'react'; +import Events from 'storybook/internal/core-events'; import type { Addon_PageType } from 'storybook/internal/types'; import { addons } from 'storybook/manager-api'; @@ -22,11 +23,42 @@ type Props = { export const App = ({ managerLayoutState, setManagerLayoutState, pages, hasTab }: Props) => { const { setMobileAboutOpen } = useLayout(); + /** + * Lets us tell the UI whether or not keyboard shortcuts are enabled, in places where it's not + * convenient to load the addons singleton to figure it out. + */ const { enableShortcuts = true } = addons.getConfig(); useEffect(() => { document.body.setAttribute('data-shortcuts-enabled', enableShortcuts ? 'true' : 'false'); }, [enableShortcuts]); + /** + * Detects when our component library has enabled a focus trap. By convention, react-aria sets the + * document root to `inert` when a focus trap is enabled. We observe that attribute and inform the + * preview iframe when to respect the focus trap, via a channel event. This is necessary because + * inert is no longer propagated into iframes as per https://github.com/whatwg/html/issues/7605, + * and the replacement permission policy is not yet widely available + * (https://github.com/w3c/webappsec-permissions-policy/issues/273). + */ + useEffect(() => { + const rootElement = document.getElementById('root'); + if (!rootElement) { + return; + } + + const observer = new MutationObserver(() => { + const hasInert = rootElement.hasAttribute('inert'); + addons.getChannel().emit(Events.MANAGER_INERT_ATTRIBUTE_CHANGED, hasInert); + }); + + observer.observe(rootElement, { + attributes: true, + attributeFilter: ['inert'], + }); + + return () => observer.disconnect(); + }, []); + return ( <> diff --git a/code/core/src/manager/globals/exports.ts b/code/core/src/manager/globals/exports.ts index 4f855b346150..4a66831516fc 100644 --- a/code/core/src/manager/globals/exports.ts +++ b/code/core/src/manager/globals/exports.ts @@ -587,6 +587,7 @@ export default { 'FORCE_REMOUNT', 'FORCE_RE_RENDER', 'GLOBALS_UPDATED', + 'MANAGER_INERT_ATTRIBUTE_CHANGED', 'NAVIGATE_URL', 'OPEN_IN_EDITOR_REQUEST', 'OPEN_IN_EDITOR_RESPONSE', diff --git a/code/core/src/manager/settings/shortcuts.tsx b/code/core/src/manager/settings/shortcuts.tsx index 33a29e239ba7..9713715f3155 100644 --- a/code/core/src/manager/settings/shortcuts.tsx +++ b/code/core/src/manager/settings/shortcuts.tsx @@ -287,8 +287,14 @@ class ShortcutsScreen extends Component { const { shortcutKeys, addonsShortcutLabels } = this.state; - // @ts-expect-error (non strict) - const arr = Object.entries(shortcutKeys).map(([feature, { shortcut }]: [Feature, any]) => ( + // Filter out keyboard shortcuts from localStorage that no longer exist in code + const availableShortcuts = (Object.entries(shortcutKeys) as [Feature, any][]).filter( + ([feature]: [Feature, any]) => + shortcutLabels[feature] !== undefined || + (addonsShortcutLabels && addonsShortcutLabels[feature]) + ); + + const arr = availableShortcuts.map(([feature, { shortcut }]: [Feature, any]) => ( {/* @ts-expect-error (non strict) */} {shortcutLabels[feature] || addonsShortcutLabels[feature]} diff --git a/code/core/src/preview/runtime.ts b/code/core/src/preview/runtime.ts index 58d509c93e04..7f2cd0e38b64 100644 --- a/code/core/src/preview/runtime.ts +++ b/code/core/src/preview/runtime.ts @@ -1,4 +1,4 @@ -import { TELEMETRY_ERROR } from 'storybook/internal/core-events'; +import { MANAGER_INERT_ATTRIBUTE_CHANGED, TELEMETRY_ERROR } from 'storybook/internal/core-events'; import { global } from '@storybook/global'; @@ -31,6 +31,24 @@ export function setup() { channel.emit(TELEMETRY_ERROR, prepareForTelemetry(error)); }; + /** + * Ensure we synchronise the preview runtime's inert state with the manager's. The inert attribute + * used to be propagated into iframes, but this has changed, breaking focus trap implementations + * that rely on inert on the document root. We synchronise inert to ensure end user components + * don't programmatically focus when a focus trap is active in the manager UI. Otherwise, the UI + * could reach a deadlock state and be unusable. + */ + document.addEventListener('DOMContentLoaded', () => { + const channel = global.__STORYBOOK_ADDONS_CHANNEL__; + channel.on(MANAGER_INERT_ATTRIBUTE_CHANGED, (isInert: boolean) => { + if (isInert) { + document.body.setAttribute('inert', 'true'); + } else { + document.body.removeAttribute('inert'); + } + }); + }); + // handle all uncaught StorybookError at the root of the application and log to telemetry if applicable global.addEventListener('error', errorListener); global.addEventListener('unhandledrejection', unhandledRejectionListener); diff --git a/code/core/src/telemetry/event-cache.test.ts b/code/core/src/telemetry/event-cache.test.ts index d7ffb38d0c5d..08fef0a117e0 100644 --- a/code/core/src/telemetry/event-cache.test.ts +++ b/code/core/src/telemetry/event-cache.test.ts @@ -1,9 +1,14 @@ -import { describe, expect, it } from 'vitest'; +import type { MockInstance } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { getPrecedingUpgrade } from './event-cache'; +import { cache } from 'storybook/internal/common'; + +import { get, getLastEvents, getPrecedingUpgrade, set } from './event-cache'; + +vi.mock('storybook/internal/common', { spy: true }); expect.addSnapshotSerializer({ - print: (val: any) => JSON.stringify(val, null, 2), + print: (val: unknown) => JSON.stringify(val, null, 2), test: (val) => typeof val !== 'string', }); @@ -174,4 +179,68 @@ describe('event-cache', () => { `); }); }); + + describe('race condition prevention', () => { + let cacheGetMock: MockInstance; + let cacheSetMock: MockInstance; + + beforeEach(() => { + vi.clearAllMocks(); + cacheGetMock = vi.mocked(cache.get); + cacheSetMock = vi.mocked(cache.set); + }); + + it('getLastEvents waits for pending set operations to complete', async () => { + const initialData = { + init: { timestamp: 1, body: { eventType: 'init', eventId: 'init-1' } }, + }; + const updatedData = { + init: { timestamp: 1, body: { eventType: 'init', eventId: 'init-1' } }, + upgrade: { timestamp: 2, body: { eventType: 'upgrade', eventId: 'upgrade-1' } }, + }; + + // Use a simple delay to simulate async operations + let setGetResolved = false; + let setSetResolved = false; + + cacheGetMock.mockImplementationOnce(async () => { + while (!setGetResolved) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + return initialData; + }); + + cacheSetMock.mockImplementationOnce(async () => { + while (!setSetResolved) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + }); + + // Mock cache.get to return updated data after set completes + cacheGetMock.mockResolvedValueOnce(updatedData); + + // Start a set operation (this will be pending) + const setPromiseResult = set('upgrade', { eventType: 'upgrade', eventId: 'upgrade-1' }); + + // Immediately call getLastEvents() - it should wait for set() to complete + const getPromise = getLastEvents(); + + // Verify that getLastEvents hasn't resolved yet (it's waiting) + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Resolve the set operations + setGetResolved = true; + await new Promise((resolve) => setTimeout(resolve, 50)); + setSetResolved = true; + await setPromiseResult; + + // Now getLastEvents should complete and return the updated data + const result = await getPromise; + + // Verify that getLastEvents waited for set to complete and got the updated data + expect(result).toEqual(updatedData); + expect(cacheGetMock).toHaveBeenCalledTimes(2); // Once in setHelper, once in getLastEvents + expect(cacheSetMock).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/code/core/src/telemetry/event-cache.ts b/code/core/src/telemetry/event-cache.ts index 97c4e60da254..6f7516afdfdd 100644 --- a/code/core/src/telemetry/event-cache.ts +++ b/code/core/src/telemetry/event-cache.ts @@ -34,6 +34,10 @@ export const get = async (eventType: EventType): Promise }; export const getLastEvents = async (): Promise> => { + // Wait for any pending set operations to complete before reading + // This prevents race conditions where getLastEvents() reads stale data + // while a set() operation is still in progress + await operation; return (await cache.get('lastEvents')) || {}; }; diff --git a/code/package.json b/code/package.json index cc13f63a8e30..4ebf20e6d09b 100644 --- a/code/package.json +++ b/code/package.json @@ -286,5 +286,6 @@ "Dependency Upgrades" ] ] - } + }, + "deferredNextVersion": "10.1.6" } diff --git a/docs/configure/integration/eslint-plugin.mdx b/docs/configure/integration/eslint-plugin.mdx index 4f598a77a2dd..5eaed2b1f513 100644 --- a/docs/configure/integration/eslint-plugin.mdx +++ b/docs/configure/integration/eslint-plugin.mdx @@ -6,7 +6,7 @@ sidebar: title: ESLint plugin --- -Storybook provides an dedicated [ESLint plugin](https://github.com/storybookjs/eslint-plugin-storybook) to help you write stories and components aligned with the latest Storybook and frontend development best practices. +Storybook provides a dedicated [ESLint plugin](https://github.com/storybookjs/storybook/tree/next/code/lib/eslint-plugin) to help you write stories and components aligned with the latest Storybook and frontend development best practices. ## Installation diff --git a/docs/versions/latest.json b/docs/versions/latest.json index 1969ed7c42ba..6ea32a093ddc 100644 --- a/docs/versions/latest.json +++ b/docs/versions/latest.json @@ -1 +1 @@ -{"version":"10.1.5","info":{"plain":"- Addon-Vitest: Isolate error reasons during postinstall - [#33295](https://github.com/storybookjs/storybook/pull/33295), thanks @valentinpalkovic!\n- CLI: Fix react native template not copying in init - [#33308](https://github.com/storybookjs/storybook/pull/33308), thanks @dannyhw!\n- Docs: Support Rolldown bundler module namespace objects - [#33280](https://github.com/storybookjs/storybook/pull/33280), thanks @akornmeier!"}} \ No newline at end of file +{"version":"10.1.6","info":{"plain":"- Manager: Do not display non-existing shortcuts in the settings page - [#32711](https://github.com/storybookjs/storybook/pull/32711), thanks @DKER2!\n- Preview: Enforce inert body if manager is focus-trapped - [#33186](https://github.com/storybookjs/storybook/pull/33186), thanks @Sidnioulz!\n- Telemetry: Await pending operations in getLastEvents to prevent race conditions - [#33285](https://github.com/storybookjs/storybook/pull/33285), thanks @valentinpalkovic!\n- UI: Fix keyboard navigation bug for \\\"reset\\\" option in `Select` - [#33268](https://github.com/storybookjs/storybook/pull/33268), thanks @Sidnioulz!"}} \ No newline at end of file diff --git a/docs/versions/next.json b/docs/versions/next.json index ea0d4bacc08c..48840788b529 100644 --- a/docs/versions/next.json +++ b/docs/versions/next.json @@ -1 +1 @@ -{"version":"10.2.0-alpha.3","info":{"plain":"- Addon Docs: Skip `!autodocs` stories when computing primary story - [#32712](https://github.com/storybookjs/storybook/pull/32712), thanks @ia319!\n- Angular: Honor --loglevel and --logfile in dev/build - [#33212](https://github.com/storybookjs/storybook/pull/33212), thanks @valentinpalkovic!\n- CSF: Export type to prevent `type cannot be named`-errors - [#33216](https://github.com/storybookjs/storybook/pull/33216), thanks @unional!\n- Chore: Upgrade Chromatic CLI - [#33176](https://github.com/storybookjs/storybook/pull/33176), thanks @ghengeveld!\n- Core: Enhance getPrettier function to provide prettier interface - [#33260](https://github.com/storybookjs/storybook/pull/33260), thanks @valentinpalkovic!\n- Core: Fix cwd handling for negated globs - [#33241](https://github.com/storybookjs/storybook/pull/33241), thanks @ia319!\n- NextJS: Alias image to use fileURLToPath for better resolution - [#33256](https://github.com/storybookjs/storybook/pull/33256), thanks @ndelangen!\n- Nextj.js: Support top-level weight/style in next/font/local with string src - [#32998](https://github.com/storybookjs/storybook/pull/32998), thanks @Chiman2937!\n- Telemetry: Cache Storybook metadata by main config content hash - [#33247](https://github.com/storybookjs/storybook/pull/33247), thanks @valentinpalkovic!\n- TypeScript: Fix summary undefined type issue - [#32585](https://github.com/storybookjs/storybook/pull/32585), thanks @afsalshamsudeen!"}} \ No newline at end of file +{"version":"10.2.0-alpha.4","info":{"plain":"- Addon-Vitest: Isolate error reasons during postinstall - [#33295](https://github.com/storybookjs/storybook/pull/33295), thanks @valentinpalkovic!\n- CLI: Fix react native template not copying in init - [#33308](https://github.com/storybookjs/storybook/pull/33308), thanks @dannyhw!\n- Core: Retry `writeFile` cache when EBUSY error occurs - [#32981](https://github.com/storybookjs/storybook/pull/32981), thanks @reduckted!\n- Docs: Support Rolldown bundler module namespace objects - [#33280](https://github.com/storybookjs/storybook/pull/33280), thanks @akornmeier!\n- SvelteKit: Align JS template with TS template - [#31451](https://github.com/storybookjs/storybook/pull/31451), thanks @brettearle!"}} \ No newline at end of file