Skip to content

Commit d98543c

Browse files
committed
fix(plugin-detail,app-shell): Esc on an open face in the record pane returns to the record; ⌘⇧, is the topmost record view's
Inside the record pane the 动态 tab and a related list's whole face never closed on Esc: the face's guard found the pane's own dialog, and the pane's Esc (the drawer's dialog layer, the split pane's document listener) closed or stepped the pane instead. The face now listens on the window in the capture phase and takes the key unless a text field, an open menu or alert, or a dialog the face is not inside owns it. A drawer wide enough for its own right column draws its own ⊟, and the page under it keeps its tree mounted, so one ⌘⇧, ran both views' listeners and the page's ran first: it flipped the hidden page's stored switch, not the drawer's. Only the topmost record root, the last in document order, answers the chord now. Claude-Session: https://claude.ai/code/session_01Utp6CaA9ZdTcj97mzGtHYt Co-authored-by: Claude <noreply@anthropic.com>
1 parent a38376b commit d98543c

4 files changed

Lines changed: 232 additions & 12 deletions

File tree

‎ui/packages/app-shell/src/views/RecordDetailView.panelToggles-27052.test.tsx‎

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,14 @@
2727
* action row (objectstack#27241) — not on the crumb row, which keeps only
2828
* the ways back and has no child without one;
2929
* - it is drawn only where the tier has a right column: both two-column
30-
* tiers, the embedded record at those widths too, never the one column.
30+
* tiers, the embedded record at those widths too, never the one column;
31+
* - the chord is the topmost record view's: a drawer wide enough for its
32+
* own right column flips its own switch, never the page's under it.
3133
*/
3234

3335
import * as React from 'react';
3436
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
35-
import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react';
37+
import { render, screen, cleanup, fireEvent, waitFor, within } from '@testing-library/react';
3638
import { MemoryRouter } from 'react-router-dom';
3739
import { MetadataCtx } from '@object-ui/react';
3840
import { RelatedCountStore } from '@object-ui/components';
@@ -125,6 +127,8 @@ const OBJECTS = [
125127
];
126128

127129
let rootWidth = 0;
130+
/** A record view inside a dialog (a drawer over the page) measures this, when set. */
131+
let dialogRootWidth: number | null = null;
128132

129133
function makeDataSource() {
130134
return {
@@ -136,8 +140,13 @@ function makeDataSource() {
136140
};
137141
}
138142

139-
function renderPage({ object = LEAD, width = 1440, embedded = false }: { object?: string; width?: number; embedded?: boolean } = {}) {
140-
rootWidth = width;
143+
function renderPage({
144+
object = LEAD,
145+
width = 1440,
146+
embedded = false,
147+
container,
148+
}: { object?: string; width?: number; embedded?: boolean; container?: HTMLElement } = {}) {
149+
if (!container) rootWidth = width;
141150
const metadata = {
142151
objects: OBJECTS,
143152
pages: [],
@@ -162,6 +171,7 @@ function renderPage({ object = LEAD, width = 1440, embedded = false }: { object?
162171
/>
163172
</MetadataCtx.Provider>
164173
</MemoryRouter>,
174+
container ? { container } : undefined,
165175
);
166176
}
167177

@@ -182,6 +192,7 @@ const RIGHT_KEY = (object: string) => `os:pref:record.columns.related@object:${o
182192

183193
beforeEach(() => {
184194
cleanup();
195+
dialogRootWidth = null;
185196
window.localStorage.clear();
186197
RelatedCountStore._reset();
187198
vi.stubGlobal(
@@ -193,7 +204,8 @@ beforeEach(() => {
193204
const measure = HTMLElement.prototype.getBoundingClientRect;
194205
vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function (this: HTMLElement) {
195206
if (!this.hasAttribute('data-record-page-root')) return measure.call(this);
196-
return { x: 0, y: 0, top: 0, left: 0, right: rootWidth, bottom: 800, width: rootWidth, height: 800, toJSON: () => ({}) } as DOMRect;
207+
const width = dialogRootWidth !== null && this.closest('[role="dialog"]') ? dialogRootWidth : rootWidth;
208+
return { x: 0, y: 0, top: 0, left: 0, right: width, bottom: 800, width, height: 800, toJSON: () => ({}) } as DOMRect;
197209
});
198210
});
199211

@@ -331,4 +343,34 @@ describe('RecordDetailView — the right-column switch ⊟ (objectstack#27052, o
331343
await screen.findByRole('group', { name: 'Panels' }, { timeout: 5000 });
332344
expect(shown(embedded.container)).toBe('details right');
333345
});
346+
347+
it('⌘⇧, flips the topmost record view\'s switch alone: a 1000 px drawer over a 1440 px page (objectstack#27524)', async () => {
348+
// A record page keeps its own tree mounted under the drawer its rows and
349+
// lookups open (`RecordPageDrawerHost`), and the drawer's sheet is
350+
// portaled after it, to the end of the body. A drawer dragged to 1000 px
351+
// has a right column of its own, so both views draw ⊟: the chord is the
352+
// drawer's, and never writes the hidden page's stored choice.
353+
dialogRootWidth = 1000;
354+
const page = renderPage();
355+
await waitFor(() => expect(shown(page.container)).toBe('details right'), { timeout: 5000 });
356+
const sheet = document.createElement('div');
357+
sheet.setAttribute('role', 'dialog');
358+
document.body.appendChild(sheet);
359+
const drawer = renderPage({ object: PRODUCT, embedded: true, container: sheet });
360+
await waitFor(() => expect(shown(sheet)).toBe('details right'), { timeout: 5000 });
361+
expect(sheet.querySelector('[data-record-page-tier]')?.getAttribute('data-record-page-tier')).toBe('two-column');
362+
expect(within(sheet).queryByRole('group', { name: 'Panels' })).not.toBeNull();
363+
364+
press('related');
365+
await waitFor(() => expect(shown(sheet)).toBe('details'));
366+
expect(window.localStorage.getItem(RIGHT_KEY(PRODUCT))).toContain('off');
367+
expect(shown(page.container)).toBe('details right');
368+
expect(window.localStorage.getItem(RIGHT_KEY(LEAD))).toBeNull();
369+
370+
// The drawer closes: the chord is the page's again.
371+
drawer.unmount();
372+
sheet.remove();
373+
press('related');
374+
await waitFor(() => expect(shown(page.container)).toBe('details'));
375+
});
334376
});

‎ui/packages/app-shell/src/views/RecordDetailView.tsx‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,20 @@ function isTypingTarget(target: EventTarget | null): boolean {
164164
return target.closest('input, textarea, select, [contenteditable]:not([contenteditable="false"]), [role="textbox"]') !== null;
165165
}
166166

167+
/**
168+
* Whether this record view is the topmost one, the one a page-wide chord is
169+
* for. A record page keeps its own tree mounted under the drawer its rows and
170+
* lookups open (`RecordPageDrawerHost`), and the drawer's sheet is portaled
171+
* after it, to the end of the body; once the drawer is wide enough for a right
172+
* column its view listens too (objectstack#27524). The last record root in
173+
* document order is the one on top.
174+
*/
175+
function isTopmostRecordView(root: Element | null): boolean {
176+
if (!root) return false;
177+
const roots = root.ownerDocument.querySelectorAll('[data-record-page-root]');
178+
return roots[roots.length - 1] === root;
179+
}
180+
167181
/** `aria-keyshortcuts` spelling of a chord, for the platform the page runs on. */
168182
function ariaKeyShortcutsOf(chord: ShortcutChord): string {
169183
const mac = typeof navigator !== 'undefined' && /Mac|iPhone|iPad/.test(navigator.platform || '');
@@ -972,7 +986,7 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
972986
);
973987
// Whether the toolbar draws ⊟ — a right column the page has content for —
974988
// set at render below and read by the shortcut, so a shortcut never flips a
975-
// switch the reader cannot see.
989+
// switch the reader cannot see: not this page's under a drawer either.
976990
const drawnPanelSwitch = useRef(false);
977991
useEffect(() => {
978992
if (!pageHasRightColumn) return undefined;
@@ -981,12 +995,13 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
981995
if (!matchesPanelChord(e, TOGGLE_RELATED_SHORTCUT.chord)) return;
982996
if (isTypingTarget(e.target)) return;
983997
if (!drawnPanelSwitch.current) return;
998+
if (!isTopmostRecordView(pageRootEl)) return;
984999
e.preventDefault();
9851000
setRelatedSwitch(relatedSwitch === 'on' ? 'off' : 'on');
9861001
};
9871002
document.addEventListener('keydown', onKeyDown);
9881003
return () => document.removeEventListener('keydown', onKeyDown);
989-
}, [pageHasRightColumn, relatedSwitch, setRelatedSwitch]);
1004+
}, [pageHasRightColumn, pageRootEl, relatedSwitch, setRelatedSwitch]);
9901005
useAdvertiseShortcut(TOGGLE_RELATED_SHORTCUT, pageHasRightColumn);
9911006
const [pageRecord, setPageRecord] = useState<any>(null);
9921007
// 'idle' | 'loading' | 'loaded' | 'missing' | 'forbidden' | 'failed' —

‎ui/packages/plugin-detail/src/RecordPageColumns.tsx‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,25 +88,51 @@ function relatedNodeId(node: any): RelatedCardId | null {
8888
* record header, at the page's full width, with ‹ to return. Esc returns too;
8989
* a dialog or menu open over the page keeps its own Esc.
9090
*/
91+
/**
92+
* Whether an `Esc` belongs to something other than the open face: a field the
93+
* reader types in, an open menu or alert, or a dialog the face is not inside
94+
* (a popover, a confirm, a drawer opened over the page that holds the face).
95+
* The dialog that holds the face — the record pane's drawer — is not one.
96+
*/
97+
function escapeOwnedElsewhere(target: EventTarget | null, face: Element | null): boolean {
98+
if (!face) return true;
99+
if (
100+
target instanceof Element &&
101+
target.closest('input, textarea, select, [contenteditable]:not([contenteditable="false"]), [role="textbox"]')
102+
) {
103+
return true;
104+
}
105+
const doc = face.ownerDocument;
106+
if (doc.querySelector('[role="alertdialog"], [role="menu"]')) return true;
107+
return Array.from(doc.querySelectorAll('[role="dialog"]')).some((dialog) => !dialog.contains(face));
108+
}
109+
91110
const EnlargedFace: React.FC<{ id: string; title: string; onClose?: () => void; children: React.ReactNode }> = ({
92111
id,
93112
title,
94113
onClose,
95114
children,
96115
}) => {
97116
const { t } = useDetailTranslation();
117+
const faceRef = React.useRef<HTMLElement | null>(null);
98118
React.useEffect(() => {
99119
if (!onClose) return undefined;
120+
// On the window, in the capture phase: inside the record pane the pane's
121+
// own `Esc` (the drawer's dialog layer, the split pane's document
122+
// listener) would act first and close or step the pane (objectstack#27524).
123+
// The face takes the key, and keeps it from the pane.
100124
const onKeyDown = (event: KeyboardEvent) => {
101-
if (event.key !== 'Escape' || event.defaultPrevented) return;
102-
if (document.querySelector('[role="dialog"], [role="alertdialog"], [role="menu"]')) return;
125+
if (event.key !== 'Escape' || event.defaultPrevented || event.isComposing) return;
126+
if (escapeOwnedElsewhere(event.target, faceRef.current)) return;
127+
event.preventDefault();
128+
event.stopPropagation();
103129
onClose();
104130
};
105-
document.addEventListener('keydown', onKeyDown);
106-
return () => document.removeEventListener('keydown', onKeyDown);
131+
window.addEventListener('keydown', onKeyDown, true);
132+
return () => window.removeEventListener('keydown', onKeyDown, true);
107133
}, [onClose]);
108134
return (
109-
<section className="flex min-w-0 flex-col gap-3" data-related-enlarged={id} aria-label={title}>
135+
<section ref={faceRef} className="flex min-w-0 flex-col gap-3" data-related-enlarged={id} aria-label={title}>
110136
<div className="flex items-center gap-2">
111137
<Button
112138
type="button"
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectstack#27524 (Target 5, 6) — `Esc` on an open face (the 动态 tab, a
11+
* related list's whole face) returns to the record's page, inside the record
12+
* pane too. Since the pane follows the width rule its record draws these
13+
* faces, and the pane has an `Esc` of its own: the drawer's dialog layer
14+
* dismisses (or, below the stack's root, goes back a level), and the split
15+
* pane beside a list closes on a document listener. The face takes the key
16+
* first; only an `Esc` pressed with no face open is the pane's.
17+
*
18+
* Driven through the real `NavigationOverlay` (its Radix sheet and its split
19+
* listener), with the page's blocks stubbed: what is measured is who answers
20+
* the key.
21+
*/
22+
23+
import * as React from 'react';
24+
import { describe, it, expect, vi, afterEach } from 'vitest';
25+
import { render, cleanup, fireEvent } from '@testing-library/react';
26+
import { NavigationOverlay, type NavigationOverlayProps } from '@object-ui/components';
27+
28+
vi.mock('@object-ui/react', async (importOriginal) => ({
29+
...(await importOriginal<Record<string, unknown>>()),
30+
SchemaRenderer: ({ schema }: { schema: any }) => <div data-node={schema?.type} />,
31+
}));
32+
33+
import { RecordPageColumns } from '../RecordPageColumns';
34+
import { buildDefaultPageColumns, RECORD_PAGE_TIMELINE_PANEL } from '../synth/recordPageColumns';
35+
import { buildDefaultPageSchema } from '../synth/buildDefaultPageSchema';
36+
37+
const page = buildDefaultPageSchema(
38+
{ name: 'lead', label: 'Lead', fields: { name: { name: 'name', type: 'text' } } },
39+
{ related: [{ objectName: 'task', relationshipField: 'lead_id', title: 'Tasks' }] },
40+
);
41+
const columns = buildDefaultPageColumns(page)!;
42+
43+
/** The record the pane draws, opened on its 动态 tab. */
44+
function PaneRecord() {
45+
const [enlarged, setEnlarged] = React.useState<string | null>(RECORD_PAGE_TIMELINE_PANEL);
46+
return (
47+
<RecordPageColumns
48+
tier="stack"
49+
columns={columns}
50+
enlargedRelated={enlarged ?? undefined}
51+
onEnlargeRelated={setEnlarged}
52+
onCloseEnlargedRelated={() => setEnlarged(null)}
53+
/>
54+
);
55+
}
56+
57+
function pane(props: Partial<NavigationOverlayProps>) {
58+
return (
59+
<NavigationOverlay
60+
isOpen
61+
isOverlay
62+
selectedRecord={{ id: 'r1' }}
63+
mode="drawer"
64+
close={() => {}}
65+
setIsOpen={() => {}}
66+
title="Lead"
67+
width="720px"
68+
{...props}
69+
>
70+
{() => <PaneRecord />}
71+
</NavigationOverlay>
72+
);
73+
}
74+
75+
const face = () => document.querySelector(`[data-related-enlarged="${RECORD_PAGE_TIMELINE_PANEL}"]`);
76+
const pageColumns = () => document.querySelector('[data-record-columns]');
77+
const escape = (target: Element) => fireEvent.keyDown(target, { key: 'Escape', code: 'Escape', bubbles: true });
78+
79+
afterEach(() => cleanup());
80+
81+
describe('Esc on an open face inside the record pane (objectstack#27524)', () => {
82+
it('the drawer: Esc on the 动态 tab returns to the record\'s page; the next Esc is the drawer\'s', () => {
83+
const setIsOpen = vi.fn();
84+
render(pane({ setIsOpen }));
85+
expect(document.querySelector('[role="dialog"]')?.contains(face())).toBe(true);
86+
87+
escape(face()!.querySelector('[data-related-enlarged-back]')!);
88+
expect(face()).toBeNull();
89+
expect(pageColumns()).not.toBeNull();
90+
expect(setIsOpen).not.toHaveBeenCalled();
91+
92+
// CONTROL: with no face open, Esc dismisses the drawer as it always did.
93+
escape(pageColumns()!);
94+
expect(setIsOpen).toHaveBeenCalledWith(false);
95+
});
96+
97+
it('a level below the drawer\'s root: the face closes and the drawer stays on that level', () => {
98+
const onBack = vi.fn();
99+
const setIsOpen = vi.fn();
100+
render(pane({ onBack, setIsOpen }));
101+
escape(face()!);
102+
expect(face()).toBeNull();
103+
expect(onBack).not.toHaveBeenCalled();
104+
expect(setIsOpen).not.toHaveBeenCalled();
105+
escape(pageColumns()!);
106+
expect(onBack).toHaveBeenCalledTimes(1);
107+
});
108+
109+
it('the split pane beside a list: the face closes, the pane stays; the next Esc closes the pane', () => {
110+
const close = vi.fn();
111+
render(pane({ mode: 'split', mainContent: <div>list</div>, close }));
112+
expect(document.querySelector('[data-testid="record-split-pane"]')?.contains(face())).toBe(true);
113+
escape(face()!);
114+
expect(face()).toBeNull();
115+
expect(close).not.toHaveBeenCalled();
116+
escape(pageColumns()!);
117+
expect(close).toHaveBeenCalledTimes(1);
118+
});
119+
120+
it('an open menu owns Esc: the face stays open under it', () => {
121+
render(pane({}));
122+
const menu = document.createElement('div');
123+
menu.setAttribute('role', 'menu');
124+
document.body.appendChild(menu);
125+
escape(menu);
126+
expect(face()).not.toBeNull();
127+
menu.remove();
128+
});
129+
130+
it('a text field owns Esc: the face stays open while the reader types in it', () => {
131+
render(pane({}));
132+
const input = document.createElement('input');
133+
face()!.appendChild(input);
134+
escape(input);
135+
expect(face()).not.toBeNull();
136+
});
137+
});

0 commit comments

Comments
 (0)