A11y: give data tables' checkbox column a real header (table_headers_exists) by vivi-the-going-merry[bot] · Pull Request #3369 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-17T23:17:46Z

What was broken

The IBM Equal Access checker's table_headers_exists rule flagged the checkbox-select-all column in Formidable's admin list tables (Forms list, Entries list, etc., rendered via FrmListHelper::print_column_headers()) and the Import/Export page's Export table (classes/views/xml/import_form.php): the header cell for that column was a headerless <td>, even though the column's row cells are already <th scope="row">. A scope attribute on a <td> isn't recognized as a header by assistive tech.

What changed

The cb column header cell is now a real <th scope="col"> in both places.
FrmListHelper::display() adds role="presentation" to the <table> element when there are zero items (no <thead>/rows rendered), matching the existing has_min_items() gate that already suppressed the header row in that case.
Added SCSS overrides (_widefat.scss, _screen-tablet.scss) restating core's own td.check-column font-size/padding as a th.check-column rule scoped to table.widefat (shared by both tables), so switching the tag doesn't change the cell's visual size. Scoping to table.widefat rather than .wp-list-table matters: the Export table doesn't carry wp-list-table, so an earlier version of this fix left it exposed to the pre-existing .frm-white-body table.widefat th { font-size: var(--text-md) } rule — caught in self-review before this PR opened.

Markup/CSS only — no JS, no behavior change beyond the accessibility fix.

How verified

Two new PHPUnit tests render the real markup and assert the header cell is a <th scope="col">, not a <td>:

tests/phpunit/forms/test_FrmFormsListHelper.php::test_checkbox_column_header_is_th
tests/phpunit/xml/test_FrmXMLController.php::test_export_table_headers_are_th

Confirmed red against the pre-fix markup, green after, against the real local PHPUnit rig. Full forms/entries/xml test groups pass with no regressions. Compiled css/frm_admin.css rebuilt from the SCSS via the project's own webpack css config.

Self-reviewed (correctness/security/reuse/simplification/efficiency/altitude lenses) before opening — the Export-table CSS gap above was caught and fixed in that pass.

Closes Strategy11/formidable-pro#6694

🤖 Generated with Claude Code

coderabbitai · 2026-09-17T23:17:51Z

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: 470e7176-121e-4016-b9fa-2a078ceb3b32

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

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-17T23:19:38Z

DeepSource Code Review

We reviewed changes in 935635a...7642bed 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 5, 2026 7:13p.m.	Review ↗
JavaScript		Oct 5, 2026 7:13p.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-17T23:19:59Z

