Improve editor keyboard navigation and accessibility - #197
Conversation
Signed-off-by: milos-plavsic <225993441+milos-plavsic@users.noreply.github.com>
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary) Thanks for this — it's an unusually careful accessibility PR. The tablist implementation follows the APG pattern correctly, the markTarget/clearTarget state restoration is genuinely well done, and the regex translation fixes a real bug (the old email pattern was invalid under the v flag, so browsers were silently ignoring it). Four independent review passes agreed on those strengths.
The findings below cluster around one theme: several code paths destroy or hide the element that currently has keyboard focus without restoring it (error-list rebuilds, wizard Finish, array-item removal). Each has a small fix.
Two smaller notes that don't warrant line comments: toHtmlPattern returns null for a leading in-class hyphen like [-abc] (fails safe, just loses native validation — fine to leave), and the PR description bullet about identifiers staying "stable after removal and reindexing" doesn't quite match the code — generateId still randomizes ids on every rebuild and the new array aria-labels are positional, so it's worth rewording that bullet.
Signed-off-by: milos-plavsic <225993441+milos-plavsic@users.noreply.github.com>
|
Addressed the focus-loss paths for periodic validation, Wizard Finish, document-level Wizard errors, and array removal. Also added the YAML preview region role, restored the clipped section-toggle focus ring, rejected unsupported POSIX character classes, retained array positions in validation-group names, and added visual treatment for array-level errors. Regression coverage is retained through focused helper tests and source-wiring checks. Browser-level coverage remains a separate follow-up. The non-blocking toast announcement concern is deferred to that follow-up. |
jmeridth
left a comment
There was a problem hiding this comment.
🤖 All review findings addressed in a4e7d5f and verified against the diff: the focus-loss paths (periodic error-list rebuild, wizard Finish, document-level wizard errors, array removal) are fixed, the YAML preview region is properly named, the section-toggle focus ring is restored, POSIX classes fail safe, and the labeling/visual-indicator notes are covered with tests. The non-blocking toast announcement item is reasonably deferred to the browser-coverage follow-up. Nice work.
|
@milos-plavsic nice work on this. We appreciate it. @scovetta do you want to take a look? @eddie-knight? |
Purpose
Make the primary Security Insights editor workflow understandable and operable with keyboard-only input and assistive technology.
This is the first incremental pull request from #196. Responsive-layout changes and browser-level regression coverage remain separate follow-up changes.
Changes
Why
The editor previously relied on visual state and pointer interaction in several critical paths. Validation messages were displayed visually but were not consistently connected to the affected controls, and Wizard navigation did not expose a complete semantic model to assistive technology.
These changes establish the accessible interaction foundation without including the responsive-layout or Playwright/CI work planned as later increments.
Validation
Part of #196.