fix: dialog closing problems
Massimo Melina committed
Apr 10, 2026 at 19:51 UTC
76b10ba7d9a82e80c123b7ae4636fa021ab6ded7
5 files changed
+141
-17
e2e/dialog-back.spec.ts
new
+98
@@ -0,0 +1,98 @@
1
+import { test, expect, Page } from '@playwright/test'
2
+import { clickAdminMenu, loginAdmin } from './common'
3
+
4
+const pluginId = 'test'
5
+
6
+function pluginRow(page: Page) {
7
+ return page.getByRole('grid').getByRole('button', { name: /^Options$/ }).locator('xpath=ancestor::*[@role="row"][1]')
8
+}
9
+
10
+async function ensureAuthAndGoPlugins(page: Page) {
11
+ // loginAdmin doesn't wait for auth completion — navigate to Options first to confirm
12
+ await clickAdminMenu(page, 'Options')
13
+ await expect(page.getByText('Correctly working on port')).toBeVisible({ timeout: 10_000 })
14
+ await clickAdminMenu(page, 'Plugins')
15
+ // wouter navigation settles slightly after the click; wait until the transient dialog marker is gone before using the page as a baseline
16
+ await expect.poll(() => page.evaluate(() => history.state?.$dialog), { timeout: 10_000 }).toBeUndefined()
17
+ await page.waitForTimeout(1000)
18
+ const row = pluginRow(page)
19
+ await expect(row).toBeVisible({ timeout: 10_000 })
20
+ const startButton = row.getByRole('button', { name: new RegExp(`^Start ${pluginId}$`) })
21
+ if (await startButton.count())
22
+ await startButton.click()
23
+}
24
+
25
+async function open3NestedDialogs(page: Page) {
26
+ // dialog 1: test plugin config
27
+ const row = pluginRow(page)
28
+ await row.getByRole('button', { name: 'Options' }).click()
29
+ await expect(page.getByText(`Options for ${pluginId}`)).toBeVisible({ timeout: 5000 })
30
+
31
+ // dialog 2: Add entry in ArrayField
32
+ await page.getByRole('button', { name: 'Add' }).click()
33
+ const addDialog = page.getByRole('dialog', { name: /Add/ })
34
+ await expect(addDialog).toBeVisible({ timeout: 5000 })
35
+
36
+ // dialog 3: Browse files picker (inside Add dialog)
37
+ await addDialog.getByRole('button', { name: 'Browse files...' }).click()
38
+ await expect(page.locator('.MuiDialog-root')).toHaveCount(3, { timeout: 5000 })
39
+}
40
+
41
+async function pressTopDialogEscape(page: Page, remainingDialogs: number) {
42
+ await page.locator('.MuiDialog-container').last().press('Escape')
43
+ await expect(page.locator('.MuiDialog-root')).toHaveCount(remainingDialogs, { timeout: 5000 })
44
+}
45
+
46
+async function expectDialogsClosed(page: Page) {
47
+ await expect(page.locator('.MuiDialog-root')).toHaveCount(0, { timeout: 10_000 })
48
+ await page.waitForTimeout(3000)
49
+ await expect(page.getByRole('heading', { name: 'Plugins' })).toBeVisible({ timeout: 10_000 })
50
+}
51
+
52
+test('nested dialog ESC does not overshoot history', async ({ page }) => {
53
+ await loginAdmin(page)
54
+ await ensureAuthAndGoPlugins(page)
55
+
56
+ await open3NestedDialogs(page)
57
+
58
+ // close all 3 dialogs via ESC
59
+ for (const remainingDialogs of [2, 1, 0])
60
+ await pressTopDialogEscape(page, remainingDialogs)
61
+
62
+ await expectDialogsClosed(page)
63
+ expect(page.url()).toContain('~/admin')
64
+})
65
+
66
+test('nested dialog browser-back does not overshoot', async ({ page }) => {
67
+ await loginAdmin(page)
68
+ await ensureAuthAndGoPlugins(page)
69
+
70
+ await open3NestedDialogs(page)
71
+
72
+ // rapid browser back ×3
73
+ await page.evaluate(() => {
74
+ setTimeout(() => history.back(), 0)
75
+ setTimeout(() => history.back(), 1000)
76
+ setTimeout(() => history.back(), 2000)
77
+ })
78
+
79
+ await page.waitForTimeout(3000)
80
+
81
+ await expectDialogsClosed(page)
82
+ expect(page.url()).toContain('~/admin')
83
+})
84
+
85
+test('repeated nested dialog open/close cycles', async ({ page }) => {
86
+ await loginAdmin(page)
87
+ await ensureAuthAndGoPlugins(page)
88
+
89
+ for (let cycle = 0; cycle < 5; cycle++) {
90
+ await open3NestedDialogs(page)
91
+
92
+ for (const remainingDialogs of [2, 1, 0])
93
+ await pressTopDialogEscape(page, remainingDialogs)
94
+
95
+ await expectDialogsClosed(page)
96
+ expect(page.url()).toContain('~/admin')
97
+ }
98
+})
frontend/src/login.ts
+1
-2
@@ -154,8 +154,7 @@ export async function loginDialog(closable=true, reloadAfter=true) {
154
await close(true)
155
toast(t`Logged in`, 'success')
156
if (res?.redirect)
157
- setTimeout(() => // workaround: the history.back() issued by closing the dialog is messing with our navigation
158
- getHFS().navigate(res.redirect), 10) // from my tests 1 was enough, 0 was not (not always). Would be nice to find a cleaner way
157
+ getHFS().navigate(res.redirect)
158
else if (reloadAfter)
159
reloadList()
160
} catch (err: any) {
playwright.config.ts
+3
-1
@@ -106,7 +106,9 @@ export default defineConfig({
106
},
107
/* Run your local dev server before starting the tests */
108
webServer: [{
109
- command: 'npm run server-for-test' + (process.env.TEST_WITH_UI ? '-dev' : ''), // use server-for-test-dev only for "test-with-ui"
109
+ command: `mkdir -p tests/work/plugins/test`
110
+ + ` && printf '%s\\n' "exports.apiRequired = 1" "exports.config = {" " icons: { type: 'array', fields: { iconFile: { type: 'real_path' } } }," "}" > tests/work/plugins/test/plugin.js`
111
+ + ` && npm run server-for-test${process.env.TEST_WITH_UI ? '-dev' : ''}`, // use server-for-test-dev only for "test-with-ui"
112
url: `http://127.0.0.1:${testPort}`,
113
reuseExistingServer: !process.env.CI,
114
}, { // launch a second server for tests with an empty/default config
shared/dialogs.ts
+37
-13
@@ -28,7 +28,8 @@ interface Dialog extends DialogOptions {
28
$id?: number
29
$opening?: NodeJS.Timeout
30
ts?: number
31
- close: (v?: any) => undefined | Promise<void>
31
+ close: (v?: any, skipHistory?: boolean) => undefined | Promise<void>
32
+ closed?: Promise<void | undefined>
33
restoreFocus?: any
34
}
35
@@ -74,11 +75,12 @@ export function isDescendant(child: Node | null | undefined, parentMatch: Node |
75
}
76
77
let waitClosing = Promise.resolve()
78
+let waitQueuedCloses = Promise.resolve()
79
let ignorePopState = false
78
-async function back() {
80
+async function doBack() {
81
ignorePopState = true
82
const was = history.state
81
- return waitClosing = waitClosing.then(() => new Promise<void>(async res => {
83
+ return new Promise<void>(async res => {
84
const timeout = Date.now() + 1500
85
let lastBack = 0
86
while (was === history.state) { // history.back seems to not always be effective, so we loop for it
@@ -91,7 +93,10 @@ async function back() {
93
await wait(10) // we wait shorter and loop faster so to exit/resolve asap
94
}
95
res()
94
- }))
96
+ })
97
+}
98
+async function back() {
99
+ return waitClosing = waitClosing.then(doBack)
100
}
101
102
const BASE_STATE = 1
@@ -101,7 +106,6 @@ const BASE_STATE = 1
106
history.back()
107
await wait(1) // history.state is not changed without this, on chrome123
108
}
104
- history.replaceState({ ...history.state, $dialog: BASE_STATE }, '')
109
})()
110
111
export function Dialogs(props: HTMLAttributes<HTMLDivElement>) {
@@ -208,25 +212,33 @@ export function newDialog(options: DialogOptions) {
212
restoreFocus: options.restoreFocus ?? ref(document.activeElement || {}),
213
})
214
let cancelOpening = false
211
- waitClosing.then(() => { // in case dialogs were just closed, account for window.history delay. You already didn't expect dialog to open immediately, but at state change
215
+ Promise.all([waitClosing, waitQueuedCloses]).then(() => { // queued closes must settle too, or a new dialog may wait behind a stale close request forever
216
if (cancelOpening) return
217
+ if (!dialogs.length) {
218
+ history.replaceState({ ...history.state, $dialog: BASE_STATE }, '')
219
+ }
220
dialogs.push(d)
214
- if (dialogs.at(-1)!.closable !== false) // use proxy object, to stay in sync with its changes
221
+ if (dialogs.at(-1)!.closable !== false) { // use proxy object, to stay in sync with its changes
222
+ // browser-back closes dialogs by walking back to the entry that was current before the first dialog opened
223
+ if (history.state?.$dialog === undefined) {
224
+ history.replaceState({ ...history.state, $dialog: BASE_STATE }, '')
225
+ }
226
history.pushState({ $dialog: $id, ts, idx: 1 + (history.state?.idx || 0) }, '')
227
+ }
228
})
229
return d
230
219
- function close(v?:any) {
231
+ function close(v?:any, skipHistory=true) {
232
cancelOpening = true
233
const i = dialogs.findIndex(x => (x as any).$id === $id)
234
if (i < 0) return
223
- d.closed = history.state?.$dialog === $id ? back() : Promise.resolve()
235
+ d.closed = !skipHistory && history.state?.$dialog === $id ? back() : Promise.resolve()
236
closeDialogAt(i, v)
237
return options.closed
238
}
239
}
240
229
-export function closeDialog(v?:any, skipHistory=false) {
241
+export function closeDialog(v?:any, skipHistory=false): Dialog | undefined {
242
let i = dialogs.length
243
if (dialogs[i - 1]?.closable === false) return
244
while (i--) {
@@ -234,7 +246,15 @@ export function closeDialog(v?:any, skipHistory=false) {
246
if (d.reserveClosing)
247
continue
248
if (!skipHistory) {
237
- if (history.state?.$dialog !== d.$id) return
249
+ if (history.state?.$dialog !== d.$id) {
250
+ // rapid ESC presses can target the next dialog before browser history has caught up with the previous close
251
+ const closed: Promise<void | undefined> = waitQueuedCloses = waitQueuedCloses
252
+ .then(() => waitClosing)
253
+ // once the history unwind has already been queued, retry the next close without re-checking the same stale state
254
+ .then(() => closeDialog(v, true)?.closed)
255
+ // return a promise here so mobile callers wait instead of spinning on the still-open dialog stack
256
+ return { ...d, closed } as Dialog
257
+ }
258
d.closed = back()
259
}
260
closeDialogAt(i, v)
@@ -247,8 +267,12 @@ function closeDialogAt(i: number, value?: any) {
267
d.restoreFocus?.focus?.() // if element is not HTMLElement, it doesn't have focus method
268
d.closingValue = value && typeof value === 'object' ? ref(value) : value // since this is being assigned to a valtio proxy, ref is necessary to avoid crashing with unusual (and possibly accidental) objects like React's SynteticEvents
269
d.closed ??= Promise.resolve()
250
- d.closed.then(() =>
251
- d?.onClose?.(value))
270
+ d.closed.then(async () => {
271
+ // queued ESC closes can empty the stack before the last synthetic history entry has been unwound
272
+ while (!dialogs.length && history.state?.$dialog !== undefined && history.state.$dialog !== BASE_STATE)
273
+ await doBack()
274
+ d?.onClose?.(value)
275
+ })
276
return d
277
}
278
tests/config.yaml
+2
-1
@@ -211,7 +211,8 @@ vfs:
211
can_see: true
212
children:
213
- name: hi
214
-enable_plugins: []
214
+enable_plugins:
215
+ - test
216
accounts:
217
rejetto:
218
srp: 45342499289060118491953864985904491552469027660732372413642410068701508460756016883587538941326969234657513592151654348700271313826438668687753011476364050308196782081349845543793071133962680938833562613037542533901213868728748288160539083771680823003225050120746634361389984903020864743362165811690166921198|19774996369091767466773785425767419323692622801450813744779985314019896203670047564571162840176134630605183591917392609010436089360338159426706132364650486739033105760664596040271171468723867761859495433833673834714019039663575196623574481195253082005981473801748471685440578887877536727938980101106408640345112365050882261328175189757112413706886166283150089763450343918391537516887689512711278015984288024916578966075794729866664625833406939511948734410200472601790276163270442611968041929062065290928050153275322302258046366227185195993685890141067515020089439381701920633381138539731031195766277095966471034607906