fix: give NBGL's reject buttons the SDK's own words instead of Done - #154
Merged
Merged
Conversation
NBGL's check-result screen shared one title, "Valid Secret\nRecovery
Phrase", between "the phrase is well formed but doesn't match this
device's seed" and "the phrase matches" -- only the paragraph
underneath said which. The two-button screens already had this right:
ux_bip39_nomatch_flow and ux_bip39_match_flow (src/bagl/ux_nano.c)
carry two different titles, "doesn't match" and "is correct", on two
distinct flows. NBGL's invalid-phrase screen also gave no advice,
where the two-button invalid flows share a line telling the user to
check length, order and spelling.
Both are fixed the same way: two new macros in ui_strings.h
(UI_STR_NBGL_RESULT_NOMATCH_TITLE, UI_STR_NBGL_RESULT_INVALID_ADVICE),
and display_check_result_page() (src/nbgl/ui.c) now indexes a
three-title array by the same 0/1/2 outcome it already computed for
the verdict icon, instead of pulling a title from the two-row body-text
table. The invalid case's advice goes into a third text field
(nbgl_pageInfoDescription_t.centeredInfo.text3), which the SDK only
renders as its own line under LARGE_CASE_GRAY_INFO -- the style this
screen used before, LARGE_CASE_INFO, maps text2 and text3 onto the
same underlying field, so text3 would have silently replaced the
verdict's own body text rather than appending to it
(nbgl_layoutAddCenteredInfo() in the SDK). Confirmed under Speculos on
stax before adopting the fix: the body text disappeared, replaced by
the advice line, the moment text3 was set under the old style.
The new nomatch title, "Mismatched Secret\nRecovery Phrase", keeps the
exact "<Adjective> Secret\nRecovery Phrase" template the other two
outcomes already use ("Invalid Secret", "Valid Secret") -- including
the same line break, after "Secret" rather than after "Recovery". The
three screens read as one answer set with a single word changing,
rather than three differently-shaped messages, which matters most on
the screen the project's own planning doc calls the one where a fault
injection can flip the result. Confirmed by rendering all three
screens under Speculos on stax and reading them side by side, not by
inspecting the string alone.
No two-button file changes: those flows already worked this way.
Confirmed on all six targets, built before and after with strings
extracted and diffed: `nanos`, `nanox` and `nanos+` are byte-for-byte
identical; the touch targets' diffs contain exactly the two new
strings and the new title array's symbol, nothing else.
tests/functional/test_bip39_seed_match.py extends from two verdicts to
three (its own docstring predicted the need), and its touch-side
helper now asserts the title too, not just the body line -- the
touch/two-button split this file already draws is what made checking
"the two stacks answer the same question the same way" for the new
invalid case a small addition rather than a new file. The invalid
case's body-text assertion had to be a full-line prefix rather than a
free substring: ragger's wait_for_text_on_screen() matches per
rendered line with re.match, which anchors at that line's start, and
"you have entered is not valid" (this string's second line) has no
internal break before "is not valid" the way the match/nomatch
strings do before their own verdict word.
Verified: all six unit-test-matrix configurations (80/80, the two new
ui_strings.h macros added to tests/unit/tests/ui_strings.c's own
enumeration); functional suite on both stacks under Speculos (nanox
9 passed/9 skipped, stax 12 passed/6 skipped, all skips pre-existing
device gating); clang-format clean on touched src/ files.
NBGL's two useCaseChoice() screens (recover BIP39 from SSKR / generate SSKR from BIP39) and its two useCaseGenericReview() screens (showing the recovered BIP39 phrase and the generated SSKR shares) all reused a single "Done" button to decline an offer or close an already-shown secret. Neither is a completion: the SDK's own signatures call the first parameter rejectString (nbgl_useCaseChoice) and the second rejectText (nbgl_useCaseGenericReview) in lib_nbgl/include/nbgl_use_case.h, and the vendored lib_nbgl already ships "Cancel" (getRejectReviewText(), lib_nbgl/src/nbgl_use_case.c) for offer-or-decline choices and "Close" (info.navWithButtons.quitText in the same file) for dismissing an already-displayed page with no offer pending. UI_STR_NBGL_DONE is replaced by UI_STR_NBGL_CANCEL (both nbgl_useCaseChoice() calls) and UI_STR_NBGL_CLOSE (both nbgl_useCaseGenericReview() calls); the old macro has no remaining callers, so it is removed rather than left dead. Screen structure, callbacks, and logic are unchanged -- only the two button labels change. BAGL is untouched: all three Nano builds are byte-identical before and after. nanos/nanox/nanosp/stax/flex/apex_p all compile; the touch builds' text/data/bss sizes are unchanged, since Cancel/Close were already present among lib_nbgl's own linked strings and the new references merge into those existing string-pool entries instead of adding weight. tests/unit/tests/ui_strings.c updated to match the macro rename.
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
2 participants
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.
Why
src/nbgl/ui.cused one shared button,"Done", on four screens, in two roles that are both something other than completion:nbgl_useCaseChoice()display_select_recover_bip39_page(),display_select_generate_sskr_page()rejectStringnbgl_useCaseGenericReview()display_generic_review(),display_sskr_shares()rejectTextThe SDK itself names this parameter
reject*in both signatures (lib_nbgl/include/nbgl_use_case.h). The first two calls decline an offer ("do you want to recover the BIP39 phrase?" / "generate SSKR shares?"); the last two dismiss a screen that just displayed a secret (the recovered BIP39 phrase, or the generated SSKR shares) without confirming anything —review_done(), their shared callback, only resets state and returns to the home screen."Done"tells the user they finished something; in all four cases they didn't.What changed
UI_STR_NBGL_DONE(src/common/ui_strings.h) is replaced by two macros, one per role:UI_STR_NBGL_CANCEL("Cancel") on bothnbgl_useCaseChoice()calls.UI_STR_NBGL_CLOSE("Close") on bothnbgl_useCaseGenericReview()calls.Both words are already used by the vendored SDK for the same roles, not invented for this PR:
getRejectReviewText()(lib_nbgl/src/nbgl_use_case.c) returns"Cancel"for an offer-or-decline choice;"Close"is what the SDK sets asinfo.navWithButtons.quitTextwhen dismissing a single already-displayed page with no offer pending, in the same file.UI_STR_NBGL_DONEhad no other caller and is removed rather than left dead.Nothing else changes: icon, title, sub-text and the positive/confirm button on all four screens are untouched, and so are
select_recover_bip39_choice(),select_generate_sskr_choice()andreview_done()— this is a button-label fix, not a screen redesign. BAGL (Nano) never had this pattern (eachUX_STEPthere already carries its own label) and is not touched.Proof that BAGL is untouched, and that the touch builds only changed what they should
All six targets built before/after (base = the tip of the branch this PR builds on, before this commit), strings extracted from every
.elfand diffed:nanos,nanox,nanos+: byte-for-byte identical, no difference at all.stax,flex,apex_p:text/data/bssunchanged. The sorted-stringsdiff shows"Done"disappearing and nothing else of substance —"Cancel"and"Close"were already present as the SDK's own linked strings before this change, so the new references merge into those existing string-pool entries instead of adding weight.Screens captured under Speculos on
stax, before/after, for both button roles (thenbgl_useCaseChoice()offer screen and thenbgl_useCaseGenericReview()share-review screen):Done→Cancel/Close, no wrapping, no layout change otherwise, despite both new words being longer thanDone.Tests
tests/unit/tests/ui_strings.c's macro enumeration updated:UI_STR_NBGL_CANCELandUI_STR_NBGL_CLOSEadded,UI_STR_NBGL_DONEremoved.No functional-test assertion on the literal text
"Done"existed to update. Two existing functional tests already exercise the new"Close"button without any change needed:test_sskr_128bit.py(review.exit()ondisplay_generic_review()'s screen) andtest_bip39_12word.py(review.exit()ondisplay_sskr_shares()'s screen).What isn't covered by a test, and why
No functional test, on either stack, ever clicks the
Cancel/decline button of annbgl_useCaseChoice()screen — every existing test that reaches one of these two screens takes the confirm path. This gap predates this PR (the button already existed, just worded differently); this PR doesn't add or remove test coverage for that path, only relabels the button. Left as-is rather than expanded, to keep this PR's diff to the vocabulary fix it's about.Verification
-O2,-Os,-fsigned-char,-funsigned-char): 80/80 each.nanox9 passed / 9 skipped,stax12 passed / 6 skipped — both unchanged from before this PR, all skips pre-existing device gating unrelated to this change.clang-format --dry-run -Werrorclean on the touchedsrc/files.