From fbf376fe20a576130f452e5648933f5f9c5937fc Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Fri, 2 Oct 2026 20:51:30 +0200 Subject: [PATCH] feat(chat): the room list marks the room the pane is answering with The commit before this one made a second tap change the transcript. It did not make the list say which room the transcript belongs to, and on a tablet those are two halves of one question: a reader choosing their next room is looking at the list, not at the app bar on the other side of the window that repeats the title. With nothing marked, the only evidence of a selection was in the half of the screen they had looked away from -- which is its own reason the pane read as unresponsive even once it worked. **`secondaryContainer`, which is M3's container for a selected list item**, set through `CardDefaults.cardColors` so the content colour comes with it: `cardColors`' `contentColor` defaults to `contentColorFor(containerColor)`, so the row's text moves to `onSecondaryContainer` without a second call site to keep in step. A row's title keeps its `ProfileColor`, which is a fixed-lightness HSL colour and already scheme-blind, so the swap does not change what that line is contrasted against in either scheme -- a pre-existing question this commit neither fixes nor worsens. **`isSelected` is a predicate, and nullable, because the two layouts disagree about whether selection exists at all** -- not merely about its value. Where the list is the whole screen, tapping a room navigates to it and the list is about to be gone: no room is "the selected one", and both a highlight left behind on the way out and a reader told "not selected" about every room would be describing a pane that is not on the screen. `null` says that. A predicate answering false for every room does not, because that is also exactly what a two-pane layout looks like before the first tap, and those two states are different things for the list to be saying. **The call site reads the layout rather than the selection**, `listPaneWidth == null` rather than `selectedChatRoomId == null`. The selection is `rememberSaveable` and outlives a window that narrows, so a tablet turned from landscape to portrait keeps it -- and deriving the mark from the selection would leave a highlighted row on a one-pane screen with no pane beside it. **The semantics property is set only where it means something.** A row in a selection-bearing list carries `selected`, true or false, so a screen reader can say which of them the pane is showing; a row in a list that navigates carries nothing, rather than announcing "not selected" about a pane that does not exist. `Modifier.then` of an empty `Modifier` is how the row opts out, which keeps the branch at the one place that knows. **Tests: 2 cases, both checked against the feature removed.** The first taps two rooms in a 1000dp window and asserts the mark lands on the right card, moves rather than accumulating, and is absent before the first tap while the rows are already selectable -- "asked nothing yet" and "not the one being shown" being the two states the nullable predicate exists to tell apart. The second asserts a 700dp list has no selectable row at all, in two halves on purpose: a list that never rendered also has no selectable rows, so it checks the room is on screen before checking it is not selectable. Both first drafts counted `isSelectable()` across the whole window and failed by exactly one, because `PrimaryScrollableTabRow`'s `Tab` is selectable too. The assertions are scoped to the rows through a matcher that requires both selectability and the room's subject -- which works because `Card(onClick = ...)` merges its descendants, so one node carries the subject and the selected state together. That merge is also why a selected room is two nodes saying its subject and a merely listed one is a single node, which is what the previous commit's case counts. **Not done.** Still nobody has looked at this on a tablet. The tests prove the mark follows the selection and that a one-pane list stays silent; they say nothing about whether the colour reads as a selection at arm's length on a real screen. 976 jvm tests, 454 common tests, `m3Audit` meeting every budget. Co-Authored-By: Claude Opus 5 --- .../compose/ui/composable/HomeScreen.kt | 13 +++- .../ui/view/model/ChatRoomListViewModel.kt | 42 +++++++++++- .../ui/composable/ChatPaneLayoutJvmTest.kt | 66 ++++++++++++++++++- 3 files changed, 115 insertions(+), 6 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt index 6f525349..638c6497 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt @@ -243,7 +243,18 @@ fun HomeScreen( key(true) { chatRoomListViewModel.RenderFeed( - onNavigateToDirectMessageDetail = onOpenChatRoom + onNavigateToDirectMessageDetail = onOpenChatRoom, + // Only where there is a pane to be selected + // into. The selection outlives a window + // that narrows -- it is `rememberSaveable`, + // and a tablet turned to portrait keeps it + // -- so this is read from the layout rather + // than from whether a room is selected. + isSelected = if (listPaneWidth == null) { + null + } else { + { id -> id == selectedChatRoomId } + }, ) } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt index e3a988f9..3bd3ff61 100755 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomListViewModel.kt @@ -10,6 +10,7 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.items import androidx.compose.material3.Card +import androidx.compose.material3.CardDefaults import androidx.compose.material3.ExperimentalMaterial3ExpressiveApi import androidx.compose.material3.LoadingIndicator import androidx.compose.material3.MaterialTheme @@ -20,6 +21,8 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.semantics.selected +import androidx.compose.ui.semantics.semantics import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp import androidx.lifecycle.ViewModel @@ -144,7 +147,20 @@ class ChatRoomListViewModel( @OptIn(ExperimentalMaterial3ExpressiveApi::class) @Composable fun RenderFeed( - onNavigateToDirectMessageDetail: (press.mantra.compose.ui.composable.navigation.routes.Route) -> Unit + onNavigateToDirectMessageDetail: (press.mantra.compose.ui.composable.navigation.routes.Route) -> Unit, + /** + * Which room the detail pane beside this list is showing, or `null` where this + * list is the whole screen. + * + * A predicate rather than an id because the two layouts disagree about whether + * selection is a concept at all, not merely about its value. Where tapping a room + * navigates to it, no room is "the selected one" -- the list is about to be gone -- + * and a highlight left behind on the way out, or a reader told "not selected" + * about every room in the list, would both be describing a pane that is not there. + * `null` says that; a predicate that answers false for every room would not, and + * is also what a two-pane layout looks like before the first tap. + */ + isSelected: ((String) -> Boolean)?, ) { Column( modifier = Modifier.fillMaxWidth(), @@ -174,8 +190,30 @@ class ChatRoomListViewModel( items = chatRoomListUIState.chatRoomList, key = { it.chatRoom.id } ) { localChatRoom -> + // Null where there is no pane, so the row says nothing + // about selection at all rather than saying "no". + val selected = isSelected?.invoke(localChatRoom.chatRoom.id) + Card( - modifier = Modifier.fillMaxWidth(), + modifier = Modifier + .fillMaxWidth() + .then( + if (selected == null) Modifier + else Modifier.semantics { this.selected = selected } + ), + // M3's container for a selected list item. The + // colour is the only signal a reader scanning the + // list gets about which room the pane is answering + // with -- the pane's own app bar repeats the title, + // but that is the half of the window they are not + // looking at while choosing the next room. + colors = if (selected == true) { + CardDefaults.cardColors( + containerColor = MaterialTheme.colorScheme.secondaryContainer + ) + } else { + CardDefaults.cardColors() + }, onClick = { onNavigateToDirectMessageDetail.invoke( press.mantra.compose.ui.composable.navigation.routes.ChatRoomMessagingRoute( diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt index 937a68aa..4d436864 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt @@ -2,7 +2,11 @@ package press.mantra.compose.ui.composable import androidx.compose.ui.test.ExperimentalTestApi import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertIsNotSelected +import androidx.compose.ui.test.assertIsSelected import androidx.compose.ui.test.assertWidthIsEqualTo +import androidx.compose.ui.test.hasText +import androidx.compose.ui.test.isSelectable import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onNodeWithText @@ -32,7 +36,7 @@ import kotlin.test.assertEquals /** * The chat list-detail split, measured in windows of the widths the phase names. * - * Four claims, and none of them can be read off the source: + * Five claims, and none of them can be read off the source: * * - below expanded the home screen is unchanged. That is the promise that let the split * land as one commit -- the chat room is a navigation destination reached from eleven @@ -42,9 +46,11 @@ import kotlin.test.assertEquals * a `ListDetailPaneScaffold` would; * - the detail pane says what it is for while nothing is selected, rather than being an * unexplained empty half of the window; - * - and a second tap in the list changes what the detail pane shows. That one is not a + * - a second tap in the list changes what the detail pane shows. That one is not a * layout claim at all -- it is about view model scope -- but it is only reachable - * through this layout, and it is the way the split visibly failed on a tablet. + * through this layout, and it is the way the split visibly failed on a tablet; + * - and the list says which room the pane is answering with, in the one layout where + * that question exists. * * The repositories are the no-op ones with the two reads this screen makes delegated to a * fixed answer. Kotlin's interface delegation makes that ten lines rather than a @@ -123,6 +129,49 @@ class ChatPaneLayoutJvmTest { assertEquals(1, nodesSaying(roomOne), "room one is listed, not shown") } + @Test + fun `the list marks the room the detail pane is showing`() = + runDesktopComposeUiTest(1000, 900) { + setContent { Home(chatRepository = TwoRooms) } + + // Selectable, and nothing selected. A pane that has been asked nothing yet is + // a different state from a room that is not the one being shown, and the list + // is allowed to say so. `card` only matches a selectable node, so finding both + // rooms through it is half of the claim. + card(roomOne).assertIsNotSelected() + card(roomTwo).assertIsNotSelected() + + card(roomOne).performClick() + waitUntil("room one opens", OpenTimeout) { nodesSaying(roomOne) == 2 } + card(roomOne).assertIsSelected() + card(roomTwo).assertIsNotSelected() + + card(roomTwo).performClick() + waitUntil("room two opens", OpenTimeout) { nodesSaying(roomTwo) == 2 } + card(roomTwo).assertIsSelected() + + // The mark moves rather than accumulating. Two highlighted rows beside one + // transcript would say the pane was showing both. + card(roomOne).assertIsNotSelected() + } + + @Test + fun `a one-pane list says nothing about selection, having nothing to select into`() = + runDesktopComposeUiTest(700, 900) { + setContent { Home(chatRepository = TwoRooms) } + + // Tapping navigates here, and the test's callback is a no-op, so the list is + // still on screen afterwards -- which is the only reason this can be asked at + // all. A row answering "not selected" would be describing a pane that is not + // there, and a highlight left behind would outlive the screen it belonged to. + onNodeWithText(roomOne).performClick() + + // Both halves, because either alone passes for the wrong reason: a list that + // never rendered has no selectable rows either. + onNodeWithText(roomOne).assertExists() + card(roomOne).assertDoesNotExist() + } + @androidx.compose.runtime.Composable private fun Home(chatRepository: ChatRepository = NoRooms) { MantraTheme { @@ -202,4 +251,15 @@ class ChatPaneLayoutJvmTest { private fun ComposeUiTest.nodesSaying(text: String) = onAllNodesWithText(text).fetchSemanticsNodes().size + + /** + * The list's card for the room with this subject. + * + * `Card(onClick = ...)` merges its descendants, so one node carries both the subject + * and the selected state -- which is also why a selected room is two nodes saying its + * subject and a merely listed one is a single node. The detail pane's app bar says the + * same subject and is not selectable, so this stays unambiguous once a room is open. + */ + private fun ComposeUiTest.card(subject: String) = + onNode(isSelectable() and hasText(subject)) }