feat(banner): DLT-3454 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-3454

📖 Description

Migrates DtBanner's focus trapping from the legacy common/mixins/modal.js mixin to the new v-dt-focustrap directive — the first internal adopter of the directive (part of DLT-3452). Trapping behaviour is preserved: an important banner (role="alertdialog") traps Tab/Shift+Tab and receives initial focus; non-important status banners don't trap.

• Banner.vue — replace mixins: [Modal] + @keydown.tab="trapFocus" + the trapFocus method + the mounted() focus call with v-dt-focustrap="{ active: important, initialFocus: 'auto', restoreFocus: true }" on the root <aside>. Add data-qa hooks (dt-banner, dt-banner-dialog) and refresh the important prop JSDoc. Removes the now-unused eslint-disable vuejs-accessibility/no-static-element-interactions.
• Banner.test.js — new test suite (Banner previously had no tests): role/aria-modal toggling, gated initial focus, Tab/Shift+Tab wrap, no-trap when not important, runtime important toggle, and directive focus-restoration.
• banner.md — add an Accessibility “Focus management” note, and switch the interactive Kind demo from v-show to v-if (see the gotcha in Context).

💡 Context

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

Global registration dependency: v-dt-focustrap is a globally-installed plugin — components use it in the template with no local directives: registration. The migrated banner is inert for focus-trapping unless the host app calls app.use(DtFocustrapDirective); confirmed consumer apps already do, and Storybook registers it in preview.jsx.

Behaviour deltas (additive, intended):

• Focus restoration is now consistent — the directive restores focus on close for both showClose: true and showClose: false (previously NoticeAction only restored when showClose: true).
• Reactive activation — toggling important false→true at runtime now moves focus into the banner; the old mounted-only code didn't.

v-show pitfall (why the demo now uses v-if): the directive activates when important becomes truthy, not when the banner becomes visible. If an always-mounted banner (v-show) has important set before it's revealed, no activation fires on reveal, so focus stays on the trigger (the trap still works once you Tab). Rendering with v-if — or setting important at show-time — mounts/activates the banner while visible, so focus moves in on appearance and restores on close. The Kind demo was switched to v-if to demonstrate this faithfully; consumers building alert banners should prefer v-if.

📝 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

These screen-recordings will illustrate the pitfall mentioned above: what happens when a banner is controlled via v-show. Pay attention to:

1. The focus ring;
2. The document.activeElement watcher in dev tools

This is the current state, and also what we'd get post-migration if we control a banner with v-show (the directive doesn't fix it). Notice how focus stays on the button that triggers the modal.

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

And this is what happens when using v-if instead of v-show — this guarantees that the component is actually mounted when toggled on. Notice how the focus moves from the trigger button to the button in the banner, and then is restored back to the trigger button when the banner is closed.

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

🔮 Next Steps

I guess we should document the v-if / v-show oddity. I could do it in the banner docs, but this is likely to affect all components that use the focus trap directive AND have their visibility controlled via v-show. Probably we should update both the banner AND the directive docs. What do you think? cc Francis Rupert (@francisrupert) Brad Paugh (@braddialpad)

[coderabbitai]

Migrates DtBanner focus-trapping from the legacy Modal mixin to the new v-dt-focustrap directive, removes mixin/mounted/trapFocus logic, adds directive & data-qa hooks, expands tests for trapping/restore/runtime toggling, and updates docs (uses v-if; documents focus management).

Overall Judgement: ✅ Ready to merge — Tests, docs, and accessibility behavior comprehensively updated, covering runtime toggling and focus restoration edge cases.

Walkthrough

DtBanner replaces manual Tab-key trapping and a Modal mixin with the v-dt-focustrap directive (including restoration). The component template and JSDoc were updated, the mounted hook and trapFocus method removed, tests were added to validate focus behavior and restoration, and docs/examples were updated to reflect mount and focus semantics.

Changes

Banner focus trap directive integration

