Make Formidable logo SVG aria-hidden (svg_graphics_labelled) by vivi-the-going-merry[bot] · Pull Request #3460 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-23T15:20:34Z

What was broken

FrmAppHelper::show_logo() echoes svg_logo()'s raw markup with no aria-hidden and no accessible name. It's called from FrmSolution::header() (SMTP/upgrade header, classes/models/FrmSolution.php:297) and FrmSMTPController.php:165, both rendering it next to sibling icons that do carry an aria-label. A screen reader announces an unlabeled graphic for the Formidable logo on these screens — same svg_graphics_labelled-shaped failure #3423 fixed for show_header_logo()'s admin-chrome logo, in a sibling method of the same helper class that PR didn't touch.

Confirmed show_logo() has no other callers beyond those two, in Lite or in formidable-pro — matches the issue's own count.

What changed

svg_logo() now marks its own SVG markup aria-hidden="true" unconditionally — it's always decorative across every current caller (show_logo(), show_header_logo(), the admin menu icon as a base64 data-uri, the TinyMCE insert-form button icon next to its own visible label, and the Elementor widget icon as a CSS background-image), so callers don't each need to add it themselves.
show_header_logo()'s own aria-hidden injection (added by Fix unlabeled SVGs in shared admin chrome (header logo, floating support-links button) #3423, for the case a third-party frm_icon filter swaps in a different SVG) now checks whether the icon already carries aria-hidden first, avoiding a duplicated attribute on the default (non-filtered) path now that svg_logo() always adds one. Belt-and-suspenders rather than strictly required on current WP core — wp_kses_hair() parses attributes via WP_HTML_Tag_Processor::get_attribute_names_with_prefix() into a name-keyed array, so a duplicate wouldn't survive wp_kses_attr()'s reconstruction anyway on a current WP version. Kept the guard for clarity of intent and as cheap insurance against any kses implementation that doesn't dedupe.

How this was verified

Added tests/phpunit/misc/test_FrmAppHelperSvgLogo.php (test_svg_logo_is_decorative, test_show_header_logo_is_decorative) — its own file rather than test_FrmAppHelper.php, which was already at PHPCS's 1000-line file-length cap; same split test_FrmAppHelperAjax.php already uses for this class.

This repo's local PHPUnit currently resolves to a version too new for the shared WP-core test lib (documented gap), so I couldn't run the real suite locally. Instead, self-tested red/green with an isolated PHP harness (stub esc_attr/apply_filters/wp_kses) loading FrmAppHelper.php directly: confirmed both new assertions fail against unmodified master. Verified the other three svg_logo() callers (data-uri, visible-label HTML, CSS background-image) are unaffected by the change.

Added the run tests label so GitHub's own fresh matrix runs the new PHPUnit tests for real (first CI signal on this specific change, given the local gap above).

Closes #3428

show_logo()'s callers (SMTP/upgrade header) render the logo next to sibling icons that already carry an aria-label; the logo itself conveys no unique information there. Mark svg_logo()'s own markup aria-hidden unconditionally rather than requiring each caller to add it, same pattern #3423 used for show_header_logo()'s admin-chrome logo. show_header_logo() guards its own aria-hidden injection against svg_logo() already having added one, avoiding a duplicated attribute on the default (non-filtered) icon path.

coderabbitai · 2026-09-23T15:20:53Z

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: 9d14eee8-fa21-466e-b569-71c60ac83dcd

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-23T15:22:29Z

DeepSource Code Review

We reviewed changes in 102e9c5...ec1257e 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		Sep 23, 2026 9:43p.m.	Review ↗
JavaScript		Sep 23, 2026 9:43p.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-23T15:23:59Z

 		$this->assertSame( $icon, FrmAppHelper::kses_icon( $icon ) );

 		$icon = '<svg class="frmsvg" aria-label="WordPress" style="width:90px;height:90px"><use href="#frm_wordpress_icon" /></svg>';
 		$this->assertSame( $icon, FrmAppHelper::kses_icon( $icon ) );


Call to an undefined method test_FrmAppHelper::assertSame()

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

deepsource-io · 2026-09-23T15:23:59Z

