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

Add reveal eye to password form control #2219

Merged
merged 95 commits into from
Sep 11, 2024

Conversation

rickyosser
Copy link
Contributor

This is an addition to the Password form-control which adds an eye to toggle password visibility.

The JS-script part maybe should be more ATK4 friendly but that is above my current knowledge, I will learn with help.

@rickyosser
Copy link
Contributor Author

Hi,
I understand the codingstyle errors and stuff but need help to decode the StaticAnalysis errors.
1, Overrride of the init() function, this might not be what I want to do, how would I go about to implement this as an addition/extend which will run automatically if the variable $this->eye is set to true?
2, I don't understand this at all, please enlighten me...

Thanks in advance.
Rickard

@@ -4,7 +4,44 @@

namespace Atk4\Ui\Form\Control;

use Atk4\Ui\Button;
use Atk4\Ui\Js\JsExpression;

class Password extends Line
Copy link
Member

@mvorisek mvorisek Aug 18, 2024

Choose a reason for hiding this comment

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

Based on whatwg/html#7293 it is true that native solution does not exist in all browsers currently.

IE - at least https://stackoverflow.com/questions/61449079/how-to-hide-the-eye-from-a-password-input-in-ms-edge-and-ie#61450596 is needed to supress the native behaviour of IE, or do we want to keep native eye where supported?

Firefox - https://connect.mozilla.org/t5/ideas/add-a-reveal-password-function-for-logins/idi-p/1140 - did you know there is native (but I agree not much intuitive) reveal - can we turn on/off the native one?

Chrome - it seems there is no native solution (except when using 3rd party extension)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I meant a more native ATK4 solution with more Js functions instead of a big javascript blob... :)

Copy link
Member

Choose a reason for hiding this comment

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

IE - I tested Windows 7 and 10, it seems Microsoft does everything to kill IE in favor of Edge - in both systems, when I opened IE, Edge was opened instead. Edge is based on Chrome. And in Edge and Chrome there is no native reveal icon added. So we do not need any additional code to coexist with the native reveal.

@mvorisek mvorisek changed the title Added 'eye-function' to toggle password visibility to form-passwords. Add reveal eye to password form control Aug 18, 2024
@mvorisek
Copy link
Member

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

@mvorisek
Copy link
Member

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

@rickyosser
Copy link
Contributor Author

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

Thanks, even though you've made it better... :)
Anyway, to answer your question, yes, according to the code, I haven't tried it, but you should be able to add another icon either with BeforeInput or AfterAfterInput and the eye will always be AfterInput. I'll try and see if it works tomorrow with the old code before your changes tonight.

@rickyosser
Copy link
Contributor Author

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

I checked 2 different applications, 1 an AccessPoint looses focus on the password field when the eye is pressed, Acronis on their on-line management platform doesn't. Anwyay, it is an easy fix, I'll commit it now.

@mvorisek
Copy link
Member

And one more question - https://jsfiddle.net/fv2bwyq5/ - In the demo the focus is kept when a div is clicked. I do not fully understand why, but the reveal/unreveal click currently removes focus from the password, but the focus should be kept as in the demo when a div is clicked.

I checked 2 different applications, 1 an AccessPoint looses focus on the password field when the eye is pressed, Acronis on their on-line management platform doesn't. Anwyay, it is an easy fix, I'll commit it now.

Based on https://stackoverflow.com/questions/8735764/prevent-firing-focus-event-when-clicking-on-div I meant to use native mousedown as click even can no longer cancel the focus, but I did some testing and your elem.focus() solution seems to have better UX and is more consistent how other elements remove focus. 👍

So only #2219 (comment) remains and we are ready to go!

@rickyosser
Copy link
Contributor Author

Well done. One more question - you initially introduced revealEyeIcon property. I would support it, the question is, can two icons be put inside the right side of input element by FUI? Imagine someone using the icon for marking required fields etc.

It was a neat idea, it doesn't work in practice! :)
Let's stay with the icon replacement. If the developer wants to add something else he can turn of revealEye or add a label/add action.

@mvorisek mvorisek merged commit 3934637 into atk4:develop Sep 11, 2024
22 of 24 checks passed
@mvorisek
Copy link
Member

I love this PR, thank you for this core contribution ❤

@rickyosser rickyosser deleted the form_password_eye branch September 11, 2024 14:13
@mvorisek mvorisek added the MAJOR label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants