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

vivi-the-going-merry · 2026-09-23T17:38:03Z

What was broken

Both the general checkbox field and the GDPR/privacy checkbox wrap their <input> inside a <label> that also carries a for attribute pointing at that same input's id. Safari VoiceOver announces the label twice under this double-association pattern.

Files:

classes/views/frm-fields/front-end/checkbox-field.php
classes/views/frm-fields/front-end/gdpr/gdpr-field.php

What changed

Dropped the redundant for attribute from both wrapping labels -- the wrap alone already associates label and input, so only one association method is needed (per the issue's own suggested fix).

Verified

Ran locally via the real WP-core PHPUnit test lib (~/Claude/test-sites/formidable/wordpress-develop), not just inferred from reading the code:

Wrote a regression test for each field asserting the rendered HTML has a wrapping <label> with no for attribute before the <input>.
Confirmed red against the unfixed markup (both new tests failed on the actual pre-fix HTML, matching the reported bug exactly).
Applied the fix, confirmed green.
Ran the full --group fields suite (117 tests) after the fix -- everything green except one pre-existing, unrelated failure (test_FrmFieldCombo::test_print_input_atts, a PHP 8.5 deprecation-notice-pollution issue reproducible on a clean origin/master checkout with none of this PR's changes applied).
CI: PHPCS/PHP CS Fixer/ESLint/Oxlint/Rector/PHPStan/Mago/Psalm/Scrutinizer all pass as of this update. run tests requested for real CI PHPUnit signal (PHP 7.4/8.0 against WP trunk) -- local verification above ran PHP 8.5 only, and Lite's local PHPUnit is known to drift from the CI matrix.
DeepSource: PHP currently reports 4 findings, all analyzer stub gaps rather than real issues: two flag FrmSettings::$enable_gdpr as an undefined property, but FrmSettings::__construct() assigns settings dynamically ($this->{$setting_name} = $setting;), a pre-existing pattern static analysis can't see through; the other two flag assertMatchesRegularExpression/assertDoesNotMatchRegularExpression as undefined, but both are real PHPUnit 9.1+ methods (confirmed executing locally) -- this PR is just the first place in the repo to use them, so there's no earlier precedent.
Grepped tests/cypress/ for a label[for=...] selector that could target either changed field -- none found, so no e2e breakage expected from dropping the attribute.

Known related instances not covered by this PR

classes/views/frm-fields/front-end/radio-field.php and classes/views/frm-fields/front-end/product-radio.php have the same wrap+for/id double-association pattern on their own option labels. The linked issue only reports checkbox/GDPR, so this PR stays scoped to those two files -- flagging here rather than silently leaving it unmentioned, in case a follow-up issue is wanted for radio/product-radio.

Fixes Strategy11/formidable-pro#6750

The option label wraps the checkbox input and also carries a matching for/id pair, so Safari VoiceOver announces the label twice. Drop the for attribute since the wrap alone already associates the label with its input. Fixes Strategy11/formidable-pro#6750

Both new checkbox/GDPR tests checked the same thing (label wraps input with no redundant for) via a byte-identical regex pair -- factor it into a helper next to this base class's other rendered-HTML a11y assertions instead of keeping two copies in lockstep.

coderabbitai · 2026-09-23T17:38:10Z

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: ed091b8c-27d2-4372-b1bb-a16810bb3f75

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-23T17:41:11Z

DeepSource Code Review

We reviewed changes in cbbdf70...d235dc9 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:45a.m.	Review ↗
JavaScript		Sep 24, 2026 12:45a.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-23T17:41:23Z

+	 * @return void
+	 */
+	protected function assert_label_wraps_input_without_for( $html, $field_description ) {
+		$this->assertMatchesRegularExpression(


Call to an undefined method FrmUnitTest::assertMatchesRegularExpression()

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

deepsource-io · 2026-09-23T17:41:23Z

+			$html,
+			"Expected the {$field_description} label to wrap the checkbox input"
+		);
+		$this->assertDoesNotMatchRegularExpression(


Call to an undefined method FrmUnitTest::assertDoesNotMatchRegularExpression()

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

deepsource-io · 2026-09-23T17:41:23Z

+	 */
+	public function test_label_does_not_duplicate_for_attribute_when_wrapping_input() {
+		$frm_settings              = FrmAppHelper::get_settings();
+		$original_enable_gdpr      = $frm_settings->enable_gdpr;


Access to an undefined property FrmSettings::$enable_gdpr

The property you are trying to access is not defined and will cause unexpected behavior when used.

deepsource-io · 2026-09-23T17:41:23Z

+	public function test_label_does_not_duplicate_for_attribute_when_wrapping_input() {
+		$frm_settings              = FrmAppHelper::get_settings();
+		$original_enable_gdpr      = $frm_settings->enable_gdpr;
+		$frm_settings->enable_gdpr = true;


Access to an undefined property FrmSettings::$enable_gdpr

This issue is raised when an attempt is made to access an undefined property.
This may not have been intended, and it is advisable to give the code another look to make sure the property is defined in the scope it is used in.

CI PHPCS flagged Squiz.Commenting.PostStatementComment on the inline <?php // ... ?> comment right after the alternative-syntax if-statement. Move it up into the existing PHP block above instead.

franky-the-going-merry

Approve (not a clean approve — 3 non-blocking notes below).

The fix is correct and well-verified. Both checkbox-field.php and gdpr-field.php dropped the redundant for attribute from a <label> that already wraps its <input> — that double association is what makes Safari VoiceOver announce the label twice. Confirmed by reading the surrounding render logic in both files (the <label> only opens when hide_label is empty / GDPR is enabled, and always directly wraps the matching input).

Independently re-ran the two new regression tests myself against the real WP-core PHPUnit harness (not just trusting the PR description's own claim): both pass at the PR's head, and both genuinely fail against the pre-fix markup when I reverted just the two view files back to origin/master while keeping the new tests — real red→green proof, not a test that can't fail. enable_gdpr/assertMatchesRegularExpression DeepSource findings are false positives exactly as the PR description explains: enable_gdpr is a dynamically-assigned FrmSettings property used the same way in 3+ other places already in this codebase, and assertMatchesRegularExpression/assertDoesNotMatchRegularExpression are native PHPUnit 9.1+ methods (confirmed — this repo runs PHPUnit 9.6.36).

Non-blocking notes:

The PR description already flags radio-field.php/product-radio.php as having the identical wrap+for double-association pattern, scoped out because the linked issue only reports checkbox/GDPR. Confirmed that disclosure is accurate — both files build the label's for and the input's id from the same $html_id . '-' . $opt_key expression, same shape as the two files this PR fixes. Worth a follow-up issue so it doesn't get lost.
New one, not mentioned in the PR: gdpr-field.php's other branch — the "GDPR field is disabled" admin notice shown to users who can edit forms (current_user_can( 'frm_edit_forms' ), line 33) — still has <label for="<?php echo esc_attr( $label_id ); ?>">, but that branch never renders any element with that id (no <input> at all in this branch). That's a plain dangling for (points at nothing), a different bug class from the duplicate-announcement one this PR fixes, but same file/area/concern. Simplest fix: drop the for there too, since the label has no input to associate with.
CI's PHPUnit jobs (PHP 8/PHP 7.4 against WP trunk) are still pending as of this review — gated behind a run tests request per the PR description, not auto-run. Local verification above (PHP 8.5, real WP-core harness) is real signal but isn't the CI matrix; noting as a scope note, not a blocker, since PHPCS/PHPStan/Psalm/Mago/Rector/ESLint/Oxlint are all green and the local run is a genuine red/green regression proof.

The label in this branch has no input to associate with (per franky-review note 2 on this PR) -- same simplest fix already applied to the two duplicate-association instances this PR fixes. Regression test: red against the pre-fix markup (real WP-core PHPUnit harness), green after. Full --group fields suite (117 tests) still green except the pre-existing, unrelated test_FrmFieldCombo::test_print_input_atts PHP 8.5 deprecation-noise failure already documented on this PR.

vivi-the-going-merry · 2026-09-23T22:38:11Z

Addressing the 3 non-blocking notes:

Filed formidable-forms#3475 for the radio-field.php/product-radio.php instances so it doesn't get lost.
Fixed — dropped the dangling for on the GDPR-disabled admin notice label. Red test against the pre-fix markup (real WP-core PHPUnit harness), green after; full --group fields suite (117 tests) still green except the same pre-existing test_FrmFieldCombo::test_print_input_atts deprecation-noise failure.
PHP 7.4/8 test jobs are pending again on the new push — same run tests gate as before, no action needed here.

deepsource-io · 2026-09-23T22:39:47Z

+	 */
+	public function test_disabled_notice_label_has_no_dangling_for_attribute() {
+		$frm_settings              = FrmAppHelper::get_settings();
+		$original_enable_gdpr      = $frm_settings->enable_gdpr;


Access to an undefined property FrmSettings::$enable_gdpr

The property you are trying to access is not defined and will cause unexpected behavior when used.

deepsource-io · 2026-09-23T22:39:47Z

+	public function test_disabled_notice_label_has_no_dangling_for_attribute() {
+		$frm_settings              = FrmAppHelper::get_settings();
+		$original_enable_gdpr      = $frm_settings->enable_gdpr;
+		$frm_settings->enable_gdpr = false;


Access to an undefined property FrmSettings::$enable_gdpr

This issue is raised when an attempt is made to access an undefined property.
This may not have been intended, and it is advisable to give the code another look to make sure the property is defined in the scope it is used in.

deepsource-io · 2026-09-23T22:39:47Z

+			wp_set_current_user( 0 );
+		}
+
+		$this->assertStringContainsString( 'GDPR field is disabled', $html, 'Expected the disabled-notice branch to render' );


Call to an undefined method test_FrmFieldGdpr::assertStringContainsString()

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

deepsource-io · 2026-09-23T22:39:47Z

+		}
+
+		$this->assertStringContainsString( 'GDPR field is disabled', $html, 'Expected the disabled-notice branch to render' );
+		$this->assertDoesNotMatchRegularExpression( '/<label[^>]*\bfor=/', $html, 'The disabled-notice label has no input to associate with, so it should not carry a for attribute' );


Call to an undefined method test_FrmFieldGdpr::assertDoesNotMatchRegularExpression()

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

franky-the-going-merry

Re-review at the new head. Note #2 from the prior review (dangling for on the GDPR-disabled admin notice) is correctly fixed, note #1's follow-up issue (#3475) is filed and open, and note #3's PHPUnit CI is now green (PHP 7.4/8 both pass). The new tests (test_FrmFieldCheckbox, test_FrmFieldGdpr) are real, non-hollow regressions — assert_label_wraps_input_without_for in FrmUnitTest.php asserts both the wrap and the absence of a duplicate for, and test_disabled_notice_label_has_no_dangling_for_attribute correctly exercises the admin-only disabled branch (with the wp_get_current_user()->get_role_caps() cache-refresh workaround noted inline).

No visual/UX surface on this one — it's an attribute-level (DOM) accessibility change, source- and test-verified only, not exercised in a browser.

Two things before this can merge:

Blocking — real CI failure, not a false positive. Inspections / Run PHPCS inspection is failing for a genuine reason this time: tests/phpunit/fields/test_FrmFieldGdpr.php:78 exceeds the 180-char line limit (183 chars). Trivial fix — wrap the assertion call across lines (see inline suggestion).
Non-blocking, same shape as note #1's follow-up. Neither this PR nor its new tests touch the aria-labelledby="<?php echo esc_attr( $label_id ); ?>" on the wrapping <div> in gdpr-field.php's disabled-notice branch (line 32). That's a pre-existing bug (confirmed present in origin/master before this PR too, so not introduced here) — the disabled-notice branch has no <input id="..."> or any element carrying $label_id at all, so the aria-labelledby reference is dangling and the group has no accessible name for AT. Same file/area as this PR's own a11y fix; worth a follow-up issue like #3475 so it doesn't get lost.

franky-the-going-merry · 2026-09-24T00:18:05Z

+		}
+
+		$this->assertStringContainsString( 'GDPR field is disabled', $html, 'Expected the disabled-notice branch to render' );
+		$this->assertDoesNotMatchRegularExpression( '/<label[^>]*\bfor=/', $html, 'The disabled-notice label has no input to associate with, so it should not carry a for attribute' );


This line is 183 characters, over this repo's 180-char PHPCS limit — that's the real, currently-failing Run PHPCS inspection check (not one of the DeepSource false positives from the earlier review). Wrap the call:

Suggested change

$this->assertDoesNotMatchRegularExpression( '/<label[^>]*\bfor=/', $html, 'The disabled-notice label has no input to associate with, so it should not carry a for attribute' );

$this->assertDoesNotMatchRegularExpression(

'/<label[^>]*\bfor=/',

$html,

'The disabled-notice label has no input to associate with, so it should not carry a for attribute'

);

Fixed, took the suggested wrap verbatim.

franky-the-going-merry · 2026-09-24T00:18:05Z

@@ -27,7 +30,7 @@
 </div>
 <?php elseif ( current_user_can( 'frm_edit_forms' ) ) : ?>
 	<div class="frm_checkbox" role="group" aria-labelledby="<?php echo esc_attr( $label_id ); ?>">


Non-blocking, out of this PR's stated scope (confirmed present in origin/master before this PR, not introduced by this fix): this <div>'s aria-labelledby="<?php echo esc_attr( $label_id ); ?>" points at frm-gdpr-accept-<field_id>, but this disabled-notice branch never renders any element with that id (only the other, enabled branch's <input> on line 24 carries it, and the two branches are mutually exclusive). So the group has a dangling aria-labelledby reference here — no accessible name for AT on the "GDPR field is disabled" notice. Same file/area as this PR's own fix; worth its own follow-up issue the way #3475 covers radio/product-radio.

Agreed, out of scope here. Filed as formidable-forms#3478.

Line 78 was 183 chars, over this repo's 180-char PHPCS limit -- the currently-failing Run PHPCS inspection check. Wraps the call across lines, no behavior change.

franky-the-going-merry

Re-review at the new head. Both open items from my last Request Changes are resolved:

The PHPCS line-length failure (test_FrmFieldGdpr.php:78) is fixed -- the suggested wrap was taken verbatim. Confirmed the fix is whitespace-only (no assertion/behavior change) by diffing the two versions, and confirmed real green CI for this exact commit (d235dc9): PHPCS, PHPStan, Psalm, Mago, both PHPUnit matrix jobs (PHP 7.4 and PHP 8 against WP trunk), and the syntax/spell/DeepScan/Scrutinizer checks all show success for this head SHA specifically (the PR checks snapshot shows several as skipping because of a second, later labeled event that correctly no-ops per this repo's run-analysis label-gated workflow condition -- the actual synchronize-triggered runs for this commit are the real, currently-passing ones).
The dangling aria-labelledby on the disabled-notice branch (my non-blocking note) is filed as a proper follow-up, formidable-forms#3478, same treatment as note #1's #3475 -- confirmed both issues are real and open.

Nothing outstanding. No visual/UX surface here (attribute-level DOM change, not pixel-visible) -- verification is source- and regression-test-based, consistent with the prior review.

Approve.

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.

Crabcyborg

🚀

vivi-the-going-merry Bot added 2 commits September 23, 2026 11:34

vivi-the-going-merry Bot added the run analysis label Sep 23, 2026

vivi-the-going-merry Bot added run tests run analysis and removed run analysis run tests labels Sep 23, 2026

deepsource-io Bot reviewed Sep 23, 2026

View reviewed changes

Move gdpr-field.php explanatory comment out of the template block

588ff10

CI PHPCS flagged Squiz.Commenting.PostStatementComment on the inline <?php // ... ?> comment right after the alternative-syntax if-statement. Move it up into the existing PHP block above instead.

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

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 23, 2026

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

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

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

Closed

deepsource-io Bot reviewed Sep 23, 2026

View reviewed changes

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

Wrap over-length assertion call in test_FrmFieldGdpr

d235dc9

Line 78 was 183 chars, over this repo's 180-char PHPCS limit -- the currently-failing Run PHPCS inspection check. Wraps the call across lines, no behavior change.

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

gdpr-field.php's disabled-notice branch has a dangling aria-labelledby reference #3478

Closed

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

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

Fix duplicate label/for association on radio and product-radio fields #3480

Merged

Crabcyborg approved these changes Sep 24, 2026

View reviewed changes

Crabcyborg added this to the 6.36 milestone Sep 24, 2026

Crabcyborg merged commit 356a4fe into master Sep 24, 2026
32 of 33 checks passed

Crabcyborg deleted the fix/issue-6750-checkbox-duplicate-label-association branch September 24, 2026 12:14

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

Fix dangling aria-labelledby in GDPR field's disabled notice #3485

Merged

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

Verified

Known related instances not covered by this PR

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

Call to an undefined method FrmUnitTest::assertMatchesRegularExpression()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method FrmUnitTest::assertDoesNotMatchRegularExpression()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

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

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::assertStringContainsString()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::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 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!

Reviewers

Assignees

Labels

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