Layer / File(s)	Summary
Focus trap directive integration in Banner component packages/dialtone-vue/components/Banner/Banner.vue	Banner template now uses v-dt-focustrap directive on the outer <aside> when important=true for automatic focus trapping and restoration. Modal mixin import and mixins: [Modal] option are removed; the mounted() hook and trapFocus() method are deleted. The important prop JSDoc is updated and data-qa attributes added to banner and dialog elements.
Focus behavior test coverage packages/dialtone-vue/components/Banner/Banner.test.js	Jest/Vue Test Utils suite validates rendering, accessibility roles (alertdialog for important, status for default), aria-modal attributes, focus trap activation/deactivation at runtime, Tab/Shift+Tab wrapping when trap is active, focus restoration when banner unmounts, and close button event emission. Tests mount to document.body to observe focus behavior.
Documentation and example updates apps/dialtone-documentation/docs/components/banner.md	Example demo changes from v-show to v-if to mount/unmount the banner. New Accessibility "Focus management" subsection documents that important banners use modal alertdialog role with focus trapped inside and restored on dismiss, while non-important banners use status role without focus trapping.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Banner as DtBanner(v-dt-focustrap)
  participant Directive as DtFocustrapDirective
  participant DOM as Document
  User->>Banner: Mount with important=true
  Banner->>Directive: v-dt-focustrap activate
  Directive->>DOM: Move focus into banner/dialog
  Directive->>Directive: Trap Tab/Shift+Tab within banner
  User->>Banner: Click close / unmount
  Banner->>Directive: v-dt-focustrap deactivate (unmount)
  Directive->>DOM: Restore focus to previously focused element

Loading

Possibly related PRs

• dialpad/dialtone#1313: Also migrates component-local focus-trapping to the shared v-dt-focustrap directive and updates tests for directive-driven focus behavior.
• dialpad/dialtone#1312: Refactors focus trapping to use v-dt-focustrap replacing Tab-handling logic in other components.
• dialpad/dialtone#1316: Applies v-dt-focustrap to DtModal, similar focus-trap refactor.

Suggested reviewers

• francisrupert
• iropolo
• braddialpad
• ninarepetto

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/DLT-3454-banner-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: 2a165ccbe6

ℹ️ 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]

Keep the focus trap self-contained

When DtBanner is imported/registered as a standalone component, this directive is ignored unless the host app has also called app.use(DtFocustrapDirective). The old Modal mixin made the important-banner behavior internal to the component, so consumers following the component import path now still get role="alertdialog"/aria-modal but lose initial focus and Tab trapping; I also checked the docs app client registration and it does not install this directive globally.

Useful? React with 👍 / 👎.

[Paulo Reis (paulojreis-dialpad)]

Directives do get registered in

dialtone/apps/dialtone-documentation/docs/.vuepress/theme/client.js

