Skip to content

[quality] useFocusTrap's non-Tab/non-Escape early return has no end-to-end coverage #1125

Description

@hivecommons-hive

Finding

src/components/hooks/useFocusTrap.js installs one document-level keydown
listener
for the whole lifetime of an open lightbox, so it sees every key
pressed anywhere on the page:

28  const onKeyDown = (event) => {
29    if (event.key === 'Escape') { onClose(); return; }
33    if (event.key !== 'Tab') return;            // <- uncovered end to end
34    const focusable = dialogRef.current?.querySelectorAll('button, a[href]');
35    if (!focusable?.length) return;
38    if (event.shiftKey && document.activeElement === first) { ... }
41    else if (!event.shiftKey && document.activeElement === last) { ... }

The Escape arm is driven in a browser by Escape closes the dialog in
tests/e2e/community-people.spec.js:191 and
tests/e2e/member-directory.spec.js; the two wrap arms by traps Tab inside the dialog (community-people.spec.js:202) and
Shift+Tab from the close button wraps to the last focusable
(tests/e2e/interactions.spec.js:217). Line 33 — the early return — has no
browser assertion at all.

What it protects is not academic. Without it, any keystroke reaching the
document while a lightbox is open falls through into the wrap logic, and a
keystroke made with focus on the dialog's last focusable element then
satisfies !event.shiftKey && activeElement === last and yanks focus back to
the close button. Typing in the dialog, or pressing an arrow key to scroll it,
would bounce focus on every keypress. The unit suite reaches the line through
tests/tools/fake-dom.mjs, which never hydrates the shipped bundle.

Evidence and provenance

  • Unit — covered. npm run test:unit:coverage (node --test coverage,
    node v26.10.0) run locally on 2026-10-06 at main 7ab301e:
    src/components/hooks/useFocusTrap.js | 100.00 | 100.00 | | — no uncovered
    region. The only sub-100% source file in the repository is
    scripts/lib/svg-active-content.mjs (99.48%, region 251).

  • E2E — not covered. Full npm run test:e2e:coverage (322 passed)
    against npm run build:e2e:coverage, rendered with
    npm run report:e2e:coverage -- --input <run> --build build, run locally at
    the same revision 7ab301e:

    src/components/hooks/useFocusTrap.js | 100.00 | 85.71 |  | 33 35 51
    src files                            | 100.00 | 91.45 | 2099/2099 lines | 417/456 regions
    

    The same 85.71% / 33 35 51 appears in CI artifact e2e-coverage, id
    11418568500, from run
    37475718502,
    workflow Validate repository, job End-to-end coverage.

  • Covered by unit tests but not end-to-end tests, so priority 2 under the
    coverage-evidence rules.

Recommendation

Add tests/e2e/focus-trap-indifferent-keys.spec.js (new file): open each
lightbox, Shift+Tab to the last focusable — the only position from which the
removal of line 33 is observable — press a key that is neither Tab nor Escape,
and assert focus has not moved, then press Escape to show the listener is still
attached.

Measured on this change, same build, two full coverage runs:

without: src/components/hooks/useFocusTrap.js | 100.00 | 85.71 |  | 33 35 51
with:    src/components/hooks/useFocusTrap.js | 100.00 | 90.48 |  | 35 51
without: src files | 100.00 | 91.45 | 2099/2099 lines | 417/456 regions
with:    src files | 100.00 | 91.61 | 2099/2099 lines | 415/453 regions

Unlike the variant-build cases in #1097 and #1120, region 33 does leave the
uncovered list — no second build is involved, so there is no region-attribution
mismatch (#1066, #1079). Both figures clear the --check-source-regions 80
gate at .github/workflows/ci.yml:233 and the 86 proposed in #1105.

  • a spec presses a non-Tab, non-Escape key from the dialog's last focusable
    element in both lightboxes and asserts focus does not move
  • src/components/hooks/useFocusTrap.js line 33 leaves the e2e
    uncovered-region list

Lines 35 and 51 are deliberately left out of scope: every shipped dialog
contains a close button, so !focusable?.length has no browser-reachable case,
and both call sites always pass a live triggerRef, so the previousFocus
fallback at line 51 has none either. Those belong in a separate finding if they
are ever worth reaching.

Coordination

Touches only tests/e2e/focus-trap-indifferent-keys.spec.js (new). It does
not touch tests/e2e/dialog-non-dismissing.spec.js (#1120), and the regions
are disjoint: #1120 claims the backdrop onMouseDown guards at
MemberProfile.js:15 and CommunityPeople/index.js:32, this claims
useFocusTrap.js:33. It does not touch tests/e2e/interactions.spec.js,
tests/e2e/community-people.spec.js, tests/e2e/member-directory.spec.js,
tests/tools/e2e-coverage-report.mjs (#1040, #1070, #1114),
tests/tools/e2e-data-fixtures.cjs (#1110), or any overlay under
tests/e2e/fixtures/ (#1104, #1117, #1124). No workflow file is involved.

Priority

  • Impact: medium (a keyboard-accessibility guard on every modal the site ships,
    reached only by a fake DOM; 1 of the 39 uncovered e2e regions)
  • Effort: low

🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5n31 | SHA: 7ab301e

— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.88

Activity

  1. added
    qualityApproved by a Hive merger/owner for auto-merge on green CI
    testingApproved by a Hive merger/owner for auto-merge on green CI
    agent/qualityApproved by a Hive merger/owner for auto-merge on green CI
    hive/covered-by-prHive verified that an open PR references or claims this issue; still actionable until confirmed
    on Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent/qualityApproved by a Hive merger/owner for auto-merge on green CIhive/covered-by-prHive verified that an open PR references or claims this issue; still actionable until confirmedhive/hosted-available-lke648397-260827-5n31Approved by a Hive merger/owner for auto-merge on green CIqualityApproved by a Hive merger/owner for auto-merge on green CItestingApproved by a Hive merger/owner for auto-merge on green CI

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions