Skip to content
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

fix(iOS): modal view flickering #1870

Merged
merged 15 commits into from
Nov 15, 2023

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Aug 17, 2023

Description

Should be merged to #1852 or first rebased and then merged to main after #1852.

Closes #1722

So the exact roots of the issue are unclear & obfuscated. It seems that it might be related to following (not 100% sure):

  1. It looks like during animation viewDidLayoutSubviews is being called which in turn calls setSize:forView: on UIManager. This triggers Yoga layout underneath which sets view dimensions to the target values (end animation values) resulting in view clipping during the animation. There are two more important facts:

    a. its hard to determine whether its Yoga who sets invalid value or it gets invalid value from us (passed in updateBounds method after viewDidLayoutSubviews is triggered by system.

    b. when uimanager is not notified of bounds size change everything works fine

I've considered various approaches:

  1. Do not pass the value to UIManager when animation is ongoing. Presence of animation was detected by checking animationKeys property of view's layer. This still resulted in visual glitches. Moreover if I sent the final value after animation finish (via completion block) it resulted in content jumping (see here).
  2. Use CADisplayLink & report to UIManager bounds size from presentationLayer (that should be (almost) accurate value), but it still resulted in visual glitches (even when sliding down), both flickering and content jumping or just content had wrong top offset / padding.
  3. Do not notify UIManager at all on bounds change.

Changes

I went with this approach for now. That is: I do not notify uimanager on bounds change when stackPresentation == formSheet. This is wild I know. I experimented a bit trying to find out what did it broke, but I did not find anything on my toy example (Test1649), however it is highly unlikely that such approach does not have negative impact, but I believe it is better that way, than having formsheets unusable due to this flickering.

Test code and steps to reproduce

Test1649

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@kkafar kkafar changed the base branch from main to @kkafar/custom-detents August 17, 2023 11:56
@dylancom
Copy link
Contributor

Is the current code safe to use as a patch?
Just discovered this bug in my apps...

@grahammendick
Copy link
Contributor

Hey, I just added Bottom Sheet support to my Navigation router on iOS. I removed the flicker you're seeing when the sheet is dropped. Details in the PR but let me know if you want to know more

@kkafar
Copy link
Member Author

kkafar commented Nov 13, 2023

Hey @grahammendick, great to hear that you've managed to find a solution to this. I took a look at your PR description and code and my understanding of your solution is as follows:

  1. You create CADisplayLink and register it to be notified each time system is updating the screen.
  2. When notified by the system, you retrieve accurate frame information from CALayer of the view, as presentationLayer should be up to date with animation state.
  3. You delegate setting the view size to React by calling setSize:forView:, which should set appropriate value in next render cycle.

Is that correct & full picture?

What worries me is that calling setSize:forView triggers react / Yoga layout under the hood, from which I received invalid values in the past (values that the view should have after the animation has finished), thus my first impression is that might be not really smooth, but I haven't tested it, so I'm especially curious of your response.

My approach was to reject frame updates coming from React during animation, but this led to "modal content jumping after the animation finish"...

Thank you for sharing your solution with us!

@grahammendick
Copy link
Contributor

Yea, that's almost the full picture.

I don't think the invalid values come from setSize. It seems that the 'invalid' values come from iOS. When the sheet is dropped, iOS calls viewDidLayoutSubviews with the bounds set to rest state of the drop. So if it's dropped when dragging down, iOS calls viewDidLayoutSubviews with the collapsed bounds. iOS doesn't call viewDidLayoutSubviews with any of the interpolated values of the animation.

So, the solution is to ignore viewDidLayoutSubviews and to get the accurate values from the CALayer. This gives a smooth animation when the sheet is dropped. But if we only used the CALayer then we wouldn't have a smooth animation when the sheet is dragged. It's better to use viewDidLayoutSubviews when the sheet is dragged.

@kkafar kkafar force-pushed the @kkafar/custom-detents branch from d1fc618 to 3ba3d43 Compare November 14, 2023 11:47
@kkafar kkafar force-pushed the @kkafar/custom-detents-flickering-fix branch from 2de9953 to c0b445b Compare November 14, 2023 14:35
@kkafar kkafar merged commit c74b2d1 into @kkafar/custom-detents Nov 15, 2023
1 check passed
@kkafar kkafar deleted the @kkafar/custom-detents-flickering-fix branch November 15, 2023 17:03
kkafar added a commit that referenced this pull request Feb 21, 2024
## Description

Should be merged to #1852 or first rebased and then merged to main after
#1852.

Closes #1722 

So the exact roots of the issue are unclear & obfuscated. It seems that
it might be related to following (not 100% sure):

1. It looks like during animation `viewDidLayoutSubviews` is being
called which in turn calls `setSize:forView:` on UIManager. This
triggers Yoga layout underneath which sets view dimensions to the target
values (end animation values) resulting in view clipping during the
animation. There are two more important facts:

a. its hard to determine whether its Yoga who sets invalid value or it
gets invalid value from us (passed in `updateBounds` method after
`viewDidLayoutSubviews` is triggered by system.

b. when uimanager is not notified of bounds size change everything works
fine

I've considered various approaches:

1. Do not pass the value to UIManager when animation is ongoing.
Presence of animation was detected by checking `animationKeys` property
of view's layer. This still resulted in visual glitches. Moreover if I
sent the final value after animation finish (via completion block) it
resulted in content jumping (see
[here](facebook/react-native#34834 (comment))).
2. Use `CADisplayLink` & report to UIManager bounds size from
`presentationLayer` (that should be (almost) accurate value), but it
still resulted in visual glitches (even when sliding down), both
flickering and content jumping or just content had wrong top offset /
padding.
3. Do not notify UIManager at all on bounds change. 


## Changes

I went with this approach for now. That is: I do not notify uimanager on
bounds change when `stackPresentation == formSheet`. This is wild I
know. I experimented a bit trying to find out what did it broke, but I
did not find anything on my toy example (Test1649), however it is highly
unlikely that such approach does not have negative impact, but I believe
it is better that way, than having formsheets unusable due to this
flickering.


## Test code and steps to reproduce

Test1649

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
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.

3 participants