Stop using jQuery .find() in formidable.js#2627
Conversation
WalkthroughReplaces jQuery-based DOM access and UI state handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
js/formidable.js (1)
849-853: AJAX form handling now assumes a DOM form; behavior is consistent but input type is stricter
getFormErrorsnow:
- Uses
object.querySelectorAll('.frm_form_field')to add/removefrm_doing_ajax.- Reads the form id and page order via
querySelector('input[name="..."]')on the form or document.- Locates field containers, captchas, and buttons via
querySelector/Alland manipulates them directly.This is internally consistent assuming
objectis always anHTMLFormElement(as now required bypostToAjaxUrl). If any external callers still pass a jQuery object intofrmFrontForm.checkFormErrors, they will now get runtime errors becausequerySelectorAllandclassListare not available.Given this function is exposed as
checkFormErrors, consider either:
- Documenting that it now requires a plain form element, or
- Normalizing the input as done elsewhere:
function getFormErrors( object, action ) { - let data, success, error, shouldTriggerEvent; + let data, success, error, shouldTriggerEvent; + + if ( object instanceof jQuery ) { + object = object.get( 0 ); + } + if ( ! object ) { + return; + } @@ - const fieldsets = object.querySelectorAll( '.frm_form_field' ); + const fieldsets = object.querySelectorAll( '.frm_form_field' );This keeps the new DOM-based logic while gracefully handling existing jQuery call sites.
Also applies to: 923-925, 939-945, 963-965, 986-999, 1009-1010, 1028-1030
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(25 hunks)
⏰ 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). (6)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (19)
js/formidable.js (19)
126-134: disableSubmitButton now robustly handles both jQuery and plain form elementsNormalizing
$formto a DOM node and usingquerySelectorAll().forEach()withbutton.disabled = trueis correct and avoids jQuery.find()while preserving behavior. Early return on falsyformalso safely covers empty jQuery selections.
143-150: enableSubmitButton native conversion looks correctSame pattern as
disableSubmitButton: converting$formto a DOM element and togglingdisabled = falseon the matching controls is behaviorally equivalent to the old jQuery-based implementation.
160-167: disableSaveDraft jQuery → DOM conversion is safeConverting
$formto a DOM element and usingquerySelectorAll('a.frm_save_draft')withpointerEvents = 'none'correctly mirrors the old.find().css()behavior, with null-guard in place.
458-462: Signature “typed vs output” handling matches intentUsing
closest('.frm_form_field')andquerySelector('[name="... [output]"]')to derive the output field when the typed value is empty is sound, with null checks so a missing container gracefully falls back to treating the field as empty.
738-744: executeInvisibleRecaptcha correctly expects a DOM element nowUsing
invisibleRecaptcha.dataset.ridand passing that intogrecaptcha.reset/executealigns withhasInvisibleRecaptchareturning the widget element. Once the.lengthchecks are fixed, this should behave as before without relying on jQuery.
747-775: validateRecaptcha: DOM-based lookup and error wiring look correctNormalizing
formtoformEl, early-returning on falsy or missing.frm-g-recaptcha, and then:
- Reading
data-ridviadataset.- Using a
try/catcharoundgrecaptcha.getResponse.- Resolving the field container via
closest('.frm_form_field')and deriving its ID.all mirror the original intent without
.find(). Null checks aroundformEland the container prevent obvious crashes.
1062-1066: afterFormSubmitted: safer content inspection via a detached divParsing
response.contentinto a temporarydivand usingquerySelector('.frm_message')to decide betweenfrmFormCompleteandfrmPageChangedavoids jQuery.find()and prevents accidental execution of inline scripts during the check. The event payload remains unchanged.
1082-1085: afterFormSubmittedBeforeReplace: consistent with afterFormSubmittedSame pattern as
afterFormSubmitted, but for the “before replace” hook. Using a detacheddivandquerySelector('.frm_message')is appropriate and keeps behavior focused on markup structure rather than jQuery.
1147-1199: addFieldError: DOM-based implementation preserves behavior and improves ARIA handlingHighlights:
- Accepts either jQuery or HTMLElement, normalizing to a container element.
- Bails out if the container is hidden (
offsetParent === null), matching prior intent.- Builds error markup when needed, preserving
frm_js.include_alert_roleandgetErrorElementId.- Updates
aria-describedbyin an order-aware way usingdata-error-first.- Marks either the input or its radiogroup/checkbox group as
aria-invalid="true".- Still fires the
frmAddFieldErrorjQuery event with a jQuery-wrapped container.This looks like a faithful port off
.find()with better accessibility support.
1220-1260: removeFieldError correctly mirrors addFieldError and handles both DOM and jQuery containersKey points:
- Normalizes
fieldContto a DOM element and early-returns if missing.- Removes
frm_blank_field/has-errorclasses.- Resets
aria-invalideither on the input or, for radios/checkboxes, on the associated[role="group"/"radiogroup"].- Removes the first
.frm_errorelement, then carefully cleansaria-describedby, stripping the error ID while keeping any remaining references.The small detail of temporarily removing
aria-describedbyand re-adding a cleaned value is fine and keeps attributes in sync.
1263-1268: removeAllErrors: DOM-native cleanup is straightforward and safeUsing three
document.querySelectorAllcalls to:
- Clear
frm_blank_field/has-errorclasses from.form-field.- Remove
.frm_errorelements.- Remove
.frm_error_styleelements.matches the old jQuery behavior and has clear, predictable effects.
1328-1339: showFileLoading: DOM version preserves intentSwitching to
document.getElementById('frm_loading')plusobject.querySelector('input[type=file]')and checkingfileInput.valuebefore showing the loader after 2 seconds retains the original semantics, assuming a single file field per form (which matches how jQuery.val()behaves on a collection).
1347-1349: confirmClick: usingdataset.frmconfirmis a clean replacementReading the confirmation message via
this.dataset.frmconfirmand thenconfirm(message)is equivalent to usingdata('frmconfirm')orgetAttribute('data-frmconfirm'), and keeps the handler simple and dependency-free.
1358-1361: onHoneypotFieldChange: directgetComputedStyleis appropriateUsing
window.getComputedStyle(this).boxShadowand checking for/inset/is an accurate way to detect WebKit autofill and remove the honeypot field without involving jQuery. Thevalidthishint is preserved.
1926-1931: submitFormNow: file field detection via DOM APIs looks correct
querySelectorAll('input[type="file"]')combined withArray.from(...).filter(input => !!input.value).lengthis an accurate port of the old “any file inputs with a value?” logic, and the guarded read of thefrm_actionhidden input viaquerySelectoris safe.
1946-1948: validateFormSubmit: tinyMCE save guard correctly handles jQuery or DOM formsNormalizing
objecttoformwithinstanceof jQuery ? .get(0) : objectand checking for.wp-editor-wrapbefore callingtinyMCE.triggerSave()keeps the integration working whether the caller passes a jQuery-wrapped form or a plain element.
1970-1976: getAjaxFormErrors: action detection via DOM is a reasonable enhancementThe added
formnormalization andquerySelector('input[name="frm_action"]')to derive the action forfrmThemeOverride_jsErrorsis sound, with null checks so a missing input just yields an empty string. Passing the originalobjectinto the override keeps the existing hook contract intact.
1997-2006: addAjaxFormErrors: DOM-based lookup of field containers is solidNormalizing
objecttoformand usingform.querySelector('#frm_field_' + key + '_container')to find each field container, then:
- Calling
addFieldErrorwhen found, or- Deleting the error key when not,
is a clean port from
.find()and keeps error state in sync with the rendered form.
2035-2037: scrollMsg: robust handling of jQuery or DOMobjectwhen scrolling to a fieldIn the
typeof id === 'string'branch, resolvingformElviaobject instanceof jQuery ? object.get(0) : object, then usingquerySelector('#frm_field_' + id + '_container')and wrapping the result back in jQuery is a good compromise: DOM APIs for lookup, jQuery for existing scroll/offset logic.
Crabcyborg
changed the title
.find().find() in formidable.js
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
js/formidable.js (1)
716-735: Invisible reCAPTCHA branch is broken by.lengthchecks on an HTMLElement
hasInvisibleRecaptchanow returns either anHTMLElementorfalse, butsubmitFormManualstill treats the result as an array-like and checks.length:
- Line 1873:
if ( classList && invisibleRecaptcha.length < 1 ) { ... }- Line 1895:
if ( invisibleRecaptcha.length ) { ... }For an
HTMLElement,invisibleRecaptcha.lengthisundefined, so:
invisibleRecaptcha.length < 1is alwaysfalse, so the Pro-only guard no longer runs when there is no invisible reCAPTCHA.if ( invisibleRecaptcha.length )never passes, soexecuteInvisibleRecaptchais never called even when an invisible widget is present.This effectively disables the invisible reCAPTCHA flow and changes gating behavior for non‑Pro forms. The checks should use truthiness of the element instead of
.length.Suggested fix:
- if ( classList && invisibleRecaptcha.length < 1 ) { + // If there is no invisible reCAPTCHA, enforce Pro-only restriction. + if ( classList && ! invisibleRecaptcha ) { isPro = classList.indexOf( 'frm_pro_form' ) > -1; if ( ! isPro ) { return; } } @@ - if ( invisibleRecaptcha.length ) { + if ( invisibleRecaptcha ) { showLoadingIndicator( jQuery( object ) ); executeInvisibleRecaptcha( invisibleRecaptcha ); } else {This aligns
submitFormManualwith the newhasInvisibleRecaptchacontract and restores the intended behavior.Also applies to: 738-744, 746-778, 1868-1903
🧹 Nitpick comments (2)
js/formidable.js (2)
1220-1268: removeFieldError/removeAllErrors: consider removing all.frm_errornodes in a containerThe DOM-based
removeFieldErrorandremoveAllErrorsimplementations correctly:
- Accept either jQuery or
HTMLElement.- Clear
frm_blank_field/has-errorclasses.- Reset
aria-invalidon inputs or their radio/checkbox groups.- Clean up
aria-describedbyto drop the specific error ID.One small robustness improvement:
removeFieldErrorcurrently removes only the first.frm_errorin the container:const errorMessage = container.querySelector( '.frm_error' ); const errorId = errorMessage ? errorMessage.id : ''; ... if ( errorMessage ) { errorMessage.remove(); }If a theme override or custom code ever injects multiple
.frm_errorelements into a single field container, the extras would be left behind. You can cheaply handle that case while still using the first ID foraria-describedbycleanup:- const errorMessage = container.querySelector( '.frm_error' ); - const errorId = errorMessage ? errorMessage.id : ''; + const errorMessages = container.querySelectorAll( '.frm_error' ); + const errorId = errorMessages.length ? errorMessages[ 0 ].id : ''; @@ - if ( errorMessage ) { - errorMessage.remove(); - } + if ( errorMessages.length ) { + errorMessages.forEach( msg => msg.remove() ); + }
removeAllErrorsalready clears all.frm_errorand.frm_error_styleelements globally, so this is just a defensive improvement for the per-field path.
848-1035: Normalize DOM/jQuery handling in form-level helpers to reduce duplication and hardening riskAcross
getFormErrors,submitFormNow,validateFormSubmit,getAjaxFormErrors,addAjaxFormErrors, andscrollMsg, you’re sometimes treatingobjectas a DOM node (usingquerySelector(All),classList, etc.) and in other places wrapping it in jQuery (jQuery( object )) or normalizing to aformvariable (object instanceof jQuery ? object.get( 0 ) : object).Internally this works because current call sites pass an actual
<form>element into these methods, butfrmFrontForm.checkFormErrorsand friends are part of the public API surface and could be called with a jQuery form in custom code.To make this more robust and DRY:
In each of these helpers, derive a single
formElup front, e.g.:const formEl = object instanceof jQuery ? object.get( 0 ) : object; if ( ! formEl ) { return; }Use
formElconsistently for DOM APIs (querySelectorAll,classList.contains,closest, etc.) and only wrap it in jQuery when you explicitly need jQuery behavior (serialization, events).This would:
- Guard against a potential
TypeErrorif any external caller passes a jQuery form intofrmFrontForm.checkFormErrors.- Remove repeated
instanceof jQuery ? .get(0)snippets spread across multiple methods.Also applies to: 1925-1937, 1945-1990, 1996-2014, 2021-2069
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(25 hunks)
⏰ 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 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (5)
js/formidable.js (5)
126-168: Submit / Save Draft controls: DOM refactor looks soundThe conversion from
$form.find(...)to resolving a realformelement ($form instanceof jQuery ? $form.get( 0 ) : $form) plusquerySelectorAll(...).forEach(...)to toggledisabled/pointerEventsis correct and safely no-ops when given an empty jQuery object or falsy input. Behavior for submit buttons and Save Draft links should remain unchanged.
193-247: validateForm: safer DOM access while preserving behaviorNormalizing
objectto a plain element (vanillaJsObject) and usingquerySelectorAllwith optional chaining prevents errors when a jQuery object or falsy value is passed in, while keeping the existing required-field and recaptcha validation semantics intact. The invisible vs visible reCAPTCHA split viahasInvisibleRecaptcha( object )continues to behave correctly.
387-482: Required field handling for typed signature fields is improvedThe new branch that, for signature fields, falls back to the
[output]field inside the same.frm_form_fieldwhen the[typed]input is empty is a solid enhancement. Theclosest/querySelectorusage is null-safe, and the subsequentfieldIDnormalization (replace('-typed', '')) keeps IDs consistent with previous behavior.
1147-1199: addFieldError: HTMLElement/jQuery interop and ARIA updates preservedHandling either a jQuery object or an
HTMLElementinaddFieldErrorviainstanceof jQuery ? .get(0) : ...is clean, and the early return whencontainer.offsetParent === nullcorrectly skips hidden fields. The new DOM-based implementation still:
- Adds
frm_blank_fieldto the container.- Appends the error HTML (or defers to
frmThemeOverride_frmPlaceError).- Maintains
aria-describedbyordering based ondata-error-first.- Marks the input or its radiogroup/checkbox group as
aria-invalid="true".The final jQuery event trigger (
frmAddFieldError) maintains backward compatibility with existing listeners.
1328-1363: showFileLoading / confirmClick / honeypot handler: lean DOM usage looks correctThe refactors here look good:
showFileLoadingnow usesdocument.getElementById/querySelectorwith simple null checks, preserving the “only show loading when a file has a value for 2s” behavior.confirmClickreadingthis.dataset.frmconfirmis equivalent to the olddata('frmconfirm')fordata-frmconfirmattributes.onHoneypotFieldChangeusinggetComputedStyle( this ).boxShadowand testing/inset/keeps the previous autofill-detection semantics without needing jQuery.All three are safe, side-effect free refactors away from
.find()and jQuery-only APIs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
js/formidable.js (6)
126-133: Submit/save‑draft helpers: DOM conversion looks good; consider normalizing argument typesThe conversions to
querySelectorAllwithHTMLElement|jQueryhandling are sane and avoid.find(). One minor nit is thatenableSaveDraftstill assumes a jQuery object whiledisableSaveDraft/disableSubmitButton/enableSubmitButtonnow normalize both. For long‑term consistency, you might consider normalizing the$formargument inenableSaveDraftthe same way so all four helpers accept either type.Also applies to: 143-150, 161-167
712-737: Invisible reCAPTCHA flow now treats the element correctly; consider guardinggrecaptchaaccess
hasInvisibleRecaptchareturning anHTMLElement|falseandsubmitFormManualcheckingif ( invisibleRecaptcha )/! invisibleRecaptchafixes the earlier.lengthbug and matches the updated contract.executeInvisibleRecaptchaandvalidateRecaptchaalso correctly pull the widget ID fromdata-ridand work on the DOM element.One robustness improvement:
hasInvisibleRecaptchaandexecuteInvisibleRecaptchacallgrecaptchawithout a guard, whereas the reset logic ingetFormErrorswrapsgrecaptchausage in atypeof grecaptcha !== 'undefined'check. To avoid hard failures on misconfigured pages/tests where.frm-g-recaptchamarkup exists but the API isn’t loaded yet, you could mirror that guard (or bail out early iftypeof grecaptcha === 'undefined').Also applies to: 740-748, 751-777
852-857:getFormErrorsDOM refactor is sound, but it now assumes a rawHTMLElementThe refactor to use
object.querySelectorAll/classListand DOM parsing forresponse.contentlooks correct and the invisible/turnstile captcha resets are preserved.One behavioral change to note:
getFormErrors(and thusfrmFrontForm.checkFormErrors) now assumesobjectis anHTMLElementand callsquerySelectorAlldirectly, while most other entry points (validateFormSubmit,getAjaxFormErrors,addAjaxFormErrors,scrollMsg) are tolerant of eitherHTMLElementorjQueryand normalize withinstanceof jQuery. If any third‑party code callsfrmFrontForm.checkFormErrors( jQuery(... ) ), this will now throw.If you want to keep backward compatibility aligned with the rest of the API, you could normalize here as well, e.g.:
- function getFormErrors( object, action ) { + function getFormErrors( object, action ) { + const form = object instanceof jQuery ? object.get( 0 ) : object; + if ( ! form ) { + return; + } - const fieldsets = object.querySelectorAll( '.frm_form_field' ); + const fieldsets = form.querySelectorAll( '.frm_form_field' ); ... - data = jQuery( object ).serialize() + '&action=frm_entries_' + action + '&nonce=' + frm_js.nonce; - shouldTriggerEvent = object.classList.contains( 'frm_trigger_event_on_submit' ); + data = jQuery( form ).serialize() + '&action=frm_entries_' + action + '&nonce=' + frm_js.nonce; + shouldTriggerEvent = form.classList.contains( 'frm_trigger_event_on_submit' ); ... - const formIdInput = object.querySelector( 'input[name="form_id"]' ); + const formIdInput = form.querySelector( 'input[name="form_id"]' ); ... - const fieldContEl = object.querySelector( '#frm_field_' + key + '_container' ); + const fieldContEl = form.querySelector( '#frm_field_' + key + '_container' ); ... - object.querySelectorAll( '.frm-g-recaptcha, .g-recaptcha, .h-captcha' ).forEach( + form.querySelectorAll( '.frm-g-recaptcha, .g-recaptcha, .h-captcha' ).forEach( ... - object.querySelectorAll( '.frm-cf-turnstile' ).forEach( + form.querySelectorAll( '.frm-cf-turnstile' ).forEach( ... - object.querySelectorAll( 'input[type="submit"], input[type="button"]' ).forEach( + form.querySelectorAll( 'input[type="submit"], input[type="button"]' ).forEach(and keep passing the original
objectwhere you intentionally want the caller’s type (e.g., when triggering events).Also applies to: 927-929, 943-949, 966-969, 990-1007, 1013-1013, 1031-1034
1152-1203:addFieldErrorDOM refactor is good; small ARIA improvement for radios/checkboxesThe new implementation correctly:
- Normalizes
HTMLElement|jQuerycontainers.- Skips hidden containers via
offsetParent.- Builds error markup or uses theme overrides as before.
- Maintains
aria-describedbyordering with thedata-error-firstconvention.- Sets
aria-invalidon either the grouped control or the input.One subtle improvement: when the field is a radio/checkbox but there is no
[role="radiogroup"]/[role="group"]wrapper, nothing gets marked invalid. A simple fallback keeps accessibility intact:- if ( input ) { - if ( [ 'radio', 'checkbox' ].includes( input.type ) ) { - const group = input.closest( '[role="radiogroup"], [role="group"]' ); - if ( group ) { - group.setAttribute( 'aria-invalid', 'true' ); - } - } else { - input.setAttribute( 'aria-invalid', 'true' ); - } - } + if ( input ) { + if ( [ 'radio', 'checkbox' ].includes( input.type ) ) { + const group = input.closest( '[role="radiogroup"], [role="group"]' ); + if ( group ) { + group.setAttribute( 'aria-invalid', 'true' ); + } else { + input.setAttribute( 'aria-invalid', 'true' ); + } + } else { + input.setAttribute( 'aria-invalid', 'true' ); + } + }This preserves the preferred group behavior but still flags standalone radio/checkbox inputs as invalid.
1224-1264:removeFieldError/removeAllErrors: ARIA cleanup could be more complete for AJAX error cycles
removeFieldErrormirrors the newaddFieldErrorbehavior well: it normalizesHTMLElement|jQuery, removes classes, clearsaria-invalid(input or group), drops the error node, and strips its ID fromaria-describedbywhile preserving other IDs.
removeAllErrors, however, only:
- Removes classes from
.form-fieldwrappers.- Removes all
.frm_errorand.frm_error_styleelements.If a field had errors on a previous AJAX submission but no longer has any, its input can still be left with
aria-invalid="true"and anaria-describedbypointing at an ID that no longer exists, becauseremoveFieldErroris never called in that path.If you want the ARIA state to reflect the current error set, consider using
removeFieldErrorper field container, e.g.:- function removeAllErrors() { - document.querySelectorAll( '.form-field' ).forEach( field => { - field.classList.remove( 'frm_blank_field', 'has-error' ); - } ); - document.querySelectorAll( '.form-field .frm_error' ).forEach( error => error.remove() ); - document.querySelectorAll( '.frm_error_style' ).forEach( error => error.remove() ); - } + function removeAllErrors() { + document.querySelectorAll( '.form-field' ).forEach( field => { + removeFieldError( field ); + } ); + document.querySelectorAll( '.frm_error_style' ).forEach( error => error.remove() ); + }You may want to extend
removeFieldErrorto remove all.frm_errorchildren if multiple can exist per field.Also applies to: 1267-1272
1950-1965: Form‑level validation path: good normalization; minor consistency nitsThe validation/get‑errors/add‑errors/scroll path looks solid:
validateFormSubmitnow only callstinyMCE.triggerSave()when the current form actually contains.wp-editor-wrap, using a normalizedformelement.getAjaxFormErrorsandaddAjaxFormErrorscorrectly normalizeHTMLElement|jQueryto a DOM element for DOM queries but still pass the originalobjectto theme overrides/events.addAjaxFormErrorsnow usesquerySelector('#frm_field_' + key + '_container')and callsaddFieldErrorwith a plain element, which matches the refactored helper.scrollMsg’s newtypeof id === 'string'branch correctly handles the(fieldId, form)signature using DOM queries while keeping existing jQuery‑only call patterns working.The only consistency nit is that some helpers (
validateFormSubmit,getAjaxFormErrors,addAjaxFormErrors,scrollMsg) normalizeobject instanceof jQuery, whilegetFormErrorscurrently does not. AligninggetFormErrorswith this pattern (as noted in the earlier comment) would make the API surface more predictable.Also applies to: 1972-1984, 2000-2018, 2039-2042
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(26 hunks)
⏰ 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). (1)
- GitHub Check: Cypress
🔇 Additional comments (6)
js/formidable.js (6)
459-462: Typed signature required check: fallback handling looks safeUsing
closest('.frm_form_field')and a scopedquerySelectorto pull the[output]input is a reasonable replacement and safely handles missing containers/outputs by falling back to an empty string.
1066-1069: Using a temporary div to inspectresponse.contentis fineParsing
response.contentvia a detached<div>and checking for.frm_messagebefore firing completion/page‑change events is a clean, side‑effect‑free replacement for the older jQuery traversal.Also applies to: 1086-1089
1332-1343:showFileLoadingDOM usage is straightforward and safeThe switch to
document.getElementByIdandobject.querySelector('input[type=file]')with a null‑safe value check keeps the old behavior while avoiding.find(). It’s only called with a real form element, so the direct DOM usage is appropriate.
1352-1353:confirmClickand honeypot change handlers: dataset/getComputedStyle usage looks fineUsing
this.dataset.frmconfirmandwindow.getComputedStyle( this ).boxShadowis a clean, modern replacement. Both handlers are only bound to real DOM elements via jQuery delegation, so relying ondatasetandgetComputedStylehere is reasonable.Also applies to: 1362-1365
1777-1783: Invisible reCAPTCHA checks insubmitFormManualnow align with the updated helperThe updated checks:
if ( classList && ! invisibleRecaptcha ) { ... }for the Pro gating.if ( invisibleRecaptcha ) { executeInvisibleRecaptcha( invisibleRecaptcha ); } else { ... }correctly treat
hasInvisibleRecaptcha’s return value as an element/falseinstead of relying on.length, addressing the earlier dead‑code path for invisible reCAPTCHA execution.Also applies to: 1899-1902
1929-1935: File field detection/frm_actionlookup via DOM is correctUsing
querySelectorAll('input[type="file"]')plusArray.from(...).filter( input => !! input.value ).lengthmatches the old jQuery semantics and properly gates the AJAX path vs full submit. The null‑safefrm_actionlookup viaquerySelectoris also appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/formidable.js (1)
852-1040:getFormErrorsuses DOM APIs directly, risking errors if jQuery objects are passedThe exported
checkFormErrorsalias forgetFormErrorsdirectly calls DOM APIs on theobjectparameter:
object.querySelectorAll(...)object.classList.contains(...)object.submit()object.querySelector(...)object.insertAdjacentHTML(...)If external code calls
frmFrontForm.checkFormErrors( jQuery( form ), action ), these DOM API calls will fail on the jQuery instance. The same applies topostToAjaxUrl, which is called withobject.To maintain backward compatibility, normalize the input once:
function getFormErrors( object, action ) { - let data, success, error, shouldTriggerEvent; - - const fieldsets = object.querySelectorAll( '.frm_form_field' ); + let data, success, error, shouldTriggerEvent; + + const form = object instanceof jQuery ? object.get( 0 ) : object; + if ( ! form ) { + return; + } + + const fieldsets = form.querySelectorAll( '.frm_form_field' ); fieldsets.forEach( field => field.classList.add( 'frm_doing_ajax' ) );Replace remaining
objectreferences withformthroughout the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(26 hunks)
⏰ 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 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (10)
js/formidable.js (10)
126-167: Submit/save button & draft link handling with native DOM APIs looks solidNormalizing
$formto a single HTMLElement and usingquerySelectorAllto toggledisabledandpointerEventskeeps behavior consistent for both jQuery and raw DOM callers. This matches existing call sites (which pass a single form) and keeps the save‑draft behavior separated but coherent.
387-470: Signature “typed” value resolution looks correctThe new logic that, for empty typed signature inputs, finds the
[output]field within the same.frm_form_fieldcontainer and uses its value before stripping-typedfromfieldIDmatches the expected signature field structure and maintains prior semantics while avoiding jQuery.find(). No issues spotted.
750-782:validateRecaptchaHTMLElement/jQuery support and error mapping look goodThe refactor to normalize
form/formEl, query.frm-g-recaptcha, usedataset.rid, and map an empty response to an error key derived from.frm_form_field’s ID keeps behavior aligned with the previous jQuery version while handling both jQuery and raw DOM inputs safely. Thetry/catcharoundgrecaptcha.getResponsewith therecaptcha_checkedescape hatch is also preserved correctly.
1066-1093: DOM‑fragment based detection of completion message is a nice cleanupUsing a temporary
divandquerySelector( '.frm_message' )in bothafterFormSubmittedandafterFormSubmittedBeforeReplaceis a straightforward and robust way to distinguish between “form complete” and “page changed” responses. That avoids brittle string checks onresponse.contentand doesn’t change the contract of the triggered events.
1152-1273: Error rendering/clearing refactor improves HTMLElement/jQuery interop and ARIA handlingThe updates to
addFieldError,removeFieldError, andremoveAllErrorslook good:
- They gracefully accept either a jQuery object or an
HTMLElement.- They avoid
.find()in favor ofquerySelectorandclosest.- ARIA attributes (
aria-describedby,aria-invalid) are applied and cleared carefully, including grouping for radio/checkbox fields via[role="radiogroup"], [role="group"].- Hidden/non‑rendered containers short‑circuit via
offsetParent === null, which prevents errors being attached to collapsed/hidden sections.The logic for preserving any existing
aria-describedbytokens while removing only the error element ID is also correct. No functional issues spotted here.
1332-1368: File‑upload spinner and honeypot/autofill detection refactor look correct
showFileLoadingnow usesdocument.getElementByIdandobject.querySelector( 'input[type=file]' ), mirroring the old.find()/.val()behavior while still using jQuery only for thefadeInanimation. It safely exits when the loading element or file input is missing.confirmClickandonHoneypotFieldChangecorrectly switch todataset.frmconfirmandgetComputedStyle( this ).boxShadowrespectively, usingthisfrom delegated event handlers as before.These changes avoid jQuery
.find()without altering observable behavior.
1755-1852: Captcha label fix and render path are consistent and robustThe new
maybeFixCaptchaLabelhelper and its use inrenderCaptcha:
- Correctly locate the parent
<form>of the captcha element.- Find both the reCAPTCHA and Turnstile labels and response inputs and ensure
label.htmlFormatches the actual response inputid.- Are called after
data-ridis set, so DOM is ready.This should resolve label/response mismatches without impacting existing render behavior for visible or invisible captchas.
1873-1935: Invisible reCAPTCHA execution path now correctly uses element truthiness
submitFormManualnow treats the value fromhasInvisibleRecaptchaas anHTMLElement|falseand uses a simple truthy check:if ( invisibleRecaptcha ) { showLoadingIndicator( jQuery( object ) ); executeInvisibleRecaptcha( invisibleRecaptcha ); } else { showSubmitLoading( jQuery( object ) ); frmFrontForm.submitFormNow( object ); }This fixes the earlier
.lengthmisuse while preserving the intended flow of:
- Execute invisible reCAPTCHA when present and not yet checked.
- Otherwise show the normal submit loading state and submit (AJAX or normal).
The additional checks in
submitFormNowfor file fields and antispam/unique ID inputs remain intact.
1943-2012: Normalization of form elements in validation and AJAX error rendering looks goodThe refactors in:
validateFormSubmit(usingform.querySelector( '.wp-editor-wrap' )beforetinyMCE.triggerSave()),getAjaxFormErrors(using a normalizedformto readfrm_action),addAjaxFormErrors(normalizingobjecttoformand querying containers withquerySelector),all correctly support both jQuery and plain
HTMLElementinputs while removing.find()usage. The use ofscrollToFirstField( object )andcheckForErrorsAndMaybeSetFocus()after populatingjsErrorsmaintains the existing UX. Aside from the separategetFormErrorsconcern noted earlier, these pieces look solid.
2024-2067:scrollMsgDOM lookup for field containers maintains behaviorIn the
typeof id === 'string'branch, converting a possible jQueryobjectto a raw form element and then usingquerySelector( '#frm_field_' + id + '_container' )before wrapping it back in jQuery keeps the old semantics while avoiding.find(). This still supports both jQuery and raw form objects without changing howscrollObj.offset().topis computed downstream.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
js/formidable.js (2)
744-748: Missinggrecaptchaguards inexecuteInvisibleRecaptcha.Lines 746-747 call
grecaptcha.reset()andgrecaptcha.execute()without verifying thatgrecaptchais defined. This will throw a ReferenceError if the reCAPTCHA script is unavailable.Apply this diff to guard all
grecaptchacalls:function executeInvisibleRecaptcha( invisibleRecaptcha ) { + if ( typeof grecaptcha === 'undefined' || ! grecaptcha ) { + return; + } const recaptchaID = invisibleRecaptcha.dataset.rid; grecaptcha.reset( recaptchaID ); grecaptcha.execute( recaptchaID ); }
717-737: Missinggrecaptchaguard causes ReferenceError when reCAPTCHA script is blocked.Line 730 calls
grecaptcha.getResponse(recaptchaID)without checking ifgrecaptchais defined. If the Google reCAPTCHA script fails to load or is blocked by an ad blocker, this throws a ReferenceError and breaks form submission.Line 993 demonstrates the correct guard pattern used elsewhere:
typeof grecaptcha !== 'undefined' && grecaptcha.Apply this diff to guard
grecaptchaaccess:const recaptcha = form.querySelector( '.frm-g-recaptcha[data-size="invisible"], .g-recaptcha[data-size="invisible"]' ); if ( recaptcha ) { const recaptchaID = recaptcha.dataset.rid; - const alreadyChecked = grecaptcha.getResponse( recaptchaID ); + let alreadyChecked = ''; + if ( typeof grecaptcha !== 'undefined' && grecaptcha ) { + alreadyChecked = grecaptcha.getResponse( recaptchaID ); + } if ( alreadyChecked.length === 0 ) { return recaptcha; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(26 hunks)
⏰ 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: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (3)
js/formidable.js (3)
990-1004: No action needed. Bothhcaptcha(line 1001) andturnstile(line 1006) already have proper safety guards in the reset logic. Thehcaptchacheck usestypeof hcaptcha !== 'undefined' && hcaptchaandturnstileusesif ( window.turnstile ), consistent with thegrecaptchahandling pattern.
127-150: The asymmetry betweendisableSubmitButtonandenableSubmitButtonregardingbutton.frm_save_draftis intentional and not an issue. Analysis of call sites shows these functions are always invoked as paired operations:disableSubmitButtonanddisableSaveDraftare called together (line 1292–1293), andenableSubmitButtonandenableSaveDraftare called together (line 1327–1328). This design pattern ensures all disabled elements are properly re-enabled through separate, responsibility-specific functions. No changes needed.
1889-1893: All call sites correctly handle theHTMLElement | falsereturn type.Verification confirms both call sites handle the return type appropriately:
- Line 242:
if ( ! hasInvisibleRecaptcha( object ) )— negation operator correctly handles bothfalseandHTMLElement- Line 1889: Assigns result to
const invisibleRecaptcha, then uses truthy checkif ( invisibleRecaptcha )— correctly distinguishesfalsefromHTMLElement
Stop using jQuery `.find()` in `formidable.js`
1 participant
Related issue https://github.com/Strategy11/formidable-pro/issues/5002
This could use more testing.
This update mostly touches code for:
Tested