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

[VDG] Confirm Recovery Words By Selection II: "The Revenge of TagsBox" #9640

Closed

Conversation

ichthus1604
Copy link
Contributor

@ichthus1604 ichthus1604 commented Nov 25, 2022

image

  • Introduces new UX for Confirm Recovery Words:
    • Uses selects recovery words one by one from a list of 4 words from the entire dictionary
    • After all 12 words are correctly selected, dialog moves automatically to the next (enter password)
    • If at any point the user fails to select the correct word:
      • a message pops up
      • The user is then taken back to the "Recovery Words" list, giving them the chance to write them down correctly
      • The process starts over from Select Word #1

image

@ichthus1604 ichthus1604 requested review from a user, soosr and SuperJMN November 25, 2022 17:47
MaxHillebrand
MaxHillebrand previously approved these changes Nov 25, 2022
Copy link
Contributor

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tACK 9ea6214

Copy link
Contributor

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Uses selects recovery words one by one from a list of 4 words from the entire dictionary

Why select from the entire dictionary? Why not just select from the 12 words?

After all 12 words are correctly selected, dialog moves automatically to the next (enter password)

If the dialog moves automatically then we should remove the Continue button.

  • If at any point the user fails to select the correct word:
    • a message pops up
    • The user is then taken back to the "Recovery Words" list, giving them the chance to write them down correctly
    • The process starts over from Select Word #1

This is an overkill, there is no need to take the user back to the recovery words list, just stop the user from entering other words if they make a mistake. I wouldn't even use a pop up window.

The 4 words are displayed too close to each other. They should be separated a bit IMO.
There is no need for the tooltip when you hover over the 4 words.
I personally don't like the blue color when I hover over the 4 words.

@ichthus1604
Copy link
Contributor Author

ichthus1604 commented Nov 26, 2022

@yahiheb

Why select from the entire dictionary? Why not just select from the 12 words?

Because by the time you reach word #12 you can only show previously selected (hence repeated) options.

If the dialog moves automatically then we should remove the Continue button.

I tried this, but it looks really bad IMO, with the Skip hyperlink floating at the bottom

This is an overkill, there is no need to take the user back to the recovery words list, just stop the user from entering other words if they make a mistake. I wouldn't even use a pop up window.

Current design was approved by UI Team. I would rather not make further changes unless those changes are also discussed and approved.

Also: we need to prevent the user from "cheating" by clicking every possible option until it becomes green, which is why the entire UX purposefully starts over when you make a mistake. User is supposed to actually write the recovery words down (in paper or otherwise physically) and not simply brute-force the UI and ignore this step.

@yahiheb
Copy link
Contributor

yahiheb commented Nov 26, 2022

Because by the time you reach word #12 you can only show previously selected (hence repeated) options.

It is okay, words can be repeated.

I tried this, but it looks really bad IMO, with the Skip hyperlink floating at the bottom

What is the point of that grayed out button if it will never be used, it is misleading.

Current design was approved by UI Team. I would rather not make further changes unless those changes are also discussed and approved.

You mean that decisions were already made and we cannot change or criticize this?

So what options do reviewers and maintainers of this repository have? They can only ACK and approve PRs??

Also: we need to prevent the user from "cheating" by clicking every possible option until it becomes green, which is why the entire UX purposefully starts over when you make a mistake. User is supposed to actually write the recovery words down (in paper or otherwise physically) and not simply brute-force the UI and ignore this step.

Users can take a photo of the recovery words which is cheating, how prevent this?
If the users want to cheat they can do that and there is nothing we can do about it. We don't need to make the UX cheating-proof, we just have make it good enough for most users.

Starting over and over whenever I make a mistake is a very slow and bad UX.

@yahiheb
Copy link
Contributor

yahiheb commented Nov 26, 2022

3

IMO the 4 words should be move d a bit down to fill the empty space below them.

And the 12 words place holder should be moved a bit up to be aligned with the place holder of the 12 words in the previous dialog.

Copy link
Contributor

@SuperJMN SuperJMN left a comment

Choose a reason for hiding this comment

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

I've discovered a problem.

  1. Skip the word recovery.
  2. You're prompted to write a password.
  3. Enter empty password
  4. In the next screen, click the "go back" arrow.
  5. The column layout gets messed up.

image

@ichthus1604
Copy link
Contributor Author

@SuperJMN Thanks! Fixed in latest commit.

@MaxHillebrand
Copy link
Contributor

should it be picking out of 3 words instead? seems more intuitive.

@ichthus1604
Copy link
Contributor Author

+1 to @MaxHillebrand's idea of using 3 words for the options.

@soosr @nopara74 what do you think?

@soosr
Copy link
Contributor

soosr commented Nov 30, 2022

+1 to @MaxHillebrand's idea of using 3 words for the options.

@soosr @nopara74 what do you think?

Doesn't matter to me.

@soosr
Copy link
Contributor

soosr commented Nov 30, 2022

  • Separators should be adjusted to match the previous screen.
  • I agree with this comment, options are too to the top.

image

#9640 (review)

This is an overkill, there is no need to take the user back to the recovery words list, just stop the user from entering other words if they make a mistake.

At first, I thought you are right, but then I tried out the feature and made a mistake on purpose just to see how it feels. And it gave me the feeling that it is actually very important to write down these words correctly. ACK for the current implementation from me.

@ichthus1604
Copy link
Contributor Author

ichthus1604 commented Dec 6, 2022

@nopara74 @soosr @MaxHillebrand latest commit introduces several improvements:

1 - Reduces available options from 4 to 3
2 - Fixes layout on large and smaller window sizes, looks good in all cases IMO.
3 - Makes XAML as consistent as possible between the 2 Views (RecoveryWordsView and ConfirmRecoveryWordsView). This is quite tricky because the latter has no Continue button, and the Skip button's visibility is dependent on whether we're in "debug mode", therefore available space varies between the two. Still, I have managed to make it quite consistent in both the XAML structure and the visual layout IMO.

Let me know your thoughts.

@yahiheb
Copy link
Contributor

yahiheb commented Dec 7, 2022

@nopara74 @soosr @MaxHillebrand latest commit introduces several improvements:

@ichthus1604 Did you forget to push this commit or am I missing something?

@ichthus1604
Copy link
Contributor Author

@yahiheb my bad. It's pushed now ;)

@soosr
Copy link
Contributor

soosr commented Dec 7, 2022

Keep decreasing the height of the window until:
image

@yahiheb
Copy link
Contributor

yahiheb commented Dec 7, 2022

This is an overkill, there is no need to take the user back to the recovery words list, just stop the user from entering other words if they make a mistake.

At first, I thought you are right, but then I tried out the feature and made a mistake on purpose just to see how it feels. And it gave me the feeling that it is actually very important to write down these words correctly. ACK for the current implementation from me.

Even if you write down the recovery words correctly, but you select a wrong word for example the 7th or worse the 12th, then you get back to the recovery words list and then reselect all of them again, and you might again make another mistake and this cycle could keep repeating. This is very slow and bad UX.

This whole PR is made to improve the UX of confirming the recovery words not to make it worse.

@ichthus1604
Copy link
Contributor Author

@soosr

Keep decreasing the height of the window until

Fixed in latest commit ;)

@ichthus1604
Copy link
Contributor Author

ichthus1604 commented Dec 7, 2022

@yahiheb

This is very slow and bad UX

It would be much worse UX if you had wrongly stored your recovery words and the software didn't give you the chance to notice that until it's too late, when you try to actually recover your wallet and it fails because your recovery words are wrong, and there's no going back and you lost all your BTC and end up poor and homeless.

No?

@yahiheb
Copy link
Contributor

yahiheb commented Dec 7, 2022

It would be much worse UX if you had wrongly stored your recovery words and the software didn't give you the chance to notice that until it's too late

This is not the case. If I have stored them wrongly then I should notice that when I select a word while confirming the words and I can go back manually and see the recovery words.

As I explained here #9640 (comment) even if I store them correctly I am forced to go back and start over whenever I make a mistake choosing a word.

@soosr
Copy link
Contributor

soosr commented Dec 8, 2022

As I explained here #9640 (comment) even if I store them correctly I am forced to go back and start over whenever I make a mistake choosing a word.

That is the point, if you made a mistake you probably didn't pay as much attention as was needed. When you have to start it over it will make it clear to you that it is super important, not just to write them down properly, but also to be able to re-type them in the correct order anytime.

soosr
soosr previously approved these changes Dec 8, 2022
Copy link
Contributor

@soosr soosr left a comment

Choose a reason for hiding this comment

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

I give my approval, but we should really consider adding back the continue button...

  1. The dialog just looks shit without it.
  2. It bothers no one if it is there, moreover it suggests that there will be additional steps too, cuz right now it is just a mystery.
  3. It is useless as we are navigating forward as soon as all the words are confirmed.
    • Except when the user cancels the password dialog... Now it navigates back to the previous page which is not expected. Cancel should only close that dialog and don't do any additional navigation.
    • So in that case an active continue button would make sense.

image

@yahiheb
Copy link
Contributor

yahiheb commented Dec 8, 2022

As I explained here #9640 (comment) even if I store them correctly I am forced to go back and start over whenever I make a mistake choosing a word.

That is the point, if you made a mistake you probably didn't pay as much attention as was needed. When you have to start it over it will make it clear to you that it is super important, not just to write them down properly, but also to be able to re-type them in the correct order anytime.

If you make a mistake selecting the words you don't need to go back to the recovery words, you just have to check your back up and select the correct word, there is no need for all this back and forth over and over whenever a mistake is made, this is terrible UX.

@yahiheb
Copy link
Contributor

yahiheb commented Dec 8, 2022

I give my approval, but we should really consider adding back the continue button...

I agree we should add back the Continue button, but we should keep its behavior the same as we used it in other dialogs.
Do not move automatically when all 12 words are correct, just enable it and the user can click it.

Copy link
Contributor

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

These are my suggestions:

  • No need to take the user back to the recovery words list, just stop the user from selecting other words if they make a mistake. Don't use a pop up window if the selected word is wrong, just give it a different color (red maybe).

  • Add back the Continue button, with the same behavior as we used it in other dialogs.

  • Select only from the 12 recovery words when suggesting the 3 words.

  • There is no need for the tooltip when you hover over the 3 words.

  • I personally don't like the blue color when I hover over the 3 words, we never used such color.

@ichthus1604
Copy link
Contributor Author

@soosr @yahiheb latest commit restores the Continue button functionality

@yahiheb
Copy link
Contributor

yahiheb commented Dec 10, 2022

@soosr @yahiheb latest commit restores the Continue button functionality

That is good, but do not move automatically to the next dialog to keep same behavior of the continue button elsewhere. Let the user do that manually.

@ghost
Copy link

ghost commented Jan 6, 2023

superceeded

@ghost ghost closed this Jan 6, 2023
This pull request was closed.
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.

[VDG] Order-checking instead of retyping the recovery words for BIP39 backup confirmation
5 participants