fix(ios): improve focus requests and focus-state handling - #2
SynurDevelopers wants to merge 9 commits into
Conversation
…try on attach UIKit honors setNeedsFocusUpdate only when the environment contains the currently focused item, so requests on nearest-ancestor controllers were silently dropped (nested controllers, react-native-screens). All focus requests now go through RNCEKVKeyboardFocusService, preferring the key window root. Imperative focus() on a detached view records a pending request replayed on window attach. Also collapses the duplicated autofocus branches in didMoveToWindow, revalidates the deferred autofocus block against a generation counter after recycling, and enables the directional order guides via a descendant check instead of a first-subview pointer comparison. The text-input wrapper additionally gains the hasOnFocusChanged emission gate ahead of the base-class gate removal in the focus-event commit; the interim tree is double-gated with unchanged behavior.
All focus delegates held plain-strong back-pointers to their host views while the views strongly own the delegates, making every keyboard view immortal; the back-pointers are now zeroing-weak. The view controller's custom focus view association is stored through a weak holder, skipped and self-cleared when off-window, so controllers no longer retain unmounted subtrees or steer later focus updates to stale views. Group entry/exit boundary views latched in the order-linking singleton are now weak.
FocusTrap now diffs forceLock/lockDisabled and requests focus only when the lock becomes active, so prop commits and disable transitions no longer steal keyboard or VoiceOver focus. Tabbing into an ordered group suppresses UIKit's default move instead of double-focusing, and latched entry/exit boundaries are revalidated against the window before use. The native focus handler chain (context-menu registration) no longer depends on a JS listener being attached — only event emission is gated. Blur is emitted even after the tracked focused child deallocates, and moves between descendants of one wrapper no longer emit duplicate focus events.
Hydrates the dormant ExternalKeyboardExampleTests target with unit tests covering focus change events, focus delegates, keyboard focus service, lock view, and retain cycles. Adds a setup script to enable coverage on the test run and updates the Podfile/project to wire the target into the example workspace.
… window root - Readiness for imperative keyboard focus is now window-based: the request base parks when the controller or window is missing, and the text-input wrapper additionally parks until its native child exists; both replay from didMoveToWindow. - RNCEKVKeyboardFocusService resolves the routing controller from the target view's own window root first, then the key-window root, then the supplied fallback, and returns the controller it routed to. - Detach and cleanReferences now clear the controller's preferred-focus target when it still points at the view's own request, so recycled or navigated-away views cannot be revived as stale preferred targets. - screenReaderFocus gets the same park-and-replay as keyboard focus, so both halves of the JS focus() call survive a pre-attach request. - A same-generation autofocus that is skipped while detached returns its attempt, so the next attach retries instead of losing autofocus. - The text-input wrapper now inherits the focus delegate's tracked focus state machine (single focus per wrapper-level entry, blur after the tracked child deallocates) instead of the descendant-only checks.
…roup endpoint cleanup - Both lock-view request guards now reject any inactive or disabled state (matching onAccessibilityFocusChanged:), and the Fabric prop diff applies lockDisabled before forceLock so a compound activate-and-disable commit never passes through a momentarily-active state. didMoveToWindow doubles as the attach replay for an active trap whose early request had no controller. - Directional-guide enablement uses nearest-wrapper ownership: a nested order-group wrapper (or a focused wrapper itself) owns its focus, so a parent's guides no longer activate for a nested wrapper's focus. - RNCEKVOrderRelationship.clear also nils its entry/exit endpoints, so emptying a group releases its cached boundaries on the unlink path.
The branch was developed and validated with a full XCTest suite (97 unit tests over the changed focus-path methods, plus coverage and mutation scoring). It lives in history: 6d0b46a adds the suite and test target, 1bfc9c9 and 94115dc extend it alongside the fixes they verify. Since the project has no existing native test infrastructure, this commit removes the suite, the test-target project changes, and the Podfile/Podfile.lock edits from the PR tip to keep the reviewable diff limited to the library sources. Revert this commit to restore the complete, passing test setup.
|
@greptile please review. |
Greptile SummaryThe PR revises iOS keyboard-focus routing and lifecycle handling without changing the JavaScript API or Android implementation.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant View as Focusable View
participant Service as KeyboardFocusService
participant Controller as Window Root Controller
participant UIKit as UIKit Focus Engine
Caller->>View: focus request
alt View is not ready
View->>View: retain pending request
UIKit-->>View: didMoveToWindow
View->>View: replay request
end
View->>Service: focus(target, fallback)
Service->>Controller: rncekvFocusView(target)
Controller->>UIKit: setNeedsFocusUpdate / updateFocusIfNeeded
UIKit-->>View: didUpdateFocusInContext
View->>View: update native wrapper focus state
Reviews (3): Last reviewed commit: "fix: clear stale pending focus request a..." | Re-trigger Greptile |
There was a problem hiding this comment.
🟡 Changes recommended
Focus routing documentation/behavior mismatch and stale “pending focus” flags can cause incorrect routing or unexpected request replay across attachments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves iOS physical-keyboard focus reliability across lifecycle edges (attachment, navigation, Fabric recycling) by centralizing focus routing, retaining focus requests until views are ready, and tightening focus-state / ordering / locking behavior without changing the JS/TS public API.
Changes:
- Introduces
RNCEKVKeyboardFocusServiceand updates multiple call sites to route focus via the most appropriate controller (ideally the target view’s window root). - Adds “pending focus” replay behavior for focus requests made before a view is ready/attached; clears routed preferred-focus targets on detach/recycle to avoid stale controller state.
- Converts multiple delegate back-pointers and order-group boundaries to
weakreferences and adjusts ordering/focus-state logic to reduce duplicate focus side effects.
File summaries
| File | Description |
|---|---|
| ios/Views/RNCEKVTextInputFocusWrapper/RNCEKVTextInputFocusWrapper.mm | Retains/replays focus requests until controller/window/child are ready; clears routed preferred-focus on detach/recycle; gates JS focus events. |
| ios/Views/RNCEKVExternalKeyboardView/RNCEKVExternalKeyboardView.mm | Gates JS focus-change emission by hasOnFocusChanged while preserving internal focus-state handling. |
| ios/Views/RNCEKVExternalKeyboardLockView/RNCEKVExternalKeyboardLockView.mm | Prevents repeated focus side effects on unchanged props; orders prop application to avoid transient focus stealing; routes focus via focus service. |
| ios/Views/Base/FocusRequest/RNCEKVViewFocusRequestBase.mm | Adds pending request retention/replay, deferred autofocus invalidation via generation, and clears preferred-focus targets on detach. |
| ios/Views/Base/FocusOrderGroup/RNCEKVViewOrderGroupBase.mm | Adjusts “focused” detection to respect nearest wrapper ownership; routes focus via focus service. |
| ios/Views/Base/FocusChange/RNCEKVViewFocusChangeBase.mm | Updates focus-change propagation so native focus-state updates don’t depend on a JS listener. |
| ios/Services/RNCEKVKeyboardOrderManager/RNCEKVOrderRelationship/RNCEKVOrderRelationship.mm | Clears cached entry/exit boundaries when relationships are emptied. |
| ios/Services/RNCEKVKeyboardOrderManager/RNCEKVOrderRelationship/RNCEKVOrderRelationship.h | Makes ordered-group entry/exit boundaries weak to avoid retaining stale views. |
| ios/Services/RNCEKVKeyboardFocusService.mm | Adds focus:withFallback: and updates focus routing to prefer target view’s window/controller context. |
| ios/Services/RNCEKVKeyboardFocusService.h | Documents new focus routing API that returns the controller used for routing. |
| ios/features/Halo/delegate/RNCEKVHaloDelegate.mm | Converts delegate back-pointer to weak to avoid retain cycles/stale references. |
| ios/Extensions/UIViewController+RNCEKVExternalKeyboard.mm | Stores preferred focus target via weak holder, purges detached targets, and updates swizzled preferred-focus environments. |
| ios/Extensions/UIViewController+RNCEKVExternalKeyboard.h | Changes rncekvCustomFocusView contract to weak to prevent retaining focused views. |
| ios/Delegates/RNCEKVGroupIdentifierDelegate/RNCEKVGroupIdentifierDelegate.mm | Converts delegate back-pointer to weak. |
| ios/Delegates/RNCEKVFocusSequenceDelegate/RNCEKVFocusSequenceDelegate.mm | Routes focus via focus service; treats redirected movement as handled; clears detached order boundaries. |
| ios/Delegates/RNCEKVFocusLinkDelegate/RNCEKVFocusLinkDelegate.mm | Converts delegate back-pointer to weak. |
| ios/Delegates/RNCEKVFocusDelegate/RNCEKVFocusDelegate.mm | Improves wrapper focus tracking so descendant changes don’t spam focus events and blur can be reported after target dealloc. |
Review details
Suppressed comments (1)
ios/Views/Base/FocusRequest/RNCEKVViewFocusRequestBase.mm:74
- Similar to
_pendingFocusRequest,_pendingScreenReaderFocusis never cleared on the successful path whenscreenReaderFocusis called directly (not viadidMoveToWindow). This can replay an old accessibility focus request on a future attachment.
- (void)screenReaderFocus {
if (self.window == nil) {
_pendingScreenReaderFocus = YES;
return;
}
dispatch_async(dispatch_get_main_queue(), ^{
UIView *focusView = [self getFocusTargetView];
UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification,
focusView);
});
}
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eviewed] A focus request parked while the view was partially ready was never cleared by a later successful focus, causing a stale replay on the next window attach.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
@greptile please review |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes multiple interdependent iOS focus/attachment/recycling code paths (including preferred-focus routing and lifecycle replay) and the Legacy Bridge path was not compiled/run per the PR description.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Tracks upstream issue #125.
Summary
This PR improves physical-keyboard focus on iOS, especially during navigation, native view attachment, and Fabric recycling.
It routes focus requests through a more reliable focus environment, retains requests until views are ready, and fixes related ownership, focus-locking, ordering, and event-state problems found while investigating upstream issue #125.
The final diff changes 17 files under
ios/. It does not change the JavaScript API, TypeScript source, codegen specifications, or Android implementation.The shared native paths used by Fabric and the Legacy Bridge were updated. Local native tests covered Fabric; the Legacy Bridge configuration was not compiled or run.
What changed
Focus requests and autofocus
RNCEKVKeyboardFocusServiceas the shared entry point for programmatic keyboard focus.Ownership and ordering
orderIndexgroup reports the redirected movement as handled, preventing UIKit from performing a second movement.Focus locking and focus state
FocusTrapnow avoids repeating focus side effects when unchanged props are reapplied.onFocusChangelistener.enableContextMenucan therefore follow native focus without requiringonFocusChange.Known limitations
Several uncommon lifecycle cases remain:
RCTKeyWindow().FocusTrapprop updates can still pass through a briefly active state. The public JavaScriptFocusTrapavoids most of these transitions by rendering a plainViewwhile inactive.orderIdcan still leave subscribers registered after unmount.Validation
Completed:
git diff --checkpasses.The temporary test infrastructure was removed from the final diff in
8ef3723to keep the PR focused on the library changes. The test history, commit references, and restoration instructions are documented in this PR comment.Not completed: