Skip to content

fix: return focus to legend item on Tab when tooltip is visible #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
albadra2 wants to merge 3 commits into
base: main
Choose a base branch
from
+94 −46
Open

fix: return focus to legend item on Tab when tooltip is visible #217

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fa30a1
fix: return focus to legend item on Tab when tooltip is visible
albadra2 May 6, 2026
364bcc1
Replace Tab interception with dismiss-button approach and fix SingleT…
albadra2 May 8, 2026
b0df621
Rework: unconditional dismiss button with invisible tab trap per review
albadra2 May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 94 additions & 46 deletions src/internal/components/chart-legend/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ const TOOLTIP_BLUR_DELAY = 50;
const HIGHLIGHT_LOST_DELAY = 50;
const SCROLL_DELAY = 100;

function focusStaysInTooltipScope(
next: EventTarget | null,
tooltipEl: HTMLElement | null,
triggerEl: HTMLElement | undefined,
): boolean {
if (!next) {
return false;
}
const node = next as Node;
return !!(tooltipEl?.contains(node) || triggerEl?.contains(node));
}

export type LegendAlignment = "horizontal" | "vertical";
export type LegendHorizontalAlignment = "start" | "center" | "end";

Expand Down Expand Up @@ -70,6 +82,7 @@ export const ChartLegend = ({
const elementsByIndexRef = useRef<Record<number, HTMLElement>>([]);
const elementsByIdRef = useRef<Record<string, HTMLElement>>({});
const tooltipRef = useRef<HTMLElement>(null);
const tabTrapRef = useRef<HTMLDivElement>(null);
const highlightControl = useMemo(() => new DebouncedCall(), []);
const scrollIntoViewControl = useMemo(() => new DebouncedCall(), []);
const [selectedIndex, setSelectedIndex] = useState<number>(0);
Expand Down Expand Up @@ -103,7 +116,17 @@ export const ChartLegend = ({
return () => {
document.removeEventListener("keydown", onDocumentKeyDown, true);
};
}, [items, tooltipItemId, hideTooltip]);
}, [tooltipItemId, hideTooltip]);

// Workaround: PopoverBody auto-focuses the dismiss button on mount.
// We re-focus the legend trigger here, relying on React's child-before-parent effect ordering.
// Remove this once InternalChartTooltip supports a `disableAutoFocus` prop.
useEffect(() => {
if (tooltipItemId) {
elementsByIdRef.current[tooltipItemId]?.focus({ preventScroll: true });
Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shaky as the auto-focus can theoretically happen after we re-focus the item (e.g. in certain browsers). Also, some screen-readers might be quick enough to announce the dismiss button if it was focused for a split-second before the focus was restored. A proper change, as per the comments, is to add the ability to disable the built-in auto-focus. Let's do it.

Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR for the tooltip: cloudscape-design/components#4510

}
}, [tooltipItemId]);

const isMouseInContainer = useRef<boolean>(false);

