Fix js error when triggering change event on hidden field#2255
Conversation
WalkthroughThe changes introduce an additional check in the Changes
Sequence Diagram(s)sequenceDiagram
participant VF as validateField()
participant FC as Field Container Check
VF->>VF: Check for field container
alt Field container is absent (hidden field)
VF-->>VF: Return early (skip validation)
else Field container present
VF-->>VF: Continue with validation logic
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/formidable.js (1)
271-279: Consider consolidating hidden field handling into a utility function.The codebase has multiple places where hidden fields are checked and handled differently. Consider extracting this logic into a utility function like
isHiddenFieldorshouldValidateFieldto improve maintainability and consistency.Example implementation:
+function isHiddenField(field) { + return field.type === 'hidden' && !field.closest('.frm_form_field'); +} function maybeValidateChange(field) { if (field.type === 'url') { maybeAddHttpToUrl(field); } const form = field.closest('form'); - if (form && hasClass(form, 'frm_js_validate')) { + if (form && hasClass(form, 'frm_js_validate') && !isHiddenField(field)) { validateField(field); } } function validateField(field) { let errors, key; errors = []; const fieldContainer = field.closest('.frm_form_field'); - if (!fieldContainer) { - // Hidden fields do not have a field container and do not require JS validation. - return; - } + if (isHiddenField(field)) { + return; + } if (hasClass(fieldContainer, 'frm_required_field') && !hasClass(field, 'frm_optional')) { errors = checkRequiredField(field, errors); }Also applies to: 299-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: PHP 8 tests in WP trunk
- 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
🔇 Additional comments (2)
js/formidable.js (2)
305-308: LGTM! The changes effectively prevent JS errors for hidden fields.The added check for field containers is a robust solution that:
- Prevents null reference errors when validating hidden fields
- Maintains clean code by explicitly documenting the behavior
- Aligns with the PR's objective to fix JS errors for hidden fields
305-308:Details
❓ Verification inconclusive
Verify handling of dynamically shown hidden fields.
The current implementation assumes that hidden fields never require JS validation. Please verify that this assumption holds true when:
- Hidden fields are made visible through JavaScript
- Hidden fields are used for dynamic form behavior
- Hidden fields contain validation rules that should be enforced
Run this script to check for dynamic visibility changes of hidden fields:
🏁 Script executed:
#!/bin/bash # Search for code that might change visibility of hidden fields rg -A 3 "\.show\(\)|\.hide\(\)|style\.display|\.visible|\.hidden"Length of output: 61724
Re-check dynamic validation for fields that transition from hidden to visible.
The current implementation skips JS validation when a field lacks a container (i.e. is hidden). Since our codebase includes several instances where elements are dynamically shown or hidden (as seen in various files like formidable_admin.js and others), please verify that any field initially hidden—and therefore not validated—will be properly revalidated if it later becomes visible. In particular, confirm that:
- Dynamically revealed fields trigger a re-check of any associated validation rules.
- There is adequate handling for validation errors on fields that transition from hidden to visible.
Crabcyborg
deleted the
fix_js_error_when_triggering_change_event_on_hidden_field
branch
1 participant
Fixes https://secure.helpscout.net/conversation/2855293169/223561
Pre-release
formidable-6.18.1b.zip