Connect login form: Refocus password field on submission error#55450
Merged
Connect login form: Refocus password field on submission error#55450
Conversation
ravicious
commented
Jun 5, 2025
Comment on lines
+27
to
31
| /** | ||
| * @deprecated Include items from refocusDeps into the calculation of shouldFocus instead. | ||
| * The list of useEffect deps should be statically known. | ||
| */ | ||
| refocusDeps?: DependencyList; |
Member
Author
There was a problem hiding this comment.
I added this because arguably it's incompatible with hooks are "supposed to" be used.
In this specific scenario, it was easy to avoid using refocusDeps by just adding another comparison when constructing the value for shouldFocus. However, there are other places where we use this hook where this might be harder. For example:
Here the callsite would like to retrigger focus when mfaType.value changes, which necessitates tracking the previous value of it, something like in "Adjusting some state when a prop changes". Maybe there are some other ways to address this but nothing comes to my mind at the moment.
ryanclark
approved these changes
Jun 5, 2025
Contributor
|
@ravicious See the table below for backport results.
|
This was referenced Jun 5, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #54508.
To understand the fix, one first must understand how it worked before. I assumed that some normal browser behavior caused it to work, but that's not the case.
Prior to #47883, upon submission the form would immediately switch to the next "step", before a response from the login attempt even arrived. When the response arrived with an error, the form would go back to the previous step, thus re-firing all effects, including the one that would focus the password field.
teleport/web/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/FormLogin/FormLocal/FormLocal.tsx
Lines 44 to 46 in 33f2bbf
Details
https://github.com/user-attachments/assets/d834a228-65d3-4fbb-9cb8-fdf636e1b41eAfter the cleanup from #47883, the form doesn't go into the next step immediately on submission. Instead it disables the fields and shows a progress bar. When an error response arrives, it enables the fields, but the focus is no longer on the form itself. The focus effects do not fire because their deps haven't changed.
Details
https://github.com/user-attachments/assets/114d00f1-51b9-4a7a-babf-dfd3fc3993c4The fix is to include the state of the form submission in the value that decides whether a particular field should be focused or not.
Details
https://github.com/user-attachments/assets/df1c89fd-d793-4f34-81a9-4570f9f30dd3I'm still not sure if regular forms lose focus on submission as well, or if the need to trigger those focus effects comes from the same place that the need to use
useRefAutoFocusinstead ofautoFocus.