diff --git a/changelog.d/2270-settings-focus-restore.fixed.md b/changelog.d/2270-settings-focus-restore.fixed.md new file mode 100644 index 00000000..2c196e4b --- /dev/null +++ b/changelog.d/2270-settings-focus-restore.fixed.md @@ -0,0 +1 @@ +Closing Settings no longer pulls focus back to its opener after you clicked somewhere else while the dialog was fading out: the focus hand-back now runs only while the dialog still holds it (#2270). diff --git a/core/client.js b/core/client.js index 56abb7a7..70b85af8 100644 --- a/core/client.js +++ b/core/client.js @@ -8261,7 +8261,10 @@ function useDialogFocus(ref) { const before = document.activeElement; ref.current?.focus?.({ preventScroll: true }); return () => { - if (before && typeof before.focus === "function" && before.isConnected) before.focus({ preventScroll: true }); + const active = document.activeElement; + const stillInDialog = active === ref.current || ref.current?.contains?.(active) === true; + const noFocusHeld = !active || active === document.body || active.isConnected === false; + if (before && typeof before.focus === "function" && before.isConnected && (stillInDialog || noFocusHeld || active === before)) before.focus({ preventScroll: true }); }; }, []); } diff --git a/core/src/client/overlay-hooks.js b/core/src/client/overlay-hooks.js index d6c19d0b..0eb6752f 100644 --- a/core/src/client/overlay-hooks.js +++ b/core/src/client/overlay-hooks.js @@ -22,11 +22,19 @@ export function useEscape(onClose) { // A dialog takes focus when it opens, so Tab starts inside it and Escape reaches it, // and gives that focus back to the still-connected opener when it closes (#2251, #274). +// It lingers for DIALOG_EXIT_MS (160 ms) with `pointer-events: none` on the leaving layer, +// so a click there lands on the page behind it: restore only while the dialog still holds +// focus (or nothing does), never over a place the user picked during the exit (#2270). export function useDialogFocus(ref) { useEffect(() => { const before = document.activeElement ref.current?.focus?.({ preventScroll: true }) - return () => { if (before && typeof before.focus === 'function' && before.isConnected) before.focus({ preventScroll: true }) } + return () => { + const active = document.activeElement + const stillInDialog = active === ref.current || ref.current?.contains?.(active) === true + const noFocusHeld = !active || active === document.body || active.isConnected === false + if (before && typeof before.focus === 'function' && before.isConnected && (stillInDialog || noFocusHeld || active === before)) before.focus({ preventScroll: true }) + } }, []) } diff --git a/core/test/dialog-focus.test.js b/core/test/dialog-focus.test.js index bfa9877c..02672e23 100644 --- a/core/test/dialog-focus.test.js +++ b/core/test/dialog-focus.test.js @@ -42,9 +42,14 @@ const react = { useSyncExternalStore: (_subscribe, get) => get(), } +// A browser moves `document.activeElement` when a node takes focus. A fake that only +// records the call cannot tell "the dialog still has focus" from "the user moved it on". +let activeDocument = null + const fakeNode = (kind) => ({ kind, focusCalls: [], style: {}, dataset: {}, isConnected: true, - focus(options) { this.focusCalls.push(options ?? {}) }, + focus(options) { this.focusCalls.push(options ?? {}); if (activeDocument) activeDocument.activeElement = this }, + contains: node => node === this, querySelector: () => null, querySelectorAll: () => [], addEventListener() {}, removeEventListener() {}, @@ -88,6 +93,7 @@ function mount(overrides = {}) { }, } context.module.exports = context.exports + activeDocument = context.document runInNewContext(outputFiles[0].text, context) const { SettingsDialog } = context.module.exports const tree = SettingsDialog({ page: 'general', id: null, roster, actions, useUpdate: () => ({}), ...overrides }) @@ -99,7 +105,7 @@ function mount(overrides = {}) { if (typeof cleanup === 'function') cleanups.push(cleanup) } return { - tree, dialogNode, opener, + tree, dialogNode, opener, document: context.document, unmount() { for (const cleanup of cleanups.reverse()) cleanup() }, } } @@ -123,6 +129,20 @@ test('opening moves focus into the dialog and closing gives it back to the still assert.equal(opener.focusCalls[0]?.preventScroll, true) }) +// #2270: the Settings dialog is drawn for DIALOG_EXIT_MS (160 ms) after it closes, with +// `pointer-events: none` on that leaving layer, so a click there lands on the page behind +// it. The unmount must not drag focus back to the opener once the user has moved it. +test('a click that moves focus during the exit animation is not pulled back to the opener', () => { + const { document, opener, unmount } = mount() + const composer = fakeNode('composer') + composer.focus({ preventScroll: true }) + assert.equal(document.activeElement, composer, 'the user\'s click took focus') + unmount() + assert.equal(document.activeElement, composer, 'focus stays where the user put it') + assert.equal(composer.focusCalls.length, 1, 'the late cleanup does not refocus the page') + assert.equal(opener.focusCalls.length, 0, 'the opener is not focused after the user moved on') +}) + test('the dialog keeps its role, aria-modal, aria-label and data-wide contract', () => { const narrow = mount().dialogNode.props assert.equal(narrow.role, 'dialog') diff --git a/docs/subsystems/core-client.md b/docs/subsystems/core-client.md index 37814c78..83555e47 100644 --- a/docs/subsystems/core-client.md +++ b/docs/subsystems/core-client.md @@ -16,7 +16,7 @@ The browser side of the patched `ds-bot` core (`core/`): sources in `core/src/cl - The chat's `ask_user` card (`questions.js`) answers where it is drawn: one option answers a single-select card at once, a multi-select card toggles its rows and **Submit** sends the set, and the × dismisses it; the same card waits in the dock above the composer when its call is not on screen. In a window that holds the team read-only the card keeps the question, its rows and the host's sentence (`roster.readOnly`, read through `lockedWindow` from `schedules.js`) and draws none of those controls — no option button, **Submit**, **Skip**, × or own-answer field, and no form to submit. Each of them reached a write the host refuses (`answer-question`, `dismiss-question`), and the refusal was invisible: a pressed answer marked the card answered and the next poll said nothing, while the × hid the card until that poll brought it back. Tests: `core/test/questions-readonly.test.js`. - Read-only banner: when another dsh holds the team (`roster.readOnly` a non-empty string), the sidebar draws the host's own sentence and, beside it, one **Reload** control in both shapes (wide and rail; the control stays outside the `role="status"` line so a live region remains text). It calls `actions.reloadTeam`, which re-reads the whole team (`state` without `since`, see `stateRequest` / `stateAnswerIsStale` in `sources.js`) so the banner and every refused control clear once the other holder let go, without a page reload that would drop what is in the composer. Tests: `core/test/sidebar-readonly-reload.test.js`, `core/test/client-reload-state.test.js`. - Chat context rows: a tool the plugin registers at install draws one `tool-addition` row, and `cells.js` keeps every name in `TEAM_TOOLS` out of the chat (`SILENT_REGISTRATION_TOOLS`, #1101) — the Project Assistant's `project_digest` joins them (#1712), so its 09:00 Session shows the digest, not its registration. -- A Settings dialog takes focus when it opens (`useDialogFocus` in `core/src/client/overlay-hooks.js`; the `.bt-settings` dialog carries `tabIndex -1` and a ref) and gives it back to the still-connected opener when it closes; it is not a focus trap, so Tab can still leave the dialog for the page behind the scrim (#2251). +- A Settings dialog takes focus when it opens (`useDialogFocus` in `core/src/client/overlay-hooks.js`; the `.bt-settings` dialog carries `tabIndex -1` and a ref) and gives it back to the still-connected opener when it closes; it is not a focus trap, so Tab can still leave the dialog for the page behind the scrim (#2251). It is drawn for `DIALOG_EXIT_MS` (160 ms) with `pointer-events: none` on the leaving layer, so the unmount restores focus only while the dialog still holds it (or nothing does) — a click on the page during that window keeps its focus (#2270). - Settings → Feedback's error list (`feedback.js`) still titles the block with the whole log (`Recent errors · 60`) but lists only the newest eight, so when the report holds more rows one `Show more` control (`aria-expanded`) sits under them and draws every loaded error, folding back to `Show less`; a report of eight or fewer draws no control (#2266). ## Does not own diff --git a/patches/ds-bot/0003-flybot-api-client.patch b/patches/ds-bot/0003-flybot-api-client.patch index f808f620..4465c984 100644 --- a/patches/ds-bot/0003-flybot-api-client.patch +++ b/patches/ds-bot/0003-flybot-api-client.patch @@ -2287,17 +2287,25 @@ diff --git a/src/client/new-chat.js b/src/client/new-chat.js diff --git a/src/client/overlay-hooks.js b/src/client/overlay-hooks.js --- a/src/client/overlay-hooks.js +++ b/src/client/overlay-hooks.js -@@ -20,6 +20,16 @@ export function useEscape(onClose) { +@@ -20,6 +20,24 @@ export function useEscape(onClose) { }, []) } +// A dialog takes focus when it opens, so Tab starts inside it and Escape reaches it, +// and gives that focus back to the still-connected opener when it closes (#2251, #274). ++// It lingers for DIALOG_EXIT_MS (160 ms) with `pointer-events: none` on the leaving layer, ++// so a click there lands on the page behind it: restore only while the dialog still holds ++// focus (or nothing does), never over a place the user picked during the exit (#2270). +export function useDialogFocus(ref) { + useEffect(() => { + const before = document.activeElement + ref.current?.focus?.({ preventScroll: true }) -+ return () => { if (before && typeof before.focus === 'function' && before.isConnected) before.focus({ preventScroll: true }) } ++ return () => { ++ const active = document.activeElement ++ const stillInDialog = active === ref.current || ref.current?.contains?.(active) === true ++ const noFocusHeld = !active || active === document.body || active.isConnected === false ++ if (before && typeof before.focus === 'function' && before.isConnected && (stillInDialog || noFocusHeld || active === before)) before.focus({ preventScroll: true }) ++ } + }, []) +} + diff --git a/patches/ds-bot/0008-client-bundle.patch b/patches/ds-bot/0008-client-bundle.patch index 3fef421f..5736251a 100644 --- a/patches/ds-bot/0008-client-bundle.patch +++ b/patches/ds-bot/0008-client-bundle.patch @@ -5029,7 +5029,7 @@ diff --git a/client.js b/client.js .bt-mem-skel{display:flex;flex-direction:column;gap:24px} .bt-mem-skel-block{display:flex;flex-direction:column;gap:10px} .bt-mem-skel-block span{display:block;height:12px;border-radius:6px;background:linear-gradient(90deg,var(--bt-hover) 25%,var(--bt-active) 50%,var(--bt-hover) 75%);background-size:200% 100%;animation:bt-mem-skel 1.4s ease-in-out infinite} -@@ -2574,2935 +2574,7487 @@ body[data-ds-dark-theme] .bt-theme-chip i{background:var(--da)} +@@ -2574,2935 +2574,7490 @@ body[data-ds-dark-theme] .bt-theme-chip i{background:var(--da)} @media (max-width:640px){.bt-mem{width:100vw;height:100%;border-radius:0;border:0}.bt-mem-body{padding:18px 18px 20px}.bt-mem-foot{padding:8px 12px 12px}} @media (prefers-reduced-motion:reduce){.bt-mem,.bt-mem-scrim,.bt-mem-summary>*,.bt-mem-empty,.bt-mem-reply,.bt-mem-skel-block span,.bt-mem-entries,.bt-mem-send>*,.bt-mem-changes li svg{animation:none}} @@ -9203,7 +9203,10 @@ diff --git a/client.js b/client.js + const before = document.activeElement; + ref.current?.focus?.({ preventScroll: true }); + return () => { -+ if (before && typeof before.focus === "function" && before.isConnected) before.focus({ preventScroll: true }); ++ const active = document.activeElement; ++ const stillInDialog = active === ref.current || ref.current?.contains?.(active) === true; ++ const noFocusHeld = !active || active === document.body || active.isConnected === false; ++ if (before && typeof before.focus === "function" && before.isConnected && (stillInDialog || noFocusHeld || active === before)) before.focus({ preventScroll: true }); + }; + }, []); } diff --git a/patches/ds-bot/files/test/dialog-focus.test.js b/patches/ds-bot/files/test/dialog-focus.test.js index bfa9877c..02672e23 100644 --- a/patches/ds-bot/files/test/dialog-focus.test.js +++ b/patches/ds-bot/files/test/dialog-focus.test.js @@ -42,9 +42,14 @@ const react = { useSyncExternalStore: (_subscribe, get) => get(), } +// A browser moves `document.activeElement` when a node takes focus. A fake that only +// records the call cannot tell "the dialog still has focus" from "the user moved it on". +let activeDocument = null + const fakeNode = (kind) => ({ kind, focusCalls: [], style: {}, dataset: {}, isConnected: true, - focus(options) { this.focusCalls.push(options ?? {}) }, + focus(options) { this.focusCalls.push(options ?? {}); if (activeDocument) activeDocument.activeElement = this }, + contains: node => node === this, querySelector: () => null, querySelectorAll: () => [], addEventListener() {}, removeEventListener() {}, @@ -88,6 +93,7 @@ function mount(overrides = {}) { }, } context.module.exports = context.exports + activeDocument = context.document runInNewContext(outputFiles[0].text, context) const { SettingsDialog } = context.module.exports const tree = SettingsDialog({ page: 'general', id: null, roster, actions, useUpdate: () => ({}), ...overrides }) @@ -99,7 +105,7 @@ function mount(overrides = {}) { if (typeof cleanup === 'function') cleanups.push(cleanup) } return { - tree, dialogNode, opener, + tree, dialogNode, opener, document: context.document, unmount() { for (const cleanup of cleanups.reverse()) cleanup() }, } } @@ -123,6 +129,20 @@ test('opening moves focus into the dialog and closing gives it back to the still assert.equal(opener.focusCalls[0]?.preventScroll, true) }) +// #2270: the Settings dialog is drawn for DIALOG_EXIT_MS (160 ms) after it closes, with +// `pointer-events: none` on that leaving layer, so a click there lands on the page behind +// it. The unmount must not drag focus back to the opener once the user has moved it. +test('a click that moves focus during the exit animation is not pulled back to the opener', () => { + const { document, opener, unmount } = mount() + const composer = fakeNode('composer') + composer.focus({ preventScroll: true }) + assert.equal(document.activeElement, composer, 'the user\'s click took focus') + unmount() + assert.equal(document.activeElement, composer, 'focus stays where the user put it') + assert.equal(composer.focusCalls.length, 1, 'the late cleanup does not refocus the page') + assert.equal(opener.focusCalls.length, 0, 'the opener is not focused after the user moved on') +}) + test('the dialog keeps its role, aria-modal, aria-label and data-wide contract', () => { const narrow = mount().dialogNode.props assert.equal(narrow.role, 'dialog')