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

Pressing Enter when "Time Based One-Time Password" is set up causes it to reset #341

Closed
dartiss opened this issue Mar 26, 2020 · 9 comments · Fixed by #348
Closed

Pressing Enter when "Time Based One-Time Password" is set up causes it to reset #341

dartiss opened this issue Mar 26, 2020 · 9 comments · Fixed by #348
Milestone

Comments

@dartiss
Copy link

dartiss commented Mar 26, 2020

It can be best demonstrated with this GIF...

blog-2fa-issue-local

Basically, set up your 2FA preference as "Time Based One-Time Password". Then go to any other profile field, update it and then press Enter (i.e. don't use the "Update Profile" button). You'll find that the 2FA setting have no deactivated.

I believe this is because pressing Enter is causing the "Reset Key" button to trigger instead of "Update Profile", due to it being the first form input.

@kasparsd
Copy link
Collaborator

Thanks for opening this @dartiss! I wonder if #91 is related to this as well.

@kasparsd kasparsd added the Bug label Mar 26, 2020
@georgestephanis
Copy link
Collaborator

georgestephanis commented Mar 26, 2020

For a bit of added context, the additional code causing that modal is coming from https://github.com/Automattic/vip-go-mu-plugins/blob/master/two-factor.php#L223

(it shouldn't /cause/ the issue, just some extra context for reproducability)

@kasparsd
Copy link
Collaborator

I think @dartiss is spot-on -- the issue is caused by the "Reset Key" button being the first "submit" type button in the form:

<input type="submit" class="button" name="two-factor-totp-delete" value="<?php esc_attr_e( 'Reset Key', 'two-factor' ); ?>" />

This was introduced in #252.

@georgestephanis
Copy link
Collaborator

Would we have the same usability without the issue if it was a <button> instead of <input> or would that cause a11y issues?

@kasparsd
Copy link
Collaborator

@georgestephanis There shouldn't be an issue with switching to <input type="button" /> or <button /> as long as we make it submit the form on click/press which is not the default behaviour for non-submit buttons:

To make buttons do anything, you have to write JavaScript code to do the work.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/button

@georgestephanis
Copy link
Collaborator

It's always hard to do one form inside another. laugh

@rahulsprajapati
Copy link

We could use a checkbox or something and rely on to remove/reset config rather then submit button value.

I've found a workaround to add fix: https://gist.github.com/rahulsprajapati/c08182f1fc88596bb3c2206e0eea4a60

If this make sense we could add it in plugin itself.
cc @dartiss

@dartiss
Copy link
Author

dartiss commented Apr 8, 2020

Thanks @rahulsprajapati. How is this solution in terms of accessibility?

@rahulsprajapati
Copy link

How is this solution in terms of accessibility?

Yeah it work just fine, check this gif.

2fa-fix

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 a pull request may close this issue.

5 participants