Replace xlink:href with href by truongwp · Pull Request #2632 · Strategy11/formidable-forms

truongwp · 2025-12-05T05:17:41Z

Fixes issue: https://github.com/Strategy11/formidable-pro/issues/6106

coderabbitai · 2025-12-05T08:33:40Z

Walkthrough

Replaced SVG xlink:href with standard href across PHP helpers, view templates, JavaScript, and tests; minor docblock whitespace edits and a trailing newline addition.

Changes

Cohort / File(s)	Summary
Core SVG/Icon Handling `classes/helpers/FrmAppHelper.php`	Icon rendering now emits `<use href="...">` instead of `xlink:href`; minor docblock whitespace reformat.
View Templates `classes/views/frm-forms/edit.php`, `classes/views/styles/_style-options.php`	Inline SVG `<use>` attributes changed from `xlink:href` → `href`; added trailing newline to `_style-options.php`.
Admin JavaScript `js/admin/dom.js`, `js/src/admin/admin.js`	DOM handling now prefers `use.getAttribute('href')` with fallback to `xlink:href`; injected HTML updated to use `href` and new helper `getSVGHref(...)` added.
Form Template JS (UI / Events) `js/src/form-templates/events/favoriteButtonListener.js`, `js/src/form-templates/ui/showModal.js`	Switched icon attribute usage from `xlink:href` → `href` for favorite toggles and upgrade modal icons.
Cypress Tests `tests/cypress/e2e/Add-Ons/validateAddOnsPage.cy.js`, `tests/cypress/e2e/Form Templates/FormTemplates.cy.js`	Updated assertions to expect `href` instead of `xlink:href` for SVG `<use>` references.
PHP Unit Tests `tests/phpunit/misc/test_FrmAppHelper.php`	Adjusted expected strings to match `<use>` elements using `href`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Pay attention to:
- classes/helpers/FrmAppHelper.php — ensure emitted SVG markup remains properly escaped/sanitized.
- js/admin/dom.js / js/src/admin/admin.js — confirm getSVGHref fallback ordering is correct for older browsers and existing SVG variants.
- Tests — verify CI/browser matrix accepts href on <use> (some environments may still assert xlink:href).

Possibly related PRs

Stop using font icons for email bcc/cc/reply to remove icons #1814 — Direct update to FrmAppHelper::icon_by_class output to use href; closely related.
Replace font icons with SVGs for edd license status #1918 — Changes icon rendering and usage patterns that overlap with these edits.
Update search icon used in form search #2450 — Also touches FrmAppHelper icon usage and callers; relevant.

Suggested reviewers

Crabcyborg
lauramekaj1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly and concisely summarizes the main change: replacing xlink:href with href throughout the codebase.
Description check	✅ Passed	The pull request description references a specific issue and indicates the purpose of the changes, which aligns with the changeset of updating SVG attribute references.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch pro-issue-6106-remove-xlink-href

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09d0133 and 5431340.

📒 Files selected for processing (2)

js/admin/dom.js (1 hunks)
js/src/admin/admin.js (8 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

js/src/admin/admin.js

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: PHP 7.4 tests in WP trunk
GitHub Check: PHP 8 tests in WP trunk
GitHub Check: Cypress

🔇 Additional comments (1)

js/admin/dom.js (1)

846-846: LGTM! Modern SVG standard correctly prioritized with backward compatibility.

The change properly prioritizes the modern href attribute while maintaining a fallback to the deprecated xlink:href for legacy SVG markup. This ensures a smooth migration path and aligns with current SVG standards.

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 and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

classes/views/frm-forms/edit.php (1)
26-29: Minor: stray double quote in button label

Line 28 prints an extra " after the translated "Update" text, so the button label will appear as Update".
-				<?php esc_attr_e( 'Update', 'formidable' ); ?>"
+				<?php esc_attr_e( 'Update', 'formidable' ); ?>
js/src/form-templates/events/favoriteButtonListener.js (1)

90-104: Favorite heart icons correctly migrated to href

Using setAttribute('href', …) for both the primary and twin heart icons keeps the toggle behavior intact while aligning with the new SVG usage.

If you ever expect templates without a <use> child, a small defensive guard like if (heartIcon) { … } would avoid a potential runtime error, but that’s not new to this PR.

js/src/admin/admin.js (1)

8536-8578: Consider fully migrating JS attribute handling away from xlink

Markup now uses <use href="…">, but helper logic still uses getAttributeNS/setAttributeNS with the XLink namespace in a few places (for example when toggling the shortcode box icons and when resetting them in hideShortcodes). Functionally this should keep working, but if the long‑term goal is to eliminate xlink:href entirely, you may want to eventually switch those spots to plain getAttribute('href') / setAttribute('href', ...) and drop the XLink namespace checks.

Also applies to: 8800-8811

tests/cypress/e2e/Add-Ons/validateAddOnsPage.cy.js (1)

155-642: SVG icon assertions correctly migrated from xlink:href to href.

All <use> icon checks now consistently assert href across the add‑on cards, matching the deprecation of xlink:href without changing test behavior or selectors. If this area gets touched again, consider a small helper/custom command for the repeated svg.frmsvg > use + icon‑ID assertion to reduce duplication, but it’s not required for this PR.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a0fb52 and d8a356d.

⛔ Files ignored due to path filters (2)

images/sales/anniversary.svg is excluded by !**/*.svg, !**/*.svg
images/sales/no-brainer.svg is excluded by !**/*.svg, !**/*.svg

📒 Files selected for processing (10)

classes/helpers/FrmAppHelper.php (3 hunks)
classes/views/frm-forms/edit.php (1 hunks)
classes/views/styles/_style-options.php (2 hunks)
js/admin/dom.js (1 hunks)
js/src/admin/admin.js (3 hunks)
js/src/form-templates/events/favoriteButtonListener.js (1 hunks)
js/src/form-templates/ui/showModal.js (1 hunks)
tests/cypress/e2e/Add-Ons/validateAddOnsPage.cy.js (40 hunks)
tests/cypress/e2e/Form Templates/FormTemplates.cy.js (1 hunks)
tests/phpunit/misc/test_FrmAppHelper.php (1 hunks)

🔇 Additional comments (10)

classes/views/frm-forms/edit.php (1)

44-51: SVG <use> migration to href is correct and consistent

Using href for #frm_break_field_group_icon, #frm_settings_icon, and #frm_delete_icon aligns with the updated helpers and sanitization and will keep these icons rendering correctly after deprecating xlink:href.

js/src/form-templates/ui/showModal.js (1)

86-96: Upgrade modal icon now correctly uses href

Switching svg.setAttribute to target href instead of xlink:href matches the rest of the SVG migration and the updated sprite markup; behavior stays the same.

tests/cypress/e2e/Form Templates/FormTemplates.cy.js (1)

33-75: Cypress expectations updated to match href-based SVG icons

The assertions on the category icons now check the href attribute instead of xlink:href, which keeps the tests aligned with the new markup without changing test intent.

classes/views/styles/_style-options.php (1)

12-15: Back button SVG now uses href

The <use> in the styles back button correctly switches to href="#frm_back", consistent with the helper and sanitization changes.

classes/helpers/FrmAppHelper.php (1)

1271-1273: Helper and kses configuration correctly updated for href-based icons

The safe HTML allowlist for <use> now permits only href, matching the removal of xlink:href.

icon_by_class() now emits <svg class="frmsvg…"><use href="#icon" /></svg>, in line with the JavaScript SVG helper.

The format_form_data docblock’s @param array $form matches the actual usage.

These changes keep server-side icon generation and sanitization aligned with the new SVG pattern.

Also applies to: 1370-1379, 3697-3707

tests/phpunit/misc/test_FrmAppHelper.php (1)

355-369: Unit tests now validate href on SVG <use>

The test_kses_icon fixtures have been updated to expect href instead of xlink:href, ensuring tests stay in sync with the new icon HTML and sanitization rules.

js/admin/dom.js (1)

767-781: SVG helper and sanitization are now consistently href-only

svg(), the allowedHtml.use whitelist, and cleanNode() all work purely with href on <use>, completing the migration away from xlink:href support in this module.

Also applies to: 818-828, 840-848

js/src/admin/admin.js (3)

1394-1407: SVG field group control icons correctly switched to href

The new innerHTML for layoutOption and moveOption now uses <use href="…">, which aligns with SVG2 and matches the rest of the icon system. No functional or safety concerns here.

2638-2653: Dropdown action icon markup updated consistently

anchor.innerHTML now uses <use href="#…"> with the existing option.icon values. This keeps the icon rendering consistent with the rest of the file and avoids deprecated xlink:href. Looks good.

7289-7298: Shortcode trigger icon injection aligned with href usage

showInputIcon now wraps inputs and prepends <svg class="frmsvg frm-show-box"><use href="#frm_more_horiz_solid_icon"/></svg>, which is consistent with the new SVG pattern. Existing logic that finds/toggles these icons remains compatible. No issues.

Crabcyborg · 2025-12-08T16:44:14Z

@truongwp I see this other block of code in admin.js, inside the showShortcodeBox function. As well as several other uses of setAttributeNS/getAttributeNS.

if ( moreIcon.tagName === 'use' ) {
	classes = moreIcon.getAttributeNS( 'http://www.w3.org/1999/xlink', 'href' );

	if ( null === classes ) {
		// If the deprecated xlink:href is not defined, check for href.
		classes = moreIcon.getAttribute( 'href' );
	}
}

truongwp · 2025-12-09T19:38:42Z

@Crabcyborg I updated this to handle the getAttributeNS(). Can you check again?

Crabcyborg

Thank you @truongwp!

This looks good to me.

🚀

…href Replace xlink:href with href

Replace xlink:href with href

d8a356d

truongwp requested a review from Crabcyborg December 5, 2025 08:29

truongwp marked this pull request as ready for review December 5, 2025 08:29

truongwp added run analysis run tests labels Dec 5, 2025

Merge branch 'master' into pro-issue-6106-remove-xlink-href

9da46d1

coderabbitai Bot reviewed Dec 5, 2025

View reviewed changes

truongwp added 2 commits December 5, 2025 15:35

Restore xlink:href in safe_html()

5d9414c

Restore xlink:href in cleanNode()

0389af5

truongwp removed the request for review from Crabcyborg December 5, 2025 16:48

Fix PHPCS

09d0133

truongwp requested a review from Crabcyborg December 5, 2025 16:52

Merge branch 'master' into pro-issue-6106-remove-xlink-href

b96da28

Crabcyborg added this to the 6.26 milestone Dec 8, 2025

Crabcyborg modified the milestones: 6.26, 6.27 Dec 8, 2025

Update some JS to use href instead of xlink:href

5431340

Crabcyborg approved these changes Dec 9, 2025

View reviewed changes

Crabcyborg merged commit 70eddfc into master Dec 9, 2025
16 checks passed

Crabcyborg deleted the pro-issue-6106-remove-xlink-href branch December 9, 2025 20:28

Crabcyborg modified the milestones: 6.27, 6.26.1 Dec 15, 2025

coderabbitai Bot mentioned this pull request Jan 12, 2026

Change how icons are rendered for inserting shortcodes to reduce numb… #2779

Merged

stephywells pushed a commit that referenced this pull request Apr 4, 2026

Merge pull request #2632 from Strategy11/pro-issue-6106-remove-xlink-…

8d0ce79

…href Replace xlink:href with href

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace xlink:href with href#2632

Replace xlink:href with href#2632
Crabcyborg merged 7 commits into
masterfrom
pro-issue-6106-remove-xlink-href

truongwp commented Dec 5, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Crabcyborg commented Dec 8, 2025 •

edited

Loading

Uh oh!

truongwp commented Dec 9, 2025

Uh oh!

Crabcyborg left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

truongwp commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Crabcyborg commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

truongwp commented Dec 9, 2025

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!

2 participants

truongwp commented Dec 5, 2025 •

edited

Loading

coderabbitai Bot commented Dec 5, 2025 •

edited

Loading

Crabcyborg commented Dec 8, 2025 •

edited

Loading