fix: Fixes chart popover click outside when in iframe (#4005)

Author: pan-kot

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));
 }

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<I18nProviderProps>;
+  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 = (
     <Box margin="m">
       <SpaceBetween size="m">
         <SpaceBetween size="xs">
@@ -44,13 +46,16 @@ export function SimplePage({ title, subtitle, settings, children, screenshotArea
       </SpaceBetween>
     </Box>
   );
-  return i18n ? (
+
+  content = i18n ? (
     <I18nProvider messages={[messages]} locale="en-GB" {...i18n}>
       {content}
     </I18nProvider>
   ) : (
     content
   );
+
+  return iframe ? <IframeWrapper id={iframe.id ?? 'content-iframe'} AppComponent={() => <>{content}</>} /> : content;
 }
 
 export function PermutationsPage({ screenshotArea = {}, ...props }: SimplePageProps) {

pages/charts-with-side-panel.page.tsx renamed to 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<number> | MixedLineBarChartProps.ThresholdSeries;
 
+type PageContext = React.Context<
+  AppContextType<{
+    iframe?: boolean;
+  }>
+>;
+
 const series: ReadonlyArray<ExpectedSeries> = [
   { 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 = <Button onClick={() => setSidePanelVisible(prev => !prev)}>Toggle side panel</Button>;
@@ -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={
+            <SpaceBetween size="s" direction="horizontal">
+              <Checkbox checked={iframe} onChange={({ detail }) => setUrlParams({ iframe: detail.checked })}>
+                In iframe
+              </Checkbox>
+            </SpaceBetween>
+          }
         >
           <SpaceBetween size="m">
-            <LineChart
-              {...commonLineProps}
-              hideFilter={true}
-              height={200}
-              series={series}
-              xTitle="Time"
-              yTitle="Latency (ms)"
-              xScaleType="linear"
-              ariaLabel="Line chart"
-              detailPopoverFooter={() => toggleButton}
-            />
+            <Container header={<Box variant="h2">Line chart</Box>}>
+              <LineChart
+                {...commonLineProps}
+                hideFilter={true}
+                height={200}
+                series={series}
+                xTitle="Time"
+                yTitle="Latency (ms)"
+                xScaleType="linear"
+                ariaLabel="Line chart"
+                detailPopoverFooter={() => toggleButton}
+              />
+            </Container>
 
-            <AreaChartExample
-              name="Linear latency chart"
-              {...linearLatencyProps}
-              detailPopoverFooter={() => toggleButton}
-            />
+            <AreaChartExample name="Area chart" {...linearLatencyProps} detailPopoverFooter={() => toggleButton} />
 
-            <PieChart
-              {...commonPieProps}
-              data={pieData}
-              ariaLabel="Food facts"
-              size="medium"
-              detailPopoverFooter={() => toggleButton}
-            />
+            <Container header={<Box variant="h2">Pie chart</Box>}>
+              <PieChart
+                {...commonPieProps}
+                data={pieData}
+                ariaLabel="Food facts"
+                size="medium"
+                detailPopoverFooter={() => toggleButton}
+              />
+            </Container>
           </SpaceBetween>
         </SimplePage>
       }

src/internal/components/chart-popover/__integ__/chart-popover.test.ts

@@ -0,0 +1,31 @@
+// 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';
+
+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();
+
+        // 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('h1');
+        await expect(page.isDisplayed(popover.toSelector())).resolves.toBe(false);
+      });
+    })
+  );
+});

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 (
+      <div>
+        <div id="outside">x</div>
+        <div id="container">x</div>
+        <ChartPopover
+          trackRef={{ current: null }}
+          container={document.querySelector('#container')}
+          onDismiss={onDismiss}
+        >
+          <div id="content">x</div>
+        </ChartPopover>
+      </div>
+    );
+  }
+
+  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(<TestComponent onDismiss={onDismiss} />);
+    rerender(<TestComponent onDismiss={onDismiss} />);
+  }
+
+  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);
+  });
+});

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<HTMLDivElement | null>(null);
-
   const popoverRef = useMergeRefs(popoverObjectRef, ref);
 
+  const clickFrameId = useRef<number | null>(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.

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);
+}

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<T extends ChartDataTypes>({
 
   const onApplicationBlur = (event: React.FocusEvent<Element>) => {
     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);
 

src/popover/internal.tsx

@@ -61,7 +61,6 @@ function InternalPopover(
   const baseProps = getBaseProps(restProps);
   const triggerRef = useRef<HTMLElement | null>(null);
   const popoverRef = useRef<HTMLDivElement | null>(null);
-  const clickFrameId = useRef<number | null>(null);
 
   const i18n = useInternalI18n('popover');
   const dismissAriaLabel = i18n('dismissAriaLabel', restProps.dismissAriaLabel);
@@ -110,6 +109,7 @@ function InternalPopover(
     },
   }));
 
+  const clickFrameId = useRef<number | null>(null);
   useEffect(() => {
     if (!triggerRef.current) {
       return;