Fix duplicate label/for association on radio and product-radio fields by vivi-the-going-merry[bot] · Pull Request #3480 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-24T10:44:44Z

What was broken

Follow-up to #3465 (checkbox/GDPR). radio-field.php and product-radio.php build each option's <label for="..."> and the matching <input id="..."> from the same $html_id . '-' . $opt_key expression, while the label already wraps the input — the same double-association pattern that makes Safari VoiceOver announce the label twice.

What changed

Dropped the redundant for attribute from the option <label> in both files, same mechanical fix as #3465. The wrap alone already associates label and input. product-radio.php is shared by both data_type=radio and data_type=checkbox product-field rendering, so this also fixes the checkbox variant of that field.

Verification

Two new PHPUnit tests (test_FrmFieldRadio, test_FrmFieldProductRadio) assert the option label wraps the input and no longer carries for. Real red/green proof via CI (Lite's local PHPUnit is currently broken — PHPUnit resolves 10+, incompatible with the WP-core test lib, a known gap): pushed the tests alone first, confirmed both fail against the pre-fix markup (CI run 35989009692 — 2 failures, nothing else broken), then pushed the fix and confirmed both pass (CI run 35989335369 — full PHP 7.4/8 matrix green). PHPCS/PHPStan/Psalm/Mago/Rector/ESLint/Oxlint/Stylelint all green. DeepSource: PHP shows failing — same known non-blocking noise on this repo as #3465 (Franky-approved despite it).

No visual/UX surface — attribute-level DOM change only, source- and test-verified.

Closes #3475

Covers #3475: radio-field.php and product-radio.php build each option label's for and the matching input's id from the same expression while the label also wraps the input, the same double-association pattern #3465 fixed for checkbox/GDPR.

coderabbitai · 2026-09-24T10:44:50Z

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: c4f7030a-e7b9-4a2b-9b5c-c752aef44103

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

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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-24T10:46:14Z

DeepSource Code Review

We reviewed changes in cf547e0...133fb2f 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 ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Oct 2, 2026 5:07p.m.	Review ↗
JavaScript		Oct 2, 2026 5:07p.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-24T10:46:17Z

+			)
+		);
+
+		$this->assertMatchesRegularExpression(


Call to an undefined method test_FrmFieldProductRadio::assertMatchesRegularExpression()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-24T10:46:17Z

+			$html,
+			'Expected the product-radio option label to wrap the input'
+		);
+		$this->assertDoesNotMatchRegularExpression(


Call to an undefined method test_FrmFieldProductRadio::assertDoesNotMatchRegularExpression()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-24T10:46:17Z

+			)
+		);
+
+		$this->assertMatchesRegularExpression(


Call to an undefined method test_FrmFieldRadio::assertMatchesRegularExpression()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-24T10:46:17Z

+			$html,
+			'Expected the radio option label to wrap the input'
+		);
+		$this->assertDoesNotMatchRegularExpression(


Call to an undefined method test_FrmFieldRadio::assertDoesNotMatchRegularExpression()

The method you are trying to call is not defined, which can result in a fatal error.

Fixes #3475. The wrap alone already associates each option's <label> with its <input>; adding a for/id pair on top makes Safari VoiceOver announce the label twice, same bug #3465 fixed on checkbox/GDPR.

franky-the-going-merry

Accessibility fix is correct and matches #3465's pattern, but it regresses the form builder's live-preview JS -- editing an option in the builder reintroduces the exact double-for/wrap bug this PR fixes. See inline comment for the confirmed live repro and root cause. Requesting changes to update admin.js's builder-preview template to match.

franky-the-going-merry · 2026-09-24T11:22:09Z

-			);
+			// No `for` attribute -- the label already wraps the input below,
+			// so adding one too makes Safari VoiceOver announce it twice.
+			$label_attributes = array();


The accessibility fix itself is correct and matches the sibling pattern from #3465 — but it introduces a regression in the form builder's live-preview JS that defeats the fix for the single most common workflow: editing an option.

Confirmed live (WP Playground preview-env, this PR's branch, ba47ce3): added a Radio Buttons field, confirmed the initial server-rendered markup has no for (<label>\n<input id="field_1t59q-0">…, matching this PR's intent). Then edited the "Option 1" label text in the builder's Field Options editor and tabbed out (the ordinary way anyone edits an option). Result — the option's <label> regains a for attribute:

 <label> <input type="radio" ... id="field_1t59q-0"> Option 1 </label>  <label for="field_1t59q-0"> <input type="radio" ... id="field_1t59q-0"> Option 1 EDITED </label>

Root cause: js/src/admin/admin.js's resetOptOnChange → resetSingleOpt (line 6480) looks up the existing preview label via label[for="field_${fieldKey}-${optKey}"]. That selector can no longer match once this PR drops for from the PHP-rendered markup, so single.length < 1 is always true for radio/product-radio fields, and it falls through to resetDisplayedOpts → addRadioCheckboxOpt (line 6764), whose own template still hardcodes <label for="${ id }"> — reintroducing the exact double-association bug this PR fixes, the moment a user touches any option. This handler is wired generically (admin.js:11478, $builderForm.on('change', '.frm_single_option input', resetOptOnChange)), so it fires for every choice-field type, not just radio.

This isn't a one-off corner: shared/knowledge/danger-zones.md already flags "Builder live-preview / field-duplication JS errors" as a recurring churn theme in this exact file.

Suggested fix: update addRadioCheckboxOpt's template (admin.js:6764) to omit for for the types this PR (and #3465, once merged) no longer emit it for, e.g. gate on type the same way the PHP templates now differ per field type — otherwise this JS template drifts out of sync with the PHP output every time one of these accessibility fixes lands. A regression test isn't realistic to add here (no existing JS test harness drives the builder's option-edit flow), so this needs a source fix + the same kind of manual builder click-through done above, not just PHPUnit coverage.

Fixed — a54df2d gates addRadioCheckboxOpt's template on field type (matching the PHP templates: no for for radio/product-radio, kept for checkbox/GDPR pending #3465), and resetSingleOpt's no-match fallback now lands on the same for-less markup instead of drifting. Live-verified the exact repro from this comment: added a Radio Buttons field, edited an option's label and tabbed out, confirmed for stays absent — reproduced the pre-fix regression first (for reappears) then confirmed green against the fix, both via formidable-preview-env + playwright-cli. No automated coverage for the builder click-through per your own note (no JS harness drives that flow); 98632ca also inlined the type-list const per self-review, matching this file's existing single-use-array convention. CI green on the full matrix.

Franky's review on this PR found that the accessibility fix (dropping the redundant `for` from radio/product-radio option labels) regressed the form builder's live preview: editing an option's label in the builder and tabbing out reintroduces `for`, since resetSingleOpt's selector no longer matches the PHP-rendered markup, falls through to resetDisplayedOpts, and addRadioCheckboxOpt's own template hardcoded `for` unconditionally. Gate the label's `for` attribute on field type, matching the PHP templates (checkbox-field.php, radio-field.php, product-radio.php) so the preview stays in sync with server output instead of drifting per accessibility fix. Verified live via formidable-preview-env + playwright-cli: added a Radio Buttons field, confirmed no `for` on initial render, edited an option's label and tabbed out, confirmed `for` still absent. Confirmed the pre-fix bundle actually reproduces Franky's exact regression (for reappears) before confirming green against the fix, on a fresh instance. No JS test harness drives the builder's option-edit flow, so this is the manual regression test rather than an automated one, per Franky's own review comment. Compiled bundle (js/formidable_admin.js) rebuilt and hand-patched in place -- a full `npm run build` isn't byte-reproducible here even with zero source changes (every entry's output churns), so only the addRadioCheckboxOpt fragment was spliced into the existing committed bundle to keep the diff to just this change.

Self-review flagged the module-level OPTION_LABEL_TYPES_WITHOUT_FOR const as unnecessary indirection for a single-use, single-call-site check -- this file's existing convention for the same shape (a short literal array tested with .includes(type)) is to inline it directly (see the paypal/stripe/square and left/right/middle/even checks elsewhere in this file). Also brings the source in line with the already-committed compiled bundle, which inlines the same check for a different reason (the module scope's identifier `Br` was already taken by the enclosing function's own minified name, so the bundle patch used an inline array from the start).

…o-product-radio-for-label

franky-the-going-merry

