From 97c51df47b4fdbaaa0c93b7c59a32b6ac0ba3061 Mon Sep 17 00:00:00 2001 From: "ag-jira-agent-ci[bot]" <286720198+ag-jira-agent-ci[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:34:27 +0100 Subject: [PATCH 1/8] AG-16759 Fix reverse tab out of a conditionally-editable cell (#14903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * AG-16759 Fix reverse tab out of a conditionally-editable cell The backwards branch of navigationService.onTabKeyDown only tabbed out to the header when the originally focused cell was on the first row. With a function-valued `editable` (conditional editing), the backwards editable-cell walk can exhaust from any row, so preventDefault() was never called and the browser focused the `.ag-row` element instead — the whole row appearing selected. Removes the firstRow gate so an exhausted backwards walk always tabs out to the header, mirroring the forwards branch, which has no row check either. Co-Authored-By: Claude Opus 5 (1M context) * AG-16759 Drop ticket keys and change narration from the new comments Code comments must carry the durable why, not the history of the diff. Co-Authored-By: Claude Opus 5 (1M context) * AG-16759 Cover the backwards tab-out from a non-first row with a suppressNavigable guard --------- Co-authored-by: claude[bot] Co-authored-by: Claude Opus 5 (1M context) --- .../src/navigation/navigationService.ts | 20 ++--- ...ing-conditional-editing-navigation.test.ts | 84 +++++++++++++++++++ .../cell-editing-tab-editor-react.test.tsx | 33 ++++++++ .../suppress-navigable-navigation.test.ts | 25 ++++++ 4 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts diff --git a/packages/ag-grid-community/src/navigation/navigationService.ts b/packages/ag-grid-community/src/navigation/navigationService.ts index fbec2ab0771..0bc5bed2bd5 100644 --- a/packages/ag-grid-community/src/navigation/navigationService.ts +++ b/packages/ag-grid-community/src/navigation/navigationService.ts @@ -419,7 +419,7 @@ export class NavigationService extends BeanStub implements NamedBean { const movedToNextCell = this.tabToNextCellCommon(previous, backwards, keyboardEvent); const beans = this.beans; - const { ctrlsSvc, pageBounds, focusSvc, gos } = beans; + const { ctrlsSvc, focusSvc, gos } = beans; if (movedToNextCell !== false) { // only prevent default if we found a cell. so if user is on last cell and hits tab, then we default @@ -434,17 +434,15 @@ export class NavigationService extends BeanStub implements NamedBean { } // if we didn't move to next cell, then need to tab out of the cells, ie to the header (if going - // backwards) + // backwards). Reached only when the walk found no cell at all, which can happen from any row - + // e.g. a function-valued `editable`, or an entirely suppressNavigable path - so there is + // deliberately no first-row check, mirroring the forwards branch below. if (backwards) { - const { rowIndex, rowPinned } = previous.getRowPosition(); - const firstRow = rowPinned ? rowIndex === 0 : rowIndex === pageBounds.getFirstRow(); - if (firstRow) { - if (gos.get('headerHeight') === 0 || _isHeaderFocusSuppressed(beans)) { - _focusNextGridCoreContainer(beans, true, 'force'); - } else { - keyboardEvent.preventDefault(); - focusSvc.focusPreviousFromFirstCell(keyboardEvent); - } + if (gos.get('headerHeight') === 0 || _isHeaderFocusSuppressed(beans)) { + _focusNextGridCoreContainer(beans, true, 'force'); + } else { + keyboardEvent.preventDefault(); + focusSvc.focusPreviousFromFirstCell(keyboardEvent); } } else { // anchor container navigation on the cell when focus is in an editor or renderer child. diff --git a/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts b/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts new file mode 100644 index 00000000000..2920e4b54f5 --- /dev/null +++ b/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts @@ -0,0 +1,84 @@ +import { userEvent } from '@testing-library/user-event'; +import { TestGridsManager, waitForInput } from 'ag-test-utils'; + +import type { ColDef, GridApi } from 'ag-grid-community'; +import { + ClientSideRowModelModule, + NumberEditorModule, + TextEditorModule, + getGridElement, + setupAgTestIds, +} from 'ag-grid-community'; + +interface PersonRow { + athlete: string; + age: number; + editable: boolean; +} + +const columnDefs: ColDef[] = [ + { field: 'athlete', editable: (params) => !!params.data?.editable }, + { field: 'age', editable: (params) => !!params.data?.editable }, +]; + +const rowData: PersonRow[] = [ + { athlete: 'Alice', age: 23, editable: false }, + { athlete: 'Bob', age: 40, editable: false }, + { athlete: 'Carol', age: 31, editable: true }, +]; + +describe('Conditional editing reverse tab navigation', () => { + const gridsManager = new TestGridsManager({ + includeDefaultModules: true, + modules: [ClientSideRowModelModule, NumberEditorModule, TextEditorModule], + }); + + beforeAll(() => { + setupAgTestIds(); + }); + + afterEach(() => { + gridsManager.reset(); + vi.clearAllMocks(); + }); + + const cell = (api: GridApi, rowIndex: number, colId: string): HTMLElement => + (getGridElement(api)! as HTMLElement).querySelector( + `[row-index="${rowIndex}"] [col-id="${colId}"]` + )!; + + test('Shift+Tab out of the only editable cell moves focus to the last header cell', async () => { + const user = userEvent.setup(); + const api = await gridsManager.createGridAndWait('ag-16759-a', { columnDefs, rowData }); + + await user.dblClick(cell(api, 2, 'athlete')); + await waitForInput(cell(api, 2, 'athlete')); + + await user.keyboard('{Shift>}{Tab}{/Shift}'); + + const active = document.activeElement as HTMLElement; + expect(active?.classList.contains('ag-header-cell')).toBe(true); + expect(active?.getAttribute('col-id')).toBe('age'); + }); + + test('Repeated Shift+Tab up into the header throws no error', async () => { + const errors: unknown[] = []; + const onError = (e: ErrorEvent) => errors.push(e.error ?? e.message); + window.addEventListener('error', onError); + try { + const user = userEvent.setup(); + const api = await gridsManager.createGridAndWait('ag-16759-b', { columnDefs, rowData }); + + await user.dblClick(cell(api, 2, 'athlete')); + await waitForInput(cell(api, 2, 'athlete')); + + for (let i = 0; i < 8; i++) { + await user.keyboard('{Shift>}{Tab}{/Shift}'); + } + + expect(errors).toHaveLength(0); + } finally { + window.removeEventListener('error', onError); + } + }); +}); diff --git a/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx b/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx index 7f761115c97..89982031d9d 100644 --- a/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx +++ b/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx @@ -554,6 +554,39 @@ describe('Cell Editing: tab into editor in React', () => { }); }); + // With a function-valued `editable`, Shift+Tab out of the only editable cell must tab out to + // the header rather than letting the browser focus the row element. React twin of the vanilla guard in + // cell-editing-conditional-editing-navigation.test.ts. + test('conditional editing: Shift+Tab out of the only editable cell focuses the last header cell', async () => { + const { gridDiv, user } = await renderGrid({ + rowData: [ + { id: '0', athlete: 'Alice', age: 23, editable: false }, + { id: '1', athlete: 'Bob', age: 40, editable: false }, + { id: '2', athlete: 'Carol', age: 31, editable: true }, + ], + columnDefs: [ + { field: 'athlete', editable: (params: any) => !!params.data?.editable }, + { field: 'age', editable: (params: any) => !!params.data?.editable }, + ], + modules: [ClientSideRowModelModule, TextEditorModule, NumberEditorModule], + }); + + const editableCell = getByTestId(gridDiv, agTestIdFor.cell('2', 'athlete')); + await user.dblClick(editableCell); + + await waitFor(() => { + expect(editableCell.querySelector('.ag-cell-edit-wrapper input')).toBeTruthy(); + }); + + await user.keyboard('{Shift>}{Tab}{/Shift}'); + + await waitFor(() => { + const active = document.activeElement as HTMLElement; + expect(active?.classList.contains('ag-header-cell')).toBe(true); + expect(active?.getAttribute('col-id')).toBe('age'); + }); + }); + describe('editType: fullRow', () => { // fullRow: cellStartedEdit is true for the focused cell on initial dblClick test('fullRow: cellStartedEdit is true for the focused cell on initial edit', async () => { diff --git a/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts b/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts index 8beeb3e68de..8c726ce3689 100644 --- a/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts +++ b/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts @@ -134,4 +134,29 @@ describe('suppressNavigable Navigation', () => { expect(getFocusedRowIndex(api)).toBe(1); }); }); + + describe('backwards tab out with nowhere to go', () => { + // AG-16759: the backwards tab-out is reached whenever the walk finds no cell at all, which + // for a row-dependent suppressNavigable happens from a row that is not the first one. + let api: GridApi; + + beforeEach(() => { + const columnDefs: ColDef[] = [ + { field: 'a', colId: 'a', suppressNavigable: (params) => params.data?.a === 'a0' }, + ]; + api = gridsManager.createGrid('myGrid', { + columnDefs, + rowData, + } as GridOptions); + }); + + test('Shift+Tab from a non-first row moves focus to the header', () => { + api.setFocusedCell(1, 'a'); + dispatchKeyDown(KeyCode.TAB, { shiftKey: true }); + + const active = document.activeElement as HTMLElement | null; + expect(active?.classList.contains('ag-header-cell')).toBe(true); + expect(active?.getAttribute('col-id')).toBe('a'); + }); + }); }); From f8a70082aea4ea9004751ca1463d130c2ca66eea Mon Sep 17 00:00:00 2001 From: "ag-jira-agent-ci[bot]" <286720198+ag-jira-agent-ci[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:17:07 +0100 Subject: [PATCH 2/8] AG-18249 Dismiss the bigint filter popup with Escape instead of a covered cell click (#14912) The greater-than spec closed the balance filter popup by clicking a cell the popup overlays, so on firefox and webkit the click was intercepted and retried until the 60s timeout. Press Escape and settle on the filtered header class, matching the two filter-date specs that already fixed the same problem. Co-authored-by: claude[bot] Co-authored-by: Claude Opus 5 (1M context) --- .../filter-bigint/_examples/bigint-filter/example.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/documentation/ag-grid-docs/src/content/docs/filter-bigint/_examples/bigint-filter/example.spec.ts b/documentation/ag-grid-docs/src/content/docs/filter-bigint/_examples/bigint-filter/example.spec.ts index ee7132295a0..a5d8617d778 100644 --- a/documentation/ag-grid-docs/src/content/docs/filter-bigint/_examples/bigint-filter/example.spec.ts +++ b/documentation/ag-grid-docs/src/content/docs/filter-bigint/_examples/bigint-filter/example.spec.ts @@ -33,7 +33,11 @@ test.agExample(import.meta, () => { const filterInput = agIdFor.textFilterInstanceInput({ source: 'column-filter' }); await filterInput.fill('45000000000000000'); - await agIdFor.cell('4', 'account').click(); + // Close the filter popup and confirm the filter is applied. Press Escape rather than + // clicking a data cell: `balance` is a middle column, so its popup overlays `account` and on + // firefox/webkit the popup intercepts the click, which then retries until the test times out. + await page.keyboard.press('Escape'); + await expect(agIdFor.headerCell('balance')).toHaveClass(/ag-header-cell-filtered/); // Echo (1e17) and Foxtrot (1.1e17) exceed the threshold; Delta (equal to it) is excluded. await expect(agIdFor.cell('4', 'account')).toHaveText('Echo'); From 07707e9cb7bb8617f95487049e2b2d668d7dfcb5 Mon Sep 17 00:00:00 2001 From: "ag-jira-agent-ci[bot]" <286720198+ag-jira-agent-ci[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:16:11 +0100 Subject: [PATCH 3/8] AG-16405 Cycle sortingOrder by position so repeated entries are reachable (#14817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * AG-16405 Cycle sortingOrder by position so repeated entries are reachable Header-click sort progression resolved its position in `sortingOrder` by value, so a repeated entry always resolved back to its first occurrence and every entry after it was unreachable (e.g. `['asc','desc','asc',null]` pinned to indices 0<->1, leaving the trailing `null` — clearing the sort — permanently out of reach). `AgColumn` now remembers the cycle position (`sortCycleIndex`, internal only — not part of `ColumnState` and never saved/restored). `progressSort` stamps it only after the sort has actually been applied to the column, and `SortService.setColSort` clears it unconditionally on every sort write, so `applyColumnState`, grid state, the column menu and sibling-clearing all fall back to today's first-matching-entry scan. Co-Authored-By: Claude Opus 5 (1M context) * AG-16405 Drop the docs/JSDoc notes and trim comments per review * Clean up the code to be less verbose * AG-16405 Stamp the sort cycle index before dispatch and honour it in getNextSortDirection * AG-16405 Cover the cycle-index consistency and re-entrant reset cases * AG-16405 Reset the sort cycle indexes when the grid-level sortingOrder changes * AG-16405 Revert the grid-level sortingOrder cycle-index reset This reverts commit 00deed486885c17ec66d1ee5934e5cd2db327014. Requested by the maintainer on the ticket: the grid-level sortingOrder change is a very edge case with no bad fallback, so the extra code is not worth carrying. --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: claude[bot] Co-authored-by: Stephen Cooper --- .../src/entities/agColumn.ts | 3 + .../ag-grid-community/src/sort/sortService.ts | 45 +++- .../src/sorting/sort-service.test.ts | 198 ++++++++++++++++++ 3 files changed, 236 insertions(+), 10 deletions(-) diff --git a/packages/ag-grid-community/src/entities/agColumn.ts b/packages/ag-grid-community/src/entities/agColumn.ts index 15cbe98eef3..9dc423de370 100644 --- a/packages/ag-grid-community/src/entities/agColumn.ts +++ b/packages/ag-grid-community/src/entities/agColumn.ts @@ -150,6 +150,8 @@ export class AgColumn public tooltipFieldContainsDots: boolean = false; // ── Cold ── structure, transient interaction state, indices, events. + /** Position in the resolved `sortingOrder` of the last header-click sort. Internal, not saved. */ + public sortCycleIndex: number | undefined = undefined; private frameworkEventListenerService: IFrameworkEventListenerService | undefined = undefined; // Lazy — most columns never get a listener; allocated on first __addEventListener/addEventListener. private colEventSvc: LocalEventService | null = null; @@ -269,6 +271,7 @@ export class AgColumn } ++this.beans.colModel.colDefsVersion; // a real colDef change invalidates anything derived from them this.cachedSortTypes = null; // sort/initialSort/sortingOrder may have changed + this.sortCycleIndex = undefined; this.initColDefHotFields(); this.beans.showValuesAsSvc?.resolveColumn(this, false); // colDef change — `initialShowValuesAs` is create-only this.initMinAndMaxWidths(); diff --git a/packages/ag-grid-community/src/sort/sortService.ts b/packages/ag-grid-community/src/sort/sortService.ts index 3c318822cd7..2cb71711e16 100644 --- a/packages/ag-grid-community/src/sort/sortService.ts +++ b/packages/ag-grid-community/src/sort/sortService.ts @@ -37,7 +37,8 @@ export class SortService extends BeanStub implements NamedBean { } public progressSort(column: AgColumn, multiSort: boolean, source: ColumnEventType): void { - this.setSortForColumn(column, this.getNextSortDirection(column), multiSort, source); + const { sortDef, index } = this.getNextSortDefAndIndex(column, column.getSortDef(), column.sortCycleIndex); + this.setSortForColumn(column, sortDef, multiSort, source, index); } public progressSortFromEvent(column: AgColumn, event: MouseEvent | KeyboardEvent): void { @@ -46,7 +47,13 @@ export class SortService extends BeanStub implements NamedBean { this.progressSort(column, multiSort, 'uiColumnSorted'); } - public setSortForColumn(column: AgColumn, sortDef: SortDef, multiSort: boolean, source: ColumnEventType): void { + public setSortForColumn( + column: AgColumn, + sortDef: SortDef, + multiSort: boolean, + source: ColumnEventType, + cycleIndex?: number + ): void { const { gos, showRowGroupCols } = this.beans; const coupled = _isColumnsSortingCoupledToGroup(gos); @@ -61,8 +68,9 @@ export class SortService extends BeanStub implements NamedBean { } } + // Only the clicked column (always first) carries the cycle position; coupled sources share the def. for (let i = 0, len = columnsToUpdate.length; i < len; ++i) { - this.setColSort(columnsToUpdate[i], sortDef, source); + this.setColSort(columnsToUpdate[i], sortDef, source, i === 0 ? cycleIndex : undefined); } const displayCol = coupled ? column.showRowGroupCol : null; @@ -162,20 +170,35 @@ export class SortService extends BeanStub implements NamedBean { } public getNextSortDirection(column: AgColumn, currentSort?: SortDef | SortDirection | null): SortDef { + const useCycle = currentSort === undefined; + const currentSortDef = useCycle ? column.getSortDef() : getSortDefFromInput(currentSort); + // Without an explicit current sort this must agree with `progressSort`, so honour the cycle position. + return this.getNextSortDefAndIndex(column, currentSortDef, useCycle ? column.sortCycleIndex : 0).sortDef; + } + + /** Next `sortingOrder` entry and its index. The scan starts at `cachedIndex` when that is a valid + * position, so a repeated entry resolves to the one the last click landed on rather than always to + * its first occurrence. */ + private getNextSortDefAndIndex( + column: AgColumn, + currentSortDef: SortDef | null, + cachedIndex = 0 + ): { sortDef: SortDef; index: number } { const sortingOrder = getSortingOrder(this.gos, column); const len = sortingOrder.length; if (len === 0) { - return getSortDefFromInput(); + return { sortDef: getSortDefFromInput(), index: 0 }; } - const currentSortDef = currentSort === undefined ? column.getSortDef() : getSortDefFromInput(currentSort); - let next = 0; - for (let i = 0; i < len; ++i) { + let current = -1; + for (let i = cachedIndex < len ? cachedIndex : 0; i < len; ++i) { if (areSortDefsEqual(sortingOrder[i], currentSortDef)) { - next = i + 1 >= len ? 0 : i + 1; + current = i; break; } } - return getSortDefFromInput(sortingOrder[next]); + // No match, or the last entry -> restart at the first entry. + const index = current === -1 || current + 1 >= len ? 0 : current + 1; + return { sortDef: getSortDefFromInput(sortingOrder[index]), index }; } private getSortedCols(): AgColumn[] { @@ -344,8 +367,10 @@ export class SortService extends BeanStub implements NamedBean { } } - private setColSort(column: AgColumn, sortDef: SortDef, source: ColumnEventType): void { + private setColSort(column: AgColumn, sortDef: SortDef, source: ColumnEventType, cycleIndex?: number): void { const prevSortDef = column.getSortDef(); + // Stamped before the events below, so a re-entrant sort write during dispatch clears it and wins. + column.sortCycleIndex = cycleIndex; if (!areSortDefsEqual(prevSortDef, sortDef)) { // Presence flip changes membership (drop all); direction/type-only keeps order (drop opts). if (!!prevSortDef?.direction !== !!sortDef.direction) { diff --git a/testing/behavioural/src/sorting/sort-service.test.ts b/testing/behavioural/src/sorting/sort-service.test.ts index 09a31fdbb62..cc1d887053e 100644 --- a/testing/behavioural/src/sorting/sort-service.test.ts +++ b/testing/behavioural/src/sorting/sort-service.test.ts @@ -1733,6 +1733,204 @@ describe('SortService', () => { `); }); + test('header click visits every entry of a sortingOrder containing a repeated entry: asc -> desc -> asc -> none', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['1', '3', '2']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['2', '3', '1']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-descending-icon']); + + // third entry is a repeat of the first: it must be visited at its own position + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['1', '3', '2']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + + // fourth entry (null) must be reachable - the user can clear the sort by clicking again + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['1', '2', '3']); + expect(getSortModel(api)).toEqual([]); + expect(visibleSortIcons(api, 'n')).toEqual([]); + await new GridColumns(api).checkColumns(` + CENTER + └── n "N" width:200 + `); + }); + + test('header click wraps back to the first entry after a sortingOrder with a repeated entry', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + for (let i = 0; i < 4; ++i) { + clickHeader(api, 'n'); + await asyncSetTimeout(0); + } + expect(getSortModel(api)).toEqual([]); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['1', '3', '2']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + }); + + test('applyColumnState resets the cycle so the next header click continues from the first matching entry', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + for (let i = 0; i < 3; ++i) { + clickHeader(api, 'n'); + await asyncSetTimeout(0); + } + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + + api.applyColumnState({ state: [{ colId: 'n', sort: 'asc' }] }); + await asyncSetTimeout(0); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['2', '3', '1']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-descending-icon']); + }); + + test('a colDef change resets the cycle so the next header click continues from the first matching entry', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + // Three clicks land on the repeated 'asc' at index 2, so the cycle position is 2. + for (let i = 0; i < 3; ++i) { + clickHeader(api, 'n'); + await asyncSetTimeout(0); + } + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + + // A new sortingOrder in which index 2 is still 'asc' - only a reset makes the next click + // resolve to index 0 and advance to 'desc'; a retained position would wrap back to 'asc'. + api.setGridOption('columnDefs', [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc'] }]); + await asyncSetTimeout(0); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-descending-icon']); + }); + + test('a header click after a sort that matches no sortingOrder entry restarts at the first entry', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + api.applyColumnState({ state: [{ colId: 'n', sort: 'asc', sortType: 'absolute' }] }); + await asyncSetTimeout(0); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(rowOrder(api)).toEqual(['1', '3', '2']); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + }); + + test('getNextSortDirection reports what the next header click will apply at a repeated entry', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + // Three clicks land on the repeated 'asc' at index 2, so the next entry is the trailing null. + for (let i = 0; i < 3; ++i) { + clickHeader(api, 'n'); + await asyncSetTimeout(0); + } + + const column = api.getColumn('n') as any; + expect(column.beans.sortSvc.getNextSortDirection(column)).toEqual({ direction: null, type: 'default' }); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(getSortModel(api)).toEqual([]); + }); + + test('a sort written re-entrantly from a sortChanged listener resets the cycle position', async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [{ colId: 'n', field: 'n', sortingOrder: ['asc', 'desc', 'asc', null] }], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + // Two clicks leave the cycle at 'desc' (index 1); the third would land on 'asc' at index 2. + for (let i = 0; i < 2; ++i) { + clickHeader(api, 'n'); + await asyncSetTimeout(0); + } + + // A column-level listener is dispatched synchronously, so its write lands mid-progression. + let reapplied = false; + api.getColumn('n')!.addEventListener!('sortChanged', () => { + if (reapplied) { + return; + } + reapplied = true; + api.applyColumnState({ state: [{ colId: 'n', sort: 'asc' }] }); + }); + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + + // The listener's write is the last one, so the cycle restarts at the first matching entry + // ('asc' at index 0) and advances to 'desc' - a retained index 2 would clear the sort instead. + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-descending-icon']); + }); + + test("header click cycles the ticket's mixed absolute sortingOrder: abs-desc -> asc -> abs-asc", async () => { + const api = gridMgr.createGrid('g', { + columnDefs: [ + { + colId: 'n', + field: 'n', + sortingOrder: [ + { type: 'absolute', direction: 'desc' }, + 'asc', + { type: 'absolute', direction: 'asc' }, + ], + }, + ], + rowData: signedRowData, + getRowId: (p) => p.data.id, + }); + + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-absolute-descending-icon']); + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-ascending-icon']); + clickHeader(api, 'n'); + await asyncSetTimeout(0); + expect(visibleSortIcons(api, 'n')).toEqual(['ag-sort-absolute-ascending-icon']); + }); + test('absolute sort applied via the column menu on a column with the default sorting order orders by magnitude and shows the absolute icon', async () => { const api = menuGridMgr.createGrid('menu-abs', { columnDefs: [ From e74bb6443d6b363720af8ada1292ca099dc077b2 Mon Sep 17 00:00:00 2001 From: Tak Tran Date: Thu, 20 Aug 2026 11:20:12 +0100 Subject: [PATCH 4/8] AG-18231 Bound the Playwright install on the Nx setup path and re-enable e2e (#14908) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * AG-18231 Bound the Playwright install on the Nx setup path and re-enable e2e `playwright install --with-deps` shells out to `apt-get update`, which has no timeout. When the Ubuntu mirror stalls the install hangs indefinitely: on CI run 32232167652 the e2e job sat 31 minutes inside `ag-grid-community:setup` without running a single test, and its re-run went on to burn three hours, because no job in ci.yml carried a `timeout-minutes` and the 6-hour default was the only ceiling. The bounded, retrying wrapper added for this in #14880 only covered doc-tests.yml. The Nx `setup` targets that `nx test:e2e` depends on still called Playwright directly, so the code-CI path kept hanging. - Promote install-playwright.sh out of the test-framework-examples action into scripts/ci/, so the Nx targets can share it, and point both existing callers at the new path. - Route all 11 `setup` targets through it. `{workspaceRoot}` only interpolates at the start of an option and resolves relative to the command's cwd, so the install-only targets drop their `cwd` (irrelevant to a browser download) and accessibility's install splits into a `setup:playwright` target its `setup` depends on, leaving the cwd-sensitive npm scripts as they were. - Record the install outcome per runner, in CI only. Nx does not bail on a failed task, so a stalled mirror would otherwise cost every one of the six e2e projects its full ~30 min budget — enough in series to overshoot the job cap and get the job cancelled rather than failed, losing the report upload the bounds exist to preserve. One project now pays; the rest report the same error immediately. - Teach the script to run outside Actions, since it is now on a developer path: plain output instead of workflow commands, and an unbounded run with a warning where GNU `timeout` is absent (stock macOS), which is the pre-existing behaviour rather than a regression. - Give every ci.yml job a `timeout-minutes`, at roughly 4-6x its observed duration on that run. e2e gets 90, matching doc-tests.yml and sitting above the installer's ~32 min give-up so the script fails first with a diagnosis. - Re-enable the e2e job disabled in #14899, reverting that hunk exactly. * AG-18231 Record installed Playwright deps per browser, and only short-circuit on a stall Addresses both P1 findings from review. The success marker was a single flag, so a `full chromium` install satisfied every later request - including `full chromium firefox webkit`, which was downgraded to a browser-only download and silently skipped the OS libraries Firefox and WebKit pull in. It is now a per-browser record, and a request short-circuits only when every browser it asks for is already covered. Nothing is keyed by Playwright version: the state is scoped to one ephemeral runner, which is one checkout, so the version cannot change underneath it. The failure marker was written for any failed install, so a caller that lost a race for the dpkg lock - seconds, not a stall - could condemn a runner whose dependencies another caller was concurrently installing correctly. Only a stall, an attempt killed at its bound, is recorded now. That is the case the short-circuit exists for; a fast failure has not cost the budget worth avoiding twice, and may well be transient. No lock is added. Concurrent callers running apt at once is what they already do with no records at all, so the records do not make it worse: apt's own locking serialises them and the retries absorb it. The hazard the records did introduce was a race loser poisoning the runner, which is fixed above. A blocking runner-wide lock would also reintroduce the unbounded wait this script exists to remove. * AG-18231 Wait for the apt lock between install attempts instead of guessing a delay Run 32269132733 hit the stall this script exists for, and the bound worked: attempt 1 was killed at its 900s limit rather than hanging the job, and the other five projects short-circuited instead of repeating it, so the job failed cleanly in 20 minutes where it used to hang for six hours. But the retry was wasted. `timeout` signals only its direct child, `npx`; the `apt-get` Playwright starts under sudo is a grandchild and survived the kill. 20 seconds later it still held /var/lib/apt/lists/lock - the log names process 2808 - so attempt 2 died on "Could not get lock" in about a second, having never reached the mirror. The fixed `sleep` was a guess at how long a TERM-ed apt needs, and it guessed wrong; the retry budget was spent on a collision with ourselves. Here it cost nothing, because the mirror was still down. On a transient stall it would have thrown away the attempt that recovers. - Wait for apt to actually release its lock before retrying, bounded by PW_INSTALL_APT_WAIT_SECONDS (120s), instead of sleeping a fixed delay and hoping. - Stop counting a lock collision as an attempt. It is not evidence about the mirror, so it must not spend the budget reserved for finding out. Bounded separately by PW_INSTALL_MAX_LOCK_RETRIES (2), so a permanently held lock still terminates - after which a lock failure is treated like any other and consumes attempts as normal. - Tee each attempt's output so a lock collision can be told apart from a real failure. The install still streams live; the copy is only read to classify. Worst case moves from ~32 to ~35 min, still well inside the 90 min job ceiling, because a lock collision fails in seconds - only a genuine stall spends 900s. * AG-18231 Stop piping the install attempt, which deadlocked and defeated its own bound Revert of the detection half of fd3288bc0b9, which broke the bounded path outright. `timeout` signals only its direct child, `npx`. The `apt-get` Playwright starts under sudo is a grandchild: it survives the kill, and it also inherited stdout. Piping the attempt through `tee` to classify the failure afterwards therefore meant the pipe never reached EOF - the surviving apt still held the write end - so the pipeline blocked forever and the 900s bound never took effect. Run 32272425164 hung 90 minutes to the job cap with `tee` still resident in the orphan list, which is the exact failure this script exists to prevent, reintroduced by the script itself. It applied to successful installs too, not just failures, so the bounded path was broken for every caller rather than only on a bad mirror. The attempt now writes straight to the step log again, with nothing downstream of it that can outlive the kill. The lock collision it was trying to detect is prevented instead: `wait_for_apt` moves to before every attempt, so a lingering apt from a previous attempt - or a foreign one such as unattended-upgrades - is waited out rather than collided with. That addresses the original cause more directly than excusing the attempt after the fact, so the lock-retry accounting goes away with it. Guarded by a regression test that reproduces the shape rather than the symptom: a stubbed command that leaves a grandchild holding stdout, a stubbed `timeout` that signals only its direct child, and a hard watchdog so a hang fails the test instead of hanging the suite. It goes red on fd3288bc0b9 and green here. The previous retry tests all passed against the broken script because their `timeout` stub exec'd the command directly and so never modelled a survivor. --- .../test-framework-examples/action.yml | 2 +- .../install-playwright.sh | 74 ------ .github/workflows/ci.yml | 65 +++-- .github/workflows/doc-tests.yml | 2 +- documentation/ag-grid-docs/project.json | 10 +- packages/ag-grid-community/project.json | 5 +- packages/ag-grid-enterprise/project.json | 5 +- packages/ag-stack/project.json | 5 +- scripts/ci/install-playwright.sh | 242 ++++++++++++++++++ testing/accessibility/project.json | 29 +-- testing/csp/project.json | 5 +- testing/public-recipes/e2e/project.json | 5 +- testing/vue3-tests/project.json | 5 +- 13 files changed, 308 insertions(+), 146 deletions(-) delete mode 100755 .github/actions/test-framework-examples/install-playwright.sh create mode 100755 scripts/ci/install-playwright.sh diff --git a/.github/actions/test-framework-examples/action.yml b/.github/actions/test-framework-examples/action.yml index e350fa369a6..fb91041f0ca 100644 --- a/.github/actions/test-framework-examples/action.yml +++ b/.github/actions/test-framework-examples/action.yml @@ -72,7 +72,7 @@ runs: # the 6-hour limit cancelled it. ${GITHUB_WORKSPACE} is needed because this step's # working-directory is documentation/ag-grid-docs. run: | - PW_INSTALL="${GITHUB_WORKSPACE}/.github/actions/test-framework-examples/install-playwright.sh" + PW_INSTALL="${GITHUB_WORKSPACE}/scripts/ci/install-playwright.sh" if [ "${{ steps.pw-cache.outputs.cache-hit }}" = "true" ]; then bash "${PW_INSTALL}" deps ${PW_BROWSERS} else diff --git a/.github/actions/test-framework-examples/install-playwright.sh b/.github/actions/test-framework-examples/install-playwright.sh deleted file mode 100755 index 77654c8cf44..00000000000 --- a/.github/actions/test-framework-examples/install-playwright.sh +++ /dev/null @@ -1,74 +0,0 @@ -#!/usr/bin/env bash -# Bounded, retrying Playwright install. `install-deps` / `install --with-deps` shell out to -# `apt-get update`, which has no timeout: when the mirror stalls (AG-18231) the step hangs until -# the 6-hour job limit cancels the job and no test ever runs. -# -# Usage: install-playwright.sh ... -# deps - `playwright install-deps` (cache hit: OS libraries only; apt) -# full - `playwright install --with-deps` (cache miss: browser download + apt) -# browsers - `playwright install` (browser download only; no apt) -# -# The step must FAIL rather than be cancelled: a job cancelled by `timeout-minutes` makes -# `cancelled()` true and skips the report upload. So the budgets below are sized to give up after -# ~32 min, well inside the 90 min ceiling every doc-tests job now carries. Healthy installs take -# 1-3 min; the bounds are generous because `timeout` cannot tell a stalled mirror from a slow one. -set -uo pipefail - -MODE="${1:-}" -shift || true -BROWSERS=("$@") - -case "$MODE" in - deps) - CMD=(npx playwright install-deps "${BROWSERS[@]}") - DEFAULT_TIMEOUT=600 - DEFAULT_ATTEMPTS=3 - LABEL="Playwright OS dependency install (playwright install-deps)" - ;; - full) - CMD=(npx playwright install --with-deps "${BROWSERS[@]}") - DEFAULT_TIMEOUT=900 - DEFAULT_ATTEMPTS=2 - LABEL="Playwright browser + OS dependency install (playwright install --with-deps)" - ;; - browsers) - CMD=(npx playwright install "${BROWSERS[@]}") - DEFAULT_TIMEOUT=900 - DEFAULT_ATTEMPTS=2 - LABEL="Playwright browser download (playwright install)" - ;; - *) - echo "::error::install-playwright.sh: unknown mode '${MODE}' (expected deps|full|browsers)" - exit 2 - ;; -esac - -ATTEMPT_TIMEOUT_SECONDS="${PW_INSTALL_TIMEOUT_SECONDS:-${DEFAULT_TIMEOUT}}" -MAX_ATTEMPTS="${PW_INSTALL_MAX_ATTEMPTS:-${DEFAULT_ATTEMPTS}}" -RETRY_DELAY_SECONDS="${PW_INSTALL_RETRY_DELAY_SECONDS:-20}" - -for attempt in $(seq 1 "${MAX_ATTEMPTS}"); do - echo "::group::${LABEL} - attempt ${attempt}/${MAX_ATTEMPTS} (bounded to ${ATTEMPT_TIMEOUT_SECONDS}s)" - timeout --signal=TERM --kill-after=30s "${ATTEMPT_TIMEOUT_SECONDS}s" "${CMD[@]}" - status=$? - echo "::endgroup::" - - if [ "${status}" -eq 0 ]; then - exit 0 - fi - - if [ "${status}" -eq 124 ] || [ "${status}" -eq 137 ]; then - reason="did not complete within its ${ATTEMPT_TIMEOUT_SECONDS}s bound and was killed (stalled or very slow package mirror / CDN fetch)" - else - reason="failed with exit code ${status}" - fi - echo "::warning::${LABEL} attempt ${attempt}/${MAX_ATTEMPTS} ${reason}." - - if [ "${attempt}" -lt "${MAX_ATTEMPTS}" ]; then - # Give a TERM-ed apt time to release the dpkg/apt lock before retrying. - sleep "${RETRY_DELAY_SECONDS}" - fi -done - -echo "::error::${LABEL} did not complete after ${MAX_ATTEMPTS} attempts, each bounded to ${ATTEMPT_TIMEOUT_SECONDS}s. This is an infrastructure failure in the dependency install step - no example tests were run, so it is NOT a test failure." -exit 1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d7b98c9c13..27f88d638f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,7 @@ permissions: jobs: detect-changes: runs-on: ubuntu-24.04 + timeout-minutes: 10 name: Detect Changes outputs: run_code_ci: ${{ steps.decide.outputs.run_code_ci }} @@ -156,6 +157,7 @@ jobs: e2e_test_count: ${{ steps.matrix.outputs.e2e_test_count }} e2e_test_matrix: ${{ steps.matrix.outputs.e2e_test_matrix }} runs-on: ubuntu-latest + timeout-minutes: 30 steps: - name: Checkout id: checkout @@ -226,6 +228,7 @@ jobs: test: runs-on: ubuntu-24.04 + timeout-minutes: 45 name: Unit Tests (${{ matrix.shard }}/${{ strategy.job-total }}) needs: [ detect-changes, init ] if: needs.detect-changes.outputs.run_code_ci == 'true' && @@ -312,6 +315,7 @@ jobs: e2e: runs-on: ubuntu-latest + timeout-minutes: 90 name: e2e Tests needs: [ detect-changes, init ] if: needs.detect-changes.outputs.run_code_ci == 'true' && @@ -326,37 +330,35 @@ jobs: # than a legitimately-unavailable environment. Local runs without Apache still skip. HTTPD_REQUIRED: 1 steps: - - name: Ignore e2e tests - run: echo "::warning e2e tests are currently disabled due to flakiness. Re-enable when fixed." - - # - name: Checkout - # id: checkout - # uses: actions/checkout@v4 - # with: - # fetch-depth: 1 # shallow copy - - # - name: Setup - # id: setup - # uses: ./.github/actions/setup-nx - # with: - # yarn_postinstall: no-install - # cache_mode: ro - - # - name: nx test:e2e - # id: tests - # run: yarn nx ${{ github.event.inputs.nx_command || 'affected' }} -t test:e2e -c - # staging --exclude 'tag:module-size' --exclude all - - # - name: Persist test results - # if: always() && matrix.shard != 0 - # uses: actions/upload-artifact@v4 - # with: - # name: test-results-e2e-shard-${{matrix.shard}} - # path: | - # reports/ + - name: Checkout + id: checkout + uses: actions/checkout@v4 + with: + fetch-depth: 1 # shallow copy + + - name: Setup + id: setup + uses: ./.github/actions/setup-nx + with: + yarn_postinstall: no-install + cache_mode: ro + + - name: nx test:e2e + id: tests + run: yarn nx ${{ github.event.inputs.nx_command || 'affected' }} -t test:e2e -c + staging --exclude 'tag:module-size' --exclude all + + - name: Persist test results + if: always() && matrix.shard != 0 + uses: actions/upload-artifact@v4 + with: + name: test-results-e2e-shard-${{matrix.shard}} + path: | + reports/ lint: runs-on: ubuntu-latest + timeout-minutes: 30 name: Lint & Format Check needs: [ detect-changes, init ] if: ${{ always() && needs.detect-changes.result == 'success' }} @@ -394,6 +396,7 @@ jobs: build: runs-on: ubuntu-latest + timeout-minutes: 30 name: Build needs: [ detect-changes, init ] if: needs.detect-changes.outputs.run_code_ci == 'true' && @@ -440,6 +443,7 @@ jobs: pr_preview: runs-on: ubuntu-latest + timeout-minutes: 20 name: PR Preview (UMD) needs: [ build ] # Same-repo PRs only (fork PRs can't hold the write token) — see pr-review.yml. @@ -529,6 +533,7 @@ jobs: docs: runs-on: ubuntu-latest + timeout-minutes: 45 name: Docs Build & Link Checker needs: [ detect-changes, init ] if: ${{ (needs.detect-changes.outputs.run_code_ci == 'true' || @@ -593,6 +598,7 @@ jobs: fw_pkg_test: runs-on: ubuntu-24.04 + timeout-minutes: 90 name: Framework Package Tests (${{ matrix.framework }}) permissions: contents: write @@ -637,6 +643,7 @@ jobs: report: runs-on: ubuntu-24.04 + timeout-minutes: 20 needs: [ detect-changes, @@ -821,6 +828,7 @@ jobs: sonar_community: name: SonarQube Community runs-on: ubuntu-latest + timeout-minutes: 30 if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository steps: - uses: actions/checkout@v4 @@ -845,6 +853,7 @@ jobs: sonar_enterprise: name: SonarQube Enterprise runs-on: ubuntu-latest + timeout-minutes: 30 if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/doc-tests.yml b/.github/workflows/doc-tests.yml index 0c7eeea887f..43491885b22 100644 --- a/.github/workflows/doc-tests.yml +++ b/.github/workflows/doc-tests.yml @@ -124,7 +124,7 @@ jobs: if: steps.pw-cache.outputs.cache-hit != 'true' working-directory: documentation/ag-grid-docs # Bounded + retried via the shared wrapper; no --with-deps here, so no apt. - run: bash "${GITHUB_WORKSPACE}/.github/actions/test-framework-examples/install-playwright.sh" browsers chromium firefox webkit + run: bash "${GITHUB_WORKSPACE}/scripts/ci/install-playwright.sh" browsers chromium firefox webkit test-vanilla: needs: initialise diff --git a/documentation/ag-grid-docs/project.json b/documentation/ag-grid-docs/project.json index d3c86db62c8..6580d0a346e 100644 --- a/documentation/ag-grid-docs/project.json +++ b/documentation/ag-grid-docs/project.json @@ -200,18 +200,16 @@ "test:interactive:setup:chromium": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "documentation/ag-grid-docs" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false }, "cache": true }, "test:interactive:setup:all": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium firefox webkit"], - "parallel": false, - "cwd": "documentation/ag-grid-docs" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium firefox webkit"], + "parallel": false }, "cache": true }, diff --git a/packages/ag-grid-community/project.json b/packages/ag-grid-community/project.json index 4780191540f..fc2d9f944ec 100644 --- a/packages/ag-grid-community/project.json +++ b/packages/ag-grid-community/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "packages/ag-grid-community" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "build": { diff --git a/packages/ag-grid-enterprise/project.json b/packages/ag-grid-enterprise/project.json index 138fe198296..a6dedc06494 100644 --- a/packages/ag-grid-enterprise/project.json +++ b/packages/ag-grid-enterprise/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "packages/ag-grid-enterprise" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "build": { diff --git a/packages/ag-stack/project.json b/packages/ag-stack/project.json index e72a27eebcf..1212255ec57 100644 --- a/packages/ag-stack/project.json +++ b/packages/ag-stack/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "packages/ag-stack" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "build": { diff --git a/scripts/ci/install-playwright.sh b/scripts/ci/install-playwright.sh new file mode 100755 index 00000000000..8931a0ef3a2 --- /dev/null +++ b/scripts/ci/install-playwright.sh @@ -0,0 +1,242 @@ +#!/usr/bin/env bash +# Bounded, retrying Playwright install. `install-deps` / `install --with-deps` shell out to +# `apt-get update`, which has no timeout: when the mirror stalls (AG-18231) the step hangs until +# the job's time limit cancels it and no test ever runs. +# +# Usage: install-playwright.sh ... +# deps - `playwright install-deps` (cache hit: OS libraries only; apt) +# full - `playwright install --with-deps` (cache miss: browser download + apt) +# browsers - `playwright install` (browser download only; no apt) +# +# In CI the step must FAIL rather than be cancelled: a job cancelled by `timeout-minutes` makes +# `cancelled()` true and skips the report upload. So the budgets below are sized to give up after +# ~35 min (2 bounded attempts, plus the retry and apt-lock waits between them), well inside the +# 90 min ceiling the workflow jobs carry. Healthy installs take 1-3 min; the bounds are generous +# because `timeout` cannot tell a stalled mirror from a slow one. +# +# This is also the installer behind the Nx `setup` targets, so it runs on developer machines as +# well as in CI. It keeps its output plain locally and only emits GitHub workflow commands under +# Actions. +set -uo pipefail + +MODE="${1:-}" +shift || true +BROWSERS=("$@") + +if [ -n "${GITHUB_ACTIONS:-}" ]; then + group_start() { echo "::group::$1"; } + group_end() { echo "::endgroup::"; } + warn() { echo "::warning::$1"; } + fail() { echo "::error::$1"; } +else + group_start() { echo "$1"; } + group_end() { :; } + warn() { echo "WARNING: $1" >&2; } + fail() { echo "ERROR: $1" >&2; } +fi + +# The OS libraries are machine-global, but several Nx `setup` targets each call this script, so a +# single `nx test:e2e` run repeats the apt work once per project. Two outcomes are recorded and +# reused, because both multiply: +# +# installed - skip the apt work the rest of the run, which is what makes it cheap. +# stalled - fail the rest of the run immediately. This is the one that matters. Nx does not bail +# on a failed task, so without it every remaining project pays the full ~30 min budget +# against a mirror already known to be stalled; enough of them in series overshoot the +# job's `timeout-minutes` and the job is CANCELLED rather than failed, losing the +# report upload these bounds exist to preserve. One project pays, the rest report the +# same infrastructure error at once. +# +# Neither is a lock, and the sequence is deliberately safe without one. Concurrent callers may all +# find no record and run apt at once, but that is what they do today with no records at all, so it +# is no worse; apt's own locking serialises them and the retries below absorb it. What must not +# happen is a caller that lost that race condemning the runner for everyone else, which is why only +# a stall - never a plain failure - is recorded. +# +# CI only: GitHub runners are ephemeral, so this state cannot outlive the machine it describes. A +# developer machine is long-lived, where it could skip libraries a later OS change actually needs, +# or keep failing an install that a mirror recovery has since fixed. Being single-runner-scoped is +# also why nothing here is keyed by Playwright version: one runner is one checkout, so the version +# cannot change underneath the state. The browser set can and does vary between callers. +DEPS_INSTALLED_FILE="" +DEPS_STALLED_MARKER="" +if [ -n "${GITHUB_ACTIONS:-}" ]; then + _state_dir="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" + DEPS_INSTALLED_FILE="${_state_dir}/.ag-playwright-deps-installed" + DEPS_STALLED_MARKER="${_state_dir}/.ag-playwright-deps-stalled" +fi + +# Only the apt-touching modes say anything about the runner's OS libraries. `browsers` is a CDN +# download, including when `full` is downgraded to it below, so its outcome is never recorded. +touches_apt() { + case "$MODE" in + deps | full) return 0 ;; + *) return 1 ;; + esac +} + +# Recorded per browser, because the OS libraries are not the same for each: Firefox and WebKit pull +# their own, so a chromium-only success must not be read as covering a later chromium+firefox+webkit +# request. Append-only, one browser per line - concurrent callers can add to it without a lock. +record_deps_installed() { + touches_apt || return 0 + [ -n "${DEPS_INSTALLED_FILE}" ] || return 0 + printf '%s\n' "${BROWSERS[@]}" >> "${DEPS_INSTALLED_FILE}" 2>/dev/null || true +} + +# Satisfied only when every browser asked for has already had its dependencies installed. +deps_already_installed() { + [ -n "${DEPS_INSTALLED_FILE}" ] && [ -s "${DEPS_INSTALLED_FILE}" ] || return 1 + local browser + for browser in "${BROWSERS[@]}"; do + grep -qxF "${browser}" "${DEPS_INSTALLED_FILE}" 2>/dev/null || return 1 + done + return 0 +} + +# Recorded only for a stall - an attempt killed at its bound - never for an install that merely +# failed. A fast failure has not cost the budget this short-circuit exists to avoid spending twice, +# and it may well be transient: a caller that lost a race for the dpkg lock fails in seconds, and +# must not condemn a runner whose dependencies another caller is busy installing correctly. +record_deps_stalled() { + touches_apt || return 0 + [ -n "${DEPS_STALLED_MARKER}" ] || return 0 + touch "${DEPS_STALLED_MARKER}" 2>/dev/null || true +} + +if touches_apt && [ -n "${DEPS_STALLED_MARKER}" ] && [ -e "${DEPS_STALLED_MARKER}" ]; then + fail "The Playwright OS dependency install already stalled on this runner, so this attempt is skipped rather than repeating a ~30 min stall. See the first failure above for the cause. This is an infrastructure failure in the dependency install step - no tests were run, so it is NOT a test failure." + exit 1 +fi + +if deps_already_installed; then + case "$MODE" in + deps) + echo "OS dependencies for [${BROWSERS[*]}] already installed on this runner - skipping apt." + exit 0 + ;; + full) + echo "OS dependencies for [${BROWSERS[*]}] already installed on this runner - downloading browsers only." + MODE=browsers + ;; + esac +fi + +case "$MODE" in + deps) + CMD=(npx playwright install-deps "${BROWSERS[@]}") + DEFAULT_TIMEOUT=600 + DEFAULT_ATTEMPTS=3 + LABEL="Playwright OS dependency install (playwright install-deps)" + ;; + full) + CMD=(npx playwright install --with-deps "${BROWSERS[@]}") + DEFAULT_TIMEOUT=900 + DEFAULT_ATTEMPTS=2 + LABEL="Playwright browser + OS dependency install (playwright install --with-deps)" + ;; + browsers) + CMD=(npx playwright install "${BROWSERS[@]}") + DEFAULT_TIMEOUT=900 + DEFAULT_ATTEMPTS=2 + LABEL="Playwright browser download (playwright install)" + ;; + *) + fail "install-playwright.sh: unknown mode '${MODE}' (expected deps|full|browsers)" + exit 2 + ;; +esac + +ATTEMPT_TIMEOUT_SECONDS="${PW_INSTALL_TIMEOUT_SECONDS:-${DEFAULT_TIMEOUT}}" +MAX_ATTEMPTS="${PW_INSTALL_MAX_ATTEMPTS:-${DEFAULT_ATTEMPTS}}" +RETRY_DELAY_SECONDS="${PW_INSTALL_RETRY_DELAY_SECONDS:-20}" +APT_WAIT_SECONDS="${PW_INSTALL_APT_WAIT_SECONDS:-120}" + +# `timeout` signals only its direct child, `npx`. The `apt-get` Playwright starts under sudo is a +# grandchild, so it survives the kill and keeps /var/lib/apt/lists/lock. A retry on a fixed delay +# therefore raced a still-dying apt and died on "Could not get lock" in about a second - burning an +# attempt without ever reaching the mirror (run 32269132733, where the 20s delay was not enough and +# process 2808 still held the lock). So wait for apt to actually go before each attempt, rather than +# guess how long it needs. +# +# Prevention, not detection, and specifically NOT by capturing the attempt's output to classify the +# failure afterwards: that surviving grandchild also inherits stdout, so piping the attempt through +# `tee` means the pipe never reaches EOF when `timeout` kills its child. The pipeline then blocks +# forever and defeats the bound entirely - run 32272425164 hung 90 min to the job cap with `tee` +# still resident. The attempt must keep writing straight to the step log, with nothing downstream +# of it that can outlive the kill. +apt_is_busy() { + [ -e /var/lib/apt/lists/lock ] || return 1 + pgrep -x apt-get >/dev/null 2>&1 || pgrep -x dpkg >/dev/null 2>&1 +} + +wait_for_apt() { + apt_is_busy || return 0 + echo "Waiting up to ${APT_WAIT_SECONDS}s for a lingering apt/dpkg to release its lock..." + waited=0 + while apt_is_busy && [ "${waited}" -lt "${APT_WAIT_SECONDS}" ]; do + sleep 5 + waited=$((waited + 5)) + done + if apt_is_busy; then + warn "apt/dpkg still held its lock after ${waited}s. Retrying anyway." + else + echo "apt released its lock after ${waited}s." + fi +} + +# GNU coreutils `timeout`, absent from a stock macOS (where coreutils installs it as `gtimeout`). +# Without it the install still runs, just unbounded: the stall this guards against is an +# apt/Linux failure mode, so an unbounded run on a developer machine is the pre-existing +# behaviour rather than a regression. +TIMEOUT_BIN="" +for candidate in timeout gtimeout; do + if command -v "${candidate}" >/dev/null 2>&1; then + TIMEOUT_BIN="${candidate}" + break + fi +done + +if [ -z "${TIMEOUT_BIN}" ]; then + warn "No \`timeout\` binary found, so the ${ATTEMPT_TIMEOUT_SECONDS}s bound cannot be applied. Running ${LABEL} unbounded - install coreutils to re-enable the bound." + group_start "${LABEL} - unbounded" + "${CMD[@]}" + status=$? + group_end + # Nothing was bounded, so a failure here is not a stall and is not recorded as one. + [ "${status}" -eq 0 ] && record_deps_installed + exit "${status}" +fi + +stalled=0 +for attempt in $(seq 1 "${MAX_ATTEMPTS}"); do + # Before, not after: the apt a previous attempt started can outlive the kill, and a foreign one + # (unattended-upgrades) can hold the lock at any point. Either way, going in while it is held + # wastes the attempt on a collision instead of learning anything about the mirror. + wait_for_apt + group_start "${LABEL} - attempt ${attempt}/${MAX_ATTEMPTS} (bounded to ${ATTEMPT_TIMEOUT_SECONDS}s)" + "${TIMEOUT_BIN}" --signal=TERM --kill-after=30s "${ATTEMPT_TIMEOUT_SECONDS}s" "${CMD[@]}" + status=$? + group_end + + if [ "${status}" -eq 0 ]; then + record_deps_installed + exit 0 + fi + + if [ "${status}" -eq 124 ] || [ "${status}" -eq 137 ]; then + stalled=1 + reason="did not complete within its ${ATTEMPT_TIMEOUT_SECONDS}s bound and was killed (stalled or very slow package mirror / CDN fetch)" + else + reason="failed with exit code ${status}" + fi + warn "${LABEL} attempt ${attempt}/${MAX_ATTEMPTS} ${reason}." + + if [ "${attempt}" -lt "${MAX_ATTEMPTS}" ]; then + sleep "${RETRY_DELAY_SECONDS}" + fi +done + +[ "${stalled}" -eq 1 ] && record_deps_stalled +fail "${LABEL} did not complete after ${MAX_ATTEMPTS} attempts, each bounded to ${ATTEMPT_TIMEOUT_SECONDS}s. This is an infrastructure failure in the dependency install step - no tests were run, so it is NOT a test failure." +exit 1 diff --git a/testing/accessibility/project.json b/testing/accessibility/project.json index d82119623ae..dd5c3690426 100644 --- a/testing/accessibility/project.json +++ b/testing/accessibility/project.json @@ -4,35 +4,28 @@ "sourceRoot": "testing/accessibility", "projectType": "application", "targets": { + "setup:playwright": { + "executor": "nx:run-commands", + "options": { + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false + } + }, "setup": { "executor": "nx:run-commands", + "dependsOn": ["setup:playwright"], "options": { - "commands": [ - "npx playwright install --with-deps chromium", - "npm run download-examples", - "mkdir -p e2e", - "npm run generate-tests" - ], + "commands": ["npm run download-examples", "mkdir -p e2e", "npm run generate-tests"], "parallel": false, "cwd": "testing/accessibility" }, "configurations": { "production": { - "commands": [ - "npx playwright install --with-deps chromium", - "npm run download-examples-prod", - "mkdir -p e2e", - "npm run generate-tests" - ] + "commands": ["npm run download-examples-prod", "mkdir -p e2e", "npm run generate-tests"] }, "staging": {}, "dev": { - "commands": [ - "npx playwright install --with-deps chromium", - "npm run download-examples-local", - "mkdir -p e2e", - "npm run generate-tests" - ] + "commands": ["npm run download-examples-local", "mkdir -p e2e", "npm run generate-tests"] } } }, diff --git a/testing/csp/project.json b/testing/csp/project.json index 55b0f673cab..27634c6682b 100644 --- a/testing/csp/project.json +++ b/testing/csp/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "testing/csp" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "test:e2e:csp": { diff --git a/testing/public-recipes/e2e/project.json b/testing/public-recipes/e2e/project.json index 5312ee307a7..8fdae2853ec 100644 --- a/testing/public-recipes/e2e/project.json +++ b/testing/public-recipes/e2e/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "testing/public-recipes" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "test:recipes": { diff --git a/testing/vue3-tests/project.json b/testing/vue3-tests/project.json index b161668e5fa..f64a97af0bc 100644 --- a/testing/vue3-tests/project.json +++ b/testing/vue3-tests/project.json @@ -7,9 +7,8 @@ "setup": { "executor": "nx:run-commands", "options": { - "commands": ["npx playwright install --with-deps chromium"], - "parallel": false, - "cwd": "testing/vue3-tests" + "commands": ["{workspaceRoot}/scripts/ci/install-playwright.sh full chromium"], + "parallel": false } }, "dev": { From 90b0e8e8529e32dcb6a3829c1b8b83a3bb01af8f Mon Sep 17 00:00:00 2001 From: Salvatore Previti Date: Thu, 20 Aug 2026 11:55:41 +0100 Subject: [PATCH 5/8] AG-18241 number-formatter-filter-inputs (#14909) * AG-18241-number-formatter-filter-inputs * AG-18241-number-formatter-filter-inputs * AG-18241-number-formatter-filter-inputs --- .rulesync/rules/testing.md | 16 +- .../content/docs/filter-advanced/index.mdoc | 2 +- .../src/content/docs/filter-bigint/index.mdoc | 4 +- .../src/content/docs/filter-number/index.mdoc | 10 +- .../src/agWidgets/agInputTextField.ts | 9 +- .../floatingFilterTextInputService.ts | 4 + .../provided/iFloatingFilterInputService.ts | 2 + .../provided/textInputFloatingFilter.ts | 9 +- .../filter/provided/bigInt/bigIntFilter.ts | 224 ++----- .../bigInt/bigIntFilterModelFormatter.ts | 5 +- .../provided/bigInt/bigIntFilterUtils.ts | 11 + .../provided/bigInt/bigIntFloatingFilter.ts | 13 +- .../provided/date/dateFilterModelFormatter.ts | 18 +- .../filter/provided/number/iNumberFilter.ts | 18 +- .../filter/provided/number/numberFilter.ts | 291 ++------- .../number/numberFilterModelFormatter.ts | 5 +- .../provided/number/numberFilterUtils.test.ts | 57 ++ .../provided/number/numberFilterUtils.ts | 29 + .../provided/number/numberFloatingFilter.ts | 40 +- .../src/filter/provided/simpleFilter.ts | 13 +- .../provided/simpleFilterModelFormatter.ts | 10 +- .../src/filter/provided/simpleFilterUtils.ts | 13 + .../filter/provided/textInputSimpleFilter.ts | 337 ++++++++++ .../advancedFilterExpressionService.ts | 131 ++-- .../builder/conditionPillWrapperComp.ts | 24 +- .../advancedFilter/builder/inputPillComp.ts | 17 +- .../colFilterExpressionParser.ts | 15 +- .../advancedFilter/filterExpressionUtils.ts | 37 +- scripts/gate/args.mjs | 16 + scripts/gate/main.mjs | 20 +- scripts/gate/run-log.mjs | 112 +++- .../filters/advancedFilterBuilderHarness.ts | 6 + .../af-parser.test.ts | 44 +- ...vanced-filter-bigint-custom-parser.test.ts | 23 +- ...vanced-filter-number-custom-parser.test.ts | 589 ++++++++++++++++++ .../bigint-filter-custom-parser.test.ts | 179 +++++- .../number-filter-conditions.test.ts | 563 ++++++++++++++++- .../src/filters/floating-filters.test.ts | 83 ++- .../number-filter-range-validation.test.ts | 30 + 39 files changed, 2403 insertions(+), 626 deletions(-) create mode 100644 packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.test.ts create mode 100644 packages/ag-grid-community/src/filter/provided/textInputSimpleFilter.ts create mode 100644 testing/behavioural/src/filters/advanced-filter/advanced-filter-number-custom-parser.test.ts diff --git a/.rulesync/rules/testing.md b/.rulesync/rules/testing.md index 05d808b794c..bb50d5e76b7 100644 --- a/.rulesync/rules/testing.md +++ b/.rulesync/rules/testing.md @@ -57,14 +57,26 @@ Pick the input that *separates* the two behaviours. A test that passes against b **Never `sleep` to wait for a run.** One you backgrounded wakes the agent by itself; one started elsewhere has `--async-status` (exit 3 = still running) and `--wait`, below. For progress mid-run, grep the log — it is written live. -**Every local run captures itself, and prints the log path as its first line** — `▶ tmp/_behave-output//output.log`, the whole of stdout and stderr with the colour codes stripped. That line is also the only proof a run happened: piping a gate (`… | tail`) reports the **pipe's** exit status, so one that failed, or that the shell never found, still comes back `0` — read the summary line, and treat a missing `▶` as "nothing ran". So no redirect has to be arranged in advance and a red run needs no second run: grep that file, during the run or after it, or pass `--bail 1` to make the run stop at the first failure itself. Beside it sit the `command` and a `status`, plus `result.json` (vitest's machine-readable results) for `./behave.sh` only. `latest` symlinks the newest and week-old runs are pruned. +**Never pipe a gate — not into `tail`, `head`, `grep` or anything else.** A pipeline exits with its **last** command's status, so `./behave.sh … | tail -40` reports `tail`'s `0` and a red run arrives labelled green; backgrounded, that bogus `0` is what the completion event carries, so the failure is never surfaced at all. Ask the script for less instead — these filter the console and still exit with the run's own status: + +```bash +./behave.sh --quiet # its own summary plus every failure — the usual choice +./behave.sh --log-tail 30 # the end of the log +./behave.sh --log-grep PROBE # an ad-hoc pattern +./behave.sh --log-grep PROBE --log-tail 5 # combined: the last 5 matches +``` + +**`--quiet` is the one to reach for.** Each gate declares its own `failRe`/`summaryRe`, which is what `--quiet` prints, so hand-writing `--log-grep '×|FAIL|Tests '` is a worse and vitest-only spelling of it. Keep `--log-grep` for what `--quiet` does not show: a probe's `console.log`, or vitest's `Errors N error` line, which reports a throw from outside a test body and **still exits 0**. + +**Every local run captures itself, and prints the log path as its first line** — `▶ tmp/_behave-output//output.log`, the whole of stdout and stderr with the colour codes stripped. That line is also the only proof a run happened, so treat a missing `▶` as "nothing ran". No redirect has to be arranged in advance and a red run needs no second run: grep that file, during the run or after it, or pass `--bail 1` to make the run stop at the first failure itself. Beside it sit the `command` and a `status`, plus `result.json` (vitest's machine-readable results) for `./behave.sh` only. `latest` symlinks the newest and week-old runs are pruned. **Under `CI`, run them in the foreground instead.** Backgrounding is there to keep an interactive session reachable, and a workflow has nobody to block, so take the output directly. The scripts capture nothing under `CI` for the same reason, so there is no log to grep and none is needed. - `--async-status [id]` — has a run finished? Exit 0 passed, 1 failed, **3 still running**. Defaults to the newest run and takes an id or any path containing one, so it also reports on a run started elsewhere. - `--wait [secs]` — the same report, waiting up to `secs` for the run to finish. - `--kill [id]` — stop a run (the newest by default) and every process it spawned. -- `--quiet` — console gets the paths, summary and failures only (not `./checks.sh`, which is quiet already); `--no-log` turns capture off. +- `--quiet` — console gets the paths, summary and failures only (not `./checks.sh`, which is quiet already); it hides a test's own `console.log`, so read the log when probing. `--no-log` turns capture off. +- `--log-tail ` / `--log-grep ` — print only part of the log when the run ends, instead of piping. Both imply `--quiet`, combine with each other, and leave the exit status the run's. `--log-grep` takes a regex, falling back to a substring if it will not compile. **Run with `--bail 1` by habit.** `./behave.sh --bail 1 ` stops at the first failing test — what you want in a fix-one-error-at-a-time loop, and it skips the rest of the reporting too. `--no-diff` reports names and messages with no diffs, for when a suite fails wholesale. A red run can take minutes where the green one takes seconds: vitest's diff serialisation of grid objects is effectively unbounded. The skill explains why, plus the `--stack-trace-len` trap and how `--bail` reads in a JSON report. diff --git a/documentation/ag-grid-docs/src/content/docs/filter-advanced/index.mdoc b/documentation/ag-grid-docs/src/content/docs/filter-advanced/index.mdoc index 9bbae8430d5..54c34a5db0a 100644 --- a/documentation/ag-grid-docs/src/content/docs/filter-advanced/index.mdoc +++ b/documentation/ag-grid-docs/src/content/docs/filter-advanced/index.mdoc @@ -275,7 +275,7 @@ The following example demonstrates configuring the Advanced Filter Builder: All of the [Cell Data Types](./cell-data-types) are supported in the Advanced Filter. The behaviour of each is described below. - **Text** - The value in the input is compared against the cell value before any [Value Formatters](./value-formatters/) are applied (similar to the [Text Filter](./filter-text/)). To change the value being compared against, a [Filter Value Getter](./filter-text/#text-filter-values) can be used. -- **Number** - The value in the input is compared against the cell value (like in the [Number Filter](./filter-number/)). +- **Number** - The value in the input is compared against the cell value (like in the [Number Filter](./filter-number/)). A column pairing a [`numberParser`](./filter-number/#custom-number-support) with a `numberFormatter` has its operands read and displayed in its own format, so custom formats such as thousands separators are accepted. Either one on its own leaves the operand as a plain number: the grid only reads a format it can also write. A format containing a space is quoted in the expression, so `[Value] = "1 234 567"` is read as one operand. A format the parser does not read back as the same number is shown as a plain number instead. - **BigInt** - The value in the input is parsed as a `bigint` (decimal integer syntax only, optional trailing `n`) and compared against the cell value (like in the [BigInt Filter](./filter-bigint/)). A column's [`bigintParser`](./filter-bigint/#custom-parsing) is used here too, so custom formats such as hexadecimal are also accepted, and its `bigintFormatter` is used to display a stored operand in the filter expression and the Filter Builder. - **Boolean** - No values are displayed for booleans as the filter option is used instead. - **Date** and **Date Time** - The value in the input is converted to a `Date` via the [Value Parser](./value-parsers/#value-parser). diff --git a/documentation/ag-grid-docs/src/content/docs/filter-bigint/index.mdoc b/documentation/ag-grid-docs/src/content/docs/filter-bigint/index.mdoc index a5bb780c95e..ec2e0131983 100644 --- a/documentation/ag-grid-docs/src/content/docs/filter-bigint/index.mdoc +++ b/documentation/ag-grid-docs/src/content/docs/filter-bigint/index.mdoc @@ -46,7 +46,7 @@ The BigInt Filter accepts decimal integer syntax only: To accept other formats, such as hexadecimal, provide a `bigintParser` that converts the entered text to a `bigint` (return `null` for values it cannot parse). Pair it with `allowedCharPattern` so the extra characters can be typed into the filter input. The parsed value is what gets applied to filtering, and the same parser is used by the [Advanced Filter](./filter-advanced/) for `bigint` operands. -The filter model always stores the parsed value as a canonical decimal string, so provide a `bigintFormatter` — the inverse of the parser — to display stored values back in your own format. It is used by the [Floating Filter](./floating-filters/) and by the [Advanced Filter](./filter-advanced/) when displaying an operand, which means an entered value is echoed back in the formatter's format rather than exactly as typed. +The filter model always stores the parsed value as a canonical decimal string, so provide a `bigintFormatter` — the inverse of the parser — to display stored values back in your own format. It is used by the filter inputs, by the [Floating Filter](./floating-filters/) and by the [Advanced Filter](./filter-advanced/) when displaying an operand, which means an entered value is echoed back in the formatter's format rather than exactly as typed. ```{% frameworkTransform=true %} const gridOptions = { @@ -56,7 +56,7 @@ const gridOptions = { cellDataType: 'bigint', filter: 'agBigIntColumnFilter', filterParams: { - allowedCharPattern: '[\\dxXa-fA-F]', + allowedCharPattern: '\\dxXa-fA-F', bigintParser: (text) => { if (text == null || text.trim() === '') { return null; diff --git a/documentation/ag-grid-docs/src/content/docs/filter-number/index.mdoc b/documentation/ag-grid-docs/src/content/docs/filter-number/index.mdoc index c1ee52fd067..a8e6f1ff6fd 100644 --- a/documentation/ag-grid-docs/src/content/docs/filter-number/index.mdoc +++ b/documentation/ag-grid-docs/src/content/docs/filter-number/index.mdoc @@ -76,11 +76,19 @@ The `numberFormatter` should take a number (e.g. from the Filter Model) and conv An `allowedCharPattern` of `\\d\\-\\.` will give similar behaviour to the default `number` input. +A `text` input is used whenever either `allowedCharPattern` or `numberFormatter` is provided, as a `number` input keeps only its own number syntax and would discard formatted text. + +Set `filterInputType` to choose the input yourself. A `numberFormatter` writing text a `number` input can hold, such as `(value) => value.toFixed(2)`, can keep that input with `filterInputType: 'number'`. An `allowedCharPattern` applies to either input, narrowing what a `number` input already accepts. + +Provide a `numberParser` alongside a `numberFormatter` to have the format read back. Without one, typed text is read with `parseFloat`, so `1,234` becomes `1`. + +Pair both to have the [Advanced Filter](./filter-advanced/) read and display this column's operands in the same format. With only one of them, its operands stay plain numbers. + The following example demonstrates custom number support: - The first column shows the default Number Filter behaviour. - The second column demonstrates custom number support, and uses commas for decimals and allows a dollar sign ($) to be included. -- Floating filters are enabled and also react to the configuration of `allowedCharPattern`. +- Floating filters are enabled and also react to the configuration of `allowedCharPattern` and `numberFormatter`. {% gridExampleRunner title="Custom Number Support" name="custom-number-support" /%} diff --git a/packages/ag-grid-community/src/agWidgets/agInputTextField.ts b/packages/ag-grid-community/src/agWidgets/agInputTextField.ts index f8d2107ca2a..f20d93220cf 100644 --- a/packages/ag-grid-community/src/agWidgets/agInputTextField.ts +++ b/packages/ag-grid-community/src/agWidgets/agInputTextField.ts @@ -67,7 +67,7 @@ export class AgInputTextField< const { allowedCharPattern, clearButton, onValueClear, searchIcon } = this.config; if (allowedCharPattern) { - this.preventDisallowedCharacters(); + this.preventDisallowedCharacters(allowedCharPattern); } if (clearButton) { this.setClearButtonEnabled(true); @@ -188,8 +188,11 @@ export class AgInputTextField< _setDisplayed(eClearButton, canDisplay && !!eInput.value); } - private preventDisallowedCharacters(): void { - const pattern = new RegExp(`[${this.config.allowedCharPattern}]`); + private preventDisallowedCharacters(allowedCharPattern: string): void { + // Already a character class: wrapping it again would only ever match a two-character string, + // so every single keystroke would be rejected. + const isCharClass = allowedCharPattern.startsWith('[') && allowedCharPattern.endsWith(']'); + const pattern = new RegExp(isCharClass ? allowedCharPattern : `[${allowedCharPattern}]`); const preventCharacters = (event: KeyboardEvent) => { if (!_isEventFromPrintableCharacter(event)) { diff --git a/packages/ag-grid-community/src/filter/floating/provided/floatingFilterTextInputService.ts b/packages/ag-grid-community/src/filter/floating/provided/floatingFilterTextInputService.ts index e0829093f43..2b6e55d190c 100644 --- a/packages/ag-grid-community/src/filter/floating/provided/floatingFilterTextInputService.ts +++ b/packages/ag-grid-community/src/filter/floating/provided/floatingFilterTextInputService.ts @@ -48,6 +48,10 @@ export class FloatingFilterTextInputService extends BeanStub implements Floating return this.eInput.getValue(); } + public getInputText(): string { + return this.eInput.getInputElement().value; + } + public setValue(value: string | null | undefined, silent?: boolean): void { this.eInput.setValue(value, silent); } diff --git a/packages/ag-grid-community/src/filter/floating/provided/iFloatingFilterInputService.ts b/packages/ag-grid-community/src/filter/floating/provided/iFloatingFilterInputService.ts index f1f5cfef1fa..6b1ead13c8f 100644 --- a/packages/ag-grid-community/src/filter/floating/provided/iFloatingFilterInputService.ts +++ b/packages/ag-grid-community/src/filter/floating/provided/iFloatingFilterInputService.ts @@ -5,6 +5,8 @@ export interface FloatingFilterInputService extends Bean { setEditable(editable: boolean): void; isFocused(): boolean; getValue(): string | null | undefined; + /** The text as typed, which `getValue` drops once the input calls it invalid. */ + getInputText(): string; setValue(value: string | null | undefined, silent?: boolean): void; setValueChangedListener(listener: (e: KeyboardEvent) => void): void; setValueClearedListener(listener: () => void): void; diff --git a/packages/ag-grid-community/src/filter/floating/provided/textInputFloatingFilter.ts b/packages/ag-grid-community/src/filter/floating/provided/textInputFloatingFilter.ts index fe3d8840cb5..cd10ffe7d22 100644 --- a/packages/ag-grid-community/src/filter/floating/provided/textInputFloatingFilter.ts +++ b/packages/ag-grid-community/src/filter/floating/provided/textInputFloatingFilter.ts @@ -107,12 +107,13 @@ export abstract class TextInputFloatingFilter< } protected recreateFloatingFilterInputService(params: TParams): void { - const { inputSvc } = this; - const value = inputSvc.getValue(); + const previous = this.inputSvc; + // The text as typed, which the widget's own reader drops once the input calls it invalid. + const value = previous.getInputText(); _clearElement(this.eFloatingFilterInputContainer); - this.destroyBean(inputSvc); + this.destroyBean(previous); this.setupFloatingFilterInputService(params); - inputSvc.setValue(value, true); + this.inputSvc.setValue(value, true); } private syncUpWithParentFilter(e?: KeyboardEvent): void { diff --git a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilter.ts b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilter.ts index 690840e7591..231356412e8 100644 --- a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilter.ts @@ -1,61 +1,66 @@ -import { _isBrowserFirefox, _parseBigIntOrNull } from 'ag-stack'; - -import { AgInputTextField } from '../../../agWidgets/agInputTextField'; -import type { IAfterGuiAttachedParams } from '../../../interfaces/iAfterGuiAttachedParams'; import type { FilterDisplayParams } from '../../../interfaces/iFilter'; -import { _createElement } from '../../../utils/element'; import type { GridInputTextField } from '../../../widgets/gridWidgetTypes'; -import type { FilterLocaleTextKey } from '../../filterLocaleText'; -import type { ProvidedFilterParams } from '../iProvidedFilter'; -import type { ICombinedSimpleModel, Tuple } from '../iSimpleFilter'; -import { SimpleFilter } from '../simpleFilter'; +import type { ICombinedSimpleModel } from '../iSimpleFilter'; +import { getValidityMessageKey } from '../simpleFilterUtils'; +import type { RenderChange } from '../textInputSimpleFilter'; +import { TextInputSimpleFilter } from '../textInputSimpleFilter'; import { DEFAULT_BIGINT_FILTER_OPTIONS } from './bigIntFilterConstants'; -import { getAllowedCharPattern, mapValuesFromBigIntFilterModel } from './bigIntFilterUtils'; +import { getAllowedCharPattern, mapValuesFromBigIntFilterModel, stringToBigInt } from './bigIntFilterUtils'; import type { BigIntFilterModel, IBigIntFilterParams } from './iBigIntFilter'; /** temporary type until `BigIntFilterParams` is updated as breaking change */ type BigIntFilterDisplayParams = IBigIntFilterParams & FilterDisplayParams>; -export class BigIntFilter extends SimpleFilter< +export class BigIntFilter extends TextInputSimpleFilter< BigIntFilterModel, bigint, GridInputTextField, BigIntFilterDisplayParams > { - private readonly eValuesFrom: GridInputTextField[] = []; - private readonly eValuesTo: GridInputTextField[] = []; - public readonly filterType = 'bigint' as const; constructor() { super('bigintFilter', mapValuesFromBigIntFilterModel, DEFAULT_BIGINT_FILTER_OPTIONS); } - protected override defaultDebounceMs = 500; - - public override afterGuiAttached(params?: IAfterGuiAttachedParams | undefined): void { - super.afterGuiAttached(params); + protected override getRenderChange( + params: BigIntFilterDisplayParams, + previous: BigIntFilterDisplayParams | undefined + ): RenderChange | undefined { + // Read once when an input is built, so only replacing the element can change it. + if (getAllowedCharPattern(params) !== getAllowedCharPattern(previous)) { + return 'rebuild'; + } + // What an input shows is rendered through these, so its text stops being readable when they change. + if (params.bigintParser !== previous?.bigintParser || params.bigintFormatter !== previous?.bigintFormatter) { + return 'rerender'; + } + return undefined; + } - this.refreshInputValidation(); + protected override parseText( + text: string | null | undefined, + params: BigIntFilterDisplayParams | undefined + ): bigint | null { + return stringToBigInt(params?.bigintParser, text); } - protected override shouldKeepInvalidInputState(): boolean { - return !_isBrowserFirefox() && this.hasInvalidInputs() && this.getConditionTypes().includes('inRange'); + protected override getValueFormatter(): ((value: bigint | null) => string | null) | undefined { + return this.params.bigintFormatter; } - private refreshInputValidation(): void { - for (let i = 0; i < this.eValuesFrom.length; i++) { - const from = this.eValuesFrom[i]; - const to = this.eValuesTo[i]; - this.refreshInputPairValidation(from, to); - } + protected override createInputWidget(): GridInputTextField { + return this.createTextInput(getAllowedCharPattern(this.params)); } - private refreshInputPairValidation(from: GridInputTextField, to: GridInputTextField, isFrom = false): void { - const { bigintParser } = this.params; - const fromValue = this.getParsedValue(from, bigintParser); - const toValue = this.getParsedValue(to, bigintParser); + protected override refreshInputPairValidation( + from: GridInputTextField, + to: GridInputTextField, + isFrom = false + ): void { + const fromValue = this.readValue(from, true); + const toValue = this.readValue(to, true); const fromInvalid = this.isInvalidValue(from, fromValue); const toInvalid = this.isInvalidValue(to, toValue); @@ -84,108 +89,6 @@ export class BigIntFilter extends SimpleFilter< } } - protected override getState(): { isInvalid: boolean } { - return { isInvalid: this.hasInvalidInputs() }; - } - - protected override areStatesEqual(stateA?: { isInvalid: boolean }, stateB?: { isInvalid: boolean }): boolean { - return (stateA?.isInvalid ?? false) === (stateB?.isInvalid ?? false); - } - - public override refresh(legacyNewParams: ProvidedFilterParams): boolean { - const result = super.refresh(legacyNewParams); - - const { state: newState, additionalEventAttributes } = legacyNewParams as unknown as BigIntFilterDisplayParams; - const oldState = this.state; - - const fromAction = additionalEventAttributes?.fromAction; - const forceRefreshValidation = fromAction && fromAction != 'apply'; - - if ( - forceRefreshValidation || - newState.model !== oldState.model || - !this.areStatesEqual(newState.state, oldState.state) - ) { - this.refreshInputValidation(); - } - - return result; - } - - protected override setElementValue( - element: GridInputTextField, - value: bigint | null, - fromFloatingFilter?: boolean - ): void { - super.setElementValue(element, value as any, fromFloatingFilter); - if (value === null) { - element.setCustomValidity(''); - } - } - - protected createEValue(): HTMLElement { - const { params, eValuesFrom, eValuesTo } = this; - const allowedCharPattern = getAllowedCharPattern(params); - - const eCondition = _createElement({ tag: 'div', cls: 'ag-filter-body', role: 'presentation' }); - - const from = this.createFromToElement(eCondition, eValuesFrom, 'from', allowedCharPattern); - const to = this.createFromToElement(eCondition, eValuesTo, 'to', allowedCharPattern); - - const getFieldChangedListener = (fromEl: GridInputTextField, toEl: GridInputTextField, isFrom: boolean) => () => - this.refreshInputPairValidation(fromEl, toEl, isFrom); - - const fromListener = getFieldChangedListener(from, to, true); - from.onValueChange(fromListener); - from.addGuiEventListener('focusin', fromListener); - - const toListener = getFieldChangedListener(from, to, false); - to.onValueChange(toListener); - to.addGuiEventListener('focusin', toListener); - - return eCondition; - } - - private createFromToElement( - eCondition: HTMLElement, - eValues: GridInputTextField[], - fromTo: string, - allowedCharPattern: string | null - ): GridInputTextField { - const eValue = this.createManagedBean( - new AgInputTextField({ - allowedCharPattern: allowedCharPattern ?? undefined, - clearButton: true, - searchIcon: true, - autoComplete: this.params.browserAutoComplete, - }) - ); - eValue.addCss(`ag-filter-${fromTo}`); - eValue.addCss('ag-filter-filter'); - eValues.push(eValue); - eCondition.appendChild(eValue.getGui()); - return eValue; - } - - protected removeEValues(startPosition: number, deleteCount?: number): void { - const removeComps = (eGui: GridInputTextField[]) => this.removeComponents(eGui, startPosition, deleteCount); - - removeComps(this.eValuesFrom); - removeComps(this.eValuesTo); - } - - protected getValues(position: number): Tuple { - const { bigintParser } = this.params; - const result: Tuple = []; - this.forEachPositionInput(position, (element, index, _elPosition, numberOfInputs) => { - if (index < numberOfInputs) { - result.push(this.getParsedValue(element, bigintParser)); - } - }); - - return result; - } - protected areSimpleModelsEqual(aSimple: BigIntFilterModel, bSimple: BigIntFilterModel): boolean { return ( aSimple.filter === bSimple.filter && aSimple.filterTo === bSimple.filterTo && aSimple.type === bSimple.type @@ -210,63 +113,8 @@ export class BigIntFilter extends SimpleFilter< return model; } - protected override removeConditionsAndOperators(startPosition: number, deleteCount?: number | undefined): void { - if (this.hasInvalidInputs()) { - return; - } - - return super.removeConditionsAndOperators(startPosition, deleteCount); - } - - protected override getInputs(position: number): Tuple { - const { eValuesFrom, eValuesTo } = this; - if (position >= eValuesFrom.length) { - return [null, null]; - } - return [eValuesFrom[position], eValuesTo[position]]; - } - - protected override hasInvalidInputs(): boolean { - let invalidInputs = false; - this.forEachInput((element) => (invalidInputs ||= !element.getInputElement().validity.valid)); - return invalidInputs; - } - - protected override positionHasInvalidInputs(position: number): boolean { - let invalidInputs = false; - this.forEachPositionInput(position, (element) => (invalidInputs ||= !element.getInputElement().validity.valid)); - return invalidInputs; - } - - protected override canApply(_model: BigIntFilterModel | ICombinedSimpleModel | null): boolean { - return !this.hasInvalidInputs(); - } - - private getParsedValue( - element: GridInputTextField, - bigintParser: IBigIntFilterParams['bigintParser'] - ): bigint | null { - const rawValue = element.getValue(); - if (rawValue == null || (typeof rawValue === 'string' && rawValue.trim() === '')) { - return null; - } - return bigintParser ? bigintParser(rawValue) : _parseBigIntOrNull(rawValue); - } - private isInvalidValue(element: GridInputTextField, parsedValue: bigint | null): boolean { const rawValue = element.getValue(); return rawValue != null && String(rawValue).trim() !== '' && parsedValue === null; } } - -function getValidityMessageKey( - fromValue: bigint | null, - toValue: bigint | null, - isFrom: boolean -): FilterLocaleTextKey | null { - const isInvalid = fromValue != null && toValue != null && fromValue >= toValue; - if (!isInvalid) { - return null; - } - return `strict${isFrom ? 'Max' : 'Min'}ValueValidation`; -} diff --git a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterModelFormatter.ts b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterModelFormatter.ts index fa1074ce76b..db41cede66d 100644 --- a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterModelFormatter.ts +++ b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterModelFormatter.ts @@ -1,6 +1,5 @@ import { _parseBigIntOrNull } from 'ag-stack'; -import type { OptionsFactory } from '../optionsFactory'; import { SCALAR_FILTER_TYPE_KEYS, SimpleFilterModelFormatter } from '../simpleFilterModelFormatter'; import type { BigIntFilterModel, IBigIntFilterParams } from './iBigIntFilter'; @@ -11,8 +10,8 @@ export class BigIntFilterModelFormatter extends SimpleFilterModelFormatter< > { protected readonly filterTypeKeys = SCALAR_FILTER_TYPE_KEYS; - constructor(optionsFactory: OptionsFactory, filterParams: IBigIntFilterParams) { - super(optionsFactory, filterParams, filterParams.bigintFormatter); + protected override getValueFormatter(): ((value: bigint | null) => string | null) | undefined { + return this.filterParams.bigintFormatter; } protected conditionToString( diff --git a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterUtils.ts b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterUtils.ts index ec9f8e9345b..61744542316 100644 --- a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterUtils.ts +++ b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFilterUtils.ts @@ -9,6 +9,17 @@ export function getAllowedCharPattern(filterParams?: IBigIntFilterParams): strin return filterParams?.allowedCharPattern ?? null; } +/** The one reading of a typed value: `bigintParser` owns it wherever it is configured. */ +export function stringToBigInt( + bigintParser: IBigIntFilterParams['bigintParser'], + value?: string | null +): bigint | null { + if (value == null || value.trim() === '') { + return null; + } + return bigintParser ? bigintParser(value) : _parseBigIntOrNull(value); +} + export function mapValuesFromBigIntFilterModel( filterModel: BigIntFilterModel | null, optionsFactory: OptionsFactory diff --git a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFloatingFilter.ts b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFloatingFilter.ts index f59fb26edf7..f6b9a126f8e 100644 --- a/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFloatingFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/bigInt/bigIntFloatingFilter.ts @@ -1,12 +1,10 @@ -import { _parseBigIntOrNull } from 'ag-stack'; - import { FloatingFilterTextInputService } from '../../floating/provided/floatingFilterTextInputService'; import type { FloatingFilterInputService } from '../../floating/provided/iFloatingFilterInputService'; import { TextInputFloatingFilter } from '../../floating/provided/textInputFloatingFilter'; import type { OptionsFactory } from '../optionsFactory'; import { DEFAULT_BIGINT_FILTER_OPTIONS } from './bigIntFilterConstants'; import { BigIntFilterModelFormatter } from './bigIntFilterModelFormatter'; -import { getAllowedCharPattern } from './bigIntFilterUtils'; +import { getAllowedCharPattern, stringToBigInt } from './bigIntFilterUtils'; import type { BigIntFilterModel, BigIntFilterParams, @@ -47,13 +45,6 @@ export class BigIntFloatingFilter extends TextInputFloatingFilter(value: string | null | undefined): TValue | null { - if (value == null || value === '') { - return null; - } - - if (this.bigintParser) { - return this.bigintParser(value) as TValue | null; - } - return _parseBigIntOrNull(value) as TValue | null; + return stringToBigInt(this.bigintParser, value) as TValue | null; } } diff --git a/packages/ag-grid-community/src/filter/provided/date/dateFilterModelFormatter.ts b/packages/ag-grid-community/src/filter/provided/date/dateFilterModelFormatter.ts index 0b441fe9494..438ab491647 100644 --- a/packages/ag-grid-community/src/filter/provided/date/dateFilterModelFormatter.ts +++ b/packages/ag-grid-community/src/filter/provided/date/dateFilterModelFormatter.ts @@ -3,7 +3,6 @@ import { _dateToFormattedString, _parseDateTimeFromString } from 'ag-stack'; import type { AgColumn } from '../../../entities/agColumn'; import type { SharedFilterParams } from '../../../interfaces/iFilter'; import { translateFilterOptionKey } from '../../filterLocaleText'; -import type { OptionsFactory } from '../optionsFactory'; import { SCALAR_FILTER_TYPE_KEYS, SimpleFilterModelFormatter } from '../simpleFilterModelFormatter'; import type { DateFilterModel, IDateFilterParams } from './iDateFilter'; @@ -13,16 +12,13 @@ export class DateFilterModelFormatter extends SimpleFilterModelFormatter< Date > { protected readonly filterTypeKeys = SCALAR_FILTER_TYPE_KEYS; - constructor(optionsFactory: OptionsFactory, filterParams: IDateFilterParams) { - super(optionsFactory, filterParams, (value) => { - const { dataTypeSvc, valueSvc } = this.beans; - const column = (filterParams as SharedFilterParams).column as AgColumn; - const dateFormatFn = dataTypeSvc?.getDateFormatterFunction(column); // only exists for dateString. - // dateString value formatter requires a string, so format it first. - // date value formatter wants the original Date. - const valueToFormat = dateFormatFn ? dateFormatFn(value ?? undefined) : value; - return valueSvc.formatValue(column, null, valueToFormat); - }); + protected override formatValue(value: Date | null = null): string { + const { dataTypeSvc, valueSvc } = this.beans; + const column = (this.filterParams as SharedFilterParams).column as AgColumn; + const dateFormatFn = dataTypeSvc?.getDateFormatterFunction(column); // only exists for dateString. + // dateString's value formatter requires a string; date's wants the original Date. + const valueToFormat = dateFormatFn ? dateFormatFn(value ?? undefined) : value; + return valueSvc.formatValue(column, null, valueToFormat) ?? ''; } protected conditionToString( diff --git a/packages/ag-grid-community/src/filter/provided/number/iNumberFilter.ts b/packages/ag-grid-community/src/filter/provided/number/iNumberFilter.ts index 9b25bd90a0a..528b3505dd3 100644 --- a/packages/ag-grid-community/src/filter/provided/number/iNumberFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/number/iNumberFilter.ts @@ -40,17 +40,29 @@ export interface INumberFilterParams extends IScalarFilterParams { /** The default filter option to be selected. Must be one of the offered options. */ defaultOption?: ScalarFilterOptionKey | CustomFilterOptionKey; /** - * When specified, the input field will be of type `text`, and this will be used as a regex of all the characters that are allowed to be typed. + * When specified, this will be used as a regex of all the characters that are allowed to be typed. * This will be compared against any typed character and prevent the character from appearing in the input if it does not match. + * Either this or `numberFormatter` makes the input field of type `text`, unless `filterInputType` says otherwise. */ allowedCharPattern?: string; + /** + * The type of input used by the filter. Defaults to `text` when `allowedCharPattern` or `numberFormatter` + * is provided, and `number` otherwise. Set it explicitly to keep a `number` input for a formatter whose + * output a `number` input can hold, or to take a `text` input without configuring either. An + * `allowedCharPattern` applies to either input, narrowing what a `number` input already accepts. + * @default undefined + */ + filterInputType?: 'text' | 'number'; /** * Typically used alongside `allowedCharPattern`, this provides a custom parser to convert the value entered in the filter inputs into a number that can be used for comparisons. + * The Advanced Filter reads this column's operands with it only when a `numberFormatter` is provided too: + * without one an operand is written as a plain decimal, which the default parser is what reads back. */ numberParser?: (text: string | null) => number | null; /** - * Typically used alongside `allowedCharPattern`, this provides a custom formatter to convert the number value in the filter model - * into a string to be used in the filter input. This is the inverse of the `numberParser`. + * Provides a custom formatter to convert the number value in the filter model into a string to be used in the + * filter input. This is the inverse of the `numberParser`. Often used alongside `allowedCharPattern`, but either + * one on its own makes the filter use a text input, since a number input would discard the formatted text. */ numberFormatter?: (value: number | null) => string | null; } diff --git a/packages/ag-grid-community/src/filter/provided/number/numberFilter.ts b/packages/ag-grid-community/src/filter/provided/number/numberFilter.ts index 93fdac6e401..cc63d585339 100644 --- a/packages/ag-grid-community/src/filter/provided/number/numberFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/number/numberFilter.ts @@ -1,70 +1,86 @@ -import { _isBrowserFirefox, _makeNull } from 'ag-stack'; - import { AgInputNumberField } from '../../../agWidgets/agInputNumberField'; -import { AgInputTextField } from '../../../agWidgets/agInputTextField'; -import type { IAfterGuiAttachedParams } from '../../../interfaces/iAfterGuiAttachedParams'; import type { FilterDisplayParams } from '../../../interfaces/iFilter'; -import { _createElement } from '../../../utils/element'; import type { GridInputNumberField, GridInputTextField } from '../../../widgets/gridWidgetTypes'; -import type { FilterLocaleTextKey } from '../../filterLocaleText'; -import type { ProvidedFilterParams } from '../iProvidedFilter'; -import type { ICombinedSimpleModel, Tuple } from '../iSimpleFilter'; -import { SimpleFilter } from '../simpleFilter'; +import type { ICombinedSimpleModel } from '../iSimpleFilter'; +import { getValidityMessageKey } from '../simpleFilterUtils'; +import type { RenderChange } from '../textInputSimpleFilter'; +import { TextInputSimpleFilter } from '../textInputSimpleFilter'; import type { INumberFilterParams, NumberFilterModel } from './iNumberFilter'; import { DEFAULT_NUMBER_FILTER_OPTIONS } from './numberFilterConstants'; -import { getAllowedCharPattern, mapValuesFromNumberFilterModel, processNumberFilterValue } from './numberFilterUtils'; +import { + getAllowedCharPattern, + mapValuesFromNumberFilterModel, + processNumberFilterValue, + stringToFloat, + usesTextInput, +} from './numberFilterUtils'; /** temporary type until `NumberFilterParams` is updated as breaking change */ type NumberFilterDisplayParams = INumberFilterParams & FilterDisplayParams>; -export class NumberFilter extends SimpleFilter< +type NumberInput = GridInputTextField | GridInputNumberField; + +export class NumberFilter extends TextInputSimpleFilter< NumberFilterModel, number, - GridInputTextField | GridInputNumberField, + NumberInput, NumberFilterDisplayParams > { - private readonly eValuesFrom: (GridInputTextField | GridInputNumberField)[] = []; - private readonly eValuesTo: (GridInputTextField | GridInputNumberField)[] = []; - public readonly filterType = 'number' as const; constructor() { super('numberFilter', mapValuesFromNumberFilterModel, DEFAULT_NUMBER_FILTER_OPTIONS); } - protected override defaultDebounceMs = 500; - - public override afterGuiAttached(params?: IAfterGuiAttachedParams | undefined): void { - super.afterGuiAttached(params); + protected override getRenderChange( + params: NumberFilterDisplayParams, + previous: NumberFilterDisplayParams | undefined + ): RenderChange | undefined { + // The element type and its pattern are fixed at build time, so only a replacement can change them. + if ( + usesTextInput(params) !== usesTextInput(previous) || + getAllowedCharPattern(params) !== getAllowedCharPattern(previous) + ) { + return 'rebuild'; + } + // What an input shows is rendered through these, so its text stops being readable when they change. + if (params.numberParser !== previous?.numberParser || params.numberFormatter !== previous?.numberFormatter) { + return 'rerender'; + } + return undefined; + } - // Refresh validation - this.refreshInputValidation(); + protected override parseText( + text: string | null | undefined, + params: NumberFilterDisplayParams | undefined + ): number | null { + return processNumberFilterValue(stringToFloat(params?.numberParser, text)); } - protected override shouldKeepInvalidInputState(): boolean { - // We deliberately keep invalid input state for inRange filters when not in Firefox - // to mimic the behaviour for incomplete date and datetime inputs (which are cleared - // in Firefox but not in Chrome/Safari) - return !_isBrowserFirefox() && this.hasInvalidInputs() && this.getConditionTypes().includes('inRange'); + protected override getValueFormatter(): ((value: number | null) => string | null) | undefined { + return this.params.numberFormatter; } - private refreshInputValidation(): void { - for (let i = 0; i < this.eValuesFrom.length; i++) { - const from = this.eValuesFrom[i]; - const to = this.eValuesTo[i]; - this.refreshInputPairValidation(from, to); + protected override createInputWidget(): NumberInput { + const params = this.params; + const allowedCharPattern = getAllowedCharPattern(params); + if (usesTextInput(params)) { + return this.createTextInput(allowedCharPattern); } + return this.createBean( + new AgInputNumberField({ + allowedCharPattern: allowedCharPattern ?? undefined, + clearButton: true, + searchIcon: true, + autoComplete: params.browserAutoComplete, + }) + ); } - private refreshInputPairValidation( - from: GridInputNumberField | GridInputTextField, - to: GridInputNumberField | GridInputTextField, - isFrom = false - ): void { - const parser = this.params.numberParser; - const fromValue = getNormalisedValue(parser, from); - const toValue = getNormalisedValue(parser, to); + protected override refreshInputPairValidation(from: NumberInput, to: NumberInput, isFrom = false): void { + const fromValue = this.readValue(from, true); + const toValue = this.readValue(to, true); const localeKey = getValidityMessageKey(fromValue, toValue, isFrom); const validityMessage = localeKey ? this.translate(localeKey, [String(isFrom ? toValue : fromValue)]) : ''; (isFrom ? from : to).setCustomValidity(validityMessage); // Set validity error state for target input @@ -74,128 +90,6 @@ export class NumberFilter extends SimpleFilter< } } - protected override getState(): { isInvalid: boolean } { - // State represents non-model related UI state, so we make this equivalent to the validity state of the inputs - // so that changes in validity state cause updates to the UI (see `ProvidedFilter.refresh`). - return { isInvalid: this.hasInvalidInputs() }; - } - - protected override areStatesEqual(stateA?: { isInvalid: boolean }, stateB?: { isInvalid: boolean }): boolean { - // For DateFilter, the state is just a boolean of whether or not any inputs are invalid. - // As such, `undefined` should be identical to `false` - return (stateA?.isInvalid ?? false) === (stateB?.isInvalid ?? false); - } - - public override refresh(legacyNewParams: ProvidedFilterParams): boolean { - const result = super.refresh(legacyNewParams); - - const { state: newState, additionalEventAttributes } = legacyNewParams as unknown as NumberFilterDisplayParams; - const oldState = this.state; - - const fromAction = additionalEventAttributes?.fromAction; - const forceRefreshValidation = fromAction && fromAction != 'apply'; - - if ( - forceRefreshValidation || - newState.model !== oldState.model || - !this.areStatesEqual(newState.state, oldState.state) - ) { - this.refreshInputValidation(); - } - - return result; - } - - protected override setElementValue( - element: GridInputTextField | GridInputNumberField, - value: number | null, - fromFloatingFilter?: boolean - ): void { - // values from floating filter are directly from the input, not from the model - const { numberFormatter } = this.params; - const valueToSet = !fromFloatingFilter && numberFormatter ? numberFormatter(value ?? null) : value; - super.setElementValue(element, valueToSet as any); - if (valueToSet === null) { - element.setCustomValidity(''); - } - } - - protected createEValue(): HTMLElement { - const { params, eValuesFrom, eValuesTo } = this; - const allowedCharPattern = getAllowedCharPattern(params); - - const eCondition = _createElement({ tag: 'div', cls: 'ag-filter-body', role: 'presentation' }); - - const from = this.createFromToElement(eCondition, eValuesFrom, 'from', allowedCharPattern); - const to = this.createFromToElement(eCondition, eValuesTo, 'to', allowedCharPattern); - - const getFieldChangedListener = - ( - from: GridInputTextField | GridInputNumberField, - to: GridInputTextField | GridInputNumberField, - isFrom: boolean - ) => - () => - this.refreshInputPairValidation(from, to, isFrom); - - const fromListener = getFieldChangedListener(from, to, true); - from.onValueChange(fromListener); - from.addGuiEventListener('focusin', fromListener); - - const toListener = getFieldChangedListener(from, to, false); - to.onValueChange(toListener); - to.addGuiEventListener('focusin', toListener); - - return eCondition; - } - - private createFromToElement( - eCondition: HTMLElement, - eValues: (GridInputTextField | GridInputNumberField)[], - fromTo: string, - allowedCharPattern: string | null - ): GridInputTextField | GridInputNumberField { - const { browserAutoComplete } = this.params; - const eValue = this.createManagedBean( - allowedCharPattern - ? new AgInputTextField({ - allowedCharPattern, - clearButton: true, - searchIcon: true, - autoComplete: browserAutoComplete, - }) - : new AgInputNumberField({ - clearButton: true, - searchIcon: true, - autoComplete: browserAutoComplete, - }) - ); - eValue.addCss(`ag-filter-${fromTo}`); - eValue.addCss('ag-filter-filter'); - eValues.push(eValue); - eCondition.appendChild(eValue.getGui()); - return eValue; - } - - protected removeEValues(startPosition: number, deleteCount?: number): void { - const removeComps = (eGui: (GridInputTextField | GridInputNumberField)[]) => - this.removeComponents(eGui, startPosition, deleteCount); - - removeComps(this.eValuesFrom); - removeComps(this.eValuesTo); - } - - protected getValues(position: number): Tuple { - const result: Tuple = []; - this.forEachPositionInput(position, (element, index, _elPosition, numberOfInputs) => { - if (index < numberOfInputs) { - result.push(processNumberFilterValue(stringToFloat(this.params.numberParser, element.getValue()))); - } - }); - - return result; - } - protected areSimpleModelsEqual(aSimple: NumberFilterModel, bSimple: NumberFilterModel): boolean { return ( aSimple.filter === bSimple.filter && aSimple.filterTo === bSimple.filterTo && aSimple.type === bSimple.type @@ -219,79 +113,4 @@ export class NumberFilter extends SimpleFilter< return model; } - - protected override removeConditionsAndOperators(startPosition: number, deleteCount?: number | undefined): void { - if (this.hasInvalidInputs()) { - // When there are invalid inputs (which currently can only be when there is an invalid range in the last condition) - // we don't want to remove those conditions, to prevent the condition from disappearing just as the user finishes - // editing it. - return; - } - - return super.removeConditionsAndOperators(startPosition, deleteCount); - } - - protected getInputs(position: number): Tuple { - const { eValuesFrom, eValuesTo } = this; - if (position >= eValuesFrom.length) { - return [null, null]; - } - return [eValuesFrom[position], eValuesTo[position]]; - } - - protected override hasInvalidInputs(): boolean { - let invalidInputs = false; - this.forEachInput((element) => (invalidInputs ||= !element.getInputElement().validity.valid)); - return invalidInputs; - } - - protected override positionHasInvalidInputs(position: number): boolean { - let invalidInputs = false; - this.forEachPositionInput(position, (element) => (invalidInputs ||= !element.getInputElement().validity.valid)); - return invalidInputs; - } - - protected override canApply(_model: NumberFilterModel | ICombinedSimpleModel | null): boolean { - return !this.hasInvalidInputs(); - } -} - -function stringToFloat( - numberParser: INumberFilterParams['numberParser'], - value?: string | number | null -): number | null { - if (typeof value === 'number') { - return value; - } - - let filterText = _makeNull(value); - - if (filterText?.trim() === '') { - filterText = null; - } - - if (numberParser) { - return numberParser(filterText); - } - - return filterText == null || filterText.trim() === '-' ? null : Number.parseFloat(filterText); -} - -function getNormalisedValue( - numberParser: INumberFilterParams['numberParser'], - input: GridInputTextField | GridInputNumberField -): number | null { - return processNumberFilterValue(stringToFloat(numberParser, input.getValue(true))); -} - -function getValidityMessageKey( - fromValue: number | null, - toValue: number | null, - isFrom: boolean -): FilterLocaleTextKey | null { - const isInvalid = fromValue != null && toValue != null && fromValue >= toValue; - if (!isInvalid) { - return null; - } - return `strict${isFrom ? 'Max' : 'Min'}ValueValidation`; } diff --git a/packages/ag-grid-community/src/filter/provided/number/numberFilterModelFormatter.ts b/packages/ag-grid-community/src/filter/provided/number/numberFilterModelFormatter.ts index 2dd2d73a3b3..c2c224dfa23 100644 --- a/packages/ag-grid-community/src/filter/provided/number/numberFilterModelFormatter.ts +++ b/packages/ag-grid-community/src/filter/provided/number/numberFilterModelFormatter.ts @@ -1,4 +1,3 @@ -import type { OptionsFactory } from '../optionsFactory'; import { SCALAR_FILTER_TYPE_KEYS, SimpleFilterModelFormatter } from '../simpleFilterModelFormatter'; import type { INumberFilterParams, NumberFilterModel } from './iNumberFilter'; @@ -9,8 +8,8 @@ export class NumberFilterModelFormatter extends SimpleFilterModelFormatter< > { protected readonly filterTypeKeys = SCALAR_FILTER_TYPE_KEYS; - constructor(optionsFactory: OptionsFactory, filterParams: INumberFilterParams) { - super(optionsFactory, filterParams, filterParams.numberFormatter); + protected override getValueFormatter(): ((value: number | null) => string | null) | undefined { + return this.filterParams.numberFormatter; } protected conditionToString( diff --git a/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.test.ts b/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.test.ts new file mode 100644 index 00000000000..9c23542c412 --- /dev/null +++ b/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.test.ts @@ -0,0 +1,57 @@ +import { stringToFloat } from './numberFilterUtils'; + +// A `number` input keeps scientific notation as the text the user typed — `1e3` is a valid floating-point +// number to the HTML parser — so what reads an input back has to read that notation too. The behavioural +// suite cannot cover it: happy-dom reports such an input as invalid, where every supported browser does not. +describe('stringToFloat', () => { + test.each([ + ['1e3', 1000], + ['1E3', 1000], + ['1.5e-3', 0.0015], + ['-2.5e2', -250], + ])('reads scientific notation %s as %s', (text, expected) => { + expect(stringToFloat(undefined, text)).toBe(expected); + }); + + test.each([ + ['a plain integer', '42', 42], + ['a negative decimal', '-2.5', -2.5], + ['blank', ' ', null], + ['empty', '', null], + ['a lone minus, which is a number half-typed', '-', null], + ['a padded lone minus', ' - ', null], + ['a padded number, which parses as the number', ' 5 ', 5], + ['absent', null, null], + ])('reads %s as %s', (_name, text, expected) => { + expect(stringToFloat(undefined, text)).toBe(expected); + }); + + test.each([ + ['a number', 1000, 1000], + ['zero, which is a value and not a blank', 0, 0], + ])('takes %s as already read', (_name, value, expected) => { + expect(stringToFloat(undefined, value)).toBe(expected); + // A parser reads text; there is none to read when the value already is a number. + expect(stringToFloat(() => 42, value)).toBe(expected); + }); + + test('a numberParser owns the reading wherever one is configured', () => { + const parser = (text: string | null) => (text === 'one thousand' ? 1000 : null); + expect(stringToFloat(parser, 'one thousand')).toBe(1000); + // Blank reaches the parser as null rather than as text it never has to recognise. + expect(stringToFloat(parser, ' ')).toBe(null); + // The default reading does not apply underneath a parser that rejected the text. + expect(stringToFloat(parser, '1e3')).toBe(null); + }); + + test('a numberParser is handed the text as typed, whitespace included', () => { + const seen: (string | null)[] = []; + const parser = (text: string | null) => { + seen.push(text); + return null; + }; + stringToFloat(parser, ' 5 '); + stringToFloat(parser, ' '); + expect(seen).toEqual([' 5 ', null]); + }); +}); diff --git a/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.ts b/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.ts index c4367fc30a5..a9927b7ff46 100644 --- a/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.ts +++ b/packages/ag-grid-community/src/filter/provided/number/numberFilterUtils.ts @@ -7,6 +7,35 @@ export function getAllowedCharPattern(filterParams?: INumberFilterParams): strin return filterParams?.allowedCharPattern ?? null; } +/** A pattern usually admits characters, and a formatter writes text, that a number input's own grammar rejects. */ +export function usesTextInput(filterParams?: INumberFilterParams): boolean { + const filterInputType = filterParams?.filterInputType; + if (filterInputType) { + return filterInputType === 'text'; + } + return filterParams?.allowedCharPattern != null || filterParams?.numberFormatter != null; +} + +/** The one reading of a typed value: `numberParser` owns it wherever it is configured. */ +export function stringToFloat( + numberParser: INumberFilterParams['numberParser'], + value?: string | number | null +): number | null { + if (typeof value === 'number') { + return value; + } + + const trimmed = value?.trim() ?? ''; + // The parser gets the text as typed; only the emptiness and half-typed tests want it trimmed. + const filterText = trimmed === '' ? null : (value ?? null); + + if (numberParser) { + return numberParser(filterText); + } + + return filterText == null || trimmed === '-' ? null : Number.parseFloat(filterText); +} + export function processNumberFilterValue(value?: number | null): number | null { if (value == null) { return null; diff --git a/packages/ag-grid-community/src/filter/provided/number/numberFloatingFilter.ts b/packages/ag-grid-community/src/filter/provided/number/numberFloatingFilter.ts index 235f4c116a8..4e7124ec9b2 100644 --- a/packages/ag-grid-community/src/filter/provided/number/numberFloatingFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/number/numberFloatingFilter.ts @@ -16,7 +16,7 @@ import type { } from './iNumberFilter'; import { DEFAULT_NUMBER_FILTER_OPTIONS } from './numberFilterConstants'; import { NumberFilterModelFormatter } from './numberFilterModelFormatter'; -import { getAllowedCharPattern } from './numberFilterUtils'; +import { getAllowedCharPattern, processNumberFilterValue, stringToFloat, usesTextInput } from './numberFilterUtils'; class FloatingFilterNumberInputService extends BeanStub implements FloatingFilterInputService { private eTextInput: GridInputTextField; @@ -26,9 +26,17 @@ class FloatingFilterNumberInputService extends BeanStub implements FloatingFilte private numberInputActive = true; + constructor(private readonly allowedCharPattern: string | null) { + super(); + } + public setupGui(parentElement: HTMLElement): void { this.eNumberInput = this.createManagedBean( - new AgInputNumberField({ clearButton: true, onValueClear: () => this.onValueCleared() }) + new AgInputNumberField({ + allowedCharPattern: this.allowedCharPattern ?? undefined, + clearButton: true, + onValueClear: () => this.onValueCleared(), + }) ); this.eTextInput = this.createManagedBean( new AgInputTextField({ clearButton: true, onValueClear: () => this.onValueCleared() }) @@ -73,6 +81,10 @@ class FloatingFilterNumberInputService extends BeanStub implements FloatingFilte return this.numberInputActive ? this.eNumberInput : this.eTextInput; } + public getInputText(): string { + return this.getActiveInputElement().getInputElement().value; + } + public setValueChangedListener(listener: (e: KeyboardEvent) => void): void { this.onValueChanged = listener; } @@ -118,6 +130,7 @@ class FloatingFilterNumberInputService extends BeanStub implements FloatingFilte export class NumberFloatingFilter extends TextInputFloatingFilter { private allowedCharPattern: string | null; + private isTextInput: boolean; protected readonly filterType = 'number'; protected readonly defaultOptions = DEFAULT_NUMBER_FILTER_OPTIONS; @@ -129,27 +142,30 @@ export class NumberFloatingFilter extends TextInputFloatingFilter(value: string | null | undefined): TValue | null { - return value ? (Number(value) as TValue) : null; + const numberParser = (this.params.filterParams as NumberFilterParams | undefined)?.numberParser; + return processNumberFilterValue(stringToFloat(numberParser, value)) as TValue | null; } } diff --git a/packages/ag-grid-community/src/filter/provided/simpleFilter.ts b/packages/ag-grid-community/src/filter/provided/simpleFilter.ts index 1a88afdfb87..20b25cfa4c4 100644 --- a/packages/ag-grid-community/src/filter/provided/simpleFilter.ts +++ b/packages/ag-grid-community/src/filter/provided/simpleFilter.ts @@ -44,7 +44,7 @@ import { } from './simpleFilterUtils'; /** temporary type until `SimpleFilterParams` is updated as breaking change */ -type SimpleFilterDisplayParams = ISimpleFilterParams & +export type SimpleFilterDisplayParams = ISimpleFilterParams & FilterDisplayParams>; type FilterModelOrCombined = M | ICombinedSimpleModel | null; @@ -639,7 +639,7 @@ export abstract class SimpleFilter< } // eslint-disable-next-line @typescript-eslint/no-unused-vars - protected setElementValue(element: E, value: V | null, fromFloatingFilter?: boolean): void { + protected setElementValue(element: E, value: V | string | null, fromFloatingFilter?: boolean): void { if (element instanceof AgAbstractInputField) { element.setValue(value != null ? String(value) : null, true); } @@ -877,6 +877,15 @@ export abstract class SimpleFilter< eType.onValueChange(this.listener); + this.attachInputsOnChange(position); + } + + /** Re-attachable on its own: a replaced input carries none of the original's listeners. */ + protected attachInputsOnChange(position: number): void { + if (this.isReadOnly()) { + return; + } + this.forEachPositionInput(position, (element) => { this.attachElementOnChange(element, this.listener); }); diff --git a/packages/ag-grid-community/src/filter/provided/simpleFilterModelFormatter.ts b/packages/ag-grid-community/src/filter/provided/simpleFilterModelFormatter.ts index 2fd0d966d47..e2078f4ad8a 100644 --- a/packages/ag-grid-community/src/filter/provided/simpleFilterModelFormatter.ts +++ b/packages/ag-grid-community/src/filter/provided/simpleFilterModelFormatter.ts @@ -36,12 +36,16 @@ export abstract class SimpleFilterModelFormatter< constructor( private optionsFactory: OptionsFactory, - protected filterParams: TFilterParams, - protected readonly valueFormatter?: (value: TValue | null) => string | null + protected filterParams: TFilterParams ) { super(); } + /** Read per call, so a `colDef` refresh that replaces the formatter reaches the summary. */ + protected getValueFormatter(): ((value: TValue | null) => string | null) | undefined { + return undefined; + } + // used by: // 1) NumberFloatingFilter & TextFloatingFilter: Always, for both when editable and read only. // 2) DateFloatingFilter: Only when read only (as we show text rather than a date picker when read only) @@ -130,7 +134,7 @@ export abstract class SimpleFilterModelFormatter< } protected formatValue(value?: TValue | null): string { - const valueFormatter = this.valueFormatter; + const valueFormatter = this.getValueFormatter(); return valueFormatter ? (valueFormatter(value ?? null) ?? '') : String(value); } } diff --git a/packages/ag-grid-community/src/filter/provided/simpleFilterUtils.ts b/packages/ag-grid-community/src/filter/provided/simpleFilterUtils.ts index c33251bd8c8..006953e7064 100644 --- a/packages/ag-grid-community/src/filter/provided/simpleFilterUtils.ts +++ b/packages/ag-grid-community/src/filter/provided/simpleFilterUtils.ts @@ -1,4 +1,5 @@ import type { LogService } from '../../validation/logService'; +import type { FilterLocaleTextKey } from '../filterLocaleText'; import type { FilterOptionKey, IFilterOptionDef, ISimpleFilterModelType, JoinOperator, Tuple } from './iSimpleFilter'; import type { OptionsFactory } from './optionsFactory'; @@ -86,3 +87,15 @@ export function getNumberOfInputs(type: FilterOptionKey | null | undefined, opti return 1; } + +/** `from >= to` is not a range; the message goes on whichever end the user is editing. */ +export function getValidityMessageKey( + fromValue: V | null, + toValue: V | null, + isFrom: boolean +): FilterLocaleTextKey | null { + if (fromValue == null || toValue == null || fromValue < toValue) { + return null; + } + return `strict${isFrom ? 'Max' : 'Min'}ValueValidation`; +} diff --git a/packages/ag-grid-community/src/filter/provided/textInputSimpleFilter.ts b/packages/ag-grid-community/src/filter/provided/textInputSimpleFilter.ts new file mode 100644 index 00000000000..4519b24b9eb --- /dev/null +++ b/packages/ag-grid-community/src/filter/provided/textInputSimpleFilter.ts @@ -0,0 +1,337 @@ +import { _getActiveDomElement, _isBrowserFirefox } from 'ag-stack'; + +import { AgInputTextField } from '../../agWidgets/agInputTextField'; +import type { IAfterGuiAttachedParams } from '../../interfaces/iAfterGuiAttachedParams'; +import { _createElement } from '../../utils/element'; +import type { GridInputNumberField, GridInputTextField } from '../../widgets/gridWidgetTypes'; +import type { ProvidedFilterParams } from './iProvidedFilter'; +import type { ICombinedSimpleModel, ISimpleFilterModel, Tuple } from './iSimpleFilter'; +import type { SimpleFilterDisplayParams } from './simpleFilter'; +import { SimpleFilter } from './simpleFilter'; + +/** The value the filter wrote into an input; it stands for that input only while the text still matches. */ +interface RenderedValue { + text: string; + value: V | null; +} + +/** A `number` input reports `null` for both, so a replacement can only be re-focused, not re-positioned. */ +interface Caret { + start: number | null; + end: number | null; +} + +/** Whether new parameters need the elements replaced, or only their text shown again. */ +export type RenderChange = 'rebuild' | 'rerender'; + +/** + * A simple filter whose condition is a pair of text-holding inputs. The element type is fixed when an input is + * built, so parameters deciding it can only take effect by replacing the element. + */ +export abstract class TextInputSimpleFilter< + M extends ISimpleFilterModel, + V, + E extends GridInputTextField | GridInputNumberField, + P extends SimpleFilterDisplayParams, +> extends SimpleFilter { + /** Held by position: removing a condition shifts every later one. */ + protected readonly eValuesFrom: E[] = []; + protected readonly eValuesTo: E[] = []; + /** Keyed on the element so it cannot outlive it, unlike a position, which shifts. */ + private readonly renderedValues = new WeakMap>(); + /** The parameters the mounted inputs were filled through; their text is only readable through these. */ + private renderedWith: P | undefined; + + /** The widget itself; the pair's shared decoration is the base's. */ + protected abstract createInputWidget(): E; + + /** How one set of parameters reads its own text back. */ + protected abstract parseText(text: string | null | undefined, params: P | undefined): V | null; + + protected abstract getValueFormatter(): ((value: V | null) => string | null) | undefined; + + /** What the new parameters need done to the mounted inputs, if anything. */ + protected abstract getRenderChange(params: P, previous: P | undefined): RenderChange | undefined; + + protected abstract refreshInputPairValidation(from: E, to: E, isFrom?: boolean): void; + + protected override defaultDebounceMs = 500; + + protected override shouldKeepInvalidInputState(): boolean { + // Mimics incomplete date and datetime inputs, which Firefox clears and Chrome/Safari keep. + return !_isBrowserFirefox() && this.hasInvalidInputs() && this.getConditionTypes().includes('inRange'); + } + + protected createTextInput(allowedCharPattern: string | null): GridInputTextField { + return this.createBean( + new AgInputTextField({ + allowedCharPattern: allowedCharPattern ?? undefined, + clearButton: true, + searchIcon: true, + autoComplete: this.params.browserAutoComplete, + }) + ); + } + + private buildInput(fromTo: 'from' | 'to'): E { + const element = this.createInputWidget(); + element.addCss(`ag-filter-${fromTo}`); + element.addCss('ag-filter-filter'); + return element; + } + + public override afterGuiAttached(params?: IAfterGuiAttachedParams | undefined): void { + super.afterGuiAttached(params); + + this.refreshInputValidation(); + } + + public override refresh(legacyNewParams: ProvidedFilterParams): boolean { + const result = super.refresh(legacyNewParams); + + const { state: newState, additionalEventAttributes } = legacyNewParams as unknown as P; + const oldState = this.state; + + const fromAction = additionalEventAttributes?.fromAction; + const forceRefreshValidation = fromAction && fromAction != 'apply'; + + if ( + forceRefreshValidation || + newState.model !== oldState.model || + !this.areStatesEqual(newState.state, oldState.state) + ) { + this.refreshInputValidation(); + } + + return result; + } + + /** Non-model UI state, so validity changes reach the UI through `ProvidedFilter.refresh`. */ + protected override getState(): { isInvalid: boolean } { + return { isInvalid: this.hasInvalidInputs() }; + } + + protected override areStatesEqual(stateA?: { isInvalid: boolean }, stateB?: { isInvalid: boolean }): boolean { + return (stateA?.isInvalid ?? false) === (stateB?.isInvalid ?? false); + } + + protected override hasInvalidInputs(): boolean { + let invalidInputs = false; + this.forEachInput((element) => (invalidInputs ||= !element.getInputElement().validity.valid)); + return invalidInputs; + } + + protected override positionHasInvalidInputs(position: number): boolean { + let invalidInputs = false; + this.forEachPositionInput(position, (element) => (invalidInputs ||= !element.getInputElement().validity.valid)); + return invalidInputs; + } + + protected override canApply(_model: M | ICombinedSimpleModel | null): boolean { + return !this.hasInvalidInputs(); + } + + protected override removeConditionsAndOperators(startPosition: number, deleteCount?: number | undefined): void { + // An invalid range lives in the last condition, which must survive until the user finishes editing it. + if (this.hasInvalidInputs()) { + return; + } + + return super.removeConditionsAndOperators(startPosition, deleteCount); + } + + protected override commonUpdateSimpleParams(params: P): void { + super.commonUpdateSimpleParams(params); + + const previous = this.renderedWith; + this.renderedWith = params; + const change = this.getRenderChange(params, previous); + if (change) { + this.refreshInputElements(change === 'rebuild', previous); + } + } + + /** + * The inputs are replaced when a parameter decides a different element, so they cannot be managed beans: + * `createManagedBean` registers a destroy func that cannot be unregistered, retaining every dead widget. + */ + public override destroy(): void { + this.destroyBeans(this.eValuesFrom); + this.destroyBeans(this.eValuesTo); + super.destroy(); + } + + protected override createEValue(): HTMLElement { + const eCondition = _createElement({ tag: 'div', cls: 'ag-filter-body', role: 'presentation' }); + + const from = this.buildInput('from'); + const to = this.buildInput('to'); + this.eValuesFrom.push(from); + this.eValuesTo.push(to); + eCondition.appendChild(from.getGui()); + eCondition.appendChild(to.getGui()); + this.attachInputPairListeners(from, to); + + return eCondition; + } + + protected override removeEValues(startPosition: number, deleteCount?: number): void { + this.removeComponents(this.eValuesFrom, startPosition, deleteCount); + this.removeComponents(this.eValuesTo, startPosition, deleteCount); + } + + protected override getInputs(position: number): Tuple { + const eValuesFrom = this.eValuesFrom; + return position < eValuesFrom.length ? [eValuesFrom[position], this.eValuesTo[position]] : [null, null]; + } + + protected override getValues(position: number): Tuple { + const result: Tuple = []; + this.forEachPositionInput(position, (element, index, _position, numberOfInputs) => { + if (index < numberOfInputs) { + result.push(this.readValue(element)); + } + }); + + return result; + } + + /** The value an input holds: the one it was rendered with, until the user makes the text their own. */ + protected readValue(element: E, ignoreValidity?: boolean): V | null { + const rendered = this.getRenderedValue(element); + return rendered ? rendered.value : this.parseText(element.getValue(ignoreValidity), this.params); + } + + /** `value` widens to `string` for the legacy floating-filter path, which passes its input's text. */ + protected override setElementValue(element: E, value: V | string | null, fromFloatingFilter?: boolean): void { + // Only that path passes text, and it is the one path that neither formats nor records a value. + const modelValue = value as V | null; + // A floating filter's value comes straight from its own input, so it is shown as the user wrote it. + const valueFormatter = this.getValueFormatter(); + const valueToSet = !fromFloatingFilter && valueFormatter ? valueFormatter(modelValue) : value; + super.setElementValue(element, valueToSet, fromFloatingFilter); + const text = element.getInputElement().value; + // A floating filter passes the text the user typed there, which is read back like any other typing; + // and an input showing nothing stands for no value, whatever a formatter failed to render into it. + if (fromFloatingFilter || (text === '' && modelValue !== null)) { + this.renderedValues.delete(element); + } else { + this.renderedValues.set(element, { text, value: modelValue }); + } + // An empty condition carries no validity, whatever a formatter chose to render null as. + if (modelValue === null || valueToSet === null) { + element.setCustomValidity(''); + } + } + + private getRenderedValue(element: E): RenderedValue | undefined { + const rendered = this.renderedValues.get(element); + return rendered?.text === element.getInputElement().value ? rendered : undefined; + } + + /** Re-validates every mounted condition; a replaced element carries none of the original's validity. */ + protected refreshInputValidation(): void { + const { eValuesFrom, eValuesTo } = this; + for (let i = 0, len = eValuesFrom.length; i < len; ++i) { + this.refreshInputPairValidation(eValuesFrom[i], eValuesTo[i]); + } + } + + /** A pair's own listeners, re-attached whenever either element is replaced. */ + private attachInputPairListeners(from: E, to: E): void { + this.attachInputListeners(from, () => this.refreshInputPairValidation(from, to, true)); + this.attachInputListeners(to, () => this.refreshInputPairValidation(from, to, false)); + } + + private attachInputListeners(element: E, refreshValidation: () => void): void { + element.onValueChange(() => { + // Typing makes the text the user's own, so the value the filter wrote no longer stands for it. + this.renderedValues.delete(element); + refreshValidation(); + }); + element.addGuiEventListener('focusin', refreshValidation); + } + + /** Shows every mounted input again, replacing the elements whose type the new parameters changed. */ + private refreshInputElements(rebuild: boolean, previous: P | undefined): void { + const eValuesFrom = this.eValuesFrom; + const numConditions = eValuesFrom.length; + if (!numConditions) { + return; + } + const eValuesTo = this.eValuesTo; + for (let position = 0; position < numConditions; ++position) { + this.refreshInputElement(position, 'from', rebuild, previous); + this.refreshInputElement(position, 'to', rebuild, previous); + if (rebuild) { + // A replacement element carries none of the original's listeners, so re-attach them all. + this.attachInputsOnChange(position); + this.attachInputPairListeners(eValuesFrom[position], eValuesTo[position]); + } + } + // Before the visibility pass: a replacement carries no validity, and an invalid condition is + // not a complete one. + this.refreshInputValidation(); + if (rebuild) { + this.updateUiVisibility(); // the replacements start visible and enabled, whatever the condition is + } + } + + private refreshInputElement( + position: number, + fromTo: 'from' | 'to', + rebuild: boolean, + previous: P | undefined + ): void { + const eValues = fromTo === 'from' ? this.eValuesFrom : this.eValuesTo; + const mounted = eValues[position]; + // Read past the validity gate: an input reported as out of order still holds what the user typed. + const text = mounted.getValue(true); + const rendered = this.getRenderedValue(mounted); + const value = rendered ? rendered.value : this.parseText(text, previous); + // Text the filter did not write is the user's own: re-rendering it would move the caret, and a + // lossy formatter would change what they typed. + const keepAsTyped = !rendered && text != null && text !== ''; + let element = mounted; + let caret: Caret | undefined; + if (rebuild) { + caret = this.takeCaret(mounted); + element = this.buildInput(fromTo); + mounted.getGui().replaceWith(element.getGui()); + this.destroyBean(mounted); + eValues[position] = element; + } + if (keepAsTyped) { + element.setValue(text, true); + // A replacement holds only its own grammar; text it refused leaves the value it stood for unshown. + if (element.getInputElement().value === text) { + this.restoreCaret(element, caret); + return; + } + } + this.setElementValue(element, value); + this.restoreCaret(element, caret); + } + + /** Where the user was, if they were in this input at all — a replacement is a different element. */ + private takeCaret(mounted: E): Caret | undefined { + const eInput = mounted.getInputElement(); + if (_getActiveDomElement(this.beans) !== eInput) { + return undefined; + } + // A `number` input reports no selection, so only its focus can be carried across. + return { start: eInput.selectionStart, end: eInput.selectionEnd }; + } + + private restoreCaret(element: E, caret: Caret | undefined): void { + if (!caret) { + return; + } + const eInput = element.getInputElement(); + eInput.focus(); + // A replacement holding no selection, such as a `number` input, reports none and throws on one. + if (caret.start != null && eInput.selectionStart != null) { + eInput.setSelectionRange(caret.start, caret.end); + } + } +} diff --git a/packages/ag-grid-enterprise/src/advancedFilter/advancedFilterExpressionService.ts b/packages/ag-grid-enterprise/src/advancedFilter/advancedFilterExpressionService.ts index 894e8b8432d..251fa1910d2 100644 --- a/packages/ag-grid-enterprise/src/advancedFilter/advancedFilterExpressionService.ts +++ b/packages/ag-grid-enterprise/src/advancedFilter/advancedFilterExpressionService.ts @@ -1,11 +1,4 @@ -import { - _exists, - _hasOwn, - _parseBigIntOrNull, - _parseDateTimeFromString, - _serialiseDate, - _toStringOrNull, -} from 'ag-stack'; +import { _hasOwn, _parseBigIntOrNull, _parseDateTimeFromString, _serialiseDate, _toStringOrNull } from 'ag-stack'; import type { AgColumn, @@ -20,7 +13,7 @@ import type { NamedBean, ValueService, } from 'ag-grid-community'; -import { BeanStub } from 'ag-grid-community'; +import { BeanStub, _toFiniteNumber } from 'ag-grid-community'; import { ADVANCED_FILTER_LOCALE_TEXT } from './advancedFilterLocaleText'; import type { AutocompleteEntry, AutocompleteListParams } from './autocomplete/autocompleteParams'; @@ -36,7 +29,20 @@ import { ScalarFilterExpressionOperators, TextFilterExpressionOperators, } from './filterExpressionOperators'; -import { getBigIntParser } from './filterExpressionUtils'; +import { getBigIntParser, getNumberFormatter, getNumberParser, hasCustomNumberOperands } from './filterExpressionUtils'; + +/** What an unquoted operand cannot carry: a space or `)` ends it, and a leading quote opens one. */ +function needsQuotes(operand: string): boolean { + return operand.includes(' ') || operand.includes(')') || operand.startsWith(`'`) || operand.startsWith('"'); +} + +/** The quote a value can be wrapped in, or null when it holds both kinds: either one would end it early. */ +function quoteChar(operand: string): `'` | `"` | null { + if (!operand.includes('"')) { + return '"'; + } + return operand.includes(`'`) ? null : `'`; +} /** The `filterParams` an Advanced Filter evaluator honours; the rest are column-filter UI concerns. */ const COPIED_FILTER_PARAMS: (keyof FilterExpressionEvaluatorParams)[] = [ @@ -59,18 +65,24 @@ export class AdvancedFilterExpressionService extends BeanStub implements NamedBe BaseCellDataType, (model: { filter?: string | number; colId: string }) => string | null > = { - number: (model) => _toStringOrNull(model.filter) ?? '', + // Written in the column's own syntax exactly where `getNumberParser` reads that syntax back. + number: (model) => { + const column = this.colModel.getNonPivotCol(model.colId); + return this.formatOperand( + model.filter, + getNumberFormatter(column), + _toFiniteNumber, + getNumberParser(column) + ); + }, bigint: (model) => { - const rawValue = _toStringOrNull(model.filter); const column = this.colModel.getNonPivotCol(model.colId); - const formatter = column?.colDef.filterParams?.bigintFormatter; - if (!formatter || rawValue == null) { - return rawValue ?? ''; - } - // The model already holds the canonical decimal string, so parse it back with the - // default parser - the custom parser expects user-facing input and could double-transform. - const parsed = _parseBigIntOrNull(rawValue); - return (parsed == null ? null : formatter(parsed)) ?? rawValue; + return this.formatOperand( + model.filter, + column?.colDef.filterParams?.bigintFormatter, + _parseBigIntOrNull, + getBigIntParser(column) + ); }, date: (model) => { const column = this.colModel.getNonPivotCol(model.colId); @@ -107,7 +119,7 @@ export class AdvancedFilterExpressionService extends BeanStub implements NamedBe BaseCellDataType, (op: string, cln: AgColumn, dt: BaseCellDataType) => number | string | null > = { - number: (operand) => (_exists(operand) ? Number(operand) : null), + number: (operand, column) => (operand != null && operand !== '' ? getNumberParser(column)(operand) : null), bigint: (operand, column) => { const parsed = getBigIntParser(column)(operand); return parsed == null ? null : String(parsed); @@ -172,6 +184,37 @@ export class AdvancedFilterExpressionService extends BeanStub implements NamedBe return columnName; } + /** + * A stored operand as the column writes it for display. The model value is canonical, so text the + * default parser cannot read is already the user's own input and is shown as they typed it. + * `readBack` is how the expression reads the display again, and only a format that survives that + * is used: the expression is what the operand is parsed back out of, so one that does not round-trip + * would quietly rewrite the stored value. + */ + private formatOperand( + filter: string | number | undefined, + formatter: ((value: V) => string | null) | null | undefined, + parse: (rawValue: string) => V | null, + readBack: (rawValue: string) => V | null + ): string { + const rawValue = _toStringOrNull(filter); + // Blank is no operand at all, and must not be presented as whatever the formatter makes of zero. + if (rawValue == null || rawValue.trim() === '') { + return ''; + } + if (!formatter) { + return rawValue; + } + const parsed = parse(rawValue); + const formatted = parsed == null ? null : formatter(parsed); + // Blank reads back as the value it came from and still leaves the expression without an operand. + if (formatted == null || formatted.trim() === '') { + return rawValue; + } + const reread = readBack(formatted); + return reread != null && String(reread) === String(parsed) ? formatted : rawValue; + } + public getOperatorDisplayValue(model: ColumnAdvancedFilterModel): string | undefined { return this.getExpressionOperator(model.filterType, model.type)?.displayValue ?? model.type; } @@ -185,16 +228,17 @@ export class AdvancedFilterExpressionService extends BeanStub implements NamedBe } /** - * Whether a stored operand is itself valid input for its data type, i.e. whether feeding the model - * value back into the expression or the builder editor yields the same value again. - * - * True for most types: text and number model values are their input form, and dates store the iso - * string the editor expects. It is false only for `bigint`, where the model holds the canonical - * decimal while input goes through the column's `bigintParser` - so a parser reading a non-decimal - * syntax would reinterpret that decimal as a different number. Those operands have to be presented - * through `getOperandDisplayValue` (the `bigintFormatter`) or kept as the text the user typed. + * Whether feeding the model value back into the expression or the builder editor yields the same value. + * False where the column's own parser reads a syntax the model value is not written in: always for + * `bigint`, whose model holds the canonical decimal, and for a `number` column naming a `numberParser`. */ - public isOperandModelValueEditable(baseCellDataType: BaseCellDataType): boolean { + public isOperandModelValueEditable( + baseCellDataType: BaseCellDataType, + column: AgColumn | null | undefined + ): boolean { + if (baseCellDataType === 'number') { + return !hasCustomNumberOperands(column); + } return baseCellDataType !== 'bigint'; } @@ -204,19 +248,26 @@ export class AdvancedFilterExpressionService extends BeanStub implements NamedBe if (filter == null) { return ''; } - let operand1 = this.filterOperandGetters[filterType]( - model as Exclude - ); - if (filterType !== 'number' && filterType !== 'bigint') { - operand1 ??= _toStringOrNull(filter) ?? ''; - if (!skipFormatting) { - // Quote with the char the value does not contain so a value holding one quote kind - // still round-trips (the parser accepts either quote); a value with both fails safe. - const quote = operand1.includes('"') && !operand1.includes(`'`) ? `'` : `"`; + const canonical = _toStringOrNull(filter) ?? ''; + let operand1 = + this.filterOperandGetters[filterType]( + model as Exclude + ) ?? canonical; + const isNumeric = filterType === 'number' || filterType === 'bigint'; + // A numeric operand is written bare, so quotes are added only for a format that could not be read + // back without them. Text is always quoted, empty included. + if (!skipFormatting && (!isNumeric || needsQuotes(operand1))) { + const quote = quoteChar(operand1); + if (quote) { operand1 = `${quote}${operand1}${quote}`; + } else if (isNumeric) { + // No quote can wrap this format, so it cannot be read back: write the canonical number instead. + operand1 = canonical; + } else { + operand1 = `"${operand1}"`; // text is the value itself, so there is nothing to fall back to } } - return skipFormatting ? operand1! : ` ${operand1}`; + return skipFormatting ? operand1 : ` ${operand1}`; } public parseColumnFilterModel(model: ColumnAdvancedFilterModel): string { diff --git a/packages/ag-grid-enterprise/src/advancedFilter/builder/conditionPillWrapperComp.ts b/packages/ag-grid-enterprise/src/advancedFilter/builder/conditionPillWrapperComp.ts index b8d294253b9..2d944121803 100644 --- a/packages/ag-grid-enterprise/src/advancedFilter/builder/conditionPillWrapperComp.ts +++ b/packages/ag-grid-enterprise/src/advancedFilter/builder/conditionPillWrapperComp.ts @@ -11,6 +11,7 @@ import { Component } from 'ag-grid-community'; import type { AdvancedFilterExpressionService } from '../advancedFilterExpressionService'; import type { AutocompleteEntry } from '../autocomplete/autocompleteParams'; +import { getNumberParser } from '../filterExpressionUtils'; import type { AdvancedFilterBuilderEvents, AdvancedFilterBuilderItem, @@ -119,16 +120,17 @@ export class ConditionPillWrapperComp extends Component; const key = (typeof filter === 'number' || typeof filter === 'bigint' ? _toStringOrNull(filter) : filter) ?? ''; - const valueFormatter = (value: string) => - this.advFilterExpSvc.getOperandDisplayValue({ ...this.filterModel, filter: value } as any, true); + // Read from the model, not from the text handed in: an edit is stored through the column's own + // parser, and the display is produced by the default one, which need not read the same syntax. + const valueFormatter = () => this.getOperandDisplayValue(); this.eOperandPill = this.createPill({ key, // Convert from the input format to display format. // Input format matches model format except for numbers, but these get stringified anyway valueFormatter, - // Where the stored operand is not valid input for its type, edit the displayed text instead: - // the display value is produced by the same formatter whose output that type's parser accepts. - editValueFormatter: this.advFilterExpSvc.isOperandModelValueEditable(this.baseCellDataType) + // Where the stored operand is not valid input for the column, edit the displayed text instead: + // the column's own parser is what reads an edit back, and its grammar need not be the input's. + editValueFormatter: this.advFilterExpSvc.isOperandModelValueEditable(this.baseCellDataType, this.column) ? undefined : valueFormatter, baseCellDataType: this.baseCellDataType, @@ -176,6 +178,7 @@ export class ConditionPillWrapperComp extends Component { private readonly eLabel: HTMLElement = RefPlaceholder; private eEditor: GridInputTextField | undefined; + /** What the editor opened with, so closing it untouched is not read back as an edit. */ + private editorOpenedWith: string | undefined; private value: string; private displayValue: string; @@ -128,11 +130,12 @@ export class InputPillComp extends Component { return; } _setDisplayed(this.ePill, false); - this.eEditor = this.createEditorComp(this.params.type); + this.eEditor = this.createEditorComp(); const { editValueFormatter } = this.params; // Edit the value as it is displayed, so a formatted operand does not flip back to the raw // model value when the editor opens. - this.eEditor.setValue(editValueFormatter?.(this.value) ?? this.value); + this.editorOpenedWith = editValueFormatter?.(this.value) ?? this.value; + this.eEditor.setValue(this.editorOpenedWith); const eEditorGui = this.eEditor.getGui(); this.eEditor.addManagedElementListeners(eEditorGui, { keydown: (event: KeyboardEvent) => { @@ -158,7 +161,9 @@ export class InputPillComp extends Component { /** * Responsible for instantiating an InputField and calling some of the setup methods */ - private createEditorComp(type: BaseCellDataType): GridInputTextField { + private createEditorComp(): GridInputTextField { + // An operand edited as displayed is text no typed input would keep, so it is edited as text. + const type = this.params.editValueFormatter ? 'text' : this.params.type; const [Comp, postConstruct] = inputComponentDescriptors[type]; // eslint-disable-next-line sonarjs/new-operator-misuse -- false positive: Comp is a class constructor from inputComponentDescriptors const instance = this.createBean(new Comp()); @@ -208,6 +213,12 @@ export class InputPillComp extends Component { return; } const value = this.eEditor.getValue() ?? ''; + // Blurring an untouched editor is not an edit: re-reading its text would put the operand back + // through the column's parser, which need not return the value the text was rendered from. + if (value === this.editorOpenedWith) { + this.hideEditor(keepFocus); + return; + } this.dispatchLocalEvent>({ type: 'fieldValueChanged', value, diff --git a/packages/ag-grid-enterprise/src/advancedFilter/colFilterExpressionParser.ts b/packages/ag-grid-enterprise/src/advancedFilter/colFilterExpressionParser.ts index f3500f28aaa..25c9bf4faa8 100644 --- a/packages/ag-grid-enterprise/src/advancedFilter/colFilterExpressionParser.ts +++ b/packages/ag-grid-enterprise/src/advancedFilter/colFilterExpressionParser.ts @@ -16,6 +16,7 @@ import { findEndPosition, findStartPosition, getBigIntParser, + getNumberParser, getSearchString, updateExpression, } from './filterExpressionUtils'; @@ -248,14 +249,16 @@ class OperandParser implements Parser { BaseCellDataType, (modelValue: string | number | bigint | null) => any > = { - number: () => { - if (this.quotes || isNaN(this.modelValue as number)) { + // Read from the argument, not `this.modelValue`, which keeps the raw text when the parser rejects it. + number: (modelValue) => { + // A column's own `numberParser` reports unreadable input as null, where `Number` gives NaN. + if (modelValue == null || isNaN(modelValue as number)) { this.valid = false; this.validationMessage = this.params.advFilterExpSvc.translate('advancedFilterValidationNotANumber'); } }, bigint: () => { - if (this.quotes || _parseBigIntOrNull(this.modelValue) === null) { + if (_parseBigIntOrNull(this.modelValue) === null) { this.valid = false; this.validationMessage = this.params.advFilterExpSvc.translate('advancedFilterValidationNotABigInt'); } @@ -290,7 +293,7 @@ class OperandParser implements Parser { return true; } } else if (char === ')') { - if (this.baseCellDataType === 'number' || !this.quotes) { + if (!this.quotes) { this.parseOperand(false, position - 1); return true; } else { @@ -334,7 +337,7 @@ class OperandParser implements Parser { * otherwise the text the user typed, which the model value cannot be turned back into. */ public getBuilderValue(): string | number { - return this.params.advFilterExpSvc.isOperandModelValueEditable(this.baseCellDataType) + return this.params.advFilterExpSvc.isOperandModelValueEditable(this.baseCellDataType, this.column) ? this.modelValue : this.operand; } @@ -382,7 +385,7 @@ export class ColFilterExpressionParser { object: (a: string) => string; text: (a: string) => string; } = { - number: Number, + number: (operand) => getNumberParser(this.columnParser!.column)(operand)!, bigint: (operand) => getBigIntParser(this.columnParser!.column)(operand)!, date: (operand) => this.params.valueSvc.parseValue(this.columnParser!.column!, null, operand, undefined) as Date, diff --git a/packages/ag-grid-enterprise/src/advancedFilter/filterExpressionUtils.ts b/packages/ag-grid-enterprise/src/advancedFilter/filterExpressionUtils.ts index 92c36a1f920..53f9c19a248 100644 --- a/packages/ag-grid-enterprise/src/advancedFilter/filterExpressionUtils.ts +++ b/packages/ag-grid-enterprise/src/advancedFilter/filterExpressionUtils.ts @@ -1,6 +1,13 @@ import { _parseBigIntOrNull } from 'ag-stack'; -import type { AgColumn, ColumnModel, DataTypeService, IRowNode, ValueService } from 'ag-grid-community'; +import type { + AgColumn, + ColumnModel, + DataTypeService, + IRowNode, + NumberFilterParams, + ValueService, +} from 'ag-grid-community'; import type { AdvancedFilterExpressionService } from './advancedFilterExpressionService'; import type { FilterExpressionEvaluatorParams, FilterExpressionOperator } from './filterExpressionOperators'; @@ -45,6 +52,34 @@ export function getBigIntParser(column: AgColumn | null | undefined): (value: st return column?.colDef.filterParams?.bigintParser ?? _parseBigIntOrNull; } +/** + * The `filterParams` of a number column whose operands are written in its own syntax rather than as plain + * numbers. Both a `numberParser` and a `numberFormatter` are needed: an operand the column cannot write, it + * must not read, or a parser reading a syntax the plain number is not in would reinterpret what the grid stored. + */ +function customNumberOperandParams(column: AgColumn | null | undefined): NumberFilterParams | undefined { + const filterParams = column?.colDef.filterParams; + return filterParams?.numberParser != null && filterParams.numberFormatter != null ? filterParams : undefined; +} + +/** `Number` reads blank text as zero, which is not a number anyone wrote. */ +const parseNumberOrNull = (value: string | null): number | null => (value?.trim() ? Number(value) : null); + +/** Plain-number reading stays the default: only a column that reads *and* writes its own syntax departs from it. */ +export function getNumberParser(column: AgColumn | null | undefined): (value: string | null) => number | null { + return customNumberOperandParams(column)?.numberParser ?? parseNumberOrNull; +} + +export function getNumberFormatter( + column: AgColumn | null | undefined +): ((value: number | null) => string | null) | undefined { + return customNumberOperandParams(column)?.numberFormatter; +} + +export function hasCustomNumberOperands(column: AgColumn | null | undefined): boolean { + return customNumberOperandParams(column) != null; +} + export function getSearchString(value: string, position: number, endPosition: number): string { if (!value) { return ''; diff --git a/scripts/gate/args.mjs b/scripts/gate/args.mjs index 13a8236ca8c..9c74e897267 100644 --- a/scripts/gate/args.mjs +++ b/scripts/gate/args.mjs @@ -134,6 +134,20 @@ export function captureUsage({ runner, quiet = true, width = 30 }) { 'is never all digits, `--wait 300` is that with a 300s cap.' ), ...(quiet ? row('--quiet', 'Console gets the paths, the summary and the failures; the log gets all.') : []), + ...row( + '--log-tail ', + 'Print the last n lines of the log when the run ends, and nothing else.', + 'Works on --wait and --async-status too, for a run started elsewhere.' + ), + ...row( + '--log-grep ', + 'Print the lines of the log matching a regex (or, if it will not compile, a', + 'substring). Combines with --log-tail for the last n matches.', + '', + 'Both exist so that wanting less than everything is a flag and not a pipe:', + 'a pipeline exits with its last command status, so `| tail` reports the', + "tail's success and hides a failed run." + ), ...row('--no-log', `No log file, and ${runner} keeps its colours.`), ].join('\n'); } @@ -145,6 +159,8 @@ const captureFlags = { '--kill': { takes: ID, apply: (state, id) => (state.capture.killId = id) }, '--wait': { takes: ID, timeout: true, apply: (state, id) => (state.capture.waitId = id) }, '--quiet': { takes: NONE, apply: (state) => (state.capture.quiet = true) }, + '--log-tail': { takes: NUMBER, hint: 'a line count', apply: (state, n) => (state.capture.logTail = Number(n)) }, + '--log-grep': { takes: VALUE, hint: 'a regex or substring', apply: (state, p) => (state.capture.logGrep = p) }, '--no-log': { takes: NONE, apply: (state) => (state.capture.noLog = true) }, // Internal: the detached child of --async is handed the id its parent already printed. '--run-id': { takes: VALUE, apply: (state, id) => (state.capture.runId = id) }, diff --git a/scripts/gate/main.mjs b/scripts/gate/main.mjs index 3f61f10f4fa..c37a3e5d261 100644 --- a/scripts/gate/main.mjs +++ b/scripts/gate/main.mjs @@ -41,13 +41,16 @@ async function main() { const wantsHelp = argv.includes('-h') || argv.includes('--help'); const state = parseArgs(wantsHelp ? [] : argv, gate.flags ?? {}); const { async: runAsync, statusId, killId, waitId, waitTimeout = 0, quiet, noLog, runId } = state.capture; + const { logTail, logGrep } = state.capture; + // Asking for part of the log means the console is not also getting all of it. + const filtered = logTail !== undefined || logGrep !== undefined; // Some modes hand the terminal to the runner and never end on their own (watch, --ui, --debug), so there // is nothing to capture and nothing to wait for: the log would grow with every re-run and the status would // stay `running` forever. A gate returns the mode's name to refuse `--async` in those words, or just // `true` to let the generic "the run log is off" answer stand. const endless = gate.endless?.(state); - const capture = noLog || isCI || endless ? 'off' : quiet || gate.capture === 'file' ? 'file' : 'stream'; + const capture = noLog || isCI || endless ? 'off' : quiet || filtered || gate.capture === 'file' ? 'file' : 'stream'; const runLog = new RunLog({ name: gate.name, @@ -55,7 +58,8 @@ async function main() { id: runId, capture, // A gate that prints its own verdict would otherwise print the same lines twice. - report: gate.report ?? Boolean(quiet), + report: gate.report ?? Boolean(quiet || filtered), + filter: filtered ? { tail: logTail, grep: logGrep } : undefined, failRe: gate.failRe, summaryRe: gate.summaryRe, }); @@ -80,6 +84,13 @@ async function main() { return 2; } + // Before any branch that could return the runner's own exit code: output that was asked for and never + // produced must not read as a pass. + if (filtered && !runLog.enabled) { + console.error('--log-tail/--log-grep need the run log, which is off (CI, --no-log, or an interactive mode)'); + return 1; + } + // Reports on or stops an existing run instead of starting one. Every gate needs the same three branches, // and duplicating them is how their spellings drifted apart. if (waitId) { @@ -96,6 +107,11 @@ async function main() { console.error(`${gate.script}: --async cannot combine with ${endless}.`); return 2; } + // The detached child's console is /dev/null, so the lines would be written where nobody can read them. + if (filtered) { + console.error(`${gate.script}: --async cannot combine with --log-tail/--log-grep; use them on --wait.`); + return 2; + } if (!runLog.enabled) { console.error('--async needs the run log, which is off (CI, --no-log, or an interactive mode)'); return 1; diff --git a/scripts/gate/run-log.mjs b/scripts/gate/run-log.mjs index fbe5ee1cb69..99eaed54e12 100644 --- a/scripts/gate/run-log.mjs +++ b/scripts/gate/run-log.mjs @@ -139,11 +139,12 @@ export class RunLog { * (the console gets nothing, so the gate can report only what matters) or 'off'. `report` closes a run * that showed the console nothing with the part a human still needs. */ - constructor({ name, rootDir, id, capture = 'stream', report = false, failRe, summaryRe }) { + constructor({ name, rootDir, id, capture = 'stream', report = false, filter, failRe, summaryRe }) { this.name = name; this.rootDir = rootDir; this.capture = capture; this.report = report; + this.filterOptions = filter; this.failRe = failRe; this.summaryRe = summaryRe; this.root = path.join(rootDir, 'tmp', `_${name}-output`); @@ -270,7 +271,45 @@ export class RunLog { } // A streamed log was stripped as it was written, so stripping it again would scan tens of MB to match // nothing; a captured one still carries the odd escape (Nx emits a few even under NO_COLOR). - return (text.includes(ESC) ? stripAnsi(text) : text).split('\n'); + const lines = (text.includes(ESC) ? stripAnsi(text) : text).split('\n'); + // The split of a newline-terminated file ends in an empty element, which is not a line: counted as + // one, `--log-tail 1` asks for it and prints nothing. + if (lines.at(-1) === '') { + lines.pop(); + } + return lines; + } + + /** + * Part of a log, printed instead of the digest — so wanting less than everything is a flag rather than a + * pipe. A pipeline exits with its last command's status, which hides the run's and reports red as green. + */ + printFiltered(file = this.file) { + if (!this.filterOptions) { + return; + } + const { tail, grep } = this.filterOptions; + let lines = this.lines(file); + if (grep) { + let matches; + try { + const re = new RegExp(grep); + matches = (line) => re.test(line); + } catch { + // An unusable pattern is a substring the caller typed, not a reason to lose the output. + matches = (line) => line.includes(grep); + } + lines = lines.filter(matches); + } + const total = lines.length; + if (tail != null && total > tail) { + lines = lines.slice(total - tail); + console.log(`… ${total - tail} earlier ${grep ? 'matching ' : ''}lines in the log`); + } + console.log(lines.join('\n').trimEnd()); + if (grep && !total) { + console.log(`(no line matched ${grep})`); + } } // The two things worth repeating out of a log: what the runner concluded, and every line it failed on. @@ -363,37 +402,39 @@ export class RunLog { const elapsed = Math.round((Date.now() - this.started) / 1000); this.writeStatus({ state: 'exit', code, at: stamp(new Date()), elapsed }); const tty = process.env.AG_GATE_TTY; - if (!this.report && !tty) { - return code; - } - const { summary, failures } = this.digest(); - if (this.report) { - for (const line of [...summary, ...failures]) { - console.log(line); + // Filtered output is the whole console answer, so the digest it would otherwise print is left out. + const report = this.report && !this.filterOptions; + if (report || tty) { + const { summary, failures } = this.digest(); + if (report) { + for (const line of [...summary, ...failures]) { + console.log(line); + } + console.log(`▶ ${this.name} exit ${code} after ${elapsed}s → ${this.relative(this.file)}`); } - console.log(`▶ ${this.name} exit ${code} after ${elapsed}s → ${this.relative(this.file)}`); - } - // A detached run's own output went to /dev/null, so it reports back to the terminal it was launched - // from - the shell there has long since returned to a prompt. Skipped when there was no terminal (an - // agent, a cron, a pipe), and best-effort: the terminal may have closed in the meantime. - if (tty) { - const verdict = code === 0 ? 'passed' : `FAILED (exit ${code})`; - try { - fs.appendFileSync( - tty, - [ - '', - `▶ ${this.name} ${this.id} finished: ${verdict} after ${elapsed}s`, - ...summary, - ...failures, - `▶ ${this.relative(this.file)}`, - '', - ].join('\n') - ); - } catch { - // The terminal has closed; the log holds everything this was repeating. + // A detached run's own output went to /dev/null, so it reports back to the terminal it was launched + // from - the shell there has long since returned to a prompt. Skipped when there was no terminal (an + // agent, a cron, a pipe), and best-effort: the terminal may have closed in the meantime. + if (tty) { + const verdict = code === 0 ? 'passed' : `FAILED (exit ${code})`; + try { + fs.appendFileSync( + tty, + [ + '', + `▶ ${this.name} ${this.id} finished: ${verdict} after ${elapsed}s`, + ...summary, + ...failures, + `▶ ${this.relative(this.file)}`, + '', + ].join('\n') + ); + } catch { + // The terminal has closed; the log holds everything this was repeating. + } } } + this.printFiltered(); return code; } @@ -500,9 +541,14 @@ export class RunLog { if (fs.existsSync(path.join(dir, 'result.json'))) { console.log(`▶ ${this.name} json report: ${rel}/result.json`); } - const { summary, failures } = this.digest(path.join(dir, 'output.log')); - for (const line of [...summary, ...failures]) { - console.log(line); + const logFile = path.join(dir, 'output.log'); + if (this.filterOptions) { + this.printFiltered(logFile); + } else { + const { summary, failures } = this.digest(logFile); + for (const line of [...summary, ...failures]) { + console.log(line); + } } return status.state === 'exit' && status.code === 0 ? 0 : 1; } diff --git a/testing/ag-test-utils/src/filters/advancedFilterBuilderHarness.ts b/testing/ag-test-utils/src/filters/advancedFilterBuilderHarness.ts index bedf5006dbb..9ea380c993c 100644 --- a/testing/ag-test-utils/src/filters/advancedFilterBuilderHarness.ts +++ b/testing/ag-test-utils/src/filters/advancedFilterBuilderHarness.ts @@ -18,6 +18,7 @@ const ITEM_WRAPPER = '.ag-advanced-filter-builder-item-wrapper'; const COLUMN_PILL = '.ag-advanced-filter-builder-column-pill'; const OPTION_PILL = '.ag-advanced-filter-builder-option-pill'; const VALUE_PILL = '.ag-advanced-filter-builder-value-pill'; +const PILL_DISPLAY = '.ag-advanced-filter-builder-pill-display'; const JOIN_PILL = '.ag-advanced-filter-builder-join-pill'; /** Column-pill captions in rendered order — the observable signature of the builder's item list. */ @@ -165,6 +166,11 @@ export class AdvancedFilterBuilderHarness { return Array.from(this.liveItem(item).querySelectorAll(VALUE_PILL)); } + /** Display text of value pill `index` on `item`. */ + public valuePillText(item: HTMLElement, index = 0): string { + return this.valuePills(item)[index]?.querySelector(PILL_DISPLAY)?.textContent?.trim() ?? ''; + } + /** Clicks value pill `index` on `item` and returns the editor input it opens. */ public async openValueEditor(item: HTMLElement, index = 0): Promise { const pill = this.valuePills(item)[index]; diff --git a/testing/behavioural/src/filters/advanced-filter-regression/af-parser.test.ts b/testing/behavioural/src/filters/advanced-filter-regression/af-parser.test.ts index 70df0197264..2345cd6936d 100644 --- a/testing/behavioural/src/filters/advanced-filter-regression/af-parser.test.ts +++ b/testing/behavioural/src/filters/advanced-filter-regression/af-parser.test.ts @@ -197,26 +197,44 @@ describe('Advanced Filter — operand parser edge cases', () => { `); }); - test('a quoted number is rejected on a number column', async () => { + // Quotes hold whatever the column's own parser reads, so a number may be written in them too. + test('a quoted number operand reads as the number it holds', async () => { const api = await gridsManager.createGridAndWait('grid1', DEFAULT_OPTIONS); await AdvancedFilterHarness.get(api).applyExpression('[Age] = "40"'); await asyncSetTimeout(0); - expect(displayedAthletes(api)).toEqual(ALL); + expect(displayedAthletes(api)).toEqual(['José']); await new FilterDom(api, 'quoted number panel').checkFilterDom(` ADVANCED FILTER input: "[Age] = "40"" - valid: false — Expression has an error. Value is not a number - "40". + valid: true buttons: Apply ⊘ | Builder - model: null + model: + filterType: "number" + colId: "age" + type: "equals" + filter: 40 `); await new GridRows(api, 'quoted-number rows').check(` ROOT id:ROOT_NODE_ID - ├── LEAF id:0 athlete:"Bolt" country:"Jamaica" age:25 - ├── LEAF id:1 athlete:"O'Brien" country:"Ireland" age:30 - ├── LEAF id:2 athlete:"José" country:"España" age:40 - ├── LEAF id:3 athlete:'say "hi"' country:"United States" age:50 - └── LEAF id:4 athlete:"123" country:"New Zealand" age:60 + └── LEAF id:2 athlete:"José" country:"España" age:40 + `); + }); + + // Quotes hold text, and blank text is not a number the user wrote, however `Number` reads it. + test('a blank quoted number operand is rejected', async () => { + const api = await gridsManager.createGridAndWait('grid1', DEFAULT_OPTIONS); + + await AdvancedFilterHarness.get(api).applyExpression('[Age] = " "'); + await asyncSetTimeout(0); + expect(displayedAthletes(api)).toEqual(ALL); + expect(api.getAdvancedFilterModel()).toBe(null); + await new FilterDom(api, 'blank quoted number panel').checkFilterDom(` + ADVANCED FILTER + input: "[Age] = " "" + valid: false — Expression has an error. Value is not a number - " ". + buttons: Apply ⊘ | Builder + model: null `); }); @@ -648,18 +666,18 @@ describe('Advanced Filter — parser edge cases', () => { `); }); - test('a quoted bigint operand is rejected as not a bigint', async () => { + test('a quoted bigint operand reads as the bigint it holds', async () => { const api = await gridsManager.createGridAndWait('grid1', OPTS); const af = AdvancedFilterHarness.get(api); await af.type('[Big] = "10000000000000000001"'); await asyncSetTimeout(0); - await new FilterDom(api, 'quoted bigint rejected').checkFilterDom(` + await new FilterDom(api, 'quoted bigint accepted').checkFilterDom(` ADVANCED FILTER input: "[Big] = "10000000000000000001"" - valid: false — Expression has an error. Value is not a big integer - "10000000000000000001". - buttons: Apply ⊘ | Builder + valid: true + buttons: Apply | Builder model: null `); }); diff --git a/testing/behavioural/src/filters/advanced-filter/advanced-filter-bigint-custom-parser.test.ts b/testing/behavioural/src/filters/advanced-filter/advanced-filter-bigint-custom-parser.test.ts index b9db9f2b364..187226129e3 100644 --- a/testing/behavioural/src/filters/advanced-filter/advanced-filter-bigint-custom-parser.test.ts +++ b/testing/behavioural/src/filters/advanced-filter/advanced-filter-bigint-custom-parser.test.ts @@ -71,15 +71,6 @@ function applyExpression(gridDiv: HTMLElement, expression: string): void { input.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); } -/** Display text of a builder condition row's value pill. */ -function valuePillText(item: HTMLElement): string { - return ( - item - .querySelector('.ag-advanced-filter-builder-value-pill .ag-advanced-filter-builder-pill-display') - ?.textContent?.trim() ?? '' - ); -} - const withParser: GridOptions['columnDefs'] = [ { field: 'value', @@ -200,7 +191,7 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { const [condition] = await builder.conditionItems(); // The stored decimal model value is displayed through the column's bigintFormatter. - expect(valuePillText(condition)).toBe('0xFF'); + expect(builder.valuePillText(condition)).toBe('0xFF'); await new FilterDom(api, 'builder pill shows the formatted operand', { mode: 'builder' }).checkFilterDom(` BUILDER AND @@ -248,12 +239,12 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { // The builder editor presents the stored operand through the formatter, not as the canonical // decimal it is stored as — assert that here, while the builder is still open, so a // regression showing `1000`/`255` in the editor cannot hide behind the applied expression. - expect(valuePillText(condition)).toBe('0x3E8'); + expect(builder.valuePillText(condition)).toBe('0x3E8'); await builder.setValue(condition, '255'); // Re-query: committing the edit re-renders the condition row, detaching the earlier element. const [editedCondition] = await builder.conditionItems(); - expect(valuePillText(editedCondition)).toBe('0xFF'); + expect(builder.valuePillText(editedCondition)).toBe('0xFF'); await builder.apply(); await asyncSetTimeout(0); @@ -284,7 +275,7 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { const builder = await AdvancedFilterBuilderHarness.open(api); const [condition] = await builder.conditionItems(); - expect(valuePillText(condition)).toBe('0x3E8'); + expect(builder.valuePillText(condition)).toBe('0x3E8'); // Editing must start from the value shown on the pill (the formatter's output, which the // column's parser accepts), not the canonical decimal the model stores. @@ -315,7 +306,7 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { const builder = await AdvancedFilterBuilderHarness.open(api); const [condition] = await builder.conditionItems(); - expect(valuePillText(condition)).toBe('0xff'); + expect(builder.valuePillText(condition)).toBe('0xff'); expect((await builder.openValueEditor(condition)).value).toBe('0xff'); await new FilterDom(api, 'builder keeps the typed operand syntax', { mode: 'builder' }).checkFilterDom(` BUILDER @@ -337,7 +328,7 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { await builder.close(); const reopened = await AdvancedFilterBuilderHarness.open(api); - expect(valuePillText((await reopened.conditionItems())[0])).toBe('0xff'); + expect(reopened.valuePillText((await reopened.conditionItems())[0])).toBe('0xff'); expect(api.getAdvancedFilterModel()).toEqual({ filterType: 'bigint', colId: 'value', @@ -370,7 +361,7 @@ describe('Advanced Filter - bigint custom parser and formatter', () => { const builder = await AdvancedFilterBuilderHarness.open(api); const [condition] = await builder.conditionItems(); - expect(valuePillText(condition)).toBe('1000'); + expect(builder.valuePillText(condition)).toBe('1000'); expect((await builder.openValueEditor(condition)).value).toBe('1000'); await new FilterDom(api, 'builder decimal operand with no formatter', { mode: 'builder' }).checkFilterDom(` BUILDER diff --git a/testing/behavioural/src/filters/advanced-filter/advanced-filter-number-custom-parser.test.ts b/testing/behavioural/src/filters/advanced-filter/advanced-filter-number-custom-parser.test.ts new file mode 100644 index 00000000000..20f3c5cd739 --- /dev/null +++ b/testing/behavioural/src/filters/advanced-filter/advanced-filter-number-custom-parser.test.ts @@ -0,0 +1,589 @@ +import { + AdvancedFilterBuilderHarness, + AdvancedFilterHarness, + GridRows, + TestGridsManager, + asyncSetTimeout, + installFilterLayoutMock, + uninstallFilterLayoutMock, +} from 'ag-test-utils'; + +import type { GridOptions } from 'ag-grid-community'; +import { ClientSideRowModelModule, NumberFilterModule, TextFilterModule } from 'ag-grid-community'; +import { AdvancedFilterModule } from 'ag-grid-enterprise'; + +interface TestRow { + value: number; + plain?: number; +} + +/** Reads the grouped form the formatter writes; plain `Number` cannot. */ +function parseGrouped(value: string | null): number | null { + const trimmed = value?.trim(); + if (!trimmed) { + return null; + } + if (!/^[+-]?[\d,]+(\.\d+)?$/.test(trimmed)) { + return null; + } + const parsed = Number(trimmed.replace(/,/g, '')); + return isNaN(parsed) ? null : parsed; +} + +function formatGrouped(value: number | null): string | null { + if (value == null) { + return null; + } + const sign = value < 0 ? '-' : ''; + const [whole, fraction] = String(Math.abs(value)).split('.'); + const grouped = whole.replace(/\B(?=(\d{3})+(?!\d))/g, ','); + return `${sign}${grouped}${fraction ? `.${fraction}` : ''}`; +} + +const ROW_DATA: TestRow[] = [{ value: 10 }, { value: 255 }, { value: 1234 }, { value: 1234567 }]; + +const withParser: GridOptions['columnDefs'] = [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { + allowedCharPattern: '\\d,.+\\-', + numberParser: parseGrouped, + numberFormatter: formatGrouped, + }, + }, +]; + +/** + * A number column's `numberParser` and `numberFormatter` decide its operands in the Advanced Filter, + * exactly as `bigintParser` and `bigintFormatter` already do for a BigInt column. + */ +describe('Advanced Filter - number custom parser and formatter', () => { + const gridsManager = new TestGridsManager({ + modules: [NumberFilterModule, TextFilterModule, AdvancedFilterModule, ClientSideRowModelModule], + }); + beforeAll(() => installFilterLayoutMock()); + afterAll(() => uninstallFilterLayoutMock()); + afterEach(() => gridsManager.reset()); + + // Both round-trip: the model holds the plain number, so only the formatter can put the grouping back. + test.each([ + ['a single separator', '[Value] = 1,234', 'equals', 1234], + ['more than one separator', '[Value] > 1,234,566', 'greaterThan', 1234566], + ] as const)( + 'a grouped operand with %s is read by the parser and displayed through the formatter', + async (_name, expression, type, filter) => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + await AdvancedFilterHarness.get(api).applyExpression(expression); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type, + filter, + }); + + api.setAdvancedFilterModel(api.getAdvancedFilterModel()); + expect(AdvancedFilterHarness.get(api).value).toBe(expression); + } + ); + + // A saved model is what users persist, and it is canonical: restoring one must give back the same model + // and the same rows, whatever the column now displays the operand as. + test.each([ + ['a value the formatter groups', 1234567], + ['a value it does not', 255], + ] as const)('a saved model with %s restores unchanged', async (_name, filter) => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + + const saved = { filterType: 'number', colId: 'value', type: 'equals', filter } as const; + api.setAdvancedFilterModel(saved); + api.onFilterChanged(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual(saved); + expect(api.getDisplayedRowAtIndex(0)?.data?.value).toBe(filter); + expect(api.getDisplayedRowCount()).toBe(1); + + // Reopening the builder re-reads the operand out of what is displayed, which must not move it either. + const builder = await AdvancedFilterBuilderHarness.open(api); + await builder.apply(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual(saved); + expect(api.getDisplayedRowAtIndex(0)?.data?.value).toBe(filter); + expect(api.getDisplayedRowCount()).toBe(1); + }); + + test('a grouped operand filters on the number its parser reads it as', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + await AdvancedFilterHarness.get(api).applyExpression('[Value] = 1,234'); + await asyncSetTimeout(0); + + await new GridRows(api, 'grouped operand matches only 1234').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:2 value:1234 + `); + }); + + // Operands are the column's own syntax only where it can both read and write that syntax. Without a + // `numberFormatter` the grid can only write a plain number, so a `numberParser` alone does not read them: + // a parser of another syntax would reinterpret the very value the grid stored. + test('a parser with no formatter leaves operands as plain numbers, which it does not reinterpret', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { allowedCharPattern: '\\d,.+\\-', numberParser: parseGrouped }, + }, + ], + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + + // The grouped form is not the operand syntax here, so it is reported rather than silently reread. + await AdvancedFilterHarness.get(api).applyExpression('[Value] = 1,234'); + await asyncSetTimeout(0); + expect(api.getAdvancedFilterModel()).toBe(null); + + await AdvancedFilterHarness.get(api).applyExpression('[Value] = 1234'); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234, + }); + await new GridRows(api, 'plain operand matches only 1234').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:2 value:1234 + `); + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 1234'); + }); + + // A parser reading another radix would read the plain number the grid stores as a different value. + test('a parser whose syntax the grid cannot write does not reinterpret an API-set operand', async () => { + const hexParser = (text: string | null) => { + const trimmed = text?.trim(); + if (!trimmed) { + return null; + } + const parsed = parseInt(trimmed.replace(/^0x/i, ''), 16); + return isNaN(parsed) ? null : parsed; + }; + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { numberParser: hexParser }, + }, + ], + rowData: [{ value: 255 }, { value: 597 }], + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 255 } as any); + await asyncSetTimeout(0); + + // `parseInt('255', 16)` is 597: the operand the grid wrote must not be read as the parser's syntax. + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 255, + }); + await new GridRows(api, 'the API operand filters the value it was set to').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:0 value:255 + `); + }); + + test('the builder value pill formats the stored operand and parses a grouped edit', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234 } as any); + await asyncSetTimeout(0); + + const builder = await AdvancedFilterBuilderHarness.open(api); + const [condition] = await builder.conditionItems(); + + // The stored plain number is displayed through the column's numberFormatter. + expect(builder.valuePillText(condition)).toBe('1,234'); + expect((await builder.openValueEditor(condition)).value).toBe('1,234'); + + await builder.setValue(condition, '1,234,567'); + await builder.apply(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234567, + }); + await new GridRows(api, 'builder operand edit matches only 1234567').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:3 value:1234567 + `); + }); + + // The expression is what an operand is read back out of, so a format that cannot survive that is + // not used: displaying it would round the stored value away on the next read. + test('a formatter whose output does not read back leaves the operand plain', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { + numberFormatter: (v: number | null) => (v == null ? null : v.toFixed(0)), + numberParser: (text: string | null) => (text == null || text === '' ? null : parseFloat(text)), + }, + }, + ], + rowData: [{ value: 1234.56 }, { value: 1235 }], + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234.56 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + // `1235` would read back as a different operand, so the plain number is shown instead. + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 1234.56'); + + const builder = await AdvancedFilterBuilderHarness.open(api); + await builder.apply(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234.56, + }); + }); + + test('an inverse formatter survives opening the builder and an editor left untouched', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234567 }); + await asyncSetTimeout(0); + + const builder = await AdvancedFilterBuilderHarness.open(api); + const [condition] = await builder.conditionItems(); + expect(builder.valuePillText(condition)).toBe('1,234,567'); + + const editor = await builder.openValueEditor(condition); + expect(editor.value).toBe('1,234,567'); + editor.dispatchEvent(new FocusEvent('focusout', { bubbles: true })); + await asyncSetTimeout(0); + + await builder.apply(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234567, + }); + }); + + test('the operand editor follows a column change between two number columns', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { field: 'plain', headerName: 'Plain', cellDataType: 'number', filter: 'agNumberColumnFilter' }, + ...withParser!, + ], + rowData: [ + { plain: 1, value: 10 }, + { plain: 2, value: 1234567 }, + ], + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'plain', type: 'equals', filter: 1 } as any); + await asyncSetTimeout(0); + + const builder = await AdvancedFilterBuilderHarness.open(api); + const [condition] = await builder.conditionItems(); + await builder.selectColumn(condition, 'Value'); + + // Both columns are `number`, so only the new column's `numberParser` makes this a text editor. + expect((await builder.openValueEditor(condition)).type).toBe('text'); + + await builder.setValue(condition, '1,234,567'); + await builder.apply(); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234567, + }); + }); + + // A change of data type takes the operator with it, and the operand pill goes when the operator does — + // so by the time the column-identity rebuild is reached there is no pill left for it to touch. + test('a column change to another data type leaves no operand pill behind', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + ...withParser!, + { field: 'name', headerName: 'Name', cellDataType: 'text', filter: 'agTextColumnFilter' }, + ], + rowData: [{ value: 1234567, name: 'a' }], + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234567 }); + await asyncSetTimeout(0); + + const builder = await AdvancedFilterBuilderHarness.open(api); + const [condition] = await builder.conditionItems(); + expect(builder.valuePillText(condition)).toBe('1,234,567'); + + await builder.selectColumn(condition, 'Name'); + + expect(builder.valuePills(condition)).toHaveLength(0); + expect(await builder.operatorOptions(condition)).toContain('contains'); + }); + + const formatterOnly: GridOptions['columnDefs'] = [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { numberFormatter: formatGrouped }, + }, + ]; + + test('a numberFormatter with no parser leaves the operand as the plain number that reads back', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: formatterOnly, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234567 } as any); + api.onFilterChanged(); + await asyncSetTimeout(0); + + // Formatting it as `1,234,567` would leave an expression only a `numberParser` could read back. + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 1234567'); + await new GridRows(api, 'formatter-only operand matches only 1234567').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:3 value:1234567 + `); + }); + + test('an empty operand is no operand, not the formatted zero', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: '' } as any); + await asyncSetTimeout(0); + + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = '); + }); + + // A blank format reads back as the value it came from and still leaves nothing in the expression, + // so the round-trip guard has to reject it on its own account. + test('a formatter blanking a value leaves the operand plain', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { + // Blanking a zero is a display convention; reading blank back as zero is its inverse. + numberFormatter: (v: number | null) => (v === 0 ? '' : String(v)), + numberParser: (text: string | null) => (text?.trim() ? Number(text) : 0), + }, + }, + ], + rowData: [{ value: 0 }, { value: 10 }], + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 0 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 0'); + await new GridRows(api, 'blanked-format operand still matches zero').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:0 value:0 + `); + }); + + // A bare operand ends at a space or a `)` and opens on a leading quote, so a format using any of them + // is only readable back in quotes — of the kind the format itself does not use. + test.each([ + ['a space', (value: number) => value.toLocaleString('en-US').replace(/,/g, ' '), '"1 234 567"'], + ['parentheses', (value: number) => `(${value})`, '"(1234567)"'], + // Quoted with the other kind, so the operand's own quotes stay part of it. + ['a leading quote', (value: number) => `"${value}"`, `'"1234567"'`], + ] as const)( + 'a numberFormatter writing %s has its operand quoted, and reads back', + async (_name, format, quoted) => { + const parse = (text: string | null) => { + const digits = text?.replace(/[^\d]/g, ''); + return digits ? Number(digits) : null; + }; + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { + numberParser: parse, + numberFormatter: (v: number | null) => (v == null ? null : format(v)), + }, + }, + ], + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234567 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + expect(AdvancedFilterHarness.get(api).value).toBe(`[Value] = ${quoted}`); + // The quotes are the grammar's, not the operand's: what they hold still goes through the parser. + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234567, + }); + await new GridRows(api, `quoted ${_name} operand matches only 1234567`).check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:3 value:1234567 + `); + } + ); + + // Either quote ends an operand, so a format using both cannot be wrapped in one that survives. + test('a numberFormatter writing both quote kinds leaves the operand plain', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [ + { + field: 'value', + headerName: 'Value', + cellDataType: 'number', + filter: 'agNumberColumnFilter', + filterParams: { + numberFormatter: (v: number | null) => (v == null ? null : `'${v}"`), + numberParser: (text: string | null) => { + const digits = text?.replace(/[^\d]/g, ''); + return digits ? Number(digits) : null; + }, + }, + }, + ], + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234567 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 1234567'); + expect(api.getAdvancedFilterModel()).toEqual({ + filterType: 'number', + colId: 'value', + type: 'equals', + filter: 1234567, + }); + await new GridRows(api, 'unquotable format falls back to the number it stands for').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:3 value:1234567 + `); + }); + + test('a plain number operand is not quoted', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: [{ field: 'value', headerName: 'Value', cellDataType: 'number', filter: true }], + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + + api.setAdvancedFilterModel({ filterType: 'number', colId: 'value', type: 'equals', filter: 1234 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + expect(AdvancedFilterHarness.get(api).value).toBe('[Value] = 1234'); + }); + + test('an operand the parser rejects is reported as not a number', async () => { + const api = gridsManager.createGrid('grid1', { + columnDefs: withParser, + rowData: ROW_DATA, + enableAdvancedFilter: true, + }); + await asyncSetTimeout(0); + // `parseGrouped` rejects exponent notation; `Number` reads it, and used to be what decided. + await AdvancedFilterHarness.get(api).applyExpression('[Value] = 1e3'); + await asyncSetTimeout(0); + + expect(api.getAdvancedFilterModel()).toBe(null); + await new GridRows(api, 'an operand no parser reads filters nothing').check(` + ROOT id:ROOT_NODE_ID + ├── LEAF id:0 value:10 + ├── LEAF id:1 value:255 + ├── LEAF id:2 value:1234 + └── LEAF id:3 value:1234567 + `); + }); +}); diff --git a/testing/behavioural/src/filters/filter-behaviour/bigint-filter-custom-parser.test.ts b/testing/behavioural/src/filters/filter-behaviour/bigint-filter-custom-parser.test.ts index f91048d4ca2..cb7b938c3db 100644 --- a/testing/behavioural/src/filters/filter-behaviour/bigint-filter-custom-parser.test.ts +++ b/testing/behavioural/src/filters/filter-behaviour/bigint-filter-custom-parser.test.ts @@ -1,6 +1,7 @@ import { ColumnFilterHarness, FilterDom, + FloatingFilterHarness, GridRows, TestGridsManager, asyncSetTimeout, @@ -36,7 +37,7 @@ describe('BigInt Filter — custom bigintParser', () => { filter: 'agBigIntColumnFilter', filterParams: { debounceMs: 0, - allowedCharPattern: '[\\dxXa-fA-F]', + allowedCharPattern: '\\dxXa-fA-F', bigintParser: (text: string | null) => text == null || text.trim() === '' ? null : BigInt(text), }, @@ -79,7 +80,7 @@ describe('BigInt Filter — custom bigintParser', () => { filter: 'agBigIntColumnFilter', filterParams: { debounceMs: 0, - allowedCharPattern: '[\\dxXa-fA-F]', + allowedCharPattern: '\\dxXa-fA-F', bigintParser: (text: string | null) => text == null || text.trim() === '' ? null : BigInt(text), }, @@ -121,6 +122,33 @@ describe('BigInt Filter — custom bigintParser', () => { `); }); + // A pattern is used as a character class, so one already written as a class must not be wrapped twice. + test.each([ + ['bare characters', '\\dxXa-fA-F'], + ['a character class', '[\\dxXa-fA-F]'], + ])('allowedCharPattern written as %s admits the same keys', async (_name, allowedCharPattern) => { + const api: GridApi = await gridsManager.createGridAndWait('grid10', { + columnDefs: [ + { + field: 'val', + cellDataType: 'bigint' as const, + filter: 'agBigIntColumnFilter' as const, + filterParams: { debounceMs: 0, allowedCharPattern }, + }, + ], + rowData: [{ val: 255n }], + }); + const filter = await ColumnFilterHarness.open(api, 'val'); + const rejectsKey = (key: string): boolean => { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }); + filter.inputs('text', 0)[0].dispatchEvent(event); + return event.defaultPrevented; + }; + + expect([rejectsKey('5'), rejectsKey('F'), rejectsKey('x')]).toEqual([false, false, false]); + expect(rejectsKey('z')).toBe(true); + }); + test('an allowedCharPattern replaced at runtime reaches inputs built before the change', async () => { const columnDefs = (allowedCharPattern: string) => [ { @@ -137,16 +165,28 @@ describe('BigInt Filter — custom bigintParser', () => { const api: GridApi = await gridsManager.createGridAndWait('grid3', { // Decimal only, so the inputs built here reject the `x` and `F` a hex value needs. - columnDefs: columnDefs('[\\d]'), + columnDefs: columnDefs('\\d'), rowData: [{ val: 255n }, { val: 16n }], }); // Built before the swap: an input reads `allowedCharPattern` once, when it is created. const filter = await ColumnFilterHarness.open(api, 'val'); - api.setGridOption('columnDefs', columnDefs('[\\dxXa-fA-F]')); + // The pattern is a keydown guard, so only a real keystroke shows which pattern an input is holding. + const rejectsKey = (key: string): boolean => { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }); + filter.inputs('text', 0)[0].dispatchEvent(event); + return event.defaultPrevented; + }; + + expect(rejectsKey('F')).toBe(true); + + api.setGridOption('columnDefs', columnDefs('\\dxXa-fA-F')); await asyncSetTimeout(0); + // Only a replaced element carries the new pattern; the guard is installed once, at build time. + expect(rejectsKey('F')).toBe(false); + await filter.selectOperator('Equals'); await filter.setText('0xFF', 0); await asyncSetTimeout(0); @@ -154,20 +194,129 @@ describe('BigInt Filter — custom bigintParser', () => { expect(filter.getModel()).toEqual({ filterType: 'bigint', type: 'equals', filter: '255' }); await new FilterDom(api, 'hex after allowedCharPattern swap', { mode: 'column-filter', colId: 'val' }) .checkFilterDom(` - COLUMN FILTER - operator: "Equals" - input: "0xFF" - AND - operator: "Equals" - input: "" ⟨Filter...⟩ - model: - filterType: "bigint" - type: "equals" - filter: "255" - `); + COLUMN FILTER + operator: "Equals" + input: "0xFF" + AND + operator: "Equals" + input: "" ⟨Filter...⟩ + model: + filterType: "bigint" + type: "equals" + filter: "255" + `); await new GridRows(api, 'hex accepted after the pattern was replaced').check(` ROOT id:ROOT_NODE_ID └── LEAF id:0 val:"255n" `); }); + + // The first reads back through `bigintParser`; the second is grouped, so nothing reads its own output back. + const FORMATTER_CASES = [ + [ + 'read back by its parser', + { + debounceMs: 0, + allowedCharPattern: '\\dxXa-fA-F', + bigintParser: (text: string | null) => (text == null || text.trim() === '' ? null : BigInt(text)), + bigintFormatter: (value: bigint | null) => (value == null ? null : `0x${value.toString(16)}`), + }, + '255', + '0xff', + ], + [ + 'nothing reads back', + { + debounceMs: 0, + bigintFormatter: (value: bigint | null) => (value == null ? null : value.toLocaleString('en-US')), + }, + '1234', + '1,234', + ], + ] as const; + + const BIGINT_COLUMN = { field: 'val', cellDataType: 'bigint' as const, filter: 'agBigIntColumnFilter' as const }; + const BIGINT_ROWS = [{ val: 255n }, { val: 1234n }, { val: 16n }]; + + test.each(FORMATTER_CASES)( + 'a bigintFormatter %s shows in the filter inputs, and the model it came from stands', + async (_name, filterParams, filter, shown) => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ ...BIGINT_COLUMN, filterParams }], + rowData: BIGINT_ROWS, + }); + await api.setColumnFilterModel('val', { filterType: 'bigint', type: 'equals', filter }); + + const columnFilter = await ColumnFilterHarness.open(api, 'val'); + expect(columnFilter.inputs('text', 0)[0].value).toBe(shown); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'bigint', type: 'equals', filter }); + } + ); + + test.each(FORMATTER_CASES)( + 'a bigintFormatter %s shows in the floating filter too', + async (_name, filterParams, filter, shown) => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ ...BIGINT_COLUMN, floatingFilter: true, filterParams }], + rowData: BIGINT_ROWS, + }); + await api.setColumnFilterModel('val', { filterType: 'bigint', type: 'equals', filter }); + await api.onFilterChanged(); + + expect(FloatingFilterHarness.get(api, 'val').input().value).toBe(shown); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'bigint', type: 'equals', filter }); + } + ); + + test('a bigintFormatter replaced at runtime re-renders the inputs it already wrote', async () => { + const columnDefs = (suffix: string) => [ + { + field: 'val', + cellDataType: 'bigint' as const, + filter: 'agBigIntColumnFilter' as const, + filterParams: { + debounceMs: 0, + bigintFormatter: (value: bigint | null) => (value == null ? null : `${value}${suffix}`), + }, + }, + ]; + + const api: GridApi = await gridsManager.createGridAndWait('grid9', { + columnDefs: columnDefs(' old'), + rowData: [{ val: 1234n }, { val: 16n }], + }); + await api.setColumnFilterModel('val', { filterType: 'bigint', type: 'equals', filter: '1234' }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + expect(filter.inputs('text', 0)[0].value).toBe('1234 old'); + + api.setGridOption('columnDefs', columnDefs(' new')); + await asyncSetTimeout(0); + + expect(filter.inputs('text', 0)[0].value).toBe('1234 new'); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'bigint', type: 'equals', filter: '1234' }); + }); + + test('a floating filter rebuilt for a new allowedCharPattern carries what was being typed', async () => { + const columnDefs = (allowedCharPattern: string) => [ + { + field: 'val', + cellDataType: 'bigint' as const, + filter: 'agBigIntColumnFilter' as const, + floatingFilter: true, + // An apply button holds the typed value in the input, which is what the rebuild has to carry. + filterParams: { buttons: ['apply' as const], allowedCharPattern }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid4', { + columnDefs: columnDefs('\\d'), + rowData: [{ val: 255n }, { val: 16n }], + }); + + await FloatingFilterHarness.get(api, 'val').setValue('16'); + api.setGridOption('columnDefs', columnDefs('\\dxXa-fA-F')); + await asyncSetTimeout(0); + + expect(FloatingFilterHarness.get(api, 'val').input().value).toBe('16'); + }); }); diff --git a/testing/behavioural/src/filters/filter-behaviour/number-filter-conditions.test.ts b/testing/behavioural/src/filters/filter-behaviour/number-filter-conditions.test.ts index 71f90508bf2..d9a955a8e1f 100644 --- a/testing/behavioural/src/filters/filter-behaviour/number-filter-conditions.test.ts +++ b/testing/behavioural/src/filters/filter-behaviour/number-filter-conditions.test.ts @@ -15,7 +15,8 @@ import { ClientSideRowModelModule, NumberFilterModule, enableDevValidations, set /** * Black-box coverage for `agNumberColumnFilter` conditions: operators, inRange boundary semantics, - * blank handling, `allowedCharPattern`/`numberParser`, AND/OR compounds, model round-trip. + * blank handling, `allowedCharPattern`/`numberParser`/`numberFormatter`/`filterInputType`, which element + * type an input is built as and how a `colDef` refresh replaces it, AND/OR compounds, model round-trip. * Complements number-filter-range-validation.test.ts (validation-focused) — no overlap. */ describe('Number Filter — conditions coverage', () => { @@ -482,6 +483,296 @@ describe('Number Filter — conditions coverage', () => { `); }); + test('a numberFormatter no parser reads back still shows, and the model it came from stands', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + // Groups the thousands, so its own output does not read back as the number it came from. + numberFormatter: (value: number | null) => + value == null ? null : value.toLocaleString('en-US'), + }, + }, + ], + rowData: [{ val: 1234 }, { val: 5 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 1234 }); + await api.onFilterChanged(); + + const filter = await ColumnFilterHarness.open(api, 'val'); + // `1,234` would come back as 1, so it is shown but never read back as the value behind it. + expect(filter.inputs('text', 0)[0].value).toBe('1,234'); + + await filter.selectOperator('Does not equal'); + await asyncSetTimeout(0); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'number', type: 'notEqual', filter: 1234 }); + await new GridRows(api, 'the value survives an operator change').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:1 val:5 + `); + }); + + test('a formatted inRange pair is validated on the values it was rendered with', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + // Grouped, so re-reading the rendered text would make `1,000` the number 1. + numberFormatter: (value: number | null) => + value == null ? null : value.toLocaleString('en-US'), + }, + }, + ], + rowData: [{ val: 1 }, { val: 500 }, { val: 3000 }], + }); + await api.setColumnFilterModel('val', { + filterType: 'number', + type: 'inRange', + filter: 2, + filterTo: 1000, + }); + await api.onFilterChanged(); + + const filter = await ColumnFilterHarness.open(api, 'val'); + const inputs = filter.inputs('text', 0); + expect([inputs[0].value, inputs[1].value]).toEqual(['2', '1,000']); + // Re-read as text the bounds would be 2 and 1, and the pair would be reported out of order. + expect(inputs[1].validationMessage).toBe(''); + await new GridRows(api, 'formatted inRange bounds').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:1 val:500 + `); + }); + + test('a formatter change re-renders the second condition too', async () => { + const columnDefs = (suffix: string) => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + filterParams: { + debounceMs: 0, + numberFormatter: (value: number | null) => (value == null ? null : `${value}${suffix}`), + }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: columnDefs(' old'), + rowData: [{ val: 5 }, { val: 7 }], + }); + await api.setColumnFilterModel('val', { + filterType: 'number', + operator: 'OR', + conditions: [ + { filterType: 'number', type: 'equals', filter: 5 }, + { filterType: 'number', type: 'equals', filter: 7 }, + ], + }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + expect([filter.inputs('text', 0)[0].value, filter.inputs('text', 1)[0].value]).toEqual(['5 old', '7 old']); + + api.setGridOption('columnDefs', columnDefs(' new')); + await asyncSetTimeout(0); + + // Every mounted position is shown again, not only the first. + expect([filter.inputs('text', 0)[0].value, filter.inputs('text', 1)[0].value]).toEqual(['5 new', '7 new']); + await new GridRows(api, 'both conditions survive the formatter change').check(` + ROOT id:ROOT_NODE_ID + ├── LEAF id:0 val:5 + └── LEAF id:1 val:7 + `); + }); + + // `Number` reads every one of these back as 1234, but an `` keeps none of them. + test.each([ + ['padded', (value: number) => ` ${value} `, ' 1234 '], + ['explicitly signed', (value: number) => `+${value}`, '+1234'], + ['hexadecimal', (value: number) => `0x${value.toString(16)}`, '0x4d2'], + ['exponent notation', (value: number) => value.toExponential(3), '1.234e+3'], + ])('%s numberFormatter output is shown as it wrote it', async (_name, format, shown) => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + numberFormatter: (value: number | null) => (value == null ? null : format(value)), + }, + }, + ], + rowData: [{ val: 1234 }, { val: 5 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 1234 }); + + // Found as a text input at all: the formatter alone is what makes it one. + const filter = await ColumnFilterHarness.open(api, 'val'); + expect(filter.inputs('text', 0)[0].value).toBe(shown); + // None of these read back as 1234, and none has to: the model holds what the filter rendered. + expect(filter.getModel()).toEqual({ filterType: 'number', type: 'equals', filter: 1234 }); + }); + + // Neither reads back: `1,234` is not what `parseFloat` makes of it, and `1235` has lost the .56. + // The last row is the formatter on its own, whose reader is the `parseFloat` the others name explicitly. + test.each([ + ['a grouping', undefined, (value: number) => value.toLocaleString('en-US'), true, 1234, '1,234', '2,000', 2], + ['a rounding', '\\d\\-\\.', (value: number) => value.toFixed(0), true, 1234.56, '1235', '2000', 2000], + ['no parser', undefined, (value: number) => value.toLocaleString('en-US'), false, 1000, '1,000', '2,000', 2], + ] as const)( + 'an editable floating filter with %s formatter shows what it wrote, and reads back what is typed', + async (_name, allowedCharPattern, format, withParser, filter, expected, typed, typedFilter) => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + floatingFilter: true, + filterParams: { + debounceMs: 0, + allowedCharPattern, + numberFormatter: (value: number | null) => (value == null ? null : format(value)), + numberParser: withParser + ? (text: string | null) => (text == null || text === '' ? null : parseFloat(text)) + : undefined, + }, + }, + ], + rowData: [{ val: filter }, { val: 5 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter }); + await api.onFilterChanged(); + await asyncSetTimeout(0); + + const floating = FloatingFilterHarness.get(api, 'val'); + expect(floating.input().value).toBe(expected); + // The text is the formatter's, so reading it back would replace the model it was rendered from. + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'number', type: 'equals', filter }); + + await floating.setValue(typed); + await asyncSetTimeout(0); + + expect(api.getColumnFilterModel('val')).toEqual({ + filterType: 'number', + type: 'equals', + filter: typedFilter, + }); + } + ); + + // The element type is derived from the other two parameters unless it is named outright. + test.each([ + [ + 'a formatter alone takes a text input', + { numberFormatter: (v: number | null) => (v == null ? null : v.toFixed(2)) }, + 'text', + '5.00', + ], + [ + 'which `number` overrides, for a formatter it can hold', + { + numberFormatter: (v: number | null) => (v == null ? null : v.toFixed(2)), + filterInputType: 'number' as const, + }, + 'number', + '5.00', + ], + ['and `text` takes one with neither parameter set', { filterInputType: 'text' as const }, 'text', '5'], + ])('%s', async (_name, extraParams, expectedType, expectedValue) => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { field: 'val', filter: 'agNumberColumnFilter', filterParams: { debounceMs: 0, ...extraParams } }, + ], + rowData: [{ val: 5 }, { val: 7 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 5 }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + const input = filter.inputs(expectedType as 'text' | 'number', 0)[0]; + expect(input.value).toBe(expectedValue); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'number', type: 'equals', filter: 5 }); + }); + + // Two construction sites: the filter builds the `number` input, the shared base builds the `text` one, + // and a replacement is built from params rather than copied from the element it replaces. + test('an input carries browserAutoComplete, and so does the replacement a colDef change builds', async () => { + const columnDefs = (allowedCharPattern?: string) => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + filterParams: { debounceMs: 0, browserAutoComplete: 'one-time-code', allowedCharPattern }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: columnDefs(), + rowData: [{ val: 5 }, { val: 7 }], + }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + expect(filter.input('number', 0).getAttribute('autocomplete')).toBe('one-time-code'); + + api.setGridOption('columnDefs', columnDefs('\\d\\-')); + await asyncSetTimeout(0); + + expect(filter.input('text', 0).getAttribute('autocomplete')).toBe('one-time-code'); + }); + + // The pattern narrows what can be typed, which a `number` input enforces as readily as a text one. + test('an allowedCharPattern applies to a `number` input too', async () => { + const filterParams = { debounceMs: 0, filterInputType: 'number' as const, allowedCharPattern: '\\d' }; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { field: 'val', filter: 'agNumberColumnFilter', filterParams }, + { field: 'floating', filter: 'agNumberColumnFilter', floatingFilter: true, filterParams }, + ], + rowData: [{ val: 5, floating: 5 }], + }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + const inputs = [filter.input('number', 0), FloatingFilterHarness.get(api, 'floating').input()]; + for (const input of inputs) { + expect(input.type).toBe('number'); + expect(input.dispatchEvent(new KeyboardEvent('keydown', { key: 'e', cancelable: true }))).toBe(false); + expect(input.dispatchEvent(new KeyboardEvent('keydown', { key: '5', cancelable: true }))).toBe(true); + } + }); + + // Handlers re-apply the model after a `colDef` change, and a value held by an apply button is not in it. + test.each([ + [false, '5'], + [true, ''], + ])( + 'a floating filter rebuilt for a new allowedCharPattern carries what was being typed (handlers: %s)', + async (enableFilterHandlers, expected) => { + const columnDefs = (allowedCharPattern?: string) => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + floatingFilter: true, + // An apply button holds the typed value in the input, which is what the rebuild has to carry. + filterParams: { buttons: ['apply' as const], allowedCharPattern }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + enableFilterHandlers, + columnDefs: columnDefs(), + rowData: [{ val: 5 }, { val: 7 }], + }); + + await FloatingFilterHarness.get(api, 'val').setValue('5'); + // The pattern decides the element type, so the input is rebuilt rather than reconfigured. + api.setGridOption('columnDefs', columnDefs('\\d\\-')); + await asyncSetTimeout(0); + + expect(FloatingFilterHarness.get(api, 'val').input().value).toBe(expected); + } + ); + test('a read-only floating filter keeps its summary, which is not one value to read back', async () => { const api: GridApi = await gridsManager.createGridAndWait('grid1', { columnDefs: [ @@ -506,6 +797,62 @@ describe('Number Filter — conditions coverage', () => { expect(FloatingFilterHarness.get(api, 'val').input().value).toBe('units: 5'); }); + test('a numberParser reading the formatter is what decides, not `Number`', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + // Grouped with `.`, which a number input holds and only this parser reads back. + numberFormatter: (value: number | null) => + value == null ? null : value.toLocaleString('de-DE'), + numberParser: (text: string | null) => + text == null || text === '' ? null : Number(text.replace(/\./g, '')), + }, + }, + ], + rowData: [{ val: 1234 }, { val: 5 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 1234 }); + + const filter = await ColumnFilterHarness.open(api, 'val'); + expect(filter.inputs('text', 0)[0].value).toBe('1.234'); + expect(filter.getModel()).toEqual({ filterType: 'number', type: 'equals', filter: 1234 }); + }); + + test('a read-only summary follows numberFormatter across a colDef refresh', async () => { + const columnDefs = (suffix: string) => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + floatingFilter: true, + filterParams: { + debounceMs: 0, + readOnly: true, + numberFormatter: (value: number | null) => (value == null ? null : `${value} ${suffix}`), + }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: columnDefs('units'), + rowData: [{ val: 5 }, { val: 7 }], + }); + + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 5 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + expect(FloatingFilterHarness.get(api, 'val').inputs()[0].value).toBe('5 units'); + + // The formatter is read from the params each time, so a refresh that replaces it is picked up. + api.updateGridOptions({ columnDefs: columnDefs('kg') }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 7 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + expect(FloatingFilterHarness.get(api, 'val').inputs()[0].value).toBe('7 kg'); + }); + test('two conditions joined with AND', async () => { const api: GridApi = await gridsManager.createGridAndWait('grid1', { columnDefs: [{ field: 'val', filter: 'agNumberColumnFilter', filterParams: { debounceMs: 0 } }], @@ -784,16 +1131,21 @@ describe('Number Filter — conditions coverage', () => { `); }); - test('a numberFormatter added at runtime reaches inputs built before it', async () => { + test('a numberFormatter added at runtime reaches inputs built before it, and survives being withdrawn', async () => { const numberParser = (text: string | null) => text == null || text === '' ? null : Number(String(text).replace(/,/g, '')); const numberFormatter = (value: number | null) => (value == null ? null : value.toLocaleString('en-US')); const allowedCharPattern = '\\d\\,\\.\\-'; + const plainColumnDefs = [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + filterParams: { debounceMs: 0, allowedCharPattern }, + }, + ]; const api: GridApi = await gridsManager.createGridAndWait('grid1', { - columnDefs: [ - { field: 'val', filter: 'agNumberColumnFilter', filterParams: { debounceMs: 0, allowedCharPattern } }, - ], + columnDefs: plainColumnDefs, rowData: [{ val: 5 }, { val: 1500 }], }); const filter = await ColumnFilterHarness.open(api, 'val'); @@ -812,6 +1164,53 @@ describe('Number Filter — conditions coverage', () => { await asyncSetTimeout(0); expect(filter.inputs('text', 0)[0].value).toBe('1,500'); + + // Nothing can read "1,500" back once the parser goes, so the value is re-rendered, not copied across. + api.setGridOption('columnDefs', plainColumnDefs); + await asyncSetTimeout(0); + + expect(filter.inputs('text', 0)[0].value).toBe('1500'); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'number', type: 'equals', filter: 1500 }); + }); + + test('a formatter replaced by an equivalent one leaves the value it wrote intact', async () => { + // No numberParser, so nothing can read "1,234" back: only what the input was rendered with can. + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + numberFormatter: (value: number | null) => + value == null ? null : value.toLocaleString('en-US'), + }, + }, + ], + rowData: [{ val: 5 }, { val: 1234 }], + }); + const filter = await ColumnFilterHarness.open(api, 'val'); + + await api.setColumnFilterModel('val', { filterType: 'number', type: 'greaterThan', filter: 1234 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + expect(filter.inputs('text', 0)[0].value).toBe('1,234'); + + // A new formatter of the same behaviour, which is what an inline arrow gives on every render. + api.setGridOption('columnDefs', [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + numberFormatter: (value: number | null) => (value == null ? null : value.toLocaleString('en-US')), + }, + }, + ]); + await asyncSetTimeout(0); + + expect(filter.inputs('text', 0)[0].value).toBe('1,234'); + expect(api.getColumnFilterModel('val')).toEqual({ filterType: 'number', type: 'greaterThan', filter: 1234 }); }); test('a colDef refresh keeps text the parser cannot read yet', async () => { @@ -845,4 +1244,158 @@ describe('Number Filter — conditions coverage', () => { await asyncSetTimeout(0); expect(filter.getModel()).toEqual({ filterType: 'number', type: 'equals', filter: -5 }); }); + + test('an input replaced by a colDef refresh still applies what is typed into it', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ field: 'val', filter: 'agNumberColumnFilter', filterParams: { debounceMs: 0 } }], + rowData: [{ val: 5 }, { val: 1500 }], + }); + const filter = await ColumnFilterHarness.open(api, 'val'); + await filter.selectOperator('Equals'); + expect(filter.inputs('number', 0)).toHaveLength(1); + + // `allowedCharPattern` makes the inputs text, so the ones typed into here are not the ones built. + api.setGridOption('columnDefs', [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { debounceMs: 0, allowedCharPattern: '\\d\\-\\.' }, + }, + ]); + await asyncSetTimeout(0); + + await filter.setText('1500'); + expect(filter.getModel()).toEqual({ filterType: 'number', type: 'equals', filter: 1500 }); + await new GridRows(api, 'typed into the replacement input').check(` + ROOT id:ROOT_NODE_ID + └── LEAF id:1 val:1500 + `); + }); + + test('a colDef refresh leaves text being typed alone rather than reformatting it', async () => { + // Declared inline, as a framework wrapper does, so every refresh passes new function identities. + const makeColumnDefs = () => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + filterParams: { + debounceMs: 0, + allowedCharPattern: '\\d\\-\\.', + numberFormatter: (value: number | null) => (value == null ? null : value.toFixed(0)), + numberParser: (text: string | null) => (text == null || text === '' ? null : parseFloat(text)), + }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: makeColumnDefs(), + rowData: [{ val: 5 }, { val: 1234.5 }], + }); + const filter = await ColumnFilterHarness.open(api, 'val'); + await filter.setText('1234.5'); + + api.setGridOption('columnDefs', makeColumnDefs()); + await asyncSetTimeout(0); + + // The user is still typing 1234.5; the formatter must not round it under the caret. + expect(filter.inputs('text', 0)[0].value).toBe('1234.5'); + }); + + test('a focused text input replaced by number inputs leaves the pair coherent', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + numberFormatter: (value: number | null) => (value == null ? null : value.toFixed(0)), + }, + }, + ], + rowData: [{ val: 5 }, { val: 1500 }], + }); + await api.setColumnFilterModel('val', { + filterType: 'number', + type: 'inRange', + filter: 1500, + filterTo: 2000, + }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + const filter = await ColumnFilterHarness.open(api, 'val'); + const eText = filter.inputs('text', 0)[0]; + eText.focus(); + eText.setSelectionRange(2, 2); + + // Withdrawing the formatter takes the text inputs with it, and a `number` input holds no selection. + api.setGridOption('columnDefs', [ + { field: 'val', filter: 'agNumberColumnFilter', filterParams: { debounceMs: 0 } }, + ]); + await asyncSetTimeout(0); + + // Both inputs are replaced, or the condition is left holding one of each type. + expect(filter.inputs('number', 0).map((input) => input.value)).toEqual(['1500', '2000']); + expect(filter.inputs('text', 0)).toHaveLength(0); + expect(document.activeElement).toBe(filter.inputs('number', 0)[0]); + }); + + test('an input a formatter rendered as empty stands for no value', async () => { + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: [ + { + field: 'val', + filter: 'agNumberColumnFilter', + filterParams: { + debounceMs: 0, + // Renders zero as nothing, so the input shows no value while the model holds one. + numberFormatter: (value: number | null) => (value === 0 ? '' : String(value)), + }, + }, + ], + rowData: [{ val: 0 }, { val: 5 }], + }); + await api.setColumnFilterModel('val', { filterType: 'number', type: 'equals', filter: 0 }); + api.onFilterChanged(); + await asyncSetTimeout(0); + + const filter = await ColumnFilterHarness.open(api, 'val'); + expect(filter.inputs('text', 0)[0].value).toBe(''); + + // An empty input is read as empty, so the condition it belongs to is no longer complete. + await filter.selectOperator('Does not equal'); + await asyncSetTimeout(0); + expect(api.getColumnFilterModel('val')).toBe(null); + }); + + test('a rebuild puts the caret back where the user left it', async () => { + const columnDefs = (allowedCharPattern: string) => [ + { + field: 'val', + filter: 'agNumberColumnFilter' as const, + filterParams: { debounceMs: 0, allowedCharPattern }, + }, + ]; + const api: GridApi = await gridsManager.createGridAndWait('grid1', { + columnDefs: columnDefs('\\d\\-\\.'), + rowData: [{ val: 5 }, { val: 1234 }], + }); + const filter = await ColumnFilterHarness.open(api, 'val'); + await filter.selectOperator('Equals'); + await filter.setText('1234'); + const before = filter.inputs('text', 0)[0]; + before.focus(); + before.setSelectionRange(2, 2); + + api.setGridOption('columnDefs', columnDefs('\\d\\-\\.,')); + await asyncSetTimeout(0); + + const after = filter.inputs('text', 0)[0]; + expect(after).not.toBe(before); + expect(after.value).toBe('1234'); + expect(document.activeElement).toBe(after); + expect([after.selectionStart, after.selectionEnd]).toEqual([2, 2]); + // Equals takes one input, so the replacements must not arrive showing the unused second one. + expect(filter.inputs('text', 0)).toHaveLength(1); + }); }); diff --git a/testing/behavioural/src/filters/floating-filters.test.ts b/testing/behavioural/src/filters/floating-filters.test.ts index 4aa58bb0fa8..1d53e9a48f1 100644 --- a/testing/behavioural/src/filters/floating-filters.test.ts +++ b/testing/behavioural/src/filters/floating-filters.test.ts @@ -5,6 +5,7 @@ import { ClientSideRowModelModule, DateFilterModule, NumberFilterModule, + RenderApiModule, TextFilterModule, agTestIdFor, getGridElement, @@ -20,7 +21,7 @@ function typeIntoFloatingFilter(input: HTMLInputElement, value: string): void { describe('Floating Filters', () => { const gridsManager = new TestGridsManager({ - modules: [ClientSideRowModelModule, TextFilterModule, NumberFilterModule, DateFilterModule], + modules: [ClientSideRowModelModule, TextFilterModule, NumberFilterModule, DateFilterModule, RenderApiModule], }); beforeAll(() => setupAgTestIds()); @@ -352,4 +353,84 @@ describe('Floating Filters', () => { expect(api.getDisplayedRowCount()).toBe(3); }); }); + + // `refreshHeader` rebuilds the header rows, so a floating filter is a new component rather than an + // existing one shown again: text still inside the debounce window belongs to the component that held it. + test('refreshHeader discards floating filter text that has not reached the model', async () => { + const api = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ field: 'country', filter: 'agTextColumnFilter', filterParams: { debounceMs: 10_000 } }], + defaultColDef: { floatingFilter: true }, + rowData: [{ country: 'Ireland' }, { country: 'Italy' }], + }); + await asyncSetTimeout(0); + + const gridDiv = getGridElement(api)! as HTMLElement; + const inputId = agTestIdFor.textFilterInstanceInput({ source: 'floating-filter', colId: 'country' }); + typeIntoFloatingFilter(getByTestId(gridDiv, inputId) as HTMLInputElement, 'Ire'); + + // Held by the input alone: the debounce has not run, so the model has nothing to rebuild it from. + expect((getByTestId(gridDiv, inputId) as HTMLInputElement).value).toBe('Ire'); + expect(api.getColumnFilterModel('country')).toBe(null); + + api.refreshHeader(); + + // The rebuilt input is a different element, so poll for it rather than reading the destroyed one. + await waitFor(() => expect((getByTestId(gridDiv, inputId) as HTMLInputElement).value).toBe('')); + expect(api.getColumnFilterModel('country')).toBe(null); + }); + + // The rebuilt input re-reads the model, so the text returns — the caret into it does not, being a + // property of the element that held it. + test('refreshHeader re-reads a committed value but keeps none of the caret into it', async () => { + const api = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ field: 'country', filter: 'agTextColumnFilter', filterParams: { debounceMs: 0 } }], + defaultColDef: { floatingFilter: true }, + rowData: [{ country: 'Ireland' }, { country: 'Italy' }], + }); + await asyncSetTimeout(0); + + const gridDiv = getGridElement(api)! as HTMLElement; + const inputId = agTestIdFor.textFilterInstanceInput({ source: 'floating-filter', colId: 'country' }); + const input = getByTestId(gridDiv, inputId) as HTMLInputElement; + typeIntoFloatingFilter(input, 'Ireland'); + await asyncSetTimeout(0); + + expect(api.getColumnFilterModel('country')).toEqual({ + filterType: 'text', + type: 'contains', + filter: 'Ireland', + }); + input.focus(); + input.setSelectionRange(3, 3); + expect(input.selectionStart).toBe(3); // the caret is real here, so its absence below means something + + api.refreshHeader(); + + const rebuilt = () => getByTestId(gridDiv, inputId) as HTMLInputElement; + await waitFor(() => expect(rebuilt().value).toBe('Ireland')); + expect(rebuilt().selectionStart).toBe('Ireland'.length); + }); + + // Focus is restored to the header position, not to the element that held it: the input is a new node, + // so what the user was typing into is gone even though the grid still considers the header focused. + test('refreshHeader moves focus from the floating filter input out to its header cell', async () => { + const api = await gridsManager.createGridAndWait('grid1', { + columnDefs: [{ field: 'country', filter: 'agTextColumnFilter', filterParams: { debounceMs: 0 } }], + defaultColDef: { floatingFilter: true }, + rowData: [{ country: 'Ireland' }, { country: 'Italy' }], + }); + await asyncSetTimeout(0); + + const gridDiv = getGridElement(api)! as HTMLElement; + const inputId = agTestIdFor.textFilterInstanceInput({ source: 'floating-filter', colId: 'country' }); + const input = getByTestId(gridDiv, inputId) as HTMLInputElement; + input.focus(); + expect(document.activeElement).toBe(input); + + api.refreshHeader(); + + const rebuilt = () => getByTestId(gridDiv, inputId) as HTMLInputElement; + await waitFor(() => expect(rebuilt()).not.toBe(input)); + expect(document.activeElement).toBe(rebuilt().closest('.ag-header-cell')); + }); }); diff --git a/testing/behavioural/src/filters/number-filter-range-validation.test.ts b/testing/behavioural/src/filters/number-filter-range-validation.test.ts index 7a0bfe024de..4785335856b 100644 --- a/testing/behavioural/src/filters/number-filter-range-validation.test.ts +++ b/testing/behavioural/src/filters/number-filter-range-validation.test.ts @@ -407,4 +407,34 @@ describe('Number Range Filter', () => { expect(filter.input('number', 0).value).toBe('9'); expect(filter.input('number', 1).value).toBe('1'); }); + + test('an out-of-order range keeps the next condition disabled across a rebuild', async () => { + const columnDefs = (allowedCharPattern: string) => [ + { + field: 'gold', + filter: 'agNumberColumnFilter' as const, + filterParams: { debounceMs: 0, filterOptions: ['inRange'], allowedCharPattern }, + }, + ]; + + const api = await gridsManager.createGridAndWait('grid1', { + columnDefs: columnDefs('\\d\\-\\.'), + rowData: [{ gold: 2 }, { gold: 8 }], + }); + + const filter = await ColumnFilterHarness.open(api, 'gold'); + await filter.setText('9', 0); + await filter.setText('1', 1); + expect(filter.input('text', 1).validity.valid).toBe(false); + // An invalid pair is not a complete condition, so no second one is offered. + expect(filter.inputs('text')).toHaveLength(2); + + // The pattern decides the element, so every input is replaced — carrying none of the validity + // that decides whether the condition counts as complete. + api.setGridOption('columnDefs', columnDefs('\\d\\-\\.,')); + await asyncSetTimeout(0); + + expect(filter.input('text', 1).validity.valid).toBe(false); + expect(filter.inputs('text')).toHaveLength(2); + }); }); From 8afcf2e4eeee59fb23ed589e19bc5d513f994f06 Mon Sep 17 00:00:00 2001 From: Tak Tran Date: Thu, 20 Aug 2026 12:33:56 +0100 Subject: [PATCH 6/8] AG-18194 - Inject the Enzuzo policy loader after the inline-script strip (#14915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vendor policy HTML carries three inline ` + + ''; +} + +function runEmbedScript(): void { + vm.runInThisContext(EMBED_SCRIPT); +} + +/** Stands in for the vendor loader's own insertion, which uses the same API. */ +function injectPolicy(html: string): void { + const range = document.createRange(); + range.setStart(document.body, 0); + document + .querySelector(`[${'data-policy-loader-src'}]`) + ?.parentNode?.appendChild(range.createContextualFragment(html)); +} + +const POLICY_HTML = + '

Cookie Policy

' + + '' + + '

How was this cookie policy generated?

' + + '

Generated by Enzuzo.

' + + '

Kept section

Body copy.

'; + +describe('Enzuzo cookie-policy embed', () => { + const pristineCreateContextualFragment = Range.prototype.createContextualFragment; + + beforeEach(() => { + Range.prototype.createContextualFragment = pristineCreateContextualFragment; + renderCookiesPage(); + }); + + it('injects the vendor loader as a sibling of its own tag, inside the content wrapper', () => { + runEmbedScript(); + + const loader = document.querySelector( + '.layout-max-width-small > script#__enzuzo-root-script' + ); + expect(loader).not.toBeNull(); + expect(loader?.src).toBe(LOADER_SRC); + }); + + // The regression itself: whatever else changes, the patch has to be in place by the time the + // loader element is created, because creating it is what starts the fetch that ends in an + // insertion. + it('installs the fragment patch before creating the loader element', () => { + const realCreateElement = document.createElement.bind(document); + let patchedWhenLoaderCreated: boolean | undefined; + + vi.spyOn(document, 'createElement').mockImplementation((tagName: string) => { + if (tagName === 'script') { + patchedWhenLoaderCreated = + Range.prototype.createContextualFragment !== pristineCreateContextualFragment; + } + return realCreateElement(tagName); + }); + + runEmbedScript(); + + expect(patchedWhenLoaderCreated).toBe(true); + vi.restoreAllMocks(); + }); + + it('strips the vendor inline scripts out of the injected policy', async () => { + runEmbedScript(); + injectPolicy(POLICY_HTML); + + expect(document.querySelectorAll('[ez-policy] script')).toHaveLength(0); + await vi.waitFor(() => expect(document.querySelector('[ez-policy]')).not.toBeNull()); + }); + + it('restores createContextualFragment once the policy has landed', async () => { + runEmbedScript(); + expect(Range.prototype.createContextualFragment).not.toBe(pristineCreateContextualFragment); + + injectPolicy(POLICY_HTML); + + await vi.waitFor(() => expect(Range.prototype.createContextualFragment).toBe(pristineCreateContextualFragment)); + }); + + it('removes the vendor self-promotional section and keeps the rest of the policy', async () => { + runEmbedScript(); + injectPolicy(POLICY_HTML); + + await vi.waitFor(() => { + const headings = [...document.querySelectorAll('[ez-policy] h1, [ez-policy] h2')].map( + (heading) => heading.textContent + ); + expect(headings).toEqual(['Cookie Policy', 'Kept section']); + }); + expect(document.querySelector('[ez-policy] a[href*="enzuzo.com"]')).toBeNull(); + }); +}); diff --git a/external/ag-website-shared/src/components/policies/pages/cookies.astro b/external/ag-website-shared/src/components/policies/pages/cookies.astro index e7735e02342..3c822ee37ce 100644 --- a/external/ag-website-shared/src/components/policies/pages/cookies.astro +++ b/external/ag-website-shared/src/components/policies/pages/cookies.astro @@ -32,29 +32,35 @@ const content = POLICY_CONTENT.cookies; /* The cookie policy is generated by Enzuzo, our consent-management platform, from an automated scan of the site, rather than maintained by hand here (AG-18194). - The loader inserts the policy as a *sibling after its own - - { - /* Strips the vendor's self-promotional section from the injected policy; see - public/scripts/enzuzo-policy-tidy.js. Externalised to a 'self' script so the site CSP - can keep script-src free of 'unsafe-inline' without needing a per-build hash, and - re-run alongside the loader above so it observes the re-injected policy too. */ - } - + data-policy-loader-src={`https://app.enzuzo.com/scripts/cookies/${enzuzoPolicyId}`} + src={urlWithBaseUrl('/scripts/enzuzo-policy-embed.js')}> From 561ef2c0fe14304a1d1540a446da2349af530cfd Mon Sep 17 00:00:00 2001 From: Saba Ahang Date: Thu, 20 Aug 2026 14:26:49 +0100 Subject: [PATCH 7/8] github-triage: self-dispatch the auto-trigger so the triage run carries a bot actor (#14907) https://ag-grid.atlassian.net/browse/AG-17369 --- .github/workflows/github-triage-pipeline.yml | 95 +++++++++++++------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/.github/workflows/github-triage-pipeline.yml b/.github/workflows/github-triage-pipeline.yml index dc3853eedb3..6e05c3b139f 100644 --- a/.github/workflows/github-triage-pipeline.yml +++ b/.github/workflows/github-triage-pipeline.yml @@ -14,7 +14,9 @@ name: GitHub issue AI triage # ag-dev-prompts → docs/github-triage-pipeline.md # # Triggers: -# issues (opened|labeled) → triage (the GitHub-native entry point) +# issues (opened|labeled) → the `redispatch` job ONLY, which re-fires this +# workflow as a workflow_dispatch so the triage run carries a bot actor. The agent +# is never run on the `issues` event itself — see the `redispatch` job. # repository_dispatch gh-triage-resume → preflight routes execute | resume # (from the single AITGH `→ In Progress` JIRA automation rule: # POST /repos/ag-grid/ag-grid/dispatches @@ -103,6 +105,44 @@ env: DEV_PROMPTS_CHANNEL: ${{ github.event.client_payload.channel || inputs.channel || 'latest' }} jobs: + # -------------------------------------------------- redispatch + # The `issues` event does not run the triage itself: it re-fires this workflow as a + # `workflow_dispatch`, and that run carries a bot actor which the shared action + # allow-lists. Running the agent directly on the `issues` event does not work — + # do not collapse this back into a single run. + # + # Rationale and the required rollout order live in the private ag-dev-prompts repo, + # docs/github-triage-pipeline.md § "Why the auto-trigger self-dispatches". + # Mirrors the arrangement release-review.yml already uses. + # + # Deliberately not `|| true`: a failed dispatch means the issue is never triaged, so + # this job must go red rather than swallow it. + redispatch: + if: | + github.event_name == 'issues' && + contains(github.event.issue.labels.*.name, 'triage') && + github.event.issue.state == 'open' && + (github.event.action == 'opened' || github.event.label.name == 'triage') + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + # Needed for `gh workflow run`. The job has no checkout and no agent, and the + # only issue-derived value it uses is the issue number. + actions: write + steps: + - name: Re-fire as workflow_dispatch (bot actor) + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_ISSUE: ${{ github.event.issue.number }} + run: | + set -euo pipefail + echo "issue #${GH_ISSUE} is triage-eligible — re-firing as workflow_dispatch so the run carries a bot actor" + gh workflow run github-triage-pipeline.yml \ + --repo "${{ github.repository }}" \ + -f stage=triage \ + -f gh_issue="${GH_ISSUE}" \ + -f channel=latest + # -------------------------------------------------- resolve-channel # Which @ag-grid/dev-prompts dist-tag should this dispatch run? # @@ -224,18 +264,21 @@ jobs: # -------------------------------------------------- run # The parameterised stage runner (triage / resume / execute / browser-verify / - # repro-rebuild). Runs on the issue event (triage), a forced dispatch, a - # non-empty preflight route, or one of the three manual-trigger button - # events (which — unlike gh-triage-resume — always resolve to ONE specific - # stage directly, no preflight/routing involved: the PM's button click IS - # the unambiguous intent signal). + # repro-rebuild). Runs on a dispatch (forced, or the auto-triage self-dispatch + # from `redispatch` above), a non-empty preflight route, or one of the three + # manual-trigger button events (which — unlike gh-triage-resume — always resolve + # to ONE specific stage directly, no preflight/routing involved: the PM's button + # click IS the unambiguous intent signal). + # + # ⚠️ There is deliberately NO `issues` branch here — the auto-triage path arrives as + # the `workflow_dispatch` that `redispatch` fires. Do not add one back; see the + # `redispatch` job above. run: needs: [preflight, resolve-channel] if: | !cancelled() && ( - (github.event_name == 'issues' && contains(github.event.issue.labels.*.name, 'triage') && github.event.issue.state == 'open' && (github.event.action == 'opened' || github.event.label.name == 'triage')) - || github.event_name == 'workflow_dispatch' + github.event_name == 'workflow_dispatch' || (github.event_name == 'repository_dispatch' && needs.preflight.outputs.stage != '') || (github.event_name == 'repository_dispatch' && github.event.action == 'gh-triage-manual-resume') || (github.event_name == 'repository_dispatch' && github.event.action == 'gh-triage-manual-browser-verify') @@ -287,23 +330,10 @@ jobs: # regardless of preflight's own behaviour. stage: ${{ (github.event.action == 'gh-triage-manual-resume' && 'resume') || (github.event.action == 'gh-triage-manual-browser-verify' && 'browser-verify') || (github.event.action == 'gh-triage-manual-repro-rebuild' && 'repro-rebuild') || needs.preflight.outputs.stage || inputs.stage || 'triage' }} product: grid - # claude-code-action refuses to run when the TRIGGERING ACTOR lacks - # write access — and on an `issues` event that actor is the ISSUE - # AUTHOR, a community reporter with `read`. Auto-triage therefore - # failed for every genuine community report ("Actor does not have - # write permissions to the repository") and only ever worked when a - # maintainer happened to touch the issue, which is the opposite of - # the feature. It looked healthy because the first live ticket was - # label-triggered by a maintainer; the first real community report - # (AITGH-28 / #14837) failed on both auto-fires. + # NOTE: `allowed_non_write_users` is deliberately NOT passed, and must + # stay unset — setting it re-breaks the auto-triage path. The actor is a + # bot, allow-listed inside the composite action. See `redispatch` above. # - # Scoped to the `issues` event ALONE: the board drag, the manual - # buttons and workflow_dispatch all already carry a write-capable - # actor, so the gate stays armed there at no cost — and stays armed - # for any trigger added later. Setting this also makes - # claude-code-action best-effort scrub secrets from subprocess - # environments. - allowed_non_write_users: ${{ github.event_name == 'issues' && '*' || '' }} # Provenance for the pin: `post` stamps this channel onto the # mapping ticket so `resolve-channel` can route the NEXT drag to the # same channel. Must match the channel actually installed above. @@ -379,13 +409,14 @@ jobs: # (B2) — security posture"). Do not treat this job as hardened. browser-verify-chain: needs: [run] + # The former `github.event_name == 'issues'` disjunct is removed, not just unused: + # auto-triage now arrives as a `workflow_dispatch stage=triage` (see `redispatch`), + # so `run` never executes on an `issues` event. Dead condition branches are not + # left in place here. if: | !cancelled() && needs.run.outputs.confirmed_bug == 'true' && - ( - github.event_name == 'issues' - || (github.event_name == 'workflow_dispatch' && inputs.stage == 'triage') - ) + github.event_name == 'workflow_dispatch' && inputs.stage == 'triage' runs-on: ubuntu-latest permissions: contents: read @@ -500,13 +531,13 @@ jobs: # too, for the same "an unused permission is a mistake" reason as `run`. confidence-refresh-chain: needs: [run, browser-verify-chain, repro-rebuild-chain] + # Same removal of the dead `issues` disjunct as browser-verify-chain above. The + # stage restriction itself is still load-bearing (it stops an ordinary human + # resume auto-firing a duplicate resume) — only the unreachable half is gone. if: | !cancelled() && needs.run.outputs.confidence_gap == 'true' && - ( - github.event_name == 'issues' - || (github.event_name == 'workflow_dispatch' && inputs.stage == 'triage') - ) + github.event_name == 'workflow_dispatch' && inputs.stage == 'triage' runs-on: ubuntu-latest permissions: contents: read From cb04d5ddf96d406c523ea280595a4b7cae56c758 Mon Sep 17 00:00:00 2001 From: Stephen Cooper Date: Thu, 20 Aug 2026 14:47:01 +0100 Subject: [PATCH 8/8] Revert "AG-16759 Fix reverse tab out of a conditionally-editable cell (#14903)" (#14920) This reverts commit 97c51df47b4fdbaaa0c93b7c59a32b6ac0ba3061. --- .../src/navigation/navigationService.ts | 20 +++-- ...ing-conditional-editing-navigation.test.ts | 84 ------------------- .../cell-editing-tab-editor-react.test.tsx | 33 -------- .../suppress-navigable-navigation.test.ts | 25 ------ 4 files changed, 11 insertions(+), 151 deletions(-) delete mode 100644 testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts diff --git a/packages/ag-grid-community/src/navigation/navigationService.ts b/packages/ag-grid-community/src/navigation/navigationService.ts index 0bc5bed2bd5..fbec2ab0771 100644 --- a/packages/ag-grid-community/src/navigation/navigationService.ts +++ b/packages/ag-grid-community/src/navigation/navigationService.ts @@ -419,7 +419,7 @@ export class NavigationService extends BeanStub implements NamedBean { const movedToNextCell = this.tabToNextCellCommon(previous, backwards, keyboardEvent); const beans = this.beans; - const { ctrlsSvc, focusSvc, gos } = beans; + const { ctrlsSvc, pageBounds, focusSvc, gos } = beans; if (movedToNextCell !== false) { // only prevent default if we found a cell. so if user is on last cell and hits tab, then we default @@ -434,15 +434,17 @@ export class NavigationService extends BeanStub implements NamedBean { } // if we didn't move to next cell, then need to tab out of the cells, ie to the header (if going - // backwards). Reached only when the walk found no cell at all, which can happen from any row - - // e.g. a function-valued `editable`, or an entirely suppressNavigable path - so there is - // deliberately no first-row check, mirroring the forwards branch below. + // backwards) if (backwards) { - if (gos.get('headerHeight') === 0 || _isHeaderFocusSuppressed(beans)) { - _focusNextGridCoreContainer(beans, true, 'force'); - } else { - keyboardEvent.preventDefault(); - focusSvc.focusPreviousFromFirstCell(keyboardEvent); + const { rowIndex, rowPinned } = previous.getRowPosition(); + const firstRow = rowPinned ? rowIndex === 0 : rowIndex === pageBounds.getFirstRow(); + if (firstRow) { + if (gos.get('headerHeight') === 0 || _isHeaderFocusSuppressed(beans)) { + _focusNextGridCoreContainer(beans, true, 'force'); + } else { + keyboardEvent.preventDefault(); + focusSvc.focusPreviousFromFirstCell(keyboardEvent); + } } } else { // anchor container navigation on the cell when focus is in an editor or renderer child. diff --git a/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts b/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts deleted file mode 100644 index 2920e4b54f5..00000000000 --- a/testing/behavioural/src/cell-editing/cell-editing-conditional-editing-navigation.test.ts +++ /dev/null @@ -1,84 +0,0 @@ -import { userEvent } from '@testing-library/user-event'; -import { TestGridsManager, waitForInput } from 'ag-test-utils'; - -import type { ColDef, GridApi } from 'ag-grid-community'; -import { - ClientSideRowModelModule, - NumberEditorModule, - TextEditorModule, - getGridElement, - setupAgTestIds, -} from 'ag-grid-community'; - -interface PersonRow { - athlete: string; - age: number; - editable: boolean; -} - -const columnDefs: ColDef[] = [ - { field: 'athlete', editable: (params) => !!params.data?.editable }, - { field: 'age', editable: (params) => !!params.data?.editable }, -]; - -const rowData: PersonRow[] = [ - { athlete: 'Alice', age: 23, editable: false }, - { athlete: 'Bob', age: 40, editable: false }, - { athlete: 'Carol', age: 31, editable: true }, -]; - -describe('Conditional editing reverse tab navigation', () => { - const gridsManager = new TestGridsManager({ - includeDefaultModules: true, - modules: [ClientSideRowModelModule, NumberEditorModule, TextEditorModule], - }); - - beforeAll(() => { - setupAgTestIds(); - }); - - afterEach(() => { - gridsManager.reset(); - vi.clearAllMocks(); - }); - - const cell = (api: GridApi, rowIndex: number, colId: string): HTMLElement => - (getGridElement(api)! as HTMLElement).querySelector( - `[row-index="${rowIndex}"] [col-id="${colId}"]` - )!; - - test('Shift+Tab out of the only editable cell moves focus to the last header cell', async () => { - const user = userEvent.setup(); - const api = await gridsManager.createGridAndWait('ag-16759-a', { columnDefs, rowData }); - - await user.dblClick(cell(api, 2, 'athlete')); - await waitForInput(cell(api, 2, 'athlete')); - - await user.keyboard('{Shift>}{Tab}{/Shift}'); - - const active = document.activeElement as HTMLElement; - expect(active?.classList.contains('ag-header-cell')).toBe(true); - expect(active?.getAttribute('col-id')).toBe('age'); - }); - - test('Repeated Shift+Tab up into the header throws no error', async () => { - const errors: unknown[] = []; - const onError = (e: ErrorEvent) => errors.push(e.error ?? e.message); - window.addEventListener('error', onError); - try { - const user = userEvent.setup(); - const api = await gridsManager.createGridAndWait('ag-16759-b', { columnDefs, rowData }); - - await user.dblClick(cell(api, 2, 'athlete')); - await waitForInput(cell(api, 2, 'athlete')); - - for (let i = 0; i < 8; i++) { - await user.keyboard('{Shift>}{Tab}{/Shift}'); - } - - expect(errors).toHaveLength(0); - } finally { - window.removeEventListener('error', onError); - } - }); -}); diff --git a/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx b/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx index 89982031d9d..7f761115c97 100644 --- a/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx +++ b/testing/behavioural/src/cell-editing/cell-editing-tab-editor-react.test.tsx @@ -554,39 +554,6 @@ describe('Cell Editing: tab into editor in React', () => { }); }); - // With a function-valued `editable`, Shift+Tab out of the only editable cell must tab out to - // the header rather than letting the browser focus the row element. React twin of the vanilla guard in - // cell-editing-conditional-editing-navigation.test.ts. - test('conditional editing: Shift+Tab out of the only editable cell focuses the last header cell', async () => { - const { gridDiv, user } = await renderGrid({ - rowData: [ - { id: '0', athlete: 'Alice', age: 23, editable: false }, - { id: '1', athlete: 'Bob', age: 40, editable: false }, - { id: '2', athlete: 'Carol', age: 31, editable: true }, - ], - columnDefs: [ - { field: 'athlete', editable: (params: any) => !!params.data?.editable }, - { field: 'age', editable: (params: any) => !!params.data?.editable }, - ], - modules: [ClientSideRowModelModule, TextEditorModule, NumberEditorModule], - }); - - const editableCell = getByTestId(gridDiv, agTestIdFor.cell('2', 'athlete')); - await user.dblClick(editableCell); - - await waitFor(() => { - expect(editableCell.querySelector('.ag-cell-edit-wrapper input')).toBeTruthy(); - }); - - await user.keyboard('{Shift>}{Tab}{/Shift}'); - - await waitFor(() => { - const active = document.activeElement as HTMLElement; - expect(active?.classList.contains('ag-header-cell')).toBe(true); - expect(active?.getAttribute('col-id')).toBe('age'); - }); - }); - describe('editType: fullRow', () => { // fullRow: cellStartedEdit is true for the focused cell on initial dblClick test('fullRow: cellStartedEdit is true for the focused cell on initial edit', async () => { diff --git a/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts b/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts index 8c726ce3689..8beeb3e68de 100644 --- a/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts +++ b/testing/behavioural/src/navigation/suppress-navigable-navigation.test.ts @@ -134,29 +134,4 @@ describe('suppressNavigable Navigation', () => { expect(getFocusedRowIndex(api)).toBe(1); }); }); - - describe('backwards tab out with nowhere to go', () => { - // AG-16759: the backwards tab-out is reached whenever the walk finds no cell at all, which - // for a row-dependent suppressNavigable happens from a row that is not the first one. - let api: GridApi; - - beforeEach(() => { - const columnDefs: ColDef[] = [ - { field: 'a', colId: 'a', suppressNavigable: (params) => params.data?.a === 'a0' }, - ]; - api = gridsManager.createGrid('myGrid', { - columnDefs, - rowData, - } as GridOptions); - }); - - test('Shift+Tab from a non-first row moves focus to the header', () => { - api.setFocusedCell(1, 'a'); - dispatchKeyDown(KeyCode.TAB, { shiftKey: true }); - - const active = document.activeElement as HTMLElement | null; - expect(active?.classList.contains('ag-header-cell')).toBe(true); - expect(active?.getAttribute('col-id')).toBe('a'); - }); - }); });