Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #7113, #7181: Ability to reveal password during wallet unlock, restore, and create #7269

Merged
merged 6 commits into from
May 1, 2023

Conversation

StephenHeaps
Copy link
Contributor

Summary of Changes

  • Add a button beside the password field to reveal the entered password, moved biometrics buttons below (having 2 buttons beside the field looked too busy).
  • When unlocking a wallet, the password field will be set to secure/hidden before we auto-fill the password via biometrics (when applicable).
  • When restoring a wallet, revealing the recovery phrase will hide the password(s) and vice-versa.
    • This is done to keep a SecureField on screen which will blocks 3rd-party keyboards on this view.
    • Unlock wallet & create/setup wallet do not need this. iOS seems to detect that a SecureField was present on screen previously and password is being revealed, where having 3 fields on view for restore seems to trip up that logic and allows 3rd-party keyboards when the fields are revealed.
    • Removing the .onChange(of: isPasswordRevealed) & .onChange(of: showingRecoveryPhase) logic from RestoreWalletView will allow 3rd-party keyboards when the fields are revealed.
  • Designs: https://www.figma.com/file/oXIiv45pIhF2RGlpEjOfUg/Wallet-onboarding?node-id=50-11678&t=Lrq127EJo2HLMio1-0

This pull request fixes #7113, fixes #7181

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New or updated UI has been tested across:
    • Light & dark mode
    • Different size classes (iPhone, landscape, iPad)
    • Different dynamic type sizes

Test Plan:

Needs tested on device with 3rd-party keyboard installed. I haven't noticed any behaviour differences between iOS 15 and iOS 16.

  • Verify you cannot get 3rd-party keyboard to show for fields on Unlock, Restore and Create (setup new wallet) views.
  • Verify password is revealed when eye button is tapped; hidden when crossed-eye button is tapped.
  • Verify that auto-filling password via biometrics will hide / mask the password before autofilling.

Screenshots:

iPhone 14 Pro iPhone 8
iPhone 14 Pro iPhone 8

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@StephenHeaps StephenHeaps requested a review from a team as a code owner April 17, 2023 19:17
@StephenHeaps StephenHeaps self-assigned this Apr 17, 2023
@StephenHeaps StephenHeaps force-pushed the wallet/reveal-password branch from 1e5a0c2 to 8d24d33 Compare April 17, 2023 19:17
@StephenHeaps StephenHeaps force-pushed the wallet/reveal-password branch from 8d24d33 to aa877a4 Compare April 25, 2023 13:25
@stoletheminerals
Copy link
Contributor

Not related, but while we are here, can you add .textInputAutocapitalization(.never) for the recovery phrase text field? When you enter a recovery phrase, it auto-capitalizes the first word, which is annoying. We also don't lowercase the input, leading to an incorrect recovery phrase if the user doesn't explicitly lowercase the first word.

@StephenHeaps
Copy link
Contributor Author

Not related, but while we are here, can you add .textInputAutocapitalization(.never) for the recovery phrase text field? When you enter a recovery phrase, it auto-capitalizes the first word, which is annoying. We also don't lowercase the input, leading to an incorrect recovery phrase if the user doesn't explicitly lowercase the first word.

@stoletheminerals good suggestion, applied in 2a164c9

@stoletheminerals
Copy link
Contributor

stoletheminerals commented Apr 26, 2023

Can keyboard popping be prevented somehow? Weirdly in the screen recording it shows that when I press the "eye" it hides the keyboard, but in fact it just hides it and then instantly brings it back. Doesn't look smooth.

234652191-20bd0deb-b04d-4f47-beea-0da39b25732c.mov

Copy link
Contributor

@stoletheminerals stoletheminerals left a comment

Choose a reason for hiding this comment

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

Security-wise lgtm

@StephenHeaps
Copy link
Contributor Author

StephenHeaps commented Apr 26, 2023

Can keyboard popping be prevented somehow? Weirdly in the screen recording it shows that when I press the "eye" it hides the keyboard, but in fact it just hides it and then instantly brings it back. Doesn't look smooth.

@stoletheminerals I tried to resolve this issue a few ways but wasn't able too; I have one more idea to try. Weirdest part is it does not do this on iOS 15, only on iOS 16 🫠:

keyboard.ios.15.mov

…toring wallet.

Hide password (if revealed) before populating unlock wallet password field with stored password.
Hide recovery phrase (if revealed) before revealing wallet password when restoring wallet from recovery phrase.
…llet. Don't focus keyboard when pre-filling password via biometrics.
…ton interaction when text is inputted in the field (SwiftUI quirk with the WalletPromptView)
@StephenHeaps StephenHeaps force-pushed the wallet/reveal-password branch from 2a164c9 to 06d5ccf Compare May 1, 2023 15:49
@StephenHeaps StephenHeaps added this to the 1.51 milestone May 1, 2023
@StephenHeaps StephenHeaps enabled auto-merge (squash) May 1, 2023 16:05
@StephenHeaps StephenHeaps merged commit 6122a2d into development May 1, 2023
@StephenHeaps StephenHeaps deleted the wallet/reveal-password branch May 1, 2023 16:18
StephenHeaps added a commit that referenced this pull request May 10, 2023
…lock, restore, and create (#7269)"

This reverts commit 6122a2d.
iccub pushed a commit that referenced this pull request May 10, 2023
* Revert "Fix #7113, #7181: Ability to reveal password during wallet unlock, restore, and create (#7269)"

This reverts commit 6122a2d.

* Revert #7382 - auto-focus password field in unlock wallet
iccub pushed a commit that referenced this pull request May 10, 2023
* Revert "Fix #7113, #7181: Ability to reveal password during wallet unlock, restore, and create (#7269)"

This reverts commit 6122a2d.

* Revert #7382 - auto-focus password field in unlock wallet
arthuredelstein pushed a commit to brave/brave-core that referenced this pull request Feb 13, 2024
…sword during wallet unlock, restore, and create (brave/brave-ios#7269)

* Add `RevealableSecureField` password field for Unlock, Create and Restoring wallet.
Hide password (if revealed) before populating unlock wallet password field with stored password.
Hide recovery phrase (if revealed) before revealing wallet password when restoring wallet from recovery phrase.

* Move Biometrics button below Unlock/Restore buttons when unlocking wallet. Don't focus keyboard when pre-filling password via biometrics.

* Disable autocapitalization in recovery phrase field
arthuredelstein pushed a commit to brave/brave-core that referenced this pull request Feb 13, 2024
…e-ios#7427)

* Revert "Fix brave/brave-ios#7113, brave/brave-ios#7181: Ability to reveal password during wallet unlock, restore, and create (brave/brave-ios#7269)"

This reverts commit 6122a2d.

* Revert brave/brave-ios#7382 - auto-focus password field in unlock wallet
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants