Repository navigation
Fix unlabeled SVGs in shared admin chrome (header logo, floating support-links button) - #3423
Conversation
Both render on every wp-admin page via shared chrome (admin-header.php's show_header_logo() and the floating-links widget), which is why IBM Equal Access flagged them on formidable-pro's White Labeling and Inbox settings tabs (formidable-pro#6733) even though neither tab's own content has any inline SVG. - show_header_logo(): the logo SVG is always wrapped in a link that already has its own adjacent screen-reader text, so mark the icon aria-hidden. - Floating links button: give the clickable div a real name/role/keyboard handling (role="button" + tabindex + Enter/Space) instead of just hiding its icon, since aria-label alone is invalid on a plain div and the button had no keyboard support at all. Verified with IBM's accessibility-checker CLI against the live rendered Inbox settings tab: both svg_graphics_labelled violations are gone, no new violations introduced.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Sep 22, 2026 10:14a.m. | Review ↗ |
| JavaScript | |
Sep 22, 2026 10:14a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
There was a problem hiding this comment.
Fix itself verified correct: confirmed both show_header_logo() call sites (admin-header.php, applications/header.php) already wrap the icon in a link with adjacent screen-reader-text, so aria-hidden on the logo SVG is safe. Confirmed the floating button's role="button"/tabindex/aria-label/keydown additions are all correct and that switchIconButton() only swaps the button's innerHTML, never touching the attributes set on the container div itself. No CI issues, no regressions found. Two non-blocking notes inline.
| bgHoverColor: '#F6FAFF', | ||
| logoIcon: ` | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="40" height="40" fill="none" viewBox="0 0 40 40"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="40" height="40" fill="none" viewBox="0 0 40 40" aria-hidden="true"> |
There was a problem hiding this comment.
Generalization gap: upgradeIcon/supportIcon/documentationIcon/notificationsIcon above (lines 23, 30, 38, 45) are the exact same shape this line just fixed for logoIcon -- a bare decorative <svg> next to a sibling that already carries the real accessible name (s11-floating-links.js:176-189 renders each as <a title="..."> with a visible .s11-floating-links-nav-text span). None of the 4 got aria-hidden="true".
Likely why the PR's own re-verification (checker run against the default/collapsed page state) didn't catch it: these only render once the nav menu is opened. A real user (or a checker run against the opened-menu state) would likely hit the same svg_graphics_labelled-shaped issue this PR is fixing here. Not blocking since it's unconfirmed live, but worth the same one-line fix on all 4 for consistency.
There was a problem hiding this comment.
Fixed in 789e958 — added aria-hidden=true to all 4 (upgradeIcon, supportIcon, documentationIcon, notificationsIcon), same pattern as logoIcon. Not independently live-verified against the opened-menu state (playwright-cli is unavailable in this unattended run), but each sits beside a sibling that already carries the visible accessible name, identical shape to the already-verified logoIcon case.
| // Create the icon button element | ||
| this.iconButtonElement = document.createElement( 'div' ); | ||
| this.iconButtonElement.classList.add( 's11-floating-links-logo-icon' ); | ||
| this.iconButtonElement.setAttribute( 'role', 'button' ); |
There was a problem hiding this comment.
Non-blocking, out of scope for the 2 violations this PR targets: this toggle button never sets aria-expanded, so a screen reader user can't tell from the button itself whether the nav menu is currently open or closed -- only the icon swaps. Worth adding aria-expanded (toggled alongside the s11-show-close-icon class in switchIconButton()) as a follow-up for full disclosure-pattern compliance.
There was a problem hiding this comment.
Agreed, out of scope for this PR. Filed as #3427 to track separately.
Same fix already applied to logoIcon in this PR — each of these 4 icons sits beside a sibling <a title=...> that already carries the visible accessible name (s11-floating-links.js), so the icon itself is purely decorative. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Method: in-place push |
There was a problem hiding this comment.
Re-reviewed at the new head (789e958) — the only change since my prior approval is the fix for the generalization-gap note: all 4 sibling icons (upgradeIcon/supportIcon/documentationIcon/notificationsIcon) now get aria-hidden="true", same pattern as the already-verified logoIcon. Confirmed each is wrapped in an <a> with a visible .s11-floating-links-nav-text span (createNavMenu()), so hiding the icon is safe. Live-verified in a running preview env (Pro active, so only the supportIcon/"Support & Docs" link renders) — opened the nav menu and confirmed via playwright-cli eval the rendered <svg> carries aria-hidden="true" with the visible text sibling intact:
The other 3 icons weren't independently live-rendered (this env only activates the Pro link set), but they go through the identical config-object → createNavMenu() pipeline and the source diff applies the byte-identical fix, so this is source-verified rather than a guess.
The other non-blocking note from my first pass (missing aria-expanded on the toggle button) was correctly moved out of scope into #3427 rather than left dangling on this PR.
Doing a fresh full-diff pass (not just the delta) turned up one real generalization gap in code this PR doesn't touch: FrmAppHelper::show_logo()/svg_logo() renders the same kind of unlabeled decorative SVG in FrmSolution.php's SMTP/upgrade header and FrmSMTPController.php, right next to sibling icons that do get an aria-label. Filed separately as #3428 (not blocking this PR, different function entirely).
Nothing outstanding on this PR itself — approving clean.
What was broken
IBM Equal Access flagged 4
svg_graphics_labelledviolations ("The SVG element has no accessible name") on formidable-pro's White Labeling and Inbox settings tabs (2 each) — Strategy11/formidable-pro#6733. Neither tab's own content template has any inline<svg>; both violations come from shared admin chrome defined here in Lite:FrmAppHelper::show_header_logo()echoes the Formidable logo SVG on every wp-admin page, with no accessible name and noaria-hidden.js/packages/floating-links/) is a plain<div>with a click handler and an injected SVG icon — no accessible name, norole, and no keyboard support at all.What changed
show_header_logo(): marks the logo SVGaria-hidden="true"— every caller already wraps it in a link with its own adjacent screen-reader text (admin-header.php,applications/header.php), so the icon itself is purely decorative.aria-label),role="button",tabindex="0", and a keydown handler (Enter/Space) alongside the existing click handler —aria-labelalone isn't valid on a plain<div>(IBM'saria_attribute_validrule confirmed this), and the button had zero keyboard support before this. Its injected logo/close SVGs are nowaria-hidden="true"since the parent carries the name.How this was verified
Reproduced formidable-pro#6733's exact violations locally: spun up a Formidable Lite + Pro instance via
formidable-preview-env, fetched the real rendered Inbox settings tab HTML (authenticated), and ran IBM'saccessibility-checkerCLI against it directly (flipping.achecker.yml'soutputFormattojsonper the issue's own suggested method). Confirmed bothsvg_graphics_labelledviolations at the exact xpaths matching the header logo and the floating-links button. After applying this fix and re-running the same check: 0 violations, no new violations introduced (in particular, confirmedaria-labelalone on the div would have created a newaria_attribute_validviolation, which is whyrole="button"+tabindexwere added rather than just the label).The White Labeling tab wasn't independently re-verified live (it requires a licensed-Pro state this local sandbox couldn't reach), but it renders the identical shared chrome (same
admin-header.php, same floating-links widget) unconditionally regardless of which settings tab is active, so the fix applies identically.Self-reviewed against 5 lenses (reuse, simplification, efficiency, altitude, security) — all clean; two suggestions (switch to a native-anchor keyboard pattern; absorb an unrelated pre-existing duplicate in
addOutsideClickListener()) were judged out of scope for this diff.Related: Strategy11/formidable-pro#6733 (2 of that issue's 5 violations are fixed by this PR; the remaining 1, on the Pro-only field-types preview page, is a separate front-end element not addressed here).
🤖 Generated with Claude Code