fix(iOS): flickering custom header items #2247

alduzy · 2024-07-15T13:08:28Z

Description

This PR intents to fix flickering custom header items when going to a previous screen on fabric architecture.
The items are unmounted before the transition happens when POP action is dispatched on navigation from JS causing the items to vanish for a moment.
The adopted solution uses snapshots of the custom items to be used until the transition's done.

Fixes #2243.

Changes

added snapshots of the custom header items
modified Test556 for repro

Screenshots / GIFs

Before

Screen.Recording.2024-07-15.at.15.06.27.mov

After

Screen.Recording.2024-07-15.at.15.04.32.mov

Test code and steps to reproduce

Use Test556 repro

Checklist

Ensured that CI passes

ios/RNSScreenStackHeaderConfig.mm

apps/src/tests/Test556.tsx

alduzy · 2024-07-18T17:27:39Z

I added a container to both header elements in repro to make sure the nested views are considered during snapshot.

kkafar

I have tested these changes together with @alduzy and I'm not sure why this change works and whether the fix will be stable.

On Fabric the views are unmounted bottom-up, implying that from PoV of given component while removing a child view component A, the children of A are already unmounted. We have positively verified that this is the case. in - (void)unmountChildComponentView:index: method UIView.subviews array of RNSScreenStackHeaderSubview is already empty.

However when making a snapshot inside this method the just-removed-children are still captured and visible on the resulting snapshot.

We take the snapshot with the - (UIView *)snapshotViewAfterScreenUpdates:(BOOL)afterUpdates; which documentation states following (I emphasised important parts):

afterUpdates
A Boolean value that specifies whether the snapshot should be taken after recent changes have been incorporated. Pass the value NO to capture the screen in its current state, which might not include recent changes.

Return Value
A new view object based on a snapshot of the current view’s rendered contents.

[...] Because the content is captured from the already rendered content, this method reflects the current visual appearance of the view and is not updated to reflect animations that are scheduled or in progress. However, calling this method is faster than trying to render the contents of the current view into a bitmap image yourself.

The documentation indicates that that the snapshot is created based on lower-level internal structure (some kind of bitmap), when afterUpdates == NO the snapshot is created without taking into account ongoing / pending changes. Hence we can conclude, and that is my hypothesis, that the state with already unmounted child views has not been rendered yet (by render I mean OS level render to the screen) and thus we can still see them.

I do not understand fully how rendering loop, transition start / end is realised by iOS and when we can rely on the unmounted views being still rendered and available for snapshotting. Thus I'm not sure how stable the patch (this PR changes) is. I think we should test this in debug and release mode @alduzy.

This patch also gives a nudge, that we might be able to do something similar on the level of the screenstack, since all mounting code is being done synchronously on the UI thread (but I'm not sure how rendering relates to that) @WoLewicki. I'll try to test this today & tomorrow alongside the useLayoutEffect approach on RN 0.75.

kkafar · 2024-07-23T07:24:19Z

Okay, after research I think I have understanding why it works and can conclude that my hypothesis was correct. I'll try to describe the mechanism behind it in #2261

kkafar

Few change requests, but overall solution is good. I think we'll proceed with merging this since we know understand why does it work. See my previous comments on this PR & description of #2261

apps/src/tests/Test556.tsx

ios/RNSScreenStackHeaderConfig.mm

…2261) ## 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](#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 #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

…planation of why this works

kkafar

Pushed few changes as I want this commit to be included in upcoming release.

…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

## Description This PR intents to fix flickering custom header items when going to a previous screen on `fabric` architecture. The items are unmounted before the transition happens when `POP` action is dispatched on navigation from JS causing the items to vanish for a moment. The adopted solution uses snapshots of the custom items to be used until the transition's done. Fixes software-mansion#2243. ## Changes - added snapshots of the custom header items - modified `Test556` for repro ## Screenshots / GIFs ### Before https://github.com/user-attachments/assets/9da060ed-b65e-4b32-9ab1-debfc2bfd02d ### After https://github.com/user-attachments/assets/0413fab0-05f6-4e55-adab-f283e01bc551 ## Test code and steps to reproduce - Use `Test556` repro ## Checklist - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>

alduzy requested review from tboba, WoLewicki and kkafar July 15, 2024 13:08

alduzy mentioned this pull request Jul 16, 2024

feat: refactor snapshots when going back on Fabric #2134

Merged

tboba reviewed Jul 16, 2024

View reviewed changes

ios/RNSScreenStackHeaderConfig.mm Outdated Show resolved Hide resolved

ios/RNSScreenStackHeaderConfig.mm Outdated Show resolved Hide resolved

alduzy requested a review from tboba July 16, 2024 16:37

WoLewicki reviewed Jul 18, 2024

View reviewed changes

apps/src/tests/Test556.tsx Outdated Show resolved Hide resolved

alduzy added 10 commits July 19, 2024 15:22

Updated Test556 repro

4052c5f

Added header title snapshot

b144514

Prevent left and right item flicker

66719c3

Move snapshot logic to header config

9033a57

Removed window != nil check

ab7d9e0

Remove unwanted change

f1d6023

Remove unwanted change

5a1eee9

Set views to snapshot before removing anything

af8765f

Nesting header items in repro

6772645

Added switch statement

99d3ffe

alduzy force-pushed the @alduzy/fabric-header-flicker-fix branch from b21c8da to 99d3ffe Compare July 19, 2024 13:23

alduzy changed the base branch from @wolewicki/fix-android-fabric-goback to main July 19, 2024 13:23

remove unwanted changes

efc85a0

kkafar reviewed Jul 22, 2024

View reviewed changes

kkafar mentioned this pull request Jul 23, 2024

refact(iOS, Fabric): take snapshot in unmountChildComponent:index: #2261

Merged

1 task

kkafar requested changes Jul 23, 2024

View reviewed changes

apps/src/tests/Test556.tsx Outdated Show resolved Hide resolved

apps/src/tests/Test556.tsx Outdated Show resolved Hide resolved

apps/src/tests/Test556.tsx Outdated Show resolved Hide resolved

ios/RNSScreenStackHeaderConfig.mm Outdated Show resolved Hide resolved

kkafar added 3 commits July 24, 2024 13:47

Upgrade example by adding contrast & different text on different screens

cd495e3

Change "any" type to a navigation prop

779fd7c

Add comments on what code needs to be kept in sync & with links to ex…

1f55775

…planation of why this works

kkafar approved these changes Jul 24, 2024

View reviewed changes

shonore mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.33.0 shonore/Fickle#1609

Open

2lambda123 mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.32.0 to 3.33.0 2lambda123/abp#163

Merged

Cabreira97 mentioned this pull request Aug 23, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.33.0 Cabreira97/Connect-Tech#5

Closed

2lambda123 mentioned this pull request Aug 26, 2024

[Snyk] Upgrade react-native-screens from 3.33.0 to 3.34.0 2lambda123/abp#164

Merged

shonore mentioned this pull request Sep 3, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.34.0 shonore/Fickle#1669

Open

undrthegraveyard mentioned this pull request Sep 4, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.34.0 undrthegraveyard/gigslick#5

Closed

asraful-cse mentioned this pull request Sep 6, 2024

[Snyk] Upgrade react-native-screens from 3.11.1 to 3.34.0 asraful-cse/planet_app_react_native#412

Open

This was referenced Sep 22, 2024

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#18

Open

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#19

Open

Ayush-Bulbule mentioned this pull request Oct 2, 2024

[Snyk] Upgrade react-native-screens from 3.22.1 to 3.34.0 Ayush-Bulbule/FoodleApp#5

Merged

tobistudio mentioned this pull request Oct 3, 2024

[Snyk] Upgrade react-native-screens from 3.27.0 to 3.34.0 tobistudio/React_Native_Audio#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS): flickering custom header items #2247

fix(iOS): flickering custom header items #2247

alduzy commented Jul 15, 2024 •

edited

Loading

alduzy commented Jul 18, 2024

kkafar left a comment •

edited

Loading

kkafar commented Jul 23, 2024

kkafar left a comment

kkafar left a comment

fix(iOS): flickering custom header items #2247

fix(iOS): flickering custom header items #2247

Conversation

alduzy commented Jul 15, 2024 • edited Loading

Description

Changes

Screenshots / GIFs

Before

After

Test code and steps to reproduce

Checklist

alduzy commented Jul 18, 2024

kkafar left a comment • edited Loading

Choose a reason for hiding this comment

kkafar commented Jul 23, 2024

kkafar left a comment

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

alduzy commented Jul 15, 2024 •

edited

Loading

kkafar left a comment •

edited

Loading