diff --git a/.changeset/6859-injected-editor-commit-comment.md b/.changeset/6859-injected-editor-commit-comment.md new file mode 100644 index 0000000000..b0c39c2eb5 --- /dev/null +++ b/.changeset/6859-injected-editor-commit-comment.md @@ -0,0 +1,21 @@ +--- +--- + +No behaviour change, and deliberately so: this is the record being corrected, not the +renderer (objectui#6859). + +The data table's document-level `pointerdown` listener — the one that exits a host-injected +inline cell editor when you click away — justified itself with "the injected widgets (text, +number, date, lookup, …) have no such handler". That has not been true since objectui#6780 / +#6802: `onBlur` is a declared DOM pass-through key, and all 27 widgets reachable as an inline +editor deliver it to a real control (26 spread `toDomProps` themselves, `UserField` delegates +to `LookupField`). + +A source audit read the same absence as silent DATA LOSS on Tab-out. It is not. Driven in a +real browser against the real widgets, a value typed into a text, date or number cell editor +survives tabbing away, and reads back intact: the host wires each widget's `onChange` to the +table's `stage`, so every keystroke is already in `pendingChanges` while the editor is still +open. The listener exits EDIT MODE; it never rescued the value. Tabbing out does leave the +cell in edit mode until Enter, Escape, or a pointer press outside — a wart, not a lost edit. + +The comment now says all of that, and both facts are pinned by tests so they cannot rot back. diff --git a/packages/components/src/__tests__/data-table-injected-editor-focus-6859.test.tsx b/packages/components/src/__tests__/data-table-injected-editor-focus-6859.test.tsx new file mode 100644 index 0000000000..6369926d66 --- /dev/null +++ b/packages/components/src/__tests__/data-table-injected-editor-focus-6859.test.tsx @@ -0,0 +1,191 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * CHARACTERIZATION — what actually happens when focus leaves a HOST-INJECTED + * cell editor (objectui#6859). + * + * ## Why this file exists + * + * `data-table.tsx` exits an injected editor through a document-level + * `pointerdown` listener, and the comment at `injectedEditorElRef` used to + * justify that with "the injected widgets (text, number, date, lookup, …) have + * no such handler". That justification is stale — `onBlur` is a declared DOM + * pass-through key every inline-edit widget forwards (objectui#6780, #6802) — + * and correcting a comment leaves nothing behind that can rot loudly. A source + * audit then read the same absence as a DATA-LOSS defect: no `focusout`, no + * `onBlurCapture`, no `relatedTarget` anywhere in the file ⇒ Tab out of an + * injected editor must silently drop the typed value. + * + * It does not, and this file is the measurement that says why. The value never + * depends on the exit event: the host wires the widget's `onChange` to `stage`, + * so every keystroke is already in `pendingChanges` while the editor is still + * open. The `pointerdown` listener exits EDIT MODE; it does not rescue values. + * + * The same sequence was driven in a real Chromium against the real + * `@object-ui/fields` widgets (text / date / number) before this file was + * written; these are the jsdom pins for CI. + * + * ## The control + * + * Test A is the control and must stay green: a BUILT-IN editor DOES commit on + * focus loss. Without it, test C's negative ("Tab commits nothing") would pass + * just as well on a harness that cannot observe a commit at all. + */ +import { describe, it, expect, vi } from 'vitest'; +import { fireEvent, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { renderComponent } from './test-utils'; +// Module scope, not a hook — see object-ui/no-dynamic-import-in-test-hook. +import '../renderers'; + +const baseSchema = { + type: 'data-table' as const, + editable: true, + singleClickEdit: true, + columns: [ + { header: 'Name', accessorKey: 'name', editable: false }, + { header: 'Qty', accessorKey: 'qty', type: 'number' }, + ], + data: [{ id: '1', name: 'row-one', qty: '' }], +} as any; + +/** + * The injected editor exactly as the in-repo host builds it + * (`ObjectGrid.renderCellEditor`): a real control whose `onChange` is wired to + * the context's `stage` — non-discrete field types stage, they do not commit. + * `packages/components` does not depend on `@object-ui/fields`, so the wiring + * is reproduced rather than imported; the browser run linked above is what + * pins it to the real widgets. + */ +const stagingEditor = ({ column, value, stage }: any) => + column.accessorKey === 'qty' ? ( + stage(e.target.value)} + /> + ) : null; + +/** A tabbable element outside the table, so `userEvent.tab()` has somewhere to go. */ +function withOutsideTabStop(): HTMLButtonElement { + const btn = document.createElement('button'); + btn.id = 'outside-tab-stop'; + btn.textContent = 'outside'; + document.body.appendChild(btn); + return btn; +} + +describe('data-table — focus loss on a host-injected cell editor (objectui#6859)', () => { + it('A) CONTROL: a BUILT-IN editor DOES commit when focus leaves it', async () => { + // Proves the harness can observe a commit driven by focus loss at all, so + // the negative results below are measurements and not dead probes. + const onCellChange = vi.fn(); + const { container } = renderComponent({ ...baseSchema, onCellChange }); + + const qtyCell = container.querySelectorAll('tbody td')[1] as HTMLElement; + fireEvent.click(qtyCell); + + const input = qtyCell.querySelector('input') as HTMLInputElement; + expect(input).toBeTruthy(); + fireEvent.change(input, { target: { value: '42' } }); + fireEvent.blur(input); + + expect(onCellChange).toHaveBeenCalledWith(0, 'qty', '42', expect.anything()); + }); + + it('B) an injected editor stages every keystroke — the value is pending BEFORE any exit event', () => { + // This is the path the source audit could not see, and the reason Tab-out + // is not lossy: `stage` writes straight into `pendingChanges` while the + // editor is still open and still focused. + const { container, getByText, queryByText } = renderComponent({ + ...baseSchema, + renderCellEditor: stagingEditor, + }); + + const qtyCell = container.querySelectorAll('tbody td')[1] as HTMLElement; + // Nothing pending yet — the toolbar's save affordance is the readout. + expect(queryByText(/Save All/i)).toBeNull(); + + fireEvent.click(qtyCell); + const editor = qtyCell.querySelector('[data-testid="injected-editor"]') as HTMLInputElement; + expect(editor).toBeTruthy(); + fireEvent.change(editor, { target: { value: 'TYPED' } }); + + // Still in edit mode, nothing committed — and the value is already staged. + expect(qtyCell.querySelector('[data-testid="injected-editor"]')).toBeTruthy(); + expect(getByText(/Save All/i)).toBeInTheDocument(); + }); + + it('C) tabbing out of an injected editor neither commits nor leaves edit mode', async () => { + const onCellChange = vi.fn(); + const outside = withOutsideTabStop(); + try { + const { container } = renderComponent({ + ...baseSchema, + onCellChange, + renderCellEditor: stagingEditor, + }); + + const qtyCell = container.querySelectorAll('tbody td')[1] as HTMLElement; + fireEvent.click(qtyCell); + const editor = qtyCell.querySelector('[data-testid="injected-editor"]') as HTMLInputElement; + fireEvent.change(editor, { target: { value: 'TYPED' } }); + + editor.focus(); + expect(document.activeElement).toBe(editor); + await userEvent.tab(); + + // Focus really left the editor … + expect(document.activeElement).not.toBe(editor); + // … and nothing committed: no exit, no onCellChange. This is the fact the + // corrected comment at `injectedEditorElRef` now states, and the reason + // the document-level pointerdown listener is still load-bearing. + expect(onCellChange).not.toHaveBeenCalled(); + expect(qtyCell.querySelector('[data-testid="injected-editor"]')).toBeTruthy(); + } finally { + outside.remove(); + } + }); + + it('D) …and the typed value is still there — the outside pointer press commits exactly it', async () => { + const onCellChange = vi.fn(); + const outside = withOutsideTabStop(); + try { + const { container } = renderComponent({ + ...baseSchema, + onCellChange, + renderCellEditor: stagingEditor, + }); + + const qtyCell = container.querySelectorAll('tbody td')[1] as HTMLElement; + fireEvent.click(qtyCell); + const editor = qtyCell.querySelector('[data-testid="injected-editor"]') as HTMLInputElement; + fireEvent.change(editor, { target: { value: 'TYPED' } }); + + editor.focus(); + await userEvent.tab(); + // A pointer press truly outside is what exits the editor today. + fireEvent.pointerDown(outside); + + await waitFor(() => + expect(onCellChange).toHaveBeenCalledWith(0, 'qty', 'TYPED', expect.anything()), + ); + // Edit mode is over and the cell reads back what was typed — nothing lost + // across the Tab-out. + await waitFor(() => + expect(qtyCell.querySelector('[data-testid="injected-editor"]')).toBeNull(), + ); + expect(qtyCell.textContent).toContain('TYPED'); + } finally { + outside.remove(); + } + }); +}); diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 3213a8c360..57efaae010 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -1002,11 +1002,37 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // don't double-commit (Enter) or resurrect a cancelled value (Escape). const skipBlurSaveRef = useRef(false); // DOM node of a host-injected widget editor (rendered via `renderCellEditor`), - // captured while it's mounted. The built-in `` editors commit via their - // own onBlur, but the injected widgets (text, number, date, lookup, …) have no - // such handler — a document-level pointerdown listener (see below) uses this - // node to detect click-outside and commit them. Null ⇒ no injected editor is - // active (a built-in editor, or nothing, is showing). + // captured while it's mounted, so the document-level pointerdown listener + // below can tell "inside this editor" from "outside" and exit edit mode. + // Null ⇒ no injected editor is active (a built-in editor, or nothing, is + // showing). + // + // This used to justify itself with "the injected widgets (text, number, date, + // lookup, …) have no such handler". That claim is no longer true and is no + // longer the reason (objectui#6859). `onBlur` is a DECLARED DOM pass-through + // key — named in `FieldWidgetDomProps` (`@object-ui/fields`), named in + // `SDUI_DOM_PASS_THROUGH_KEYS` (`@object-ui/core`), forwarded by + // `toDomProps` — and every widget reachable as an inline editor spreads that + // whitelist onto a real control (26 of the 27 components in `EDIT_WIDGETS` + // call `toDomProps` directly; `UserField` delegates its whole props object to + // `LookupField`, which does). The five widgets that own a blur handler now + // COMPOSE the host's rather than overriding it (objectui#6780, #6802). + // + // The listener is still needed, for a different reason: NOTHING EVER HANDS + // THE WIDGET ONE. The wrapper below carries `onKeyDown` alone, and the + // context object `renderCellEditor` receives — `{ column, row, value, stage, + // commit, cancel }` — has no DOM-props slot to put an `onBlur` in. The + // in-repo factory behind that seam, `@object-ui/fields`' `FieldEditWidget`, + // forwards `autoFocus` and nothing else out of the DOM block, so a host + // handler could not reach the control through it even if one were passed. + // + // Note also what the listener is NOT load-bearing for. Its job is exiting + // EDIT MODE, not rescuing the value: injected widgets stage on every change + // (the host wires the widget's `onChange` to `stageEdit` below), so a typed + // value is already in `pendingChanges` before any exit event — measured in a + // real browser on the text, date and number editors for objectui#6859. + // Retiring this listener would strand cells in edit mode; it would not drop + // edits. const injectedEditorElRef = useRef(null); // Snapshot of the active cell's pending value when editing began, so Escape / // cancel can revert this session's changes. Injected widgets stage on every @@ -1728,10 +1754,12 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // Commit a host-injected widget editor on click-outside (objectui#2321). // - // Built-in `` editors commit via their own onBlur (handleEditBlur), but - // the widgets injected through `renderCellEditor` (text, number, date, lookup, - // …) have no such handler, so without this they stay stuck in edit mode when - // the user clicks away. A capture-phase document listener (capture so a cell's + // Built-in `` editors commit via their own onBlur (handleEditBlur). The + // widgets injected through `renderCellEditor` (text, number, date, lookup, …) + // never receive one — not because they cannot deliver it (they can, and do: + // see `injectedEditorElRef` above and objectui#6859) but because nothing on + // this seam passes it to them — so without this they stay stuck in edit mode + // when the user clicks away. A capture-phase document listener (capture so a cell's // own `stopPropagation` can't hide it) commits the staged value and exits edit // mode when the pointer goes down truly outside the editor — but NOT inside a // Radix overlay the widget itself opened (a lookup popover / record-picker @@ -2302,6 +2330,15 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // picker's `