Skip to content

Commit 5b4f619

Browse files
SIG-2 Address review round 4: M3 bridge slots, sheet modality, a11y, test fixes
Theme bridge (Theme.kt, Type.kt): - Map the M3 slots existing screens read but the bridge left at baseline: tertiary -> warn (signing chip), secondary -> good (signed/broadcast chips; was duplicating onSurface), errorContainer/onErrorContainer from bad, surfaceContainer* -> surfaceSunken/surface/surfaceElev (filled Card and AlertDialog backgrounds in material3 1.3). - outline was the 8%-alpha hairline: invisible as OutlinedTextField/ OutlinedButton borders and unreadable as the "deleted" chip text. Now textMute (opaque); outlineVariant takes the hairline for dividers. - Collapse the verbatim-duplicated dark/light mapping into one .copy() over the base scheme so the two modes can't drift. - Set explicit lineHeight on every Vault TextStyle (and on the fontSize- changing copies in vaultMaterialTypography): the M3 styles being replaced all carry explicit line heights, so leaving lineHeight unspecified tightened multi-line text on existing screens. BottomSheetOverlay: - Consume taps on the sheet container; clip/background don't block hit testing, so taps inside the sheet fell through to the scrim's clickable and dismissed the sheet (regression test added). - BackHandler(enabled = visible): system back now dismisses the sheet instead of firing the host screen's handler. - navigationBarsPadding + imePadding so the keyboard can't cover sheet content; paneTitle semantics; KDoc on the overlay-slot constraint and the remaining TalkBack limitation (no Popup window). Accessibility: - BackButton: 48dp touch target around the 36dp visual (foundation clickable gets no Material minimum-size enforcement); use material3 Icon instead of hand-rolled Image+tint. - Spinner: progressSemantics() so TalkBack announces the busy state (parity with the CircularProgressIndicator it replaces). - AppButton: focus now shows a visible border; with indication = null the press-scale was the only cue, leaving keyboard/D-pad focus invisible. Tests: - addr_truncatesLong expected "bc1qme…qmlv6" but Addr renders "bc1qme…lqmlv6" (takeLast(6) of the fixture is "lqmlv6"). - Disabled-button test asserted assertHasNoClickAction(), which fails on foundation 1.7.6 where applySemantics adds onClick unconditionally and only gates disabled() on enabled; assert the Disabled flag instead. Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>
1 parent f5a41fa commit 5b4f619

7 files changed

Lines changed: 133 additions & 48 deletions

File tree

app/src/androidTest/kotlin/com/remotesigner/ui/components/PrimitivesTest.kt

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import androidx.compose.runtime.mutableStateOf
77
import androidx.compose.runtime.remember
88
import androidx.compose.ui.Modifier
99
import androidx.compose.ui.platform.testTag
10-
import androidx.compose.ui.test.assertHasNoClickAction
1110
import androidx.compose.ui.test.assertIsDisplayed
1211
import androidx.compose.ui.test.assertIsNotEnabled
1312
import androidx.compose.ui.test.junit4.createComposeRule
@@ -72,17 +71,17 @@ class PrimitivesTest(private val darkTheme: Boolean) {
7271
}
7372

7473
@Test
75-
fun appButton_disabled_exposesNoClickAction() {
76-
// Compose's `clickable(enabled = false)` strips the OnClick semantics
77-
// action, so calling performClick() on the disabled node would fail
78-
// the test outright. Verify the disabled state instead.
74+
fun appButton_disabled_isNotEnabled() {
75+
// Foundation 1.7's clickable adds the OnClick semantics action even
76+
// when disabled (only the Disabled flag is gated on `enabled`), so
77+
// asserting the absence of a click action would fail. The disabled
78+
// flag is the reliable contract.
7979
setThemedContent {
8080
AppButton(text = "Disabled", onClick = {}, enabled = false)
8181
}
8282
composeTestRule.onNodeWithText("Disabled")
8383
.assertIsDisplayed()
8484
.assertIsNotEnabled()
85-
.assertHasNoClickAction()
8685
}
8786

8887
@Test
@@ -131,7 +130,7 @@ class PrimitivesTest(private val darkTheme: Boolean) {
131130
setThemedContent {
132131
Addr(value = "bc1qmek5jz2m9l4k6s7yqfdjl2mxv3lqmlv6", head = 6, tail = 6)
133132
}
134-
composeTestRule.onNodeWithText("bc1qme…qmlv6", substring = true).assertIsDisplayed()
133+
composeTestRule.onNodeWithText("bc1qme…lqmlv6", substring = true).assertIsDisplayed()
135134
}
136135

137136
@Test
@@ -170,6 +169,29 @@ class PrimitivesTest(private val darkTheme: Boolean) {
170169
assertEquals(1, dismissed)
171170
}
172171

172+
@Test
173+
fun bottomSheet_tapOnSheetBody_doesNotDismiss() {
174+
// Regression: the sheet container must consume taps — without it,
175+
// taps inside the sheet fall through to the scrim and dismiss.
176+
var dismissed = 0
177+
setThemedContent {
178+
Box(Modifier.fillMaxSize()) {
179+
BottomSheetOverlay(
180+
visible = true,
181+
onDismiss = { dismissed++ },
182+
) {
183+
androidx.compose.material3.Text(
184+
"Sheet body",
185+
modifier = Modifier.testTag("body"),
186+
)
187+
}
188+
}
189+
}
190+
composeTestRule.onNodeWithTag("body").performClick()
191+
composeTestRule.waitForIdle()
192+
assertEquals(0, dismissed)
193+
}
194+
173195
@Test
174196
fun appLogo_renders() {
175197
setThemedContent {

app/src/main/kotlin/com/remotesigner/ui/components/AppButton.kt

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import androidx.compose.foundation.background
66
import androidx.compose.foundation.border
77
import androidx.compose.foundation.clickable
88
import androidx.compose.foundation.interaction.MutableInteractionSource
9+
import androidx.compose.foundation.interaction.collectIsFocusedAsState
910
import androidx.compose.foundation.interaction.collectIsPressedAsState
1011
import androidx.compose.foundation.layout.Arrangement
1112
import androidx.compose.foundation.layout.Box
@@ -49,6 +50,7 @@ fun AppButton(
4950
val height = if (small) 40.dp else 52.dp
5051
val interactionSource = remember { MutableInteractionSource() }
5152
val isPressed by interactionSource.collectIsPressedAsState()
53+
val isFocused by interactionSource.collectIsFocusedAsState()
5254
val scale by animateFloatAsState(if (isPressed && enabled) 0.98f else 1f, label = "btnScale")
5355

5456
val (bg, fg, border) = when (variant) {
@@ -57,6 +59,10 @@ fun AppButton(
5759
AppButtonVariant.Ghost -> Triple(Color.Transparent, colors.text, null)
5860
AppButtonVariant.Danger -> Triple(Color.Transparent, colors.bad, BorderStroke(1.dp, colors.bad))
5961
}
62+
// `indication = null` suppresses the ripple, so keyboard/D-pad focus needs
63+
// its own visual cue: a text-tinted border readable on every variant bg.
64+
val effectiveBorder =
65+
if (isFocused) BorderStroke(2.dp, colors.text.copy(alpha = 0.6f)) else border
6066

6167
Box(
6268
modifier = modifier
@@ -65,7 +71,7 @@ fun AppButton(
6571
.graphicsLayer { scaleX = scale; scaleY = scale }
6672
.alpha(if (enabled) 1f else 0.45f)
6773
.clip(shapes.pill)
68-
.let { if (border != null) it.border(border, shapes.pill) else it }
74+
.let { if (effectiveBorder != null) it.border(effectiveBorder, shapes.pill) else it }
6975
.background(bg)
7076
.clickable(
7177
interactionSource = interactionSource,

app/src/main/kotlin/com/remotesigner/ui/components/BottomSheet.kt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
11
package com.remotesigner.ui.components
22

3+
import androidx.activity.compose.BackHandler
34
import androidx.compose.animation.AnimatedVisibility
45
import androidx.compose.animation.fadeIn
56
import androidx.compose.animation.fadeOut
67
import androidx.compose.animation.slideInVertically
78
import androidx.compose.animation.slideOutVertically
89
import androidx.compose.foundation.background
910
import androidx.compose.foundation.clickable
11+
import androidx.compose.foundation.gestures.detectTapGestures
1012
import androidx.compose.foundation.interaction.MutableInteractionSource
1113
import androidx.compose.foundation.layout.Box
1214
import androidx.compose.foundation.layout.fillMaxSize
1315
import androidx.compose.foundation.layout.fillMaxWidth
16+
import androidx.compose.foundation.layout.imePadding
17+
import androidx.compose.foundation.layout.navigationBarsPadding
1418
import androidx.compose.foundation.layout.padding
1519
import androidx.compose.foundation.shape.RoundedCornerShape
1620
import androidx.compose.runtime.Composable
@@ -19,8 +23,11 @@ import androidx.compose.ui.Alignment
1923
import androidx.compose.ui.Modifier
2024
import androidx.compose.ui.draw.clip
2125
import androidx.compose.ui.graphics.Color
26+
import androidx.compose.ui.input.pointer.pointerInput
2227
import androidx.compose.ui.platform.testTag
2328
import androidx.compose.ui.semantics.Role
29+
import androidx.compose.ui.semantics.paneTitle
30+
import androidx.compose.ui.semantics.semantics
2431
import androidx.compose.ui.unit.dp
2532
import com.remotesigner.ui.motion.tweenFade
2633
import com.remotesigner.ui.motion.tweenSlideUp
@@ -29,6 +36,18 @@ import com.remotesigner.ui.theme.LocalVaultShapes
2936

3037
const val BOTTOM_SHEET_SCRIM_TAG = "bottomSheetScrim"
3138

39+
/**
40+
* Vault-styled modal sheet. Place it as the last child of a full-screen `Box`
41+
* overlay slot — the overlay measures at full size even while hidden, so it is
42+
* not meant to sit inside a `Column`/`Row`.
43+
*
44+
* Dismissal: scrim tap and system back both call [onDismiss]. The sheet body
45+
* consumes taps so touches inside it never reach the scrim.
46+
*
47+
* Known limitation: the overlay lives in the host window (no Popup), so
48+
* TalkBack can still traverse content behind the open sheet. Full modality
49+
* needs a Popup/Dialog rewrite, which requires on-device validation.
50+
*/
3251
@Composable
3352
fun BottomSheetOverlay(
3453
visible: Boolean,
@@ -39,6 +58,7 @@ fun BottomSheetOverlay(
3958
val colors = LocalVaultColors.current
4059
val shapes = LocalVaultShapes.current
4160
val noIndication = remember { MutableInteractionSource() }
61+
BackHandler(enabled = visible, onBack = onDismiss)
4262
Box(modifier = modifier.fillMaxSize()) {
4363
AnimatedVisibility(
4464
visible = visible,
@@ -70,6 +90,13 @@ fun BottomSheetOverlay(
7090
.fillMaxWidth()
7191
.clip(RoundedCornerShape(topStart = shapes.radiusCard, topEnd = shapes.radiusCard))
7292
.background(colors.surfaceElev)
93+
// Consume taps: clip/background don't block hit-testing, so
94+
// without this, taps on the sheet body would fall through to
95+
// the scrim's clickable and dismiss the sheet.
96+
.pointerInput(Unit) { detectTapGestures { } }
97+
.semantics { paneTitle = "Sheet" }
98+
.navigationBarsPadding()
99+
.imePadding()
73100
.padding(20.dp),
74101
) {
75102
content()

app/src/main/kotlin/com/remotesigner/ui/components/ScreenHeader.kt

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@ import androidx.compose.foundation.layout.Row
99
import androidx.compose.foundation.layout.fillMaxWidth
1010
import androidx.compose.foundation.layout.padding
1111
import androidx.compose.foundation.layout.size
12+
import androidx.compose.material3.Icon
1213
import androidx.compose.material3.Text
1314
import androidx.compose.runtime.Composable
1415
import androidx.compose.ui.Alignment
1516
import androidx.compose.ui.Modifier
1617
import androidx.compose.ui.draw.clip
17-
import androidx.compose.ui.graphics.vector.rememberVectorPainter
1818
import androidx.compose.ui.semantics.Role
1919
import androidx.compose.ui.unit.dp
2020
import com.remotesigner.ui.icons.AppIcons
@@ -55,20 +55,29 @@ fun ScreenHeader(
5555
private fun BackButton(onClick: () -> Unit) {
5656
val colors = LocalVaultColors.current
5757
val shapes = LocalVaultShapes.current
58-
val painter = rememberVectorPainter(image = AppIcons.ChevronLeft)
58+
// 48dp interactive area (accessibility minimum touch target) around the
59+
// 36dp visual; plain foundation clickable gets no Material minimum-size
60+
// enforcement, so the touch target must be sized explicitly.
5961
Box(
6062
modifier = Modifier
61-
.size(36.dp)
63+
.size(48.dp)
6264
.clip(shapes.small)
63-
.border(1.dp, colors.line, shapes.small)
6465
.clickable(role = Role.Button, onClick = onClick),
6566
contentAlignment = Alignment.Center,
6667
) {
67-
androidx.compose.foundation.Image(
68-
painter = painter,
69-
contentDescription = "Back",
70-
colorFilter = androidx.compose.ui.graphics.ColorFilter.tint(colors.text),
71-
modifier = Modifier.size(16.dp),
72-
)
68+
Box(
69+
modifier = Modifier
70+
.size(36.dp)
71+
.clip(shapes.small)
72+
.border(1.dp, colors.line, shapes.small),
73+
contentAlignment = Alignment.Center,
74+
) {
75+
Icon(
76+
imageVector = AppIcons.ChevronLeft,
77+
contentDescription = "Back",
78+
tint = colors.text,
79+
modifier = Modifier.size(16.dp),
80+
)
81+
}
7382
}
7483
}

app/src/main/kotlin/com/remotesigner/ui/components/Spinner.kt

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package com.remotesigner.ui.components
22

33
import androidx.compose.foundation.Canvas
44
import androidx.compose.foundation.layout.size
5+
import androidx.compose.foundation.progressSemantics
56
import androidx.compose.runtime.Composable
67
import androidx.compose.runtime.getValue
78
import androidx.compose.ui.Modifier
@@ -26,7 +27,9 @@ fun Spinner(
2627
val rotation by rememberSsSpin()
2728
val tint = color ?: colors.accent
2829
val stroke = strokeWidth
29-
Canvas(modifier = modifier.size(size)) {
30+
// progressSemantics exposes an indeterminate ProgressBarRangeInfo so
31+
// TalkBack announces the busy state (parity with CircularProgressIndicator).
32+
Canvas(modifier = modifier.size(size).progressSemantics()) {
3033
rotate(degrees = rotation) {
3134
val pad = stroke.toPx()
3235
val rect = Size(this.size.width - pad * 2, this.size.height - pad * 2)

app/src/main/kotlin/com/remotesigner/ui/theme/Theme.kt

Lines changed: 28 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,41 +7,46 @@ import androidx.compose.material3.lightColorScheme
77
import androidx.compose.runtime.Composable
88
import androidx.compose.runtime.CompositionLocalProvider
99

10-
private fun materialFromVault(v: VaultColors) = if (v.isDark) {
11-
darkColorScheme(
10+
// Bridge for the not-yet-migrated Material3 screens: every slot those screens
11+
// read must resolve to a Vault color, otherwise M3 baseline (purple-tinted)
12+
// defaults leak through. Single mapping over the base scheme so dark and light
13+
// can't drift apart.
14+
//
15+
// Slot notes:
16+
// - `secondary` colors the signed/broadcast inbox chips -> good (success).
17+
// - `tertiary` colors the in-progress "signing" chip -> warn.
18+
// - `outline` borders OutlinedTextField/OutlinedButton and is read as text by
19+
// the "deleted" chip, so it must be opaque -> textMute (v.line at 8% alpha
20+
// is invisible as a border and unreadable as text).
21+
// - `errorContainer` backs PSBT-warning and broadcast-failure cards -> bad
22+
// tinted like the Pill tones, with full-strength bad for the text on it.
23+
// - `surfaceContainer*` back filled Cards and AlertDialogs in material3 1.3.
24+
private fun materialFromVault(v: VaultColors) =
25+
(if (v.isDark) darkColorScheme() else lightColorScheme()).copy(
1226
primary = v.accent,
1327
onPrimary = v.accentInk,
14-
secondary = v.text,
28+
secondary = v.good,
1529
onSecondary = v.bg,
30+
tertiary = v.warn,
31+
onTertiary = v.bg,
1632
background = v.bg,
1733
onBackground = v.text,
1834
surface = v.surface,
1935
onSurface = v.text,
2036
surfaceVariant = v.surfaceElev,
2137
onSurfaceVariant = v.textDim,
22-
outline = v.line,
23-
outlineVariant = v.lineStrong,
38+
surfaceContainerLowest = v.surfaceSunken,
39+
surfaceContainerLow = v.surface,
40+
surfaceContainer = v.surface,
41+
surfaceContainerHigh = v.surfaceElev,
42+
surfaceContainerHighest = v.surfaceElev,
43+
outline = v.textMute,
44+
outlineVariant = v.line,
2445
error = v.bad,
2546
onError = v.bg,
47+
errorContainer = v.bad.copy(alpha = 0.12f),
48+
onErrorContainer = v.bad,
2649
)
27-
} else {
28-
lightColorScheme(
29-
primary = v.accent,
30-
onPrimary = v.accentInk,
31-
secondary = v.text,
32-
onSecondary = v.bg,
33-
background = v.bg,
34-
onBackground = v.text,
35-
surface = v.surface,
36-
onSurface = v.text,
37-
surfaceVariant = v.surfaceElev,
38-
onSurfaceVariant = v.textDim,
39-
outline = v.line,
40-
outlineVariant = v.lineStrong,
41-
error = v.bad,
42-
onError = v.bg,
43-
)
44-
}
4550

4651
@Composable
4752
fun SatoshiSignerTheme(

0 commit comments

Comments
 (0)