Set aria-invalid to true only on name part input with error#2036
Conversation
![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
🧹 Outside diff range and nitpick comments (3)
classes/models/FrmFieldFormHtml.php (1)
397-406: Improved accessibility for name fields, consider edge casesThe changes effectively implement the PR objective by setting
aria-invalidattributes specifically for first and last name parts in name fields. This granular approach enhances accessibility for screen readers.Consider the following suggestions for further improvement:
- Add support for potential middle name fields or other name parts that might be present in different cultures or use cases.
- Consider adding a comment explaining the rationale behind this special handling of name fields for future maintainers.
Example implementation:
if ( $this->field_obj->get_field_column( 'type' ) === 'name' ) { + // Special handling for name fields to provide granular accessibility feedback if ( isset( $this->pass_args['errors'][ 'field' . $this->field_id . '-first' ] ) ) { $shortcode_atts['aria-invalid-first'] = 'true'; } if ( isset( $this->pass_args['errors'][ 'field' . $this->field_id . '-last' ] ) ) { $shortcode_atts['aria-invalid-last'] = 'true'; } + // Add support for other potential name parts (e.g., middle name) + if ( isset( $this->pass_args['errors'][ 'field' . $this->field_id . '-middle' ] ) ) { + $shortcode_atts['aria-invalid-middle'] = 'true'; + } } else { $shortcode_atts['aria-invalid'] = isset( $this->pass_args['errors'][ 'field' . $this->field_id ] ) ? 'true' : 'false'; }This approach would make the code more flexible for various name field configurations.
js/formidable.js (2)
1235-1244: LGTM! Consider adding a comment for clarity.The
maybeSetFocusOnNameFieldElementfunction effectively handles focus management for name fields with errors. It's well-structured and addresses accessibility concerns.Consider adding a brief comment explaining the purpose of this function at the beginning, e.g.:
// Sets focus on the first invalid input within a name field container function maybeSetFocusOnNameFieldElement(element) { // ... existing code ... }
1267-1269: LGTM! Consider extracting the condition to a variable for readability.The addition of the 'FIELDSET' check and the call to
maybeSetFocusOnNameFieldElementaligns well with the PR objectives. It improves accessibility for name fields within fieldsets.To improve readability, consider extracting the node name check into a variable:
const isFieldset = 'FIELDSET' === element.nodeName; if (isFieldset) { maybeSetFocusOnNameFieldElement(element); }This makes the code's intent slightly clearer at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- classes/controllers/FrmFieldsController.php (1 hunks)
- classes/models/FrmFieldFormHtml.php (1 hunks)
- classes/models/fields/FrmFieldCombo.php (1 hunks)
- js/formidable.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
classes/models/fields/FrmFieldCombo.php (1)
384-386: LGTM! This change enhances sub-field identification.The addition of the
subfield_nameattribute to the$fieldarray is a positive change. It provides more specific context for each sub-field, which can be beneficial for:
- Improving accessibility by allowing more granular attribute handling.
- Enhancing form validation processes that may need to distinguish between different sub-fields.
- Facilitating more precise error reporting, aligning with the PR's objective of setting
aria-invalidcorrectly.This change is well-implemented with a proper null check, ensuring that the attribute is only added when necessary.
js/formidable.js (1)
Line range hint
1235-1270: Summary: Accessibility improvements for name fieldsThe changes in this file effectively implement the PR objective of improving accessibility for name fields. The new
maybeSetFocusOnNameFieldElementfunction and the modifications tocheckForErrorsAndMaybeSetFocuswork together to ensure that focus is properly set on invalid name field inputs.These changes enhance the user experience by providing more accurate feedback on form validation errors, particularly for name fields. The implementation is clean and focused, with no apparent negative impact on other parts of the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1235-1247: LGTM! Consider adding JSDoc for better documentation.The
maybeSetFocusOnNameFieldElementfunction effectively enhances accessibility for name fields by setting focus on invalid elements. This aligns well with the PR objective.Consider adding JSDoc comments to improve function documentation:
/** * Sets focus on the first invalid name field element within a fieldset. * * @param {HTMLElement} element - The fieldset element to check. */ function maybeSetFocusOnNameFieldElement(element) { // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- js/formidable.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
js/formidable.js (2)
1264-1271: Excellent integration of new functionality!The changes in
checkForErrorsAndMaybeSetFocuseffectively incorporate the newmaybeSetFocusOnNameFieldElementfunction. The additional check for specific input types enhances the precision of focus setting, which aligns well with the PR's accessibility improvement goals.
Line range hint
1235-1271: PR Approved: Excellent accessibility improvements for name fieldsThis PR successfully implements the objective of improving accessibility for name fields:
- A new function
maybeSetFocusOnNameFieldElementis added to handle focus setting for invalid name field elements.- The existing
checkForErrorsAndMaybeSetFocusfunction is updated to integrate this new functionality.These changes enhance the form's accessibility without disrupting existing behavior. The code is clean, well-structured, and aligns with the current coding standards.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
js/formidable.js (3)
1235-1255: Approve with suggestions: Enhance focus management for name fieldsThe new
maybeSetFocusOnNameFieldElementfunction is a good addition for improving accessibility and user experience. It effectively manages focus for name fields with errors. However, consider the following suggestions:
- The function name could be more concise, e.g.,
focusInvalidNameField.- Add a fallback if no invalid field is found to improve error handling.
- Consider handling cases where multiple fields might have errors.
Here's a suggested refactor:
function focusInvalidNameField(element) { if (element.nodeName !== 'FIELDSET' || !element.querySelector('.frm_combo_inputs_container[data-name-layout]')) { return; } const invalidFields = element.querySelectorAll('[aria-invalid="true"]'); if (invalidFields.length) { invalidFields[0].focus(); } }This refactored version addresses the suggestions and simplifies the code structure.
1277-1278: Approve with suggestions: Improve error handling for name fieldsThe addition of
maybeSetFocusOnNameFieldElementin thecheckForErrorsAndMaybeSetFocusfunction is a good improvement for handling name field errors. However, consider the following suggestions:
- Optimize the call to
maybeSetFocusOnNameFieldElementto avoid unnecessary checks for non-name fields.- Consider renaming the function to something more concise, e.g.,
handleErrorsAndFocus.- Refactor the function to reduce complexity and improve readability.
Here's a suggested refactor:
function handleErrorsAndFocus() { if (!frm_js.focus_first_error) { return; } const errors = document.querySelectorAll('.frm_form_field .frm_error'); if (!errors.length) { return; } const firstErrorField = errors[0].closest('.frm_form_field'); if (firstErrorField) { focusInvalidNameField(firstErrorField); if (!document.activeElement || document.activeElement === document.body) { focusFirstInputInField(firstErrorField); } } } function focusFirstInputInField(field) { const input = field.querySelector('input, select, textarea'); if (input) { input.focus(); } }This refactored version simplifies the logic, reduces nesting, and separates concerns for better maintainability.
Line range hint
1235-1278: Overall assessment: Good improvements with room for optimizationThe changes to
formidable.jsfocus on enhancing error handling and focus management for name fields, which is a positive step towards improving accessibility and user experience. The newmaybeSetFocusOnNameFieldElementfunction and the modification tocheckForErrorsAndMaybeSetFocusare well-intentioned additions.However, there's room for optimization in terms of:
- Code structure and naming conventions
- Function complexity and readability
- Efficiency of error handling and focus management
Consider implementing the suggested refactors to further improve the code quality and maintainability. These changes align well with the PR objective of setting
aria-invalidto true only on name part inputs with errors.To further improve the overall architecture:
- Consider creating a separate module for focus management and error handling functions.
- Implement unit tests for these new functions to ensure reliability and ease of maintenance.
- Document the new focus management behavior in the project's accessibility guidelines or documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- classes/controllers/FrmFieldsController.php (1 hunks)
- js/formidable.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
classes/controllers/FrmFieldsController.php (1)
918-927: Verify consistent handling of 'aria-invalid' attributesEnsure that the condition correctly handles cases where
$kis exactly'aria-invalid'versus when it starts with'aria-invalid'. This will help prevent any unintended behavior when processing different keys.Run the following script to check for all usages of
'aria-invalid'keys and confirm their handling:✅ Verification successful
Consistent handling of 'aria-invalid' attributes verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all occurrences of 'aria-invalid' keys in the PHP codebase. # Test: Find lines where 'aria-invalid' is used in arrays. Expect: Proper handling in all instances. grep -nr --include='*.php' "\['aria-invalid" .Length of output: 591
Script:
#!/bin/bash # Description: Search for all occurrences of 'aria-invalid-*' keys in the PHP codebase. # Test: Find lines where keys starting with 'aria-invalid-' are used in arrays. grep -nr --include='*.php' "\['aria-invalid-" .Length of output: 428
Script:
#!/bin/bash # Description: Find all conditionals and assignments involving 'aria-invalid', 'aria-invalid-first', and 'aria-invalid-last' in PHP files. # Using ast-grep to find conditionals and assignments for the specified keys. ast-grep --lang php --pattern ' $_ = $_; if ($_["aria-invalid"] $_@_) { $$$ } $_["aria-invalid-first"] = $_; $_["aria-invalid-last"] = $_; ' .Length of output: 158
Script:
#!/bin/bash # Description: Search for conditionals and assignments involving 'aria-invalid', 'aria-invalid-first', and 'aria-invalid-last' in PHP files. # Find conditionals involving the specified 'aria-invalid' keys. rg --no-filename --no-heading 'if\s*\(\s*\$[^)]+\["aria-invalid(?:-first|-last)?"]' --type php # Find assignments involving the specified 'aria-invalid' keys. rg --no-filename --no-heading '\$[^=]+\["aria-invalid(?:-first|-last)?"]\s*=' --type phpLength of output: 187
Crabcyborg
requested review from
lauramekaj1
and removed request for
garretlaxton
|
Hi @AbdiTolesa, I tested this by doing multiple scenarios using different input fields.
|
WalkthroughThe changes in this pull request focus on enhancing the handling of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
classes/models/fields/FrmFieldCombo.php (1)
384-386: LGTM! Consider adding a comment for clarity.The addition of the
subfield_nameproperty to the$fieldarray is a good way to provide more granular information about subfields. This change aligns well with the PR objective of handling specific parts of input fields, such as settingaria-invalidattributes.Consider adding a brief comment explaining the purpose of this new property, for example:
// Add subfield name to support individual subfield processing (e.g., for aria-invalid attributes) if ( ! empty( $sub_field['name'] ) ) { $field['subfield_name'] = $sub_field['name']; }css/_single_theme.css.php (1)
Line range hint
1-24: Consider future refactoring for improved maintainabilityWhile not directly related to the current changes, the overall structure of this file mixes PHP and CSS, which can make it challenging to maintain and debug. In the future, consider exploring CSS-in-JS solutions or CSS preprocessors that could provide similar dynamic capabilities while keeping the CSS more isolated and easier to manage.
js/formidable.js (1)
Line range hint
1235-1277: Overall impact and considerationsThe addition of the
maybeSetFocusOnNameFieldElementfunction and its integration into the existingcheckForErrorsAndMaybeSetFocusfunction improves the form's error handling for name fields. This change aligns well with the PR objectives and enhances the user experience.Consider the following points:
- Test thoroughly to ensure this change doesn't introduce any unexpected behavior in other form field types.
- Update documentation to reflect this new behavior for name fields.
- Consider adding similar functionality for other composite field types in the future, if applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- classes/controllers/FrmFieldsController.php (2 hunks)
- classes/models/FrmFieldFormHtml.php (1 hunks)
- classes/models/fields/FrmFieldCombo.php (1 hunks)
- css/_single_theme.css.php (1 hunks)
- js/formidable.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
css/_single_theme.css.php (1)
Line range hint
182-193: Excellent improvements to focus states!These additions enhance the consistency of focus states across various form elements, including textareas and select fields. This change aligns well with accessibility best practices and addresses the previously missing
textarea:focusstyling.classes/controllers/FrmFieldsController.php (3)
918-927: Improved accessibility handling for name fields.The changes enhance the handling of 'aria-invalid' attributes for name fields, ensuring that each subfield (e.g., first name, last name) can have its own accessibility state. This improvement contributes to better screen reader support and overall accessibility of the form.
941-943: Improved code readability.The addition of the end-of-loop comment and the empty line before the method's closing brace enhances the code's readability. These small changes make it easier to identify the loop's scope and the method's structure.
Line range hint
918-943: Summary of changes: Improved accessibility and code readabilityThe modifications in this file focus on two main areas:
- Enhanced accessibility handling for name fields, allowing for more granular control of the 'aria-invalid' attribute.
- Improved code readability with the addition of an end-of-loop comment and proper spacing.
These changes align well with the PR objectives and contribute to better form accessibility and maintainability of the codebase.
js/formidable.js (3)
1235-1254: New function added to handle focus on name field partsA new function
maybeSetFocusOnNameFieldElementhas been added to set focus on a name field part (first or last name) if it has an error. This enhances the user experience by directing focus to the specific part of a name field that needs attention.The function correctly checks for the following conditions:
- The element is a fieldset.
- It contains a
.frm_combo_inputs_containerwith adata-name-layoutattribute.- There's an input with
aria-invalid="true"within the fieldset.This implementation aligns well with the PR objective of applying
aria-invalidonly to specific parts of a name input field that contain errors.
1276-1277: Integration of new function into existing codeThe
maybeSetFocusOnNameFieldElementfunction is now called within thecheckForErrorsAndMaybeSetFocusfunction. This integration ensures that the new functionality is applied when checking for errors and setting focus.This change is consistent with the PR objectives and improves the form's accessibility and user experience.
Line range hint
1-1277: Conclusion of review for js/formidable.jsThe changes made to this file are focused and well-implemented. They address the PR objective of improving the handling of
aria-invalidattributes for name fields. The new functionmaybeSetFocusOnNameFieldElementand its integration enhance the user experience and accessibility of the form.The implementation is clean, follows existing coding patterns, and doesn't introduce any obvious issues. Good job on this improvement!
Hi @lauramekaj1, thank you for catching the issue with Paragraph fields! I have deployed this to the QA so that we can be on the same page while testing. Here is the link: https://qa.formidableforms.com/abdi/wp-admin/admin-ajax.php?action=frm_forms_preview&form=c35sh Let me know if you can't access it for some reason. |
AbdiTolesa
requested review from
lauramekaj1
and removed request for
lauramekaj1
Hi @AbdiTolesa, I tested this from your QA site and it seems that it's working as expected. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1235-1250: Consider generalizing the combo field focus handling.The implementation correctly handles focus management for combo fields, but could be generalized to support other combo field types (like address fields) in the future.
Consider:
- Renaming the function to something more generic like
focusFirstInvalidComboSubField- Adding JSDoc param and return type annotations
- Adding examples in the documentation
/** * Sets focus on a the first subfield of a combo field that has an error. * + * @param {HTMLElement} element The fieldset element containing combo inputs + * @return {void} + * @example + * // Focus first invalid field in a name/address combo field + * focusFirstInvalidComboSubField(fieldsetElement); */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable.js(2 hunks)
🔇 Additional comments (1)
js/formidable.js (1)
1272-1273: LGTM! Clean integration of combo field focus handling.
The new function is integrated appropriately into the error handling flow, maintaining existing functionality while adding support for combo fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1322-1337: Consider making the function more generic and handle edge cases.The implementation looks good but could be improved in a few ways:
Consider making it more generic by:
- Using data attributes instead of relying on the specific class name
- Adding a parameter to specify the container class/attribute to check
Consider handling the case where no invalid field is found to avoid silent failures
Example implementation:
- function maybeFocusOnComboSubField( element ) { - if ( 'FIELDSET' !== element.nodeName ) { - return; - } - if ( ! element.querySelector( '.frm_combo_inputs_container' ) ) { - return; - } - element.querySelector( '[aria-invalid="true"]' )?.focus(); - } + function maybeFocusOnComboSubField( element, containerSelector = '.frm_combo_inputs_container' ) { + if ( 'FIELDSET' !== element.nodeName ) { + return false; + } + if ( ! element.querySelector( containerSelector ) ) { + return false; + } + const invalidField = element.querySelector( '[aria-invalid="true"]' ); + if ( invalidField ) { + invalidField.focus(); + return true; + } + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
classes/controllers/FrmFieldsController.php(2 hunks)classes/models/FrmFieldFormHtml.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)js/formidable.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/controllers/FrmFieldsController.php
- classes/models/FrmFieldFormHtml.php
- classes/models/fields/FrmFieldType.php
🔇 Additional comments (1)
js/formidable.js (1)
1359-1359: LGTM! The placement of maybeFocusOnComboSubField call is appropriate.
The modification correctly handles focusing on combo subfields while preserving existing functionality for other field types.
…github.com/Strategy11/formidable-forms into issue-5409-fix_name_field_validation_issue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1322-1339: Fix JSDoc issues and improve documentation.The JSDoc for
maybeFocusOnComboSubFieldhas a few issues:
- Use
@returninstead of@returns- Use lowercase
booleantype instead ofBoolean- Add description of the return value
Apply this diff to fix the documentation:
/** * Sets focus on a the first subfield of a combo field that has an error. * * @since x.x * * @param {HTMLElement} element - * @returns {Boolean} + * @return {boolean} True if focus was set on a combo subfield, false otherwise. */🧰 Tools
🪛 eslint
[error] 1328-1328: Invalid JSDoc tag (preference). Replace "returns" JSDoc tag with "return".
(jsdoc/check-tag-names)
[error] 1328-1328: Invalid JSDoc @returns type "Boolean"; prefer: "boolean".
(jsdoc/check-types)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable.js(2 hunks)
🧰 Additional context used
🪛 eslint
js/formidable.js
[error] 1328-1328: Invalid JSDoc tag (preference). Replace "returns" JSDoc tag with "return".
(jsdoc/check-tag-names)
[error] 1328-1328: Invalid JSDoc @returns type "Boolean"; prefer: "boolean".
(jsdoc/check-types)
🔇 Additional comments (3)
js/formidable.js (3)
1330-1339: LGTM! Clean and focused implementation.
The function follows good practices:
- Early returns for invalid cases
- Clear conditions for combo field detection
- Uses optional chaining for safe focus setting
1361-1363: LGTM! Good integration with existing code.
The changes to checkForErrorsAndMaybeSetFocus properly integrate the new functionality:
- Breaks the loop if focus is set on a combo subfield
- Maintains existing behavior for other field types
1330-1339: Great improvement in handling all combo fields.
The implementation now works for any combo field type (not just name fields) by:
- Using generic selectors (
.frm_combo_inputs_container) - Relying on the
aria-invalidattribute for validation state - Following a clean, reusable approach
This addresses the previous review feedback about making the code more generic.
Also applies to: 1361-1363
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
js/formidable.js (2)
1322-1329: Update @SInCE version tagThe @SInCE tag should specify the actual version number instead of "x.x".
Line range hint
1322-1364: Consider adding automated tests for combo field focus handlingThe implementation is clean and generic, which is great for maintainability. To ensure long-term reliability, consider adding:
- Unit tests for
maybeFocusOnComboSubFieldwith different field types- Integration tests for the focus handling flow
- Documentation examples for other combo field types that can benefit from this implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable.js(2 hunks)
🔇 Additional comments (2)
js/formidable.js (2)
1330-1339: LGTM! Clean implementation of combo field focus handling
The function effectively handles focus management for combo fields with a clean implementation:
- Early returns for non-matching cases
- Safe DOM traversal
- Clear boolean return value
1361-1363: LGTM! Clean integration of combo field focus handling
The integration is well-placed in the error handling flow and properly breaks the loop when focus is set.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
js/formidable.js (2)
1322-1343: Update JSDoc documentationThe function implementation looks good and follows the generic approach requested in past reviews. However, the documentation could be improved:
- Replace "x.x" with the actual version number
- Add
@paramdescription for theelementparameter- Add
@returndescription explaining the boolean return value/** - * @since x.x + * @since 6.x.x * - * @param {HTMLElement} element + * @param {HTMLElement} element The DOM element to check for combo field focus + * @return {boolean} Returns true if focus was set on a combo subfield, false otherwise */
Line range hint
1322-1368: Well-structured implementation with good architectural choicesThe implementation shows good architectural decisions:
- Generic approach that works for all combo fields rather than being name-field specific
- Clear separation of concerns between field detection and focus management
- Proper integration with existing error handling flow
- Maintainable and extensible for future combo field types
This aligns well with the PR objectives while following good software design principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable.js(2 hunks)
🔇 Additional comments (1)
js/formidable.js (1)
1365-1367: LGTM! Clean integration of combo field focus handling
The integration follows good practices:
- Generic implementation that works for any combo field
- Proper loop control with break statement
- Maintains existing error handling flow
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
js/formidable.js (2)
1337-1338: Consider caching the querySelector result.Since we use the result of
querySelectorfor both the condition and focus action, we could optimize by storing it in a variable.-const comboSubfield = element.querySelector( '[aria-invalid="true"]' ); -if ( comboSubfield ) { +const invalidField = element.querySelector( '[aria-invalid="true"]' ); +if ( invalidField ) { - comboSubfield.focus(); + invalidField.focus(); return true; }
1331-1336: Consider combining the validation checks.The two condition checks could be combined for better readability and slightly improved performance.
-if ( 'FIELDSET' !== element.nodeName ) { - return false; -} -if ( ! element.querySelector( '.frm_combo_inputs_container' ) ) { - return false; -} +if ( 'FIELDSET' !== element.nodeName || ! element.querySelector( '.frm_combo_inputs_container' ) ) { + return false; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/models/fields/FrmFieldType.php(1 hunks)css/_single_theme.css.php(1 hunks)js/formidable.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/models/fields/FrmFieldType.php
- css/_single_theme.css.php
🔇 Additional comments (3)
js/formidable.js (3)
1322-1343: Well-structured implementation for handling combo field focus!
The function effectively handles focusing on the first invalid subfield within a combo field container. The implementation is clean, well-documented, and follows good practices.
1365-1367: Clean integration with existing error handling logic.
The integration of maybeFocusOnComboSubField is well placed and maintains the existing flow while adding the new functionality.
1337-1340: Verify screen reader behavior with aria-invalid.
The implementation correctly uses aria-invalid for identifying invalid fields. However, it would be good to verify that screen readers properly announce the invalid state when focus is set.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get a complete picture.
The additional scripts will help verify:
- If error messages are properly associated with fields using aria-describedby
- If error messages are announced immediately using role="alert"
- If there are other accessibility attributes used for error handling
This will give us a complete picture of the accessibility implementation for screen readers.
Based on the results, I can now provide a final response as the verification is complete. The implementation shows comprehensive accessibility support:
Screen reader behavior with aria-invalid is properly implemented
The code follows accessibility best practices for form validation:
- Uses
aria-invalid="true"to mark invalid fields - Properly associates error messages using
aria-describedby - Includes
role="alert"on error messages for immediate announcement - Maintains proper focus management by focusing the first invalid field
- Correctly handles error message cleanup by removing aria attributes
The implementation ensures screen readers will:
- Announce the invalid state when focus moves to the field
- Read the associated error message through aria-describedby
- Get immediate notification of errors through role="alert"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if we're consistently using aria-invalid across the codebase
rg -A 2 "aria-invalid.*true"
Length of output: 4240
Script:
#!/bin/bash
# Check for aria-describedby usage with error messages
rg -A 3 "aria-describedby.*frm_error"
# Check for role="alert" usage with error messages
rg -A 3 'role="alert"'
# Check for any other accessibility-related attributes in error handling
rg -A 3 'aria-(required|live|label|errormessage)'
Length of output: 70776
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I think this is all good to merge now.
Thanks @AbdiTolesa @truongwp and @lauramekaj1 for working on this!
🚀
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
js/formidable.js (2)
1322-1343: Add JSDoc param and return type documentation.The function implementation looks good but could benefit from more detailed documentation.
/** * Sets focus on a the first subfield of a combo field that has an error. * * @since x.x * + * @param {HTMLElement} element The field container element to check. + * @return {boolean} True if focus was set on a combo subfield, false otherwise. */
Line range hint
1322-1368: Great job on making the implementation generic!The code successfully addresses the previous review comments by:
- Making the implementation work for all combo fields, not just name fields
- Using proper attribute checks instead of field-specific logic
- Maintaining clean separation of concerns between validation and focus management
This makes the code more maintainable and reusable for future combo field types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable.js(2 hunks)
🔇 Additional comments (1)
js/formidable.js (1)
1365-1367: LGTM! Integration looks good.
The integration of maybeFocusOnComboSubField is well placed to handle combo field errors first, with proper loop control based on the return value.
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/5409
Test steps
aria-invalidattribute is only set on fields with error.Screen record