Skip to content

fix(designer): Improve toolbar accessibility with dynamic tabIndex and ARIA attributes #8727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ccastrotrejo merged 6 commits into from
Jan 23, 2026
Merged

fix(designer): Improve toolbar accessibility with dynamic tabIndex and ARIA attributes #8727

ccastrotrejo merged 6 commits into from
Jan 23, 2026

Conversation

@ccastrotrejo
Copy link
Contributor

@ccastrotrejo ccastrotrejo commented Jan 23, 2026 •
edited
Loading

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

This PR improves the keyboard navigation and accessibility of the designer toolbar controls. Previously, toolbar buttons could receive focus before workflow elements, making it confusing for keyboard users to navigate the designer.

Changes Made:

  1. Dynamic tabIndex calculation (Controls.tsx):

    • Added useStore hook from @xyflow/react to get node and edge counts
    • Implemented formula (nodes * 2) + edges + 1 to ensure toolbar controls receive focus after all workflow elements
    • Used useMemo for performance optimization

  2. ARIA attributes (Designer.tsx):

    • Added role="toolbar" and aria-label="Designer controls" to the toolbar container for proper screen reader identification

  3. CollapseExpandControl enhancement (collapseExpandControl.tsx):

    • Added optional tabIndex prop interface
    • Passed tabIndex to the underlying ControlButton

  4. CSS z-index fix (styles.less):

    • Added z-index: 5 to .msla-designer-tools to ensure the toolbar is above the canvas for proper visual stacking and click/focus hit-testing

  5. Unit tests (Controls.spec.tsx):

    • Added comprehensive tests for tabIndex calculation with various node/edge combinations
    • Tests verify all control buttons receive the same calculated tabIndex
    • Tests for edge cases (no nodes/edges, large workflows)

  6. E2E tests (tabFocus.spec.ts):

    • Added 4 new accessibility-focused tests:
      • Should focus toolbar controls after workflow elements
      • Should focus toolbar controls after closing panel
      • Should be able to tab through all toolbar controls
      • Should activate toolbar controls with keyboard

Impact of Change

  • Users: Keyboard users will now navigate through workflow nodes and edges first before reaching toolbar controls, following a logical tab order. Screen readers will properly identify and announce the toolbar.
  • Developers: CollapseExpandControl component now accepts an optional tabIndex prop for custom tab order control.
  • System: Minimal performance impact - uses memoized selector for node/edge counts to avoid re-renders on drag/move operations.

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: Designer with keyboard navigation (Chromium)

Contributors

Screenshots/Videos

CleanShot.2026-01-23.at.13.29.01.mp4

Copilot AI review requested due to automatic review settings January 23, 2026 18:35
@ccastrotrejo ccastrotrejo changed the title Added useNodes and useEdges hooks to calculate the count of workflow … fix: Improve toolbar accessibility with proper tab order and ARIA attributes Jan 23, 2026
@github-actions
Copy link

github-actions bot commented Jan 23, 2026 •
edited
Loading

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

✅ PR Title

  • Current: fix(designer): Improve toolbar accessibility with dynamic tabIndex and ARIA attributes
  • Issue: None — title is clear, follows conventional commit style, and accurately describes the change.
  • Recommendation: Keep as-is. It's concise and descriptive.

✅ Commit Type

  • Properly selected (fix).
  • Only one type is checked in the PR body which is correct.

✅ Risk Level

  • The PR body selects Medium and the PR has the label risk:medium — they match.
  • Advised risk (from code diff): risk:medium — appropriate given UI behavior changes that affect keyboard navigation, added runtime code, and many test additions.

✅ What & Why

  • Current: This section is present and detailed. It explains dynamic tabIndex computation, ARIA attributes added, component prop changes, CSS change, and tests added.
  • Issue: None critical. One small suggestion: briefly justify the chosen formula (nodes * 2) + edges + 1 in plain language (e.g., why nodes contribute *2) so reviewers who are not familiar with nodeIndex/nodeLeafIndex/edgeIndex logic can validate the reasoning quickly.
  • Recommendation: Add one sentence that explains the 2 multiplier for nodes (what it represents) to make the heuristic explicit in the PR body.

✅ Impact of Change

  • Impact is clearly described for Users, Developers, and System.
  • Recommendation: (Optional) call out any approximated performance/lot-of-nodes behavior if you expect very large flows (you do have a test for large workflows which is good — consider referencing that test here).
    • Users: Keyboard and screen-reader users benefit from logical tab order and a labeled toolbar.
    • Developers: CollapseExpandControl now accepts tabIndex; ControlButtons are given consistent tabIndex.
    • System: Minimal perf impact due to useMemo/useStore; note this is already mentioned.

✅ Test Plan

  • Unit and E2E tests are added in the diff (Controls.spec.tsx, Designer.spec.tsx, collapseExpandControl.spec.tsx, and e2e/designer/tabFocus.spec.ts). Manual testing is checked and described.
  • Assessment: Tests are present and validate both the tabIndex calculation and keyboard interactions. Good coverage.
  • Recommendation: E2E tests are currently skipped for Firefox (test.skip for firefox). If possible, add a follow-up issue or TODO in the PR body referencing Firefox coverage so that cross-browser behavior is tracked and resolved later.

