From 47bdb6f976656e10cca8e773a4247e570fd801ca Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 8 Sep 2026 00:23:31 +0200 Subject: [PATCH] fix: promote the two pill colours to extended roles, fixing both contrast failures Phase 1, step 5 of docs/material-design-conformance.md. `BluePill` and `RedPill` were raw `Color` values in `Color.kt`, paired at the call site with `Color.White` and `Color.DarkGray` by eye. Both pairings were below the 4.5:1 floor, and one of them was not the colour it looked like. **This step could not leave the pixels alone, and it is the only one so far that changes them.** `Color.DarkGray` on `BluePill` measures **2.90:1**. `RedPill` was `Color(230, 32, 32, 191)` -- the four-Int constructor, whose last argument is alpha, so it is `#E62020` at 0.749. Opaque, white on it is 4.57:1 and passes; composited over the surface as it actually renders, it is **3.50:1** and does not. Any correct version of these two buttons is a visible change, so "adds, does not restyle" does not apply here and the plan already said the call sites would move in this step. **What M3 asks for here is an extended colour**, not a literal: a brand colour promoted to a full role family -- `color` / `onColor` / `colorContainer` / `onColorContainer` -- so that contrast is a property of the family rather than a decision repeated at each use. `ColorFamily` was already declared in `Theme.kt`, unused, alongside an `unspecified_scheme`; Material Theme Builder emits both, and this is what they are for. **Derived by the same rule as the gold palette**, which the fixed-roles commit established and verified: maximum in-gamut chroma at the source colour's Lab hue, sampled at M3's role tones. BluePill's hue is 277.0 and RedPill's is 36.3. role light dark blue light red light color tone 40 tone 80 #0060AB #C00012 onColor tone 100 tone 20 #FFFFFF #FFFFFF colorContainer tone 90 tone 30 #D7E2FF #FFDAD3 onColorContainer tone 10 tone 90 #001C39 #390C00 The buttons take `color`/`onColor`: 6.46:1 for the red pill and 6.44:1 for the blue, from 2.90 and 3.50. **A side effect worth having.** At tone 40 the two pills are the same lightness, so they now read as a matched pair. Before, `#E62020` sat beside `#5D8DD6` -- a saturated red next to a soft periwinkle -- and the blue looked like the lesser option. On a screen whose whole content is "commit, or wipe and leave", weighting one choice by accident is a defect of its own. **They travel on a composition local, not on `isSystemInDarkTheme()`.** `ColorScheme` has no slot for extended colours, so `LocalExtendedColors` is provided by `TorchTheme` from the same `darkTheme` it chooses the scheme with. Reading `isSystemInDarkTheme()` at the call site would have been one line shorter and subtly wrong: it ignores a caller who passed `darkTheme` explicitly, so a preview forcing dark would show light pills. The local defaults to the light families rather than to `unspecified_scheme` -- nothing composes outside `TorchTheme` today, and an invisible button is a worse way to discover that than a light-themed one. **No medium- or high-contrast variants, deliberately.** The entire surface is two buttons on one screen, and the light family's weakest pair is 6.44:1 -- clear of the floor by more than the contrast schemes would add. 32 more values for that would be out of proportion, and the comment in `Color.kt` says so rather than leaving the omission to be read as an oversight. **`QRCodeView` lost its constructor default.** `QRCodeBackgroundPainter` defaulted `backgroundColor` to `BluePill` -- a colour picked outside the theme for a surface that is almost never seen, since at the default `padding = 0.dp` the logo painter covers the rect it fills. The default is gone and the one call site passes it, so the choice is visible rather than buried. **Two new assertions, one of which is about the constructor.** `ColorSchemeContrastTest` grows to 9. The first checks both pairs of every extended family at 4.5:1. The second checks that every extended role is **opaque**, because `RedPill`'s alpha is what made the first assertion insufficient: a translucent container has no ratio of its own -- it has one only once composited -- so a contrast test would have measured a colour the user never sees. That is the bug this commit fixes, and it would have passed a naive contrast test. **The audit stopped counting its own commentary.** Fixing these call sites left a comment *explaining* what `Color.White`/`Color.DarkGray` had been, and `m3-audit.sh` counted it as a hardcoded colour -- so the file stayed in the report after being fixed. The script now drops comment lines before counting. Budget ratcheted 11 -> 9: the two real sites, plus the false positive the filter removes. **Tests.** 930 pass, 588 jvm over 71 classes and 342 android over 43, up from 926/586/340. `:composeApp:compileDebugKotlinAndroid` and `:composeApp:compileKotlinJvm` build, `m3-audit.sh --check` exits 0. The nine remaining hardcoded colours are phase 3's, and are listed by the audit. Co-Authored-By: Claude Opus 5 --- .../ui/composable/CreateProfileScreen.kt | 15 +++-- .../ui/composable/widgets/QRCodeView.kt | 9 ++- .../press/mantra/compose/ui/theme/Color.kt | 64 ++++++++++++++++++- .../press/mantra/compose/ui/theme/Theme.kt | 58 ++++++++++++++--- .../ui/theme/ColorSchemeContrastTest.kt | 55 ++++++++++++++++ docs/scripts/m3-audit.sh | 18 ++++-- 6 files changed, 196 insertions(+), 23 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/CreateProfileScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/CreateProfileScreen.kt index 7de489f3..770a0a84 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/CreateProfileScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/CreateProfileScreen.kt @@ -32,6 +32,7 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.lifecycle.viewmodel.compose.viewModel import press.mantra.compose.repository.MarmotRepository +import press.mantra.compose.ui.theme.LocalExtendedColors import press.mantra.compose.ui.view.model.CreateProfileViewModel import press.mantra.compose.ui.view.state.CreateProfileUIState @@ -292,6 +293,12 @@ fun CreateProfileScreen( ) } is CreateProfileUIState.ProfileReady -> { + // Both pills come from the extended families rather than from a + // literal paired with Color.White/Color.DarkGray by eye. The old + // pairings read 4.57:1 and 2.90:1 against their containers, and + // RedPill carried alpha 0.749, so composited over the surface the + // first was really 3.50:1. Both are below the 4.5:1 floor. + val extendedColors = LocalExtendedColors.current Spacer( modifier = Modifier.weight(1f) @@ -308,8 +315,8 @@ fun CreateProfileScreen( Button( colors = ButtonDefaults.buttonColors( - containerColor = press.mantra.compose.ui.theme.RedPill, - contentColor = Color.White + containerColor = extendedColors.redPill.color, + contentColor = extendedColors.redPill.onColor ), onClick = { createProfileViewModel.createAccount(writeSeed) @@ -326,8 +333,8 @@ fun CreateProfileScreen( Button( colors = ButtonDefaults.buttonColors( - containerColor = press.mantra.compose.ui.theme.BluePill, - contentColor = Color.DarkGray + containerColor = extendedColors.bluePill.color, + contentColor = extendedColors.bluePill.onColor ), onClick = { // TODO: Delete everything and close the app diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/QRCodeView.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/QRCodeView.kt index f4e0a4db..38909501 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/QRCodeView.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/QRCodeView.kt @@ -14,7 +14,7 @@ import androidx.compose.ui.graphics.drawscope.translate import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.dp -import press.mantra.compose.ui.theme.BluePill +import press.mantra.compose.ui.theme.LocalExtendedColors import io.github.alexzhirkevich.qrose.options.QrBrush import io.github.alexzhirkevich.qrose.options.QrLogoPadding import io.github.alexzhirkevich.qrose.options.QrLogoShape @@ -26,7 +26,11 @@ import mantra.composeapp.generated.resources.compose_multiplatform class QRCodeBackgroundPainter( private val painter: Painter, - private val backgroundColor: Color = BluePill, + // No default. It used to be the `BluePill` literal, which meant a colour chosen + // outside the theme for a surface that is almost never seen: at the default padding + // of 0.dp the logo painter covers the rect it fills. Making the one call site pass + // it keeps that visible rather than buried in a constructor default. + private val backgroundColor: Color, private val padding: Dp = 0.dp, ): Painter() { override val intrinsicSize: Size = painter.intrinsicSize @@ -71,6 +75,7 @@ fun QRCodeView( painter = painterResource( Res.drawable.compose_multiplatform ), + backgroundColor = LocalExtendedColors.current.bluePill.color, ) val painter = rememberQrCodePainter( diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Color.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Color.kt index 3e650908..e70c520d 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Color.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Color.kt @@ -2,8 +2,68 @@ package press.mantra.compose.ui.theme import androidx.compose.ui.graphics.Color -val BluePill = Color(0xFF5D8DD6) -val RedPill = Color(230, 32, 32, 191) +// --------------------------------------------------------------------------- +// Extended colour roles: the two pills +// --------------------------------------------------------------------------- +// +// The choice on CreateProfileScreen -- commit to the account, or wipe and leave -- is +// the Matrix pill choice, and the two colours carry that meaning rather than a place in +// the scheme. M3 calls this an extended colour: a brand colour promoted to a full role +// family (`color` / `onColor` / `colorContainer` / `onColorContainer`) so that contrast +// is a property of the family rather than something chosen per call site. +// +// They were `val BluePill = Color(0xFF5D8DD6)` and `val RedPill = Color(230, 32, 32, 191)`, +// paired at the call site with `Color.DarkGray` and `Color.White`. Both pairings were +// below the 4.5:1 floor -- 2.90:1 and, because RedPill carried alpha 0.749 and composited +// over the surface, 3.50:1 -- so this step could not leave the pixels alone. Any correct +// version of these two buttons is a visible change. +// +// **Derivation**, the same rule as the gold palette in the fixed-roles block: maximum +// in-gamut chroma at the source colour's Lab hue, sampled at M3's role tones. BluePill's +// hue is 277.0 and RedPill's is 36.3. +// +// role light dark +// color tone 40 tone 80 +// onColor tone 100 tone 20 +// colorContainer tone 90 tone 30 +// onColorContainer tone 10 tone 90 +// +// A side effect worth having: at tone 40 the two pills are the same lightness, so they +// read as a matched pair. Before, a saturated red sat beside a soft periwinkle and the +// blue looked like the lesser option -- which is not what the screen is asking. +// +// No medium- or high-contrast variants. The whole surface is two buttons on one screen, +// and the light family's weakest pair is 6.44:1 -- clear of the 4.5:1 floor by more than +// the contrast schemes would need to add. Generating 32 more values for that would be +// out of proportion to what they cover. + +val bluePillLight = ColorFamily( + color = Color(0xFF0060AB), // tone 40 + onColor = Color(0xFFFFFFFF), // tone 100 + colorContainer = Color(0xFFD7E2FF), // tone 90 + onColorContainer = Color(0xFF001C39) // tone 10 +) + +val bluePillDark = ColorFamily( + color = Color(0xFFACC7FF), // tone 80 + onColor = Color(0xFF00315C), // tone 20 + colorContainer = Color(0xFF004882), // tone 30 + onColorContainer = Color(0xFFD7E2FF) // tone 90 +) + +val redPillLight = ColorFamily( + color = Color(0xFFC00012), + onColor = Color(0xFFFFFFFF), + colorContainer = Color(0xFFFFDAD3), + onColorContainer = Color(0xFF390C00) +) + +val redPillDark = ColorFamily( + color = Color(0xFFFFB4A5), + onColor = Color(0xFF690000), + colorContainer = Color(0xFF93000B), + onColorContainer = Color(0xFFFFDAD3) +) val primaryLight = Color(0xFF000000) val onPrimaryLight = Color(0xFFFFFFFF) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Theme.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Theme.kt index 499efe98..761a44ab 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Theme.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/theme/Theme.kt @@ -8,7 +8,9 @@ import androidx.compose.material3.MotionScheme import androidx.compose.material3.darkColorScheme import androidx.compose.material3.lightColorScheme import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.Immutable +import androidx.compose.runtime.staticCompositionLocalOf import androidx.compose.ui.graphics.Color // The six schemes are `internal` rather than `private` so ColorSchemeContrastTest can @@ -385,6 +387,40 @@ val unspecified_scheme = ColorFamily( Color.Unspecified, Color.Unspecified, Color.Unspecified, Color.Unspecified ) +/** + * Brand colours that are not part of the generated scheme. + * + * `ColorScheme` has no slot for them -- M3 calls them extended colours and expects them to + * ride alongside -- so they travel on their own composition local, provided by [TorchTheme] + * from the same `darkTheme` the scheme is chosen with. Reading `isSystemInDarkTheme()` at + * the call site instead would be subtly wrong: it would ignore a caller who passed + * `darkTheme` explicitly, and a preview forcing dark would show light pills. + */ +@Immutable +data class ExtendedColors( + val bluePill: ColorFamily, + val redPill: ColorFamily, +) + +/** + * The extended colours for the current theme. + * + * Defaults to the light families rather than to `unspecified_scheme` so that anything + * composing outside [TorchTheme] renders a real colour instead of nothing. That should not + * happen -- as of this commit nothing in the tree does -- but an invisible button is a worse + * way to find out than a light-themed one. + */ +val LocalExtendedColors = staticCompositionLocalOf { + ExtendedColors(bluePill = bluePillLight, redPill = redPillLight) +} + +internal fun extendedColorsFor(darkTheme: Boolean): ExtendedColors = + if (darkTheme) { + ExtendedColors(bluePill = bluePillDark, redPill = redPillDark) + } else { + ExtendedColors(bluePill = bluePillLight, redPill = redPillLight) + } + /** * How much contrast the person using the device has asked for. * @@ -461,16 +497,18 @@ fun TorchTheme( val colorScheme = dynamicColorScheme(darkTheme, dynamicColor) ?: appColorScheme(darkTheme, contrast) - MaterialExpressiveTheme( - colorScheme = colorScheme, - // Every animation in the app should come from here rather than from a literal - // `tween`, so that the whole app's feel is one decision. Nothing reads it yet; - // the motion phase is what puts it to work. - motionScheme = MotionScheme.expressive(), - shapes = MantraShapes, - typography = AuxTypography, - content = content - ) + CompositionLocalProvider(LocalExtendedColors provides extendedColorsFor(darkTheme)) { + MaterialExpressiveTheme( + colorScheme = colorScheme, + // Every animation in the app should come from here rather than from a literal + // `tween`, so that the whole app's feel is one decision. Nothing reads it yet; + // the motion phase is what puts it to work. + motionScheme = MotionScheme.expressive(), + shapes = MantraShapes, + typography = AuxTypography, + content = content + ) + } } /** diff --git a/composeApp/src/commonTest/kotlin/press/mantra/compose/ui/theme/ColorSchemeContrastTest.kt b/composeApp/src/commonTest/kotlin/press/mantra/compose/ui/theme/ColorSchemeContrastTest.kt index 4cc6bf1f..e9a6f267 100644 --- a/composeApp/src/commonTest/kotlin/press/mantra/compose/ui/theme/ColorSchemeContrastTest.kt +++ b/composeApp/src/commonTest/kotlin/press/mantra/compose/ui/theme/ColorSchemeContrastTest.kt @@ -193,6 +193,61 @@ class ColorSchemeContrastTest { assertTrue(failures.isEmpty(), "unassigned roles —\n" + failures.joinToString("\n")) } + @Test + fun `the extended brand families read at 4_5 to 1`() { + // The two pills on CreateProfileScreen. They are not part of any ColorScheme, so + // nothing else in this file reaches them, and their previous incarnation is + // exactly why they need asserting: `BluePill` paired with `Color.DarkGray` by eye + // was 2.90:1, and `RedPill` carried alpha 0.749 so its white label was 3.50:1 once + // composited. Both looked fine to whoever wrote them. + val failures = mutableListOf() + + listOf( + "bluePill light" to bluePillLight, + "bluePill dark" to bluePillDark, + "redPill light" to redPillLight, + "redPill dark" to redPillDark, + ).forEach { (name, family) -> + listOf( + "onColor on color" to contrastRatio(family.color, family.onColor), + "onColorContainer on colorContainer" to + contrastRatio(family.colorContainer, family.onColorContainer), + ).forEach { (pairName, ratio) -> + if (ratio < SMALL_TEXT_MINIMUM) { + failures += "$name: $pairName is ${ratio.format()}:1" + } + } + } + + assertTrue(failures.isEmpty(), "below 4.5:1 —\n" + failures.joinToString("\n")) + } + + @Test + fun `the extended brand families are opaque`() { + // `RedPill` was `Color(230, 32, 32, 191)` -- the four-Int constructor, whose last + // argument is alpha. A translucent container has no contrast ratio of its own; it + // has one only once composited, and the test above would have measured a colour + // the user never sees. Roles are opaque. + val translucent = listOf( + "bluePillLight" to bluePillLight, + "bluePillDark" to bluePillDark, + "redPillLight" to redPillLight, + "redPillDark" to redPillDark, + ).flatMap { (name, family) -> + listOf( + "$name.color" to family.color, + "$name.onColor" to family.onColor, + "$name.colorContainer" to family.colorContainer, + "$name.onColorContainer" to family.onColorContainer, + ) + }.filter { (_, color) -> color.alpha != 1f } + + assertTrue( + translucent.isEmpty(), + "translucent roles —\n" + translucent.joinToString("\n") { "${it.first} alpha ${it.second.alpha}" }, + ) + } + @Test fun `outline separates from every surface it is drawn on at 3 to 1`() { val failures = mutableListOf() diff --git a/docs/scripts/m3-audit.sh b/docs/scripts/m3-audit.sh index 842baf6a..512f198b 100755 --- a/docs/scripts/m3-audit.sh +++ b/docs/scripts/m3-audit.sh @@ -24,7 +24,7 @@ THEME="$UI/theme" # --------------------------------------------------------------------------- # Budgets. "-1" means not yet budgeted -- reported, but never fails --check. # --------------------------------------------------------------------------- -BUDGET_HARDCODED_COLOR=11 # phase 3 drives to 0 outside theme/ +BUDGET_HARDCODED_COLOR=9 # phase 3 drives to 0 outside theme/ BUDGET_DP_LITERALS=-1 # phase 2 drives to ~0 outside theme/ BUDGET_OFF_SCALE_DP=-1 # phase 2 drives to 0 BUDGET_BARE_CLICKABLE=33 # phase 3 drives to 0 @@ -60,15 +60,21 @@ report() { fi } +# Lines that are comments rather than code. Without this a note *about* a hardcoded +# colour counts as one -- which happened the first time a call site was fixed and the +# commit explained what it had replaced. +NOT_A_COMMENT='^[^:]*:[[:space:]]*(//|\*|/\*)' + # Count matches across the UI tree, optionally excluding the theme package. # $1 pattern, $2 "exclude-theme" | "all" count() { local pattern=$1 scope=${2:-all} if [[ $scope == exclude-theme ]]; then grep -rE "$pattern" "$UI" --include=*.kt 2>/dev/null \ - | grep -v "^$THEME/" | wc -l | tr -d ' ' + | grep -v "^$THEME/" | grep -vE "$NOT_A_COMMENT" | wc -l | tr -d ' ' else - grep -rE "$pattern" "$UI" --include=*.kt 2>/dev/null | wc -l | tr -d ' ' + grep -rE "$pattern" "$UI" --include=*.kt 2>/dev/null \ + | grep -vE "$NOT_A_COMMENT" | wc -l | tr -d ' ' fi } @@ -119,8 +125,10 @@ report 'roles falling to the baseline palette' "$unset_baseline" "$BUDGET_UNSET_ hardcoded=$(count 'Color\(0x|Color\.(Red|Blue|Green|Gray|LightGray|DarkGray|White|Black|Yellow|Magenta|Cyan)' exclude-theme) report 'hardcoded Color outside theme/' "$hardcoded" "$BUDGET_HARDCODED_COLOR" -[[ $hardcoded -gt 0 ]] && grep -rEln 'Color\(0x|Color\.(Red|Blue|Green|Gray|LightGray|DarkGray|White|Black|Yellow|Magenta|Cyan)' \ - "$UI" --include=*.kt | grep -v "^$THEME/" | sed "s|$UI/| |" +# No -n, so the line is path:content and NOT_A_COMMENT's single-colon prefix matches. +[[ $hardcoded -gt 0 ]] && grep -rE 'Color\(0x|Color\.(Red|Blue|Green|Gray|LightGray|DarkGray|White|Black|Yellow|Magenta|Cyan)' \ + "$UI" --include=*.kt | grep -v "^$THEME/" | grep -vE "$NOT_A_COMMENT" \ + | cut -d: -f1 | sort -u | sed "s|$UI/| |" alpha=$(count '\.copy\(alpha') report 'colours derived with .copy(alpha =)' "$alpha" -1