Skip to content

Fix sam-datepicker-v2 outside-click close and tabindex/aria-hidden restore leak - #674

Merged
fpigeonjr merged 3 commits into
masterfrom
gh-666-fix-sam-datepicker-v2-outside-click-close-and-the
Sep 2, 2026
Merged

Fix sam-datepicker-v2 outside-click close and tabindex/aria-hidden restore leak#674
fpigeonjr merged 3 commits into
masterfrom
gh-666-fix-sam-datepicker-v2-outside-click-close-and-the

Conversation

@fpigeonjr

Copy link
Copy Markdown
Contributor

Description

experimental/date-range-v2/datepicker/picker.component.ts had two entangled
defects that had to be fixed together (fixing either alone regresses the
other):

  1. calendarpopup never resolves, so outside-click-to-close is dead code.
    @ViewChild("calendarpopup", { static: true }) targeted an element inside
    an *ngIf="showCalendar" block. A static query resolves once, before the
    first change-detection run, while showCalendar is still false — so
    calendarpopup stayed undefined forever. handleGlobalClick guards on
    this.calendarpopup, so its whole body — including
    enablePageTabIndex(), which restores the tabindex/aria-hidden
    attributes disablePageTabIndex() strips from every focusable element on
    the page when the calendar opens — was unreachable. Opening the calendar
    and clicking away left the rest of the page permanently unfocusable and
    hidden from screen readers.

    Fix: changed the @ViewChild to non-static (the default), so it resolves
    once the *ngIf-gated element actually renders.

  2. Strict target comparison treats the icon's own child as an outside
    click.
    handleGlobalClick compared
    this.calendarButton.nativeElement !== event.target. The calendar
    button contains a .sr-only child span; a real mouse click at the
    icon's centre lands on that child, not on #calendarButton itself, so
    strict inequality treated the button's own child as an outside click.
    This was latent while defect 1 masked it, but once calendarpopup
    resolves, the opening click immediately closes the calendar again in
    the same event — the picker becomes unopenable by mouse (keyboard Enter
    still worked, since it dispatches no click).

    Fix: changed the comparison to containment —
    !this.calendarButton.nativeElement.contains(event.target).

Also un-skipped the two picker.spec.ts specs that were blocked on this fix
(should close the calendar when clicking outside of it,
should not close the calendar when clicking the calendar button) and
replaced their stale "why this is skipped" comments with real assertions —
the second spec previously passed only vacuously, because the guard on
this.calendarpopup always early-returned.

