diff --git a/.changeset/stupid-bees-marry.md b/.changeset/stupid-bees-marry.md new file mode 100644 index 00000000000..ab7c9a9fe65 --- /dev/null +++ b/.changeset/stupid-bees-marry.md @@ -0,0 +1,5 @@ +--- +"@primer/react": minor +--- + +chore: always render ActionMenu in viewport when inside Dialog under feature flag diff --git a/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx b/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx index 7391258896d..1f2b3bf420e 100644 --- a/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx @@ -21,6 +21,7 @@ import { } from '@primer/octicons-react' import type {AnchorPosition, AnchorSide} from '@primer/behaviors' import classes from './ActionMenu.examples.stories.module.css' +import {FeatureFlags} from '../FeatureFlags' export default { title: 'Components/ActionMenu/Examples', @@ -672,86 +673,89 @@ export const InsideDialog = () => { )) return ( -
- {/* Main scrollable content */} -
- - Main Page Content - - - - - {/* Show more content after the button to make it scrollable */} - {scrollableContent} -
- - {/* Dialog containing ActionMenu */} - {isDialogOpen && ( - - - This dialog contains an ActionMenu. The main page content behind is long enough to be scrollable. + +
+ {/* Main scrollable content */} +
+ + Main Page Content - - Document Settings - - - - Configure the document properties and sharing settings. These options allow you to control how the document - is displayed and who has access to it. - - - - Actions - - - alert('Save clicked')}> - Save - ⌘S - - alert('Save as clicked')}> - Save as... - ⌘⇧S - - alert('Export clicked')}> - Export - ⌘E - - alert('Print clicked')}> - Print - ⌘P - - - alert('Copy clicked')}> - Copy - ⌘C - - alert('Paste clicked')}> - Paste - ⌘V - - alert('Duplicate clicked')}> - Duplicate - ⌘D - - - alert('Share clicked')}> - Share - ⌘⇧U - - alert('Share via email clicked')}>Share via email - alert('Share via link clicked')}>Share via link - - - - - - You can interact with the ActionMenu above while the main page content remains scrollable in the background. - -
- )} -
+ + + {/* Show more content after the button to make it scrollable */} + {scrollableContent} + + + {/* Dialog containing ActionMenu */} + {isDialogOpen && ( + + + This dialog contains an ActionMenu. The main page content behind is long enough to be scrollable. + + + + Document Settings + + + + Configure the document properties and sharing settings. These options allow you to control how the + document is displayed and who has access to it. + + + + Actions + + + alert('Save clicked')}> + Save + ⌘S + + alert('Save as clicked')}> + Save as... + ⌘⇧S + + alert('Export clicked')}> + Export + ⌘E + + alert('Print clicked')}> + Print + ⌘P + + + alert('Copy clicked')}> + Copy + ⌘C + + alert('Paste clicked')}> + Paste + ⌘V + + alert('Duplicate clicked')}> + Duplicate + ⌘D + + + alert('Share clicked')}> + Share + ⌘⇧U + + alert('Share via email clicked')}>Share via email + alert('Share via link clicked')}>Share via link + + + + + + You can interact with the ActionMenu above while the main page content remains scrollable in the + background. + + + )} + + ) } diff --git a/packages/react/src/ActionMenu/ActionMenu.test.tsx b/packages/react/src/ActionMenu/ActionMenu.test.tsx index 0be609c68e8..f7e74c3db77 100644 --- a/packages/react/src/ActionMenu/ActionMenu.test.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.test.tsx @@ -1,17 +1,41 @@ -import {describe, expect, it, vi} from 'vitest' +import {describe, expect, it, vi, beforeEach} from 'vitest' import {render as HTMLRender, waitFor, act, within} from '@testing-library/react' import userEvent from '@testing-library/user-event' import type React from 'react' import BaseStyles from '../BaseStyles' -import {ActionMenu, ActionList, Button, IconButton} from '..' +import {ActionMenu, ActionList, Button, IconButton, Dialog} from '..' import Tooltip from '../Tooltip' import {Tooltip as TooltipV2} from '../TooltipV2/Tooltip' import {SingleSelect} from '../ActionMenu/ActionMenu.features.stories' import {MixedSelection} from '../ActionMenu/ActionMenu.examples.stories' import {SearchIcon, KebabHorizontalIcon} from '@primer/octicons-react' +import {getAnchoredPosition} from '@primer/behaviors' +import type {AnchorPosition} from '@primer/behaviors' import type {JSX} from 'react' import {implementsClassName} from '../utils/testing' +import {FeatureFlags} from '../FeatureFlags' + +// Mock getAnchoredPosition for feature flag tests +vi.mock('@primer/behaviors', async () => { + const actual = await vi.importActual('@primer/behaviors') + return { + ...actual, + getAnchoredPosition: vi.fn( + ( + _floatingElement: Element, + _anchorElement: Element | DOMRect, + _settings?: Partial<{displayInViewport?: boolean}>, + ) => + ({ + top: 100, + left: 100, + anchorSide: 'outside-bottom', + anchorAlign: 'start', + }) as AnchorPosition, + ), + } +}) function Example(): JSX.Element { return ( @@ -812,4 +836,223 @@ describe('ActionMenu', () => { expect(mockOnKeyDown).toHaveBeenCalledTimes(1) }) }) + + describe('feature flag: primer_react_action_menu_display_in_viewport_inside_dialog', () => { + const mockGetAnchoredPosition = vi.mocked(getAnchoredPosition) + + beforeEach(() => { + // Reset mock before each test + mockGetAnchoredPosition.mockClear() + }) + + it('should enable displayInViewport when flag is enabled and ActionMenu is inside a dialog', async () => { + // When the ActionMenu is wrapped in a Dialog, it's inside a dialog context. + // With the flag enabled, displayInViewport should be automatically enabled. + const component = HTMLRender( + + {}}> + + Toggle Menu + + + New file + + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button', {name: 'Toggle Menu'}) + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called with displayInViewport: true + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).toBe(true) + }) + + it('should not enable displayInViewport when flag is enabled but ActionMenu is NOT inside a dialog', async () => { + // Without being wrapped in a Dialog, the ActionMenu is not in a dialog context. + // Even with the flag enabled, displayInViewport should remain at its default (false/undefined). + const component = HTMLRender( + + + Toggle Menu + + + New file + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called without displayInViewport enabled + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).not.toBe(true) + }) + + it('should not enable displayInViewport when flag is disabled, even inside a dialog', async () => { + // Even when inside a Dialog, with the flag disabled, displayInViewport + // should remain at its default (false/undefined). + const component = HTMLRender( + + {}}> + + Toggle Menu + + + New file + + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button', {name: 'Toggle Menu'}) + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called without displayInViewport enabled + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).not.toBe(true) + }) + + it('should not enable displayInViewport when flag is disabled and outside dialog', async () => { + // Default scenario: flag disabled and not in a dialog context. + // displayInViewport should remain at its default (false/undefined). + const component = HTMLRender( + + + Toggle Menu + + + New file + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called without displayInViewport enabled + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).not.toBe(true) + }) + + it('should respect explicit displayInViewport prop over feature flag logic', async () => { + // Test that an explicit displayInViewport=false prop overrides the automatic + // detection, even when the flag is enabled and the ActionMenu is inside a dialog. + const component = HTMLRender( + + {}}> + + Toggle Menu + + + New file + + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button', {name: 'Toggle Menu'}) + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called with displayInViewport: false (explicit override) + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).toBe(false) + }) + + it('should respect explicit displayInViewport=true prop even when flag is disabled', async () => { + // Test that an explicit displayInViewport=true prop works regardless of + // the flag state or dialog context. + const component = HTMLRender( + + + Toggle Menu + + + New file + + + + , + ) + + const user = userEvent.setup() + const button = component.getByRole('button') + await user.click(button) + + await waitFor(() => { + expect(component.queryByRole('menu')).toBeInTheDocument() + }) + + // Verify getAnchoredPosition was called with displayInViewport: true (explicit override) + await waitFor(() => { + expect(mockGetAnchoredPosition).toHaveBeenCalled() + }) + + const calls = mockGetAnchoredPosition.mock.calls + const lastCall = calls[calls.length - 1] + expect(lastCall[2]?.displayInViewport).toBe(true) + }) + }) }) diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index 3e1233ff55b..271d0ef349d 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -17,6 +17,8 @@ import styles from './ActionMenu.module.css' import {useResponsiveValue, type ResponsiveValue} from '../hooks/useResponsiveValue' import {isSlot} from '../utils/is-slot' import type {FCWithSlotMarker, WithSlotMarker} from '../utils/types/Slots' +import {useFeatureFlag} from '../FeatureFlags' +import {DialogContext} from '../Dialog/Dialog' export type MenuCloseHandler = ( gesture: 'anchor-click' | 'click-outside' | 'escape' | 'tab' | 'item-select' | 'arrow-left' | 'close', @@ -318,6 +320,12 @@ const Overlay: FCWithSlotMarker> = ({ } }, [anchorRef]) + const featureFlagDisplayInViewportInsidePortal = useFeatureFlag( + 'primer_react_action_menu_display_in_viewport_inside_dialog', + ) + + const isInsideDialog = useContext(DialogContext) !== undefined + return ( > = ({ focusZoneSettings={isNarrowFullscreen ? {disabled: true} : {focusOutBehavior: 'wrap'}} onPositionChange={onPositionChange} variant={variant} - displayInViewport={displayInViewport} + displayInViewport={ + displayInViewport !== undefined ? displayInViewport : featureFlagDisplayInViewportInsidePortal && isInsideDialog + } >
= [] +// useful to determine whether we're inside a Dialog from a nested component +export const DialogContext = React.createContext(undefined) + const _Dialog = React.forwardRef>((props, forwardedRef) => { const { title = 'Dialog', @@ -331,7 +334,7 @@ const _Dialog = React.forwardRef +
- + ) }) _Dialog.displayName = 'Dialog' diff --git a/packages/react/src/Dialog/index.tsx b/packages/react/src/Dialog/index.tsx new file mode 100644 index 00000000000..c0faabe53fa --- /dev/null +++ b/packages/react/src/Dialog/index.tsx @@ -0,0 +1,3 @@ +export {Dialog} from './Dialog' + +export type {DialogButtonProps, DialogHeaderProps, DialogHeight, DialogProps, DialogWidth} from './Dialog' diff --git a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts index b7952bde39e..796448887e8 100644 --- a/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts +++ b/packages/react/src/FeatureFlags/DefaultFeatureFlags.ts @@ -7,4 +7,5 @@ export const DefaultFeatureFlags = FeatureFlagScope.create({ primer_react_select_panel_order_selected_at_top: false, primer_react_select_panel_remove_active_descendant: false, primer_react_spinner_synchronize_animations: false, + primer_react_action_menu_display_in_viewport_inside_dialog: false, }) diff --git a/packages/react/src/TreeView/TreeView.examples.stories.tsx b/packages/react/src/TreeView/TreeView.examples.stories.tsx index 874e081b3fe..009b7a4aeb6 100644 --- a/packages/react/src/TreeView/TreeView.examples.stories.tsx +++ b/packages/react/src/TreeView/TreeView.examples.stories.tsx @@ -3,7 +3,7 @@ import type {StoryFn, Meta} from '@storybook/react-vite' import React from 'react' import {TreeView} from './TreeView' import {IconButton} from '../Button' -import {Dialog} from '../Dialog/Dialog' +import {Dialog} from '../Dialog' import classes from './TreeView.stories.module.css' const meta: Meta = { diff --git a/packages/react/src/TreeView/TreeView.tsx b/packages/react/src/TreeView/TreeView.tsx index c10d740513a..73f19d08c24 100644 --- a/packages/react/src/TreeView/TreeView.tsx +++ b/packages/react/src/TreeView/TreeView.tsx @@ -21,7 +21,7 @@ import {getFirstChildElement, useRovingTabIndex} from './useRovingTabIndex' import {useTypeahead} from './useTypeahead' import {SkeletonAvatar} from '../SkeletonAvatar' import {SkeletonText} from '../SkeletonText' -import {Dialog} from '../Dialog/Dialog' +import {Dialog} from '../Dialog' import {Button, IconButton} from '../Button' import {ActionList} from '../ActionList' import {getAccessibleKeybindingHintString} from '../KeybindingHint' diff --git a/packages/react/src/experimental/index.ts b/packages/react/src/experimental/index.ts index 0a8bd1bb9e0..4c194a02f24 100644 --- a/packages/react/src/experimental/index.ts +++ b/packages/react/src/experimental/index.ts @@ -34,7 +34,7 @@ export type { ObjectPaths, } from '../DataTable' -export * from '../Dialog/Dialog' +export * from '../Dialog' export {InlineMessage} from '../InlineMessage' export type {InlineMessageProps} from '../InlineMessage' diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index b3b6a1ca40d..66c8d438e12 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -92,8 +92,8 @@ export {default as CounterLabel} from './CounterLabel' export type {CounterLabelProps} from './CounterLabel' export {default as Details} from './Details' export type {DetailsProps} from './Details' -export {Dialog} from './Dialog/Dialog' -export type {DialogProps, DialogHeaderProps, DialogButtonProps, DialogWidth, DialogHeight} from './Dialog/Dialog' +export {Dialog} from './Dialog' +export type {DialogProps, DialogHeaderProps, DialogButtonProps, DialogWidth, DialogHeight} from './Dialog' export type {ConfirmationDialogProps} from './ConfirmationDialog/ConfirmationDialog' export {ConfirmationDialog} from './ConfirmationDialog/ConfirmationDialog' export {default as Flash} from './Flash'