Skip to content

Commit 340e55a

Browse files
authored
fix(superdoc): hoist closeOnEscape to request top level (SD-2870) (#3045)
The find/replace composable opened its surface with `floating: { closeOnEscape: true, ... }`. The surface manager only reads `request.closeOnEscape` (surface-manager.js:315). The nested `floating.closeOnEscape` was preserved on the request but never consulted, so the explicit intent was a silent no-op. The escape-closes-surface behavior happened to work because the top-level default is true, but a user override on `moduleConfig.floating .closeOnEscape` would silently win over the find/replace request. Move the field to its correct top-level position in all three open() call sites. Tighten the existing unit test to assert the top-level shape (the previous assertion verified the call shape but not the runtime path). Add a one-line clarification to the typedef so future contributors do not nest the field under `floating`. Found while probing checkJs against use-find-replace.js for SD-2863.
1 parent 2473353 commit 340e55a

3 files changed

Lines changed: 13 additions & 6 deletions

File tree

packages/superdoc/src/composables/use-find-replace.js

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,8 @@ export function useFindReplace({ getSurfaceManager, getActiveEditor, activeEdito
531531
surfaceHandle = manager.open({
532532
mode: 'floating',
533533
ariaLabel: config.texts.findAriaLabel,
534-
floating: { placement: 'top-right', closeOnEscape: true, autoFocus: true },
534+
closeOnEscape: true,
535+
floating: { placement: 'top-right', autoFocus: true },
535536
component: markRaw(resolution.component),
536537
props: { ...resolution.props, findReplace: handle },
537538
});
@@ -541,7 +542,8 @@ export function useFindReplace({ getSurfaceManager, getActiveEditor, activeEdito
541542
surfaceHandle = manager.open({
542543
mode: 'floating',
543544
ariaLabel: config.texts.findAriaLabel,
544-
floating: { placement: 'top-right', closeOnEscape: true, autoFocus: true },
545+
closeOnEscape: true,
546+
floating: { placement: 'top-right', autoFocus: true },
545547
render: (ctx) =>
546548
userRender({
547549
container: ctx.container,
@@ -579,7 +581,8 @@ export function useFindReplace({ getSurfaceManager, getActiveEditor, activeEdito
579581
mode: 'floating',
580582
ariaLabel: config.texts.findAriaLabel,
581583
component: markRaw(FindReplaceSurface),
582-
floating: { placement: 'top-right', closeOnEscape: true, autoFocus: true },
584+
closeOnEscape: true,
585+
floating: { placement: 'top-right', autoFocus: true },
583586
props: { findReplace: handle },
584587
});
585588
}

packages/superdoc/src/composables/use-find-replace.test.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,11 @@ describe('useFindReplace', () => {
9393
const call = manager.open.mock.calls[0][0];
9494
expect(call.mode).toBe('floating');
9595
expect(call.floating.placement).toBe('top-right');
96-
expect(call.floating.closeOnEscape).toBe(true);
96+
// closeOnEscape lives at the request top level — surface-manager
97+
// only reads `request.closeOnEscape`, not `request.floating.closeOnEscape`.
98+
// Asserting the nested location (as this test did before SD-2870) was
99+
// verifying the call shape but not the runtime effect.
100+
expect(call.closeOnEscape).toBe(true);
97101
});
98102

99103
it('does not create second surface when already open', async () => {

packages/superdoc/src/core/types/index.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@
171171
* @property {string} [title] Optional title rendered in the surface chrome
172172
* @property {string} [ariaLabel] Accessible name for the surface when no visible title is provided. Used as aria-label fallback when neither title nor ariaLabelledBy is set.
173173
* @property {string} [ariaLabelledBy] ID of the element that labels the surface. Takes precedence over ariaLabel. Use this when the content component renders its own heading that should serve as the accessible name.
174-
* @property {boolean} [closeOnEscape] Whether Escape closes the surface (default: true)
174+
* @property {boolean} [closeOnEscape] Whether Escape closes the surface (default: true). Set at the request top level — the runtime does not read `floating.closeOnEscape` on a per-request basis.
175175
* @property {boolean} [closeOnBackdrop] Whether backdrop click closes a dialog (default: true)
176176
* @property {{ maxWidth?: string | number }} [dialog] Dialog-specific overrides
177177
* @property {Object} [floating] Floating-specific overrides
@@ -196,7 +196,7 @@
196196
* @property {string} [title] Optional title rendered in the surface chrome
197197
* @property {string} [ariaLabel] Accessible name for the surface when no visible title is provided. Used as aria-label fallback when neither title nor ariaLabelledBy is set.
198198
* @property {string} [ariaLabelledBy] ID of the element that labels the surface. Takes precedence over ariaLabel. Use this when the content component renders its own heading that should serve as the accessible name.
199-
* @property {boolean} [closeOnEscape] Whether Escape closes the surface (default: true)
199+
* @property {boolean} [closeOnEscape] Whether Escape closes the surface (default: true). Set at the request top level — the runtime does not read `floating.closeOnEscape` on a per-request basis.
200200
* @property {boolean} [closeOnBackdrop] Whether backdrop click closes a dialog (default: true)
201201
* @property {{ maxWidth?: string | number }} [dialog] Dialog-specific overrides
202202
* @property {Object} [floating] Floating-specific overrides

0 commit comments

Comments
 (0)