Hide written chars in passphrase keyboard#6851
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes add a passphrase display state machine (Hidden, LastOnly, LastWithMarker, Shown) across bolt, delizia, and eckhart keyboard components. Input now updates Sequence Diagram(s)sequenceDiagram
actor User
participant Keypad
participant Input
participant Timer
participant Renderer
User->>Keypad: Press key
Keypad->>Input: Deliver keystroke (single / multi-tap)
Input->>Input: Update display_style (LastOnly / LastWithMarker)
alt single-char
Input->>Timer: Start LAST_DIGIT_TIMEOUT
else multi-tap
Input->>Timer: Stop (keep marker visible)
end
Input->>Renderer: Request repaint (masked + last/marker)
Timer-->>Input: Timeout
Input->>Input: Set display_style = Hidden
Input->>Renderer: Request repaint (masked asterisks)
User->>Input: TouchStart in reveal area
Input->>Input: Set display_style = Shown
Input->>Keypad: Suppress keypad/page/back/confirm interactions
Input->>Renderer: Request repaint (overlay full text)
User->>Input: TouchEnd
Input->>Input: Set display_style = Hidden
Input->>Keypad: Re-enable interactions
Input->>Renderer: Request repaint (masked asterisks)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the on-device passphrase keyboards to improve privacy by masking entered characters (briefly showing the last character, plus touch-to-reveal behavior), aiming to unify behavior across device layouts/models.
Changes:
- Implement masking/reveal logic for passphrase input on Delizia and Bolt (timer-based “show last char briefly”, hold-to-reveal full passphrase, disable keypad while revealed).
- Adjust Eckhart passphrase touch-end handling to reliably hide revealed passphrase.
- Minor Delizia UI/theme cleanup (remove unused
borders()helper usage and related no-op insets; tweak keyboard icon color).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/apps/base.py | Modifies handle_Ping button-protection branch (currently includes passphrase-request UI + commented debug code). |
| core/embed/rust/src/ui/layout_eckhart/firmware/keyboard/passphrase.rs | Tweaks touch-end behavior while passphrase is revealed. |
| core/embed/rust/src/ui/layout_delizia/ui_firmware.rs | Removes Border wrapper (previously only applied zero insets). |
| core/embed/rust/src/ui/layout_delizia/theme/mod.rs | Adjusts keyboard button icon color; removes borders() helper (was zero-insets). |
| core/embed/rust/src/ui/layout_delizia/component/progress.rs | Removes no-op inset(theme::borders()) from progress area. |
| core/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rs | Adds masking/reveal behavior (dots + last-char display with timer; touch-to-reveal overlay). |
| core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rs | Adds masking/reveal behavior (asterisks + last-char display with timer; touch-to-reveal overlay). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rs`:
- Around line 349-355: Input::event() can clear multi_tap.pending_key() when a
touch-reveal commits a pending key, but if the textbox is already at max_len the
early return triggered by self.input.display_style == DisplayStyle::Shown
prevents calling update_input_btns_state(ctx), leaving the previously pending
key visually enabled after the overlay hides; ensure that after calling
self.input.event(ctx, event) you always refresh keypad states — call
update_input_btns_state(ctx) when multi_tap.pending_key() was cleared or
whenever display_style transitions from Shown back to Hidden (i.e., after
self.input.event returns) so the button enable/disable rendering is consistent
with the internal pending_key state.
In `@core/src/apps/base.py`:
- Around line 443-462: The Ping handler incorrectly prompts for a passphrase
(request_passphrase_on_device) and discards it; revert to a confirmation flow
for Ping.button_protection instead. Replace the request_passphrase_on_device
call with a protected-call confirmation (e.g., import confirm_action from
trezor.ui.layouts and, if needed, ButtonRequestType as B, then await
confirm_action("ping", TR.words__confirm, "ping", br_code=B.ProtectCall) or
equivalent confirm API) and only proceed to return Success(message=msg.message)
after the confirmation completes; do not request or store a passphrase in this
handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83465176-7dc9-4df5-ab9e-dfec97679883
📒 Files selected for processing (7)
core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rscore/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rscore/embed/rust/src/ui/layout_delizia/component/progress.rscore/embed/rust/src/ui/layout_delizia/theme/mod.rscore/embed/rust/src/ui/layout_delizia/ui_firmware.rscore/embed/rust/src/ui/layout_eckhart/firmware/keyboard/passphrase.rscore/src/apps/base.py
obrusvit
force-pushed
the
obrusvit/delizia/bolt/passphrase-hide-chars
branch
from
e4ebe80 to
831d8ed
Compare
lapohoda
approved these changes
Apr 30, 2026
romanz
added
the
translations
Contributor
Author
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rs (1)
287-293:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh key states after touch-reveal commits a pending key.
If the passphrase is already at
max_len,Input::event()clears the pending multi-tap key on reveal, but this path never recomputes the key states. After the overlay hides, the old pending key can still render as enabled even though the event loop now treats it as inactive.Suggested fix
- // Handle input touch events (reveal/hide passphrase) - self.input.event(ctx, event); + // Handle input touch events (reveal/hide passphrase) + let had_pending = self.input.inner().multi_tap.pending_key().is_some(); + self.input.event(ctx, event); + if had_pending + && self.input.inner().multi_tap.pending_key().is_none() + && self.input.inner().textbox.len() >= self.max_len + { + self.update_input_btns_state(ctx); + } // When passphrase is shown in full, disable all keypad interaction if self.input.inner().display_style == DisplayStyle::Shown { return None; }Also applies to: 533-540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rs` around lines 287 - 293, After calling self.input.event(ctx, event) and hitting the early-return when self.input.inner().display_style == DisplayStyle::Shown, ensure you refresh the keypad key states so any pending multi-tap key cleared by Input::event() is reflected in the UI; locate the same pattern around lines 533-540 and add a call to the existing key-state recomputation routine (e.g., the method that computes/enables keys on the keypad, such as recompute_keys/update_keys_state/update_key_states) before returning None so the overlay hide does not leave an enabled-but-inactive key rendered.core/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rs (1)
349-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecompute key states after touch-reveal clears the pending key.
When the textbox is already full,
Input::event()can clearmulti_tap.pending_key()on reveal, but this path never callsupdate_input_btns_state(ctx). The previously pending key can then stay rendered as enabled after the overlay hides even though it is no longer actionable.Suggested fix
- // Handle input touch events (reveal/hide passphrase) - self.input.event(ctx, event); + // Handle input touch events (reveal/hide passphrase) + let had_pending = self.input.multi_tap.pending_key().is_some(); + self.input.event(ctx, event); + if had_pending + && self.input.multi_tap.pending_key().is_none() + && self.input.textbox.len() >= self.max_len + { + self.update_input_btns_state(ctx); + } // When passphrase is shown in full, disable all keypad interaction if self.input.display_style == DisplayStyle::Shown { return None; }Also applies to: 629-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rs` around lines 349 - 355, Input::event can clear multi_tap.pending_key() during a touch-reveal, but when the textbox is full the code returns early on DisplayStyle::Shown without recomputing keypad state; call update_input_btns_state(ctx) after self.input.event(ctx, event) and before returning when self.input.display_style == DisplayStyle::Shown so any cleared pending_key() is reflected in the buttons (apply the same fix in the other occurrence around lines 629-643). Ensure you reference the DisplayStyle::Shown branch, Input::event, update_input_btns_state(ctx), and multi_tap.pending_key() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rs`:
- Around line 287-293: After calling self.input.event(ctx, event) and hitting
the early-return when self.input.inner().display_style == DisplayStyle::Shown,
ensure you refresh the keypad key states so any pending multi-tap key cleared by
Input::event() is reflected in the UI; locate the same pattern around lines
533-540 and add a call to the existing key-state recomputation routine (e.g.,
the method that computes/enables keys on the keypad, such as
recompute_keys/update_keys_state/update_key_states) before returning None so the
overlay hide does not leave an enabled-but-inactive key rendered.
In `@core/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rs`:
- Around line 349-355: Input::event can clear multi_tap.pending_key() during a
touch-reveal, but when the textbox is full the code returns early on
DisplayStyle::Shown without recomputing keypad state; call
update_input_btns_state(ctx) after self.input.event(ctx, event) and before
returning when self.input.display_style == DisplayStyle::Shown so any cleared
pending_key() is reflected in the buttons (apply the same fix in the other
occurrence around lines 629-643). Ensure you reference the DisplayStyle::Shown
branch, Input::event, update_input_btns_state(ctx), and multi_tap.pending_key()
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a868ec19-904b-48ae-8a5b-e1c8461f8ec8
📒 Files selected for processing (4)
core/embed/rust/src/ui/layout_bolt/component/keyboard/passphrase.rscore/embed/rust/src/ui/layout_delizia/component/keyboard/passphrase.rscore/embed/rust/src/ui/layout_eckhart/firmware/keyboard/passphrase.rstests/ui_tests/fixtures.json
romanz
reviewed
Apr 30, 2026
Contributor
|
Tested on T2T1 and T3T1. When entering 50 characters' passphrase on T2T1, if the last button pressed has more than one character, it is not disabled. Last button pressed is
|
Contributor
Author
|
@romanz thanks for review. I fixed the last key disabling here: f65b949 One more thing, on the physical model T, the "reveal passphrase" functionality is difficult to trigger due to the display shape. So this commit 81f89e6 expands the area to reveal passphrase the whole area above the keyboard (i.e. input field and scrollbar showing the keyboard type shown). This shrinks the area for swipe detector by a little bit but that's not an issue. |
[no changelog]
[no changelog]
- hide already written characters in the passphrase keyboard in Delizia and Bolt - holding a finger in the input field reveals the whole passphrase in a potentially multi-line manner - update fixtures
- this commit changes all touch display passphrase keyboards to allow persisting the reveal mode until the user lifts the finger off the screen anywhere, not just within the extended input shown area [no changelog]
obrusvit
force-pushed
the
obrusvit/delizia/bolt/passphrase-hide-chars
branch
from
81f89e6 to
f4f0831
Compare
Contributor
|
on
on
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3959 and #6342
Design:
Diff:
Notes for QA
Properly test passphrase keyboard of Bolt (T2T1) and Delizia (T3T1). The characters should now disappear similarly to Eckhart (T3W1). A reveal passphrase functionality should also be available if you hold finger in the input area.