Skip to content

review(#18): align the @selectionChange contract across iOS and Android#23

Open
shanerbaner82 wants to merge 1 commit into
feat/text-input-selectionfrom
review/selection-contract-parity
Open

review(#18): align the @selectionChange contract across iOS and Android#23
shanerbaner82 wants to merge 1 commit into
feat/text-input-selectionfrom
review/selection-contract-parity

Conversation

@shanerbaner82

@shanerbaner82 shanerbaner82 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review follow-ups for #18, targeting feat/text-input-selection so they can be merged into it (or cherry-picked) rather than filed as comments. cc @simonhamp

The design in #18 is right — riding TEXT_CHANGE with a text_selection kind is zero-wire-change and the feature-off paths really are cheap early returns. Everything below is edge-case cleanup, and the four changes are independent.

1. iOS: report an end-caret event on programmatic value pushes

Today the same event produces three different behaviors:

behavior on a value push
Android (all 3 renderers) selectionReporter.flush(pushed) — always fires, (text, len, len)
iOS, unfocused nothing.onChange(of: text) is gated on isFocused and selection is nil
iOS, focused the stale cached selStart/selEnd clamped to the new length, not end-of-text

That lands right on the mention-typeahead flow the PR pitches, where the handler rewrites the bound model. iOS now emits the end-caret event from .onChange(of: serverValue), matching Android; the .onChange(of: text) pass that follows recomputes the identical payload and is dropped by the existing dedupe.

2. Both: unify selection_debounce_ms, default 50ms → 150ms

PHP only serializes the prop when configured, so natively "absent" and an explicit 0 are indistinguishable. Android's if (it > 0) it else 50 mapped both to 50ms; iOS's getInt(default: 50) kept 0 and emitted immediately. Both now treat <= 0 as "use the default".

Positive values are floored at one frame (16ms). Nothing clamped the low end before, so selection-debounce-ms="1" was legal and bought a bridge frame plus a full component re-render every millisecond of caret drag.

The 150ms default is the one opinionated change in this PR — isolated to a pair of named constants if you'd rather keep 50ms. The reasoning: every event carries the entire field text and costs a Livewire render plus a native tree diff, so 50ms is a lot to spend by default for a feature whose typical consumer (a mention typeahead) is human-paced.

3. PHP: never serialize on_selection_change for a secure input

Both renderers already refuse to emit, but that leaves a privacy-relevant invariant restated in every renderer — including any future one — with no test covering it. Suppressing in resolveProps() makes it unleakable regardless of what the native side does with the prop, and assertable from PHP. ->secure(false) still registers; three tests cover it.

4. Docs and two comments that describe something other than what the code does

README gains a contract section: programmatic pushes, discontiguous selections reported as a span that covers their gaps (so mb_substr($text, $start, $end - $start) includes unselected text), and the read-only asymmetry — Android reports selection on read-only fields for copy support, iOS can't because read-only implies .disabled. Plus a callout that every event ships the full text independent of the native:model sync mode, so pairing @selectionChange with .blur/.debounce does not keep field contents off the wire the way the sync mode otherwise would.

Two comment corrections:

  • BaseTextInput::applyAttributes describes the _selectionChange self-wire as a fallback for "cores that predate the text_selection callback kind". That case can't occur — such a core doesn't rewrite the @selectionChange directive either, so _selectionChange never reaches the element. What the self-wire actually buys is decoupling from the collector half, which is what this repo's CI builds against (tests.yml targets dev-element, and origin/element's NativeElementCollector has no _selectionChange handling). Worth stating plainly, because it means it('wires _selectionChange through the collector…') isn't testing the collector — it would stay green even if the collector change in feat(edge): @selectionChange event delivers text-input caret/selection to PHP mobile-air#221 never merged. Renamed accordingly.
  • scalarOffset claims its clamp "keeps a momentarily-stale index from trapping distance". The clamp bounds range, not scalar alignment. Swift rounds rather than traps for a misaligned index in the scalar view, so the worst case is an off-by-one, not a crash — the code is fine, the comment just promises more than it delivers.

Not addressed here

  • The ^4.0 constraint. The README's arity warning is really a dependency-resolution problem: once feat(edge): @selectionChange event delivers text-input caret/selection to PHP mobile-air#221 lands, composer.json should pin a version that can't resolve to a core lacking the kind. Moot if both merge before 4.0.0 stable — worth confirming that's the plan rather than leaving it to a README note.
  • cappedTo and surrogate pairs. text.take(maxLength) is UTF-16 units, so an over-long paste can sever a pair and leave a lone surrogate that codePointCount then counts as one. Pre-existing semantics, but the offsets now flow to PHP where mb_substr on the mangled UTF-8 may misalign. Low priority; didn't want to change max_length behavior in a review PR.
  • Neither renderer change is typechecked. tests.yml does run a swiftc -parse pass over resources/ios/*.swift — but parse-only doesn't resolve types, so API misuse and availability errors sail through it, and (per the job's own comment) Kotlin has no parse-only mode and isn't checked at all. I verified the new SwiftUI surface by hand against the iOS 26.5 SDK .swiftinterface: TextSelection is @available(iOS 18.0, …) and conforms to Equatable/Hashable (so .onChange(of: selection) is valid), the .selection/.multiSelection cases match, and TextField<S: StringProtocol>(_:text:selection:prompt:axis:) exists with axis: Axis? = nil — so both the single-line and axis: .vertical call sites resolve against the 18.2 deployment target. That's the API surface holding up, not a build; a real device build is still the gate.

Verification

Pint clean. Pest not run locally — that needs composer install inside a plugin repo, which the plugins-tree rules forbid — so CI is the check on the three new tests.

🤖 Generated with Claude Code

Follow-ups from reviewing #18. Four independent changes:

1. iOS: report an end-caret event on programmatic value pushes.
   Android's value-sync effect flushes `(text, len, len)` unconditionally;
   iOS emitted nothing when unfocused (the text path is gated on
   `isFocused`) and, when focused, emitted the STALE cached `selStart`/
   `selEnd` clamped to the new length. Three behaviors for one event —
   and it lands squarely on the mention-typeahead flow the PR pitches,
   where the handler rewrites the bound model. iOS now matches Android.

2. Both: unify `selection_debounce_ms` resolution, and raise the default
   50ms -> 150ms. PHP only serializes the prop when configured, so
   "absent" and an explicit 0 are indistinguishable natively; Android
   mapped both to 50ms while iOS honored 0 as "emit immediately". Both
   now treat <= 0 as "use the default" and floor positive values at one
   frame (16ms) — nothing clamped the low end before, so
   `selection-debounce-ms="1"` bought a bridge frame plus a full
   component re-render every millisecond of caret drag. The 150ms default
   is the one opinionated change here: every event carries the full field
   text and costs a re-render, so 50ms is a lot to spend by default.

3. PHP: never serialize `on_selection_change` for a `secure` input.
   Both renderers already refuse to emit, but that leaves a
   privacy-relevant invariant restated in every renderer, including
   future ones, with no test covering it. Suppressing at the source makes
   it unleakable and assertable. `->secure(false)` still registers.

4. Docs/comments. README gains a contract section (programmatic pushes,
   discontiguous selections spanning their gaps, the read-only
   asymmetry) and a callout that every event ships the full text
   independent of the `native:model` sync mode — so pairing
   `@selectionChange` with `.blur`/`.debounce` does not keep the field
   contents off the wire. Also corrects two comments that describe
   something other than what the code does: the `applyAttributes`
   self-wire is not an old-core fallback (a core without the precompiler
   change never produces `_selectionChange`) but a decoupling from the
   collector half, which is what this repo's CI actually builds against;
   and `scalarOffset`'s clamp bounds range, not scalar alignment. The
   collector test is renamed for the same reason — it cannot distinguish
   the collector path from the self-wire.

Pint clean. Pest not run locally (needs composer inside a plugin repo);
CI covers it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant