feat(ui): three buttons, and a label you can read on each - #83
Merged
Conversation
Four designs for "the primary button" existed at once, and they disagreed about every part of it. The text was `Color::WHITE` in three places and `palette.background` in two; the radius was 6 in three and 3 in two; hover was 0.85, 0.9, or the border colour depending where you looked. The text colour was not merely inconsistent. White on the dark theme's accent measures 3.21:1 and on its danger 3.35:1, under the 4.5:1 a button label needs, while `palette.background` clears it in both themes — 5.52 and 5.30 on dark, 4.89 and 4.74 on light. So the primary button of every confirmation dialog, the Commit button and both welcome buttons were below AA on the default theme. That is the blind spot this item was for. The palette's contrast is thoroughly asserted — body text, secondary text, the semantic colours used *as* text, the graph lanes, even under simulated colour blindness — but nothing checked a label *on* a filled button, because no palette slot describes that pairing. Painting a destructive button with the accent passed every test in the crate too, and now does not. Two of the offending whites were painted from inside the button's content, over the style's own `text_color`, where no change to the style could have corrected them. The guard that keeps rules out of view files now also refuses `Color::WHITE` and `Color::BLACK` there: after the scrim, both hardcoded colours have caused the same class of bug, and neither has a legitimate use left outside the highlighter's contrast bisection. Disabled is an opaque pair rather than the fill at an alpha. What an alpha lands on depends on whatever is behind the button, so its contrast cannot be stated, let alone asserted — and asserting it is the point. `quiet` gains an outline, which the welcome screen's secondary button already had and the dialogs' Cancel did not. `UI_SPEC.md` wants Cancel unemphasised; an outline does not undo that, it only makes the target visible before the pointer is over it. Left alone: the rebase plan's segmented verb control and the tab's close button, which are not one of the three roles and would be worse for pretending to be.
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.
C2, and it found a live accessibility failure.
Four primary buttons, disagreeing about everything
Color::WHITE(3 places) vspalette.background(2)palette.borderborder+mutedThe text colour was not merely inconsistent
Measured against WCAG 2.1:
Color::WHITEon accentColor::WHITEon dangerpalette.backgroundon accentpalette.backgroundon dangerButton labels need 4.5:1. So on the default theme, the primary button of every confirmation dialog, the Commit button, and both welcome buttons were below AA.
Why nothing caught it
This is exactly the blind spot the plan named. The palette's contrast is thoroughly asserted — body text, secondary text, the semantic colours used as text, the graph lanes, even under simulated deuteranopia and protanopia. Nothing checked a label on a filled button, because no palette slot describes that pairing.
Painting a destructive button with the accent also passed every test in the crate. It now fails one.
Two of the offending whites were painted from inside the button's content, over the style's own
text_color— no change to the style could have corrected those. So the guard that keeps rules out of view files now also refusesColor::WHITEandColor::BLACKthere. After the scrim in #81, both hardcoded colours have caused the same class of bug, and neither has a legitimate use left outside the highlighter's contrast bisection.Design notes
Disabled is an opaque pair, not the fill at an alpha. What an alpha lands on depends on whatever is behind the button, so its contrast cannot be stated — let alone asserted, which is the point.
quietgains an outline. The welcome screen's secondary button already had one; the dialogs' Cancel did not.UI_SPEC.mdwants Cancel unemphasised — an outline does not undo that, it only makes the target visible before the pointer is over it.Left alone: the rebase plan's segmented verb control and the tab's close button. Neither is one of the three roles, and both would be worse for pretending to be.
Sabotage — two, both confirmed
Color::WHITEa_button_label_is_readable_on_the_button— "dark/primary in Active is 3.21:1, under the 4.5:1 a label needs"dangeris filled withaccenta_destructive_button_never_wears_the_ordinary_one_s_colourThe first reproduces the exact number computed by hand before any code changed, which is what makes it a measurement rather than an opinion.
Screenshots
Three retaken —
graph,stagingandlight-theme. The toolbar buttons are outlined now, and the Commit button's label changed colour.command-paletteis unchanged: it has no buttons.Gate
cargo fmt --all -- --check,cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo test --workspace --all-features --locked— 900 tests, all green.