feat(focusgroup): DLT-3285 add v-dt-focusgroup directive for declarative roving tabindex#1187
Conversation
|
Please add either the |
Francis Rupert (francisrupert)
added
the
no-visual-test
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new v-dt-focusgroup directive (implementation, constants, export), Storybook docs/stories/examples, unit tests, ESLint rules/docs enforcing role/label, package re-exports and Storybook registration, documentation/data updates for a "Vue Utilities" nav entry, and extends active-link detection to include /functions-and-utilities/ in docs nav logic. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Dir as v-dt-focusgroup<br/>(Directive)
participant DOM as DOM<br/>Elements
participant App as Vue App
User->>Dir: Key press or focus
activate Dir
Dir->>Dir: parseConfig() / resolveSelector() / resolveSkipDisabled()
Dir->>DOM: querySelectorAll -> filter hidden / skipped / disabled
DOM-->>Dir: items[]
Dir->>Dir: compute next target (axis, loop, RTL)
Dir->>DOM: update roving tabindex attributes
Dir->>DOM: focus(target)
DOM->>App: dispatch dt-focusgroup-move(detail: item,index,prev)
deactivate Dir
App-->>User: consumer handles selection/activation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js (1)
199-214: RTL detection edge case (optional).Line 202's
|| el.closest('[dir="rtl"]')could yield incorrect results if the container has explicitdir="ltr"but sits inside an RTL ancestor.getComputedStyle(el).directionalone is authoritative since it reflects inheritance correctly. The closest check appears redundant.Low-impact since nested opposing
dirattributes are rare.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js` around lines 199 - 214, The RTL detection is using getComputedStyle(el).direction combined with el.closest('[dir="rtl"]'), which can be wrong when the element has an explicit dir="ltr" but is inside an RTL ancestor; change the isRTL assignment in attach to rely solely on getComputedStyle(el).direction === 'rtl' (remove the el.closest('[dir="rtl"]') fallback) and update the state object (state.isRTL) initialization to use that single authoritative value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-label.js`:
- Around line 35-38: The predicate that currently only accepts literal
accessible-name attributes (it checks !attr.directive) should be expanded to
also accept Vue bound attributes; update the condition in
focusgroup-requires-label.js so it returns true for either non-directive
attributes matching attr.key.name === 'aria-label' or 'aria-labelledby' OR for
directive/bound attributes where the attribute identifier lives on
attr.key.name.name (check attr.directive && attr.key && attr.key.name &&
(attr.key.name === 'aria-label' || attr.key.name === 'aria-labelledby') or
attr.key.name.name depending on AST shape), and add RuleTester valid cases
covering templates with :aria-label and :aria-labelledby to prevent false
positives.
- Around line 27-29: The call to
sourceCode.parserServices.defineTemplateBodyVisitor in the rule assumes
parserServices exists; add a guard that checks for sourceCode.parserServices and
sourceCode.parserServices.defineTemplateBodyVisitor before calling it (e.g., if
missing, return an empty visitor object). Update the visitor creation in the
top-level of focusgroup-requires-label (the code using sourceCode and
defineTemplateBodyVisitor and the VAttribute visitor) to early-return {} when
parserServices/defineTemplateBodyVisitor is falsy so the rule no longer throws
when vue-eslint-parser is not configured.
In `@packages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-role.js`:
- Around line 34-36: The predicate computing hasRole in
focusgroup-requires-role.js currently ignores directive attributes (attr =>
!attr.directive && attr.key.name === 'role'), which misses bound roles like
:role/v-bind:role; update the predicate used for hasRole to treat either a plain
attribute with key.name === 'role' OR a directive whose argument identifies the
role (e.g., attr.directive && attr.key.argument && attr.key.argument.name ===
'role'), so bound role attributes are counted as present; after updating the
hasRole logic, add a RuleTester valid case for a template using :role="someRole"
(or v-bind:role) to prevent regressions.
---
Nitpick comments:
In `@packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js`:
- Around line 199-214: The RTL detection is using getComputedStyle(el).direction
combined with el.closest('[dir="rtl"]'), which can be wrong when the element has
an explicit dir="ltr" but is inside an RTL ancestor; change the isRTL assignment
in attach to rely solely on getComputedStyle(el).direction === 'rtl' (remove the
el.closest('[dir="rtl"]') fallback) and update the state object (state.isRTL)
initialization to use that single authoritative value.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e16b61d6-df89-43c6-9772-177e5882a8a9
📒 Files selected for processing (25)
apps/dialtone-documentation/docs/.vuepress/theme/components/Navbar.vueapps/dialtone-documentation/docs/.vuepress/theme/components/Page.vueapps/dialtone-documentation/docs/.vuepress/theme/composables/useSidebarItems.jsapps/dialtone-documentation/docs/.vuepress/theme/layouts/Layout.vueapps/dialtone-documentation/docs/.vuepress/views/UiKitsOverview.vueapps/dialtone-documentation/docs/_data/site-nav.jsonapps/dialtone-documentation/docs/_data/vue-utilities.jsonapps/dialtone-documentation/docs/functions-and-utilities/index.mdapps/dialtone-documentation/docs/scratch.mdpackages/dialtone-vue/.storybook/preview.jsxpackages/dialtone-vue/directives/focusgroup_directive/focusgroup.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup.mdxpackages/dialtone-vue/directives/focusgroup_directive/focusgroup.stories.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup.test.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup_constants.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup_directive_events.story.vuepackages/dialtone-vue/directives/focusgroup_directive/focusgroup_directive_recipes.story.vuepackages/dialtone-vue/directives/focusgroup_directive/index.jspackages/dialtone-vue/directives/mode_directive/mode_directive_default.story.vuepackages/dialtone-vue/directives/scrollbar_directive/scrollbar_directive_default.story.vuepackages/dialtone-vue/index.jspackages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-label.jspackages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-role.jspackages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-label.jspackages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-role.js
packages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-label.js
Outdated
Show resolved
Hide resolved
…isabledSkipped story
…tainer and add unknown token warning
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js`:
- Line 204: The RTL detection currently uses el.closest('[dir="rtl"]') which can
be overridden by nearer dir attributes; replace that logic so isRTL is
determined solely from getComputedStyle(el).direction === 'rtl' (remove the
el.closest check) inside the focusgroup directive where isRTL is computed, and
update any related references to rely on that computed direction. Also add a
unit/integration test that renders a parent with dir="rtl" and a nested
focusgroup element with dir="ltr" and assert horizontal navigation behaves as
LTR (i.e., left/right are not inverted) to prevent regressions.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dcdd7fe1-6861-42d8-a2b0-d441ab6a9271
📒 Files selected for processing (4)
packages/dialtone-vue/directives/focusgroup_directive/focusgroup.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup.mdxpackages/dialtone-vue/directives/focusgroup_directive/focusgroup.stories.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup_constants.js
✅ Files skipped from review due to trivial changes (1)
- packages/dialtone-vue/directives/focusgroup_directive/focusgroup.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dialtone-vue/directives/focusgroup_directive/focusgroup_constants.js
packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js
Outdated
Show resolved
Hide resolved
…nabled item on blur
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js (1)
209-209:⚠️ Potential issue | 🟠 MajorUse computed direction only for RTL detection
Line 209 incorrectly combines computed direction with
closest('[dir="rtl"]'). In nesteddiroverrides (e.g., RTL parent, LTR focusgroup), this inverts horizontal navigation incorrectly. Use only computed style direction.Suggested fix
- const isRTL = getComputedStyle(el).direction === 'rtl' || el.closest('[dir="rtl"]') !== null; + const isRTL = getComputedStyle(el).direction === 'rtl';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js` at line 209, The RTL detection incorrectly combines computed style with a DOM ancestor check; update the isRTL assignment in focusgroup.js to rely solely on the computed style for the element (use getComputedStyle(el).direction === 'rtl') and remove the el.closest('[dir="rtl"]') part so nested dir overrides (e.g., an LTR focusgroup inside an RTL parent) don't invert navigation; locate and change the isRTL declaration where it’s currently set to const isRTL = getComputedStyle(el).direction === 'rtl' || el.closest('[dir="rtl"]') !== null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/dialtone-vue/directives/focusgroup_directive/focusgroup.js`:
- Line 209: The RTL detection incorrectly combines computed style with a DOM
ancestor check; update the isRTL assignment in focusgroup.js to rely solely on
the computed style for the element (use getComputedStyle(el).direction ===
'rtl') and remove the el.closest('[dir="rtl"]') part so nested dir overrides
(e.g., an LTR focusgroup inside an RTL parent) don't invert navigation; locate
and change the isRTL declaration where it’s currently set to const isRTL =
getComputedStyle(el).direction === 'rtl' || el.closest('[dir="rtl"]') !== null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 44fc47ce-5b41-4811-9d8c-3b6a52cb7604
📒 Files selected for processing (9)
packages/dialtone-vue/directives/focusgroup_directive/focusgroup.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup.test.jspackages/dialtone-vue/directives/focusgroup_directive/focusgroup_directive_events.story.vuepackages/eslint-plugin-dialtone/docs/rules/focusgroup-requires-label.mdpackages/eslint-plugin-dialtone/docs/rules/focusgroup-requires-role.mdpackages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-label.jspackages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-role.jspackages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-label.jspackages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-role.js
✅ Files skipped from review due to trivial changes (4)
- packages/eslint-plugin-dialtone/docs/rules/focusgroup-requires-role.md
- packages/eslint-plugin-dialtone/docs/rules/focusgroup-requires-label.md
- packages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-label.js
- packages/eslint-plugin-dialtone/tests/lib/rules/focusgroup-requires-role.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-role.js
- packages/eslint-plugin-dialtone/lib/rules/focusgroup-requires-label.js
- packages/dialtone-vue/directives/focusgroup_directive/focusgroup.test.js
…r to check dir attribute value
Francis Rupert (francisrupert)
requested review from
Brad Paugh (braddialpad),
Ignacio Ropolo (iropolo) and
Nina Repetto (ninarepetto)
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bda3cb6fdc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Brad Paugh (braddialpad)
left a comment
Brad Paugh (braddialpad)
left a comment
There was a problem hiding this comment.
This PR looks pretty good IMO. I have one question, what is the behaviour of multi nested v-dt-focusgroup is this something that should be acknowledged, managed or handled? I believe at the moment if you have multiple nested groups and you press left arrow for example, this event will trigger in all of them. Expected behaviour?
| function parseObjectConfig (config, value) { | ||
| for (const key of CONFIG_KEYS) { | ||
| if (value[key] !== undefined) config[key] = value[key]; | ||
| } | ||
| } | ||
|
|
||
| function parseStringConfig (config, value) { | ||
| const tokens = value.split(/\s+/); | ||
| for (const token of tokens) { | ||
| const mapping = TOKEN_MAP[token]; | ||
| if (mapping) { | ||
| config[mapping.key] = mapping.value; | ||
| } else if (token && process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `[DtFocusgroupDirective] Unknown token "${token}". ` + | ||
| `Valid tokens: ${Object.keys(TOKEN_MAP).join(', ')}.`, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export function parseConfig (value) { | ||
| const config = { ...FOCUSGROUP_DEFAULTS }; | ||
|
|
||
| if (!value || value === true) return config; | ||
|
|
||
| if (typeof value === 'object') { | ||
| parseObjectConfig(config, value); | ||
| } else if (typeof value === 'string') { | ||
| parseStringConfig(config, value); | ||
| } | ||
|
|
||
| return config; | ||
| } | ||
|
|
||
| /** | ||
| * Shallow comparison of two parsed config objects. | ||
| * Used by the updated hook to avoid teardown/reattach when config is semantically identical. | ||
| */ | ||
| export function configsEqual (a, b) { | ||
| return CONFIG_KEYS.every(key => a[key] === b[key]); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the final item selector for a focusgroup container. | ||
| * | ||
| * Priority: explicit config.selector > role-aware default > fallback (all focusable). | ||
| * | ||
| * @param {HTMLElement} el - The focusgroup container element | ||
| * @param {{ selector: string|null }} config - Parsed focusgroup config | ||
| * @returns {string} CSS selector string | ||
| */ | ||
| export function resolveSelector (el, config) { | ||
| if (config.selector) return config.selector; | ||
|
|
||
| const role = el.getAttribute('role'); | ||
| if (role && ROLE_DEFAULTS_MAP[role]) { | ||
| return ROLE_DEFAULTS_MAP[role].selector; | ||
| } | ||
|
|
||
| return FOCUSABLE_SELECTOR; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve whether disabled items should be skipped during navigation. | ||
| * | ||
| * Priority: explicit config.skipDisabled > role-aware default > true. | ||
| * | ||
| * @param {HTMLElement} el - The focusgroup container element | ||
| * @param {{ skipDisabled: boolean|null }} config - Parsed focusgroup config | ||
| * @returns {boolean} | ||
| */ | ||
| export function resolveSkipDisabled (el, config) { | ||
| if (config.skipDisabled !== null) return config.skipDisabled; | ||
|
|
||
| const role = el.getAttribute('role'); | ||
| if (role && ROLE_DEFAULTS_MAP[role]) { | ||
| return ROLE_DEFAULTS_MAP[role].skipDisabled; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
These are functions, not constants. Should probably just be moved to a different file to avoid confusion.
There was a problem hiding this comment.
extracted into focusgroup_utils.js
Nesting is not recommended, though technicaly possible. Each Distinct axes is effectively the workaround if nested is truly needed. The treeview example I did demonstrates this: directive owns vertical (Up/Down) by default, and I created custom script to manage left/right for DtCollapsible's expand/collapse with 2D grid navigation is the proper answer for most "I want to nest" cases. When someone thinks they need nested focusgroups, it's probably a grid where Up/Down and Left/Right mean different things (rows vs columns). 2D Grid could be a future token extension (e.g., I've added language in the doc to discourage nested use. |
|
I threw in a blog post to announce this directive: https://dialtone.dialpad.com/deploy-previews/pr-1187/dialtone/whats-new/ |
|
✔️ Deploy previews ready! |
Brad Paugh (braddialpad)
left a comment
Brad Paugh (braddialpad)
left a comment
There was a problem hiding this comment.
Thanks for this, it's really cool. Approving with one additional question.
Does it also make sense to use this component as a "focus trap" component where we want to trap the focus within a certain area, but don't necessarily want to use arrow keyboard navigation? ex/ what we do in popover/modal
Nah. This directive makes the container a single What does sound very reasonable: a Dialtone And to that end, we should consider combing through our products to discover what is done over and over, what should we make an equivalent available... functions, utils, composables, etc... |
2 participants
🛠️ Type Of Change
📖 Jira Ticket
DLT-3285
DLT-3042
📖 Description
New Vue directive
v-dt-focusgroupthat declaratively adds roving tabindex to any composite widget container. Arrow-key cycling, Home/End, looping, focus memory, disabled-item skipping, RTL support — all without writing keyboard event handlers.Token syntax
Object syntax for advanced
Role-aware defaults infer the item selector and disabled behavior from the container's
roleattribute (tablist→[role="tab"],listbox→[role="option"], etc.).Also includes:
focusgroup-requires-role,focusgroup-requires-label) as accessibility guardrails/functions-and-utilities/) listing all consumer-facing directives, functions, and utilities — driven by_data/vue-utilities.json<dt-icon name="storybook-color">Does NOT:
keyboard_list_navigationmixin (different pattern — activedescendant)📦 Cross-Package Impact
dialtone-vueeslint-plugin-dialtonedialtone-documentation💡 Context
Roving tabindex is reimplemented from scratch in 6+ Dialtone components with inconsistent feature support. Consumers building custom widgets have no primitive for this. A loose version was built in Beacon and proved the need. This directive gives every team a confident, one-line way to add keyboard cycling to toolbars, tablists, menus, contact lists, and data tables.
Benefits all users — not just accessibility or power users. Arrow-key navigation is standardized, faster, and more predictable than Tab for navigating grouped controls.
Aligned with the Open UI Scoped Focusgroup proposal so the mental model transfers when the native HTML attribute ships.
📝 Checklist
For all PRs:
For all Vue changes:
🔮 Next Steps
useFocusGroup) for DtTabGroup/DtSegmentedControl internal adoption🔗 Sources
📷 Screenshots / GIFs
vue-utilities.mp4
focus-group-stories.mp4
Adds v-dt-focusgroup Vue directive for declarative roving tabindex (arrow/Home/End, looping, memory, RTL, disabled-item skipping, role-aware defaults), two ESLint accessibility rules with tests, Storybook stories/MDX docs and recipes, Vue Utilities docs/site-nav updates, and package exports/Storybook registration.