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

Persistent words in recovery #3859

Closed
Hannsek opened this issue May 24, 2024 · 4 comments · Fixed by #4110
Closed

Persistent words in recovery #3859

Hannsek opened this issue May 24, 2024 · 4 comments · Fixed by #4110
Assignees
Labels
bug Something isn't working as expected

Comments

@Hannsek
Copy link
Contributor

Hannsek commented May 24, 2024

Describe the bug
When a word (e.g. 3rd) is deleted and user goes to the previous word (2nd), change the 2nd word and go to the next word (3rd) the word is present there.

Firmware version and revision
Core

To Reproduce
Steps to reproduce the behavior:

  1. type 3rd word in recovery and confirm it
  2. Delete the word and go to 2nd word, change it and confirm it
  3. 3rd word is be present

Expected behavior
Once the word is deleted it shouldn't appear again.

@Hannsek Hannsek added the bug Something isn't working as expected label May 24, 2024
@Hannsek Hannsek added this to Firmware May 24, 2024
@Hannsek Hannsek moved this to 🎯 To do in Firmware May 24, 2024
@Hannsek Hannsek removed the status in Firmware May 24, 2024
@Hannsek
Copy link
Contributor Author

Hannsek commented Aug 1, 2024

@trezor/qa can you please try if this is still happening? IMO it should be fixed by reworking the flow with TS5 but I'm not sure now and I cannot test it now by myself. :/

@bosomt
Copy link

bosomt commented Aug 1, 2024

QA NOK

im sorry its still there :(
used
academic again academic academic academic seed...
returned to again
after i filled in again academic is prefilled

2024-08-01.13.29.54.mp4

Info:

  • Suite version: web 24.8.0 (6016db166cafd3d2fd2cae67cafe95e9e08cd3f3)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:128.0) Gecko/20100101 Firefox/128.0
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.1 regular (revision 55473a0)
  • Transport: BridgeTransport 2.0.32

@obrusvit
Copy link
Contributor

obrusvit commented Aug 17, 2024

@Hannsek I've looked into this but the requirement is unclear and it's a little bit more complicated:

Currently

There are two ways how to go to previous word:

  • delete the input field and press the "Previous" button (TS3, TS5)
  • swipe from the left to the right (TT, TS5)

That means, TS5 has two options (swipe is inherited from TT). The swipe movement is interesting because it allow to go back multiple words without affecting them. So if you are at 6th word, you can swipe right to the 3rd word, rewrite it, and "confirm" again the pre-filled 4th and 5th word. Current behavior when going to previous word:

  • TS3 - wrong, a user deletes the word but it is persistent
  • TT - intuitive, a user does not delete anything
  • TS5 - intuitive on swipe, wrong on "Previous" button

Possible solution:

  1. Keep the swipe behavior but distinguish whether the user go to previous word with something pre-filled or whether the input field is empty. Then use whatever was there previously as pre-fill.
    • TS3 - problem solved
    • TT/TS5 - problem solved, behavior of swiping would be preserved and deleting everything and going back would be intuitive
  2. Always delete the ith word if you go to (i-1)th word
    • TS3 - problem solved
    • TT/TS5 - swipe would delete everything
  3. Always delete the ith word if you go to (i-1)th word and disable going back by a swipe movement on both TT and TS5
    • TS3 - problem solved
    • TT would have to get a new button to "go back" if the input field is empty, similar to TS5
    • TS5 - problem solved
    • TT/TS5 would lose the ability to go back multiple words and fix them

EDIT:
We'll go with a variation on 3.
Always delete the ith word if you go to (i-1)th word and disable going back by a swipe movement on TS5

  • TS3 - problem solved
  • TS5 - problem solved
  • TT - potentially undesirable that swipe deletes the word
  • So all models lose the ability to go back multiple words while keeping the words unchanged.

@obrusvit obrusvit self-assigned this Aug 17, 2024
@Hannsek
Copy link
Contributor Author

Hannsek commented Aug 18, 2024

The swipe on TS5 shouldn't be there as it might lead some users into unexpected situations. I'd choose number 3 but keep the swipe action on TT (which would delete the ith word)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants