-
-
Notifications
You must be signed in to change notification settings - Fork 533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refact(iOS, Fabric): take snapshot in unmountChildComponent:index:
#2261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
kkafar
changed the title
refact(iOS, Fabric): take snapshot in unmountChildComponent:index:
refact(iOS, Fabric): take snapshot in Jul 23, 2024
unmountChildComponent:index:
WoLewicki
approved these changes
Jul 23, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2 tasks
This was referenced Aug 23, 2024
This was referenced Aug 27, 2024
This was referenced Sep 24, 2024
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…oftware-mansion#2261) ## Description > [!note] This PR applies to iOS only. Ok, so this PR is related to software-mansion#2247 & to get broader context I highly recommend to read [this comment](software-mansion#2247 (review)) at the very minimum. ### Issue context On Fabric during JS initialised Screen dismissal (view removing in general) children are unmounted before their parents, thus when dismissing screen from screen stack & starting a dismiss transition all Screen content is already removed & we're animating only a blank screen resulting in visual glitch. ### Current approaches Right now we're utilising `RCTMountingTransactionObserving` protocol, filter all mounting operations *before* they are applied and if screen dismissal is to be done, we take a snapshot of to-be-removed-screen. ### Alternative approaches software-mansion#2134 sets mounting coordinator delegate and effectively does the same as the current approach, however it can also be applied to Android. ### Proposed approach On iOS we can utilise the platform & how it works. Namely the fact of unmounting child view does not impact the hardware buffer, nor bitmap layer immediately, thus we can take the snapshot simply in `- [RNSScreen unmountChildComponentView: index:]` and the children will still be visible. This approach is safe and reliable, because: ##### 10k feet explanation Drawing is not performed immediately after an update to UIKit model (such as removing a view), the system handles all operations pending on main queue and just after that it schedules drawing. We're removing the views & making snapshot in the middle of block execution on the main thread, thus the drawing can't happen and just-unmounted-views will be visible on the snapshot. ##### More detailed explanation 1. the main thread run loop of Cocoa application drains the main queue till it's empty [[1]](https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html) [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) [[5]](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/RunLoopManagement/RunLoopManagement.html#//apple_ref/doc/uid/10000057i-CH16-SW1) 2. CoreAnimation framework integrates with the main run loop by registering an observer and listening for `kCFRunLoopBeforeWaiting` event (so after the main queue is drained & run loop is to become idle due to no more pending tasks). [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) 3. CoreAnimation is responsible for applying all transactions from the last loop pass & sending them to render server (this happens on main thread), which in turn finally leads up to the changes being applied, drawn & displayed (this happens on different threads). [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) [[3]](https://developer.apple.com/library/archive/documentation/WindowsViews/Conceptual/ViewPG_iPhoneOS/WindowsandViews/WindowsandViews.html#//apple_ref/doc/uid/TP40009503-CH2-SW1) 4. [We know that the RN's mounting stage will be executed on main thread](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm#L258), because UIKit is thread-safe only in selected parts and requires calling from the main thread. 5. Single RN transaction is a complete diff between ["rendered tree" & "next tree"](https://reactnative.dev/architecture/render-pipeline#phase-2-commit-1) and is performed [atomically & synchronously](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/ReactCommon/react/renderer/mounting/TelemetryController.cpp#L18-L51) on [main thread](https://github.com/facebook/react-native/blob/91ecd7eb5330f5d725f5587744713064d614a6b3/packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm#L258), thus whole batch of updates will be finished before drawing instructions will be send to render server. #### Reference: [[1]](https://opensource.apple.com/source/CF/CF-1153.18/CFRunLoop.c.auto.html) (Look for `__CFRunLoopDoBlocks(...)` & `__CFRunLoopRun(...)` functions) Important thing to notice if `__CFRunLoopDoBlocks` is that it locks the `rl` (run loop) lock, takes & copies reference to the list of the blocks to execute, clears the original list of blocks and releases the `rl` lock. Thus only the "already scheduled" blocks are executed in the single pass of this function. It is called multiple times in the single pass of the run loop, but I haven't dug deeper, it should be enough for our use case that we have guarantee that all the blocks are drained. [[2]](https://fabernovel.github.io/2021-01-04/uikit-rendering-part-3) (Blog post on rendering in UIKit) [[3]](https://developer.apple.com/library/archive/documentation/WindowsViews/Conceptual/ViewPG_iPhoneOS/WindowsandViews/WindowsandViews.html#//apple_ref/doc/uid/TP40009503-CH2-SW1) (Apple docs - The View Drawing Cycle section) [[4]](https://bou.io/RunRunLoopRun.html) (Blog post on the run loop) [[5]](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/RunLoopManagement/RunLoopManagement.html#//apple_ref/doc/uid/10000057i-CH16-SW1) (Apple docs on run loops) ## Changes * Snapshot is not done in `unmountChildComponentView: index:` & only when needed. * Removed old mechanism * Removed now unused implementation of `RCTMountingObserving` protocol ## Test code and steps to reproduce Run any example on Fabric, push a screen, initiate go-back via JS (e.g. by clicking a button with `navigation.goBack()` action), see that the screen transitions correctly (the content is visible throughout transition) ## Checklist - [x] Ensured that CI passes
WoLewicki
added a commit
that referenced
this pull request
Nov 14, 2024
## Description Based on Expensify/App#49937 (comment) and the comments above, PR fixes the unnecessary checks for creating snapshot on the view on dismiss. It was refactored already in #2134 and #2261 (👏 to @kkafar). Those checks do nothing now since each screen is responsible for making its own snapshot. Having those checks can only lead to problems.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Note
This PR applies to iOS only.
Ok, so this PR is related to #2247 & to get broader context I highly recommend to read this comment at the very minimum.
Issue context
On Fabric during JS initialised Screen dismissal (view removing in general) children are unmounted before their parents, thus when dismissing screen from screen stack & starting a dismiss transition all Screen content is already removed & we're animating only a blank screen resulting in visual glitch.
Current approaches
Right now we're utilising
RCTMountingTransactionObserving
protocol, filter all mounting operations before they are applied and if screen dismissal is to be done, we take a snapshot of to-be-removed-screen.Alternative approaches
#2134 sets mounting coordinator delegate and effectively does the same as the current approach, however it can also be applied to Android.
Proposed approach
On iOS we can utilise the platform & how it works. Namely the fact of unmounting child view does not impact the hardware buffer, nor bitmap layer immediately, thus we can take the snapshot simply in
- [RNSScreen unmountChildComponentView: index:]
and the children will still be visible.This approach is safe and reliable, because:
10k feet explanation
Drawing is not performed immediately after an update to UIKit model (such as removing a view), the system handles all operations pending on main queue and just after that it schedules drawing. We're removing the views & making snapshot in the middle of block execution on the main thread, thus the drawing can't happen and just-unmounted-views will be visible on the snapshot.
More detailed explanation
kCFRunLoopBeforeWaiting
event (so after the main queue is drained & run loop is to become idle due to no more pending tasks). [2]Reference:
[1] (Look for
__CFRunLoopDoBlocks(...)
&__CFRunLoopRun(...)
functions)Important thing to notice if
__CFRunLoopDoBlocks
is that it locks therl
(run loop) lock, takes & copies reference to the list of the blocks to execute, clears the original list of blocks and releases therl
lock. Thus only the "already scheduled" blocks are executed in the single pass of this function. It is called multiple times in the single pass of the run loop, but I haven't dug deeper, it should be enough for our use case that we have guarantee that all the blocks are drained.[2] (Blog post on rendering in UIKit)
[3] (Apple docs - The View Drawing Cycle section)
[4] (Blog post on the run loop)
[5] (Apple docs on run loops)
Changes
unmountChildComponentView: index:
& only when needed.RCTMountingObserving
protocolTest code and steps to reproduce
Run any example on Fabric, push a screen, initiate go-back via JS (e.g. by clicking a button with
navigation.goBack()
action), see that the screen transitions correctly (the content is visible throughout transition)Checklist