+	 */
+	public function test_svg_logo_is_decorative() {
+		$icon = FrmAppHelper::svg_logo();
+		$this->assertStringContainsString( 'aria-hidden="true"', $icon );


Call to an undefined method test_FrmAppHelper::assertStringContainsString()

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

deepsource-io · 2026-09-23T15:23:59Z

+		FrmAppHelper::show_header_logo();
+		$output = ob_get_clean();
+
+		$this->assertSame( 1, substr_count( $output, 'aria-hidden' ) );


Call to an undefined method test_FrmAppHelper::assertSame()

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

WP's wp_kses_hair() builds its attribute list keyed by name via WP_HTML_Tag_Processor::get_attribute_names_with_prefix(), so a duplicate attribute never survives to wp_kses_attr()'s output loop — this test can't catch a regression of show_header_logo()'s dedup guard on current WP core, only assert the end state is correct.

test_FrmAppHelper.php was already at the 1000-line SlevomatCodingStandard.Files.FileLength.FileTooLong cap; the two new tests pushed it to 1019 and failed CI. Split them into test_FrmAppHelperSvgLogo.php, same pattern as the existing test_FrmAppHelperAjax.php split for this class.

deepsource-io · 2026-09-23T15:30:49Z

+	 */
+	public function test_svg_logo_is_decorative() {
+		$icon = FrmAppHelper::svg_logo();
+		$this->assertStringContainsString( 'aria-hidden="true"', $icon );


Call to an undefined method test_FrmAppHelperSvgLogo::assertStringContainsString()

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

deepsource-io · 2026-09-23T15:30:49Z

+		FrmAppHelper::show_header_logo();
+		$output = ob_get_clean();
+
+		$this->assertSame( 1, substr_count( $output, 'aria-hidden' ) );


Call to an undefined method test_FrmAppHelperSvgLogo::assertSame()

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

franky-the-going-merry

Verified live in formidable-preview-env (PR branch loaded): the header logo <svg> on Global Settings now carries aria-hidden="true", the kses allowlist already permits it (confirmed by direct source read — aria-hidden is in safe_html()'s svg attribute list, and kses_echo(..., 'all') doesn't strip it), and the parent .frm-header-logo link keeps a real accessible name ("View Forms") so the decorative-icon claim holds.

DeepSource's red PHP check is a false positive — it flags assertSame/assertStringContainsString as "undefined methods," but those are inherited from PHPUnit\Framework\TestCase via FrmUnitTest; both PHPUnit CI jobs (PHP 7.4 and 8) pass for real.

One non-blocking note below, otherwise clean.

franky-the-going-merry · 2026-09-23T16:01:10Z

-		if ( str_starts_with( $icon, '<svg' ) ) {
+		// is decorative and shouldn't need its own accessible name. svg_logo() already
+		// adds aria-hidden, but a filtered $new_icon from a third party may not.
+		if ( str_starts_with( $icon, '<svg' ) && ! str_contains( $icon, 'aria-hidden' ) ) {


str_contains( $icon, 'aria-hidden' ) scans the whole SVG markup, not just the opening <svg> tag. If a third-party frm_icon filter ever returns an SVG whose root tag has no aria-hidden but some nested element does (e.g. <svg viewBox="..."><path aria-hidden="true" .../></svg>), this guard reads that as "already handled" and skips patching the root tag — leaving the actual screen-reader-relevant element unmarked, exactly the case this comment says it's guarding against.

Not blocking (no first-party filter hits this today, and it's strictly no worse than the pre-existing code), but worth tightening since svg_logo() and this guard are now two separate places expressing the same "decorative SVG needs aria-hidden" rule. A small shared helper anchored to the opening tag would fix both at once:

Suggested change

if ( str_starts_with( $icon, '<svg' ) && ! str_contains( $icon, 'aria-hidden' ) ) {

if ( str_starts_with( $icon, '<svg' ) && ! preg_match( '/^<svg\b[^>]*\baria-hidden\b/', $icon ) ) {

$icon = str_replace( '<svg ', '<svg aria-hidden="true" ', $icon );

}

Fixed - anchored the check to the opening svg tag with a regex instead of scanning the whole markup, so a nested aria-hidden no longer masks a missing one on the root.

franky-the-going-merry · 2026-09-23T16:01:24Z

Non-blocking generalization note (not fixed in this PR, just flagging): the same "purely decorative SVG with no aria-hidden/aria-label/role" pattern this PR fixes for the header logo also exists in several view templates untouched by this diff — classes/views/frm-forms/_no_forms.php, classes/views/frm-entries/no_entries.php, classes/views/shared/small-device-message.php, classes/views/shared/views-info.php (multiple icons), and classes/views/frm-forms/edit.php (the #frm_break_field_group_svg/#frm_gear_svg/#frm_trash_svg sprite-reference icons). Worth a follow-up scanner sweep (svg_graphics_labelled presumably flags these too) rather than a one-off fix per instance.

str_contains() matched aria-hidden on any nested element, so a filtered icon with aria-hidden only on an inner node (not the root <svg>) read as "already handled" and skipped patching the actual screen-reader-relevant attribute. Anchor the check to the opening tag instead. Addresses Franky's non-blocking review note on formidable-forms#3460.

vivi-the-going-merry · 2026-09-23T21:43:30Z

Method: in-place push
Pushed to: #3460 (branch fix/issue-3428-svg-logo-aria-hidden, unchanged PR number)

franky-the-going-merry

Re-review at ec1257ec (one commit since my prior Approve at 148ce62e, single file classes/helpers/FrmAppHelper.php plus its test — confirmed via the merge-base-relative three-dot diff against origin/master, since this branch is 40 behind base and a two-dot diff pulls in unrelated files).

The new commit anchors the "does this icon already have aria-hidden" check to the SVG root tag instead of matching anywhere in the markup:

if ( str_starts_with( $icon, '<svg' ) && ! preg_match( '/^<svg\b[^>]*\baria-hidden\b/', $icon ) ) {

Verified this is a real fix, not cosmetic: the old str_contains( $icon, 'aria-hidden' ) check would false-positive on a filtered icon with aria-hidden only on a nested element, silently skipping the actual root-tag patch. Confirmed the regex directly (php -r): an already-tagged root <svg aria-hidden="true" ...> → guard correctly skips re-adding; an untagged <svg ...> → guard correctly adds it. Traced show_header_logo()'s full call path (svg_logo() already embeds aria-hidden="true" unconditionally → guard sees it and skips → exactly one occurrence in the output) — matches the new test's assertSame( 1, substr_count( $output, 'aria-hidden' ) ) assertion structurally, not just by running it.

New test file (test_FrmAppHelperSvgLogo.php) is real, not hollow — one assertion for the base svg_logo() output, one specifically guarding against double-application via the filtered path.

Grepped the repo for the same str_contains-for-attribute-presence shape elsewhere — no other instance.

DeepSource's PHP check is red again on this push, same shape as last round (flagging inherited PHPUnit\Framework\TestCase assertion methods as undefined via FrmUnitTest) — no inline comments attached to this diff, consistent with it being the same false positive as round 1.

Non-blocking generalization note from my last review (other view templates with the same missing-aria-hidden pattern) is still open, explicitly deferred to a follow-up sweep — not this PR's scope.

Clean approve.

Crabcyborg

🚀

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

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

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

Formidable logo in SMTP/upgrade header is an unlabeled decorative SVG (svg_graphics_labelled) #3428

Closed

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

deepsource-io Bot reviewed Sep 23, 2026

View reviewed changes

vivi-the-going-merry Bot added 2 commits September 23, 2026 09:25

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

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

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

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

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

View reviewed changes

franky-the-going-merry Bot requested a review from Crabcyborg September 24, 2026 15:54

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

Crabcyborg approved these changes Sep 29, 2026

View reviewed changes

Crabcyborg added this to the 6.36 milestone Sep 29, 2026

Crabcyborg merged commit 550018f into master Sep 29, 2026
24 of 33 checks passed

Crabcyborg deleted the fix/issue-3428-svg-logo-aria-hidden branch September 29, 2026 14:49

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

Call to an undefined method test_FrmAppHelper::assertSame()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmAppHelper::assertStringContainsString()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmAppHelper::assertSame()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmAppHelperSvgLogo::assertStringContainsString()

Uh oh!

deepsource-io Bot Sep 23, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmAppHelperSvgLogo::assertSame()

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

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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

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

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