Add autocomplete attribute to the first name field of the form#2595
Conversation
WalkthroughAdds a reusable method in FrmFieldCombo to build sub-field input attributes, overrides it in FrmFieldName to track per-form first-name fields and apply autocomplete to "first"/"last" sub-fields, updates the combo-field front-end view to use the new method, and registers the tracked first-name field during form field loading. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/models/fields/FrmFieldCombo.php(1 hunks)classes/models/fields/FrmFieldName.php(2 hunks)classes/views/frm-fields/front-end/combo-field/combo-field.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
classes/views/frm-fields/front-end/combo-field/combo-field.php (2)
classes/models/fields/FrmFieldCombo.php (1)
get_sub_field_input_attrs(517-533)classes/models/fields/FrmFieldName.php (1)
get_sub_field_input_attrs(306-327)
classes/models/fields/FrmFieldCombo.php (1)
classes/models/fields/FrmFieldName.php (1)
get_sub_field_input_attrs(306-327)
classes/models/fields/FrmFieldName.php (2)
classes/models/fields/FrmFieldCombo.php (1)
get_sub_field_input_attrs(517-533)classes/models/FrmField.php (2)
FrmField(6-1426)get_option(1279-1287)
⏰ 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). (9)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Run ESLint
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Run ESLint
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (3)
classes/models/fields/FrmFieldCombo.php (1)
517-533: Well-structured attribute construction method.The centralized approach for building sub-field input attributes improves maintainability and enables specialized behavior in child classes (like the autocomplete additions in FrmFieldName). The logic correctly handles value population and conditional name attributes.
classes/views/frm-fields/front-end/combo-field/combo-field.php (1)
60-60: Clean refactoring to centralized attribute construction.The change delegates sub-field input attribute construction to the field type class's
get_sub_field_input_attrs()method. This enables child classes like FrmFieldName to inject specialized attributes (such as autocomplete) while keeping the view template clean and maintainable.classes/models/fields/FrmFieldName.php (1)
316-324: HTML autocomplete attribute values are valid per the standard.The values
given-nameandfamily-namecomply with the HTML standard as specified in the WHATWG and MDN documentation.Per-form tracking behavior is intentional and documented.
The code design explicitly implements per-form tracking using the static property
$added_autocomplete_attrkeyed by form ID (line 21). The documented purpose (lines 16–17) states: "Track if the autocomplete attribute is added to a name field in each form." This prevents duplicate autocomplete attributes within a single form by allowing the attributes only once per form. The implementation also excludes name fields inside repeaters (lines 309–314). This is an intentional design decision, not a limitation requiring remediation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2595 +/- ##
============================================
- Coverage 26.95% 26.38% -0.57%
- Complexity 8838 8852 +14
============================================
Files 144 144
Lines 29803 29932 +129
============================================
- Hits 8032 7899 -133
- Misses 21771 22033 +262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
classes/models/fields/FrmFieldName.php (3)
296-312: Update the @SInCE version placeholder.Line 299 uses
@since x.xas a placeholder. Please update it to the actual release version number before merging.
314-329: Update the @SInCE version placeholder.Line 317 uses
@since x.xas a placeholder. Please update it to the actual release version number before merging.
331-335: The repeater field detection logic is fundamentally flawed.Based on detailed past review analysis, this code will not correctly identify fields inside repeaters:
Incorrect data source:
parent_form_idis a form-level property stored in thefrm_formstable, not a field option.FrmField::get_option()retrieves field options, not form properties.Inverted logic: The condition returns early when IDs don't match, but the correct approach is to check if the field's form has a non-zero
parent_form_idproperty.Correct approach: Load the form record via
FrmForm::getOne($form_id)and check the form object'sparent_form_idproperty.Apply this diff to fix the detection:
- $parent_form_id = (int) FrmField::get_option( $args['field'], 'parent_form_id' ); - if ( $form_id !== $parent_form_id ) { - // Do not add autocomplete attribute to a name field inside repeater. - return $attrs; - } + $form = FrmForm::getOne( $form_id ); + if ( $form && ! empty( $form->parent_form_id ) ) { + // Do not add autocomplete attribute to a name field inside repeater. + return $attrs; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmFieldsHelper.php(1 hunks)classes/models/fields/FrmFieldName.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/helpers/FrmFieldsHelper.php (1)
classes/models/fields/FrmFieldName.php (2)
FrmFieldName(13-349)track_first_name_field(305-312)
🪛 GitHub Actions: Inspections
classes/models/fields/FrmFieldName.php
[error] 1-1: PHP CS Fixer detected issue (phpdoc_separation). File would be fixed by running the formatter. Command: './vendor/friendsofphp/php-cs-fixer/php-cs-fixer fix --dry-run --verbose'. Exit code: 8.
⏰ 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 (2)
classes/models/fields/FrmFieldName.php (1)
337-348: Autocomplete attribute logic is correct.The implementation correctly applies
autocomplete="given-name"to the "first" sub-field andautocomplete="family-name"to the "last" sub-field for the tracked first name field. This follows HTML autocomplete standards.However, this relies on the repeater detection logic in lines 331-335, which needs to be fixed first.
classes/helpers/FrmFieldsHelper.php (1)
493-494: No action required - name field tracking works correctly with paginated forms.The current implementation correctly tracks name fields regardless of pagination.
FrmFieldsHelper::get_form_fields()retrieves all fields for a form first, thentrack_first_name_field()operates on the complete field set before pagination filtering is applied. Name fields on any page (including page 2) are included in this unfiltered list and will be tracked properly.
|
@Crabcyborg I use another approach to make this work in paged form and with randomized fields setting. Those Pro features are done in this PR https://github.com/Strategy11/formidable-pro/pull/6122 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
classes/models/fields/FrmFieldCombo.php (1)
509-536: LGTM! Well-structured base method for sub-field attribute generation.The method correctly builds the attributes array with proper conditional logic for the
nameattribute based onremove_namesflag. This provides a clean extension point for subclasses likeFrmFieldNameto override and add custom attributes.One minor note: the
@since x.xplaceholder on line 513 was already flagged in a previous review and should be updated before merging.classes/models/fields/FrmFieldName.php (3)
15-20: Fix docblock grammar.As previously noted, line 18 contains a grammatical error.
Apply this diff:
- * @var array Array with keys are form ID and values are name field IDs. + * @var array Array where keys are form IDs and values are name field IDs.
335-339: Verify repeater field exclusion logic.This was flagged in a previous review: the code retrieves
parent_form_idviaFrmField::get_option(), but throughout the codebase,parent_form_idis typically a form-level property, not a field option. Please confirm that this approach correctly identifies fields inside repeaters, or adjust the logic to check the form'sparent_form_idproperty instead.
317-326: Update the @SInCE version placeholder.The
@sincetag shows "x.x" as a placeholder. Please update it to the actual release version number before merging.
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldName.php (1)
341-349: Minor: Inconsistent integer casting.Line 341 uses explicit
(int)cast, while line 343 usesintval()on the already-integer-keyed array value. Consider using consistent casting for clarity.- if ( intval( self::$first_name_field_ids[ $form_id ] ) === $field_id ) { + if ( (int) self::$first_name_field_ids[ $form_id ] === $field_id ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/models/fields/FrmFieldCombo.php(3 hunks)classes/models/fields/FrmFieldName.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
classes/models/fields/FrmFieldName.php (2)
classes/models/fields/FrmFieldCombo.php (1)
get_sub_field_input_attrs(520-536)classes/models/FrmField.php (2)
FrmField(6-1426)get_option(1279-1287)
classes/models/fields/FrmFieldCombo.php (1)
classes/models/fields/FrmFieldName.php (1)
get_sub_field_input_attrs(327-352)
⏰ 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 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (1)
classes/models/fields/FrmFieldName.php (1)
308-315: No changes needed. Thetrack_first_name_fieldmethod correctly receives all form fields, including those on all pages, becauseFrmField::get_all_for_form()retrieves all fields for a form in a single call without page-based filtering. Multi-page structure is metadata on individual fields, not a retrieval mechanism. The method will find the first name field regardless of which page it appears on.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I haven't tested this yet but I think these updates look great!
I'll merge shortly, after I've tried it out.
Thanks @truongwp!
🚀
Add `autocomplete` attribute to the first name field of the form
2 participants
Except the name fields inside Repeater.
Fixes https://github.com/Strategy11/formidable-pro/issues/6099