From ca6ab973b4ef143d762e02d468ca2447bf0fe4c9 Mon Sep 17 00:00:00 2001 From: Ashley Harrison Date: Fri, 9 Jan 2026 16:47:25 +0000 Subject: [PATCH] Modal/Drawer: Switch to use floating-ui's focus trapping (#116017) * Add awareness of a parent when toggletip is rendered to work inside other modals * switch modal + drawer to use floating-ui's focus trapping * remove outdated docs * fix some unit tests * fix scopes tests * remove duplicate aria-label * kick CI * fix e2e tests --------- Co-authored-by: tdbishop --- .../dashboard-keybindings.spec.ts | 2 + .../dashboard-keybindings.spec.ts | 2 + .../panels-suite/table-kitchenSink.spec.ts | 2 +- .../src/components/Drawer/Drawer.tsx | 45 +++----- .../grafana-ui/src/components/Modal/Modal.tsx | 58 +++++----- .../src/components/Toggletip/Toggletip.mdx | 2 - .../components/Toggletip/Toggletip.story.tsx | 87 +++++++++++++++ .../receivers/NewReceiverView.test.tsx | 3 + .../SqlExpressions/SqlExpr.test.tsx | 105 +++++++++--------- .../LibraryPanelsSearch.test.tsx | 18 +-- .../features/scopes/tests/selector.test.ts | 3 +- .../features/scopes/tests/utils/actions.ts | 2 +- 12 files changed, 207 insertions(+), 122 deletions(-) diff --git a/e2e-playwright/dashboard-new-layouts/dashboard-keybindings.spec.ts b/e2e-playwright/dashboard-new-layouts/dashboard-keybindings.spec.ts index 19ad38f16d5..867337ba088 100644 --- a/e2e-playwright/dashboard-new-layouts/dashboard-keybindings.spec.ts +++ b/e2e-playwright/dashboard-new-layouts/dashboard-keybindings.spec.ts @@ -51,6 +51,8 @@ test.describe('Dashboard keybindings with new layouts', { tag: ['@dashboards'] } await expect(dashboardPage.getByGrafanaSelector(selectors.components.PanelInspector.Json.content)).toBeVisible(); + // Press Escape to close tooltip on the close button + await page.keyboard.press('Escape'); // Press Escape to close inspector await page.keyboard.press('Escape'); diff --git a/e2e-playwright/dashboards-suite/dashboard-keybindings.spec.ts b/e2e-playwright/dashboards-suite/dashboard-keybindings.spec.ts index f874cefa27c..dd83b3a0cfd 100644 --- a/e2e-playwright/dashboards-suite/dashboard-keybindings.spec.ts +++ b/e2e-playwright/dashboards-suite/dashboard-keybindings.spec.ts @@ -58,6 +58,8 @@ test.describe( await expect(dashboardPage.getByGrafanaSelector(selectors.components.PanelInspector.Json.content)).toBeVisible(); + // Press Escape to close tooltip on the close button + await page.keyboard.press('Escape'); // Press Escape to close inspector await page.keyboard.press('Escape'); diff --git a/e2e-playwright/panels-suite/table-kitchenSink.spec.ts b/e2e-playwright/panels-suite/table-kitchenSink.spec.ts index 6dddba81820..ac10cb2b735 100644 --- a/e2e-playwright/panels-suite/table-kitchenSink.spec.ts +++ b/e2e-playwright/panels-suite/table-kitchenSink.spec.ts @@ -82,9 +82,9 @@ test.describe('Panels test: Table - Kitchen Sink', { tag: ['@panels', '@table'] await expect(getCellHeight(page, 1, longTextColIdx)).resolves.toBeLessThan(100); // click cell inspect, check that cell inspection pops open in the side as we'd expect. - await loremIpsumCell.getByLabel('Inspect value').click(); const loremIpsumText = await loremIpsumCell.textContent(); expect(loremIpsumText).toBeDefined(); + await loremIpsumCell.getByLabel('Inspect value').click(); await expect(page.getByRole('dialog').getByText(loremIpsumText!)).toBeVisible(); }); diff --git a/packages/grafana-ui/src/components/Drawer/Drawer.tsx b/packages/grafana-ui/src/components/Drawer/Drawer.tsx index 00039adfc3f..7d115e3f9d0 100644 --- a/packages/grafana-ui/src/components/Drawer/Drawer.tsx +++ b/packages/grafana-ui/src/components/Drawer/Drawer.tsx @@ -1,9 +1,7 @@ import { css, cx } from '@emotion/css'; +import { FloatingFocusManager, useFloating } from '@floating-ui/react'; import RcDrawer from '@rc-component/drawer'; -import { useDialog } from '@react-aria/dialog'; -import { FocusScope } from '@react-aria/focus'; -import { useOverlay } from '@react-aria/overlays'; -import { ReactNode, useCallback, useEffect, useState } from 'react'; +import { ReactNode, useCallback, useEffect, useId, useState } from 'react'; import * as React from 'react'; import { GrafanaTheme2 } from '@grafana/data'; @@ -81,17 +79,16 @@ export function Drawer({ const styles = useStyles2(getStyles); const wrapperStyles = useStyles2(getWrapperStyles, size); const dragStyles = useStyles2(getDragStyles); + const titleId = useId(); - const overlayRef = React.useRef(null); - const { dialogProps, titleProps } = useDialog({}, overlayRef); - const { overlayProps } = useOverlay( - { - isDismissable: false, - isOpen: true, - onClose, + const { context, refs } = useFloating({ + open: true, + onOpenChange: (open) => { + if (!open) { + onClose?.(); + } }, - overlayRef - ); + }); // Adds body class while open so the toolbar nav can hide some actions while drawer is open useBodyClassWhileOpen(); @@ -117,6 +114,8 @@ export function Drawer({ minWidth, }, }} + aria-label={typeof title === 'string' ? selectors.components.Drawer.General.title(title) : undefined} + aria-labelledby={typeof title !== 'string' ? titleId : undefined} width={''} motion={{ motionAppear: true, @@ -129,18 +128,8 @@ export function Drawer({ motionName: styles.maskMotion, }} > - -
+ +
{/* eslint-disable-next-line jsx-a11y/no-static-element-interactions */}
{typeof title === 'string' ? ( - + {title} {subtitle && ( @@ -169,13 +158,13 @@ export function Drawer({ )} ) : ( - title +
{title}
)} {tabs &&
{tabs}
}
{!scrollableContent ? content : {content}}
- + ); } diff --git a/packages/grafana-ui/src/components/Modal/Modal.tsx b/packages/grafana-ui/src/components/Modal/Modal.tsx index aaeb2c3e426..f6ea73c3d31 100644 --- a/packages/grafana-ui/src/components/Modal/Modal.tsx +++ b/packages/grafana-ui/src/components/Modal/Modal.tsx @@ -1,9 +1,7 @@ import { cx } from '@emotion/css'; -import { useDialog } from '@react-aria/dialog'; -import { FocusScope } from '@react-aria/focus'; -import { OverlayContainer, useOverlay } from '@react-aria/overlays'; -import { PropsWithChildren, useRef, type JSX } from 'react'; -import * as React from 'react'; +import { FloatingFocusManager, useDismiss, useFloating, useInteractions, useRole } from '@floating-ui/react'; +import { OverlayContainer } from '@react-aria/overlays'; +import { PropsWithChildren, ReactNode, useId, type JSX } from 'react'; import { t } from '@grafana/i18n'; @@ -66,23 +64,26 @@ export function Modal(props: PropsWithChildren) { trapFocus = true, } = props; const styles = useStyles2(getModalStyles); + const titleId = useId(); - const ref = useRef(null); - - // Handle interacting outside the dialog and pressing - // the Escape key to close the modal. - const { overlayProps, underlayProps } = useOverlay( - { isKeyboardDismissDisabled: !closeOnEscape, isOpen, onClose: onDismiss }, - ref - ); - - // Get props for the dialog and its title - const { dialogProps, titleProps } = useDialog( - { - 'aria-label': ariaLabel, + const { context, refs } = useFloating({ + open: isOpen, + onOpenChange: (open) => { + if (!open) { + onDismiss?.(); + } }, - ref - ); + }); + + const dismiss = useDismiss(context, { + enabled: closeOnEscape, + }); + + const role = useRole(context, { + role: 'dialog', + }); + + const { getFloatingProps } = useInteractions([dismiss, role]); if (!isOpen) { return null; @@ -96,12 +97,17 @@ export function Modal(props: PropsWithChildren) { role="presentation" className={styles.modalBackdrop} onClick={onClickBackdrop || (closeOnBackdropClick ? onDismiss : undefined)} - {...underlayProps} /> - -
+ +
- {typeof title === 'string' && } + {typeof title === 'string' && } { // FIXME: custom title components won't get an accessible title. // Do we really want to support them or shall we just limit this ModalTabsHeader? @@ -118,12 +124,12 @@ export function Modal(props: PropsWithChildren) {
{children}
- + ); } -function ModalButtonRow({ leftItems, children }: { leftItems?: React.ReactNode; children: React.ReactNode }) { +function ModalButtonRow({ leftItems, children }: { leftItems?: ReactNode; children: ReactNode }) { const styles = useStyles2(getModalStyles); if (leftItems) { diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx b/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx index 53b75f5b5cb..fcfea97a45f 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx @@ -75,5 +75,3 @@ return ( ); ``` - - diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx b/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx index 1c3cbb57919..a7006fd70eb 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx @@ -1,6 +1,11 @@ import { Meta, StoryFn } from '@storybook/react'; +import { useState } from 'react'; import { Button } from '../Button/Button'; +import { Drawer } from '../Drawer/Drawer'; +import { Field } from '../Forms/Field'; +import { Input } from '../Input/Input'; +import { Modal } from '../Modal/Modal'; import { ScrollContainer } from '../ScrollContainer/ScrollContainer'; import mdx from '../Toggletip/Toggletip.mdx'; @@ -133,4 +138,86 @@ LongContent.parameters = { }, }; +export const InsideDrawer: StoryFn = () => { + const [isDrawerOpen, setIsDrawerOpen] = useState(false); + + return ( + <> + + {isDrawerOpen && ( + setIsDrawerOpen(false)}> +
+

This demonstrates using Toggletip inside a Drawer.

+ + + + + +
+ } + footer="Focus should work correctly within this Toggletip" + placement="bottom-start" + > + + +
+ + )} + + ); +}; + +InsideDrawer.parameters = { + controls: { + hideNoControlsWarning: true, + exclude: ['title', 'content', 'footer', 'children', 'placement', 'theme', 'closeButton', 'portalRoot'], + }, +}; + +export const InsideModal: StoryFn = () => { + const [isModalOpen, setIsModalOpen] = useState(false); + + return ( + <> + + setIsModalOpen(false)}> +
+

This demonstrates using Toggletip inside a Modal.

+ + + + + + +
+ } + footer="Focus should work correctly within this Toggletip" + placement="bottom-start" + > + + + +
+ + + ); +}; + +InsideDrawer.parameters = { + controls: { + hideNoControlsWarning: true, + exclude: ['title', 'content', 'footer', 'children', 'placement', 'theme', 'closeButton', 'portalRoot'], + }, +}; + export default meta; diff --git a/public/app/features/alerting/unified/components/receivers/NewReceiverView.test.tsx b/public/app/features/alerting/unified/components/receivers/NewReceiverView.test.tsx index ad6ee94d73e..b57242f0e49 100644 --- a/public/app/features/alerting/unified/components/receivers/NewReceiverView.test.tsx +++ b/public/app/features/alerting/unified/components/receivers/NewReceiverView.test.tsx @@ -79,6 +79,9 @@ describe('new receiver', () => { // click test await user.click(ui.testContactPoint.get()); + // close the modal + await user.click(screen.getByRole('button', { name: 'Close' })); + // we shouldn't be testing implementation details but when the request is successful // it can't seem to assert on the success toast await user.click(ui.saveContactButton.get()); diff --git a/public/app/features/expressions/components/SqlExpressions/SqlExpr.test.tsx b/public/app/features/expressions/components/SqlExpressions/SqlExpr.test.tsx index 9e2cfd50b75..433ce7f1d91 100644 --- a/public/app/features/expressions/components/SqlExpressions/SqlExpr.test.tsx +++ b/public/app/features/expressions/components/SqlExpressions/SqlExpr.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, render, testWithFeatureToggles } from 'test/test-utils'; +import { render, testWithFeatureToggles, userEvent, waitFor } from 'test/test-utils'; import { ExpressionQuery, ExpressionQueryType } from '../../types'; @@ -72,12 +72,12 @@ describe('SqlExpr', () => { const refIds = [{ value: 'A' }]; const query = { refId: 'expr1', type: 'sql', expression: '' } as ExpressionQuery; - await act(async () => { - render(); - }); + render(); // Verify onChange was called - expect(onChange).toHaveBeenCalled(); + await waitFor(() => { + expect(onChange).toHaveBeenCalled(); + }); // Verify essential SQL structure without exact string matching const updatedQuery = onChange.mock.calls[0][0]; @@ -90,19 +90,12 @@ describe('SqlExpr', () => { const existingExpression = 'SELECT 1 AS foo'; const query = { refId: 'expr1', type: 'sql', expression: existingExpression } as ExpressionQuery; - await act(async () => { - render(); - }); - - // Check if onChange was called - if (onChange.mock.calls.length > 0) { - // If called, ensure it didn't change the expression value - const updatedQuery = onChange.mock.calls[0][0]; - expect(updatedQuery.expression).toBe(existingExpression); - } + render(); // The SQLEditor should receive the existing expression - expect(query.expression).toBe(existingExpression); + await waitFor(() => { + expect(query.expression).toBe(existingExpression); + }); }); it('adds alerting format when alerting prop is true', async () => { @@ -110,40 +103,12 @@ describe('SqlExpr', () => { const refIds = [{ value: 'A' }]; const query = { refId: 'expr1', type: 'sql' } as ExpressionQuery; - await act(async () => { - render(); + render(); + + await waitFor(() => { + const updatedQuery = onChange.mock.calls[0][0]; + expect(updatedQuery.format).toBe('alerting'); }); - - const updatedQuery = onChange.mock.calls[0][0]; - expect(updatedQuery.format).toBe('alerting'); - }); -}); - -describe('SqlExpr with GenAI features', () => { - const defaultProps: SqlExprProps = { - onChange: jest.fn(), - refIds: [{ value: 'A' }], - query: { refId: 'expression_1', type: ExpressionQueryType.sql, expression: `SELECT * FROM A LIMIT 10` }, - queries: [], - }; - - it('renders suggestions drawer when isDrawerOpen is true', async () => { - const { useSQLSuggestions } = require('./GenAI/hooks/useSQLSuggestions'); - useSQLSuggestions.mockImplementation(() => ({ - isDrawerOpen: true, - suggestions: ['suggestion1', 'suggestion2'], - })); - - const { findByTestId } = render(); - expect(await findByTestId('suggestions-drawer')).toBeInTheDocument(); - }); - - it('renders explanation drawer when isExplanationOpen is true', async () => { - const { useSQLExplanations } = require('./GenAI/hooks/useSQLExplanations'); - useSQLExplanations.mockImplementation(() => ({ isExplanationOpen: true })); - - const { findByTestId } = render(); - expect(await findByTestId('explanation-drawer')).toBeInTheDocument(); }); }); @@ -166,10 +131,10 @@ describe('Schema Inspector feature toggle', () => { }); }); - it('renders panel open by default', () => { - const { getByText } = render(); + it('renders panel open by default', async () => { + const { findByText } = render(); - expect(getByText('No schema information available')).toBeInTheDocument(); + expect(await findByText('No schema information available')).toBeInTheDocument(); }); it('closes panel and shows reopen button when close button clicked', async () => { @@ -178,7 +143,7 @@ describe('Schema Inspector feature toggle', () => { expect(queryByText('No schema information available')).toBeInTheDocument(); const closeButton = getByText('Schema inspector'); - await act(async () => fireEvent.click(closeButton)); + await userEvent.click(closeButton); expect(queryByText('No schema information available')).not.toBeInTheDocument(); expect(await findByText('Schema inspector')).toBeInTheDocument(); @@ -188,12 +153,12 @@ describe('Schema Inspector feature toggle', () => { const { queryByText, getByText } = render(); const closeButton = getByText('Schema inspector'); - await act(async () => fireEvent.click(closeButton)); + await userEvent.click(closeButton); expect(queryByText('No schema information available')).not.toBeInTheDocument(); const reopenButton = getByText('Schema inspector'); - await act(async () => fireEvent.click(reopenButton)); + await userEvent.click(reopenButton); expect(queryByText('No schema information available')).toBeInTheDocument(); }); @@ -233,3 +198,33 @@ describe('Schema Inspector feature toggle', () => { }); }); }); + +describe('SqlExpr with GenAI features', () => { + const defaultProps: SqlExprProps = { + onChange: jest.fn(), + refIds: [{ value: 'A' }], + query: { refId: 'expression_1', type: ExpressionQueryType.sql, expression: `SELECT * FROM A LIMIT 10` }, + queries: [], + }; + + it('renders suggestions drawer when isDrawerOpen is true', async () => { + // TODO this inline require breaks future tests - do it differently! + const { useSQLSuggestions } = require('./GenAI/hooks/useSQLSuggestions'); + useSQLSuggestions.mockImplementation(() => ({ + isDrawerOpen: true, + suggestions: ['suggestion1', 'suggestion2'], + })); + + const { findByTestId } = render(); + expect(await findByTestId('suggestions-drawer')).toBeInTheDocument(); + }); + + it('renders explanation drawer when isExplanationOpen is true', async () => { + // TODO this inline require breaks future tests - do it differently! + const { useSQLExplanations } = require('./GenAI/hooks/useSQLExplanations'); + useSQLExplanations.mockImplementation(() => ({ isExplanationOpen: true })); + + const { findByTestId } = render(); + expect(await findByTestId('explanation-drawer')).toBeInTheDocument(); + }); +}); diff --git a/public/app/features/library-panels/components/LibraryPanelsSearch/LibraryPanelsSearch.test.tsx b/public/app/features/library-panels/components/LibraryPanelsSearch/LibraryPanelsSearch.test.tsx index 941dca5416d..8645dfb4c4e 100644 --- a/public/app/features/library-panels/components/LibraryPanelsSearch/LibraryPanelsSearch.test.tsx +++ b/public/app/features/library-panels/components/LibraryPanelsSearch/LibraryPanelsSearch.test.tsx @@ -340,14 +340,16 @@ describe('LibraryPanelsSearch', () => { await user.click(screen.getAllByRole('button', { name: 'Delete' })[1]); await waitFor(() => - expect(getLibraryPanelsSpy).toHaveBeenCalledWith({ - searchString: '', - folderFilterUIDs: ['wfTJJL5Wz'], - page: 1, - typeFilter: [], - sortDirection: undefined, - perPage: 40, - }) + expect(getLibraryPanelsSpy).toHaveBeenCalledWith( + expect.objectContaining({ + searchString: '', + folderFilterUIDs: ['wfTJJL5Wz'], + page: 1, + typeFilter: [], + sortDirection: undefined, + perPage: 40, + }) + ) ); }); }); diff --git a/public/app/features/scopes/tests/selector.test.ts b/public/app/features/scopes/tests/selector.test.ts index f12f848657a..f4ac22b6850 100644 --- a/public/app/features/scopes/tests/selector.test.ts +++ b/public/app/features/scopes/tests/selector.test.ts @@ -105,7 +105,6 @@ describe('Selector', () => { // Lowercase because we don't have any backend that returns the correct case, then it falls back to the value in the URL expectScopesSelectorValue('grafana'); await openSelector(); - //screen.debug(undefined, 100000); expectResultApplicationsGrafanaSelected(); jest.spyOn(locationService, 'getLocation').mockRestore(); @@ -175,6 +174,7 @@ describe('Selector', () => { await applyScopes(); // Deselect all scopes + await hoverSelector(); await clearSelector(); // Recent scopes should still be available @@ -197,6 +197,7 @@ describe('Selector', () => { await selectResultApplicationsMimir(); await applyScopes(); + await hoverSelector(); await clearSelector(); // Check recent scopes are updated diff --git a/public/app/features/scopes/tests/utils/actions.ts b/public/app/features/scopes/tests/utils/actions.ts index fc5fde93193..e49acb8670a 100644 --- a/public/app/features/scopes/tests/utils/actions.ts +++ b/public/app/features/scopes/tests/utils/actions.ts @@ -47,7 +47,7 @@ const type = async (selector: () => HTMLInputElement, value: string) => { export const updateScopes = async (service: ScopesService, scopes: string[]) => act(async () => service.changeScopes(scopes)); export const openSelector = async () => click(getSelectorInput); -export const hoverSelector = async () => fireEvent.mouseOver(getSelectorInput()); +export const hoverSelector = async () => userEvent.hover(getSelectorInput()); export const clearSelector = async () => click(getSelectorClear); export const applyScopes = async () => { await click(getSelectorApply);