// Scrolling to the highlighted legend item.
Expand Down Expand Up @@ -147,8 +170,15 @@ export const ChartLegend = ({
function onBlur(event: React.FocusEvent) {
navigationAPI.current!.updateFocusTarget();

// Hide tooltip and clear highlight unless focus moves inside tooltip;
if (tooltipRef.current && event.relatedTarget && !tooltipRef.current.contains(event.relatedTarget)) {
// Hide tooltip and clear highlight unless focus moves inside tooltip or to the tab trap;
Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wrap the tooltip and its tab traps in a div and check that div instead?

<div ref={tooltipWrapperRef}>
  <div tabIndex={0} />
  <Tooltip ... />
  <div tabIndex={0} />
</div>

const next = event.relatedTarget as Node | null;
if (next && tooltipRef.current?.contains(next)) {
return;
}
if (next && tabTrapRef.current?.contains(next)) {
return;
}
if (next) {
clearHighlight();
hideTooltip();
}
Expand Down Expand Up @@ -217,25 +247,25 @@ export const ChartLegend = ({
const tooltipPosition = isVertical ? "left" : "bottom";

return (
<SingleTabStopNavigationProvider
ref={navigationAPI}
navigationActive={true}
getNextFocusTarget={() => getNextFocusTarget()}
onUnregisterActive={(element: HTMLElement) => onUnregisterActive(element, navigationAPI)}
<div
role="toolbar"
aria-label={legendTitle || ariaLabel}
className={clsx(testClasses.root, styles.root, className)}
data-axisid={axisId}
onMouseEnter={() => (isMouseInContainer.current = true)}
onMouseLeave={() => (isMouseInContainer.current = false)}
>
<div
role="toolbar"
aria-label={legendTitle || ariaLabel}
className={clsx(testClasses.root, styles.root, className)}
data-axisid={axisId}
onMouseEnter={() => (isMouseInContainer.current = true)}
onMouseLeave={() => (isMouseInContainer.current = false)}
{legendTitle && (
<Box fontWeight="bold" className={testClasses.title} textAlign={getBoxTextAlignment(horizontalAlignment)}>
{legendTitle}
</Box>
)}
<SingleTabStopNavigationProvider
ref={navigationAPI}
navigationActive={true}
getNextFocusTarget={() => getNextFocusTarget()}
onUnregisterActive={(element: HTMLElement) => onUnregisterActive(element, navigationAPI)}
>
{legendTitle && (
<Box fontWeight="bold" className={testClasses.title} textAlign={getBoxTextAlignment(horizontalAlignment)}>
{legendTitle}
</Box>
)}
<div
// The list element is not focusable. However, the focus lands on it regardless, when testing in Firefox.
// Setting the tab index to -1 does fix the problem.
Expand Down Expand Up @@ -306,33 +336,51 @@ export const ChartLegend = ({
);
})}
</div>
{tooltipContent && (
<InternalChartTooltip
trackRef={tooltipTrack}
triggerClampRef={containerRef}
trackKey={tooltipTarget.id}
container={null}
dismissButton={false}
onDismiss={() => {}}
position={tooltipPosition}
title={tooltipContent.header}
onMouseEnter={() => showTooltip(tooltipTarget.id)}
onMouseLeave={() => hideTooltip()}
onBlur={() => hideTooltip()}
footer={
tooltipContent.footer && (
<>
<hr aria-hidden={true} />
{tooltipContent.footer}
</>
)
</SingleTabStopNavigationProvider>
{tooltipContent && (
<div
ref={tabTrapRef}
tabIndex={0}
onFocus={() => tooltipRef.current?.querySelector<HTMLElement>("button")?.focus()}
Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use getAllFocusables()[0]?.focus() instead - to make the code better general and better readable at the same time.

style={{ position: "absolute", width: 0, height: 0, overflow: "hidden" }}
Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these styles needed?

/>
)}
{tooltipContent && (
<InternalChartTooltip
ref={tooltipRef}
trackRef={tooltipTrack}
triggerClampRef={containerRef}
trackKey={tooltipTarget.id}
container={null}
dismissButton={true}
onDismiss={() => {
hideTooltip(true);
elementsByIdRef.current[tooltipTarget.id]?.focus();
}}
position={tooltipPosition}
title={tooltipContent.header}
onMouseEnter={() => showTooltip(tooltipTarget.id)}
onMouseLeave={() => hideTooltip()}
onBlur={(event) => {
const trigger = elementsByIdRef.current[tooltipTarget.id];
if (focusStaysInTooltipScope(event.relatedTarget, tooltipRef.current, trigger)) {
Copy link
Copy Markdown
Member

@pan-kot pan-kot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use exit tab trap instead? There is already a tab trap (an invisible element with tabIndex=0) before the tooltip - we can also add one after it, so that every time it gets focused - the focus is goes to back to the dismiss button.

return;
}
>
{tooltipContent.body}
</InternalChartTooltip>
)}
</div>
</SingleTabStopNavigationProvider>
hideTooltip();
}}
footer={
tooltipContent.footer && (
<>
<hr aria-hidden={true} />
{tooltipContent.footer}
</>
)
}
>
{tooltipContent.body}
</InternalChartTooltip>
)}
</div>
);
};

Expand Down
Loading