Since jsdom has no layout engine to hit-test against, a synthetic
{ target } object can't reproduce defect 2 (a click landing on the icon's
child span) — only a real browser click can. Added a /datepicker gallery
route to test-app (following the /tabs convention from #665) and a new
test-app/e2e/datepicker.spec.ts Playwright spec that exercises the fix
with a real mouse click.

Motivation and Context

Closes #666

Type of Change (Select One and Apply Label)

  • Bug fix (non-breaking change which fixes an issue) → Apply bugfix label
  • New feature (non-breaking change which adds functionality) → Apply enhancement label
  • Breaking change (fix or feature that would cause existing functionality to change) → Apply breaking label
  • Documentation / configuration update → Apply maintenance label

How to Test

  1. npm ci && npm ci --prefix test-app
  2. npm --prefix test-app test — confirms unit specs pass, including the
    two newly-un-skipped picker.spec.ts specs
  3. npm run coverage:check — confirms coverage stays at/above the floor
  4. npx playwright install --with-deps chromium && npm --prefix test-app run test:e2e
    runs test-app/e2e/datepicker.spec.ts against a real Chromium browser
  5. Manual check: cd test-app && npm start, open http://localhost:4200/datepicker
    • Click the calendar icon → the calendar opens and stays open
    • Click outside the calendar → it closes
    • Tab through the page before/after opening the calendar to confirm other
      elements are disabled while it's open and restored once it closes
    • Focus the calendar icon and press Enter → the calendar opens
    • Select a day, click Cancel, and use the year nav arrows → all still work

Expected result: The calendar opens on a real mouse click and stays
open; clicking outside closes it and restores page focusability/screen-reader
visibility; all existing interactions (day select, cancel, month/year nav,
keyboard open) are unaffected.

Screenshots (if appropriate)

N/A — no visual changes; behavior-only fix in a component with no styling
changes.

Checklist

  • Branch name follows convention (e.g. gh-<number>-<slug>)
  • PR title starts with a verb in the imperative mood
  • I have self-reviewed my own code
  • format:check passes (npm run format:check)
  • lint passes (npm run lint)
  • build passes (cd test-app && npm run build)
  • Tests pass and coverage is reported (cd test-app && npm test)
  • If this change requires a documentation update, I have updated it accordingly
  • If there are dependent changes, they have been merged and published in downstream modules

…store leak

- calendarpopup ViewChild changed from static:true to the (non-static)
  default so it resolves once the *ngIf="showCalendar" element renders,
  making the outside-click-close path in handleGlobalClick reachable.
- handleGlobalClick now compares containment
  (calendarButton.nativeElement.contains(event.target)) instead of strict
  identity, so a real click landing on the icon's .sr-only child span is
  not treated as an outside click.
- Un-skip the two picker.spec.ts specs that were blocked on this fix;
  replaced their vacuous-pass commentary with real assertions.
- Add a /datepicker gallery route in test-app (following the #665
  tabs-gallery convention) and test-app/e2e/datepicker.spec.ts covering:
  real-mouse-click open+stays-open, outside-click close,
  tabindex/aria-hidden restore on sibling elements, keyboard Enter open,
  and day-select/cancel/month-year-nav regression coverage.

Fixes #666

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new Playwright spec introduces a fixed waitForTimeout(250) that can slow the suite and add flakiness; prefer a deterministic next-frame wait (see comment).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes sam-datepicker-v2’s outside-click-to-close behavior and ensures tabindex/aria-hidden state is properly restored after the calendar closes, including coverage for the real-browser “click lands on icon child” scenario via Playwright.

Changes:

  • Make calendarpopup a non-static @ViewChild and update outside-click logic to use containment for the calendar button.
  • Un-skip and strengthen unit specs in picker.spec.ts to cover the now-reachable close behavior and the button containment branch.
  • Add a /datepicker gallery route plus Playwright e2e coverage validating open/close and focusability/screen-reader restoration.
File summaries
File Description
src/ui-kit/experimental/date-range-v2/datepicker/picker.component.ts Fix @ViewChild resolution and outside-click containment logic; ensures tab/ARIA restoration runs when closing.
src/ui-kit/experimental/date-range-v2/datepicker/picker.spec.ts Un-skip and update unit tests to assert outside-click close and “click on button doesn’t close”.
test-app/src/app/app.module.ts Register /datepicker route and import the standalone datepicker gallery component.
test-app/src/app/datepicker-gallery/datepicker-gallery.component.ts Add standalone gallery component wiring FormsModule + SamDateRangeV2Module, providing SamFormService.
test-app/src/app/datepicker-gallery/datepicker-gallery.component.html Gallery page with sibling focusable elements to validate tabindex/aria-hidden disabling/restoration.
test-app/src/app/datepicker-gallery/datepicker-gallery.component.spec.ts Basic creation test for the new gallery component.
test-app/e2e/datepicker.spec.ts Playwright tests for real-click open/close behavior and tabindex/aria-hidden restore behavior.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test-app/e2e/datepicker.spec.ts
…wait

Copilot review flagged the fixed waitForTimeout(250) in datepicker.spec.ts
as a suite-slowdown/flakiness risk. handleGlobalClick runs synchronously
during the click's document-level bubble phase, so waiting two animation
frames is sufficient to let it run before re-asserting popup visibility.
@fpigeonjr
fpigeonjr marked this pull request as ready for review September 2, 2026 19:53
@fpigeonjr
fpigeonjr requested a review from a team as a code owner September 2, 2026 19:53
The "opens and stays open" spec clicked `.datepicker .fa-calendar` at its
centre and relied on the browser's hit-test landing on the icon's `.sr-only`
child span -- which is what makes `event.target` a *descendant* of
`#calendarButton` and therefore what distinguishes the fixed
`contains(event.target)` check from the old strict `!==` comparison.

That was verified not to hold: with the fix reverted to `!==`, the previous
centre-click spec still passed, so it was not actually guarding Defect 2. The
hit-test outcome also depends on CSS this harness does not load (`test-app`
has no FontAwesome webfont CSS and no `.sr-only` rule, so the unclipped child
fills the icon's box), meaning any future harness styling would change which
element is hit without any test turning red.

Click the `.sr-only` child explicitly with `{ force: true }` instead, so the
assertion is pinned to the defect regardless of what CSS is present.
Re-verified by reverting the component fix: the spec now fails on
`expect(popup).toBeVisible()` as intended, and passes with the fix in place.
@fpigeonjr

Copy link
Copy Markdown
Contributor Author

Manual testing note: /datepicker renders unstyled — expected, tracked separately

Manually testing the new /datepicker gallery route surfaced two things that look like bugs but are pre-existing and out of scope for this PR. Both are now filed:

Neither is caused by this PR — git diff on picker.component.ts touches only the @ViewChild decorator and the contains() comparison.

One in-scope change pushed: e2e guard now targets the .sr-only child (e4535e0)

While investigating the above I checked whether the open-click spec actually catches Defect 2, by reverting the fix to strict !==:

spec with !== reintroduced
centre click on .fa-calendar (before) passed — not guarding the defect
explicit click on .fa-calendar .sr-only (now) failed on expect(popup).toBeVisible()

Defect 2 only reproduces when event.target is a descendant of #calendarButton, and which element a real click lands on depends on CSS the harness doesn't load. Clicking the child explicitly with { force: true } pins the assertion to the defect regardless of stylesheets — and means #681's harness styling can't silently defang it (called out in that issue's AC).

Verification

Playwright   7 passed  (datepicker x5, tabs, smoke)
Vitest       152 files / 1383 tests passed
coverage     ✓ statements 88.79 · branches 78.83 · functions 85.95 · lines 88.78 (all above floor)
format:check ✓ clean

coverage-floor.json and eslint-baseline.json untouched, per the ratchet rule in AGENTS.md.

@fpigeonjr
fpigeonjr merged commit ff67d00 into master Sep 2, 2026
7 checks passed
fpigeonjr added a commit that referenced this pull request Sep 3, 2026
…store leak (#674)

* Fix sam-datepicker-v2 outside-click close and tabindex/aria-hidden restore leak

- calendarpopup ViewChild changed from static:true to the (non-static)
  default so it resolves once the *ngIf="showCalendar" element renders,
  making the outside-click-close path in handleGlobalClick reachable.
- handleGlobalClick now compares containment
  (calendarButton.nativeElement.contains(event.target)) instead of strict
  identity, so a real click landing on the icon's .sr-only child span is
  not treated as an outside click.
- Un-skip the two picker.spec.ts specs that were blocked on this fix;
  replaced their vacuous-pass commentary with real assertions.
- Add a /datepicker gallery route in test-app (following the #665
  tabs-gallery convention) and test-app/e2e/datepicker.spec.ts covering:
  real-mouse-click open+stays-open, outside-click close,
  tabindex/aria-hidden restore on sibling elements, keyboard Enter open,
  and day-select/cancel/month-year-nav regression coverage.

Fixes #666

* Address PR review feedback: replace fixed timeout with deterministic wait

Copilot review flagged the fixed waitForTimeout(250) in datepicker.spec.ts
as a suite-slowdown/flakiness risk. handleGlobalClick runs synchronously
during the click's document-level bubble phase, so waiting two animation
frames is sufficient to let it run before re-asserting popup visibility.

* Target the .sr-only child in the datepicker open-click e2e guard

The "opens and stays open" spec clicked `.datepicker .fa-calendar` at its
centre and relied on the browser's hit-test landing on the icon's `.sr-only`
child span -- which is what makes `event.target` a *descendant* of
`#calendarButton` and therefore what distinguishes the fixed
`contains(event.target)` check from the old strict `!==` comparison.

That was verified not to hold: with the fix reverted to `!==`, the previous
centre-click spec still passed, so it was not actually guarding Defect 2. The
hit-test outcome also depends on CSS this harness does not load (`test-app`
has no FontAwesome webfont CSS and no `.sr-only` rule, so the unclipped child
fills the icon's box), meaning any future harness styling would change which
element is hit without any test turning red.

Click the `.sr-only` child explicitly with `{ force: true }` instead, so the
assertion is pinned to the defect regardless of what CSS is present.
Re-verified by reverting the component fix: the spec now fails on
`expect(popup).toBeVisible()` as intended, and passes with the fix in place.
fpigeonjr added a commit that referenced this pull request Sep 3, 2026
- Rebase branch tip onto latest #675 (gh-582) so this PR's diff/baseline
  reflect only #586's type-safety changes, not #675's still-open autofix
  set. #675 is CLEAN/MERGEABLE and independently reviewed; stacking is the
  documented convention for this repo's slice-based PRs.
- Retarget PR base to gh-582-apply-safe-eslint-autofixes-and-lower-the-baseline
  so the GitHub diff matches (rebase alone doesn't move a PR's base).
- Drop the #674 datepicker outside-click fix and #677 sidenav RxJS fix
  commits from this branch entirely (dropped during rebase) -- both are
  unrelated bugfixes already merged to master via their own PRs; they were
  only present here as inherited ancestry from an earlier base choice, not
  something this PR should carry or take credit for.
- pagination.component.ts: options.value widened to 'string | number' --
  the template already supports numeric option values via attribute
  binding; the prior 'any[]' allowed this and the fix's 'string' literal
  would have narrowed a supported case.
- date-time-display.pipe.ts: transform() parameter widened to include
  'undefined' explicitly -- the implementation's own guard branch handles
  undefined and the pipe previously accepted it; the stricter signature
  would have been a source-compatibility break for existing callers.
- types.ts: HistoryNodeType.queryParams value type widened to include
  readonly arrays and null, matching Angular Router's actual accepted
  queryParams value shapes (repeated params, param removal) instead of
  the narrower 'string | number | boolean' which would reject valid
  existing usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix sam-datepicker-v2 outside-click close and the tabindex/aria-hidden restore leak

3 participants