Finish the nav convention, and confirm with a tick - #12
Closed
oroderico wants to merge 4 commits into
Closed
Conversation
Four gaps from reviewing the chrome after it landed, all the same shape: a
convention that holds almost everywhere teaches the reader it does not hold.
The Wallet and Origo menus were still on the old list, so the two most-visited
screens in the firmware were the two that looked unlike the rest. They wear the
chrome now, without an arrow: choose_nav grew a `back` flag, the way nav_screen
already had one. Neither screen has a level above it - Origo is the top, and a
loaded wallet's only exit erases the session and reboots. Putting that on the
arrow would give a control that is cheap and reversible on every other screen a
destructive meaning here, one press from any row, which is the same class of
inconsistency the chrome was built to remove. Both keep their exit as a row the
reader travels to. A screen with somewhere to go back to gets an arrow; one
without does not pretend.
The ring arithmetic learned about the missing arrow rather than assuming it:
nav_ring_size, nav_position and nav_selection take `back` and count its slot
only when it is there.
QR export and Descriptor export opened on the confirm bar while every other
warning screen opened on the arrow - including Compact SeedQR, which makes the
same kind of claim. All three say a photograph of what follows gives away more
than the reader may intend; the way forward should not be preselected on any of
them. A test now pins that, matched on the warning text a screen shows rather
than on a list of call sites, so a new screen making one of those claims is
covered the day it is written. Proved by putting one of the two back.
nav_bar_label_fits claimed to hold every screen on the chrome and did not hold
the coin-word reflip warning, which arrived after the table was written.
The old path went with the menus: choose, choose_at and choose_kept had no
callers left, and -Werror=unused-function said so. seedtool_render_list stays
for the self-test's render smoke checks - the linker already drops it from the
image, which is why deleting it would cost diff and save nothing.
Left alone deliberately: the footer hint on the dice, value and brightness
screens. Those have no chrome and up/down there change a value rather than
moving a cursor, so the hint is the only thing telling the reader what the
buttons do. Its comment did describe a string that no longer exists ("L/R
move"), which is fixed.
The image is 320 bytes smaller.
Verified with the host self-test, 52 Python tests, and a firmware build the
audit passes at 303664 bytes.
Rules between the rows and a bordered back button, taken from Jade's Settings screen rather than approximated: ui/dashboard.c:362 builds it, ui/dialogs.c draws it. Three pieces, and the third is why the first two are not enough on their own: - A rule above every row, so rows read as cells rather than as stacked lines of text. Jade borders each menu item and the top one twice (dialogs.c:290). It sits in the gap between cells, where the selection bar never paints over it. - The back arrow gets a box, drawn whether or not it is selected, dim when idle. A bare arrow in the corner reads as decoration on a screen with no other chrome up there; a box reads as something that can be taken. - The box has no bottom edge, and the rule above the first row is what closes it. That is Jade's own arrangement, with its reason in its comment: "bottom edge will be covered by upper line above top menu item" (dialogs.c:268). Without it the two are a button hovering over an unrelated list; with it they are one frame. The box grew a pixel, 18 to 19, so its sides reach the rule instead of stopping short of it. Both list renderers get the rules, not just the nav one: the plain list still draws the Wallet and Origo menus, and a screen that gains the chrome should not also change how its rows are drawn. nav_chrome_bands_do_not_collide had to be told the difference between a rule and a collision. It asserted the gap under the title bar was entirely dark, and the new rule lands there - but its own comment says what it is for, which is catching text that wrapped into the band. So it now permits exactly the rule's colour and still fails on anything brighter, which is what a glyph is. Verified by pushing the title 5px down and watching it fail. Verified with the host self-test, 51 Python tests, and a firmware build the audit passes at 304064 bytes. Walked on hardware.
A screen the reader takes or leaves has two controls and nothing else, so they sit where Jade puts them: reject on the left of the title bar, accept on the right, nothing along the bottom (ui/confirm_address.c builds exactly that, and passes no footer buttons at all). The slot was already there. The title has been centred between two margins the width of the arrow's box since the chrome arrived, so the right margin has been empty and exactly the right size all along - the tick moves in without anything else moving over. Drawn rather than set, for the same reason the back arrow is: the 16px face has no glyph for a tick and Jade's symbols font is a component the audit rejects by name. Two strokes of 2px squares from a common low point, so the diagonals read as solid rather than as a dotted line. What this gives up is the label. The bars said what would happen - Show QR, Open wallet, Proceed anyway - and a tick says only yes. That is why it goes on the screens whose title already names the act, and why the label stays in the struct: the self-test still measures it, and a caller that needs the words back can still draw the bar. The lists keep theirs, where the confirm is one stop of a ring among the items rather than one of two answers. Two self-test checks needed rethinking rather than adjusting, and finding out which took instrumenting all seventeen of the function's exits to say which had fired: - nav_bar_label_fits measured a bar these screens no longer draw. It would have failed loudly, but only because it treats an absent label as a failure - so the screens now assert their slot is drawn instead. On a notice, which has no arrow, the tick is the only control: if it failed to draw, the screen would be one the reader cannot leave, and nothing else here would notice. - nav_title_stays_in_its_column asserts nothing lights right of x=218, to catch a title overflowing its column. The tick's box starts at exactly 218, so the check could no longer tell an overlong title from the control. The title's placement depends only on whether there is an arrow, never on the tick, so it is measured on a tick-free rendering of the same screen. Confirmed still honest by giving one screen an absurd title and watching it fail. Verified with the host self-test, 52 Python tests, and a firmware build the audit passes at 303920 bytes. Walked on hardware.
The confirm was a tick on the screens that are only a question, and still a bar everywhere else - which is the same convention-in-half the branch below this one exists to fix. It is a tick everywhere now: Review words, the numbered lists, the transcript and hash pages. No screen draws a bar, and none of the labels that went with them said anything a tick does not: they were all "Continue". The ring objection turns out to be weaker than it looks. Reading it as a walk down the screen breaks at exactly one point either way - with a bar, moving down off it wrapped up to the arrow; with the tick, the last row wraps to the corner. A ring has one discontinuity wherever it is put, and putting it here sets the two ways out of a screen side by side rather than at opposite ends. Two things this got wrong first, both caught rather than reasoned out: The tick ignored confirm_enabled, so the checksum screen with a word still wrong drew a live-looking control that could not be taken. It has the bar's three states now - filled, outlined, dim. And it drew on menus, which pass no label because their rows are their actions. The bar guarded against that with `if (!nav->confirm) return;` and the tick did not inherit it, so the Origo and Wallet menus each grew a tick offering an answer to a question they never ask. Found on the device, not here, and the reason nothing here found it is worth keeping: every check written for the tick asserted it was *present*. A test that only knows how to see presence cannot see excess. The menu case now asserts both places a confirm could appear stay empty, and was proved by putting the bug back. The self-test's five chrome blocks all built their nav structs by hand without the tick, so they were measuring a layout the firmware had stopped shipping. Each also had to measure its title on a tick-free rendering: the title's column ends exactly where the tick's box begins, so an overlong title landing past it reads as the control. nav_bar_label_fits went with the bars - the compiler pointed out it had no callers left, which is also how it is known no screen draws one. Verified with the host self-test, 52 Python tests, and a firmware build the audit passes at 303936 bytes. Walked on hardware.
This was referenced Aug 16, 2026
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
1 participant
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.
Draft — parked overnight, not ready to merge. Opened as a draft so CI runs while nobody is watching and tomorrow starts with an answer rather than a wait.
What is here
Four commits, in the order the decisions happened:
fd81835Finish the nav convention — the review findings from One back arrow and one confirm bar, instead of a footer per screen #11 after it landed. The Wallet and Origo menus were still on the old list, so the two most-visited screens looked unlike the rest; they wear the chrome now, without an arrow, since neither has a level above it and the wallet's only exit erases the session. QR export and Descriptor export opened on the confirm rather than the arrow while every other warning screen opened on the arrow. The oldchoose/choose_at/choose_keptpath went with the menus —-Werror=unused-functionsaid so.5bc9ccbFrame the menus like Jade — rules between rows, a bordered back button whose bottom edge is the first row's rule. Taken fromui/dialogs.crather than approximated.6a904dfConfirm with a tick — reject on the left of the title bar, accept on the right, nothing along the bottom, the wayui/confirm_address.clays it out. The slot was already held open by the title's own margins.21936e4Take the tick everywhere — lists and paged screens too, so there is one confirm control in the firmware.Verified
Host self-test, 52 Python tests, firmware build and ELF audit at 303936 bytes — 58% of the ceiling. Walked on hardware after each step.
Worth knowing before reviewing
Two bugs in the tick were found on the device, not by the tests, and the second one says something about the tests: every check written for the tick asserted it was present, so nothing could notice it appearing where it should not. It drew on menus, which pass no label because their rows are their actions. The menu case now asserts both places a confirm could appear stay empty.
The self-test's chrome blocks each measure their title on a tick-free rendering, because the title's column ends exactly where the tick's box begins — an overlong title landing past it would read as the control rather than as the overflow it is.
Still open
collect_entropy,flip_words) — flagged in review, deliberately left out of this branch.