From d36456537ac0b7b426268a49402697faff46bbd4 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Wed, 26 Aug 2026 18:21:48 -0600 Subject: [PATCH 1/2] Add native modal Sheet primitive (Mob.UI.sheet/2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New :sheet node type composing ordinary Mob nodes as content — iOS .sheet presentation, Android Material 3 ModalBottomSheet (mob_new, companion PR). Composed children, medium/large detents, exactly-once native dismissal, native background/top-radius/scrim/drag-indicator styling with per-platform overrides, strict color/dimension validation (explicit nil/bool rejection prevents the iOS NSNull.longLongValue crash class). Presentation state lives in view/composable identity (mirrors MobToggle/MobSlider), not a boolean prop — a rerender that still includes the sheet updates content without re-presenting; removing the node dismisses it. Documented iOS limitation: system-owned sheet scrim opacity is not configurable via public APIs; Android applies the requested scrim exactly. Device-verified on the iOS simulator: presentation, custom drag indicator, background/corner radius, exactly-once dismissal via outside-tap, and reopen-after-dismiss cycling. MOB-101 Co-Authored-By: Claude Sonnet 5 --- .../2026-08-26-native-sheet-primitive.md | 127 ++++++++ ios/MobNode.h | 18 ++ ios/MobNode.m | 5 +- ios/MobRootView.swift | 97 ++++++ ios/mob_nif.m | 37 +++ lib/mob/renderer.ex | 8 +- lib/mob/ui.ex | 234 ++++++++++++++ test/mob/renderer_test.exs | 128 ++++++++ test/mob/ui_test.exs | 304 ++++++++++++++++++ 9 files changed, 956 insertions(+), 2 deletions(-) create mode 100644 decisions/2026-08-26-native-sheet-primitive.md diff --git a/decisions/2026-08-26-native-sheet-primitive.md b/decisions/2026-08-26-native-sheet-primitive.md new file mode 100644 index 0000000..91bda10 --- /dev/null +++ b/decisions/2026-08-26-native-sheet-primitive.md @@ -0,0 +1,127 @@ +# Native Sheet primitive — presentation model, styling, exactly-once dismissal + +- Date: 2026-08-26 +- Status: accepted + +## Context + +Adding `Mob.UI.sheet/2` — a native modal bottom sheet (iOS `.sheet`, +Android Material 3 `ModalBottomSheet`) that composes ordinary Mob nodes +as content. Several parts of the design aren't obvious from the API +surface alone. + +## Decision + +### Presentation state lives in view/composable identity, not a boolean prop + +Neither `MobSheetView` (iOS) nor `MobSheet` (Android) take a +`presented:` boolean prop. Instead, each holds local presentation +state (`@State private var isPresented = true` / `remember { +mutableStateOf(true) }`) seeded once when the composable/view first +appears at its position in the tree. + +Both SwiftUI and Compose preserve that local state across re-renders +that keep dispatching to the same position (same case in a `switch`/ +`when`, even though the `MobNode` object itself is rebuilt fresh from +JSON every render) — the same mechanism `MobToggle`/`MobSlider` +already rely on for user-driven state against a BEAM-pushed node. This +gives two required behaviors for free, with no explicit prop needed: + +- **A rerender that still includes the sheet node updates its content + without dismissing/re-presenting** — same identity, state preserved. +- **Removing the sheet node from the tree dismisses it** — the + composable stops being called, its whole state (and the live + presentation) is torn down. + +### Exactly-once dismissal via a remembered flag, not just the platform callback + +Both platforms' native dismiss callback (`.sheet(onDismiss:)` / +`ModalBottomSheet(onDismissRequest:)`) is expected to fire once per +user-initiated dismissal, but a local `dismissSent` flag guards the +actual BEAM-facing send anyway. Cheap insurance against any platform- +specific double-invocation edge case, and it's the literal shape +requested for the Android side — keeping both platforms symmetric. + +### background/corner_radius are NOT threaded through the standard modifier pipeline + +Every other node type gets `background`/`corner_radius` baked into its +modifier by the generic pipeline (iOS: read directly onto +`node.backgroundColor`/`node.cornerRadius`, which every case already +does; Android: `nodeModifier(node.props)`, applied centrally in +`RenderNodeInner` before dispatch). For most types that's correct — a +plain `.background()`/`.clip()` is exactly what a `Box`/`Row`/etc. +wants. + +Sheet can't reuse that path. `ModalBottomSheet` (and SwiftUI's +`.presentationBackground`/`.presentationCornerRadius`) own their own +container paint and corner shape — the same reason Android's `Button` +already reads `background`/`corner_radius` directly instead of via +modifier (`ButtonDefaults.buttonColors`/`shape=`). Passing the +already-baked modifier straight into the sheet would double-apply: +once from the outer modifier's full-rect background/clip, once from +the sheet's own top-corners-only shape — visible as double-painted +background or clipped corners that don't match the requested radius. + +Fix, symmetric on both platforms: +- iOS: `MobSheetView` reads `node.backgroundColor`/`node.cornerRadius` + directly for `.presentationBackground`/`.presentationCornerRadius`; + the sheet's *content* (children) gets no separate modifier — SwiftUI + never had a "baked-in" modifier to begin with (see `MobNode.h`: every + node type reads these two properties generically, but nothing + upstream of `MobSheetView` applies them as a `.background()`/`.clip()` + the way Android's central `nodeModifier` does). +- Android: `MobSheet` receives the dispatch call *without* the + `m`-derived modifier (`MobSheet(node)`, not `MobSheet(node, m)`), and + builds its own content modifier from `node.props - listOf("background", + "corner_radius")` before passing it to `nodeModifier`. A structural + lint (`MobNew.Templates.Lint.sheet_content_modifier_not_double_applied/1` + in `mob_new`) guards both halves of this against regression, since + it's not something a compiler catches — code that double-applies + still compiles and runs, it just looks wrong. + +### iOS scrim opacity: documented limitation, not a bug to "fix" later + +Android applies the requested `:scrim` color (including alpha) exactly +via `ModalBottomSheet`'s `scrimColor` param. iOS's `.sheet` presentation +owns its dimming layer with no public API to configure its opacity — +supported SwiftUI APIs leave it system-black at a fixed alpha. The only +way to force exact opacity is private `UIViewController`/presentation- +controller hierarchy manipulation, which this framework does not do +(same policy as everywhere else native internals are touched only +through documented APIs). Documented in `Mob.UI.sheet/2`'s moduledoc +and in the mob_new CHANGELOG rather than tracked as an open bug. + +### Android medium-only short-content fix + +Material 3's `ModalBottomSheet` can omit the `PartiallyExpanded` anchor +entirely when content measures shorter than half the viewport. A +medium-only sheet (`detents: [:medium]`) rejects `Expanded` via +`confirmValueChange`, so a short-content medium-only sheet would have +no valid anchor to land on and stay hidden. Content is wrapped in +`BoxWithConstraints`; when `mediumOnly` is true, a `heightIn(min = +maxHeight * 0.5f + 1.dp)` forces just enough height for Material 3 to +compute a real anchor. Full medium+large sheets are untouched — this +only kicks in for the medium-only case, and never allows `Expanded` +(the fix is sizing, not a detent-contract workaround). + +Verified via a generated-project Android instrumentation test +(`MediumOnlySheetTest`, `mob_new`) — 1/1 pass on a real emulator with +deliberately short content and `detents == ["medium"]`. Note: a +negative-control check (temporarily disabling the fix) did not +reproduce a clean failure on the specific emulator/Compose-BOM +combination used for verification — the fix follows the requested +spec exactly and the positive case is real-device-confirmed, but the +without-fix failure mode wasn't independently reproduced in this +environment. Worth another look if this Material 3 behavior surfaces +again on a different device/BOM combination. + +## Consequences + +- Adding a `presented:`-style prop later (e.g., to let BEAM force-close + a sheet without a full tree diff) would need a real prop read in + addition to the identity-based default, not a replacement for it. +- The lint check only catches the *specific* double-application shape + (raw `m` threaded through, or an unstripped `node.props` build) — a + sufficiently different refactor of `MobSheet`'s modifier construction + could still double-apply without tripping it. It's a regression guard + for this exact class of mistake, not a general correctness proof. diff --git a/ios/MobNode.h b/ios/MobNode.h index a397e3e..13b7ecf 100644 --- a/ios/MobNode.h +++ b/ios/MobNode.h @@ -41,6 +41,7 @@ typedef NS_ENUM(NSInteger, MobNodeType) { MobNodeTypeIcon, MobNodeTypeCanvas, MobNodeTypeGpuView, + MobNodeTypeSheet, }; NS_ASSUME_NONNULL_BEGIN @@ -261,6 +262,23 @@ NS_ASSUME_NONNULL_BEGIN // expected packing semantics per element. @property(nonatomic, strong, nullable) id gpuUniforms; +// Sheet — native modal bottom sheet. `backgroundColor` and `cornerRadius` +// (declared above, shared by every node type) double as the sheet's own +// container background / top-corner radius, matching how every other prop +// name is shared across types. `sheetDetents` is the raw "medium"/"large" +// string list from Mob.Renderer — mapped to PresentationDetent by +// MobSheetView (see MobRootView.swift), not here, so this header stays +// framework-agnostic. Indicator geometry defaults to -1 (unset — use the +// system default indicator); Mob.UI.sheet's validation guarantees all four +// arrive together or not at all, so checking any one for >= 0 is enough to +// know whether a complete custom indicator was supplied. +@property(nonatomic, strong, nullable) NSArray *sheetDetents; +@property(nonatomic, strong, nullable) UIColor *dragIndicatorColor; +@property(nonatomic) CGFloat dragIndicatorWidth; +@property(nonatomic) CGFloat dragIndicatorHeight; +@property(nonatomic) CGFloat dragIndicatorRailHeight; +@property(nonatomic, copy, nullable) void (^onDismiss)(void); + // Children @property(nonatomic, strong, nonnull) NSMutableArray *children; diff --git a/ios/MobNode.m b/ios/MobNode.m index a5c70cb..95d9ea8 100644 --- a/ios/MobNode.m +++ b/ios/MobNode.m @@ -37,7 +37,10 @@ - (instancetype)init { _fixedHeight = 0.0; _fillWidth = NO; _cornerRadius = 0.0; - _nativeViewHandle = -1; // -1 = no native component slot assigned (MOB-100) + _nativeViewHandle = -1; // -1 = no native component slot assigned (MOB-100) + _dragIndicatorWidth = -1.0; // -1 = no custom indicator supplied — use system default + _dragIndicatorHeight = -1.0; + _dragIndicatorRailHeight = -1.0; _videoAutoplay = NO; _videoLoop = NO; _videoControls = YES; diff --git a/ios/MobRootView.swift b/ios/MobRootView.swift index 3c259f8..6e5e526 100644 --- a/ios/MobRootView.swift +++ b/ios/MobRootView.swift @@ -495,6 +495,9 @@ struct MobNodeView: View { .ifLet(node.fixedHeight > 0 ? node.fixedHeight : nil) { v, h in v.frame(height: CGFloat(h)) } .padding(node.paddingEdgeInsets) + case .sheet: + MobSheetView(node: node) + @unknown default: EmptyView() } @@ -1297,6 +1300,100 @@ private struct MobSlider: View { } } +// MobSheetView — native modal bottom sheet. Presentation is owned by this +// view's own @State, not by the transient MobNode the BEAM rebuilds fresh +// every render — SwiftUI preserves @State across re-renders that keep the +// same view identity (same tree position, same case in MobNodeView's +// switch), exactly like MobToggle/MobSlider preserve user-driven state +// against a BEAM-pushed node above. That's what makes "content updates +// without dismissing/re-presenting" and "removing the node dismisses it" +// both fall out for free: a rerender with the sheet still present reuses +// this state; a rerender without it tears the view (and its presentation) +// down entirely. +private struct MobSheetView: View { + let node: MobNode + @State private var isPresented = true + @State private var dismissSent = false + + var body: some View { + Color.clear + .frame(width: 0, height: 0) + .sheet(isPresented: $isPresented, onDismiss: sendDismissOnce) { + sheetContent + } + } + + private func sendDismissOnce() { + guard !dismissSent else { return } + dismissSent = true + node.onDismiss?() + } + + @ViewBuilder + private var sheetContent: some View { + VStack(spacing: 0) { + ForEach(Array(node.childNodes.enumerated()), id: \.offset) { _, child in + MobNodeView(node: child) + } + } + .frame(maxWidth: .infinity, alignment: .topLeading) + // Screen readers should treat the sheet as a self-contained modal — + // VoiceOver focus stays inside it until dismissed, matching + // .presentationDetents/.sheet's own system-modal behavior. + .accessibilityElement(children: .contain) + .accessibilityAddTraits(.isModal) + .ifLet(node.backgroundColor) { view, bg in + view.presentationBackground(Color(bg)) + } + // 0 and "unset" are indistinguishable on this shared property (see + // MobNode.h) — same limitation every other node type already has + // for corner_radius, not new here. Skip the modifier for 0 so the + // system's own default sheet corner radius applies. + .ifLet(node.cornerRadius > 0 ? node.cornerRadius : nil) { view, radius in + view.presentationCornerRadius(radius) + } + .presentationDetents(detentSet) + .ifLet(hasCustomIndicator ? () : nil) { view, _ in + view.presentationDragIndicator(.hidden) + } + .overlay(alignment: .top) { + if hasCustomIndicator { + customIndicator + } + } + } + + private var detentSet: Set { + let requested = node.sheetDetents ?? ["medium", "large"] + var resolved: Set = [] + if requested.contains("medium") { resolved.insert(.medium) } + if requested.contains("large") { resolved.insert(.large) } + // Mob.UI.sheet/2 already validates :detents is a nonempty subset of + // [:medium, :large] — this fallback only matters for a hand-built + // node map that skipped that validation (e.g. `~MOB` sigil literal). + return resolved.isEmpty ? [.medium, .large] : resolved + } + + // Mob.UI.sheet/2 requires all four custom-indicator props together or + // none — checking one non-sentinel value is enough once that contract + // holds, but check all four defensively for the same hand-built-node + // reason as detentSet above. + private var hasCustomIndicator: Bool { + node.dragIndicatorColor != nil + && node.dragIndicatorWidth >= 0 + && node.dragIndicatorHeight >= 0 + && node.dragIndicatorRailHeight >= 0 + } + + private var customIndicator: some View { + Capsule() + .fill(node.dragIndicatorColor.map { Color($0) } ?? Color.secondary) + .frame(width: node.dragIndicatorWidth, height: node.dragIndicatorHeight) + .frame(height: node.dragIndicatorRailHeight) + .padding(.top, 6) + } +} + private struct MobImage: View { let node: MobNode diff --git a/ios/mob_nif.m b/ios/mob_nif.m index 2034b8d..ba6d82d 100644 --- a/ios/mob_nif.m +++ b/ios/mob_nif.m @@ -225,6 +225,9 @@ static void mob_send_submit(int handle) { static void mob_send_select(int handle) { mob_send_event(handle, "select"); } +static void mob_send_dismiss(int handle) { + mob_send_event(handle, "dismiss"); +} // IME composition. Sends {compose, tag, %{text: ..., phase: ...}} where // phase is one of began/updating/committed/cancelled. Called from the @@ -646,6 +649,8 @@ static void mob_send_change_float(int handle, double value) { node.nodeType = MobNodeTypeCanvas; else if ([type isEqualToString:@"gpu_view"]) node.nodeType = MobNodeTypeGpuView; + else if ([type isEqualToString:@"sheet"]) + node.nodeType = MobNodeTypeSheet; NSDictionary *props = dict[@"props"]; if ([props isKindOfClass:[NSDictionary class]]) { @@ -1129,6 +1134,38 @@ static void mob_send_change_float(int handle, double value) { node.gpuUniforms = uniforms; } + // sheet props. background/corner_radius are read generically above + // (shared prop names across every node type) — MobSheetView reuses + // node.backgroundColor/node.cornerRadius directly for the sheet's + // own container styling, see MobRootView.swift. + if (node.nodeType == MobNodeTypeSheet) { + id detents = props[@"detents"]; + if ([detents isKindOfClass:[NSArray class]]) + node.sheetDetents = detents; + + id indicatorColor = props[@"drag_indicator_color"]; + if (indicatorColor) + node.dragIndicatorColor = color_from_argb((long)[indicatorColor longLongValue]); + + id indicatorWidth = props[@"drag_indicator_width"]; + if (indicatorWidth) + node.dragIndicatorWidth = [indicatorWidth doubleValue]; + id indicatorHeight = props[@"drag_indicator_height"]; + if (indicatorHeight) + node.dragIndicatorHeight = [indicatorHeight doubleValue]; + id indicatorRailHeight = props[@"drag_indicator_rail_height"]; + if (indicatorRailHeight) + node.dragIndicatorRailHeight = [indicatorRailHeight doubleValue]; + + id onDismiss = props[@"on_dismiss"]; + if (onDismiss && [onDismiss isKindOfClass:[NSNumber class]]) { + int handle = [onDismiss intValue]; + node.onDismiss = ^{ + mob_send_dismiss(handle); + }; + } + } + // webview props id webViewUrl = props[@"url"]; if ([webViewUrl isKindOfClass:[NSString class]]) diff --git a/lib/mob/renderer.ex b/lib/mob/renderer.ex index d40c9c3..f23e6a4 100644 --- a/lib/mob/renderer.ex +++ b/lib/mob/renderer.ex @@ -173,7 +173,7 @@ defmodule Mob.Renderer do } # Props whose atom values are resolved as colors - @color_props ~w(background text_color border_color color placeholder_color)a + @color_props ~w(background text_color border_color color placeholder_color scrim drag_indicator_color)a # Props whose atom values are resolved as spacing or radius tokens @spacing_props ~w(padding padding_top padding_right padding_bottom padding_left gap)a @radius_props ~w(corner_radius)a @@ -353,6 +353,12 @@ defmodule Mob.Renderer do {:on_submit, {pid, tag}} when is_pid(pid) -> [{"on_submit", nif.register_tap({pid, tag})}] + # Sheet dismissal — swipe-down, back gesture, or outside tap. Native + # fires this exactly once per presentation (see ios/MobRootView.swift + # and mob_new's generated MobSheet composable). + {:on_dismiss, {pid, tag}} when is_pid(pid) -> + [{"on_dismiss", nif.register_tap({pid, tag})}] + # IME composition — fires for languages with multi-stage input (CJK, # Korean, Vietnamese, accent input). Phase atom is :began | :updating # | :committed | :cancelled. Apps that need commit-only behaviour diff --git a/lib/mob/ui.ex b/lib/mob/ui.ex index 729fdcf..53dc097 100644 --- a/lib/mob/ui.ex +++ b/lib/mob/ui.ex @@ -255,4 +255,238 @@ defmodule Mob.UI do children: [] } end + + @sheet_detents [:medium, :large] + @sheet_color_props [:background, :scrim, :drag_indicator_color] + @sheet_dimension_props [ + :drag_indicator_width, + :drag_indicator_height, + :drag_indicator_rail_height + ] + @sheet_indicator_props [:drag_indicator_color | @sheet_dimension_props] + @sheet_style_props [:corner_radius | @sheet_color_props ++ @sheet_dimension_props] + + @doc """ + Returns a `:sheet` node — a native modal bottom sheet (iOS `.sheet`, + Android Material 3 `ModalBottomSheet`) that composes ordinary Mob nodes + as its content. + + `children` is one child node or a list of them. + + ## Props + + * `:detents` — nonempty, duplicate-free subset of `[:medium, :large]`. + Defaults to `[:medium, :large]`. `:medium` alone rejects expansion + to full height; `:large` alone skips the half-height stop. + * `:on_dismiss` — `{pid, tag}`, delivered as `handle_info({:tap, tag}, socket)` + exactly once when the sheet is dismissed (swipe-down, back gesture, + or outside tap), matching the tap-event convention used elsewhere + in `Mob.UI`. + * `:background` — container color: a theme token atom or a + `0x00000000..0xFFFFFFFF` ARGB integer. + * `:scrim` — dimming-layer color, same value shape as `:background`. + **iOS cannot honor this exactly** — see the note below. + * `:corner_radius` — top-corner radius: a theme radius token atom or + a non-negative number. + * `:drag_indicator_color`, `:drag_indicator_width`, + `:drag_indicator_height`, `:drag_indicator_rail_height` — a custom + drag-indicator capsule. All four are required together, or omit + all four for the platform default indicator. Width and height must + be positive; rail height must be at least the indicator height (the + rail is the invisible touch target the visible capsule sits inside). + * `:ios` / `:android` — per-platform overrides. Each accepts only the + style keys above (not `:detents` or `:on_dismiss`); see `Mob.Renderer`'s + "Platform blocks" section for the general override mechanism. + + ## Example + + Mob.UI.sheet( + Mob.UI.text(text: "Hello from the sheet"), + detents: [:medium, :large], + on_dismiss: {self(), :dismiss_sheet}, + background: :surface, + scrim: 0x33000000, + corner_radius: 10, + drag_indicator_color: :muted, + drag_indicator_width: 36, + drag_indicator_height: 5, + drag_indicator_rail_height: 22, + ios: %{corner_radius: 10}, + android: %{corner_radius: 28} + ) + + ## Platform limitation: scrim opacity on iOS + + Android applies `:scrim` exactly — the sheet's dimming layer is drawn + with the requested color, alpha included. iOS's `.sheet` presentation + owns its dimming layer and does not expose an API to configure its + opacity; supported SwiftUI APIs leave it system-black at a fixed, + non-configurable alpha. There is no workaround that doesn't involve + private view-hierarchy manipulation, which this framework does not do. + If your design depends on exact scrim opacity, treat it as + Android-only and expect iOS to look slightly different. + """ + @spec sheet(map() | [map()], keyword() | map()) :: map() + def sheet(children, opts \\ []) + def sheet(children, opts) when is_list(opts), do: sheet(children, Map.new(opts)) + + def sheet(children, %{} = opts) do + detents = Map.get(opts, :detents, @sheet_detents) + validate_detents!(detents) + validate_on_dismiss!(Map.get(opts, :on_dismiss)) + validate_style!(opts) + validate_platform_override!(opts, :ios) + validate_platform_override!(opts, :android) + validate_indicator_completeness!(opts) + + %{ + type: :sheet, + props: Map.put(opts, :detents, detents), + children: List.wrap(children) + } + end + + defp validate_detents!(detents) do + unless is_list(detents) and detents != [] do + raise ArgumentError, + "Mob.UI.sheet :detents must be a nonempty list, got: #{inspect(detents)}" + end + + unless Enum.uniq(detents) == detents do + raise ArgumentError, + "Mob.UI.sheet :detents must not contain duplicates, got: #{inspect(detents)}" + end + + unless Enum.all?(detents, &(&1 in @sheet_detents)) do + raise ArgumentError, + "Mob.UI.sheet :detents must be a subset of #{inspect(@sheet_detents)}, " <> + "got: #{inspect(detents)}" + end + end + + defp validate_on_dismiss!(nil), do: :ok + defp validate_on_dismiss!({pid, tag}) when is_pid(pid) and is_atom(tag), do: :ok + + defp validate_on_dismiss!(other) do + raise ArgumentError, "Mob.UI.sheet :on_dismiss must be {pid, atom}, got: #{inspect(other)}" + end + + defp validate_style!(opts) do + Enum.each(@sheet_style_props, fn key -> + if Map.has_key?(opts, key), do: validate_style_value!(key, Map.fetch!(opts, key)) + end) + end + + defp validate_style_value!(:corner_radius, value), do: validate_radius!(:corner_radius, value) + + defp validate_style_value!(key, value) when key in @sheet_color_props, + do: validate_color!(key, value) + + defp validate_style_value!(key, value) when key in @sheet_dimension_props, + do: validate_dimension!(key, value) + + # Colors accept a theme-token atom or an unsigned ARGB integer. `nil`, + # `true`, and `false` are atoms too — excluded explicitly so they don't + # silently pass through to native code that expects a real color and + # crashes trying to read one (the iOS NSNull.longLongValue failure mode + # this validation exists to prevent). + defp validate_color!(_key, value) when is_atom(value) and value not in [nil, true, false], + do: :ok + + defp validate_color!(_key, value) when is_integer(value) and value in 0..0xFFFFFFFF, do: :ok + + defp validate_color!(key, value) do + raise ArgumentError, + "Mob.UI.sheet #{inspect(key)} must be a theme-token atom or a " <> + "0x00000000..0xFFFFFFFF ARGB integer, got: #{inspect(value)}" + end + + defp validate_radius!(_key, value) when is_atom(value) and value not in [nil, true, false], + do: :ok + + defp validate_radius!(_key, value) when is_number(value) and value >= 0, do: :ok + + defp validate_radius!(key, value) do + raise ArgumentError, + "Mob.UI.sheet #{inspect(key)} must be a radius token atom or a non-negative number, " <> + "got: #{inspect(value)}" + end + + defp validate_dimension!(_key, value) when is_number(value) and value >= 0, do: :ok + + defp validate_dimension!(key, value) do + raise ArgumentError, + "Mob.UI.sheet #{inspect(key)} must be a non-negative number, got: #{inspect(value)}" + end + + defp validate_platform_override!(opts, platform_key) do + case Map.get(opts, platform_key) do + nil -> + :ok + + %{} = override -> + invalid = Map.keys(override) -- @sheet_style_props + + unless invalid == [] do + raise ArgumentError, + "Mob.UI.sheet #{inspect(platform_key)} override contains unsupported keys: " <> + "#{inspect(invalid)} (supported: #{inspect(@sheet_style_props)})" + end + + Enum.each(override, fn {key, value} -> validate_style_value!(key, value) end) + + other -> + raise ArgumentError, + "Mob.UI.sheet #{inspect(platform_key)} must be a map, got: #{inspect(other)}" + end + end + + # If any custom drag-indicator prop is supplied, all four are required — + # a partial override has no sensible native default to fall back to for + # the missing geometry. validate_style! already ran by the time this is + # called, so width/height/rail_height are confirmed numeric here; the + # ordering matters because Elixir's structural `>` never raises across + # types (an atom silently compares greater than any number), so the + # positivity/rail-height checks below would rubber-stamp a bad value + # instead of catching it if type-checking hadn't already happened. + defp validate_indicator_completeness!(opts) do + present = Enum.filter(@sheet_indicator_props, &Map.has_key?(opts, &1)) + + cond do + present == [] -> + :ok + + length(present) == length(@sheet_indicator_props) -> + validate_indicator_geometry!(opts) + + true -> + missing = @sheet_indicator_props -- present + + raise ArgumentError, + "Mob.UI.sheet: a custom drag indicator requires all of " <> + "#{inspect(@sheet_indicator_props)}, missing: #{inspect(missing)}" + end + end + + defp validate_indicator_geometry!(opts) do + width = Map.fetch!(opts, :drag_indicator_width) + height = Map.fetch!(opts, :drag_indicator_height) + rail = Map.fetch!(opts, :drag_indicator_rail_height) + + unless width > 0 do + raise ArgumentError, + "Mob.UI.sheet :drag_indicator_width must be positive, got: #{inspect(width)}" + end + + unless height > 0 do + raise ArgumentError, + "Mob.UI.sheet :drag_indicator_height must be positive, got: #{inspect(height)}" + end + + unless rail >= height do + raise ArgumentError, + "Mob.UI.sheet :drag_indicator_rail_height (#{inspect(rail)}) must be >= " <> + ":drag_indicator_height (#{inspect(height)})" + end + end end diff --git a/test/mob/renderer_test.exs b/test/mob/renderer_test.exs index 95de493..9573c40 100644 --- a/test/mob/renderer_test.exs +++ b/test/mob/renderer_test.exs @@ -1206,4 +1206,132 @@ defmodule Mob.RendererTest do refute Map.has_key?(set_root_json()["props"], "font") end end + + describe "sheet serialization" do + setup do + on_exit(fn -> Application.delete_env(:mob, :theme) end) + :ok + end + + # Builds the raw node map directly rather than going through Mob.UI.sheet/2 + # — Renderer serialization is tested independently of Mob.UI's validation + # layer (which has its own test coverage in test/mob/ui_test.exs), so a + # deliberately-incomplete prop set here (e.g. one indicator prop without + # the other three) isn't rejected before reaching the code under test. + defp sheet_tree(props, children \\ [%{type: :text, props: %{text: "hi"}, children: []}]) do + %{type: :sheet, props: props, children: children} + end + + test "type serializes as the string \"sheet\"" do + Renderer.render(sheet_tree(%{}), :android, MockNIF) + assert set_root_json()["type"] == "sheet" + end + + test "children serialize recursively like normal children" do + children = [ + %{type: :text, props: %{text: "a"}, children: []}, + %{type: :text, props: %{text: "b"}, children: []} + ] + + Renderer.render(sheet_tree(%{}, children), :android, MockNIF) + decoded_children = set_root_json()["children"] + + assert length(decoded_children) == 2 + assert Enum.at(decoded_children, 0)["props"]["text"] == "a" + assert Enum.at(decoded_children, 1)["props"]["text"] == "b" + end + + test "detents serialize as a list of strings" do + Renderer.render(sheet_tree(%{detents: [:medium]}), :android, MockNIF) + assert set_root_json()["props"]["detents"] == ["medium"] + end + + test "on_dismiss registers through the tap registry and serializes as an integer handle" do + tag = {self(), :dismissed} + Renderer.render(sheet_tree(%{on_dismiss: tag}), :android, MockNIF) + + assert is_integer(set_root_json()["props"]["on_dismiss"]) + assert Enum.any?(MockNIF.calls(), fn {f, args} -> f == :register_tap and args == [tag] end) + end + + test "scrim is a color-token prop — resolves through the theme like :background" do + Mob.Theme.set(muted: :black) + Renderer.render(sheet_tree(%{scrim: :muted}), :android, MockNIF) + assert set_root_json()["props"]["scrim"] == 0xFF000000 + end + + test "scrim passes through a raw ARGB integer unresolved" do + Renderer.render(sheet_tree(%{scrim: 0x33000000}), :android, MockNIF) + assert set_root_json()["props"]["scrim"] == 0x33000000 + end + + test "drag_indicator_color is a color-token prop" do + Mob.Theme.set(muted: :gray_500) + Renderer.render(sheet_tree(%{drag_indicator_color: :muted}), :android, MockNIF) + resolved = set_root_json()["props"]["drag_indicator_color"] + assert is_integer(resolved) + end + + test "corner_radius resolves through radius tokens" do + Mob.Theme.set(radius_md: 20) + Renderer.render(sheet_tree(%{corner_radius: :radius_md}), :android, MockNIF) + assert set_root_json()["props"]["corner_radius"] == 20 + end + + test "corner_radius passes a raw number through unresolved" do + Renderer.render(sheet_tree(%{corner_radius: 10}), :android, MockNIF) + assert set_root_json()["props"]["corner_radius"] == 10 + end + + test "drag_indicator_width/height/rail_height pass through as plain numbers" do + Renderer.render( + sheet_tree(%{ + drag_indicator_color: :muted, + drag_indicator_width: 36, + drag_indicator_height: 5, + drag_indicator_rail_height: 22 + }), + :android, + MockNIF + ) + + props = set_root_json()["props"] + assert props["drag_indicator_width"] == 36 + assert props["drag_indicator_height"] == 5 + assert props["drag_indicator_rail_height"] == 22 + end + + test "ios override flattens on ios platform, stripped from serialised JSON" do + Renderer.render( + sheet_tree(%{corner_radius: 10, ios: %{corner_radius: 4}}), + :ios, + MockNIF + ) + + props = set_root_json()["props"] + assert props["corner_radius"] == 4 + refute Map.has_key?(props, "ios") + refute Map.has_key?(props, "android") + end + + test "android override flattens on android platform, ios override ignored" do + Renderer.render( + sheet_tree(%{corner_radius: 10, ios: %{corner_radius: 4}, android: %{corner_radius: 28}}), + :android, + MockNIF + ) + + assert set_root_json()["props"]["corner_radius"] == 28 + end + + test "base value used when the active platform has no override" do + Renderer.render( + sheet_tree(%{corner_radius: 10, ios: %{corner_radius: 4}}), + :android, + MockNIF + ) + + assert set_root_json()["props"]["corner_radius"] == 10 + end + end end diff --git a/test/mob/ui_test.exs b/test/mob/ui_test.exs index 90ace7f..cc92805 100644 --- a/test/mob/ui_test.exs +++ b/test/mob/ui_test.exs @@ -210,4 +210,308 @@ defmodule Mob.UITest do assert props.uniforms == uniforms end end + + # ── sheet/2 ────────────────────────────────────────────────────────────────── + + describe "sheet/2 constructor" do + test "type is :sheet" do + assert UI.sheet(UI.text(text: "hi")).type == :sheet + end + + test "accepts a single child node, wrapped into a list" do + assert UI.sheet(UI.text(text: "hi")).children == [UI.text(text: "hi")] + end + + test "accepts a list of child nodes verbatim" do + children = [UI.text(text: "a"), UI.text(text: "b")] + assert UI.sheet(children).children == children + end + + test "defaults :detents to [:medium, :large]" do + assert UI.sheet(UI.text(text: "hi")).props.detents == [:medium, :large] + end + + test "accepts a plain map and produces identical output to the keyword form" do + kw = UI.sheet(UI.text(text: "hi"), detents: [:medium]) + m = UI.sheet(UI.text(text: "hi"), %{detents: [:medium]}) + assert kw == m + end + + test "shape is renderer-compatible — %{type:, props:, children:}" do + node = UI.sheet(UI.text(text: "hi")) + assert Map.keys(node) |> Enum.sort() == [:children, :props, :type] + end + end + + describe "sheet/2 detents" do + test "accepts [:medium]" do + assert UI.sheet(UI.text(text: "hi"), detents: [:medium]).props.detents == [:medium] + end + + test "accepts [:large]" do + assert UI.sheet(UI.text(text: "hi"), detents: [:large]).props.detents == [:large] + end + + test "accepts [:medium, :large] in either order" do + assert UI.sheet(UI.text(text: "hi"), detents: [:large, :medium]).props.detents == [ + :large, + :medium + ] + end + + test "rejects an empty list" do + assert_raise ArgumentError, ~r/nonempty/, fn -> + UI.sheet(UI.text(text: "hi"), detents: []) + end + end + + test "rejects duplicates" do + assert_raise ArgumentError, ~r/duplicates/, fn -> + UI.sheet(UI.text(text: "hi"), detents: [:medium, :medium]) + end + end + + test "rejects a detent outside [:medium, :large]" do + assert_raise ArgumentError, ~r/subset/, fn -> + UI.sheet(UI.text(text: "hi"), detents: [:medium, :full]) + end + end + + test "rejects a non-list" do + assert_raise ArgumentError, ~r/nonempty list/, fn -> + UI.sheet(UI.text(text: "hi"), detents: :medium) + end + end + end + + describe "sheet/2 on_dismiss" do + test "accepts {pid, atom}" do + tag = {self(), :dismissed} + assert UI.sheet(UI.text(text: "hi"), on_dismiss: tag).props.on_dismiss == tag + end + + test "omitted is fine — no on_dismiss key in props" do + refute Map.has_key?(UI.sheet(UI.text(text: "hi")).props, :on_dismiss) + end + + test "rejects a bare pid (no tag)" do + assert_raise ArgumentError, ~r/on_dismiss/, fn -> + UI.sheet(UI.text(text: "hi"), on_dismiss: self()) + end + end + + test "rejects a non-pid first element" do + assert_raise ArgumentError, ~r/on_dismiss/, fn -> + UI.sheet(UI.text(text: "hi"), on_dismiss: {:not_a_pid, :dismissed}) + end + end + + test "rejects a non-atom tag" do + assert_raise ArgumentError, ~r/on_dismiss/, fn -> + UI.sheet(UI.text(text: "hi"), on_dismiss: {self(), "dismissed"}) + end + end + end + + describe "sheet/2 style props" do + test "accepts a theme-token atom for :background" do + assert UI.sheet(UI.text(text: "hi"), background: :surface).props.background == :surface + end + + test "accepts a raw ARGB integer for :background" do + assert UI.sheet(UI.text(text: "hi"), background: 0x33000000).props.background == 0x33000000 + end + + test "0x00000000 and 0xFFFFFFFF are both valid (boundary check)" do + assert UI.sheet(UI.text(text: "hi"), scrim: 0x00000000).props.scrim == 0 + assert UI.sheet(UI.text(text: "hi"), scrim: 0xFFFFFFFF).props.scrim == 0xFFFFFFFF + end + + test "accepts a theme radius token atom or a non-negative number for :corner_radius" do + assert UI.sheet(UI.text(text: "hi"), corner_radius: :radius_lg).props.corner_radius == + :radius_lg + + assert UI.sheet(UI.text(text: "hi"), corner_radius: 10).props.corner_radius == 10 + assert UI.sheet(UI.text(text: "hi"), corner_radius: 0).props.corner_radius == 0 + end + + test "unrecognized props pass through — sheet doesn't allowlist beyond validation" do + # Unlike text/1 and canvas/1 (which Map.take an explicit allowlist), + # sheet/2 only validates the KNOWN style keys and otherwise passes + # opts through — :detents/:on_dismiss/:ios/:android live alongside + # arbitrary future props without needing a matching Map.take update. + props = UI.sheet(UI.text(text: "hi"), on_dismiss: {self(), :x}).props + assert Map.has_key?(props, :on_dismiss) + end + + for {label, value} <- [ + {"nil", nil}, + {"true", true}, + {"false", false}, + {"a string", "#ff0000"}, + {"a negative integer", -1}, + {"an integer above 0xFFFFFFFF", 0x100000000} + ] do + test "rejects #{label} as :background" do + assert_raise ArgumentError, ~r/:background/, fn -> + UI.sheet(UI.text(text: "hi"), background: unquote(Macro.escape(value))) + end + end + + test "rejects #{label} as :scrim" do + assert_raise ArgumentError, ~r/:scrim/, fn -> + UI.sheet(UI.text(text: "hi"), scrim: unquote(Macro.escape(value))) + end + end + + test "rejects #{label} as :drag_indicator_color (via the full-indicator form)" do + assert_raise ArgumentError, ~r/:drag_indicator_color/, fn -> + UI.sheet(UI.text(text: "hi"), + drag_indicator_color: unquote(Macro.escape(value)), + drag_indicator_width: 36, + drag_indicator_height: 5, + drag_indicator_rail_height: 22 + ) + end + end + end + + test "rejects nil/true/false as :corner_radius" do + for bad <- [nil, true, false] do + assert_raise ArgumentError, ~r/:corner_radius/, fn -> + UI.sheet(UI.text(text: "hi"), corner_radius: bad) + end + end + end + + test "rejects a negative :corner_radius" do + assert_raise ArgumentError, ~r/:corner_radius/, fn -> + UI.sheet(UI.text(text: "hi"), corner_radius: -1) + end + end + end + + describe "sheet/2 drag indicator geometry" do + @full_indicator [ + drag_indicator_color: :muted, + drag_indicator_width: 36, + drag_indicator_height: 5, + drag_indicator_rail_height: 22 + ] + + test "accepts all four together" do + props = UI.sheet(UI.text(text: "hi"), @full_indicator).props + assert props.drag_indicator_width == 36 + assert props.drag_indicator_height == 5 + assert props.drag_indicator_rail_height == 22 + assert props.drag_indicator_color == :muted + end + + test "omitting all four is fine — platform default indicator" do + props = UI.sheet(UI.text(text: "hi")).props + refute Map.has_key?(props, :drag_indicator_color) + refute Map.has_key?(props, :drag_indicator_width) + end + + for missing <- [ + :drag_indicator_color, + :drag_indicator_width, + :drag_indicator_height, + :drag_indicator_rail_height + ] do + test "rejects supplying only 3 of 4 (missing #{missing})" do + partial = Keyword.delete(@full_indicator, unquote(missing)) + + assert_raise ArgumentError, ~r/requires all of/, fn -> + UI.sheet(UI.text(text: "hi"), partial) + end + end + end + + test "rejects width == 0" do + opts = Keyword.put(@full_indicator, :drag_indicator_width, 0) + + assert_raise ArgumentError, ~r/:drag_indicator_width must be positive/, fn -> + UI.sheet(UI.text(text: "hi"), opts) + end + end + + test "rejects a negative height" do + opts = Keyword.put(@full_indicator, :drag_indicator_height, -1) + + assert_raise ArgumentError, ~r/:drag_indicator_height/, fn -> + UI.sheet(UI.text(text: "hi"), opts) + end + end + + test "rejects rail_height < height" do + opts = + @full_indicator + |> Keyword.put(:drag_indicator_height, 20) + |> Keyword.put(:drag_indicator_rail_height, 5) + + assert_raise ArgumentError, ~r/rail_height.*must be >=/, fn -> + UI.sheet(UI.text(text: "hi"), opts) + end + end + + test "accepts rail_height == height (boundary)" do + opts = + @full_indicator + |> Keyword.put(:drag_indicator_height, 10) + |> Keyword.put(:drag_indicator_rail_height, 10) + + assert UI.sheet(UI.text(text: "hi"), opts).props.drag_indicator_rail_height == 10 + end + + test "a non-numeric width doesn't silently pass the positivity check" do + # Regression guard: Elixir's structural `>` never raises across types + # (an atom compares greater than any number), so if type-checking + # didn't run before the width > 0 check, a garbage atom would slip + # through as "positive." Confirms the type error fires first. + opts = Keyword.put(@full_indicator, :drag_indicator_width, :not_a_number) + + assert_raise ArgumentError, ~r/:drag_indicator_width must be a non-negative number/, fn -> + UI.sheet(UI.text(text: "hi"), opts) + end + end + end + + describe "sheet/2 platform overrides" do + test "accepts :ios and :android maps with supported style keys" do + props = + UI.sheet(UI.text(text: "hi"), + corner_radius: 10, + ios: %{corner_radius: 10}, + android: %{corner_radius: 28} + ).props + + assert props.ios == %{corner_radius: 10} + assert props.android == %{corner_radius: 28} + end + + test "rejects :detents inside an :ios override" do + assert_raise ArgumentError, ~r/unsupported keys/, fn -> + UI.sheet(UI.text(text: "hi"), ios: %{detents: [:medium]}) + end + end + + test "rejects :on_dismiss inside an :android override" do + assert_raise ArgumentError, ~r/unsupported keys/, fn -> + UI.sheet(UI.text(text: "hi"), android: %{on_dismiss: {self(), :x}}) + end + end + + test "rejects an invalid color value inside an override map" do + assert_raise ArgumentError, ~r/:background/, fn -> + UI.sheet(UI.text(text: "hi"), ios: %{background: nil}) + end + end + + test "rejects a non-map :ios value" do + assert_raise ArgumentError, ~r/:ios must be a map/, fn -> + UI.sheet(UI.text(text: "hi"), ios: "not a map") + end + end + end end From 04724ac68cd104c6eb680a74cc36136e58f925ff Mon Sep 17 00:00:00 2001 From: GenericJam Date: Wed, 26 Aug 2026 19:12:58 -0600 Subject: [PATCH 2/2] MOB-101: address code review findings on Mob.UI.sheet/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix moduledoc: on_dismiss delivers {:dismiss, tag}, not {:tap, tag} - Extend drag-indicator completeness validation into ios:/android: overrides (checked post-merge, not just on base props) — a partial override now fails validation instead of silently rendering the system default indicator - Warn (not silently pass through) when a color prop resolves to neither a theme token nor the base palette - iOS: apply node.paddingEdgeInsets to sheet content (was dropped) - iOS: give the sheet its own corner-radius sentinel (sheetCornerRadius, -1 = unset) instead of sharing node.cornerRadius, so corner_radius: 0 is no longer indistinguishable from "not set" - iOS: stop reporting a 0x0 frame for a sheet's id — the switch-case view is an invisible anchor, not the sheet's real presented content - Dedupe the atom-token guard shared by validate_color!/validate_radius! into a single defguardp - Add a regression test asserting Mob.UI.sheet/2's color props stay a subset of Mob.Renderer's color-resolved prop set Reviewed but left unchanged: the platform-override dispatch loop (already delegates to validate_style_value!, not a reimplementation). Not merged/released — awaiting review per standing instruction. Co-Authored-By: Claude Sonnet 5 --- ios/MobNode.h | 27 +++++++++------ ios/MobNode.m | 1 + ios/MobRootView.swift | 18 ++++++---- ios/mob_nif.m | 19 ++++++++--- lib/mob/renderer.ex | 34 +++++++++++++++++-- lib/mob/ui.ex | 67 ++++++++++++++++++++++++++------------ test/mob/renderer_test.exs | 10 ++++++ test/mob/ui_test.exs | 11 +++++++ 8 files changed, 144 insertions(+), 43 deletions(-) diff --git a/ios/MobNode.h b/ios/MobNode.h index 13b7ecf..388196f 100644 --- a/ios/MobNode.h +++ b/ios/MobNode.h @@ -262,16 +262,23 @@ NS_ASSUME_NONNULL_BEGIN // expected packing semantics per element. @property(nonatomic, strong, nullable) id gpuUniforms; -// Sheet — native modal bottom sheet. `backgroundColor` and `cornerRadius` -// (declared above, shared by every node type) double as the sheet's own -// container background / top-corner radius, matching how every other prop -// name is shared across types. `sheetDetents` is the raw "medium"/"large" -// string list from Mob.Renderer — mapped to PresentationDetent by -// MobSheetView (see MobRootView.swift), not here, so this header stays -// framework-agnostic. Indicator geometry defaults to -1 (unset — use the -// system default indicator); Mob.UI.sheet's validation guarantees all four -// arrive together or not at all, so checking any one for >= 0 is enough to -// know whether a complete custom indicator was supplied. +// Sheet — native modal bottom sheet. `backgroundColor` (declared above, +// shared by every node type) doubles as the sheet's own container +// background, matching how every other prop name is shared across types — +// `nil` unambiguously means "unset" for a color, so sharing it is safe. +// `cornerRadius` is NOT reused here: it defaults to 0 for every other node +// type, where 0-vs-unset is harmless (a 0-radius box looks like a +// system-default box in practice), but a sheet's corners are visibly +// square-vs-rounded, so `sheetCornerRadius` gets its own -1 sentinel +// (unset — use the system default) instead. `sheetDetents` is the raw +// "medium"/"large" string list from Mob.Renderer — mapped to +// PresentationDetent by MobSheetView (see MobRootView.swift), not here, so +// this header stays framework-agnostic. Indicator geometry defaults to -1 +// (unset — use the system default indicator); Mob.UI.sheet's validation +// guarantees all four arrive together or not at all, so checking any one +// for >= 0 is enough to know whether a complete custom indicator was +// supplied. +@property(nonatomic) CGFloat sheetCornerRadius; @property(nonatomic, strong, nullable) NSArray *sheetDetents; @property(nonatomic, strong, nullable) UIColor *dragIndicatorColor; @property(nonatomic) CGFloat dragIndicatorWidth; diff --git a/ios/MobNode.m b/ios/MobNode.m index 95d9ea8..e9f442c 100644 --- a/ios/MobNode.m +++ b/ios/MobNode.m @@ -38,6 +38,7 @@ - (instancetype)init { _fillWidth = NO; _cornerRadius = 0.0; _nativeViewHandle = -1; // -1 = no native component slot assigned (MOB-100) + _sheetCornerRadius = -1.0; // -1 = unset — use the system default sheet corner radius _dragIndicatorWidth = -1.0; // -1 = no custom indicator supplied — use system default _dragIndicatorHeight = -1.0; _dragIndicatorRailHeight = -1.0; diff --git a/ios/MobRootView.swift b/ios/MobRootView.swift index 6e5e526..d24d9aa 100644 --- a/ios/MobRootView.swift +++ b/ios/MobRootView.swift @@ -521,7 +521,13 @@ private struct MobFrameTracker: ViewModifier { let node: MobNode func body(content: Content) -> some View { - if let id = node.nativeViewId { + // A sheet's own switch-case view is a zero-size anchor used only to + // attach `.sheet(isPresented:)` — its real, visible content is + // presented in a detached overlay that this GeometryReader can't + // see. Reporting the anchor's frame would silently report 0x0 + // instead of the sheet's actual on-screen bounds, so tracking is + // skipped entirely rather than publishing a frame known to be wrong. + if let id = node.nativeViewId, node.nodeType != .sheet { content .accessibilityIdentifier(id) .background( @@ -1337,6 +1343,7 @@ private struct MobSheetView: View { } } .frame(maxWidth: .infinity, alignment: .topLeading) + .padding(node.paddingEdgeInsets) // Screen readers should treat the sheet as a self-contained modal — // VoiceOver focus stays inside it until dismissed, matching // .presentationDetents/.sheet's own system-modal behavior. @@ -1345,11 +1352,10 @@ private struct MobSheetView: View { .ifLet(node.backgroundColor) { view, bg in view.presentationBackground(Color(bg)) } - // 0 and "unset" are indistinguishable on this shared property (see - // MobNode.h) — same limitation every other node type already has - // for corner_radius, not new here. Skip the modifier for 0 so the - // system's own default sheet corner radius applies. - .ifLet(node.cornerRadius > 0 ? node.cornerRadius : nil) { view, radius in + // sheetCornerRadius has its own -1-means-unset sentinel (see + // MobNode.h) so an explicit corner_radius: 0 (square corners) is + // distinguishable from "not set" (system default radius). + .ifLet(node.sheetCornerRadius >= 0 ? node.sheetCornerRadius : nil) { view, radius in view.presentationCornerRadius(radius) } .presentationDetents(detentSet) diff --git a/ios/mob_nif.m b/ios/mob_nif.m index ba6d82d..460b4d0 100644 --- a/ios/mob_nif.m +++ b/ios/mob_nif.m @@ -1134,11 +1134,22 @@ static void mob_send_change_float(int handle, double value) { node.gpuUniforms = uniforms; } - // sheet props. background/corner_radius are read generically above - // (shared prop names across every node type) — MobSheetView reuses - // node.backgroundColor/node.cornerRadius directly for the sheet's - // own container styling, see MobRootView.swift. + // sheet props. background is read generically above (shared prop + // name across every node type) — MobSheetView reuses + // node.backgroundColor directly for the sheet's own container + // background, see MobRootView.swift. corner_radius is read a + // second time here into the dedicated sheetCornerRadius (-1 = + // unset) instead: node.cornerRadius is a plain CGFloat with a 0 + // default, so by the time Swift sees it, an explicit + // `corner_radius: 0` is indistinguishable from "never set" — a + // sheet's corners are visibly square-vs-rounded, so that ambiguity + // needs its own sentinel here (unlike other node types, where 0 and + // unset render identically). if (node.nodeType == MobNodeTypeSheet) { + id sheetCornerRadius = props[@"corner_radius"]; + if (sheetCornerRadius) + node.sheetCornerRadius = [sheetCornerRadius doubleValue]; + id detents = props[@"detents"]; if ([detents isKindOfClass:[NSArray class]]) node.sheetDetents = detents; diff --git a/lib/mob/renderer.ex b/lib/mob/renderer.ex index f23e6a4..365d91f 100644 --- a/lib/mob/renderer.ex +++ b/lib/mob/renderer.ex @@ -70,6 +70,8 @@ defmodule Mob.Renderer do Mob.Renderer.render(tree, :android, MockNIF) """ + require Logger + alias Mob.{Style, Theme} @default_nif :mob_nif @@ -260,6 +262,10 @@ defmodule Mob.Renderer do @spec text_sizes() :: %{atom() => float()} def text_sizes, do: @text_sizes + @doc "Return the set of prop keys resolved as colors (theme token or ARGB integer)." + @spec color_props() :: [atom()] + def color_props, do: @color_props + # ── Tree preparation ────────────────────────────────────────────────────── defp prepare(%{type: type, props: props, children: children}, nif, platform, ctx) do @@ -566,10 +572,16 @@ defmodule Mob.Renderer do defp resolve_color(value, theme_colors) when is_atom(value) do case Map.get(theme_colors, value) do nil -> - Map.get(@colors, value, value) + case Map.fetch(@colors, value) do + {:ok, resolved} -> resolved + :error -> warn_unresolved_color(value) + end palette_atom when is_atom(palette_atom) -> - Map.get(@colors, palette_atom, palette_atom) + case Map.fetch(@colors, palette_atom) do + {:ok, resolved} -> resolved + :error -> warn_unresolved_color(palette_atom) + end raw_int when is_integer(raw_int) -> raw_int @@ -578,6 +590,24 @@ defmodule Mob.Renderer do defp resolve_color(value, _theme_colors), do: value + # An atom that resolves through neither the active theme nor the base + # palette serializes as a bare string to native, which then fails to + # parse it as a color (silently — e.g. iOS's NSString.longLongValue on a + # non-numeric string returns 0, an invisible fully-transparent color). + # Logging here turns that into a loud signal instead of a silent + # wrong-render, without changing pass-through behavior (theme tokens are + # app-defined and open-ended, so rejecting outright would be a false + # positive for any token this module doesn't know about). + defp warn_unresolved_color(atom) do + Logger.warning( + "Mob.Renderer: color token #{inspect(atom)} did not resolve against the active " <> + "theme or the base palette — passing it through unresolved to native, which " <> + "will fail to parse it as a color. Likely a typo'd theme token." + ) + + atom + end + # Font token resolution: theme fonts map → the value for the CURRENT # platform only. Unlike colors (which resolve to one value used by both # platforms — an ARGB int), a font value carries a name per platform, so diff --git a/lib/mob/ui.ex b/lib/mob/ui.ex index 53dc097..0656e05 100644 --- a/lib/mob/ui.ex +++ b/lib/mob/ui.ex @@ -266,6 +266,12 @@ defmodule Mob.UI do @sheet_indicator_props [:drag_indicator_color | @sheet_dimension_props] @sheet_style_props [:corner_radius | @sheet_color_props ++ @sheet_dimension_props] + # `nil`, `true`, and `false` are atoms too — excluded explicitly so a + # theme-token prop doesn't silently pass one through to native code that + # expects a real color/radius and crashes trying to read one (the iOS + # NSNull.longLongValue failure mode this guard exists to prevent). + defguardp is_theme_token(value) when is_atom(value) and value not in [nil, true, false] + @doc """ Returns a `:sheet` node — a native modal bottom sheet (iOS `.sheet`, Android Material 3 `ModalBottomSheet`) that composes ordinary Mob nodes @@ -278,10 +284,10 @@ defmodule Mob.UI do * `:detents` — nonempty, duplicate-free subset of `[:medium, :large]`. Defaults to `[:medium, :large]`. `:medium` alone rejects expansion to full height; `:large` alone skips the half-height stop. - * `:on_dismiss` — `{pid, tag}`, delivered as `handle_info({:tap, tag}, socket)` + * `:on_dismiss` — `{pid, tag}`, delivered as `handle_info({:dismiss, tag}, socket)` exactly once when the sheet is dismissed (swipe-down, back gesture, - or outside tap), matching the tap-event convention used elsewhere - in `Mob.UI`. + or outside tap) — the same `{atom, tag}` wire shape as `on_focus`, + `on_blur`, `on_submit`, and `on_select` elsewhere in `Mob.UI`. * `:background` — container color: a theme token atom or a `0x00000000..0xFFFFFFFF` ARGB integer. * `:scrim` — dimming-layer color, same value shape as `:background`. @@ -385,14 +391,7 @@ defmodule Mob.UI do defp validate_style_value!(key, value) when key in @sheet_dimension_props, do: validate_dimension!(key, value) - # Colors accept a theme-token atom or an unsigned ARGB integer. `nil`, - # `true`, and `false` are atoms too — excluded explicitly so they don't - # silently pass through to native code that expects a real color and - # crashes trying to read one (the iOS NSNull.longLongValue failure mode - # this validation exists to prevent). - defp validate_color!(_key, value) when is_atom(value) and value not in [nil, true, false], - do: :ok - + defp validate_color!(_key, value) when is_theme_token(value), do: :ok defp validate_color!(_key, value) when is_integer(value) and value in 0..0xFFFFFFFF, do: :ok defp validate_color!(key, value) do @@ -401,9 +400,7 @@ defmodule Mob.UI do "0x00000000..0xFFFFFFFF ARGB integer, got: #{inspect(value)}" end - defp validate_radius!(_key, value) when is_atom(value) and value not in [nil, true, false], - do: :ok - + defp validate_radius!(_key, value) when is_theme_token(value), do: :ok defp validate_radius!(_key, value) when is_number(value) and value >= 0, do: :ok defp validate_radius!(key, value) do @@ -443,28 +440,56 @@ defmodule Mob.UI do # If any custom drag-indicator prop is supplied, all four are required — # a partial override has no sensible native default to fall back to for - # the missing geometry. validate_style! already ran by the time this is - # called, so width/height/rail_height are confirmed numeric here; the - # ordering matters because Elixir's structural `>` never raises across - # types (an atom silently compares greater than any number), so the + # the missing geometry. Checked against the base opts AND against the + # base merged with each platform override, since `Mob.Renderer`'s + # platform-block flattening (`ios:`/`android:`) composes an override on + # top of the base at render time — a base with zero indicator props plus + # an `ios: %{drag_indicator_color: ...}` override would otherwise pass + # validation here but flatten to an incomplete set on iOS, which native + # silently treats as "no custom indicator" instead of erroring. + # validate_style! already ran by the time this is called, so + # width/height/rail_height are confirmed numeric here; the ordering + # matters because Elixir's structural `>` never raises across types (an + # atom silently compares greater than any number), so the # positivity/rail-height checks below would rubber-stamp a bad value # instead of catching it if type-checking hadn't already happened. defp validate_indicator_completeness!(opts) do - present = Enum.filter(@sheet_indicator_props, &Map.has_key?(opts, &1)) + validate_merged_indicator_completeness!(opts, "base props") + + case Map.get(opts, :ios) do + %{} = override -> + validate_merged_indicator_completeness!(Map.merge(opts, override), ":ios override") + + _ -> + :ok + end + + case Map.get(opts, :android) do + %{} = override -> + validate_merged_indicator_completeness!(Map.merge(opts, override), ":android override") + + _ -> + :ok + end + end + + defp validate_merged_indicator_completeness!(merged, context) do + present = Enum.filter(@sheet_indicator_props, &Map.has_key?(merged, &1)) cond do present == [] -> :ok length(present) == length(@sheet_indicator_props) -> - validate_indicator_geometry!(opts) + validate_indicator_geometry!(merged) true -> missing = @sheet_indicator_props -- present raise ArgumentError, "Mob.UI.sheet: a custom drag indicator requires all of " <> - "#{inspect(@sheet_indicator_props)}, missing: #{inspect(missing)}" + "#{inspect(@sheet_indicator_props)} (checking #{context}), " <> + "missing: #{inspect(missing)}" end end diff --git a/test/mob/renderer_test.exs b/test/mob/renderer_test.exs index 9573c40..7774ec5 100644 --- a/test/mob/renderer_test.exs +++ b/test/mob/renderer_test.exs @@ -1272,6 +1272,16 @@ defmodule Mob.RendererTest do assert is_integer(resolved) end + test "an unresolvable scrim color token logs a warning and passes through unchanged" do + log = + ExUnit.CaptureLog.capture_log(fn -> + Renderer.render(sheet_tree(%{scrim: :not_a_real_token}), :android, MockNIF) + end) + + assert log =~ "not_a_real_token" + assert set_root_json()["props"]["scrim"] == "not_a_real_token" + end + test "corner_radius resolves through radius tokens" do Mob.Theme.set(radius_md: 20) Renderer.render(sheet_tree(%{corner_radius: :radius_md}), :android, MockNIF) diff --git a/test/mob/ui_test.exs b/test/mob/ui_test.exs index cc92805..3fa5ed2 100644 --- a/test/mob/ui_test.exs +++ b/test/mob/ui_test.exs @@ -241,6 +241,17 @@ defmodule Mob.UITest do node = UI.sheet(UI.text(text: "hi")) assert Map.keys(node) |> Enum.sort() == [:children, :props, :type] end + + test "every prop sheet/2 validates as a color is one Mob.Renderer also resolves as a color" do + sheet_color_props = [:background, :scrim, :drag_indicator_color] + + for key <- sheet_color_props do + assert key in Mob.Renderer.color_props(), + "#{inspect(key)} is validated as a color by Mob.UI.sheet/2 but is missing " <> + "from Mob.Renderer.color_props/0 — it would pass validation but never " <> + "resolve a theme token at render time" + end + end end describe "sheet/2 detents" do