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/.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 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/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/documentation/ag-grid-docs/public/scripts/enzuzo-policy-tidy.js b/documentation/ag-grid-docs/public/scripts/enzuzo-policy-embed.js similarity index 67% rename from documentation/ag-grid-docs/public/scripts/enzuzo-policy-tidy.js rename to documentation/ag-grid-docs/public/scripts/enzuzo-policy-embed.js index 013db358b76..d2076561db7 100644 --- a/documentation/ag-grid-docs/public/scripts/enzuzo-policy-tidy.js +++ b/documentation/ag-grid-docs/public/scripts/enzuzo-policy-embed.js @@ -1,20 +1,28 @@ /* - * Two clean-ups on the Enzuzo cookie-policy embed (AG-18194), both of which have to happen here - * rather than in CSS or in the markup: + * Renders the Enzuzo cookie-policy embed on /cookies/ (AG-18194), doing three things the markup + * and CSS cannot do on their own: * - * 1. Drop the vendor's 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')}> 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-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/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/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-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/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-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/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/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/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/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); + }); }); 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: [ 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": {