From e0e967ea413ef90903aa8b28191fe7f7c1b4ae94 Mon Sep 17 00:00:00 2001 From: Mayank <9084735+mayank99@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:04:36 -0400 Subject: [PATCH] portal `PortalContainer` into `` (#2185) --- .changeset/afraid-fans-push.md | 5 ++++ apps/website/src/content/docs/popover.mdx | 2 +- .../src/content/docs/themeprovider.mdx | 2 +- apps/website/src/content/docs/tooltip.mdx | 2 +- .../src/core/Popover/Popover.test.tsx | 6 +++-- .../src/core/ThemeProvider/ThemeProvider.tsx | 7 +++--- .../src/utils/components/Portal.test.tsx | 19 +++++++++------ testing/e2e/app/routes/ThemeProvider/spec.ts | 23 +++++++++++-------- 8 files changed, 41 insertions(+), 25 deletions(-) create mode 100644 .changeset/afraid-fans-push.md diff --git a/.changeset/afraid-fans-push.md b/.changeset/afraid-fans-push.md new file mode 100644 index 00000000000..cfe40eae6c9 --- /dev/null +++ b/.changeset/afraid-fans-push.md @@ -0,0 +1,5 @@ +--- +'@itwin/itwinui-react': patch +--- + +Portal containers will now default to a `
` rendered at the end of `` instead of a `
` rendered inside the ``. diff --git a/apps/website/src/content/docs/popover.mdx b/apps/website/src/content/docs/popover.mdx index ac79c35285f..f650624d742 100644 --- a/apps/website/src/content/docs/popover.mdx +++ b/apps/website/src/content/docs/popover.mdx @@ -40,7 +40,7 @@ There are some advanced positioning options available. ### Portals -It is important to know that before calculating the position, the popover gets [portaled](https://react.dev/reference/react-dom/createPortal) into the nearest `ThemeProvider` to avoid [stacking context](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/Understanding_z-index/Stacking_context) issues. This behavior can be controlled using the Popover's `portal` prop or the ThemeProvider's `portalContainer` prop. Using portals can often lead to issues with keyboard accessibility, so Popover adds some additional logic (described below). +It is important to know that before calculating the position, the popover gets [portaled](https://react.dev/reference/react-dom/createPortal) to the end of `` to avoid [stacking context](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/Understanding_z-index/Stacking_context) issues. This behavior can be controlled using the Popover's `portal` prop or the ThemeProvider's `portalContainer` prop. Using portals can often lead to issues with keyboard accessibility, so Popover adds some additional logic (described below). ## Accessibility diff --git a/apps/website/src/content/docs/themeprovider.mdx b/apps/website/src/content/docs/themeprovider.mdx index a3b4b82a92f..196248b74f5 100644 --- a/apps/website/src/content/docs/themeprovider.mdx +++ b/apps/website/src/content/docs/themeprovider.mdx @@ -108,7 +108,7 @@ const Application = () => { ## Portals -By default, `ThemeProvider` will also be re-used as a [portal](https://react.dev/reference/react-dom/createPortal) target for floating elements (e.g. [`Tooltip`](tooltip), [`Toast`](toast), [`DropdownMenu`](dropdownmenu), [`Dialog`](dialog), etc). +By default, `ThemeProvider` will use an element at the end of `` as the [portal](https://react.dev/reference/react-dom/createPortal) target for floating elements (e.g. [`Tooltip`](tooltip), [`Toast`](toast), [`DropdownMenu`](dropdownmenu), [`Dialog`](dialog), etc). If you want to specify a different element as the portal target for all components within a tree, the `portalContainer` prop can be used. diff --git a/apps/website/src/content/docs/tooltip.mdx b/apps/website/src/content/docs/tooltip.mdx index fe55b1379f2..23ba9060e2d 100644 --- a/apps/website/src/content/docs/tooltip.mdx +++ b/apps/website/src/content/docs/tooltip.mdx @@ -37,7 +37,7 @@ There are some advanced props available for more granular control over positioni ### Portals -It is important to know that before calculating the position, the tooltip gets [portaled](https://react.dev/reference/react-dom/createPortal) into the nearest [`ThemeProvider`](themeprovider). This is done to avoid [stacking context](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/Understanding_z-index/Stacking_context) issues in browsers where the [`popover` API](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API) is not supported. This portaling behavior can be controlled using the Tooltip's `portal` prop or the ThemeProvider's [`portalContainer`](themeprovider#portals) prop. +It is important to know that before calculating the position, the tooltip gets [portaled](https://react.dev/reference/react-dom/createPortal) to the end of ``. This is done to avoid [stacking context](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/Understanding_z-index/Stacking_context) issues in browsers where the [`popover` API](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API) is not supported. This portaling behavior can be controlled using the Tooltip's `portal` prop or the ThemeProvider's [`portalContainer`](themeprovider#portals) prop. ## Accessibility diff --git a/packages/itwinui-react/src/core/Popover/Popover.test.tsx b/packages/itwinui-react/src/core/Popover/Popover.test.tsx index d2559f9690c..c4a599d395f 100644 --- a/packages/itwinui-react/src/core/Popover/Popover.test.tsx +++ b/packages/itwinui-react/src/core/Popover/Popover.test.tsx @@ -67,6 +67,8 @@ it('should portal to within the ThemeProvider', async () => { expect(screen.getByText('Popped over')).toBeVisible(); const root = document.querySelector('.the-root') as HTMLElement; - const popover = root.querySelector('.the-popover'); - expect(popover).toBeVisible(); + expect(root.querySelector('.the-popover')).toBeNull(); + expect( + document.body.querySelector('body > [data-iui-portal] .the-popover'), + ).toBeVisible(); }); diff --git a/packages/itwinui-react/src/core/ThemeProvider/ThemeProvider.tsx b/packages/itwinui-react/src/core/ThemeProvider/ThemeProvider.tsx index 5a83584cae2..3c16e9c6ecf 100644 --- a/packages/itwinui-react/src/core/ThemeProvider/ThemeProvider.tsx +++ b/packages/itwinui-react/src/core/ThemeProvider/ThemeProvider.tsx @@ -376,8 +376,8 @@ const PortalContainer = React.memo( return null; } - if (shouldSetupPortalContainer) { - return ( + if (shouldSetupPortalContainer && ownerDocument) { + return ReactDOM.createPortal( - + , + ownerDocument.body, ); } else if (portalContainerProp) { return ReactDOM.createPortal(, portalContainerProp); diff --git a/packages/itwinui-react/src/utils/components/Portal.test.tsx b/packages/itwinui-react/src/utils/components/Portal.test.tsx index 3c56a8afd16..03c89dc20d3 100644 --- a/packages/itwinui-react/src/utils/components/Portal.test.tsx +++ b/packages/itwinui-react/src/utils/components/Portal.test.tsx @@ -17,9 +17,10 @@ it('should work', () => { ); expect(document.querySelector('main')).toBeEmptyDOMElement(); - expect( - screen.getByTestId('root').querySelector(':scope > div'), - ).toHaveTextContent('thing'); + expect(screen.getByTestId('root')).not.toHaveTextContent('thing'); + expect(document.querySelector('body > [data-iui-portal]')).toHaveTextContent( + 'thing', + ); }); it('should allow turning off', () => { @@ -32,9 +33,9 @@ it('should allow turning off', () => { ); expect(document.querySelector('main')).toHaveTextContent('thing'); - expect( - screen.getByTestId('root').querySelector(':scope > div'), - ).toHaveTextContent(''); + expect(document.querySelector('[data-iui-portal]')).not.toHaveTextContent( + 'thing', + ); }); it('should accept an element', () => { @@ -48,6 +49,9 @@ it('should accept an element', () => { expect(document.querySelector('main')).toBeEmptyDOMElement(); expect(screen.getByTestId('root')).toHaveTextContent(''); + expect( + document.querySelector('body > [data-iui-portal]'), + ).not.toHaveTextContent('thing'); expect(document.body).toHaveTextContent('thing'); }); @@ -79,8 +83,9 @@ it.each([null, undefined, () => null, () => undefined])( ); expect(document.querySelector('main')).toBeEmptyDOMElement(); + expect(screen.getByTestId('root')).not.toHaveTextContent('thing'); expect( - screen.getByTestId('root').querySelector(':scope > div'), + document.querySelector('body > [data-iui-portal]'), ).toHaveTextContent('thing'); }, ); diff --git a/testing/e2e/app/routes/ThemeProvider/spec.ts b/testing/e2e/app/routes/ThemeProvider/spec.ts index 393008f2ff8..e6ca65a4811 100644 --- a/testing/e2e/app/routes/ThemeProvider/spec.ts +++ b/testing/e2e/app/routes/ThemeProvider/spec.ts @@ -13,15 +13,15 @@ test('should inherit the portalContainer if inheriting theme', async ({ }) => { await page.goto('/ThemeProvider?nested=true'); - await expect(page.locator('[data-container="main"]')).toContainText( - 'main tooltip', - ); - await expect(page.locator('[data-container="main"]')).toContainText( - 'nested tooltip', - ); - await expect(page.locator('[data-container="nested"]')).not.toContainText( - 'nested tooltip', - ); + const firstPortal = page.locator('[data-iui-portal]').first(); + + // both tooltips should be in the same container + await expect(firstPortal).toContainText('main tooltip'); + await expect(firstPortal).toContainText('nested tooltip'); + + // main container should not have any tooltips because we portal to + const mainContainer = page.locator('[data-container="main"]'); + await expect(mainContainer).not.toContainText('tooltip'); }); test('should not inherit portalContainer across different windows', async ({ @@ -32,9 +32,12 @@ test('should not inherit portalContainer across different windows', async ({ await page.click('button'); const popout = await popoutPromise; - await expect(popout.locator('[data-container="popout"]')).toContainText( + await expect(popout.locator('[data-iui-portal]')).toContainText( 'popout tooltip', ); + await expect(popout.locator('[data-container="popout"]')).not.toContainText( + 'tooltip', + ); }); test('should not cause an infinite loop when portaled', async ({ page }) => {