From 849098bcf06e0d3d903f94b6cb96f57579c4e791 Mon Sep 17 00:00:00 2001 From: Paul Marbach Date: Fri, 9 Jan 2026 17:09:52 -0500 Subject: [PATCH] wip: improve the working branch --- .../various-suite/pie-chart.spec.ts | 4 + .../ContextMenu/WithContextMenu.test.tsx | 174 ++++++++++++++++++ .../ContextMenu/WithContextMenu.tsx | 70 ++++--- .../DataLinks/DataLinksContextMenu.tsx | 27 +-- .../app/plugins/panel/piechart/PieChart.tsx | 116 +++++------- 5 files changed, 274 insertions(+), 117 deletions(-) create mode 100644 packages/grafana-ui/src/components/ContextMenu/WithContextMenu.test.tsx diff --git a/e2e-playwright/various-suite/pie-chart.spec.ts b/e2e-playwright/various-suite/pie-chart.spec.ts index 6ead086de5b..4f52dff2a14 100644 --- a/e2e-playwright/various-suite/pie-chart.spec.ts +++ b/e2e-playwright/various-suite/pie-chart.spec.ts @@ -17,5 +17,9 @@ test.describe( ); await expect(pieChartSlices).toHaveCount(5); }); + + describe('keyboard accessibility', () => { + // FIXME: DATALINKS ACCESSIBILITY TESTS HERE + }); } ); diff --git a/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.test.tsx b/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.test.tsx new file mode 100644 index 00000000000..a8cbae377ab --- /dev/null +++ b/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.test.tsx @@ -0,0 +1,174 @@ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { MenuItem, MenuGroup } from '@grafana/ui'; + +import { WithContextMenu } from './WithContextMenu'; + +describe('WithContextMenu', () => { + it('supports mouse events', async () => { + render( + ( + <> + + + + + + )} + > + {({ openMenu }) => ( +
+ Click me +
+ )} + + ); + + expect(screen.getByTestId('context-menu-target')).toBeInTheDocument(); + expect(screen.queryByText('Item 1')).not.toBeInTheDocument(); + expect(screen.queryByText('Item 2')).not.toBeInTheDocument(); + + // Simulate click to open context menu + await userEvent.click(screen.getByTestId('context-menu-target')); + + expect(screen.getByText('Item 1')).toBeInTheDocument(); + expect(screen.getByText('Item 2')).toBeInTheDocument(); + }); + + // FIXME: this test isn't correct yet, probably because of how I've done the user-events wrong + it('supports keyboard events', async () => { + class DOMRect { + public get top(): number { + return this.y; + } + public get left(): number { + return this.x; + } + public get bottom(): number { + return this.y + this.height; + } + public get right(): number { + return this.x + this.width; + } + constructor( + public x = 0, + public y = 0, + public width = 0, + public height = 0 + ) { + this.x = x; + this.y = y; + this.width = width; + this.height = height; + } + static fromRect(other: DOMRect) { + return new DOMRect(other.x, other.y, other.width, other.height); + } + toJSON() { + return JSON.stringify(this); + } + } + + render( + ( + <> + + + + + + )} + > + {({ openMenu }) => ( +
{ + ev.preventDefault(); + openMenu(ev); + }} + > + Press enter on me +
+ )} + + ); + + expect(screen.getByTestId('context-menu-target')).toBeInTheDocument(); + expect(screen.queryByText('Item 1')).not.toBeInTheDocument(); + expect(screen.queryByText('Item 2')).not.toBeInTheDocument(); + + const target = screen.getByTestId('context-menu-target'); + + // Simulate key down to open context menu + await userEvent.type(target, ' '); + + expect(screen.getByText('Item 1')).toBeInTheDocument(); + expect(screen.getByText('Item 2')).toBeInTheDocument(); + }); + + it('supports explicit positioning', async () => { + render( + ( + <> + + + + + + )} + > + {({ openMenu }) => ( +
openMenu({ x: ev.pageX, y: ev.pageY })}> + Click me +
+ )} + + ); + + expect(screen.getByTestId('context-menu-target')).toBeInTheDocument(); + expect(screen.queryByText('Item 1')).not.toBeInTheDocument(); + expect(screen.queryByText('Item 2')).not.toBeInTheDocument(); + + // Simulate key down to open context menu + await userEvent.click(screen.getByTestId('context-menu-target')); + + expect(screen.getByText('Item 1')).toBeInTheDocument(); + expect(screen.getByText('Item 2')).toBeInTheDocument(); + }); + + it('does not open menu when openMenu is called with undefined', async () => { + render( + ( + <> + + + + + + )} + > + {({ openMenu }) => ( +
openMenu(undefined)}> + Click me +
+ )} + + ); + + expect(screen.getByTestId('context-menu-target')).toBeInTheDocument(); + expect(screen.queryByText('Item 1')).not.toBeInTheDocument(); + expect(screen.queryByText('Item 2')).not.toBeInTheDocument(); + + // Simulate click to open context menu + await userEvent.click(screen.getByTestId('context-menu-target')); + + expect(screen.queryByText('Item 1')).not.toBeInTheDocument(); + expect(screen.queryByText('Item 2')).not.toBeInTheDocument(); + }); +}); diff --git a/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.tsx b/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.tsx index 426e648062a..6772abfd94a 100644 --- a/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.tsx +++ b/packages/grafana-ui/src/components/ContextMenu/WithContextMenu.tsx @@ -3,9 +3,23 @@ import * as React from 'react'; import { ContextMenu } from '../ContextMenu/ContextMenu'; +/** + * This callback supports several ways to provide the x/y coordinates to open the context menu: + * - MouseEvent, to open the menu at the mouse position + * - SyntheticEvent, to open the menu at the location of the currentTarget element for non-mouse events + * - An object with x and y coordinates to open the menu at a specific position, for other use-cases + */ +export type WithContextMenuOpenMenuCallback = ( + e: + | React.MouseEvent + | React.SyntheticEvent + | { x: number; y: number } + | undefined +) => void; + export interface WithContextMenuProps { /** Menu item trigger that accepts openMenu prop */ - children: (props: { openMenu: React.MouseEventHandler }) => JSX.Element; + children: (props: { openMenu: WithContextMenuOpenMenuCallback }) => JSX.Element; /** A function that returns an array of menu items */ renderMenuItems: () => React.ReactNode; /** On menu open focus the first element */ @@ -15,39 +29,43 @@ export interface WithContextMenuProps { export const WithContextMenu = ({ children, renderMenuItems, focusOnOpen = true }: WithContextMenuProps) => { const [isMenuOpen, setIsMenuOpen] = useState(false); const [menuPosition, setMenuPosition] = useState({ x: 0, y: 0 }); - - const handleOpenMenu = React.useCallback( - (e: React.MouseEvent | { x: number; y: number } | HTMLElement | SVGElement) => { - setIsMenuOpen(true); - if (e && 'pageX' in e && 'pageY' in e) { - // Mouse event - setMenuPosition({ - x: e.pageX, - y: e.pageY - window.scrollY, - }); - } else if (e && 'x' in e && 'y' in e && typeof e.x === 'number') { - // Position object - setMenuPosition({ - x: e.x, - y: e.y, - }); - } else if (e && 'getBoundingClientRect' in e) { - // Element - calculate position from element's bounding rect - const rect = (e as HTMLElement | SVGElement).getBoundingClientRect(); + + const handleOpenMenu: WithContextMenuOpenMenuCallback = React.useCallback((e) => { + if (!e) { + return; + } + + setIsMenuOpen(true); + + if ('pageX' in e && 'pageY' in e) { + // Mouse event + setMenuPosition({ + x: e.pageX, + y: e.pageY - window.scrollY, + }); + } else if ('currentTarget' in e) { + // Element - calculate position from element's bounding rect + const rect = e.currentTarget.getBoundingClientRect(); + if (rect) { setMenuPosition({ x: rect.left + rect.width / 2, y: rect.top + rect.height / 2 + window.scrollY, }); } - }, - [] - ); + } else if ('x' in e && 'y' in e && typeof e.x === 'number') { + // Position object + setMenuPosition({ + x: e.x, + y: e.y, + }); + } else if (process.env.NODE_ENV !== 'production') { + console.warn('WithContextMenu: Unsupported parameter to openMenu:', e); + } + }, []); return ( <> - {children({ - openMenu: handleOpenMenu as React.MouseEventHandler, - })} + {children({ openMenu: handleOpenMenu })} {isMenuOpen && ( | ((position?: { x: number; y: number }) => void); + openMenu?: WithContextMenuOpenMenuCallback; targetClassName?: string; - /** Function to calculate menu position from an element (for keyboard events) */ - getMenuPosition?: (element: HTMLElement | SVGElement) => { x: number; y: number }; } export const DataLinksContextMenu = ({ children, links, style }: DataLinksContextMenuProps) => { @@ -49,6 +46,7 @@ export const DataLinksContextMenu = ({ children, links, style }: DataLinksContex active={item.active} onClick={item.onClick} className={styles.itemWrapper} + tabIndex={0} /> ))} @@ -64,24 +62,7 @@ export const DataLinksContextMenu = ({ children, links, style }: DataLinksContex return ( {({ openMenu }) => { - // Wrapper that handles both mouse events and position/element for keyboard events - const handleOpenMenu: React.MouseEventHandler | ((positionOrElement?: { x: number; y: number } | HTMLElement | SVGElement) => void) = ( - e: React.MouseEvent | { x: number; y: number } | HTMLElement | SVGElement | undefined - ) => { - if (openMenu) { - openMenu(e as any); - } - }; - - const getMenuPosition = (element: HTMLElement | SVGElement) => { - const rect = element.getBoundingClientRect(); - return { - x: rect.left + rect.width / 2, - y: rect.top + rect.height / 2 + window.scrollY, - }; - }; - - return children({ openMenu: handleOpenMenu, targetClassName, getMenuPosition }); + return children({ openMenu, targetClassName }); }} ); diff --git a/public/app/plugins/panel/piechart/PieChart.tsx b/public/app/plugins/panel/piechart/PieChart.tsx index a3fcc3c292b..de4ec81e24b 100644 --- a/public/app/plugins/panel/piechart/PieChart.tsx +++ b/public/app/plugins/panel/piechart/PieChart.tsx @@ -16,6 +16,7 @@ import { GrafanaTheme2, DataHoverClearEvent, DataHoverEvent, + LinkModel, } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { t } from '@grafana/i18n'; @@ -28,7 +29,7 @@ import { SeriesTable, usePanelContext, } from '@grafana/ui'; -import { getTooltipContainerStyles, useComponentInstanceId } from '@grafana/ui/internal'; +import { DataLinksContextMenuApi, getTooltipContainerStyles, useComponentInstanceId } from '@grafana/ui/internal'; import { PieChartType, PieChartLabels } from './panelcfg.gen'; import { filterDisplayItems, sumDisplayItemsReducer } from './utils'; @@ -117,40 +118,40 @@ export const PieChart = ({ {(pie) => ( <> {pie.arcs.map((arc) => { - const color = arc.data.display.color ?? FALLBACK_COLOR; - const highlightState = getHighlightState(highlightedTitle, arc); + const color = arc.data.display.color ?? FALLBACK_COLOR; + const highlightState = getHighlightState(highlightedTitle, arc); - if (arc.data.hasLinks && arc.data.getLinks) { - return ( - - ); - } else { - return ( - - ); - } - })} + if (arc.data.hasLinks && arc.data.getLinks) { + return ( + + ); + } else { + return ( + + ); + } + })} {showLabel && pie.arcs.map((arc) => { const highlightState = getHighlightState(highlightedTitle, arc); @@ -195,18 +196,18 @@ interface SliceProps { fill: string; tooltip: UseTooltipParams; tooltipOptions: VizTooltipOptions; - openMenu?: (event: React.MouseEvent) => void; + openMenu?: DataLinksContextMenuApi['openMenu']; outerRadius: number; innerRadius: number; } interface PieSliceWithDataLinksProps extends Omit { - links: () => any[]; + links: () => LinkModel[]; } interface PieChartDataLinksContextMenuProps { - links: () => any[]; - children: (props: { openMenu?: React.MouseEventHandler }) => React.ReactElement; + links: () => LinkModel[]; + children: (props: { openMenu?: DataLinksContextMenuApi['openMenu'] }) => React.ReactElement; elementRef: React.RefObject; publishDataHoverEvent: (raw: Event | React.SyntheticEvent) => void; publishDataHoverClearEvent: (raw: Event | React.SyntheticEvent) => void; @@ -253,13 +254,8 @@ function PieChartDataLinksContextMenu({ setTimeout(ensureNotFocusable, 0); } - const handleAnchorFocus = (e: FocusEvent) => { - publishDataHoverEvent(e); - }; - - const handleAnchorBlur = (e: FocusEvent) => { - publishDataHoverClearEvent(e); - }; + const handleAnchorFocus = publishDataHoverEvent; + const handleAnchorBlur = publishDataHoverClearEvent; const handleAnchorKeyDown = (e: KeyboardEvent) => { if (e.key === 'Tab' && elementRef.current) { @@ -353,12 +349,7 @@ function PieSliceWithDataLinks({ ); return ( - + {(api) => ( )} - + ); } @@ -383,7 +373,6 @@ function PieSlice({ pie, highlightState, openMenu, - getMenuPosition, fill, tooltip, tooltipOptions, @@ -455,12 +444,12 @@ function PieSlice({ }); } }, - [publishDataHoverEvent, tooltip, pie, tooltipOptions] + [publishDataHoverEvent, tooltip, pie, tooltipOptions, arc] ); const handleKeyDown = useCallback( (event: React.KeyboardEvent) => { - if (hasDataLinks && event.key === 'Enter') { + if (hasDataLinks && (event.key === 'Enter' || event.key === ' ')) { event.preventDefault(); event.stopPropagation(); @@ -480,21 +469,12 @@ function PieSlice({ y: svgRect.top + svgRect.height / 2 + centerY + window.scrollY, }; - // Use the updated API that supports position objects - if (typeof openMenu === 'function' && openMenu.length === 1) { - openMenu(position); - } else if (getMenuPosition && elementRef.current) { - // Fallback: use getMenuPosition if available - const calculatedPosition = getMenuPosition(elementRef.current); - if (typeof openMenu === 'function') { - openMenu(calculatedPosition); - } - } + openMenu(position); } } } }, - [hasDataLinks, openMenu, getMenuPosition, arc, outerRadius, innerRadius] + [hasDataLinks, openMenu, arc, outerRadius, innerRadius, elementRef] ); const handleFocus = useCallback( @@ -527,7 +507,7 @@ function PieSlice({ blurTimeoutRef.current = null; }, 100); }, - [publishDataHoverClearEvent] + [publishDataHoverClearEvent, elementRef] ); useEffect(() => {