nforwardauth icon indicating copy to clipboard operation
nforwardauth copied to clipboard

Switch to forms

Open bt90 opened this issue 2 years ago • 6 comments

This PR switches the page to use a form to group the inputs. This allows the required field and enter key handling to work without Javascript.

bt90 avatar Aug 15 '23 14:08 bt90

Thanks for your countless contributions! You're a real pioneer of this project :)

I have some questions about this PR though, what are the benefits of switching to a form? I understand that not everyone wants to run Javascript, mainly for security reasons.

nosduco avatar Aug 15 '23 21:08 nosduco

The change is mostly semantic as it groups the input elements in a form to tell the browser that they belong together. The benefit is that things work the way the user expects them to, without having to manually handle things we used to do with custom js.

  • the required attribute of the username/password fields is enforced by the browser itself. The user can't submit empty fields and instead gets a nicely localized warn message for each field
  • since it's a single form, it can be submitted via Enter key and we don't need to check for the key event anymore

While not strictly part of the form rework, I also changed the js code to reenable the inputs in the event of an unknown error.

Feel free to test it. As it's a bit bigger change and I'm not very familiar with javascript :sweat_smile:

bt90 avatar Aug 16 '23 05:08 bt90

We could also go one step further and allow the user to submit the form without having javascript enabled. But that would require changes to /login endpoint in the backend as it would need to handle application/x-www-form-urlencoded form data.

bt90 avatar Aug 16 '23 05:08 bt90

The best way would probably to handle the POST of the form in / and get rid of /login:

<form action="/" method="post">

bt90 avatar Aug 16 '23 07:08 bt90

Understood! Yeah, I was thinking of potentially trying to remove all JS from it... will test out your branch and explore what may be possible. Thanks again for your countless contributions :)

nosduco avatar Aug 18 '23 14:08 nosduco

We can merge the current state and try to replace the JS based logic later on.

bt90 avatar Aug 18 '23 15:08 bt90