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 5 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds a new helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |  |
Mar 27, 2026 6:36p.m. | Review ↗ |
| JavaScript | |
Mar 27, 2026 6:36p.m. | Review ↗ |
![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
js/formidable.js
Outdated
| if ( errorMessage.startsWith( '<div' ) ) { | ||
| const tempDiv = document.createElement( 'div' ); | ||
| tempDiv.innerHTML = errorMessage; | ||
| tempDiv.classList.add( 'frm_error' ); | ||
| errorHtml = tempDiv.innerHTML; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does HTMLElement.innerHTML include the element itself or only its descendants? Please cite MDN or equivalent documentation and give a short example.
💡 Result:
HTMLElement.innerHTML includes only the element's descendants, not the element itself. It returns the HTML serialization of the nested child DOM elements within the element. From MDN: "Getting the property returns a string containing the HTML serialization of the element's descendants." [Result 1] Example:
My name is Joe
My name is Joe
" (no outerCitations:
🏁 Script executed:
# Check the exact code at lines 1158-1162 and surrounding context
sed -n '1150,1170p' js/formidable.jsRepository: Strategy11/formidable-forms
Length of output: 866
Mutate the parsed error node, not the temporary container.
On lines 1159–1162, adding the frm_error class to tempDiv then reading tempDiv.innerHTML loses the class—innerHTML returns only the element's descendants, not the element itself. This path inserts the original errorMessage without the .frm_error wrapper or id attribute, breaking the PR objective and downstream selectors that depend on those attributes (e.g., removeFieldError, aria-describedby).
🐛 Proposed fix
if ( jsErrors[ key ].includes( '<div' ) ) {
if ( errorMessage.startsWith( '<div' ) ) {
const tempDiv = document.createElement( 'div' );
tempDiv.innerHTML = errorMessage;
- tempDiv.classList.add( 'frm_error' );
- errorHtml = tempDiv.innerHTML;
+ const existingWrapper = tempDiv.firstElementChild;
+ if ( existingWrapper && existingWrapper.tagName === 'DIV' ) {
+ existingWrapper.classList.add( 'frm_error' );
+ existingWrapper.id = id;
+ if ( frm_js.include_alert_role ) {
+ existingWrapper.setAttribute( 'role', 'alert' );
+ }
+ errorHtml = tempDiv.innerHTML;
+ } else {
+ errorHtml = wrapInErrorClass( errorMessage );
+ }
} else {
errorHtml = wrapInErrorClass( errorMessage );
}
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable.js` around lines 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.
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
1 participant
Fix https://github.com/Strategy11/formidable-pro/issues/6392
Test procedure
<div>tag in it. Eg. "Enter your answerSummary by CodeRabbit