⚠️ Contributors

  • Current: Empty section in PR body.
  • Assessment: This is optional and does not block the PR.
  • Recommendation: Consider adding a short contributors list (PMs, designers, reviewers) or at least a note to remember to credit collaborators. Example: Contributors: @ccastrotrejo (implementation), @someone (design review).

✅ Screenshots/Videos

  • A link is provided in the PR body. Good to have for visual/UX changes (even though this is mostly keyboard/accessibility changes).
  • Recommendation: If the link is an attachment that reviewers may not be able to access, consider embedding a short GIF or including a screenshot thumbnail in the PR description for convenience.

Summary Table

Section Status Recommendation
Title ✅ None — keep as-is.
Commit Type ✅ None — correct.
Risk Level ✅ None — matches label.
What & Why ✅ Add one-line rationale for the * 2 in the formula.
Impact of Change ✅ Optional: reference large-workflow test in this section.
Test Plan ✅ Add a TODO/follow-up for Firefox E2E coverage.
Contributors ⚠️ Add contributor credits (optional).
Screenshots/Videos ✅ Ensure attachment link is accessible; consider embedding a small GIF or thumbnail.

Final Notes and Actionable Recommendations

This PR meets the repository's PR-body template and contains appropriate unit and E2E tests. The labels and chosen risk level are consistent with the code changes and the advised risk level remains risk:medium.

Please consider the following small updates before merging (none are blocking):

  • In "What & Why", add a short explanation of why nodes are multiplied by 2 in the tabIndex formula so reviewers can quickly understand the mapping to nodeIndex/nodeLeafIndex semantics.
  • Add a short TODO or issue reference in the Test Plan noting that E2E tests are currently skipped for Firefox and should be addressed (or explain why Firefox is intentionally excluded).
  • Optionally list contributors in the Contributors section to credit reviewers/PM/Design.
  • Confirm that the screenshot/video attachment is accessible to reviewers; if not, include a brief inline GIF or screenshot.

Thanks — this is a well-structured PR with good test coverage for an accessibility fix. Please update the minor suggestions above if you want them addressed before merging.

Last updated: Fri, 23 Jan 2026 22:36:50 GMT

@ccastrotrejo ccastrotrejo changed the title fix: Improve toolbar accessibility with proper tab order and ARIA attributes fix(desinger): Improve toolbar accessibility with proper tab order and ARIA attributes Jan 23, 2026
Copilot AI reviewed Jan 23, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the designer’s control toolbar to improve layering/semantics and to place toolbar controls after workflow elements in the tab sequence.

Changes:

  • Add a tabIndex prop to CollapseExpandControl and pass a computed tabIndex to all toolbar buttons.
  • Compute a “toolbar last” tabIndex based on current node/edge counts via useNodes/useEdges.
  • Add toolbar semantics (role="toolbar") and adjust CSS stacking via z-index.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
libs/designer/src/lib/ui/styles.less Raises toolbar stacking (z-index) so tools appear above the canvas.
libs/designer/src/lib/ui/common/controls/collapseExpandControl.tsx Adds optional tabIndex support for the expand/collapse button.
libs/designer/src/lib/ui/Designer.tsx Adds toolbar ARIA semantics for the controls container.
libs/designer/src/lib/ui/Controls.tsx Computes a toolbar tabIndex from workflow element counts and applies it to all control buttons.

@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

1 similar comment
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

@ccastrotrejo ccastrotrejo added the risk:medium Medium risk change with potential impact label Jan 23, 2026
@ccastrotrejo
test(designer): add unit and E2E tests for toolbar accessibility tabI…
47a0dc1 
…ndex

- Add unit tests for Controls.tsx tabIndex calculation logic
  - Verify formula: (nodes * 2) + edges + 1
  - Test edge cases: empty workflow, nodes only, edges only, large workflows
  - Verify all toolbar buttons receive same tabIndex

- Extend E2E tabFocus.spec.ts with toolbar navigation tests
  - Verify toolbar is focusable after tabbing through workflow elements
  - Test tab order through all toolbar controls
  - Verify keyboard activation (Enter/Space) for toolbar buttons
  - Test toolbar focus after closing node panel
@ccastrotrejo ccastrotrejo changed the title fix(desinger): Improve toolbar accessibility with proper tab order and ARIA attributes fix(designer): Improve toolbar accessibility with proper tab order and ARIA attributes Jan 23, 2026
@ccastrotrejo
perf(designer): optimize toolbar re-renders with useStore selector
5f9e1d1 
- Replace useNodes/useEdges with useStore selectors for counts only
- Prevents toolbar re-renders on node/edge drag/move operations
- Fix misleading comment about z-index and tab order
@ccastrotrejo ccastrotrejo changed the title fix(designer): Improve toolbar accessibility with proper tab order and ARIA attributes fix(designer): Improve toolbar accessibility with dynamic tabIndex and ARIA attributes Jan 23, 2026
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

github-actions bot commented Jan 23, 2026

📊 Coverage check completed. See workflow run for details.

@ccastrotrejo ccastrotrejo merged commit 38c5550 into main Jan 23, 2026
13 checks passed
@ccastrotrejo ccastrotrejo deleted the ccastrotrejo/toolBarA11y branch January 23, 2026 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@rllyy97 rllyy97 rllyy97 approved these changes

Assignees

No one assigned

Labels

pr-validated risk:medium Medium risk change with potential impact

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ccastrotrejo @rllyy97