diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bc20805..de046e14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,42 @@ Full module documentation: [hexdocs.pm/mob](https://hexdocs.pm/mob). --- +## [Unreleased] + +### Fixed +- **`Mob.Test.element_frames/1` no longer reports elements that are in the + render tree but not on screen.** 0.7.29 shipped a fix (MOB-102) that stopped + wiping the frame registry on every render and instead dropped only ids + absent from the incoming tree. That fixed static elements vanishing, but + "in the tree" is not "on screen": a `lazy_list` row scrolled out of range, + an inactive tab's subtree, and a dismissed sheet's content all stay in the + tree, so their last on-screen frame was reported indefinitely — and + `Mob.Test.tap_id/2` would tap whatever now occupied those coordinates. It + now returns `{:error, :not_found}` for them again, as it did before 0.7.29. + Tracked elements drop their own entry when the platform stops laying them + out, via a compare-and-delete so an outgoing screen can't remove an entry an + incoming screen just claimed under the same `:id` (MOB-103). +- **A screen animating out of a nav transition no longer re-registers itself + at mid-animation coordinates.** `set_root` applies the new tree + asynchronously on the main thread, so an outgoing screen kept reporting + frames after its ids had already been purged — including when both screens + tagged an element with the same `:id`, which tree membership alone can't + reject. Writes are now refused for ids absent from the current tree, and for + any tracker belonging to a superseded navigation (MOB-103). +- **A list delete no longer loses the frame of the element below it.** Every + `ForEach` keys children by index while the registry is keyed by `:id`, so + removing an item shifts each later id onto a different tracker; with + same-height rows nothing re-registered and the surviving element went + missing. Trackers now re-register when the `:id` beneath them changes, and + on appearance (MOB-103). + +### Changed +- `Mob.Test.element_frames/1`'s docs now state what counts as rendered, and + that a frame is a last-known position recorded at layout — poll until it + settles rather than trusting the first read after a change. + +--- + ## [0.7.29] - 2026-08-27 ### Added @@ -44,6 +80,16 @@ Full module documentation: [hexdocs.pm/mob](https://hexdocs.pm/mob). presentation anchor, not the sheet's real on-screen content, so frame tracking is skipped there rather than publishing a value known to be wrong. +- iOS: `Mob.Test.element_frames/1` no longer drops a still-visible element + that didn't move. The registry was cleared on every render on the + assumption that frame tracking would repopulate it, but tracking only + fires when an element's frame *changes*, so anything that stayed put + went missing until something moved it. Only ids absent from the incoming + tree are dropped now (MOB-102). See + `decisions/2026-08-27-frame-registry-purge-by-id.md` — and note the + Unreleased entry above, which corrects the converse case this introduced. + *(Documented after the fact: this shipped in 0.7.29 but was omitted from + its notes, so it is not in the published 0.7.29 changelog.)* ## [0.7.28] - 2026-08-26 diff --git a/decisions/2026-08-27-frame-registry-liveness.md b/decisions/2026-08-27-frame-registry-liveness.md new file mode 100644 index 00000000..389aa8a0 --- /dev/null +++ b/decisions/2026-08-27-frame-registry-liveness.md @@ -0,0 +1,179 @@ +# iOS element_frames: tree membership is not liveness + +- Date: 2026-08-27 +- Status: accepted +- Extends: `2026-08-27-frame-registry-purge-by-id.md` (that decision stands; + this adds the second half it was missing) + +## Context + +MOB-102 replaced the unconditional `mob_clear_frames()` in `nif_set_root` with +a purge-by-id: collect every `:id` in the incoming tree, drop only the registry +entries whose id fell out of it. That correctly fixed the original bug (a +static, unmoved element's frame was wiped and never repopulated, because +`MobFrameTracker` only writes on `onChange(of: geo.frame)` and an unmoved +element never fires it). + +But it swapped the predicate. `Mob.Test.element_frames/1` documents — and +`tap_id/2` depends on — *rendered on screen*. Purge-by-id implements *present +in the BEAM tree*. Those diverge in three ways. + +**1. In the tree, but not laid out.** `.lazyList` renders into a `LazyVStack`, +`MobTabView` keeps every tab's subtree in the tree while displaying one, and a +dismissed sheet's content stays mounted by design. In all three the `:id` +remains in the tree, so the purge never drops it, and `onChange` never fires +again — so the element's last on-screen frame is reported forever. Scroll a +500-row list to row 200 and `element_frames` still places `row_3` where it sat +200 rows ago; `tap_id(node, "row_3")` then taps whatever occupies those +coordinates now. Under the pre-MOB-102 wipe that was `{:error, :not_found}`, so +MOB-102 turned a safe failure into a silently wrong action. + +**2. The teardown race is narrowed, not closed.** `mob_adopt_frame_ids` runs +synchronously on the NIF thread inside `nif_set_root`, while +`MobViewModel.setRoot` is `DispatchQueue.main.async` — the main thread hasn't +applied the new root yet. Outgoing screens then animate out under +`.transition(navTransition(...))`, which is `.move` for push/pop, so their +global frames change continuously and their `GeometryReader`s keep firing +`onChange` the whole way out, re-registering at mid-animation coordinates +*after* the purge already ran. Tree membership cannot reject this when both +screens tag an element with the same `:id` — that id genuinely is in the new +tree. + +**3. Trackers are not bound to an `:id`.** Every `ForEach` in `MobRootView.swift` +keys children by index (`id: \.offset`) while the registry is keyed by `:id`. +Delete an item from a list and every later id shifts down one position, under +a tracker that keeps its view identity. If the rows are the same height the +tracker's own frame value is unchanged, so nothing fires — and the id it just +inherited keeps the previous occupant's entry, or loses it entirely to the +departing tracker's teardown. + +## Decision + +Keep purge-by-id and add the three things it can't express on its own. + +**Liveness signal: compare-and-delete on `.onDisappear`.** `mob_register_frame` +returns a monotonic write seq, which `MobFrameTracker` keeps and hands back on +`.onDisappear` via `mob_unregister_frame(id, seq)`. The entry is removed only +if that write is still the current one, so a tracker can only ever delete an +entry it still owns. A lazy row scrolling off or a tab deactivating drops its +own entry. + +This is deliberately on the *unregistration* side. All four mechanisms MOB-102 +tried and rejected were attempts to make *registration* race-proof against +removal timing; `.onDisappear` is the platform telling us an element stopped +being laid out, which is precisely the fact the registry was missing. + +**Generation gating for navigation.** A non-`"none"` transition is what makes +`MobViewModel` bump `navVersion`, and `MobRootView` keys the whole tree on +`.id(currentNavVersion)` — so every view identity is destroyed and rebuilt. +`nif_set_root` bumps a frame generation in lockstep; a tracker captures the +current generation when it appears and stamps every write with it, and writes +carrying a superseded generation are refused. That makes "the outgoing screen +may not write" true even for an `:id` both screens share, which the seq +compare-and-delete alone was silently assuming. Because the refused write +returns seq 0 and `mob_unregister_frame` no-ops on 0, the outgoing tracker's +teardown also can't delete the incoming entry. + +The generation is read through a plain C call, not `@Environment` or an +`@ObservedObject`. It is a rejection gate, not a repopulation trigger — the +distinction that separates it from MOB-102's rejected attempts 1, 2 and 4. + +**Write gating on tree membership.** `nif_set_root` retains the live id set and +`mob_register_frame` ignores writes for ids not in it. Cheaper than the +generation check and catches the non-shared-id case directly. + +**Registration that doesn't depend on `onChange` alone.** The tracker also +registers on `.onAppear`, and re-registers on `.onChange(of: id)`. The first +makes every disappear/reappear cycle self-healing, which matters because +`initial:` is not guaranteed to re-run when SwiftUI preserved the view identity +across the disappearance (a `TabView` tab demonstrably preserves `@State` +across switches). The second handles divergence 3: whichever order the +departing tracker's teardown and the inheriting tracker's re-registration land +in, the surviving element ends up correctly registered at its new position. + +Seqs live in a side table (`g_element_frame_seqs`) rather than as a fifth array +element, so `nif_element_frames` keeps JSON-encoding the registry directly and +the `{"id":[x,y,w,h]}` wire shape is unchanged. Per-tracker bookkeeping lives in +a reference box held by `@State`, not `@State` scalars: the writes happen inside +a layout-driven callback once per display frame per element during a transition, +which is the classic "modifying state during view update" shape, and the values +must stay readable during the same transaction that tears the view down. + +### Known gap + +A pure **reorder** of same-sized siblings still reports stale positions. List +`[a, b, c]` re-rendered as `[c, a, b]`: all three ids are still in the tree so +nothing is purged, no view is destroyed so no `.onDisappear` fires, and each +tracker's own frame value is unchanged, so `onChange(of: frame)` fires nowhere. +`onChange(of: id)` catches this only because the id under each index *does* +change — which it does here, so the reorder case is in fact covered by the same +mechanism as divergence 3. What is *not* covered is a reorder that leaves every +index holding the id it already had, which by definition isn't a reorder. Keying +the `ForEach`s on `nativeViewId` instead of index would make this structural +rather than incidental, and is the better long-term fix; it changes SwiftUI +identity semantics across seven call sites (affecting `@State` preservation and +animations in every list), so it wants its own change and its own device pass. + +## Consequences + +- `mob_register_frame`'s signature changed (`void` → `uint64_t`, plus a + generation parameter), so `MobDemo-Bridging-Header.h` and its one Swift + caller move together. `mob_nif.m` now imports that header so the compiler + diagnoses future drift — C has no name mangling, so before this a changed + signature linked fine and Swift read whatever was in the return register. + No generated-app template declares it (`mob_new`'s `build.zig.eex` passes + mob's header by path), so `mob_new` needs no companion change. +- `nif_element_frames` now snapshots under the lock and serializes outside it. + The main thread takes that lock on every frame write, and the docs tell + callers to poll this NIF until a frame settles. +- **Host checks.** `ios/mob_nif.m` compiles clean (`clang -fsyntax-only + -fobjc-arc` against the iOS 26.5 simulator SDK, with `MobApp-Swift.h` stubbed + since it's generated by swiftc during the real build). `test/native/` holds a + runnable harness (`make -C test/native run`) asserting twenty registry + behaviours, including both MOB-102 non-regression cases and the shared-`:id` + nav case — but it reproduces the registry functions rather than linking the + shipped ones, so it checks the algorithm, not the binary. +- **Device-verified**, via a generated probe app (300-row `:list`, plus two + screens deliberately tagging a button with the same `:id`) read over dist + with `Mob.Test.element_frames/1`. On the iPhone 17 Pro simulator (iOS 26.4) + and a physical iPhone (iOS 26.5.2): + + - *Divergence 1 — the headline.* Only the visible window is ever registered + (12 of 300 rows on the simulator, 10 on the phone), so `LazyVStack` does + destroy far-offscreen rows and `.onDisappear` does fire for them. After + scrolling well past it, `row_3` is absent on **both** targets — the exact + case that under MOB-102 alone reported a stale frame and produced a wrong + tap. Rows scrolled back into range re-register, so the cycle is + self-healing. + - *MOB-102 non-regression.* `probe_title` and `shared_btn` — static and + unmoved — survived all of that without re-registering. + - *Divergence 2 — shared `:id` across a nav push.* The incoming screen's + frame is what's reported (simulator y=512 vs the outgoing screen's y=112; + phone y=470.5 vs 70.5), stable across repeated reads seconds after the + animation, and restored exactly on pop. Neither deleted nor left at + mid-animation coordinates. + - *Backgrounding.* A background/foreground cycle leaves the registry intact. + - *`MobTabView`.* This is the case the lazy-list result does **not** settle: + a tab switching away keeps its subtree in the tree, and `TabView` preserves + view identity across switches, so `.onAppear` re-firing on re-selection was + an open question. Verified on the simulator with a real `tab_bar` render + node: with tab A active only `tab_a_marker` is registered; switching to B + drops it and registers `tab_b_marker`; switching back restores + `tab_a_marker` — repeatably, in both directions. So `.onAppear` does + re-fire on re-selection and a tab round trip is not permanently + unreportable. + + Note the reachable path is the `tab_bar` **render node** (`props.tabs` + + `props.active`), not `Mob.App.tab_bar/1` navigation. Also worth recording: + `Mob.Renderer` has no `on_tab_select` handler, so that prop reaches native + as a raw `{pid, tag}` tuple and kills the screen process on serialize — + tab selection has to be driven from Elixir through `active`. That's a + separate bug, not this change's. +- Only the `MobTabView` case was left simulator-only; the physical iPhone had + rebound dist to its USB link-local address (`169.254.1.100`), which dist + can't route to, and clearing that needs the cable physically unplugged. Every + other scenario ran on both targets and agreed exactly, and the mechanism + under test is SwiftUI behaviour rather than anything device-specific. +- `Mob.Test.element_frames/1`'s docstring now states the liveness rule, the + settle caveat (a frame is a last-known position, not a synchronous read), and + the reorder gap. diff --git a/decisions/2026-08-27-frame-registry-purge-by-id.md b/decisions/2026-08-27-frame-registry-purge-by-id.md index ffc8b74e..c48d842e 100644 --- a/decisions/2026-08-27-frame-registry-purge-by-id.md +++ b/decisions/2026-08-27-frame-registry-purge-by-id.md @@ -1,7 +1,10 @@ # iOS element_frames registry: purge-by-id, not wipe-and-repopulate - Date: 2026-08-27 -- Status: accepted +- Status: accepted; extended by `2026-08-27-frame-registry-liveness.md` + (the decision below stands, but its "nothing to race" claim is overstated — + see that file for the teardown case it misses, and for the tree-present + but not-laid-out elements purge-by-id can't see) ## Context diff --git a/ios/MobDemo-Bridging-Header.h b/ios/MobDemo-Bridging-Header.h index 77919985..b9af29b8 100644 --- a/ios/MobDemo-Bridging-Header.h +++ b/ios/MobDemo-Bridging-Header.h @@ -1,6 +1,8 @@ // MobDemo-Bridging-Header.h — Exposes Mob ObjC types to Swift. // Passed to swiftc via -import-objc-header. +#import + #import "MobNode.h" // Called from MobHostingController to signal a back gesture to the BEAM. @@ -26,7 +28,31 @@ void mob_notify_color_scheme(const char *scheme); // its on-screen frame (logical points) keyed by the element's :id. Read back via // the element_frames NIF so an agent can locate/drive elements without a // screenshot. Implemented in mob_nif.m. -void mob_register_frame(const char *id, double x, double y, double w, double h); +// +// `generation` is the value mob_frame_generation() returned when the caller +// appeared; writes stamped with a superseded generation are refused, so a +// screen animating out of a nav transition stops reporting itself. Pass 0 if +// it hasn't been captured yet and the write will be accepted. +// +// Returns a monotonic write sequence number the caller keeps so it can pair +// this write with mob_unregister_frame below; 0 means the write was rejected +// (unknown id, a superseded generation, or an id absent from the tree BEAM +// most recently sent). +uint64_t mob_register_frame(const char *id, uint64_t generation, double x, double y, double w, + double h); + +// Current frame generation — bumped on every identity-destroying navigation. +// MobFrameTracker captures this on appear and stamps its writes with it. +uint64_t mob_frame_generation(void); + +// Called from MobFrameTracker's .onDisappear when a tracked element stops being +// laid out — a lazy-list row scrolled out of range, an inactive tab's subtree — +// while its :id is still present in the BEAM tree, so nif_set_root's purge +// never drops it. `seq` is the value the matching mob_register_frame returned: +// the entry is removed only if that write is still the current one, so an +// outgoing screen can't delete an entry an incoming screen just claimed under +// the same :id. +void mob_unregister_frame(const char *id, uint64_t seq); // Called from MobRootView.swift's resolvedFont to get the ordered fallback // font names from the last Mob.Theme.set/1 (nif_set_theme in mob_nif.m diff --git a/ios/MobRootView.swift b/ios/MobRootView.swift index d24d9aac..2d348545 100644 --- a/ios/MobRootView.swift +++ b/ios/MobRootView.swift @@ -517,9 +517,23 @@ struct MobNodeView: View { // identifier and report the element's global frame (logical points) to the C // registry as it lays out / moves. Untagged nodes pass through untouched, so // there's no cost unless a dev opts an element in by giving it an :id. +// Per-tracker bookkeeping, deliberately a reference type held by @State rather +// than @State scalars. Two reasons: mutating it never invalidates the view (the +// writes happen inside a GeometryReader's layout-driven callback, once per +// display frame per element during a transition — the classic "modifying state +// during view update" shape), and the values stay readable during the same +// transaction that tears the view down, which is exactly when .onDisappear +// needs the seq. +private final class MobFrameBox { + var seq: UInt64 = 0 + var generation: UInt64 = 0 +} + private struct MobFrameTracker: ViewModifier { let node: MobNode + @State private var box = MobFrameBox() + func body(content: Content) -> some View { // A sheet's own switch-case view is a zero-size anchor used only to // attach `.sheet(isPresented:)` — its real, visible content is @@ -532,17 +546,64 @@ private struct MobFrameTracker: ViewModifier { .accessibilityIdentifier(id) .background( GeometryReader { geo in - Color.clear.onChange(of: geo.frame(in: .global), initial: true) { _, frame in - mob_register_frame( - id, Double(frame.minX), Double(frame.minY), - Double(frame.width), Double(frame.height)) - } + Color.clear + // Registration must not depend on onChange alone. + // onChange fires on frame *value* changes, so an + // element that reappears at the position it left + // (a tab switched back to, a row scrolled back into + // range) would never re-register after the + // .onDisappear below removed it — and `initial:` is + // not guaranteed to re-run when SwiftUI preserved + // the view identity across that disappearance. + // Registering on appear makes every + // disappear/reappear cycle self-healing. + .onAppear { + box.generation = mob_frame_generation() + record(id, geo.frame(in: .global)) + } + .onChange(of: geo.frame(in: .global), initial: true) { _, frame in + record(id, frame) + } + // Every ForEach in this file keys children by index + // (`id: \.offset`) while the registry is keyed by + // :id, so a tracker is NOT bound to one id for its + // lifetime — delete an item and every later id + // shifts down a position under a tracker that keeps + // its identity. Its frame value may be unchanged + // (equal-height rows), so nothing else here would + // fire and the id we just took over would keep the + // previous occupant's entry — or lose it entirely to + // the departing tracker's .onDisappear. + .onChange(of: id) { _, newId in + record(newId, geo.frame(in: .global)) + } + // Being in the BEAM tree isn't the same as being on + // screen: a LazyVStack row scrolled out of range, an + // inactive tab's subtree, or a dismissed sheet's + // content all stay in the tree (so nif_set_root's + // purge keeps them) while SwiftUI stops laying them + // out — and onChange won't fire for them again. + // Without this their last frame is reported forever + // and Mob.Test.tap_id taps whatever is there now. + .onDisappear { mob_unregister_frame(id, box.seq) } } ) } else { content } } + + private func record(_ id: String, _ frame: CGRect) { + // Capture lazily as well as in onAppear: the ordering of onAppear + // against onChange(initial: true) isn't contractual, and a write + // stamped 0 is accepted rather than refused as stale. + if box.generation == 0 { + box.generation = mob_frame_generation() + } + box.seq = mob_register_frame( + id, box.generation, Double(frame.minX), Double(frame.minY), + Double(frame.width), Double(frame.height)) + } } // ── Box ────────────────────────────────────────────────────────────────────── diff --git a/ios/mob_nif.m b/ios/mob_nif.m index 12cec61f..45c7e7e4 100644 --- a/ios/mob_nif.m +++ b/ios/mob_nif.m @@ -30,6 +30,11 @@ extern char *dlerror(void) __attribute__((weak)); #import "MobApp-Swift.h" #import "MobNode.h" +// The prototypes Swift sees for every mob_* bridge function. Imported here so +// the compiler diagnoses a definition drifting from its declaration — C has no +// name mangling, so without this a changed signature links fine and Swift reads +// whatever happens to be in the return register. +#import "MobDemo-Bridging-Header.h" #include "erl_nif.h" #import #import @@ -2000,9 +2005,10 @@ static ERL_NIF_TERM nif_set_theme(ErlNifEnv *env, int argc, const ERL_NIF_TERM a return g_font_fallback ?: @[]; } -static NSMutableDictionary *mob_frame_registry(void); // both defined with the -static void mob_purge_frames_except(NSSet *); // element frame registry below +static NSMutableDictionary *mob_frame_registry(void); // all defined with the +static void mob_adopt_frame_ids(NSSet *); // element frame registry below static NSSet *mob_collect_frame_ids(MobNode *); +static void mob_bump_frame_generation(void); static ERL_NIF_TERM nif_set_root(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { ErlNifBinary bin; @@ -2031,7 +2037,13 @@ static ERL_NIF_TERM nif_set_root(ErlNifEnv *env, int argc, const ERL_NIF_TERM ar // disappearing, and their stale frame survives. Purging by id instead // never touches a surviving element's existing entry (nothing to race), // and correctly drops one that's genuinely gone from the new tree. - mob_purge_frames_except(mob_collect_frame_ids(node)); + // + // The retained id set also gates mob_register_frame, so the outgoing + // screen can't re-register itself while it animates away (setRoot below + // is dispatched to the main thread async — the teardown animation + // outlives this call). Elements that stay in the tree but stop being + // laid out are handled separately, via mob_unregister_frame. + mob_adopt_frame_ids(mob_collect_frame_ids(node)); // Snapshot and reset the transition enif_mutex_lock(tap_mutex); @@ -2047,6 +2059,18 @@ static ERL_NIF_TERM nif_set_root(ErlNifEnv *env, int argc, const ERL_NIF_TERM ar tap_handle_next = tap_build_count; enif_mutex_unlock(tap_mutex); + // A non-"none" transition is what makes MobViewModel bump navVersion, and + // MobRootView keys the whole tree on `.id(currentNavVersion)` — so every + // view identity is destroyed and rebuilt. Bump the frame generation in + // lockstep: trackers belonging to the outgoing tree captured the old + // generation and are refused from here on, which is the only thing that + // stops them re-registering at mid-animation coordinates while they slide + // away. (`.move` transitions change their global frames continuously, so + // they keep firing onChange the whole way out; tree membership alone can't + // reject them when both screens tag the same :id.) + if (strcmp(transition, "none") != 0) + mob_bump_frame_generation(); + NSString *transitionStr = [NSString stringWithUTF8String:transition]; [[MobViewModel shared] setRoot:node transition:transitionStr]; @@ -5770,10 +5794,17 @@ static ERL_NIF_TERM nif_scroll_to(ErlNifEnv *env, int argc, const ERL_NIF_TERM a // (logical points). Recorded by MobFrameTracker; see mob_register_frame. static ERL_NIF_TERM nif_element_frames(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { NSMutableDictionary *reg = mob_frame_registry(); - NSData *jsonData = nil; + // Snapshot under the lock, serialize outside it. The main thread takes this + // same lock on every frame write — once per tracked element per display + // frame during a transition — and the docs tell callers to poll this NIF + // until a frame settles, so holding it across a JSON encode of the whole + // registry from a dirty scheduler would stall UI layout on a thread with no + // QoS relationship to it. + NSDictionary *snapshot = nil; @synchronized(reg) { - jsonData = [NSJSONSerialization dataWithJSONObject:reg options:0 error:nil]; + snapshot = [reg copy]; } + NSData *jsonData = [NSJSONSerialization dataWithJSONObject:snapshot options:0 error:nil]; if (!jsonData) return enif_make_tuple2(env, enif_make_atom(env, "error"), enif_make_atom(env, "encode_failed")); @@ -6391,30 +6422,116 @@ void mob_send_component_event(int handle, const char *event, const char *payload // thread. Both use only public APIs, so this is compiled unconditionally (the // reading NIF is still debug-gated). @synchronized guards the shared dictionary. static NSMutableDictionary *> *g_element_frames = nil; + +// Side table: id -> the seq of the write that produced g_element_frames[id]. +// Kept separate from g_element_frames so nif_element_frames can go on +// JSON-encoding that dictionary directly and the {"id":[x,y,w,h]} wire shape +// stays unchanged. +static NSMutableDictionary *g_element_frame_seqs = nil; +static uint64_t g_frame_write_seq = 0; + +// The id set from the most recent nif_set_root; nil until the first render. +// Guarded by the same lock as the registry. +static NSSet *g_live_frame_ids = nil; + +// Bumped by nif_set_root on any identity-destroying (non-"none") transition, +// in lockstep with MobViewModel's navVersion. A tracker captures the value +// current when it appeared; writes stamped with an older one are refused, so +// an outgoing screen can't keep reporting itself as it animates away. Starts +// at 1 so 0 can mean "not captured yet". +static uint64_t g_frame_generation = 1; + static dispatch_once_t g_element_frames_once; static NSMutableDictionary *mob_frame_registry(void) { dispatch_once(&g_element_frames_once, ^{ g_element_frames = [NSMutableDictionary dictionary]; + g_element_frame_seqs = [NSMutableDictionary dictionary]; }); return g_element_frames; } -void mob_register_frame(const char *id, double x, double y, double w, double h) { +// Read the current generation so a tracker can stamp its writes with the one +// that was current when it appeared. +uint64_t mob_frame_generation(void) { + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + return g_frame_generation; + } +} + +static void mob_bump_frame_generation(void) { + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + g_frame_generation++; + } +} + +uint64_t mob_register_frame(const char *id, uint64_t generation, double x, double y, double w, + double h) { if (!id) + return 0; + NSString *key = [NSString stringWithUTF8String:id]; + if (!key) + return 0; + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + // Refuse a tracker from a superseded tree. Tree membership alone can't + // do this: when the outgoing and incoming screens both tag an element + // with the same :id, that id IS in the new tree, so the outgoing + // screen's mid-animation writes would sail through the check below and + // clobber the incoming element's frame with coordinates from halfway + // off the screen. `generation == 0` means "not captured yet" and is + // allowed through, so a tracker that registers before its onAppear + // runs still records something. + if (generation && generation < g_frame_generation) + return 0; + + // Ignore writes for an id that isn't in the tree BEAM most recently + // sent. MobViewModel.setRoot dispatches to the main thread + // asynchronously, so a screen being animated out is still sliding + // offscreen well after nif_set_root purged it. + if (g_live_frame_ids && ![g_live_frame_ids containsObject:key]) + return 0; + + reg[key] = @[ @(x), @(y), @(w), @(h) ]; + uint64_t seq = ++g_frame_write_seq; + g_element_frame_seqs[key] = @(seq); + return seq; + } +} + +// Drop a tracked element's frame when it stops being laid out while its :id is +// still in the tree — a lazy-list row scrolled out of range (LazyVStack +// discards it), an inactive tab's subtree (TabView keeps every tab in the tree +// at once), a dismissed sheet's content (the sheet node stays mounted). Purging +// by id alone can't see any of these: the id is still present, so +// mob_adopt_frame_ids never drops it, and MobFrameTracker's onChange won't fire +// for it again. Its last on-screen frame would otherwise be reported forever, +// and Mob.Test.tap_id would tap whatever occupies those coordinates now. +// +// Compare-and-delete on `seq`: remove only if this caller's write is still the +// current one. An outgoing screen's .onDisappear therefore can't delete an +// entry an incoming screen just claimed under the same :id. +void mob_unregister_frame(const char *id, uint64_t seq) { + if (!id || seq == 0) return; NSString *key = [NSString stringWithUTF8String:id]; if (!key) return; NSMutableDictionary *reg = mob_frame_registry(); @synchronized(reg) { - reg[key] = @[ @(x), @(y), @(w), @(h) ]; + NSNumber *owner = g_element_frame_seqs[key]; + if (!owner || owner.unsignedLongLongValue != seq) + return; + [reg removeObjectForKey:key]; + [g_element_frame_seqs removeObjectForKey:key]; } } // Recursively collect every :id present in a freshly-parsed tree, so // nif_set_root can purge just the registry entries that fell out of the new -// tree instead of wiping everything (see mob_purge_frames_except below for +// tree instead of wiping everything (see mob_adopt_frame_ids below for // why: a wipe-everything + MobFrameTracker-repopulates design races // SwiftUI's own removal pass for an outgoing element). static void mob_collect_frame_ids_into(MobNode *node, NSMutableSet *ids) { @@ -6430,11 +6547,16 @@ static void mob_collect_frame_ids_into(MobNode *node, NSMutableSet * return ids; } -// Remove any registered frame whose id isn't in the incoming tree (called -// from nif_set_root with that tree's live id set). A surviving element's -// entry is never touched here — no race, no dependency on it re-registering -// itself — only genuinely-removed ids are dropped. -static void mob_purge_frames_except(NSSet *liveIds) { +// Adopt the incoming tree's id set (called from nif_set_root): drop every +// registered frame whose id fell out of the tree, and retain the set so +// mob_register_frame can reject writes from views that are no longer in it. +// A surviving element's entry is never touched here — no race, no dependency +// on it re-registering itself — only genuinely-removed ids are dropped. +// +// Being in the tree is necessary but not sufficient for a frame to be live: +// see mob_unregister_frame for the elements that stay in the tree but stop +// being laid out. +static void mob_adopt_frame_ids(NSSet *liveIds) { NSMutableDictionary *reg = mob_frame_registry(); @synchronized(reg) { NSMutableArray *stale = [NSMutableArray array]; @@ -6442,6 +6564,8 @@ static void mob_purge_frames_except(NSSet *liveIds) { if (![liveIds containsObject:key]) [stale addObject:key]; [reg removeObjectsForKeys:stale]; + [g_element_frame_seqs removeObjectsForKeys:stale]; + g_live_frame_ids = [liveIds copy]; } } diff --git a/lib/mob/test.ex b/lib/mob/test.ex index 5b20157a..2371c64a 100644 --- a/lib/mob/test.ex +++ b/lib/mob/test.ex @@ -1112,6 +1112,26 @@ defmodule Mob.Test do Mob.Test.element_frames(node) #=> %{"save" => {24.0, 720.0, 327.0, 48.0}, "row_3" => {0.0, 300.0, 393.0, 56.0}} + + ## What counts as "rendered" + + An element appears here once it has laid out, and is dropped when it leaves + the tree *or* stops being laid out — a lazy-list row scrolled out of range, + a tab that isn't the active one, a dismissed sheet's content. Those all stay + in the render tree while off screen, so tree membership alone would report + them; they're dropped on the platform's own disappear signal instead. + + A frame is a *last known* position, not a synchronous read: it's recorded as + the element lays out. After a render that moves an element, there's a brief + window before the next layout pass where the previous frame is still what's + reported. If you've just triggered a change and are about to act on the + result, poll until the frame settles rather than trusting the first read. + + On iOS a frame is refreshed when the element appears, when the `:id` at that + position changes, or when the element's own frame *value* changes — so a + rearrangement that moves ids between same-sized slots without moving any slot + can briefly report a stale position. For list content where that's a risk, + confirm against `render_tree/1` or a screenshot before acting on coordinates. """ @spec element_frames(node()) :: %{optional(String.t()) => {float(), float(), float(), float()}} | {:error, term()} diff --git a/test/native/.gitignore b/test/native/.gitignore new file mode 100644 index 00000000..76203f51 --- /dev/null +++ b/test/native/.gitignore @@ -0,0 +1 @@ +frame_registry_test diff --git a/test/native/Makefile b/test/native/Makefile new file mode 100644 index 00000000..69c85afa --- /dev/null +++ b/test/native/Makefile @@ -0,0 +1,22 @@ +# Native-side checks that can run on the host (no simulator, no XCTest target). +# +# make run build + run everything here +# make clean +# +# See frame_registry_test.m for what this does and does not cover. + +CC ?= clang +CFLAGS ?= -fobjc-arc -Wall -Wextra -Wno-unused-parameter +LDFLAGS ?= -framework Foundation + +BINS := frame_registry_test + +.PHONY: run clean +run: $(BINS) + @for b in $(BINS); do echo "== $$b"; ./$$b || exit 1; done + +frame_registry_test: frame_registry_test.m + $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@ + +clean: + rm -f $(BINS) diff --git a/test/native/frame_registry_test.m b/test/native/frame_registry_test.m new file mode 100644 index 00000000..2f009e60 --- /dev/null +++ b/test/native/frame_registry_test.m @@ -0,0 +1,219 @@ +// Frame-registry semantics check (MOB-102 / MOB-103). +// +// make -C test/native run +// +// There is no XCTest target in this repo and `ios/mob_nif.m` can't be compiled +// on the host (it needs UIKit and the swiftc-generated MobApp-Swift.h), so the +// registry functions are reproduced here verbatim from the "Element frame +// registry" section of ios/mob_nif.m. That makes this a check of the ALGORITHM +// — ownership, generation gating, purge, and the MOB-102 non-regression case — +// not of the shipped binary. Keep the copies in sync; they're small and change +// rarely, and a drift shows up as an assertion that no longer describes the +// real code. +// +// It does NOT cover the SwiftUI half, which is where the real risk lives: +// whether .onDisappear actually fires for a LazyVStack row leaving the render +// window or a TabView tab switching away, and whether onChange(initial:) +// re-fires on reappearance. Those need a device or simulator — see +// decisions/2026-08-27-frame-registry-liveness.md. +#import + +static NSMutableDictionary *> *g_element_frames = nil; +static NSMutableDictionary *g_element_frame_seqs = nil; +static uint64_t g_frame_write_seq = 0; +static NSSet *g_live_frame_ids = nil; +static uint64_t g_frame_generation = 1; +static dispatch_once_t g_element_frames_once; + +static NSMutableDictionary *mob_frame_registry(void) { + dispatch_once(&g_element_frames_once, ^{ + g_element_frames = [NSMutableDictionary dictionary]; + g_element_frame_seqs = [NSMutableDictionary dictionary]; + }); + return g_element_frames; +} + +static uint64_t mob_frame_generation(void) { + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + return g_frame_generation; + } +} + +static void mob_bump_frame_generation(void) { + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + g_frame_generation++; + } +} + +static uint64_t mob_register_frame(const char *id, uint64_t generation, double x, double y, + double w, double h) { + if (!id) + return 0; + NSString *key = [NSString stringWithUTF8String:id]; + if (!key) + return 0; + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + if (generation && generation < g_frame_generation) + return 0; + if (g_live_frame_ids && ![g_live_frame_ids containsObject:key]) + return 0; + + reg[key] = @[ @(x), @(y), @(w), @(h) ]; + uint64_t seq = ++g_frame_write_seq; + g_element_frame_seqs[key] = @(seq); + return seq; + } +} + +static void mob_unregister_frame(const char *id, uint64_t seq) { + if (!id || seq == 0) + return; + NSString *key = [NSString stringWithUTF8String:id]; + if (!key) + return; + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + NSNumber *owner = g_element_frame_seqs[key]; + if (!owner || owner.unsignedLongLongValue != seq) + return; + [reg removeObjectForKey:key]; + [g_element_frame_seqs removeObjectForKey:key]; + } +} + +static void mob_adopt_frame_ids(NSSet *liveIds) { + NSMutableDictionary *reg = mob_frame_registry(); + @synchronized(reg) { + NSMutableArray *stale = [NSMutableArray array]; + for (NSString *key in reg) + if (![liveIds containsObject:key]) + [stale addObject:key]; + [reg removeObjectsForKeys:stale]; + [g_element_frame_seqs removeObjectsForKeys:stale]; + g_live_frame_ids = [liveIds copy]; + } +} + +// ── harness ────────────────────────────────────────────────────────────────── + +static int g_failures = 0; + +static void check(BOOL cond, const char *what) { + printf("%s %s\n", cond ? " ok " : "FAIL", what); + if (!cond) + g_failures++; +} + +static BOOL present(const char *id) { + return mob_frame_registry()[[NSString stringWithUTF8String:id]] != nil; +} + +static double yOf(const char *id) { + NSArray *f = mob_frame_registry()[[NSString stringWithUTF8String:id]]; + return f ? [f[1] doubleValue] : -1; +} + +static void reset(void) { + [mob_frame_registry() removeAllObjects]; + [g_element_frame_seqs removeAllObjects]; + g_live_frame_ids = nil; + g_frame_generation = 1; +} + +int main(void) { + @autoreleasepool { + uint64_t g = mob_frame_generation(); + + // 1. A row that scrolls out of range drops its own entry. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"row_3" ]]); + uint64_t s = mob_register_frame("row_3", g, 0, 300, 393, 56); + check(s != 0, "register returns a non-zero seq"); + check(present("row_3"), "registered row is present"); + mob_unregister_frame("row_3", s); + check(!present("row_3"), "MOB-103: offscreen lazy row is dropped on disappear"); + + // 2. An outgoing screen must not delete an entry the incoming screen + // just claimed under the same :id. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"save" ]]); + uint64_t outgoing = mob_register_frame("save", g, 24, 720, 327, 48); + uint64_t incoming = mob_register_frame("save", g, 24, 640, 327, 48); + check(outgoing != incoming, "second write gets a distinct seq"); + mob_unregister_frame("save", outgoing); + check(present("save"), "MOB-103: stale owner's disappear does NOT delete the new entry"); + check(yOf("save") == 640.0, "the incoming screen's frame is the one retained"); + mob_unregister_frame("save", incoming); + check(!present("save"), "the current owner CAN delete its own entry"); + + // 3. A screen animating out after the purge can't re-register itself. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"screen_a_btn" ]]); + mob_register_frame("screen_a_btn", g, 10, 10, 100, 40); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"screen_b_btn" ]]); // nav push + check(!present("screen_a_btn"), "outgoing screen's id purged by adopt"); + check(mob_register_frame("screen_a_btn", g, -393, 10, 100, 40) == 0, + "MOB-103: mid-animation re-register from a dead id is rejected"); + check(!present("screen_a_btn"), "...and leaves no stale offscreen entry"); + + // 4. The shared-:id nav case — the one tree membership alone can't + // reject, because the id IS in the new tree. The outgoing screen's + // generation is superseded, so its slide-out writes are refused and + // its .onDisappear (seq 0) is a no-op. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"save" ]]); + uint64_t oldGen = mob_frame_generation(); + mob_register_frame("save", oldGen, 24, 720, 327, 48); + mob_bump_frame_generation(); // nav push: non-"none" transition + uint64_t newGen = mob_frame_generation(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"save" ]]); // still present + uint64_t incomingSeq = mob_register_frame("save", newGen, 24, 640, 327, 48); + check(incomingSeq != 0, "incoming screen registers under the new generation"); + uint64_t lateSeq = mob_register_frame("save", oldGen, -393, 720, 327, 48); + check(lateSeq == 0, "MOB-103: outgoing screen's slide-out write is refused (shared id)"); + check(yOf("save") == 640.0, "the incoming screen's frame survives the animation"); + mob_unregister_frame("save", lateSeq); // lateSeq == 0 → no-op + check(present("save"), "MOB-103: refused write leaves seq 0, so disappear can't delete"); + + // 5. MOB-102 must not regress: a static, unmoved element survives an + // unrelated re-render without re-registering. An ordinary re-render + // is transition "none", so the generation does NOT move. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"static", @"sibling" ]]); + uint64_t sg = mob_frame_generation(); + mob_register_frame("static", sg, 5, 5, 50, 20); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"static", @"sibling" ]]); + check(present("static"), "MOB-102 preserved: unmoved element survives a re-render"); + check(mob_register_frame("static", sg, 5, 5, 50, 20) != 0, + "MOB-102 preserved: a survivor's later writes are still accepted"); + + // 6. List delete: index-keyed ForEach shifts ids under trackers, so the + // tracker that inherits an id re-registers it (onChange(of: id)). + // The departing tracker's disappear must not win. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"a", @"b" ]]); + uint64_t lg = mob_frame_generation(); + mob_register_frame("a", lg, 0, 50, 393, 50); + uint64_t bSeq = mob_register_frame("b", lg, 0, 100, 393, 50); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"b" ]]); // "a" deleted + uint64_t bReclaimed = mob_register_frame("b", lg, 0, 50, 393, 50); // idx0 takes over "b" + mob_unregister_frame("b", bSeq); // idx1 tears down with its old seq + check(present("b"), "MOB-103: surviving element keeps its entry after a list delete"); + check(yOf("b") == 50.0, "...at its NEW position, not the previous occupant's"); + check(bReclaimed != bSeq, "the re-registration took ownership"); + + // 7. Purge clears the seq side table too. + reset(); + mob_adopt_frame_ids([NSSet setWithArray:@[ @"tmp" ]]); + mob_register_frame("tmp", mob_frame_generation(), 1, 1, 1, 1); + mob_adopt_frame_ids([NSSet set]); + check(g_element_frame_seqs[@"tmp"] == nil, "purge drops the seq side-table entry too"); + + printf("\n%s (%d failure%s)\n", g_failures ? "FAILED" : "ALL PASSED", g_failures, + g_failures == 1 ? "" : "s"); + return g_failures ? 1 : 0; + } +}