Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK-2588] Avoid multiple simultaneous HTTP calls #1998

Merged
merged 3 commits into from
Jun 2, 2021

Conversation

frederikprijck
Copy link
Member

There appears to be a bug in lock that can result in multiple, simultaneuour, HTTP calls to the /login endpoint(s).

To reproduce:
In lock, try the following In Chrome:
• Ensure username/email and password are filled in
• Focus the username/email field
• hit enter twice fast

Network tab should show multiple calls to login.

In safari, this issue exists on both fields, not only username/email.

The reason the behavior is different in Chrome and Safari seems to be because Chrome avoids submitting the form when pressing enter in a disabled input. Password is the only field that is getting disabled when the form is being submitted.

The fix here is to ensure both username and email fields are also disabled while the form is being submitted.

On safari however, it appears that safari does not respect the disabled value and still submits the form. Therefor I changed the fix that was done in #1968, by using the isSubmitting value instead of the disableSubmitButton

@frederikprijck frederikprijck requested a review from a team as a code owner May 28, 2021 13:47
@frederikprijck frederikprijck marked this pull request as draft May 28, 2021 14:19
@frederikprijck
Copy link
Member Author

Marked as draft as I believe it can benefit from a test or two.

@stevehobbsdev
Copy link
Contributor

I'm having trouble adding an acceptance test that will automatically test this scenario; trying to add something that will fire a key event for the "enter" key that submits a form appears to be impossible, as the browser just ignores it.

This issue on SO is a little old but demonstrates the same problem.

Happy to proceed with the fix given that manual testing can show that the bug has been mitigated.

@frederikprijck frederikprijck marked this pull request as ready for review June 1, 2021 14:17
@frederikprijck
Copy link
Member Author

Sure, let's proceed!

Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants