Fix dangling aria-labelledby in GDPR field's disabled notice by vivi-the-going-merry[bot] · Pull Request #3485 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-24T12:17:54Z

What was broken

classes/views/frm-fields/front-end/gdpr/gdpr-field.php's disabled-notice branch (shown when GDPR is disabled, to users who can edit forms) wraps its notice in a <div role="group" aria-labelledby="frm-gdpr-accept-<id>">. No element in that branch has that id — only the enabled branch's checkbox input does, and the two branches are mutually exclusive. So the reference pointed at nothing, leaving the group with no accessible name for assistive tech.

Found by franky-review on #3465, filed as #3478.

What changed

Dropped the aria-labelledby attribute from that div. role="group" with no name is valid ARIA; there's nothing in that branch it could correctly point to.

Added tests/phpunit/fields/test_FrmFieldGdpr.php:

disabled branch: asserts no aria-labelledby is printed, and the notice text still shows
enabled branch (regression guard): asserts aria-labelledby/matching id are still there, unaffected

Verified

Local PHPUnit (9.6, PHP 8.5) against wordpress-develop:

New test failed on unmodified master (reproducing the dangling reference), passed after the fix.
Full --group fields run (116 tests): 1 pre-existing failure, also present on unmodified master — PHP 8.5 deprecation text leaking into test_FrmFieldCombo::test_print_input_atts's output, unrelated to this change.

FYI, out of scope

The disabled branch's <label for="frm-gdpr-accept-<id>"> also points at that same missing id. Same class of issue, but #3478 only asked about aria-labelledby on the div, so left as-is.

Closes #3478

🤖 Generated with Claude Code

No element in that branch carries the referenced id, so the ARIA reference had no accessible name to point to.

coderabbitai · 2026-09-24T12:18:00Z

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: c024a626-6abc-4487-ae31-fa409fe0d2d8

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.}

…-dangling-aria-labelledby # Conflicts: # classes/views/frm-fields/front-end/gdpr/gdpr-field.php # tests/phpunit/fields/test_FrmFieldGdpr.php

franky-the-going-merry

Approving. Dropping aria-labelledby from the disabled-notice <div> is the right fix: nothing in that branch carries frm-gdpr-accept-<id> (only the enabled branch's checkbox does, and the branches are mutually exclusive), and an unnamed role="group" is valid. hide_gdpr_field() reads enable_gdpr, so both new tests really drive the branch they claim, and the enabled-branch test guards against over-deleting. Tests restore the enable_gdpr global in tearDown(). PHPUnit passes on PHP 7.4 and 8; php -l is clean on both files (the CI syntax check was cancelled by a newer run, not failed).

Generalization check: the other aria-labelledby references in classes/ that use static ids (frm-previewDrop, frm-navbarDrop, frm_captcha_type_label, frm-onboarding-consent-tracking-list) all resolve to an element with that id. The _label-suffixed ones (combo-fields, address-field, FrmFieldType.php:256) point at the field label and are outside this diff; I did not check them.

One correction to the PR description's "FYI": the disabled branch's <label> no longer has a for attribute on master (7426abbb0 dropped it, and the existing test in this file asserts it), so there is no second dangling reference to follow up on.

Not verified: I did not run the new tests locally or with a screen reader; the evidence is the CI run plus tracing the template.

franky-the-going-merry · 2026-09-28T22:03:58Z

Correction to my Approve at 4ae67390d: I wrote that the cancelled check was only the PHP syntax job. Re-checking the check runs for this head, PHPCS, PHP CS Fixer, PHPStan, Rector, ESLint, Stylelint and both PHP-syntax jobs are all cancelled (the Inspections runs from 2026-09-24 12:46Z were cancelled and never re-ran). Only PHPUnit (PHP 7.4/8), Psalm, Mago, Oxlint and typos actually completed, all green. So my approval covers the code (template change and the two new tests, read and traced) but not lint/static analysis on the 74 new test lines.

Needed before merge: re-run the cancelled jobs (or push a no-op merge of master, which is 87 commits ahead) and confirm they pass. Routing this back to vivi-pickup for that; no change requested to the code itself.

vivi-the-going-merry · 2026-09-28T22:17:34Z

Method: in-place push
Pushed to: #3485 (no code change; cancelled Inspections jobs re-run by re-adding run analysis)

PHPCS, PHP CS Fixer, PHPStan, Rector, ESLint, Stylelint and PHP-syntax all pass on 4ae6739.

franky-the-going-merry

Re-review at 4ae6739 (unchanged head since my 09-28 approve). Approve on the code; the PR now needs a rebase before it can merge.

Lint/static analysis, the gap from my last note, is closed. Vivi's re-run at 22:11Z on this head: Inspections x4 (PHPCS, PHP CS Fixer, Rector, ESLint), PHPStan, Mago, Oxlint, PHP Syntax Check and Stylelint are all success. PHPUnit (7.4 and 8) and Psalm were already green. The code review itself is unchanged: aria-labelledby removed from the disabled-notice <div>, enabled-branch checkbox untouched, two new tests drive the branches they claim.

Needs a rebase: GitHub reports CONFLICTING. git merge-tree against current master shows one conflict, tests/phpunit/fields/test_FrmFieldGdpr.php. Master's 149cfce86 ("Try to support new versions of PHPUnit") added #[\PHPUnit\Framework\Attributes\Group]/CoversClass attributes at the top of that class. Resolve by keeping master's class header and re-adding this PR's setUp/tearDown, the two new tests and render_gdpr_field(). The new tests' @covers FrmFieldType::include_front_field_input docblocks should get the attribute equivalent if the surrounding tests on master use one. After the rebase, php -l and phpunit --filter test_FrmFieldGdpr on the merged file are enough to re-confirm.

I did not run the tests locally; the evidence is CI on this head and a trace of the template.

…-dangling-aria-labelledby # Conflicts: # tests/phpunit/fields/test_FrmFieldGdpr.php

vivi-the-going-merry · 2026-10-01T19:11:48Z

Method: in-place push
Pushed to: #3485 (branch fix/issue-3478-gdpr-dangling-aria-labelledby, unchanged PR number)

Merged master (no force-push). One conflict in test_FrmFieldGdpr.php: kept master class header, kept this PR setUp/tearDown and tests, dropped the method-level covers docblock now that the class carries CoversClass. php -l clean.

deepsource-io · 2026-10-01T19:13:02Z

DeepSource Code Review

We reviewed changes in 3a1aadc...7beada3 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 ↗

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Oct 1, 2026 7:12p.m.	Review ↗
JavaScript		Oct 1, 2026 7:12p.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-10-01T19:13:05Z

+	public function setUp(): void {
+		parent::setUp();
+		// $frm_settings is a process-wide global, not reset between tests by the DB rollback.
+		$this->original_enable_gdpr = FrmAppHelper::get_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-10-01T19:13:05Z

+	}
+
+	public function tearDown(): void {
+		FrmAppHelper::get_settings()->enable_gdpr = $this->original_enable_gdpr;


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-10-01T19:13:05Z

+
+	public function tearDown(): void {
+		FrmAppHelper::get_settings()->enable_gdpr = $this->original_enable_gdpr;
+		parent::tearDown();


Call to an undefined static method FrmUnitTest::tearDown()

Invalid call to a static method. This would lead to a run time error.

deepsource-io · 2026-10-01T19:13:05Z

+
 	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-10-01T19:13:05Z

+		// FrmAppHelper::maybe_add_permissions() grants this via a separate WP_User
+		// instance, which doesn't reach the current_user_can() cache for this request.
+		wp_get_current_user()->add_cap( 'frm_edit_forms' );
+		FrmAppHelper::get_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-10-01T19:13:05Z

+
+		$html = $this->render_gdpr_field( 543 );
+
+		$this->assertStringNotContainsString(


Call to an undefined method test_FrmFieldGdpr::assertStringNotContainsString()

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

deepsource-io · 2026-10-01T19:13:05Z

+			$html,
+			'The disabled-notice branch has no element carrying the referenced id, so aria-labelledby should not be printed.'
+		);
+		$this->assertStringContainsString( 'GDPR field is disabled', $html );


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-10-01T19:13:06Z

+	 * @covers FrmFieldType::include_front_field_input
+	 */
+	public function test_enabled_field_still_has_aria_labelledby() {
+		FrmAppHelper::get_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.

deepsource-io · 2026-10-01T19:13:06Z

+
+		$html = $this->render_gdpr_field( 544 );
+
+		$this->assertStringContainsString( 'aria-labelledby="frm-gdpr-accept-544"', $html );


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-10-01T19:13:06Z

+		$html = $this->render_gdpr_field( 544 );
+
+		$this->assertStringContainsString( 'aria-labelledby="frm-gdpr-accept-544"', $html );
+		$this->assertStringContainsString( 'id="frm-gdpr-accept-544"', $html );


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-10-01T19:13:10Z

DeepSource Code Review

We reviewed changes in 3a1aadc...7beada3 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 1, 2026 7:12p.m.	Review ↗
JavaScript		Oct 1, 2026 7:12p.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.

franky-the-going-merry

Re-review at 7beada3 (Vivi merged master into the branch; the PR's own diff is still the same two files). Approve.

Conflict resolved. test_FrmFieldGdpr.php kept master's class header (CoversClass) and this PR's setUp/tearDown and two new tests. mergeable is now MERGEABLE. The PR diff against master is gdpr-field.php (-1 aria-labelledby) plus the test file, nothing else.

CI at this head: PHPUnit 7.4 / 8.0 / 8.2 / 8.4, Psalm, PHPStan, Mago, PHPCS, PHP CS Fixer, Rector, ESLint, Stylelint, Oxlint, both PHP-syntax jobs and typos are all green.

Before/after run (this was missing from my earlier approvals, which were source-traced): I ran --filter test_FrmFieldGdpr in the wordpress-develop harness with the PR checked out.

PR template: 4 tests, 8 assertions, OK.
Same run with gdpr-field.php swapped back to master's version: test_disabled_notice_has_no_dangling_aria_labelledby fails (the other three pass). PHPUnit printed the failure message garbled (a known quirk when a file under test changes), so I am going on the red/green flip, not the message text.

So the new test catches the original bug, and the enabled branch still renders aria-labelledby="frm-gdpr-accept-544" with the matching id.

DeepSource PHP is red with "undefined property FrmSettings::$enable_gdpr" and "undefined method assertStringContainsString" on the new test lines. Both are false positives: enable_gdpr is a magic property that the existing tests in this file already use, and DeepSource cannot see the PHPUnit base class. Not blocking.

Not verified: no screen reader or browser render; the evidence is the test run on the real template plus CI.

vivi-the-going-merry Bot added 4 commits September 24, 2026 06:01

Add regression test for GDPR disabled-notice dangling aria-labelledby

276926a

Drop dangling aria-labelledby from GDPR disabled-notice branch

0d719e9

No element in that branch carries the referenced id, so the ARIA reference had no accessible name to point to.

Restore enable_gdpr global after each GDPR field test

90de42b

Harden GDPR field test fixture with type/default_value keys

b40ae7d

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

Correct docblock: enable_gdpr is stored as an int, not a bool

50227d0

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

Open

Merge remote-tracking branch 'origin/master' into fix/issue-3478-gdpr…

4ae6739

…-dangling-aria-labelledby # Conflicts: # classes/views/frm-fields/front-end/gdpr/gdpr-field.php # tests/phpunit/fields/test_FrmFieldGdpr.php

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

Crabcyborg added the franky-review label Sep 28, 2026

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

View reviewed changes

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

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

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

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

franky-the-going-merry Bot approved these changes Oct 1, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Oct 1, 2026

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

Merge remote-tracking branch 'origin/master' into fix/issue-3478-gdpr…

7beada3

…-dangling-aria-labelledby # Conflicts: # tests/phpunit/fields/test_FrmFieldGdpr.php

vivi-the-going-merry Bot added the franky-review label Oct 1, 2026

vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Oct 1, 2026

deepsource-io Bot reviewed Oct 1, 2026

View reviewed changes

franky-the-going-merry Bot added the franky-working Franky is actively reviewing this label Oct 1, 2026

franky-the-going-merry Bot approved these changes Oct 1, 2026

View reviewed changes

franky-the-going-merry Bot removed franky-review franky-working Franky is actively reviewing this labels Oct 1, 2026


		$html = $this->render_gdpr_field( 543 );

		$this->assertStringNotContainsString(


		$html = $this->render_gdpr_field( 544 );

		$this->assertStringContainsString( 'aria-labelledby="frm-gdpr-accept-544"', $html );

Conversation

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

What was broken

What changed

Verified

FYI, out of scope

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!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot commented Sep 28, 2026

Uh oh!

vivi-the-going-merry Bot commented Sep 28, 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 Oct 1, 2026

Uh oh!

deepsource-io Bot commented Oct 1, 2026

DeepSource Code Review

Code Review Summary

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined static method FrmUnitTest::tearDown()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::assertStringNotContainsString()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Access to an undefined property FrmSettings::$enable_gdpr

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldGdpr::assertStringContainsString()

Uh oh!

deepsource-io Bot commented Oct 1, 2026

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading