From 0c5f06da0101a770d771f6e3dc4de504f7a6c6ca Mon Sep 17 00:00:00 2001 From: GenericJam Date: Thu, 27 Aug 2026 15:13:09 -0600 Subject: [PATCH 1/2] feat(android): render intrinsic sheets and box semantics --- .../app/src/main/java/MobBridge.kt.eex | 72 ++++++++++++++++--- test/mob_new/project_generator_test.exs | 6 ++ .../android_box_accessibility_test.exs | 28 ++++++++ .../templates/android_content_sheet_test.exs | 27 +++++++ 4 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 test/mob_new/templates/android_box_accessibility_test.exs create mode 100644 test/mob_new/templates/android_content_sheet_test.exs diff --git a/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex b/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex index b6f1ba3..0e50713 100644 --- a/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex +++ b/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex @@ -81,6 +81,11 @@ import androidx.compose.foundation.verticalScroll import androidx.compose.ui.layout.onGloballyPositioned import androidx.compose.ui.layout.boundsInWindow import androidx.compose.ui.platform.testTag +import androidx.compose.ui.semantics.Role +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.disabled +import androidx.compose.ui.semantics.role +import androidx.compose.ui.semantics.semantics import android.view.PixelCopy import android.view.WindowManager import kotlinx.coroutines.CoroutineScope @@ -2229,9 +2234,20 @@ private fun RenderNodeInner(node: MobNode, modifier: Modifier) { // button installs its own onClick via the Button composable. Mirrors iOS, // where most node types pick up onTapGesture via .ifLet(node.onTap). val tapHandle = intProp(node.props, "on_tap") - val tapModifier = if (tapHandle != null && node.type != "button") { - modifier.clickable { MobBridge.nativeSendTap(tapHandle) } - } else modifier + val isDisabled = boolProp(node.props, "disabled") ?: false + val accessibilityRole = node.props["accessibility_role"] as? String + val isButton = node.type == "box" && accessibilityRole == "button" + val tapModifier = when { + isButton -> + modifier.clickable(enabled = !isDisabled && tapHandle != null, role = Role.Button) { + if (tapHandle != null) MobBridge.nativeSendTap(tapHandle) + } + + tapHandle != null && node.type != "button" -> + modifier.clickable { MobBridge.nativeSendTap(tapHandle) } + + else -> modifier + } val base = tapModifier.then(nodeModifier(node.props)) // Track on-screen frame + set a testTag for any node carrying an :id, so the // agent can read positions (Mob.Test.element_frames) without a screenshot. @@ -2257,7 +2273,16 @@ private fun RenderNodeInner(node: MobNode, modifier: Modifier) { // "top_leading" / etc.) — defaults to TopStart for back-compat. "box" -> { val hasWidth = floatProp(node.props, "width") != null - val boxModifier = if (hasWidth) m else m.fillMaxWidth() + val accessibilityLabel = node.props["accessibility_label"] as? String + val accessibilityModifier = Modifier.semantics( + mergeDescendants = accessibilityLabel != null, + ) { + if (accessibilityLabel != null) contentDescription = accessibilityLabel + if (isButton) role = Role.Button + if (isDisabled) disabled() + } + val boxModifier = (if (hasWidth) m else m.fillMaxWidth()) + .then(accessibilityModifier) Box(modifier = boxModifier, contentAlignment = boxAlignProp(node.props)) { node.children.forEach { RenderNode(it) } } @@ -3115,9 +3140,15 @@ private fun MobGpuView(node: MobNode, modifier: Modifier) { @OptIn(ExperimentalMaterial3Api::class) @Composable private fun MobSheet(node: MobNode) { - val detents = detentsProp(node.props) + val rawDetents = sheetDetentsProp(node.props) + val contentDetent = rawDetents.filterIsInstance() + .firstOrNull { detent -> detent.optString("type") == "content" } + val detents = rawDetents.filterIsInstance() + .filter { detent -> detent == "medium" || detent == "large" } + .ifEmpty { if (contentDetent == null) listOf("medium", "large") else emptyList() } + val contentOnly = contentDetent != null val allowsMedium = "medium" in detents - val allowsLarge = "large" in detents + val allowsLarge = "large" in detents || contentOnly val mediumOnly = allowsMedium && !allowsLarge var visible by remember { mutableStateOf(true) } @@ -3205,13 +3236,34 @@ private fun MobSheet(node: MobNode) { // half the measured viewport height in that case only — full // medium+large sheets size to their natural content height as usual. BoxWithConstraints(modifier = Modifier.fillMaxWidth()) { + val configuredMaximumHeight = contentDetent + ?.takeIf { detent -> detent.has("max_height") } + ?.optDouble("max_height") + ?.toFloat() + ?.dp + val contentMaximumHeight = configuredMaximumHeight + ?.let(maxHeight::coerceAtMost) + ?: maxHeight val mediumDetentModifier = if (mediumOnly) { Modifier.heightIn(min = maxHeight * 0.5f + 1.dp) } else { Modifier } - Column(modifier = mediumDetentModifier.then(contentModifier).fillMaxWidth()) { + val contentDetentModifier = if (contentOnly) { + Modifier + .heightIn(max = contentMaximumHeight) + .verticalScroll(rememberScrollState()) + } else { + Modifier + } + + Column( + modifier = mediumDetentModifier + .then(contentModifier) + .then(contentDetentModifier) + .fillMaxWidth() + ) { node.children.forEach { RenderNode(it) } } } @@ -3746,10 +3798,10 @@ private fun tabDefsProp(props: Map): List> { // pattern). The `is List<*>` branch stays as a fallback purely so a // directly-constructed MobNode (e.g. an instrumentation test building props // by hand instead of through JSON parsing) still works. -private fun detentsProp(props: Map): List = +private fun sheetDetentsProp(props: Map): List = when (val raw = props["detents"]) { - is JSONArray -> (0 until raw.length()).map { raw.getString(it) } - is List<*> -> raw.map { it.toString() } + is JSONArray -> (0 until raw.length()).map { raw.get(it) } + is List<*> -> raw else -> listOf("medium", "large") } diff --git a/test/mob_new/project_generator_test.exs b/test/mob_new/project_generator_test.exs index 7b2dbaa..6942616 100644 --- a/test/mob_new/project_generator_test.exs +++ b/test/mob_new/project_generator_test.exs @@ -787,6 +787,12 @@ defmodule MobNew.ProjectGeneratorTest do assert content =~ "BoxWithConstraints(modifier = Modifier.fillMaxWidth())" assert content =~ "maxHeight * 0.5f + 1.dp" + # Content detents wrap short bodies, cap at either the configured + # maximum or viewport, then scroll overflow internally. + assert content =~ ~S|detent.optString("type") == "content"| + assert content =~ "heightIn(max = contentMaximumHeight)" + assert content =~ "verticalScroll(rememberScrollState())" + # Sheet-owned background/corner_radius must not double-apply onto the # child content modifier (see the lint check for the structural guard). assert content =~ ~s|node.props - listOf("background", "corner_radius")| diff --git a/test/mob_new/templates/android_box_accessibility_test.exs b/test/mob_new/templates/android_box_accessibility_test.exs new file mode 100644 index 0000000..f93121c --- /dev/null +++ b/test/mob_new/templates/android_box_accessibility_test.exs @@ -0,0 +1,28 @@ +defmodule MobNew.Templates.AndroidBoxAccessibilityTest do + use ExUnit.Case, async: true + + @bridge Path.expand( + "../../../priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex", + __DIR__ + ) + + test "generated boxes expose labels, action roles, and disabled semantics" do + source = File.read!(@bridge) + + assert source =~ "val isButton = node.type == \"box\" && accessibilityRole == \"button\"" + assert source =~ "clickable(enabled = !isDisabled && tapHandle != null, role = Role.Button)" + assert source =~ "if (tapHandle != null) MobBridge.nativeSendTap(tapHandle)" + assert source =~ "val accessibilityModifier = Modifier.semantics(" + assert source =~ "mergeDescendants = accessibilityLabel != null" + assert source =~ "contentDescription = accessibilityLabel" + assert source =~ "if (isButton) role = Role.Button" + assert source =~ "if (isDisabled) disabled()" + end + + test "only boxes with an explicit button role receive button semantics" do + source = File.read!(@bridge) + + assert source =~ "if (isButton)" + assert source =~ "tapHandle != null && node.type != \"button\"" + end +end diff --git a/test/mob_new/templates/android_content_sheet_test.exs b/test/mob_new/templates/android_content_sheet_test.exs new file mode 100644 index 0000000..858b4d2 --- /dev/null +++ b/test/mob_new/templates/android_content_sheet_test.exs @@ -0,0 +1,27 @@ +defmodule MobNew.Templates.AndroidContentSheetTest do + use ExUnit.Case, async: true + + @bridge Path.expand( + "../../../priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex", + __DIR__ + ) + + test "Compose sheet preserves built-ins and recognizes encoded content detents" do + source = File.read!(@bridge) + + assert source =~ ~S|detent.optString("type") == "content"| + assert source =~ "val contentOnly = contentDetent != null" + assert source =~ "skipPartiallyExpanded = !allowsMedium" + assert source =~ "allowsLarge || value != SheetValue.Expanded" + end + + test "content sheet wraps naturally, caps height, and scrolls overflow" do + source = File.read!(@bridge) + + assert source =~ ~S|detent.has("max_height")| + assert source =~ ~S|?.optDouble("max_height")| + assert source =~ "let(maxHeight::coerceAtMost)" + assert source =~ "heightIn(max = contentMaximumHeight)" + assert source =~ "verticalScroll(rememberScrollState())" + end +end From 7c646b7f1307f0573b26d79cd33fb6550b554163 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Thu, 27 Aug 2026 19:58:44 -0600 Subject: [PATCH 2/2] Address adversarial review of Android intrinsic sheets and box semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from an adversarial pass, one of them a hard crash. All verified on a physical Moto G power (2021), including a negative control. 1. CRASH — an intrinsic sheet containing a scrollable child killed the app. The content-detent path wrapped the sheet body in verticalScroll unconditionally. Compose's checkScrollableContainerConstraints THROWS when a scrollable is measured with an infinite max height, so a `scroll` or `lazy_list` inside `detents: [:content]` — the most natural use of an intrinsic sheet — died at first measure with FATAL EXCEPTION: java.lang.IllegalStateException: Vertically scrollable component was measured with an infinity maximum height constraints iOS survives the equivalent nesting because SwiftUI tolerates it; Compose does not. The sheet now only adds its own scroll when the content has none (isScrollableNode walks the subtree); the height cap applies either way and the child keeps owning its scrolling. Proven, not assumed: with the guard disabled the app dies with that exception; with it, the same screen reports 40 rows and stays alive. 2. A disabled box still dispatched taps unless it also carried accessibility_role: :button. The non-button `when` arm had no `enabled` check while the semantics block below emitted disabled() — TalkBack announced it as disabled and it kept firing. iOS disables every box regardless of role, so this was also a platform split. 3. A box declared `accessibility_role: :button` whose tap is handled by an ancestor was announced as "…, button, disabled". Compose publishes disabled() semantics whenever clickable's `enabled` is false, so encoding "no handler" as enabled = false mislabelled a live control. The branch now requires a handler and falls through to semantics-only otherwise. 4. The height cap sat outside the node's own padding, so a padded sheet overshot max_height by the padding amount and disagreed with iOS, which caps the already-padded body. Modifier order corrected. Also: merge descendants on label OR button role, matching the iOS fix — role without merging leaves children as separate nodes inside a "button". Device results (Moto G power): short content hugs (3 rows), max_height 320 caps and scrolls internally (40 rows), scrollable child renders and survives (40 rows), app alive across all cases. 340 tests, format, credo --strict, ktlint generate-then-lint, and a real Gradle build all clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/src/main/java/MobBridge.kt.eex | 69 +++++++++++++++---- .../android_box_accessibility_test.exs | 20 +++++- .../templates/android_content_sheet_test.exs | 18 +++++ 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex b/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex index 0e50713..c5bac31 100644 --- a/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex +++ b/priv/templates/mob.new/android/app/src/main/java/MobBridge.kt.eex @@ -2238,13 +2238,25 @@ private fun RenderNodeInner(node: MobNode, modifier: Modifier) { val accessibilityRole = node.props["accessibility_role"] as? String val isButton = node.type == "box" && accessibilityRole == "button" val tapModifier = when { - isButton -> - modifier.clickable(enabled = !isDisabled && tapHandle != null, role = Role.Button) { - if (tapHandle != null) MobBridge.nativeSendTap(tapHandle) - } - + // Require a handler here. Compose's ClickableSemanticsNode publishes + // disabled() whenever `enabled` is false, so encoding "no tap handler" + // as enabled = false made a perfectly live box whose tap is handled by + // an ancestor announce as "…, button, disabled". With no handler we + // fall through to the semantics-only path below, which still sets the + // button role. + isButton && tapHandle != null -> + modifier.clickable(enabled = !isDisabled, role = Role.Button) { + MobBridge.nativeSendTap(tapHandle) + } + + // `enabled = !isDisabled` here too, not just on the button arm above. + // Without it a box with `disabled: true` and no explicit + // accessibility_role still dispatched taps, while the semantics block + // below simultaneously marked it disabled() — announced as disabled to + // TalkBack and still firing. iOS applies .disabled() to every box + // regardless of role, so this also keeps the platforms in step. tapHandle != null && node.type != "button" -> - modifier.clickable { MobBridge.nativeSendTap(tapHandle) } + modifier.clickable(enabled = !isDisabled) { MobBridge.nativeSendTap(tapHandle) } else -> modifier } @@ -2274,8 +2286,13 @@ private fun RenderNodeInner(node: MobNode, modifier: Modifier) { "box" -> { val hasWidth = floatProp(node.props, "width") != null val accessibilityLabel = node.props["accessibility_label"] as? String + // Merge for a label OR an explicit button role. Setting + // role/disabled without merging leaves them on the container while + // each child stays its own node, so TalkBack walks into a + // "button" and reads its children as separate elements. Matches + // the iOS side, which collapses on the same condition. val accessibilityModifier = Modifier.semantics( - mergeDescendants = accessibilityLabel != null, + mergeDescendants = accessibilityLabel != null || isButton, ) { if (accessibilityLabel != null) contentDescription = accessibilityLabel if (isButton) role = Role.Button @@ -3137,6 +3154,12 @@ private fun MobGpuView(node: MobNode, modifier: Modifier) { // container paint the same way M3's Button does, and can't take those // via a modifier chain). Content-area props are read directly off // `node.props` with those two keys stripped instead. +// Node types that install their own scrollable container. A sheet must not +// wrap these in another vertical scroll: Compose throws on a scrollable +// measured with infinite max height. +private fun isScrollableNode(node: MobNode): Boolean = + node.type == "scroll" || node.type == "lazy_list" || node.children.any(::isScrollableNode) + @OptIn(ExperimentalMaterial3Api::class) @Composable private fun MobSheet(node: MobNode) { @@ -3250,18 +3273,36 @@ private fun MobSheet(node: MobNode) { Modifier } - val contentDetentModifier = if (contentOnly) { - Modifier - .heightIn(max = contentMaximumHeight) - .verticalScroll(rememberScrollState()) - } else { - Modifier + // Only add our own scroll when the content doesn't already + // contain one. Compose's checkScrollableContainerConstraints + // THROWS when a scrollable is measured with an infinite max + // height, which is exactly what wrapping verticalScroll around a + // `scroll` or `lazy_list` child does — so an intrinsic sheet + // containing a list (the most natural use of one) crashed at first + // measure. iOS survives the equivalent nesting because SwiftUI + // tolerates it; Compose does not. Cap the height either way; let + // the child own the scrolling when it has its own. + val hasScrollableChild = node.children.any(::isScrollableNode) + val contentDetentModifier = when { + contentOnly && hasScrollableChild -> + Modifier.heightIn(max = contentMaximumHeight) + + contentOnly -> + Modifier + .heightIn(max = contentMaximumHeight) + .verticalScroll(rememberScrollState()) + + else -> Modifier } Column( + // Cap BEFORE the node's own padding, so padding counts against + // max_height instead of being added outside it — otherwise a + // sheet with padding overshoots the documented cap, and + // disagrees with iOS, which caps the already-padded body. modifier = mediumDetentModifier - .then(contentModifier) .then(contentDetentModifier) + .then(contentModifier) .fillMaxWidth() ) { node.children.forEach { RenderNode(it) } diff --git a/test/mob_new/templates/android_box_accessibility_test.exs b/test/mob_new/templates/android_box_accessibility_test.exs index f93121c..c336842 100644 --- a/test/mob_new/templates/android_box_accessibility_test.exs +++ b/test/mob_new/templates/android_box_accessibility_test.exs @@ -10,13 +10,29 @@ defmodule MobNew.Templates.AndroidBoxAccessibilityTest do source = File.read!(@bridge) assert source =~ "val isButton = node.type == \"box\" && accessibilityRole == \"button\"" - assert source =~ "clickable(enabled = !isDisabled && tapHandle != null, role = Role.Button)" - assert source =~ "if (tapHandle != null) MobBridge.nativeSendTap(tapHandle)" + assert source =~ "isButton && tapHandle != null ->" + assert source =~ "clickable(enabled = !isDisabled, role = Role.Button)" + + # Compose publishes disabled() semantics whenever clickable's `enabled` is + # false, so "no handler" must not be encoded as enabled = false — that + # announced a live box as disabled. + refute source =~ "enabled = !isDisabled && tapHandle != null" assert source =~ "val accessibilityModifier = Modifier.semantics(" assert source =~ "mergeDescendants = accessibilityLabel != null" assert source =~ "contentDescription = accessibilityLabel" assert source =~ "if (isButton) role = Role.Button" assert source =~ "if (isDisabled) disabled()" + + # A disabled box must not dispatch regardless of whether it carries an + # explicit button role. The non-button arm originally had no enabled + # check, so `disabled: true` without a role announced as disabled to + # TalkBack and still fired. + assert source =~ + "modifier.clickable(enabled = !isDisabled) { MobBridge.nativeSendTap(tapHandle) }" + + # Merge on label OR button role, matching iOS. Role without merging leaves + # children as separate accessibility nodes inside a "button". + assert source =~ "mergeDescendants = accessibilityLabel != null || isButton" end test "only boxes with an explicit button role receive button semantics" do diff --git a/test/mob_new/templates/android_content_sheet_test.exs b/test/mob_new/templates/android_content_sheet_test.exs index 858b4d2..9ab23ab 100644 --- a/test/mob_new/templates/android_content_sheet_test.exs +++ b/test/mob_new/templates/android_content_sheet_test.exs @@ -24,4 +24,22 @@ defmodule MobNew.Templates.AndroidContentSheetTest do assert source =~ "heightIn(max = contentMaximumHeight)" assert source =~ "verticalScroll(rememberScrollState())" end + + test "a content sheet does not nest its own scroll around a scrollable child" do + source = File.read!(@bridge) + + # Compose THROWS when a scrollable is measured with an infinite max height, + # so wrapping verticalScroll around a `scroll`/`lazy_list` child crashed the + # app at first measure. The cap still applies; the child owns the scrolling. + assert source =~ "val hasScrollableChild = node.children.any(::isScrollableNode)" + assert source =~ "contentOnly && hasScrollableChild ->" + assert source =~ "node.type == \"scroll\"" + assert source =~ "node.type == \"lazy_list\"" + + # The height cap must sit inside the node's own padding, or padding is + # added outside max_height and overshoots the documented cap. + detent_at = :binary.match(source, ".then(contentDetentModifier)") |> elem(0) + padding_at = :binary.match(source, ".then(contentModifier)") |> elem(0) + assert detent_at < padding_at, "height cap must be applied before the node's padding" + end end