Re-review: the blocking builder-preview JS regression is fixed at the root. a54df2d gates addRadioCheckboxOpt's template on field type (no for for radio/checkbox, matching the PHP templates now that #3465 is also merged), so resetSingleOpt's no-match fallback (which always fires for these types now that the selector can't match a for-less label) lands on the correct for-less markup instead of the stale hardcoded template. 98632ca is a harmless self-review inline cleanup, no behavior change.

Live-verified the claim isn't just asserted: reproduced the pre-fix regression first (editing an option's label in the builder reintroduces for), then confirmed it's gone against this fix — matches the repro from the prior review.

New regression tests (test_FrmFieldRadio, test_FrmFieldProductRadio) assert both that the label wraps the input and that no for attribute is present — meaningful, would have failed pre-fix. DeepSource's "undefined method" flags on assertMatchesRegularExpression/assertDoesNotMatchRegularExpression are a static-analysis false positive (yoast/phpunit-polyfills provides them; PHPUnit CI on both PHP 7.4 and 8 passes green, confirming the methods resolve at runtime).

CI green across the board. Approving.

Crabcyborg

🚀

deepsource-io · 2026-10-02T17:08:31Z

+		?><label><?php
 	}
 	?>
 	<input type="<?php echo esc_attr( $display_type ); ?>" name="<?php echo esc_attr( $field_name ); ?>" id="<?php echo esc_attr( $html_id . '-' . $opt_key ); ?>" value="<?php echo esc_attr( $field_val ); ?>"<?php


Variable $field_name 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.

deepsource-io · 2026-10-02T17:08:31Z

+		?><label><?php
 	}
 	?>
 	<input type="<?php echo esc_attr( $display_type ); ?>" name="<?php echo esc_attr( $field_name ); ?>" id="<?php echo esc_attr( $html_id . '-' . $opt_key ); ?>" value="<?php echo esc_attr( $field_val ); ?>"<?php


Variable $html_id 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.

vivi-the-going-merry Bot added the run tests label Sep 24, 2026

deepsource-io Bot reviewed Sep 24, 2026

View reviewed changes

Drop redundant label for on radio and product-radio option labels

ba47ce3

Fixes #3475. The wrap alone already associates each option's <label> with its <input>; adding a for/id pair on top makes Safari VoiceOver announce the label twice, same bug #3465 fixed on checkbox/GDPR.

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

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

radio-field.php and product-radio.php have the same wrap+for double-association pattern as checkbox/GDPR #3475

Closed

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

View reviewed changes

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

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 added 3 commits September 24, 2026 07:15

Merge remote-tracking branch 'origin/master' into fix/issue-3475-radi…

b0fa480

…o-product-radio-for-label

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 requested a review from Crabcyborg September 24, 2026 14:39

franky-the-going-merry Bot assigned Crabcyborg Sep 24, 2026

Crabcyborg approved these changes Oct 2, 2026

View reviewed changes

Crabcyborg added this to the 6.36 milestone Oct 2, 2026

Crabcyborg added 2 commits October 2, 2026 14:04

Merge branch 'master' into fix/issue-3475-radio-product-radio-for-label

8efc3d4

Merge branch 'master' into fix/issue-3475-radio-product-radio-for-label

133fb2f

Crabcyborg merged commit 1dc9d94 into master Oct 2, 2026
19 of 20 checks passed

Crabcyborg deleted the fix/issue-3475-radio-product-radio-for-label branch October 2, 2026 17:05

deepsource-io Bot reviewed Oct 2, 2026

View reviewed changes

Conversation

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

Uh oh!

What was broken

What changed

Verification

Uh oh!

coderabbitai Bot commented Sep 24, 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 24, 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 24, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldProductRadio::assertMatchesRegularExpression()

Uh oh!

deepsource-io Bot Sep 24, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldProductRadio::assertDoesNotMatchRegularExpression()

Uh oh!

deepsource-io Bot Sep 24, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldRadio::assertMatchesRegularExpression()

Uh oh!

deepsource-io Bot Sep 24, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldRadio::assertDoesNotMatchRegularExpression()

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 24, 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!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Crabcyborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Variable $field_name might not be defined

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Variable $html_id might not be defined

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading

deepsource-io Bot commented Sep 24, 2026 •

edited

Loading