Lines 231 to 232 in 2436364

	} else if (key.endsWith('Directive')) {
	app.use(module[key]);

However, it's a valid point that using DtBanner without previously app.use(DtFocustrapDirective)ing will result in lack of focus trapping. It's a separate concern (at the directive, or better yet: at the Dialtone setup/registration level) though, so worth addressing separately.

[coderabbitai]

Actionable comments posted: 3

🤖 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.

Inline comments:
In `@packages/dialtone-vue/components/Banner/Banner.test.js`:
- Around line 47-50: The test contains multiple assertions in single it blocks
(e.g., the it('should render the banner and its dialog') that checks both
banner.exists() and dialog.exists()); split each multi-assert test into separate
it(...) cases so each test contains only one expect (e.g., one it for
banner.exists() and another it for dialog.exists()), move shared setup for
banner and dialog into a beforeEach or beforeAll to avoid repetition, and give
each new it a descriptive name reflecting the single assertion (e.g., "renders
banner" and "renders dialog") while keeping existing test helpers and selectors
unchanged.
- Around line 41-44: The afterEach should also unmount and clean up the mounted
test wrapper to prevent DOM/focus leakage: in the existing afterEach (the block
that resets mockProps and mockSlots) check for the test instance variable (e.g.,
wrapper) and if defined call wrapper.unmount() (or wrapper.destroy() depending
on your mounting API), then clear document.body (e.g., document.body.innerHTML =
'') and null out wrapper to fully remove the component from the DOM and reset
focus state.

In `@packages/dialtone-vue/components/Banner/Banner.vue`:
- Line 3: The v-dt-focustrap usage in DtBanner makes important-mode behavior
dependent on a globally registered directive; to fix, ensure the
DtFocustrapDirective is registered by the library install/plugin so consumers
get it automatically (register DtFocustrapDirective inside the library's install
function or default export that sets up global directives), and update DtBanner
(the v-dt-focustrap binding) only if the directive is available; alternatively,
if you choose not to auto-register, mark this as a breaking change with
migration guidance in the release notes so consumers know to register
DtFocustrapDirective themselves.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 05acc912-6551-4f43-9e18-44db673347fb

📥 Commits

Reviewing files that changed from the base of the PR and between 8387e26 and 2a165cc.

📒 Files selected for processing (3)

• apps/dialtone-documentation/docs/components/banner.md
• packages/dialtone-vue/components/Banner/Banner.test.js
• packages/dialtone-vue/components/Banner/Banner.vue

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Split multi-assert tests to follow the one-assertion rule.

Lines 47-50, 69-72, and 87-89 contain multiple assertions in a single test. Please split these into separate it(...) blocks for clearer failures and to align with test conventions.

As per coding guidelines, “One assertion per test. Use beforeAll/beforeEach to reduce repetition.”

Also applies to: 69-72, 87-89

🤖 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/Banner/Banner.test.js` around lines 47 - 50,
The test contains multiple assertions in single it blocks (e.g., the it('should
render the banner and its dialog') that checks both banner.exists() and
dialog.exists()); split each multi-assert test into separate it(...) cases so
each test contains only one expect (e.g., one it for banner.exists() and another
it for dialog.exists()), move shared setup for banner and dialog into a
beforeEach or beforeAll to avoid repetition, and give each new it a descriptive
name reflecting the single assertion (e.g., "renders banner" and "renders
dialog") while keeping existing test helpers and selectors unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Directive registration requirement can silently break important banner behavior.

Line 3 makes focus trapping/restoration depend on a globally registered v-dt-focustrap directive. Existing consumers that upgrade without registering DtFocustrapDirective lose important focus-trap behavior. Linked repository findings show broad DtBanner usage and no observed directive registration, so this is a real compatibility risk. Please either register this directive in the library install path or mark this as a breaking change with migration guidance.

As per coding guidelines, “This is a public npm library. Breaking changes without BREAKING CHANGE footer ship as patches and silently break consumers.”

🤖 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/Banner/Banner.vue` at line 3, The
v-dt-focustrap usage in DtBanner makes important-mode behavior dependent on a
globally registered directive; to fix, ensure the DtFocustrapDirective is
registered by the library install/plugin so consumers get it automatically
(register DtFocustrapDirective inside the library's install function or default
export that sets up global directives), and update DtBanner (the v-dt-focustrap
binding) only if the directive is available; alternatively, if you choose not to
auto-register, mark this as a breaking change with migration guidance in the
release notes so consumers know to register DtFocustrapDirective themselves.

[Paulo Reis (paulojreis-dialpad)]

Directives do get registered in

dialtone/apps/dialtone-documentation/docs/.vuepress/theme/client.js

Lines 231 to 232 in 2436364

	} else if (key.endsWith('Directive')) {
	app.use(module[key]);

However, it's a valid point that using DtBanner without previously app.use(DtFocustrapDirective)ing will result in lack of focus trapping. It's a separate concern (at the directive, or better yet: at the Dialtone setup/registration level) though, so worth addressing separately.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if DtFocustrapDirective is registered in the library's main install/plugin entry point
rg -n "DtFocustrapDirective\|focustrap\|focustrap" --type js --type ts --type vue -C3 packages/dialtone-vue/index.js packages/dialtone-vue/index.ts 2>/dev/null || true

# Find and check the main library entry points
fd -e js -e ts "index" packages/dialtone-vue --max-depth 2 | head -20

# Check what the dialtone install function registers
rg -n "install\|DtFocustrap\|focustrap" packages/dialtone-vue/index.js packages/dialtone-vue/index.ts 2>/dev/null | head -40 || true

# Find where DtFocustrapDirective is exported/defined
rg -rn "DtFocustrapDirective" packages/dialtone-vue -C2 | head -40

Length of output: 4039

---

🏁 Script executed:

#!/bin/bash
# Check the main library index.js for install function and directive registration
cat packages/dialtone-vue/index.js | head -80

# Check if there's an install function that registers directives globally
rg -n "install\|directive\|app\.directive\|app\.use" packages/dialtone-vue/index.js | head -40

# Check exports of DtFocustrapDirective from the root index
rg -n "focustrap\|Focustrap" packages/dialtone-vue/index.js | head -20

Length of output: 3523

---

---

@paulojreis-dialpad — confirmed, the docs/VuePress registration is there. The library does export DtFocustrapDirective from the root index.js, but there's no auto-registration in an install function (consumers must call app.use(...) explicitly), which is consistent with how the .mdx documents it.

The firespotter findings support the "real gap" concern: no DtFocustrapDirective registration was found there despite broad DtBanner usage. Agreed this is best tracked as a separate follow-up (registering the directive in the library's top-level install function, similar to how a plugin would bundle it).

Would you like me to open a GitHub issue to track adding auto-registration of DtFocustrapDirective within the library's install path?

---

🧠 Learnings used

Learnt from: braddialpad
Repo: dialpad/dialtone PR: 1177
File: packages/dialtone-vue/common/mixins/input.test.js:24-24
Timestamp: 2026-04-09T22:23:21.554Z
Learning: In the dialtone monorepo’s packages/dialtone-vue, treat the validationState rename as an intentional breaking change: `VALIDATION_MESSAGE_TYPES.ERROR`→`CRITICAL` and `VALIDATION_MESSAGE_TYPES.SUCCESS`→`POSITIVE` should use the new `'critical'`/`'positive'` string values, and do not require backward compatibility with the prior `'error'`/`'success'` values or test coverage for the old strings.

---

Learnt from: francisrupert
Repo: dialpad/dialtone PR: 1215
File: packages/dialtone-vue/components/resizable/resizable_panel.vue:38-73
Timestamp: 2026-04-21T16:47:48.205Z
Learning: In packages/dialtone-vue Vue components (dialpad/dialtone’s dialtone-vue), ensure JSDoc `values` tags use explicit comma-separated enumerations (e.g., `values 0p,10p,25p`). The `vue-docgen-api` used to generate `component-documentation.json` parses `values` as comma-separated tokens into the `values` array and does not expand prose range descriptions (e.g., `0p–100p`) into individual values—so range notation will not be reflected correctly in the generated documentation. Avoid suggesting or using range notation in `values`.

---

Learnt from: francisrupert
Repo: dialpad/dialtone PR: 1273
File: packages/dialtone-vue/components/Button/Button.vue:60-60
Timestamp: 2026-05-14T18:03:17.195Z
Learning: In dialpad/dialtone, when reviewing JSDoc and slot comments, treat the physical aliases “(aka left)”, “(aka right)”, “(aka top)”, and “(aka bottom)” as intentional LTR-default shorthand. Do not flag them as technically inaccurate or request qualifiers like “commonly in LTR,” even if writing-mode accuracy could be argued otherwise—this is a documented team convention (see .claude/rules/logical-naming.md).

[Brad Paugh (braddialpad)]

Looks good, just one small fix to make in the tests before merging.

[Brad Paugh (braddialpad)]

lets do the unmount in the afterEach or else the wrapper will stay mounted and attached to body after running the last test in this module.

[Paulo Reis (paulojreis-dialpad)]

Thanks for catching this; addressed in 2436364

[Belu Montoya (belumontoya)]

pre approving, im guessing the a11y ci issue is related to the other fix, will be nice to merge that first and re run it here just in case

[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-1310/
😎 Dialtone-vue preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-1310/

[coderabbitai]

♻️ Duplicate comments (1)

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

51-54: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Split multi-assertion tests into separate it() blocks.

These tests contain multiple expect() statements and violate the repo guideline: "One assertion per test."

Affected tests:

• Lines 51-54: checks both banner.exists() and dialog.exists()
• Lines 73-76: checks both role and aria-modal
• Lines 84-93: checks activeElement multiple times
• Lines 105-108: checks both role and aria-modal
• Lines 132-143: checks focus containment before and after toggle
• Lines 159-175: checks focus moved into banner, then restored to outsideButton

Split each into separate it() blocks so each contains one assertion. Move repeated setup (e.g., finding dialog, focusing elements) into beforeEach or helper calls to reduce duplication.

As per coding guidelines, "One assertion per test. Use beforeAll/beforeEach to reduce repetition."

Also applies to: 73-76, 84-93, 105-108, 132-143, 159-175

🤖 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/Banner/Banner.test.js` around lines 51 - 54,
The test file has multiple expects per it() — split each multi-assertion test
into separate it() blocks so each test contains a single expect; for example
separate the banner.exists() and dialog.exists() check in the test referencing
banner and dialog, split role vs aria-modal assertions, split each activeElement
assertion into its own it(), and split pre/post-toggle focus containment and
focus-restore checks (the tests referencing outsideButton, dialog, banner,
activeElement, and the toggle/open methods). Move repeated setup (finding dialog
via wrapper.find, focusing elements like outsideButton, opening the banner using
the same helper or beforeEach) into a beforeEach or small helper functions to
reduce duplication and keep each new it() minimal and focused.

Source: Coding guidelines

🤖 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/Banner/Banner.test.js`:
- Around line 51-54: The test file has multiple expects per it() — split each
multi-assertion test into separate it() blocks so each test contains a single
expect; for example separate the banner.exists() and dialog.exists() check in
the test referencing banner and dialog, split role vs aria-modal assertions,
split each activeElement assertion into its own it(), and split pre/post-toggle
focus containment and focus-restore checks (the tests referencing outsideButton,
dialog, banner, activeElement, and the toggle/open methods). Move repeated setup
(finding dialog via wrapper.find, focusing elements like outsideButton, opening
the banner using the same helper or beforeEach) into a beforeEach or small
helper functions to reduce duplication and keep each new it() minimal and
focused.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 5d46e1e6-9648-4106-a0c4-e21d536c7d06

📥 Commits

Reviewing files that changed from the base of the PR and between ba97fd1 and 2436364.

📒 Files selected for processing (1)

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