From b3c960f854216704a43f07c40287029c749f6e8e Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Tue, 4 Nov 2025 10:09:35 +0100 Subject: [PATCH 1/2] fix: Fixes chart popover click outside when in iframe --- pages/app/index.tsx | 2 +- pages/app/templates.tsx | 11 ++- ...de-panel.page.tsx => charts.test.page.tsx} | 79 +++++++++++++------ .../__integ__/chart-popover.test.ts | 30 +++++++ .../__tests__/chart-popover.test.tsx | 74 +++++++++++++++++ .../components/chart-popover/index.tsx | 39 +++++---- src/internal/utils/dom.ts | 4 + src/mixed-line-bar-chart/chart-container.tsx | 7 +- src/popover/internal.tsx | 2 +- 9 files changed, 197 insertions(+), 51 deletions(-) rename pages/{charts-with-side-panel.page.tsx => charts.test.page.tsx} (54%) create mode 100644 src/internal/components/chart-popover/__integ__/chart-popover.test.ts create mode 100644 src/internal/components/chart-popover/__tests__/chart-popover.test.tsx diff --git a/pages/app/index.tsx b/pages/app/index.tsx index 84e4d54467..26895fc272 100644 --- a/pages/app/index.tsx +++ b/pages/app/index.tsx @@ -50,7 +50,7 @@ function isAppLayoutPage(pageId?: string) { 'prompt-input/simple', 'funnel-analytics/static-single-page-flow', 'funnel-analytics/static-multi-page-flow', - 'charts-with-side-panel', + 'charts.test', ]; return pageId !== undefined && appLayoutPages.some(match => pageId.includes(match)); } diff --git a/pages/app/templates.tsx b/pages/app/templates.tsx index 8d93b629da..b92d568cbe 100644 --- a/pages/app/templates.tsx +++ b/pages/app/templates.tsx @@ -7,6 +7,7 @@ import { Box, SpaceBetween } from '~components'; import I18nProvider, { I18nProviderProps } from '~components/i18n'; import messages from '~components/i18n/messages/all.en'; +import { IframeWrapper } from '../utils/iframe-wrapper'; import ScreenshotArea, { ScreenshotAreaProps } from '../utils/screenshot-area'; interface SimplePageProps { @@ -16,10 +17,11 @@ interface SimplePageProps { children: React.ReactNode; screenshotArea?: ScreenshotAreaProps; i18n?: Partial; + iframe?: { id?: string }; } -export function SimplePage({ title, subtitle, settings, children, screenshotArea, i18n }: SimplePageProps) { - const content = ( +export function SimplePage({ title, subtitle, settings, children, screenshotArea, i18n, iframe }: SimplePageProps) { + let content = ( @@ -44,13 +46,16 @@ export function SimplePage({ title, subtitle, settings, children, screenshotArea ); - return i18n ? ( + + content = i18n ? ( {content} ) : ( content ); + + return iframe ? <>{content}} /> : content; } export function PermutationsPage({ screenshotArea = {}, ...props }: SimplePageProps) { diff --git a/pages/charts-with-side-panel.page.tsx b/pages/charts.test.page.tsx similarity index 54% rename from pages/charts-with-side-panel.page.tsx rename to pages/charts.test.page.tsx index e466a2fffc..7d86902f89 100644 --- a/pages/charts-with-side-panel.page.tsx +++ b/pages/charts.test.page.tsx @@ -1,10 +1,21 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import React, { useState } from 'react'; +import React, { useContext, useState } from 'react'; -import { AppLayout, Button, MixedLineBarChartProps, PieChart, SpaceBetween, SplitPanel } from '~components'; +import { + AppLayout, + Box, + Button, + Checkbox, + Container, + MixedLineBarChartProps, + PieChart, + SpaceBetween, + SplitPanel, +} from '~components'; import LineChart from '~components/line-chart'; +import AppContext, { AppContextType } from './app/app-context'; import { SimplePage } from './app/templates'; import labels from './app-layout/utils/labels'; import { splitPaneli18nStrings } from './app-layout/utils/strings'; @@ -17,12 +28,22 @@ const linearLatencyProps = createLinearTimeLatencyProps(); type ExpectedSeries = MixedLineBarChartProps.LineDataSeries | MixedLineBarChartProps.ThresholdSeries; +type PageContext = React.Context< + AppContextType<{ + iframe?: boolean; + }> +>; + const series: ReadonlyArray = [ { title: 'Series 1', type: 'line', data: lineData }, { title: 'Threshold', type: 'threshold', y: 150 }, ]; export default function () { + const { + urlParams: { iframe = false }, + setUrlParams, + } = useContext(AppContext as PageContext); const [splitPanelSize, setSplitPanelSize] = useState(300); const [sidePanelVisible, setSidePanelVisible] = useState(false); const toggleButton = ; @@ -45,33 +66,41 @@ export default function () { title="Line chart with side panel demo" subtitle="Open side panel from chart's popover. The popover's position should be updated." screenshotArea={{}} + iframe={iframe ? {} : undefined} + settings={ + + setUrlParams({ iframe: detail.checked })}> + In iframe + + + } > - toggleButton} - /> + Line chart}> + toggleButton} + /> + - toggleButton} - /> + toggleButton} /> - toggleButton} - /> + Pie chart}> + toggleButton} + /> + } diff --git a/src/internal/components/chart-popover/__integ__/chart-popover.test.ts b/src/internal/components/chart-popover/__integ__/chart-popover.test.ts new file mode 100644 index 0000000000..5eb4342ad6 --- /dev/null +++ b/src/internal/components/chart-popover/__integ__/chart-popover.test.ts @@ -0,0 +1,30 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { BasePageObject } from '@cloudscape-design/browser-test-tools/page-objects'; +import useBrowser from '@cloudscape-design/browser-test-tools/use-browser'; + +import createWrapper from '../../../../../lib/components/test-utils/selectors'; + +test.each([false, true])('can be unpinned by clicking outside, iframe=%s', async iframe => { + await useBrowser(async browser => { + const page = new BasePageObject(browser); + await browser.url(`#/light/charts.test?iframe=${iframe}`); + await page.runInsideIframe('#content-iframe', iframe, async () => { + const chart = createWrapper().findLineChart(); + const popover = chart.findDetailPopover(); + const popoverDismiss = popover.findDismissButton(); + + // Show popover with an offset from chart's center. + await page.hoverElement(chart.toSelector(), 100); + // Pin the popover by clicking on the left from it to ensure the click lands on the SVG, not popover content. + await page.click(chart.toSelector()); + await page.waitForAssertion(() => expect(page.isDisplayed(popoverDismiss.toSelector())).resolves.toBe(true)); + await page.waitForAssertion(() => expect(page.isFocused(popoverDismiss.toSelector())).resolves.toBe(true)); + + // Unpin by clicking outside the chart. + await page.click('h2'); + await expect(page.isDisplayed(popover.toSelector())).resolves.toBe(false); + }); + })(); +}); diff --git a/src/internal/components/chart-popover/__tests__/chart-popover.test.tsx b/src/internal/components/chart-popover/__tests__/chart-popover.test.tsx new file mode 100644 index 0000000000..949cf242bf --- /dev/null +++ b/src/internal/components/chart-popover/__tests__/chart-popover.test.tsx @@ -0,0 +1,74 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import React from 'react'; +import { act, render } from '@testing-library/react'; + +import ChartPopover from '../../../../../lib/components/internal/components/chart-popover'; + +describe('click outside', () => { + function TestComponent({ onDismiss }: { onDismiss: () => void }) { + return ( +
+
x
+
x
+ +
x
+ +
+ ); + } + + function nextFrame() { + return act(async () => { + await new Promise(resolve => requestAnimationFrame(resolve)); + }); + } + + // We render the component twice to ensure the container reference is set correctly. + function renderPopover({ onDismiss }: { onDismiss: () => void }) { + const { rerender } = render(); + rerender(); + } + + test('calls popover dismiss on outside click', () => { + const onDismiss = jest.fn(); + renderPopover({ onDismiss }); + + document.querySelector('#outside')!.dispatchEvent(new Event('mousedown', { bubbles: true })); + expect(onDismiss).toHaveBeenCalledWith(true); + }); + + test('does not call popover dismiss when clicked inside container', () => { + const onDismiss = jest.fn(); + renderPopover({ onDismiss }); + + document.querySelector('#container')!.dispatchEvent(new Event('mousedown', { bubbles: true })); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + test('does not call popover dismiss when clicked inside popover', () => { + const onDismiss = jest.fn(); + renderPopover({ onDismiss }); + + document.querySelector('#content')!.dispatchEvent(new Event('mousedown', { bubbles: true })); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + test('calls popover dismiss when clicked inside popover and then outside', async () => { + const onDismiss = jest.fn(); + renderPopover({ onDismiss }); + + document.querySelector('#content')!.dispatchEvent(new Event('mousedown', { bubbles: true })); + expect(onDismiss).not.toHaveBeenCalled(); + + await nextFrame(); + + document.querySelector('#outside')!.dispatchEvent(new Event('mousedown', { bubbles: true })); + expect(onDismiss).toHaveBeenCalledWith(true); + }); +}); diff --git a/src/internal/components/chart-popover/index.tsx b/src/internal/components/chart-popover/index.tsx index 13a6c7c91e..47757a6985 100644 --- a/src/internal/components/chart-popover/index.tsx +++ b/src/internal/components/chart-popover/index.tsx @@ -10,7 +10,6 @@ import PopoverBody from '../../../popover/body'; import PopoverContainer from '../../../popover/container'; import { PopoverProps } from '../../../popover/interfaces'; import { getBaseProps } from '../../base-component'; -import { nodeBelongs } from '../../utils/node-belongs'; import popoverStyles from '../../../popover/styles.css.js'; import styles from './styles.css.js'; @@ -87,24 +86,31 @@ function ChartPopover( ) { const baseProps = getBaseProps(restProps); const popoverObjectRef = useRef(null); - const popoverRef = useMergeRefs(popoverObjectRef, ref); + const clickFrameId = useRef(null); + const onMouseDown = () => { + // Indicate there was a click inside popover recently. + clickFrameId.current = requestAnimationFrame(() => (clickFrameId.current = null)); + }; + useEffect(() => { - const onDocumentClick = (event: MouseEvent) => { - if ( - event.target && - !nodeBelongs(popoverObjectRef.current, event.target as Element) && // click not in popover - !nodeContains(container, event.target as Element) // click not in segment - ) { - onDismiss(true); - } - }; - - document.addEventListener('mousedown', onDocumentClick, { capture: true }); - return () => { - document.removeEventListener('mousedown', onDocumentClick, { capture: true }); - }; + if (popoverObjectRef.current) { + const document = popoverObjectRef.current.ownerDocument; + const onDocumentClick = (event: MouseEvent) => { + // Dismiss popover unless there was a click inside within the last animation frame. + // Ignore clicks inside the chart as those are handled separately. + if (clickFrameId.current === null && !nodeContains(container, event.target as Element)) { + onDismiss(true); + } + }; + + document.addEventListener('mousedown', onDocumentClick); + + return () => { + document.removeEventListener('mousedown', onDocumentClick); + }; + } }, [container, onDismiss]); // In chart popovers, dismiss button is present when they are pinned, so both values are equivalent. @@ -117,6 +123,7 @@ function ChartPopover( ref={popoverRef} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave} + onMouseDown={onMouseDown} onBlur={onBlur} // The tabIndex makes it so that clicking inside popover assigns this element as blur target. // That is necessary in charts to ensure the blur target is within the chart and no cleanup is needed. diff --git a/src/internal/utils/dom.ts b/src/internal/utils/dom.ts index 85cf134baa..b0d300d348 100644 --- a/src/internal/utils/dom.ts +++ b/src/internal/utils/dom.ts @@ -94,3 +94,7 @@ export function isSVGElement(target: unknown): target is SVGElement { typeof target.ownerSVGElement === 'object') ); } + +export function isElement(target: unknown): target is Element { + return isHTMLElement(target) || isSVGElement(target); +} diff --git a/src/mixed-line-bar-chart/chart-container.tsx b/src/mixed-line-bar-chart/chart-container.tsx index 095c657684..5d3beb65c7 100644 --- a/src/mixed-line-bar-chart/chart-container.tsx +++ b/src/mixed-line-bar-chart/chart-container.tsx @@ -19,6 +19,7 @@ import VerticalMarker from '../internal/components/cartesian-chart/vertical-mark import ChartPlot, { ChartPlotRef } from '../internal/components/chart-plot'; import { useHeightMeasure } from '../internal/hooks/container-queries/use-height-measure'; import { useVisualRefresh } from '../internal/hooks/use-visual-mode'; +import { isElement } from '../internal/utils/dom'; import { nodeBelongs } from '../internal/utils/node-belongs'; import useContainerWidth from '../internal/utils/use-container-width'; import BarGroups from './bar-groups'; @@ -391,11 +392,7 @@ export default function ChartContainer({ const onApplicationBlur = (event: React.FocusEvent) => { const blurTarget = event.relatedTarget || event.target; - if ( - blurTarget === null || - !(blurTarget instanceof Element) || - !nodeBelongs(containerRefObject.current, blurTarget) - ) { + if (blurTarget === null || !isElement(blurTarget) || !nodeBelongs(containerRefObject.current, blurTarget)) { clearHighlightedSeries(); setVerticalMarkerX(null); diff --git a/src/popover/internal.tsx b/src/popover/internal.tsx index c14a384500..2ab11bf790 100644 --- a/src/popover/internal.tsx +++ b/src/popover/internal.tsx @@ -61,7 +61,6 @@ function InternalPopover( const baseProps = getBaseProps(restProps); const triggerRef = useRef(null); const popoverRef = useRef(null); - const clickFrameId = useRef(null); const i18n = useInternalI18n('popover'); const dismissAriaLabel = i18n('dismissAriaLabel', restProps.dismissAriaLabel); @@ -110,6 +109,7 @@ function InternalPopover( }, })); + const clickFrameId = useRef(null); useEffect(() => { if (!triggerRef.current) { return; From 71e792d3931be9a9251b30095dc56d82604356d0 Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Tue, 4 Nov 2025 15:24:30 +0100 Subject: [PATCH 2/2] fix integ test def --- .../__integ__/chart-popover.test.ts | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/internal/components/chart-popover/__integ__/chart-popover.test.ts b/src/internal/components/chart-popover/__integ__/chart-popover.test.ts index 5eb4342ad6..2f721d4acf 100644 --- a/src/internal/components/chart-popover/__integ__/chart-popover.test.ts +++ b/src/internal/components/chart-popover/__integ__/chart-popover.test.ts @@ -6,25 +6,26 @@ import useBrowser from '@cloudscape-design/browser-test-tools/use-browser'; import createWrapper from '../../../../../lib/components/test-utils/selectors'; -test.each([false, true])('can be unpinned by clicking outside, iframe=%s', async iframe => { - await useBrowser(async browser => { - const page = new BasePageObject(browser); - await browser.url(`#/light/charts.test?iframe=${iframe}`); - await page.runInsideIframe('#content-iframe', iframe, async () => { - const chart = createWrapper().findLineChart(); - const popover = chart.findDetailPopover(); - const popoverDismiss = popover.findDismissButton(); +describe.each([false, true])('iframe=%s', iframe => { + test( + 'can be unpinned by clicking outside', + useBrowser(async browser => { + const page = new BasePageObject(browser); + await browser.url(`#/light/charts.test?iframe=${iframe}`); + await page.runInsideIframe('#content-iframe', iframe, async () => { + const chart = createWrapper().findLineChart(); + const popover = chart.findDetailPopover(); + const popoverDismiss = popover.findDismissButton(); - // Show popover with an offset from chart's center. - await page.hoverElement(chart.toSelector(), 100); - // Pin the popover by clicking on the left from it to ensure the click lands on the SVG, not popover content. - await page.click(chart.toSelector()); - await page.waitForAssertion(() => expect(page.isDisplayed(popoverDismiss.toSelector())).resolves.toBe(true)); - await page.waitForAssertion(() => expect(page.isFocused(popoverDismiss.toSelector())).resolves.toBe(true)); + // Pins popover on the first point. + await page.click('h2'); + await page.keys(['Tab', 'ArrowRight', 'Enter']); + await page.waitForVisible(popoverDismiss.toSelector()); - // Unpin by clicking outside the chart. - await page.click('h2'); - await expect(page.isDisplayed(popover.toSelector())).resolves.toBe(false); - }); - })(); + // Unpin by clicking outside the chart. + await page.click('h1'); + await expect(page.isDisplayed(popover.toSelector())).resolves.toBe(false); + }); + }) + ); });