A11y: valid widget roles for Styles page tabbable elements by vivi-the-going-merry[bot] · Pull Request #3471 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-23T20:52:41Z

What was broken

IBM Equal Access flagged element_tabbable_role_valid 8 times on the Styles admin page (tests/cypress/e2e/admin-a11y.cy.js, CI log had ruleId/message only, no selectors — see Strategy11/formidable-pro#6756).

What changed

Field Shape's 4 radio-style labels (field-shape.php): each <label> had its own tabindex="0" (needed because the underlying native <input type="radio"> is display: none) but no ARIA role. A widget role can't go directly on <label> — IBM's aria_role_valid rejects any role there. Moved tabindex/role="radio"/aria-checked onto a new inner <span> wrapping the icon, and added aria-label (Regular / Rounded corners / Circle / Underline) so the new radio role has an accessible name.
Primary Color's colorpicker span (primary-color.php): had a redundant tabindex="0" duplicating the native focusability of its own nested <input type="text">. Removed the redundant tabindex rather than adding a role — role="button" here would wrap a focusable descendant (a new aria_descendant_valid violation) for no behavioral benefit, since the input was already the real interactive control.

No CSS or JS changes — matches the issue's stated scope.

How this was verified

checkIbmAccessibility's assertCompliance(false) means CI never actually fails on this rule, so a green Cypress run proves nothing either way. Reproduced locally instead, per fix-sop's documented exception for this rule: logged into a formidable-preview-env instance (Lite only, matching what cypress.yml checks out), curled the authenticated Styles page, injected <base href>, and ran IBM's own accessibility-checker CLI against the saved HTML.

Red: 8/8 element_tabbable_role_valid violations reproduced, exact XPath/snippet match for the two templates above.
Green: 0 element_tabbable_role_valid violations after the fix.
Compared every rule's violation count before/after to check for regressions: total ARIA violation-level findings actually dropped by 12 (8 target rule + 4 svg_graphics_labelled, fixed as a side effect since the new aria-label now gives the wrapped icon an accessible name too).
A few new potentialviolation-level input_label_visible/element_tabbable_unobscured advisories appeared (aria-label isn't a visible label) — same rule/severity already present elsewhere on this page for other controls, not a new failure class. Making the label visible would need a CSS/layout change, out of scope for this markup-only fix.

Refs Strategy11/formidable-pro#6756

element_tabbable_role_valid (IBM Equal Access), 8 instances all on the Styles admin page: - Field Shape's 4 radio-style labels had their own tabindex="0" (needed since the underlying native <input type="radio"> is display:none) but no ARIA role - and role can't go on <label> itself (IBM's aria_role_valid rejects any widget role there). Moved tabindex/role="radio"/aria-checked onto a wrapping <span> around the icon instead, with aria-label naming each shape so the new radio role still has an accessible name. - Primary Color's colorpicker span had a redundant tabindex="0" duplicating the native focusability of its own nested text input, with no valid role of its own. Removed the redundant tabindex rather than adding a role, since role="button" here would wrap a focusable descendant (a new aria_descendant_valid violation) for no behavioral benefit - the input was already the real interactive control. Verified via curl + IBM's accessibility-checker CLI against the real rendered Styles page (login-authenticated, <base href> injected for relative assets) per fix-sop.md's exception for this specific rule - CI's own checkIbmAccessibility only logs violations, never fails (assertCompliance(false)), so a green Cypress run doesn't prove anything. Confirmed all 8 element_tabbable_role_valid violations before the fix (exact XPath/snippet match), 0 after, with a live before/after diff of every rule's violation count to confirm no regression - total ARIA "violation"-level count actually dropped by 12 (8 target rule + 4 svg_graphics_labelled, fixed as a side effect of the new aria-label now covering the icon's own accessible name). A few new potentialviolation advisories appeared (aria-label isn't a visible label) - same rule/severity already present elsewhere on this page, not a new failure class, and adding visible text is a CSS layout change out of scope for this markup-only fix. Refs #6756

coderabbitai · 2026-09-23T20:52:47Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: f70b11f4-793c-4a5d-917d-1023e2f3db92

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

deepsource-io · 2026-09-23T20:55:01Z

DeepSource Code Review

We reviewed changes in cbbdf70...d41891d on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 24, 2026 12:23a.m.	Review ↗
JavaScript		Sep 24, 2026 12:23a.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.

deepsource-io · 2026-09-23T20:55:03Z

-		<label class="frm-flex-center" for="frm-field-shape-regular" tabindex="0">
-			<?php FrmAppHelper::icon_by_class( 'frmfont frm-square' ); ?>
+		<label class="frm-flex-center" for="frm-field-shape-regular">
+			<span tabindex="0" role="radio" aria-checked="<?php echo esc_attr( $field_value === 'regular' ? 'true' : 'false' ); ?>" aria-label="<?php esc_attr_e( 'Regular', 'formidable' ); ?>">


Variable $field_value might not be defined

A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

franky-the-going-merry

Request Changes. Two live-verified defects in this diff's own new code, plus non-blocking generalization notes.

1. aria-checked is set once at PHP render time and never updated when the user picks a different shape. Confirmed live in wp-preview-env (lite, PR branch loaded): clicked "Circle" — the real underlying <input id="frm-field-shape-circle"> correctly became checked: true, but its role="radio" span's aria-checked attribute stayed "false", and the previously-selected span's aria-checked="true" never cleared. radio-component.js's onRadioChange() (the only change handler wired to these inputs) moves the visual tracker and shows/hides extra elements, but never touches aria-checked — grepped the whole diff and radio-component.js for aria-checked, zero hits outside the PHP template. A screen reader user who changes shape and revisits the group is told the previous selection is still checked. Fix belongs in onRadioChange(): after moving the tracker, sync each sibling span's aria-checked to its associated input's .checked.

2. The new role="radio"/tabindex="0" spans are keyboard-focusable but not keyboard-operable. Live-verified: focused the "Regular" span via .focus() (matching real Tab behavior) and pressed both Enter and Space — neither selects the option (confirmed via the underlying input's .checked staying false); a plain mouse/label click does work (native label[for] click-forwarding). Grepped the repo for any keydown/keypress handler on this component — none exists. A keyboard-only user now tabs to an element a screen reader explicitly announces as an operable "radio button" (via the new role + aria-checked + aria-label this PR adds) and finds it does nothing — a more misleading state than the previous plain, roleless tabindex="0" label, which at least didn't claim to be a working widget. The 4 spans also aren't wrapped in a role="radiogroup" container and all carry tabindex="0" simultaneously rather than a roving tabindex — arrow-key navigation between options (expected once role="radio" is declared, per the ARIA APG radio-group pattern) doesn't exist either. Minimum fix: a keydown handler on each span forwarding Enter/Space to click() on its associated native radio input (or programmatically checking it and dispatching change), ideally alongside the roving-tabindex/arrow-key pattern the role implies.

Both confirmed independently by a backgrounded code-review pass, which I cross-verified against the live page and source before including.

Non-blocking generalization notes (same defect classes this PR fixes elsewhere in the same directory, left untouched):

colorpicker.php:5 has the identical redundant tabindex="0" on a <span> wrapping an already-focusable <input type="text"> that this PR removes from primary-color.php — same component family (FrmColorpickerStyleComponent), same fix would apply.
direction.php, align.php, and text-toggle.php still render <label tabindex="0"> with no role at all, wrapping a display:none radio input — the same element_tabbable_role_valid-shaped gap this PR's title ("valid widget roles for Styles page tabbable elements") describes fixing, just outside the specific 8 violations this fixture happened to hit. Given finding #2 above, simply copying this PR's role="radio" pattern onto them isn't sufficient on its own — worth fixing keyboard operability across all of them together rather than one template at a time.

CI: DeepSource: PHP" is red (PHP-W1066, "$field_value might not be defined" on field-shape.php:9") but confirmed a false positive — read FrmStyleComponent::load_view() directly: it sets $field_value = $this->field_value; in local scope immediately before include-ing this exact template, so the variable is always defined at render time. DeepSource's static analyzer doesn't trace include-based variable injection. Not treated as blocking.

…adio spans Franky's CHANGES_REQUESTED review on this PR found two live defects in the role="radio" spans this PR added to field-shape.php: aria-checked was only ever set at PHP render time and never updated when the selection changed, and the spans were focusable but had no keydown handler, so Enter/Space did nothing despite being announced as an operable radio widget. radio-component.js now syncs aria-checked across the group on every change (syncAriaChecked) and forwards Enter/Space activation from each span to its associated native input (onSpanKeydown), via a new getRadioSpan lookup. Compiled js/formidable-settings-components.js hand-patched with only this function's minified output spliced in (module IDs churn non-deterministically on a full rebuild, per TOOLS.md) — verified byte-identical to a full rebuild's functional change before splicing. Verified live in formidable-preview-env: label-click, Enter, and Space all correctly select the option and sync aria-checked across the group; no new console errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

vivi-the-going-merry · 2026-09-23T23:49:27Z

Method: in-place push
Pushed to: #3471 (branch fix/issue-6756-tabbable-role-valid, unchanged PR number)

Fixed both defects in radio-component.js:

syncAriaChecked() updates every span's aria-checked in the group on each change, not just at render time.
onSpanKeydown() forwards Enter/Space on a span to its native input (via a new getRadioSpan() lookup), so the role="radio" spans are keyboard-operable, not just focusable.

Verified live in formidable-preview-env: label-click, Enter, and Space all select the option and sync aria-checked across the group; no new console errors.

Left the non-blocking generalization notes (colorpicker.php:5, direction/align/text-toggle) untouched, as suggested — out of this issue's scope.

deepsource-io · 2026-09-23T23:50:33Z

+	 * @param {HTMLInputElement} radio - The native radio input.
+	 * @return {HTMLElement|null} The associated span, or null if the label doesn't wrap one.
+	 */
+	getRadioSpan( radio ) {


Expected 'this' to be used by class method 'getRadioSpan'

If a class method does not use this, it can sometimes be made into a static function. If you do convert the method into a static function, instances of the class that call that particular method have to be converted to a static call as well (MyClass.callStaticMethod())

deepsource-io · 2026-09-23T23:50:33Z

+	 * @return {HTMLElement|null} The associated span, or null if the label doesn't wrap one.
+	 */
+	getRadioSpan( radio ) {
+		const label = radio.labels && radio.labels[ 0 ];


Prefer using an optional chain expression instead, as it's more concise and easier to read

The optional chaining operator can be used to perform null checks before accessing a property, or calling a function.

deepsource-io · 2026-09-23T23:50:33Z

+	 * @param {KeyboardEvent}    event - The keydown event.
+	 * @param {HTMLInputElement} radio - The native radio input to activate.
+	 */
+	onSpanKeydown( event, radio ) {


Expected 'this' to be used by class method 'onSpanKeydown'

If a class method does not use this, it can sometimes be made into a static function. If you do convert the method into a static function, instances of the class that call that particular method have to be converted to a static call as well (MyClass.callStaticMethod())

franky-the-going-merry

Approve, with one non-blocking note. Both previously-flagged live defects are fixed and live-verified.

Live-verified in formidable-preview-env (lite branch loaded to this PR's head):

aria-checked now stays in sync — clicked through all 4 shapes; the correct span flips to "true" and every sibling flips to "false" on each change, confirmed via getAttribute('aria-checked') on all 4 spans after each click.
Keyboard operability — focused the Regular span and pressed Enter: input became checked, change fired, aria-checked synced. Repeated with Space on the Underline span: same result. Mouse/label click still works unchanged.

Screenshot of the widget in its current (Underline-selected) state:

CI: green except DeepSource: PHP/DeepSource: JavaScript. PHP is the same pre-existing field-shape.php:9 false positive already confirmed last round (FrmStyleComponent::load_view() sets $field_value before the include, DeepSource just doesn't trace it). JavaScript is new — inline note below, non-blocking.

Non-blocking, unchanged from last round: colorpicker.php:5's identical redundant tabindex="0" on an already-focusable input, and direction.php/align.php/text-toggle.php's still-unfixed unlabeled-icon-only-radio + missing keyboard-operability pattern this PR's title describes fixing more broadly — worth a follow-up pass across all of them together.

franky-the-going-merry · 2026-09-23T23:55:45Z

+	 * @return {HTMLElement|null} The associated span, or null if the label doesn't wrap one.
+	 */
+	getRadioSpan( radio ) {
+		const label = radio.labels && radio.labels[ 0 ];


DeepSource JS-W1044 (new, on this diff's own added code): prefer optional chaining here.

Suggested change

const label = radio.labels && radio.labels[ 0 ];

const label = radio.labels?.[ 0 ];

Fixed, took the suggested diff verbatim. Behaviorally identical (radio.labels is either a NodeList or null, same short-circuit either way), confirmed via ESLint against the real repo config plus a syntax check - no other change.

Separately, re-scanning surfaced two pre-existing DeepSource JS-0105 findings on the same file (getRadioSpan/onSpanKeydown "expected this to be used") that were already there before this push, not raised in your review - leaving those out of scope for this round.

DeepSource JS-W1044 non-blocking note. Behaviorally identical to the existing 'radio.labels && radio.labels[0]' form.

vivi-the-going-merry · 2026-09-24T00:27:27Z

Method: in-place push
Pushed to: #3471 (branch fix/issue-6756-tabbable-role-valid, unchanged PR number)

vivi-the-going-merry · 2026-09-24T00:28:06Z

Non-blocking notes (colorpicker.php:5 redundant tabindex, direction.php/align.php/text-toggle.php unlabeled-icon-only-radio pattern): leaving both out of scope for this PR, same as last round - framed as a follow-up pass across all of them together, not an ask for this diff.

franky-the-going-merry

Approve. Only change since the prior approval is a pure syntactic refactor — no behavior change.

Diff since last franky approval (d3052f7 → d41891d): getRadioSpan()'s radio.labels && radio.labels[0] became radio.labels?.[0]. Functionally identical (radio.labels is always a live NodeList on a real <input>, never undefined) — confirms the previously live-verified aria-checked sync and keyboard-operability fixes are untouched by this commit.

Non-blocking notes carried over unchanged from last round (still deliberately out of scope per the PR's own comments): colorpicker.php:5 redundant tabindex, and direction.php/align.php/text-toggle.php's unlabeled-icon-only-radio + missing keyboard-operability pattern — worth a follow-up pass across all of them together.

vivi-the-going-merry · 2026-09-24T00:57:58Z

Approved, no unresolved findings — the non-blocking notes carried over (colorpicker.php tabindex, the direction/align/text-toggle unlabeled-icon-radio pattern) are already deferred from earlier rounds, nothing new to action. Clearing vivi-pickup/vivi-working, ready for a human merge.

Crabcyborg

🚀

vivi-the-going-merry Bot added run analysis franky-review labels Sep 23, 2026

deepsource-io Bot reviewed Sep 23, 2026

View reviewed changes

franky-the-going-merry Bot requested changes Sep 23, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Sep 23, 2026

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 23, 2026

vivi-the-going-merry Bot added the franky-review label Sep 23, 2026

vivi-the-going-merry Bot added run analysis and removed vivi-working Vivi is actively working this run analysis labels Sep 23, 2026

deepsource-io Bot reviewed Sep 23, 2026

View reviewed changes

franky-the-going-merry Bot approved these changes Sep 23, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Sep 23, 2026

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 24, 2026

Use optional chaining in getRadioSpan()

d41891d

DeepSource JS-W1044 non-blocking note. Behaviorally identical to the existing 'radio.labels && radio.labels[0]' form.

vivi-the-going-merry Bot added franky-review and removed vivi-working Vivi is actively working this labels Sep 24, 2026

franky-the-going-merry Bot approved these changes Sep 24, 2026

View reviewed changes

franky-the-going-merry Bot removed the franky-review label Sep 24, 2026

franky-the-going-merry Bot added the vivi-pickup label Sep 24, 2026

franky-the-going-merry Bot mentioned this pull request Sep 24, 2026

A11y: give empty labels descriptive text (label_content_exists) #3470

Open

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 24, 2026

vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Sep 24, 2026

Crabcyborg approved these changes Sep 30, 2026

View reviewed changes

Crabcyborg added this to the 6.36 milestone Sep 30, 2026

Crabcyborg merged commit be17510 into master Sep 30, 2026
52 of 54 checks passed

Crabcyborg deleted the fix/issue-6756-tabbable-role-valid branch September 30, 2026 13:36

	const label = radio.labels && radio.labels[ 0 ];
	const label = radio.labels?.[ 0 ];

Conversation

vivi-the-going-merry Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was broken

What changed

How this was verified

Uh oh!

coderabbitai Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

deepsource-io Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Variable $field_value might not be defined

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot commented Sep 23, 2026

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Expected 'this' to be used by class method 'getRadioSpan'

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Prefer using an optional chain expression instead, as it's more concise and easier to read

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Expected 'this' to be used by class method 'onSpanKeydown'

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot commented Sep 24, 2026

Uh oh!

vivi-the-going-merry Bot commented Sep 24, 2026

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot commented Sep 24, 2026

Uh oh!

Crabcyborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vivi-the-going-merry Bot commented Sep 23, 2026 •

edited

Loading

coderabbitai Bot commented Sep 23, 2026 •

edited

Loading

deepsource-io Bot commented Sep 23, 2026 •

edited

Loading