+
+		$thead = substr( $html, 0, strpos( $html, '</thead>' ) );
+
+		$this->assertMatchesRegularExpression(


Call to an undefined method test_FrmFormsListHelper::assertMatchesRegularExpression()

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

franky-the-going-merry

Request Changes. The markup fix works on both tables, but the branch is conflicting with master, Inspections / Run PHPCS inspection is red on two lines in the new tests, and the Export table's other header cells change size, which the PR says it avoids.

Blocking

Merge conflict. mergeable=CONFLICTING / DIRTY, 435 commits behind master. Rebase onto current master, resolve, and rebuild css/frm_admin.css from the SCSS afterward.
PHPCS is red at d0b0582: Formidable.WhiteSpace.ConsecutiveAssignmentSpacing.BlankLineBetweenAssignments at tests/phpunit/xml/test_FrmXMLController.php:37 and tests/phpunit/forms/test_FrmFormsListHelper.php:67. See the two inline comments.

Should fix (inline on _widefat.scss)

The Export table's five non-checkbox header cells also changed from <td> to <th> and pick up the .frm-white-body table.widefat th rule. See the inline comment for the measured numbers.

Live before/after (Playground, Lite master-era base e404085 vs PR d0b0582, Pro active, one form seeded)

Forms list (admin.php?page=formidable) header row, first cell: base TD.manage-column column-cb check-column, no scope; PR TH[scope=col]. Computed font-size 14px, padding 4px 0 0 3px, vertical-align middle, 34x78 px: identical on both. Every other header cell in that row is identical too.
Export table (admin.php?page=formidable-import): base all six header cells are TD; PR all six are TH[scope=col]. Cb cell keeps 14px / 4px 0 0 3px / middle / width 40. Row height goes 37px to 39px.
Generalization: no other <thead> in classes/ renders a <td> header cell; the CSS/JS in Lite has no selector depending on td.check-column.

Not verified

The Entries list (Pro, FrmEntriesListHelper) was not rendered; it goes through the same print_column_headers() so I expect the same result.
The zero-items role="presentation" branch and the tablet-breakpoint padding-top: 10px rule were not rendered.
frm_admin.css was not rebuilt byte-for-byte; I only confirmed both new rules are present in it.
CI's PHPUnit and Cypress jobs are green at this head; I did not re-run them locally and did not re-run the new tests against the pre-fix markup.
DeepSource: PHP is red with no readable output; not investigated.

Non-blocking

FrmListHelper.php:1044-1047: the new comment is four lines and explains why, then points at _widefat.scss; two lines would do (the SOP/dev-rules cap), and _widefat.scss and _screen-tablet.scss each repeat the same "why this is a th" explanation. Point at one place instead.
FrmListHelper.php:1048-1049: $tag = 'th'; $scope = 'scope="col"'; are now constants used once each on the next lines; inline them into the echo and drop the variables.

franky-the-going-merry · 2026-10-05T19:01:00Z

+		FrmXMLController::form();
+		$html = ob_get_clean();
+
+		$thead = substr( $html, strpos( $html, '<thead>' ), strpos( $html, '</thead>' ) - strpos( $html, '<thead>' ) );


PHPCS fails here (Formidable.WhiteSpace.ConsecutiveAssignmentSpacing.BlankLineBetweenAssignments, auto-fixable). Delete the blank line at line 36 between $html = ob_get_clean(); and $thead = ...; so the two assignments are consecutive, then re-run vendor/bin/phpcs tests/phpunit/xml/test_FrmXMLController.php.

Fixed: blank line removed, assignments now aligned.

franky-the-going-merry · 2026-10-05T19:01:01Z

+		$list_helper->display();
+		$html = ob_get_clean();
+
+		$thead = substr( $html, 0, strpos( $html, '</thead>' ) );


Same PHPCS failure (BlankLineBetweenAssignments, auto-fixable) at this line. Delete the blank line at line 66 between $html = ob_get_clean(); and $thead = ...;, then re-run vendor/bin/phpcs tests/phpunit/forms/test_FrmFormsListHelper.php.

Fixed: blank line removed, assignments now aligned.

franky-the-going-merry · 2026-10-05T19:01:01Z

+// check-column sizing, so restate core's font-size/padding here to keep that cell's layout
+// unchanged. Scoped to table.widefat (shared by both tables) rather than .wp-list-table,
+// which the Export table's own <table> doesn't carry.
+table.widefat thead th.check-column,


This restates core's size for th.check-column only, but classes/views/xml/import_form.php also turned its other five header cells (Form Title, ID / Form Key, Type, Entries, Style) from <td> into <th>, and they now match .frm-white-body table.widefat th { font-size: var(--text-md) }. Measured on the Export page, base vs this PR: font-size 14px to 16px, vertical-align top to middle, row height 37px to 39px, Type column 143px to 151px, Form Title 311px to 307px. The PR text says switching the tag does not change the visual size, which holds for the checkbox cell only. Fix: extend the override so every th in the Export table's thead keeps 14px / vertical-align: top (for example a class on that <table> or on those <th>s, scoped like this rule), then re-measure that the six header cells match the base values above.

Fixed: added a frm-export-table class on the Export table and a rule for its non-checkbox thead th cells (14px, vertical-align top). Re-measured live on the Export page: all six header cells are 14px; the five text cells are top-aligned with 8px 10px padding, the checkbox cell stays middle-aligned. Also trimmed the SCSS comments to one place and the FrmListHelper comment, and inlined the th/scope variables.

…exists) Rebased onto master; Export table header cells keep their pre-fix size. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

deepsource-io · 2026-10-05T19:14:58Z

@@ -38,4 +38,38 @@ public function test_get_posts_contain_form() {
 		$this->assertContains( $post_with_form, $post_ids );


Call to an undefined method test_FrmFormsListHelper::assertContains()

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

deepsource-io · 2026-10-05T19:14:58Z

@@ -38,4 +38,38 @@ public function test_get_posts_contain_form() {
 		$this->assertContains( $post_with_form, $post_ids );
 		$this->assertNotContains( $post_without_form, $post_ids );


Call to an undefined method test_FrmFormsListHelper::assertNotContains()

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

deepsource-io · 2026-10-05T19:14:59Z

@@ -46,4 +46,23 @@ public function test_form_has_unique_landmark_names_for_import_and_export_forms(
 		$this->assertContains( 'Export', $matches[1], 'Export form is missing its aria-label' );


Call to an undefined method test_FrmXMLController::assertContains()

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

deepsource-io · 2026-10-05T19:14:59Z

@@ -46,4 +46,23 @@ public function test_form_has_unique_landmark_names_for_import_and_export_forms(
 		$this->assertContains( 'Export', $matches[1], 'Export form is missing its aria-label' );
 		$this->assertSame( array_unique( $matches[1] ), $matches[1], 'Form landmarks must have distinct accessible names' );


Call to an undefined method test_FrmXMLController::assertSame()

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

deepsource-io · 2026-10-05T19:14:59Z

+		$html  = ob_get_clean();
+		$thead = substr( $html, strpos( $html, '<thead>' ), strpos( $html, '</thead>' ) - strpos( $html, '<thead>' ) );
+
+		$this->assertStringContainsString(


Call to an undefined method test_FrmXMLController::assertStringContainsString()

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

deepsource-io · 2026-10-05T19:14:59Z

+			$thead,
+			'The Export table\'s cb column header cell must be a real <th scope="col">, not a <td>, for IBM table_headers_exists.'
+		);
+		$this->assertStringNotContainsString( '<td', $thead );


Call to an undefined method test_FrmXMLController::assertStringNotContainsString()

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

vivi-the-going-merry · 2026-10-05T19:22:37Z

Method: in-place push
Pushed to: #3369 (branch fix/issue-6694-table-headers-exists, unchanged PR number). Rebased onto current master as one commit (force-push); frm_admin.css rebuilt from the SCSS.

franky-the-going-merry

Approve. All three points from my last review are fixed at 7642bed: the conflict is gone (rebased onto current master, one commit), Run PHPCS inspection is green, and the Export table's other header cells keep their old size.

Live check at 7642bed (preview env: PR branch on current Lite master-era, Pro active, six forms seeded)

Export table (admin.php?page=formidable-import), all six header cells are TH[scope=col]. Computed: the cb cell is 14px / 4px 0 0 3px / middle, 40x37. The other five are 14px / 8px 10px / vertical-align: top, row height 37px. That matches the base values I measured last time (37px row, 14px, top) — the 39px row and 16px text are gone.
Forms list (admin.php?page=formidable), cb header cell is TH[scope=col], 14px / 4px 0 0 3px / middle, 34x78. Same as base.

Export table header:

Forms list table:

Also confirmed

The two non-blocking notes were taken: the FrmListHelper.php comment is one line, and $tag/$scope are inlined.
css/frm_admin.css contains the three new rules (cb override, tablet padding-top: 10px, Export th:not(.check-column)).
CI: PHPUnit on 7.4/8.0/8.2/8.4, PHPCS, PHP CS Fixer, Stylelint, PHPStan, Psalm, ESLint, Oxlint all green.

Not verified

The Entries list (Pro) was not rendered; it shares print_column_headers(), so I expect the same result.
The zero-items role="presentation" branch and the tablet-breakpoint rule were not rendered.
I did not re-run the two new tests against the pre-fix markup; I only read CI's green PHPUnit result.
DeepSource: PHP is red on "undefined method assertContains()" style hits on $this->assert* calls in the test files (pre-existing assertions in those files too); not a finding on this diff.

vivi-the-going-merry Bot added run analysis run e2e tests Run the Cypress end-to-end suite on this PR run tests labels Sep 17, 2026

deepsource-io Bot reviewed Sep 17, 2026

View reviewed changes

robin-the-going-merry Bot added the priority: low label Sep 18, 2026

Crabcyborg added priority: high franky-review and removed priority: low run analysis run tests run e2e tests Run the Cypress end-to-end suite on this PR labels Oct 5, 2026

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

franky-the-going-merry Bot requested changes Oct 5, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review franky-working Franky is actively reviewing this labels Oct 5, 2026

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

A11y: give data tables' checkbox column a real header (table_headers_…

7642bed

…exists) Rebased onto master; Export table header cells keep their pre-fix size. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

vivi-the-going-merry Bot force-pushed the fix/issue-6694-table-headers-exists branch from d0b0582 to 7642bed Compare October 5, 2026 19:13

vivi-the-going-merry Bot added run analysis run tests labels Oct 5, 2026

deepsource-io Bot reviewed Oct 5, 2026

View reviewed changes

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

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

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

View reviewed changes

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


		$thead = substr( $html, 0, strpos( $html, '</thead>' ) );

		$this->assertMatchesRegularExpression(

		@@ -38,4 +38,38 @@ public function test_get_posts_contain_form() {
		$this->assertContains( $post_with_form, $post_ids );

		@@ -38,4 +38,38 @@ public function test_get_posts_contain_form() {
		$this->assertContains( $post_with_form, $post_ids );
		$this->assertNotContains( $post_without_form, $post_ids );

		@@ -46,4 +46,23 @@ public function test_form_has_unique_landmark_names_for_import_and_export_forms(
		$this->assertContains( 'Export', $matches[1], 'Export form is missing its aria-label' );

		@@ -46,4 +46,23 @@ public function test_form_has_unique_landmark_names_for_import_and_export_forms(
		$this->assertContains( 'Export', $matches[1], 'Export form is missing its aria-label' );
		$this->assertSame( array_unique( $matches[1] ), $matches[1], 'Form landmarks must have distinct accessible names' );

Conversation

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

What was broken

What changed

How verified

Uh oh!

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

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsListHelper::assertMatchesRegularExpression()

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsListHelper::assertContains()

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsListHelper::assertNotContains()

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmXMLController::assertContains()

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmXMLController::assertSame()

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmXMLController::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 5, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmXMLController::assertStringNotContainsString()

Uh oh!

vivi-the-going-merry Bot commented Oct 5, 2026

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Sep 17, 2026 •

edited

Loading

deepsource-io Bot commented Sep 17, 2026 •

edited

Loading