Focus on first invalid field for wizard#1836
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
sakshi-arora1
left a comment
sakshi-arora1
left a comment
There was a problem hiding this comment.
add a test case for same
rismehta
left a comment
rismehta
left a comment
There was a problem hiding this comment.
Overall the intent is clear and the fix makes sense. A couple of things to tighten up before merge.
| constructor(params) { | ||
| super(params); | ||
| const {element} = params; | ||
| const {element, formContainer} = params; |
There was a problem hiding this comment.
formContainer is stored here but never guarded before use. If any caller constructs the wizard view without passing formContainer in params (e.g. a subclass, test harness, or future refactor), this.formContainer will be undefined and the setFocus call in #navigateToNextTab will throw. Consider either asserting it is present here (if (!formContainer) console.warn(...)) or using optional chaining at the call site.
| this.#navigateAndFocusTab(nextVisibleIndex); | ||
| } | ||
| } else { | ||
| this.formContainer.setFocus(validationErrorList[0].fieldName); |
There was a problem hiding this comment.
No null guard on this.formContainer. If formContainer was not passed in params for any reason, this crashes with Cannot read properties of undefined (reading 'setFocus'). Safer as:
this.formContainer?.setFocus(validationErrorList[0].fieldName);| getTabs().should('have.length', 5); | ||
| getWizardPanels().should('have.length', 5); | ||
| getTabAtIndex(0).should('have.class', 'cmp-adaptiveform-wizard__tab--active'); | ||
| getWizardPanelAtIndex(0).should('have.class', 'cmp-adaptiveform-wizard__wizardpanel--active'); |
There was a problem hiding this comment.
The test verifies the tab stays active and error messages appear, but the main new behavior — that focus moves to the first invalid field — is not asserted. The setFocus call is the entire point of this change. Consider adding something like:
// verify focus landed on the first invalid field
cy.focused().closest('[data-cmp-is]').should('have.attr', 'data-cmp-is');or checking document.activeElement is within the first errored component. Without this, a regression in setFocus would not be caught by this test.
4 participants
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: