bartificer / xkpasswd-js

The official JavaScript port of the Crypt::HSXKPasswd Perl module.
https://bartificer.github.io/xkpasswd-js/
BSD 2-Clause "Simplified" License
51 stars 11 forks source link

Requested Feature: generated-password textbox replaced with individual copy buttons, causing inefficient user access #58

Closed macLurker closed 6 months ago

macLurker commented 6 months ago

Can we put the textbox back? I usually take all generated passwords at one time. It's a pain to have to copy & paste each one.

Or, failing that, can we at least make the tabs work for all the fields, so we can tab to each password? It's nice to be able to navigate the whole web-page from the keyboard. Also this helps to automate the process of getting passwords.

podfeet commented 6 months ago

Interesting, I never take all of the passwords. I always select and copy just one. What's the use case for copying multiple at once? just curious.

hepabolu commented 6 months ago

@macLurker you can still select all passwords at once and copy them.

hepabolu commented 6 months ago

I suggest that you make a separate issue for navigating the app with the keyboard, because that is in fact a different request. I've thought about it myself, but wanted to focus first on getting the original functionality ported to this version.

I like the idea of navigating the app using a keyboard, but for automating the process of getting passwords, I would prefer to move the actual library to a commandline version. That is much more automation friendly.

macLurker commented 6 months ago

@podfeet: Mostly because I tend to change passwords on multiple accounts at one time. You know, like update all the 5-year-old passwords at once. I usually have to tweak them because every log in has different requirements as to special characters allowed or length. So it’s just easier to grab 3, paste them in a text document, make them fit the login site du jour, etc.

I had a Keyboard Maestro macro to do all that, until the XKPasswd website changed.

@hepabolu : I guess I can do that.

pricemi115 commented 6 months ago

@macLurker - That is an interesting use case, and one I sort of use now but not via the website. I took Bart's initial stab from eons ago at porting the engine of XKPasswd to javascript and built my own KBM macros around that....though I just make them one at a time from my trigger. One thing I always struggled with when XKPasswd passwords were in a text area was selecting the password of interest on mobile. It was always a struggle for me to get the start and end of the passwords.

@hepabolu - Even though Dorothy would be able to use the API/CLI when this becomes available to meet her needs, surely others with a similar use case will not. I wonder if adding a control to allow users to select how the passwords are presented (list with copy buttons (default) or text area) would not provide a better experience for all?? Ideally, this setting could be persisted per user so the "Dorothy"s of the world would not need to constantly change it.

Sorry y'all for starting the XKPasswd version of a "Tabs vs Spaces" Holy War 😅

hepabolu commented 6 months ago

@pricemi115 I like your idea.

podfeet commented 6 months ago

I guess I'd have to see how it was implemented but a choice on display could make it really messy.

@macLurker I just tried selecting all the passwords to copy at once just like we used to and it worked just fine. The copy buttons don't make it any harder. We really do have the best of both worlds right now - easy to copy just one with a button (especially on mobile) and easy to copy them all.

macLurker commented 6 months ago

@podfeet Yes, so can I, but can you select/copy all/any without using the mouse?

I never use XKPasswd on my phone. It's just too much of pain to do that kind of thing on a tiny screen. So I will admit having it work nicely on a phone is not a priority for me. Maybe that's where you split the functionality, but, yes, that could get messy.

podfeet commented 6 months ago

no, but you couldn't before in the OG version.

macLurker commented 6 months ago

OG version being the one Bart originally did? Yes, I could. Actually he made it easy by selecting the text after placing the newly generated passwords in the text box. So all I had to do was CMD-C. But I could also tab to the text box, do Select-All, then CMD-C. All from the keyboard.

podfeet commented 6 months ago

I think I have the solution, and it solves something I have wanted. My desire was to be able to edit the passwords right in the same field so I don't have to paste these private secrets elsewhere to edit them when a site barfs on a config of characters.

If we make the text box editable, @macLurker can tab to the field after typing in her desired number of passwords and then hit CMD-A, CMD-C to select them all. I tested the idea locally and it worked. If line 317 in index.html is changed to:

<div class="card-body" contenteditable="true">

I think I talked to @hepabolu about making it editable and she was worried about the stats being inaccurate if someone edited. We might be able to put in an event listener that if someone edits, the stats get greyed out?

@macLurker maybe you could add that editable thing I wrote and then take a stab at the idea of greying out the stats with an event handler.

hepabolu commented 6 months ago

@podfeet that solves your problem and @macLurker’s problem, but reverses the problem that @pricemi115 solved, aka being able to easily select just one password. Can you figure out a solution that solves all three problems?

pricemi115 commented 6 months ago

That may not be the best place for that @podfeet. You can also delete everything, including the copy buttons. But the idea has merit. Line#319, the <ul></ul> line, might be a bit better, but you can still do some damage. I will see if the <li> node can make it a bit more targeted.

Also, the event handler for the copy click needs to be updated. otherwise that copy will contain the ORIGINAL password, not the modified one.

podfeet commented 6 months ago

I see what you mean.

pricemi115 commented 6 months ago

All - This is what I was thinking of for the "password presentation" idea. https://github.com/pricemi115/xkpasswd-js/tree/exp_PwdList_vs_Text

I don't have it tracking when you change between List and Text. Just create more passwords to get the idea. As an added benefit for Dorothy, when redering in textarea mode, I select all the password text for you, so you only need to press Ctrl-C.

Ideally, I see the selection mode as a user config setting, but that could be handled as a separate feature.

Sorry @podfeet - data remain read-only

image image
podfeet commented 6 months ago

Interesting idea. I'm not sure I'd guess what list vs. text means without toggling it to see, but I get how it works. Might need to think of more descriptive names.

Why can't the text version be editable?

pricemi115 commented 6 months ago

It could be (editable text area), just didn't do that in the example. As far as the names - I would be open to ideas. Actually to be up front, I should probably tell you I have been explicitly forbidden by the tech pubs folks at work from ever naming anything that users see - guess they don't like some of my very detailed and technically accurate ideas ;)

macLurker commented 6 months ago

@pricemi115 That looks really good. I like it. The important thing for me (and, I think, you) is that the various HTML elements are labelled & navigable from the keyboard.

Possible labels for the two options: List = "Copy one password to use", Text = "Copy all passwords to use"

pricemi115 commented 6 months ago

"Use jQuery", "navigable from the keyboard" , "accessibility friendly", ..... Boy, you folks are gonna make a web developer out of me yet !!🤣

hepabolu commented 6 months ago

I thought that was our hidden agenda 😜

hepabolu commented 6 months ago

@pricemi115 That looks really good. I like it. The important thing for me (and, I think, you) is that the various HTML elements are labelled & navigable from the keyboard.

Possible labels for the two options: List = "Copy one password to use", Text = "Copy all passwords to use"

I would revert to icons: list icon for the list and page icon for the text box

podfeet commented 6 months ago

There are two weird artifacts that have been created in VoiceOver with the recent changes. I THINK it happened with the new copy buttons, but I'm not sure. I made you a little video to demonstrate the problem

https://share.cleanshot.com/Smh9VHbQ

pricemi115 commented 6 months ago

I don't think the initial copy button changes introduced those artifacts, but it doesn't really matter (I am accustomed to writing bugs for a living😅) since they exist now. I submitted a new issue for those artifacts. As far as this one, I wanted to close the loop with @hepabolu to see if we should address this now, presumably with the proposed list v. text area selection. I know this is pushing into the scope-creep realm for this phase, but we did break Dorothy's workflow.

I am happy to take on the initial work for the dual password output support and we can figure out a better home for the presentation format in a future phase.

macLurker commented 6 months ago

Thanks, @pricemi115 That would be cool. Meanwhile I think I’ll investigate creating a command line version. I agree with @hepabolu that that would be better for automation. I mean, how hard can it be? Right?

pricemi115 commented 6 months ago

@macLurker - as the R&D Director at work likes to say (when he wants to needle the sw devs) - "It's just typing" 🤣

pricemi115 commented 6 months ago

Nice suggestion to use icons, @hepabolu 💯

image
podfeet commented 6 months ago

ooh - I LOVE the icons. Mostly because I don't have to interpret what the words mean. I don't know what the icons mean exactly but they're inviting to click and then it will become obvious.

Where did you find this, @pricemi115? I don't see it in the latest release.

pricemi115 commented 6 months ago

Well @podfeet, please know that the screen reader text will be very clear as to what the radio buttons do 😉

It is not in the latest since I am still working on the changes.

The icons aren't that special, just bootstrap icons.

podfeet commented 6 months ago

Ah - Now I understand, she suggested icons and you're implementing them. Awesome.

pricemi115 commented 6 months ago

@macLurker - Dorothy Mode is now live.

macLurker commented 6 months ago

@pricemi115 - Dorothy Mode tested successfully. All good. Many thanks for getting this done. I am going to close the issue now.