diff --git a/TESTING.md b/TESTING.md index 22ed40bd..dc3be25c 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1572,6 +1572,12 @@ Recorded deviations on this page (spec §8): | 429 `busy` | "The folder is already opening." | | Anything else (415, 404, 409, 500, offline) | "The folder could not be opened. Try again." | + **D176 (#533, beyond the concept):** the viewer is a modal, so `showWorkspaceToast` + moves `#workspace-toast` into the topmost open modal, keeping its fixed position, + and back when that modal closes. Otherwise every toast raised from a modal is + covered and inert while `toHaveText` still passes: assert `toBeVisible` and the + hit test as `full-inspector.spec.mjs` does. + The route is local-only. It answers 403 `not-local` unless all of these hold: no forwarding-style header (`Forwarded`, `Via`, `X-Real-IP`, `CF-Connecting-IP`, `True-Client-IP`, `X-Client-IP`, `X-Cluster-Client-IP`, `Fastly-Client-IP`, and any @@ -1596,7 +1602,9 @@ Recorded deviations on this page (spec §8): reach it), falling back to `os.startfile(folder, "explore")`. macOS runs `/usr/bin/open -R`; Linux asks FileManager1 `ShowItems` with `dbus-send --print-reply --reply-timeout=2000` (a real reply or a fallback) and else runs - `xdg-open` on the folder. Those openers are argv lists resolved only on a fixed + `xdg-open` on the folder, or answers 501 without `DISPLAY` and `WAYLAND_DISPLAY`. + `open` and `xdg-open` get `OPENER_WAIT` (1 s): a nonzero exit is the 500 above, and + one still running counts as started and goes to the reaper (#533). Those openers are argv lists resolved only on a fixed system PATH, with no shell, DEVNULL streams, `close_fds`, their own session, a minimal environment (no venv PATH, PYTHONPATH or API keys) and a daemon-thread reaper. `tests/test_image_reveal.py` mocks `Popen`, `sys.platform`, `shutil.which`, @@ -1604,6 +1612,9 @@ Recorded deviations on this page (spec §8): (`POST`/`GET /__test__/reveal`) with no cooldown. The inspector comparison reads closed disclosures as hidden, so the button needs no mapping there. + LoRA Manager's open-location route has no local-only gate, so it also opens the + server's file manager for a proxied or remote browser; ours says it cannot. + Residual risk, accepted: a reverse proxy or tunnel that adds no forwarding header (a bare nginx `proxy_pass` to 127.0.0.1, `ssh -R`, `socat`) cannot be told apart from a local client, and a non-browser client behind it can forge a matching diff --git a/tests/browser/full-inspector.spec.mjs b/tests/browser/full-inspector.spec.mjs index 424efb0b..ce3bea31 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -363,6 +363,10 @@ test('Show in folder asks the server to open the listed file and announces each expect(Object.keys(body).sort()).toEqual(['path','root_index','root_scope']); expect(body).toMatchObject({root_index:0,path:'inspector/complete.png'});expect(body.root_scope).toMatch(/^[0-9a-f]{64}$/); await expect(toast).toHaveText('Opened the folder on this computer.'); + // #533: the answer is really on screen and reachable above the modal viewer, not covered by it. + await expect(toast).toBeVisible(); + expect(await toast.evaluate(node=>{const box=node.getBoundingClientRect();const top=document.elementFromPoint(box.x+box.width/2,box.y+box.height/2);return top===node;})).toBe(true); + await expect(page.locator('#viewer-dialog').getByRole('status').filter({hasText:'Opened the folder on this computer.'})).toHaveCount(1); const opened=async()=>(await (await request.get('/__test__/reveal')).json()).opened; expect(await opened()).toEqual(['output/inspector/complete.png']); for (const [outcome,message] of [['unavailable','No file manager is available on this computer.'],['fail','The folder could not be opened. Try again.']]) { @@ -377,7 +381,11 @@ test('Show in folder asks the server to open the listed file and announces each await expect(await request.post('/__test__/reveal',{data:{outcome:'opened'}})).toBeOK(); await page.route('**/prompt_manager/images/reveal',route=>route.continue({headers:{...route.request().headers(),'x-forwarded-for':'203.0.113.7'}})); await show.click();await expect(toast).toHaveText('Show in folder works only in a browser on the computer running ComfyUI.'); + await expect(toast).toBeVisible(); expect(await opened()).toEqual(['output/inspector/complete.png']); + // #533: closing the viewer takes the toast back to the page, still showing the message. + await page.keyboard.press('Escape');await expect(page.locator('#viewer-dialog')).toHaveCount(0); + await expect(toast).toBeVisible();await expect(page.locator('body > #workspace-toast')).toHaveCount(1); }); test('full inspector shows six facts and scoped provenance',async({page,request},testInfo)=>{ await page.setViewportSize({width:1600,height:1100});await fullFixture(page,request);const panel=inspector(page); diff --git a/tests/js/workspace-toast.test.js b/tests/js/workspace-toast.test.js index 907498f7..79eb3394 100644 --- a/tests/js/workspace-toast.test.js +++ b/tests/js/workspace-toast.test.js @@ -13,10 +13,11 @@ class FakeToast extends EventTarget { matches(selector) { assert.equal(selector, ':hover, :focus-within'); return this.held; } } -function install() { +function install(dialogs = []) { const toast = new FakeToast(); const previous = global.document; global.document = { + querySelectorAll: selector => { assert.equal(selector, 'dialog'); return dialogs; }, getElementById: id => id === 'workspace-toast' ? toast : null, createTextNode: text => ({ textContent: text }), createElement: tag => ({ tag, href: '', textContent: '' }), @@ -24,6 +25,18 @@ function install() { return { toast, restore: () => { global.document = previous; } }; } +// First: the module remembers a toast it has shown. +test('a page without the toast shows nothing and does not fail', async () => { + const { showWorkspaceToast } = await source; + const previous = global.document; + global.document = { getElementById: () => null }; + try { + assert.doesNotThrow(() => showWorkspaceToast('Nothing.')); + } finally { + global.document = previous; + } +}); + test('a plain toast clears after 6.5 s and a linked one after 20 s', async t => { const { showWorkspaceToast, TOAST_MS, LINK_TOAST_MS } = await source; t.mock.timers.enable({ apis: ['setTimeout'] }); @@ -84,13 +97,50 @@ test('a toast under the pointer or focus waits until they leave', async t => { } }); -test('a page without the toast shows nothing and does not fail', async () => { + +class FakeHost extends EventTarget { + constructor(modal = false, localName = 'dialog') { super(); this.modal = modal; this.localName = localName; this.nodes = []; } + matches(selector) { assert.equal(selector, ':modal'); return this.modal; } + append(node) { node.parentNode?.nodes.splice(node.parentNode.nodes.indexOf(node), 1); node.parentNode = this; this.nodes.push(node); } +} + +test('a toast shows inside the topmost open modal and goes home when it closes (#533)', async () => { const { showWorkspaceToast } = await source; - const previous = global.document; - global.document = { getElementById: () => null }; + const closed = new FakeHost(false), lower = new FakeHost(true), top = new FakeHost(true); + const dialogs = [closed, lower, top]; + const { toast, restore } = install(dialogs); + const home = new FakeHost(false, 'body');home.append(toast); try { - assert.doesNotThrow(() => showWorkspaceToast('Nothing.')); + showWorkspaceToast('Opened the folder on this computer.'); + assert.equal(toast.parentNode, top); + // The upper modal closes: the toast stays visible in the one under it. + top.modal = false;top.dispatchEvent(new Event('close')); + assert.equal(toast.parentNode, lower); + lower.modal = false;lower.dispatchEvent(new Event('close')); + assert.equal(toast.parentNode, home); + showWorkspaceToast('Next.'); + assert.equal(toast.parentNode, home); } finally { - global.document = previous; + restore(); + } +}); + +test('a toast removed along with its modal still shows the next message (#533)', async () => { + const { showWorkspaceToast } = await source; + const modal = new FakeHost(true); + const { toast, restore } = install([modal]); + const home = new FakeHost(false, 'body');home.append(toast); + try { + showWorkspaceToast('Inside.'); + assert.equal(toast.parentNode, modal); + modal.modal = false; + const byId = document.getElementById; + document.getElementById = () => null; // the dialog left the page with the toast in it + showWorkspaceToast('After.'); + assert.equal(toast.parentNode, home); + assert.equal(toast.textContent, 'After.'); + document.getElementById = byId; + } finally { + restore(); } }); diff --git a/tests/test_image_reveal.py b/tests/test_image_reveal.py index 50415862..3522e4ba 100644 --- a/tests/test_image_reveal.py +++ b/tests/test_image_reveal.py @@ -171,7 +171,10 @@ class TestRevealInFileManager(unittest.TestCase): self.path.parent.mkdir(parents=True) self.path.write_bytes(b"png") self.popen = patch.object(subprocess, "Popen").start() + self.popen.return_value.wait.return_value = 0 self.reaped = patch.object(file_reveal, "_reap").start() + # A desktop session unless a test says otherwise; CI has no display. + patch.dict(os.environ, {"DISPLAY": ":0"}).start() self.addCleanup(patch.stopall) def reveal(self, path=None): @@ -262,8 +265,72 @@ class TestRevealInFileManager(unittest.TestCase): self.reveal() self.assertEqual(self.argv(), ["/usr/bin/open", "-R", str(self.path)]) self.assert_detached() - self.popen.return_value.wait.assert_not_called() - self.reaped.assert_called_once_with(self.popen.return_value) + self.popen.return_value.wait.assert_called_once_with( + timeout=file_reveal.OPENER_WAIT + ) + self.reaped.assert_not_called() + + def test_a_failing_opener_is_reported_on_macos_and_linux(self): + cases = { + "darwin": patch.object( + file_reveal, "_macos_open", return_value="/usr/bin/open" + ), + "linux": self.which({"xdg-open": "/usr/bin/xdg-open"}), + } + for platform, opener in cases.items(): + with self.subTest(platform): + self.popen.return_value.wait.return_value = 4 + with ( + patch.object(file_reveal.sys, "platform", platform), + opener, + self.assertRaises(file_reveal.OpenerFailed), + ): + self.reveal() + self.reaped.assert_not_called() + + def test_an_opener_still_running_counts_as_started_and_is_reaped(self): + cases = { + "darwin": patch.object( + file_reveal, "_macos_open", return_value="/usr/bin/open" + ), + "linux": self.which({"xdg-open": "/usr/bin/xdg-open"}), + } + for platform, opener in cases.items(): + with self.subTest(platform): + self.reaped.reset_mock() + process = self.popen.return_value + process.wait.side_effect = subprocess.TimeoutExpired("opener", 1) + with patch.object(file_reveal.sys, "platform", platform), opener: + self.reveal() + self.reaped.assert_called_once_with(process) + + def test_linux_without_a_display_or_a_service_is_unavailable(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": ""}), + self.which({"xdg-open": "/usr/bin/xdg-open"}), + self.assertRaises(OpenerUnavailable), + ): + self.reveal() + self.popen.assert_not_called() + + def test_linux_on_wayland_alone_still_runs_xdg_open(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": "wayland-0"}), + self.which({"xdg-open": "/usr/bin/xdg-open"}), + ): + self.reveal() + self.assertEqual(self.argv(), ["/usr/bin/xdg-open", str(self.path.parent)]) + + def test_the_service_answers_without_needing_a_display(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": ""}), + self.which({"dbus-send": "/usr/bin/dbus-send"}), + ): + self.reveal() + self.assertEqual(self.popen.call_count, 1) def test_macos_open_is_the_fixed_system_path(self): self.assertEqual(file_reveal.MACOS_OPEN, "/usr/bin/open") @@ -324,6 +391,12 @@ class TestRevealInFileManager(unittest.TestCase): "XDG_SESSION_DESKTOP": "KDE", "XDG_CONFIG_HOME": "/home/u/.config", "XDG_DATA_DIRS": "/usr/share", + # What xdg-open reads to choose an opener (kde-open6 on Plasma 6). + "KDE_FULL_SESSION": "true", + "KDE_SESSION_VERSION": "6", + "GNOME_DESKTOP_SESSION_ID": "this-is-deprecated", + "MATE_DESKTOP_SESSION_ID": "this-is-mate", + "LXQT_SESSION_CONFIG": "session", } with patch.dict(os.environ, inherited, clear=True): env = file_reveal._child_env() @@ -343,6 +416,11 @@ class TestRevealInFileManager(unittest.TestCase): "XDG_SESSION_DESKTOP": "KDE", "XDG_CONFIG_HOME": "/home/u/.config", "XDG_DATA_DIRS": "/usr/share", + "KDE_FULL_SESSION": "true", + "KDE_SESSION_VERSION": "6", + "GNOME_DESKTOP_SESSION_ID": "this-is-deprecated", + "MATE_DESKTOP_SESSION_ID": "this-is-mate", + "LXQT_SESSION_CONFIG": "session", }, ) self.assertNotIn("/venv", file_reveal.SYSTEM_PATH) @@ -387,15 +465,19 @@ class TestRevealInFileManager(unittest.TestCase): for name, (available, outcome) in cases.items(): with self.subTest(name): self.popen.reset_mock(side_effect=True, return_value=True) - self.reaped.reset_mock() dbus = MagicMock() + opener = MagicMock() + opener.wait.return_value = 0 if isinstance(outcome, OSError): - self.popen.side_effect = [outcome, MagicMock()] + self.popen.side_effect = [outcome, opener] elif isinstance(outcome, subprocess.TimeoutExpired): dbus.wait.side_effect = [outcome, None] - self.popen.side_effect = [dbus, MagicMock()] + self.popen.side_effect = [dbus, opener] + elif outcome is None: + self.popen.side_effect = [opener] else: - self.popen.return_value.wait.return_value = outcome + dbus.wait.return_value = outcome + self.popen.side_effect = [dbus, opener] with ( patch.object(file_reveal.sys, "platform", "linux"), self.which(available), @@ -406,7 +488,7 @@ class TestRevealInFileManager(unittest.TestCase): ["/usr/bin/xdg-open", str(self.path.parent)], ) self.assert_detached(self.popen.call_count - 1) - self.reaped.assert_called_once() + opener.wait.assert_called_once_with(timeout=file_reveal.OPENER_WAIT) if name == "hung": dbus.kill.assert_called_once_with() diff --git a/utils/file_reveal.py b/utils/file_reveal.py index 61f56822..dc0d5fbc 100644 --- a/utils/file_reveal.py +++ b/utils/file_reveal.py @@ -51,6 +51,9 @@ REVEAL_COOLDOWN = 1.0 DBUS_REPLY_TIMEOUT_MS = 2000 # The dbus-send process itself gets a little longer before it is killed. DBUS_WAIT = 3.0 +# An opener that fails says so at once; one still running after this long has +# started a file manager (it may stay in the foreground). +OPENER_WAIT = 1.0 FILE_MANAGER_DEST = "org.freedesktop.FileManager1" # Openers are looked up here only, never on ComfyUI's (venv) PATH, and the # children get this as their PATH. @@ -77,6 +80,12 @@ CHILD_ENV_NAMES = ( "DBUS_SESSION_BUS_ADDRESS", "LANG", "LC_ALL", + # What xdg-open reads to choose an opener (kde-open6 on Plasma 6). + "KDE_FULL_SESSION", + "KDE_SESSION_VERSION", + "GNOME_DESKTOP_SESSION_ID", + "MATE_DESKTOP_SESSION_ID", + "LXQT_SESSION_CONFIG", ) @@ -84,6 +93,10 @@ class OpenerUnavailable(Exception): """No file manager can be started on this computer.""" +class OpenerFailed(Exception): + """The file manager's opener ran and reported a failure.""" + + class RevealTargetChanged(Exception): """The file is no longer a regular file inside its root.""" @@ -393,6 +406,19 @@ def _launch(argv: list[str]) -> subprocess.Popen: return subprocess.Popen(argv, **options) # nosec B603 +def _launch_opener(argv: list[str]) -> None: + """Start an opener and report its failure (OpenerFailed), never waiting on + a file manager it stays attached to: that one is left to the reaper.""" + process = _launch(argv) + try: + status = process.wait(timeout=OPENER_WAIT) + except subprocess.TimeoutExpired: + _reap(process) + return + if status != 0: + raise OpenerFailed(f"exit status {status}") + + def _show_items(path: Path) -> bool: """Ask the freedesktop FileManager1 service to select *path*; True only when the service replied (``--print-reply`` makes the exit status real).""" @@ -427,8 +453,9 @@ def reveal_in_file_manager(path: Path, root: Path) -> None: """Open the file manager at *path*, a regular file inside *root*. Re-checks that containment first (RevealTargetChanged). Raises - OpenerUnavailable when no opener exists and OSError when the one found - cannot start. Never waits on the file manager itself. + OpenerUnavailable when no opener exists or Linux has no graphical session, + OSError when the one found cannot start and OpenerFailed when it reports a + failure. Never waits on the file manager itself. """ path = Path(path) if not path.is_absolute(): @@ -441,11 +468,12 @@ def reveal_in_file_manager(path: Path, root: Path) -> None: opener = _macos_open() if opener is None: raise OpenerUnavailable() - _reap(_launch([opener, "-R", str(path)])) + _launch_opener([opener, "-R", str(path)]) return if _show_items(path): return opener = _executable("xdg-open") - if opener is None: + has_display = os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY") + if opener is None or not has_display: raise OpenerUnavailable() - _reap(_launch([opener, str(path.parent)])) + _launch_opener([opener, str(path.parent)]) diff --git a/web/js/workspace/toast.mjs b/web/js/workspace/toast.mjs index 249509c9..2ec98216 100644 --- a/web/js/workspace/toast.mjs +++ b/web/js/workspace/toast.mjs @@ -5,6 +5,28 @@ export const LINK_TOAST_MS = 20000; let toastTimer; // Stops waiting for the pointer or focus to leave a toast that a newer one replaced. let releaseWait = null; +// The toast and the page element it lives in when no modal is open. +let toastElement = null; +let toastHome = null; + +/** The open modal the user is in: the last in the page, as a nested dialog is opened after its parent. */ +function topModal() { + return [...document.querySelectorAll('dialog')].filter(dialog => dialog.matches(':modal')).at(-1) ?? null; +} + +/** + * #533: a modal covers and inerts the rest of the page, so the toast shows inside the topmost one (D176) and + * follows it out when it closes. Called by that dialog's `close` too; the same listener is only ever added once. + */ +function placeToast() { + const toast = toastElement; + const parent = toast.parentNode; + if (parent && parent.localName !== 'dialog') toastHome = parent; + const modal = topModal(); + const host = modal ?? toastHome; + if (host && parent !== host) host.append(toast); + modal?.addEventListener('close', placeToast); +} /** Whether the pointer or keyboard focus rests on the toast. */ const held = toast => typeof toast.matches === 'function' && toast.matches(':hover, :focus-within'); @@ -41,8 +63,11 @@ function scheduleClear(toast, delay) { * for LINK_TOAST_MS and while the pointer or focus is on it. */ export function showWorkspaceToast(message) { - const toast = document.getElementById('workspace-toast'); + // Null only when a dialog left the page with the toast still in it. + const toast = document.getElementById('workspace-toast') ?? toastElement; if (!toast) return; + toastElement = toast; + placeToast(); releaseWait?.(); if (Array.isArray(message)) { toast.replaceChildren(...message.map(part => {