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.

Sign up for GitHub

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

Jump to bottom

T3T1 UI - passphrase flow improvement #4094

Merged
merged 9 commits into from
Aug 21, 2024
Merged

T3T1 UI - passphrase flow improvement #4094

merged 9 commits into from
Aug 21, 2024

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Aug 7, 2024

This PR adds reassuring screen when entering empty passphrase.

In order to do that, this includes some necessary changes:

  • aa79f02 adds FlowMsg::Text variant which enables passing ShortString to uPy as str. Btw. ShortString is conveniently limited to 50 chars which needs to be revisited if we implement Remove 50 character limit from passphrase #4084 (comment)
  • ebc297f implements new component - a simple yes/no prompt with buttons. It will be used further in some flows from Figma
  • 55633a7 switching two buttons in passphrase keyboard, one for empty passphrase, one for non-empty
  • a06c83d adds dedicated PromptMsg instead of passing empty Option around. This has the benefit that the BinarySelection from ebc297f can be used as PromptScreen::new_yes_or_no()
  • 395bcb6 implements the passphrase flow

@obrusvit obrusvit added the T3T1 Trezor Safe 5 label Aug 7, 2024
@obrusvit obrusvit added this to the T3T1 milestone Aug 7, 2024
@obrusvit obrusvit self-assigned this Aug 7, 2024
@obrusvit obrusvit linked an issue Aug 7, 2024 that may be closed by this pull request
Enter passphrase on device flow - small tweaks #4054
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2024 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T2B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@obrusvit obrusvit marked this pull request as ready for review August 9, 2024 07:50
@obrusvit obrusvit requested review from TychoVrahe and Hannsek and removed request for prusnak August 9, 2024 07:51
@Hannsek
Copy link
Contributor

Hannsek commented Aug 9, 2024

cc @lapohoda

@obrusvit
Copy link
Contributor Author

obrusvit commented Aug 9, 2024

@Hannsek UI changes briefly:

The confirmation of empty passphrase:
refresh00-00000001 refresh00-00000002
refresh00-00000003 refresh00-00000004

Usage of subtitle in other screens:
refresh00-00000006 refresh00-00000007

@lapohoda
Copy link

lapohoda commented Aug 9, 2024 •
edited
Loading

@obrusvit pls doublecheck the size of confirmation button – should be circular according to Figma 👇 .. Otherwise it is ok👍

image

@obrusvit
Copy link
Contributor Author

obrusvit commented Aug 15, 2024 •
edited
Loading

UI diff of FIDO2 actions are coincidental due to confirm button stylesheet change and will be superseded by #4069.

@lapohoda thank you for your comment, the "confirm empty" button was in fact narrower by 2px due to a mistake. Fixed in 7e86a32

PR is ready for review.

matejcik
matejcik approved these changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

utACK.

general note: it's not great that you're adding Text(ShortString) in there, and especially not great that you're adding it to the passphrase Confirmed() variant. What we originally intended with ComponentMsgObj (on the model_tt side) is that you return a "user is done" message, then the ComponentMsgObj impl asks the layout to give out a &str, and we convert that to micropython.
Here, we'd want .map() to pass you both the original layout and the returned message -- so that you can do this part in the map() closure.

on the other hand, i'd prefer getting rid of Map component the way we use it now. and currently doing it like this fits well with overall way of doing things in mercury. We can refactor the things when refactoring time comes, i guess.

@obrusvit obrusvit added the translations Put this label on a PR to run tests in all languages label Aug 21, 2024
@obrusvit
feat(core/ui): introduce FlowMsg::Text option
bf5b4b1 
This commit allows flows to work with ShortString which can be converted
to micropython as str.

[no changelog]
@obrusvit
feat(core/mercury): allow passphrase in FlowStore
f151318 
[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/ui-t3t1/passphrase-flow-improvements branch from 7e86a32 to 9bca204 Compare August 21, 2024 14:36
@obrusvit
feat(core/mercury): introduce binary_selection
3381957 
This commit adds a Component which prompts a user with a pair of two
buttons - left and right. The Component is parametrized by the buttons
contents and styles.

[no changelog]
@obrusvit
feat(core/mercury): passphrase confirm empty btn
9517145 
[no changelog]
@obrusvit
refactor(core): use PromptMsg instead of Option<>
af93fd7 
PromptScreen now uses dedicated PromptMsg with Confirmed and Cancelled
values instead of empty Option<>. This change affects only mercury code.

This is more explicit and enables "yes" or "no" prompts screens.
Otherwise, the "no" option was handled by the 'x' button handled by
Frame.

[no changelog]
@obrusvit
feat(core/mercury): impl flow_request_passphrase
c1726f9 
This commit replaces request_passphrase with flow_request_passphrase.
The added benefit is that the user is prompted for confirmation if they
want to proceed with an empty passphrase.
@obrusvit
chore(core): regenerate lang jsons
98685ba 
[no changelog]
@obrusvit
feat(core): slight change of passphrase copy
a1fdfbe 
Usage of different copy in mercury (especially titles and subtitles)
requires moving the layout code deeper into the model specifics.

[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/ui-t3t1/passphrase-flow-improvements branch from 9bca204 to a1fdfbe Compare August 21, 2024 15:02
@github-actions GitHub Actions
Copy link

legacy UI changes device test(screens) main(screens)

@obrusvit
Copy link
Contributor Author

Merging amidst T3T1 coverage failing. I'll take a look at different PR.

@obrusvit obrusvit merged commit 9b7ce3c into main Aug 21, 2024
120 of 121 checks passed
@obrusvit obrusvit deleted the obrusvit/ui-t3t1/passphrase-flow-improvements branch August 21, 2024 22:47
@obrusvit obrusvit mentioned this pull request Sep 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik matejcik approved these changes

@TychoVrahe TychoVrahe Awaiting requested review from TychoVrahe

@Hannsek Hannsek Awaiting requested review from Hannsek

Assignees

@obrusvit obrusvit

Labels
T3T1 Trezor Safe 5 translations Put this label on a PR to run tests in all languages
Projects
Firmware
Archived in project
Milestone
T3T1
Development

Successfully merging this pull request may close these issues.

Enter passphrase on device flow - small tweaks
4 participants
@obrusvit @Hannsek @lapohoda @matejcik