Add password visibility toggle feature #2429

c-ciobanu · 2018-05-08T17:17:00Z

My solution for the issue #2360.
What do you think?
Something to improve?

xPaw · 2018-05-08T17:18:25Z

There's also a password field on login and connect (edit) windows.

c-ciobanu · 2018-05-08T17:21:27Z

I was thinking of creating a tpl file with the password block so you can put it where you want.
Need to take a look at handlebars.

xPaw · 2018-05-08T17:22:10Z

I don't think a separate template is necessary, as every input is going to be different.

c-ciobanu · 2018-05-08T17:23:58Z

Can i add more committs to this pull request or i have to create a new one?

xPaw · 2018-05-08T17:24:17Z

This PR is just a branch, you can keep pushing to it.

c-ciobanu · 2018-05-08T17:26:33Z

Ok, later will add the 2 other fields.

MaxLeiter · 2018-05-08T19:47:13Z

client/css/style.css

+}
+
+#settings #change-password .see-pw::before {
+	content: "\f06e";


If you could add /** **/ comments here with links to the font-awesome website for consistency with other CSS, that'd be great

xPaw · 2018-05-09T11:15:57Z

client/css/style.css

+	width: 100%;
+}
+
+.password-container .see-pw {


Add cursor: pointer and :hover styles to change color to something more visible.

xPaw · 2018-05-09T11:16:38Z

client/css/style.css

+	content: "\f06e"; /* https://fontawesome.com/icons/eye?style=solid */
+}
+
+.password-container .see-pw.visible::before {


Maybe also change color for visible icon to red or something.

xPaw · 2018-05-09T11:17:43Z

This is conflicting with Edge's own reveal button, which seems to be an easy fix: vuetifyjs/vuetify#537

I think you also need increase padding-right on the input itself, so that the button doesn't overlay the text.

xPaw · 2018-05-09T11:26:06Z

client/views/windows/settings.tpl

 					<label for="verify_password_input" class="sr-only">Repeat new password</label>
 					<input type="password" id="verify_password_input" name="verify_password" class="input" placeholder="Repeat new password">
+					<i class="see-pw" title="Show Password"></i>


Use aria-label (you can also add tooltipped class to show a proper tooltip).

Put `Hide password in an alt-attribute something like that:

thelounge/client/js/socket-events/more.js

Line 118 in 49c5e6a

function swapText(btn) {

Since see-pw element is a little verbose, you could make a separate template file for it.

Change the classname to something more verbose like reveal-password.

c-ciobanu · 2018-05-09T23:30:41Z

Fixed all the things following your suggestions.
Coudn't create a new template, can't understand how the templates interact with each other.
If you can explain it to me would be great.

client/css/style.css

+}
+
+.password-container .reveal-password i::before {
+	content: "\f06e"; /* https://fontawesome.com/icons/eye?style=solid */


astorije · 2018-05-10T04:59:55Z

client/css/style.css

+	position: absolute;
+	top: 2px;
+	right: 15px;
+	cursor: pointer;


I would love if we could make the .reveal-password element a button instead of a span. It comes with benefits such as no need to set the cursor property, but also improves accessibility with keyboard navigation and such. Thoughts?

I'm not sure it should be keyboard accessible (tabbing into it would be counter intuitive?).

Edge doesn't allow tabbing into their reveal button.

astorije · 2018-05-10T05:00:58Z

client/views/windows/connect.tpl

 			<input class="input" id="connect:password" type="password" name="password" value="{{defaults.password}}">
+			<span class="reveal-password tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show Password" data-alt-label="Hide Password">


This is rather nitpicky, but in order to be consistent with similar elements like this, could you go for Show password and Hide password (i.e. drop the uppercase on Password)? Thanks!

astorije · 2018-05-10T05:03:05Z

client/views/windows/sign_in.tpl

 		<input class="input" type="password" name="password">
-	</label>
+		<span class="reveal-password tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show Password" data-alt-label="Hide Password">
+			<i></i>


I don't think we need (should?) have an i elements for those. If you look at other icon buttons, you'll see we don't wrap those in i elements. That will probably mean you'll need a container for the tooltip. That's rather unfortunate, but that's consistent with the rest so that when we improve/change things, we can do it all over the place more easily.

astorije · 2018-05-10T05:07:46Z

That is great stuff @c-ciobanu, thanks a lot for helping with that!! ❤️

I have noticed (on the default theme) that the icons are not correctly vertically centered:

Connect window:
Login page:

It's more visible on the second one.

xPaw · 2018-05-10T10:04:12Z

client/js/utils.js

@@ -97,6 +98,24 @@ function toggleNotificationMarkers(newState) {
 	viewport.toggleClass("notified", newState);
 }

+function togglePasswordField(elem) {
+	$(elem).on("click", function() {


This needs to pass in a more specific selector or direct jquery element, otherwise you will be binding multiple events to the same buttons if you open connect window 5 times (it'll bind 5 times on the button in settings).

@xPaw, is this still relevant?

xPaw · 2018-05-10T18:48:19Z

@c-ciobanu for a template, you just create a file in views folder (like reveal-password.tpl) and then include it like {{> reveal-password}} in html where you need it.

c-ciobanu · 2018-05-10T18:55:37Z

@astorije the icons are not aligned because the css resets are not working properly, on chrome they look fine but on firex not. A separate file just for resets should be created for clarity, i sow some rules repeated on the css files.
For the span and i i would do something like this:

<span class="tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show password">
<button type="button" class="reveal-password" aria-label="Show password" data-alt-label="Hide password"></button>
</span>

This can go?
@xPaw can i associate a js file with the logic to the template like in react or vue?

xPaw · 2018-05-10T18:56:24Z

@xPaw can i associate a js file with the logic to the template like in react or vue?

We don't have anything like that setup, sadly.

xPaw · 2018-05-11T09:53:19Z

client/js/utils.js

+		const $this = $(this);
+		const input = $this.closest("div").find("input");
+
+		input.attr("type") === "password" ? input.attr("type", "text") : input.attr("type", "password");


input.attr("type", input.attr("type") === "password" ? "text" : "password"); is shorter and reads better.

@c-ciobanu, mind doing this?

Sure, will commit this along with the changes on the  when @xPaw will express his opinion on what to use.

astorije · 2018-05-22T04:11:53Z

For the span and i i would do something like this: [...]
This can go?

Yep, I think it's nicer than , if @xPaw is okay with that (that enables keyboard tabbing so might prefer span, I don't know, up to you guys).

xPaw · 2018-05-23T13:58:12Z

Use span for consistency with other icons we already have.

xPaw · 2018-05-24T06:55:55Z

client/views/reveal-password.tpl

@@ -0,0 +1,3 @@
+<span class="reveal-password tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show password" data-alt-label="Hide password">
+    <button type="button" aria-label="Show password" data-alt-label="Hide password"></button>


You changed it to button instead of span as discussed.

i thought you wanted them to be consistent with the other icons, a <button> inside a . To be sure, you want a  inside a ?

Yes. In this instance we don't want to use button.

astorije

The icons are still not peeerfectly aligned (I think it's more visible on login page, though I have not measured), but this is already pretty good!
It would be lovely if you could follow-up in a further PR to accurately vertically center the icon. Either way, thanks a lot, this is solid work!! 👏

And congrats on your first PR here! Since you are now a contributor, feel free to request a sticker pack at goo.gl/forms/f5usqAEp5DWoeXC92 😊

c-ciobanu · 2018-05-29T17:16:03Z

Thank you, I'll take a look into it and will submit a pr soon.
Maybe I'll wait for the PRs with css changes to be merged before doing it.
@astorije, @xPaw you thought about using a preprocessor like sass and organize better the css part?

Add password visibility toggle feature

97b178d

xPaw added this to Todo in Accessibility via automation May 8, 2018

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 8, 2018

MaxLeiter reviewed May 8, 2018

View reviewed changes

add password toggle to connect and login

2d49e34

xPaw reviewed May 9, 2018

View reviewed changes

refactor password visibility toggle feature

bdfc367

astorije reviewed May 10, 2018

View reviewed changes

client/css/style.css Outdated

}

.password-container .reveal-password i::before {

content: "\f06e"; /* https://fontawesome.com/icons/eye?style=solid */

This comment was marked as resolved.

Sign in to view

astorije reviewed May 10, 2018

View reviewed changes

xPaw reviewed May 10, 2018

View reviewed changes

minor fixes

0294643

moved the reveal password block into his own template

bab78f9

xPaw reviewed May 11, 2018

View reviewed changes

xPaw approved these changes May 11, 2018

View reviewed changes

changed i with button

25dc284

xPaw reviewed May 24, 2018

View reviewed changes

change icon tag

490c94b

xPaw requested a review from astorije May 26, 2018 20:59

astorije approved these changes May 28, 2018

View reviewed changes

astorije added this to the 3.0.0 milestone May 28, 2018

astorije merged commit e30984a into thelounge:master May 28, 2018

Accessibility automation moved this from Todo to Done May 28, 2018

c-ciobanu deleted the c-ciobanu/feature-2360 branch May 29, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add password visibility toggle feature #2429

Add password visibility toggle feature #2429

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

MaxLeiter May 8, 2018

c-ciobanu May 8, 2018

xPaw May 9, 2018

xPaw May 9, 2018

xPaw commented May 9, 2018 •

edited

xPaw May 9, 2018 •

edited

c-ciobanu commented May 9, 2018

This comment was marked as resolved.

astorije May 10, 2018

xPaw May 10, 2018

astorije May 10, 2018

astorije May 10, 2018

astorije commented May 10, 2018

xPaw May 10, 2018

astorije May 22, 2018

xPaw commented May 10, 2018

c-ciobanu commented May 10, 2018

xPaw commented May 10, 2018

xPaw May 11, 2018

astorije May 22, 2018

c-ciobanu May 22, 2018

astorije commented May 22, 2018

xPaw commented May 23, 2018

xPaw May 24, 2018

c-ciobanu May 24, 2018

xPaw May 24, 2018

astorije left a comment

c-ciobanu commented May 29, 2018 •

edited

		<input class="input" id="connect:password" type="password" name="password" value="{{defaults.password}}">
		<span class="reveal-password tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show Password" data-alt-label="Hide Password">

		@@ -0,0 +1,3 @@
		<span class="reveal-password tooltipped tooltipped-n tooltipped-no-delay" aria-label="Show password" data-alt-label="Hide password">
		<button type="button" aria-label="Show password" data-alt-label="Hide password"></button>

Add password visibility toggle feature #2429

Add password visibility toggle feature #2429

Conversation

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

xPaw commented May 8, 2018

c-ciobanu commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented May 9, 2018 • edited

xPaw May 9, 2018 • edited

Choose a reason for hiding this comment

c-ciobanu commented May 9, 2018

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented May 10, 2018

c-ciobanu commented May 10, 2018

xPaw commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented May 22, 2018

xPaw commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment

c-ciobanu commented May 29, 2018 • edited

xPaw commented May 9, 2018 •

edited

xPaw May 9, 2018 •

edited

c-ciobanu commented May 29, 2018 •

edited