refactor(popover): DLT-3455 trap focus with v-dt-focustrap directive

[Paulo Reis (paulojreis-dialpad)]

Obligatory GIF (super important!)

🛠️ Type Of Change

• Fix
• Feature
• Performance Improvement
• Refactor

📖 Jira Ticket

DLT-3455

📖 Description

Migrates DtPopover's Tab focus-trapping from the legacy common/mixins/modal.js mixin to the v-dt-focustrap directive — a trap-only swap. The directive owns only the Tab trap; Popover keeps its existing initial-focus and anchor-restoration (and the Modal mixin, still used by those). Behaviour is preserved. Part of DLT-3452.

• Popover.vue — add v-dt-focustrap="{ active: modal && isOpen, initialFocus: false, restoreFocus: false }" on the <dt-lazy-show> content (the exact element the old trap scoped to — Tippy relocates this node but the directive's listener moves with it). Remove the Tab branch from onKeydown (Escape handling + the keydown emit are kept). The Modal mixin stays — focusInitialElement / focusFirstElementIfNeeded / _getFocusableElements still drive Popover's own initial focus and restoration.
• Popover.test.js — register DtFocustrapDirective in the mount global.plugins; add a "Focus trapping" suite (modal wraps both directions; non-modal doesn't trap).
• Dropdown (the other half of DLT-3455) needs no standalone change: Dropdown.vue has no trap of its own — it wraps <dt-popover :modal="modal" role="menu"> with modal defaulting true, so its trapping is fully delegated to Popover. This migration already covers it; a modal Dropdown traps via the directive on its Popover.

💡 Context

Focus trapping was scattered across components via the Modal mixin; DLT-3452 consolidates it on one directive (v-dt-focustrap, shipped in #1195).

Why trap-only (not full delegation): full delegation would change Popover's initial-focus behaviour — the directive's 'auto' targets the first tabbable in the whole dialog (the header close button, when shown), whereas Popover deliberately focuses the first focusable in the content slot (focusFirstElementIfNeeded($refs.popover__content)). Keeping Popover's own initial-focus + restoration preserves that exactly, on the component that underpins Dropdown/Combobox/Hovercard/FilterPill.

Out of scope — arrow-key navigation. This PR is only about Tab focus-trapping. The roving-tabindex / arrow-key navigation in Dropdown (and Combobox, etc.) is a separate concern and will be migrated to the v-dt-focusgroup directive under DLT-3458 (the companion story) — not here.

Global registration: v-dt-focustrap is a globally-installed plugin — components use it in the template with no local directives: registration. Confirmed consumer apps already call app.use(DtFocustrapDirective), and Storybook registers it in preview.jsx.

📝 Checklist

For all PRs:

• I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
• I have reviewed my changes.
• I have added all relevant documentation.
• I have considered the performance impact of my change.

For all Vue changes:

• I have added / updated unit tests.
• I have validated components with a screen reader.
• I have validated components keyboard navigation.

📷 Screenshots / GIFs

Just noting the expected behaviour.

Focus moves into the popover (when it's modal) and back to the previously focused element when closing:

Screen.Recording.2026-06-04.at.15.25.45.mov

When focus is within the popover, it navigates (in this example) between the visible "Button" and the screen reader-only close button:

Screen.Recording.2026-06-04.at.15.35.16.mov

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: a3636014-8a3c-45c0-ad79-bccc4751a03d

📥 Commits

Reviewing files that changed from the base of the PR and between 99de17d and b6d9444.

⛔ Files ignored due to path filters (1)

• apps/dialtone-documentation/docs/.vuepress/public/assets/images/components/modal-dark.png is excluded by !**/*.png and included by **

📒 Files selected for processing (1)

• packages/dialtone-vue/components/Popover/Popover.test.js

---

Migrates Popover Tab focus-trapping from the modal mixin to the v-dt-focustrap directive (trap-only), keeps Popover initial-focus/restore via the modal mixin, removes Tab handling from onKeydown, and adds directive-registered focus-trapping tests.

Overall Judgement: ✅ Ready to merge — focused refactor preserving behavior with tests and accessibility validation.

Walkthrough

Popover component offloads Tab focus-trapping to the v-dt-focustrap directive. The directive is applied to the content container when modal && isOpen, and prior inline Tab handling is removed from the onKeydown method. Tests verify focus wrapping behavior when modal is enabled and non-trapping when disabled.

Changes

Focus trap delegation

Layer / File(s)	Summary
Directive application and Tab handling removal packages/dialtone-vue/components/Popover/Popover.vue	v-dt-focustrap directive added to popover content element with conditional activation (modal && isOpen). Explicit Tab key handling removed from onKeydown method; focus trapping now delegated to the directive.
Test setup and focus-trapping verification packages/dialtone-vue/components/Popover/Popover.test.js	Directive and flushPromises imported and registered in test setup. New "Focus trapping" test suite verifies Tab/Shift+Tab focus wrapping when modal: true and confirms focus is not trapped when modal: false. The directive is also added to other local mount configs used in appendTo and anchor-slot-change tests.

• Possibly related PRs:
  • dialpad/dialtone#1313: Similar migration of Tab focus-trapping from component logic to v-dt-focustrap and updated focus-trapping tests.
  • dialpad/dialtone#1316: Implements v-dt-focustrap-based Tab trapping in a different modal component and its tests.

Suggested reviewers

• francisrupert
• iropolo

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/DLT-3455-popover-focustrap

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f5fc904d3f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

[chatgpt-codex-connector]

Preserve radio-group boundary handling in modal popovers

For modal popovers whose first tabbable control is a radio group with a checked option that is not the first DOM radio, this removes the old focusTrappedTabPress path that normalized the first boundary to the checked radio. The new v-dt-focustrap boundary logic uses the first DOM element, so pressing Shift+Tab from the checked radio is no longer treated as being at the start of the trap and browser focus can escape the popover.

Useful? React with 👍 / 👎.

[Paulo Reis (paulojreis-dialpad)]

Acknowledged as a potential bug. Out of scope for this PR (popover-only migration), as it affects the directive. Will try to create minimal reproducible example and raise a separate ticket if confirmed.

[Brad Paugh (braddialpad)]

Looks good, happy to see this in use.

[Brad Paugh (braddialpad)]

All of these localWrapper mounts should also register the FocusTrap directive.

[Paulo Reis (paulojreis-dialpad)]

👍 Addressed with b6d9444

[Brad Paugh (braddialpad)]

hmm the accessibility tests have been running for over 3 hours... going to try restarting.

[Belu Montoya (belumontoya)]

looks good, same as the other one, will be nice to see the test run first and then merge this

[Brad Paugh (braddialpad)]

merge next into this branch to fix

[coderabbitai]

♻️ Duplicate comments (1)

packages/dialtone-vue/components/Popover/Popover.test.js (1)

456-461: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Register DtFocustrapDirective in localWrapper mounts.

These localWrapper mounts should also register DtFocustrapDirective in global.plugins for consistency and to avoid console warnings about the unknown directive.

🔧 Add directive registration

For each localWrapper mount, add the directive to global.plugins:

 const localWrapper = mount(DtPopover, {
   props: { ...baseProps, open: null },
   slots: { ...baseSlots },
-  global: { stubs: { transition: false } },
+  global: {
+    plugins: [DtFocustrapDirective],
+    stubs: { transition: false },
+  },
   attachTo: document.body,
 });

Apply this pattern to all four localWrapper mounts at lines 456, 475, 496, and 535.

Also applies to: 475-480, 496-501, 535-543

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dialtone-vue/components/Popover/Popover.test.js` around lines 456 -
461, The local test mounts of DtPopover are missing registration of
DtFocustrapDirective which causes console warnings; update each localWrapper
mount (the ones creating localWrapper for DtPopover at the blocks around lines
where props/open are set) to include DtFocustrapDirective in the mount options
by adding it to global.plugins (e.g., global.plugins: [DtFocustrapDirective]) so
all four localWrapper mounts (the mounts around the existing open: null and
other variants) register the directive consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@packages/dialtone-vue/components/Popover/Popover.test.js`:
- Around line 456-461: The local test mounts of DtPopover are missing
registration of DtFocustrapDirective which causes console warnings; update each
localWrapper mount (the ones creating localWrapper for DtPopover at the blocks
around lines where props/open are set) to include DtFocustrapDirective in the
mount options by adding it to global.plugins (e.g., global.plugins:
[DtFocustrapDirective]) so all four localWrapper mounts (the mounts around the
existing open: null and other variants) register the directive consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 6d1d97cb-c9f7-4239-9a6c-441c3d6c911a

📥 Commits

Reviewing files that changed from the base of the PR and between f5fc904 and 99de17d.

📒 Files selected for processing (2)

• packages/dialtone-vue/components/Popover/Popover.test.js
• packages/dialtone-vue/components/Popover/Popover.vue

[github-actions]

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

[github-actions]

✔️ Deploy previews ready!
😎 Dialtone documentation preview: https://dialtone.dialpad.com/deploy-previews/pr-1312/
😎 Dialtone-vue preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-1312/