-
-
Notifications
You must be signed in to change notification settings - Fork 100
feat(devtools): add openAsModal to use the panel over app modal dialogs #550
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.
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
AlemTuzlak
wants to merge
2
commits into
main
Choose a base branch
from
feat/369-open-as-modal
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@tanstack/devtools': minor | ||
| --- | ||
|
|
||
| Add the `openAsModal` config option. A dialog opened with `dialog.showModal()` makes the devtools inert. With this option, the open panel moves on top of such a dialog and takes input, and the app dialog is inert until the panel closes. Open the panel with the open hotkey while the dialog is open. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| import { test, expect } from '@playwright/test' | ||
| import { DevtoolsPage, SELECTORS } from '@tanstack/devtools-e2e' | ||
| import type { Page } from '@playwright/test' | ||
|
|
||
| // See hotkey.spec.ts: dispatch the exact keydown events of Control+~. | ||
| async function pressOpenHotkey(page: Page) { | ||
| await page.evaluate(() => { | ||
| window.dispatchEvent( | ||
| new KeyboardEvent('keydown', { key: 'Control', ctrlKey: true }), | ||
| ) | ||
| window.dispatchEvent( | ||
| new KeyboardEvent('keydown', { key: '~', ctrlKey: true }), | ||
| ) | ||
| window.dispatchEvent(new KeyboardEvent('keyup', { key: '~' })) | ||
| window.dispatchEvent(new KeyboardEvent('keyup', { key: 'Control' })) | ||
| }) | ||
| } | ||
|
|
||
| /** Whether a click at the center of the close button reaches it. */ | ||
| async function closeButtonTakesClicks(page: Page) { | ||
| const box = await page.getByTestId(SELECTORS.closeButton).boundingBox() | ||
| return page.evaluate( | ||
| ([x, y]) => | ||
| document | ||
| .elementFromPoint(x!, y!) | ||
| ?.closest('[data-testid="tsd-close-button"]') != null, | ||
| [box!.x + box!.width / 2, box!.y + box!.height / 2], | ||
| ) | ||
| } | ||
|
|
||
| test.describe('openAsModal', () => { | ||
| test('the open panel takes input on top of a modal app dialog', async ({ | ||
| page, | ||
| }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await dt.goto('/?open-as-modal') | ||
| await expect(dt.trigger()).toBeVisible() | ||
| await page.getByTestId('open-app-dialog').click() | ||
| await expect(page.getByTestId('app-dialog')).toBeVisible() | ||
|
|
||
| await pressOpenHotkey(page) | ||
| await dt.expectOpen() | ||
| await expect( | ||
| page.locator('dialog:modal > [data-testid="tanstack_devtools"]'), | ||
| ).toHaveCount(1) | ||
| expect(await closeButtonTakesClicks(page)).toBe(true) | ||
|
|
||
| await dt.closeViaButton() | ||
| await expect(dt.panel()).toHaveAttribute('data-open', 'false') | ||
| await expect( | ||
| page.locator('dialog [data-testid="tanstack_devtools"]'), | ||
| ).toHaveCount(0) | ||
| // The app dialog takes input again. | ||
| await page.getByTestId('app-dialog-button').click() | ||
| await expect(page.getByTestId('app-dialog-button')).toBeFocused() | ||
| }) | ||
|
|
||
| test('Escape closes the panel and keeps the app dialog open', async ({ | ||
| page, | ||
| }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await dt.goto('/?open-as-modal') | ||
| await expect(dt.trigger()).toBeVisible() | ||
| await page.getByTestId('open-app-dialog').click() | ||
| await pressOpenHotkey(page) | ||
| await dt.expectOpen() | ||
|
|
||
| await page.keyboard.press('Escape') | ||
| await expect(dt.panel()).toHaveAttribute('data-open', 'false') | ||
| await expect(page.getByTestId('app-dialog')).toBeVisible() | ||
| await expect( | ||
| page.locator('dialog [data-testid="tanstack_devtools"]'), | ||
| ).toHaveCount(0) | ||
| }) | ||
|
|
||
| test('stays on top when the app opens another modal dialog', async ({ | ||
| page, | ||
| }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await dt.goto('/?open-as-modal') | ||
| await expect(dt.trigger()).toBeVisible() | ||
| await page.getByTestId('open-app-dialog').click() | ||
| await pressOpenHotkey(page) | ||
| await dt.expectOpen() | ||
|
|
||
| await page.evaluate(() => { | ||
| const second = document.createElement('dialog') | ||
| second.textContent = 'second app dialog' | ||
| document.body.append(second) | ||
| second.showModal() | ||
| }) | ||
|
|
||
| await expect.poll(() => closeButtonTakesClicks(page)).toBe(true) | ||
| }) | ||
|
|
||
| test('releases the page when an open app dialog is removed', async ({ | ||
| page, | ||
| }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await dt.goto('/?open-as-modal') | ||
| await expect(dt.trigger()).toBeVisible() | ||
| await page.getByTestId('open-app-dialog').click() | ||
| await pressOpenHotkey(page) | ||
| await dt.expectOpen() | ||
|
|
||
| // Removed without close(), as when a component unmounts. | ||
| await page.evaluate(() => document.querySelector('#app-dialog')!.remove()) | ||
|
|
||
| await expect( | ||
| page.locator('dialog [data-testid="tanstack_devtools"]'), | ||
| ).toHaveCount(0) | ||
| await expect(page.getByTestId('text-input')).toBeEditable() | ||
| await page.getByTestId('text-input').fill('works') | ||
| }) | ||
|
|
||
| test('without the option, a modal app dialog blocks the panel', async ({ | ||
| page, | ||
| }) => { | ||
| const dt = new DevtoolsPage(page) | ||
| await dt.goto('/') | ||
| await expect(dt.trigger()).toBeVisible() | ||
| await page.getByTestId('open-app-dialog').click() | ||
|
|
||
| await pressOpenHotkey(page) | ||
| await dt.expectOpen() | ||
| expect(await closeButtonTakesClicks(page)).toBe(false) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { createEffect, onCleanup } from 'solid-js' | ||
| import type { Accessor } from 'solid-js' | ||
|
|
||
| /** | ||
| * A modal dialog of the app (`dialog.showModal()`) makes the rest of the page | ||
| * inert, the devtools included, and no z-index or popover gets past that: only | ||
| * the content of the topmost modal dialog takes input. | ||
| * | ||
| * While the panel is open and the app has a modal dialog open, this moves the | ||
| * devtools root into a modal dialog of its own, shown on top of the app's. It | ||
| * moves the root back when the panel or the app dialog closes. The app dialog | ||
| * is inert in the meantime. | ||
| */ | ||
| export function createModalHost( | ||
| enabled: Accessor<boolean>, | ||
| root: Accessor<HTMLElement | undefined>, | ||
| isOpen: Accessor<boolean>, | ||
| ) { | ||
| createEffect(() => { | ||
| const element = root() | ||
| if (!enabled() || !element) return | ||
|
|
||
| const doc = element.ownerDocument | ||
| const host = doc.createElement('dialog') | ||
| // A zero-size box: the devtools are `position: fixed`, so they still lay | ||
| // out against the viewport. | ||
| host.style.cssText = | ||
| 'position:fixed;inset:0;width:0;height:0;max-width:none;max-height:none;margin:0;padding:0;border:0;overflow:visible;background:transparent' | ||
| // Escape closes the panel through the devtools' own keydown handler, which | ||
| // also closes this dialog. Without preventDefault the browser then sends | ||
| // the same Escape to the app dialog and closes it too. The capture phase | ||
| // on the window sees the key wherever the focus is. | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (host.open && event.key === 'Escape') event.preventDefault() | ||
| } | ||
| doc.defaultView?.addEventListener('keydown', onKeyDown, true) | ||
| // The dialog must never close by itself, or the root stays hidden in it. | ||
| host.addEventListener('cancel', (event) => event.preventDefault()) | ||
| let home: Node | null = null | ||
|
|
||
| const sync = () => { | ||
| const wanted = | ||
| isOpen() && | ||
| Array.from(doc.querySelectorAll('dialog')).some( | ||
| (dialog) => dialog !== host && dialog.matches(':modal'), | ||
| ) | ||
| if (wanted) { | ||
| if (host.open) return | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| home = element.parentNode | ||
| host.append(element) | ||
| doc.body.append(host) | ||
| host.showModal() | ||
| } else if (host.open) { | ||
| host.close() | ||
| home?.appendChild(element) | ||
| host.remove() | ||
| } | ||
| } | ||
|
|
||
| // `showModal()` and `close()` toggle the `open` attribute. Removing an open | ||
| // dialog from the page is a child list change instead. | ||
| const observer = new MutationObserver((records) => { | ||
| // An app dialog shown after the host goes on top of it. Showing the host | ||
| // again puts the host back on top. | ||
| const appModalShown = records.some( | ||
| (record) => | ||
| record.type === 'attributes' && | ||
| record.target !== host && | ||
| (record.target as Element).matches('dialog:modal'), | ||
| ) | ||
| if (host.open && appModalShown) { | ||
| host.close() | ||
| host.showModal() | ||
| return | ||
| } | ||
| sync() | ||
| }) | ||
| observer.observe(doc.documentElement, { | ||
| subtree: true, | ||
| childList: true, | ||
| attributeFilter: ['open'], | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| createEffect(() => { | ||
| isOpen() | ||
| sync() | ||
| }) | ||
|
|
||
| onCleanup(() => { | ||
| doc.defaultView?.removeEventListener('keydown', onKeyDown, true) | ||
| observer.disconnect() | ||
| if (host.open) { | ||
| host.close() | ||
| home?.appendChild(element) | ||
| } | ||
| host.remove() | ||
| }) | ||
| }) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Detect application modals inside shadow roots.
If an application calls
showModal()on a dialog inside a shadow root,doc.querySelectorAll('dialog')does not find it. The observer ondoc.documentElementalso misses itsopenchange. The application dialog still makes the Devtools root inert, soopenAsModaldoes not make the panel interactive. Include accessible shadow roots in modal detection and observation, or state this limitation in the option’s contract. (dom.spec.whatwg.org)🤖 Prompt for AI Agents