Wrap custom html field labels in error class when showing in field validation messages#3049
Wrap custom html field labels in error class when showing in field validation messages#3049AbdiTolesa wants to merge 8 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors error message handling in the form validation system by introducing a centralized ChangesError HTML Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
May 7, 2026 6:45p.m. | Review ↗ |
| JavaScript | |
May 7, 2026 6:45p.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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/formidable.js`:
- Around line 1158-1162: The current code creates a temporary container
(tempDiv) and adds the frm_error class to that container but then uses
tempDiv.innerHTML, which returns only children and drops the wrapper and its
attributes; instead, parse the errorMessage into tempDiv, get the parsed node
(e.g., tempDiv.firstElementChild or firstChild), add the frm_error class (and
preserve any id) to that parsed node, and then set errorHtml from the parsed
node's outerHTML so the wrapper, class and id are preserved; reference tempDiv,
errorMessage, errorHtml and the parsed node (firstElementChild) when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db707e86-b21a-4e3c-a630-1824cb7b2997
📒 Files selected for processing (1)
js/formidable.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/formidable.js (1)
1147-1155:⚠️ Potential issue | 🟠 MajorCustom
<div>errors still missid/role, soaria-describedbycan point to a non-existent node.Line 1153 only adds
frm_erroron the parsed node. In this branch, the generated element still doesn’t receive theidfromaddFieldError(and ignores alert role), which breaks the accessibility linkage/cleanup path.🐛 Proposed fix
function getErrorHtml( errorMessage, id ) { const roleString = frm_js.include_alert_role ? 'role="alert"' : ''; if ( ! errorMessage.startsWith( '<div' ) ) { return `<div class="frm_error" ${ roleString } id="${ id }">${ errorMessage }</div>`; } const tempDiv = document.createElement( 'div' ); tempDiv.innerHTML = errorMessage; - tempDiv.firstChild.classList.add( 'frm_error' ); + const existingWrapper = tempDiv.firstElementChild; + if ( existingWrapper ) { + existingWrapper.classList.add( 'frm_error' ); + existingWrapper.id = id; + if ( frm_js.include_alert_role ) { + existingWrapper.setAttribute( 'role', 'alert' ); + } + } return tempDiv.innerHTML; }#!/bin/bash # Verify current helper behavior around parsed custom HTML error markup. sed -n '1140,1195p' js/formidable.js # Confirm parsed-div branch currently mutates firstChild and does not set id/role. rg -n "getErrorHtml|startsWith\\( '<div' \\)|firstChild\\.classList|existingWrapper|setAttribute\\( 'role'|\\.id\\s*=\\s*id" js/formidable.js # Verify aria-describedby and cleanup depend on error element id. rg -n "aria-describedby|getErrorElementId|errorMessage\\.id|removeFieldError" js/formidable.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 1147 - 1155, The parsed-HTML branch fails to apply the id and role attributes that addFieldError expects, breaking aria-describedby and cleanup; in the function that builds error markup (the branch that creates tempDiv and uses tempDiv.firstChild), after adding the frm_error class to tempDiv.firstChild also set its id to the same id value and apply the roleString (or setAttribute('role', ...)) so the parsed node gets the same id/role as the non-parsed branch; update getErrorHtml/addFieldError related logic to ensure tempDiv.firstChild receives id and roleString before returning tempDiv.innerHTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/formidable.js`:
- Around line 1147-1155: The parsed-HTML branch fails to apply the id and role
attributes that addFieldError expects, breaking aria-describedby and cleanup; in
the function that builds error markup (the branch that creates tempDiv and uses
tempDiv.firstChild), after adding the frm_error class to tempDiv.firstChild also
set its id to the same id value and apply the roleString (or
setAttribute('role', ...)) so the parsed node gets the same id/role as the
non-parsed branch; update getErrorHtml/addFieldError related logic to ensure
tempDiv.firstChild receives id and roleString before returning
tempDiv.innerHTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22574bc4-b215-4e7a-97ae-fbfa07460255
📒 Files selected for processing (1)
js/formidable.js
AbdiTolesa
changed the title
garretlaxton
left a comment
garretlaxton
left a comment
There was a problem hiding this comment.
This fix works great!
I do want to confirm that script tags should be allowed in the field labels though?
|
@garretlaxton No, it shouldn't be allowed in the label. |
|
@truongwp I am able to add a script tag to the label that runs when viewing the form, unless I have |
|
@Crabcyborg Should we pass the label through @garretlaxton If that's an issue, we will fix it in a separate PR. |
|
@truongwp I think it should be fine. I don't expect that people are actually using scripts in field labels. I expect that people use descriptions or custom HTML if they do need to add scripts. Just in case, I think a new hook would be nice. Like |
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/6392
Test procedure
<div>tag in it. Eg. "Enter your answerSummary by CodeRabbit