Skip to content

Conversation

kligarski
Copy link
Contributor

@kligarski kligarski commented May 22, 2025

Description

Changing detents on iOS form sheets would not work correctly if sheetInitialDetent was set to value other than 0. For some reason, initial detent was set on every update instead of being set only once.

Fixes #2543.

Changes

  • add _hasInitialDetentSet flag and use it to determine whether to set initial detent
  • add Test2543 test screen and e2e test

Screenshots / GIFs

before after
2543_before.mp4
2543_after.mp4

First example works on both videos, second one only after fix.

Test code and steps to reproduce

Run example app, open Test2543.

Checklist

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

@kligarski kligarski requested a review from kkafar May 22, 2025 10:25
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks!

@kligarski kligarski requested a review from kkafar May 22, 2025 10:36
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@kligarski kligarski merged commit c188d35 into main May 22, 2025
8 checks passed
@kligarski kligarski deleted the @kligarski/issue-2543 branch May 22, 2025 11:39
@lodev09
Copy link

lodev09 commented May 23, 2025

@kligarski

For some reason, initial detent was set on every update instead of being set only once.

I think the main culprit here is this block:

- (void)finalizeUpdates:(RNComponentViewUpdateMask)updateMask
{
[super finalizeUpdates:updateMask];
#if !TARGET_OS_TV && !TARGET_OS_VISION
[self updateFormSheetPresentationStyle];
#endif // !TARGET_OS_TV
}

This gets called everytime you drag, causing performance issues. hence the jumpy glitch. Moving it to updateProps should fix the issue.

I've added this fix to my PR #2902 since it caused an issue with the translation value I wanted to add. Also, we wanted to actually use updateFormSheetPresentationStyle when sheetInitialDetentIndex is changed in order for us to change detent index programatically.

lodev09 added a commit to lodev09/react-native-screens that referenced this pull request May 23, 2025
kligarski added a commit that referenced this pull request Jun 2, 2025
…eProps (#2952)

## Description

Previously, `updateFormSheetPresentationStyle` was called from
`finalizeUpdates` every time it was called, which would run many times
when the sheet was being dragged. This impacted performance and was the
cause of the bug which was fixed in
#2935 by
introducing a flag for setting initial detent. Now, we are checking
`updateMask` in `finalizeUpdates` method. If the props have changed, we
call `updateFormSheetPresentationStyle`. We don't need the flag anymore
so I removed it as well.

Thanks to @lodev09 for [reporting the problem and suggesting possible
solution](#2935 (comment)).

## Changes

- remove `_sheetHasInitialDetentSet`
- check if props have changed using `updateMask` before calling
`updateFormSheetPresentationStyle`

## Test code and steps to reproduce

Run formSheet-related test screens in the example app, such as
`Test2543`, `TestFormSheet` (you can change `sheetInitialDetent` in the
file and save to see the update), `Test2877`.

## Checklist

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

Jumpy behaviour when setting sheetInitialDetentIndex to other then 0.
3 participants