fix: don't react to snap point changes if container height is 0 #855
Conversation
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Not resolved, please don't close. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
bump |
|
hi @simon-abbott thanks for submitting this pr, could you provide a repo video records of this issue or a sample code for this issue. i have used this library with navigations but never faced this issue 🤔 |
|
@gorhom Well, in trying to record the issue I was unable to reproduce it. Seems the issue has been fixed in some other update of some dependency. If it re-occurs I'll reopen this PR. |
|
@gorhom Unfortunately the problem did resurface, but this time I have proof: https://github.com/simon-abbott/BottomSheetMRE, and I can confirm that this patch still fixes the problem. This video was taken on an LG G7 ThinQ running Android 9. XRecorder_04052022_120447.mp4 |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Not stale, unfortunately. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Not stale. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Not stale |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
not stale |
|
looking into this PR now |
In React Native 65 the 'OnSnapPointChange' reaction sometimes fires when
a view is going out of sight. Specifically it fires *after* the
`animatedContainerHeight` has been set to `0`. This causes the computed
snap points to all be `0` themselves, which, in turn, causes the `index`
calculation to select the last item in the snap point array. All of this
is not really an issue except that if the view then comes *back* into
view without being recreated (like if a user presses 'back' on a
navigation stack) the index is preserved when rendering the recomputed
now-non-zero snap points, so the sheet appears to snap to the fully open
position.
Here is an example series of events:
1. The BottomSheet is created with defined snap points `["25%", "75%"]`
and index `0`.
2. The view loads with a height of `1000`.
3. The snap points are correctly calculated to be `[750, 250]`
4. The sheet is rendered at height `750`
5. The view is dismissed by navigating to another screen
6. The `animatedContainerHeight` is calculated to be `0`
7. The snap points are calculated to be `[0, 0]`
8. The `OnSnapPointChange` reaction is fired, which tries to animate to
the nearest snap point, which is at position `0`. Since both snap
points are `0` it picks the last one, so `index` becomes `1`, and
position is set to `0`.
9. The navigation stack is popped, and the original View is displayed
again.
10. The `animatedContainerHeight` is once again calculated to be `1000`
11. The snap points are again calculated to be `[750, 250]`.
12. The `OnSnapPointChange` reaction is fired, which again animates to
the nearest snap point. Since step 8 set the position to `0`, the
nearest snap point is always the last one, which in this case has
index `1` (height = `250`), and renders as fully open.
This patch fixes this issue by only running the `OnSnapPointChange`
reaction when `animatedContainerHeight > 0`. If the height is zero then
the internal state of the sheet does not matter since it won't be
rendered anyway. With this change the aforementioned issue is resolved.
Specifically, step 8 is skipped which means that step 12 runs against
the last _valid_ position, and the sheet stays where it should.
gorhom
force-pushed
the
fix-snapping-open
branch
from
8da5959
to
34961e1
Compare
|
@simon-abbott thanks for submitting this PR, this issue is weird, you may need to cut an issue ticket to react navigation repo too |
Motivation
In React Native 65 the 'OnSnapPointChange' reaction sometimes fires when a view is going out of sight. Specifically it fires after the
animatedContainerHeighthas been set to0. This causes the computed snap points to all be0themselves, which, in turn, causes the new calculated position to also be0. All of this is not really an issue except that if the view then comes back into view without being recreated (like if a user presses 'back' on a navigation stack) the position is preserved when rendering with the recomputed now-non-zero snap points, which causes the sheet to snap to the fully open position.Here is an example series of events:
["25%", "75%"]and index0.1000.[750, 250]750animatedContainerHeightis calculated to be0[0, 0]OnSnapPointChangereaction is fired, which tries to animate to the nearest snap point, which is at position0. Since both snap points are0it picks the last one, soindexbecomes1, and position is set to0.animatedContainerHeightis once again calculated to be1000[750, 250].OnSnapPointChangereaction is fired, which again animates to the nearest snap point. Since step 8 set the position to0, the nearest snap point is always the last one, which in this case has index1(height =250), and renders as fully open.This patch fixes this issue by only running the
OnSnapPointChangereaction whenanimatedContainerHeight > 0. If the height is zero then the internal state of the sheet does not matter since it won't be rendered anyway. With this change the aforementioned issue is resolved. Specifically, step 8 is skipped which means that step 12 runs against the last valid position (750in the example), and the sheet stays where it should.