From 1cf75e5f73270aabdc4b39f35372fee546d7b0a0 Mon Sep 17 00:00:00 2001 From: Bao Nguyen Date: Sun, 27 Sep 2026 20:44:37 +0700 Subject: [PATCH] fix(android): apply index after the update's detents Fabric hands a props update to the view manager in folly::dynamic map order, not in the order the props were written. When one render grows detents and moves index onto a new detent, index can arrive first, fall outside the old detents and be dropped. Defer index to onAfterUpdateTransaction so it always sees the update's detents, the same order iOS applies them in. --- .../reactnativebottomsheet/BottomSheetView.kt | 9 ++ .../BottomSheetViewManager.kt | 9 +- .../BottomSheetViewManagerCloseRequestTest.kt | 4 +- .../BottomSheetViewManagerIndexTest.kt | 141 ++++++++++++++++++ 4 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerIndexTest.kt diff --git a/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetView.kt b/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetView.kt index f4d07d91..2eacac06 100644 --- a/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetView.kt +++ b/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetView.kt @@ -128,6 +128,15 @@ class BottomSheetView(context: Context) : ReactViewGroup(context), LifecycleEven fun setIndex(newIndex: Int) = host.setIndex(newIndex) + /** Set by the view manager and applied once its whole props update has been processed. */ + internal var pendingIndex: Int? = null + + internal fun applyPendingIndex() { + val index = pendingIndex ?: return + pendingIndex = null + setIndex(index) + } + var animateIn: Boolean get() = host.animateIn set(value) { diff --git a/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManager.kt b/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManager.kt index 4cea849b..a9478042 100644 --- a/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManager.kt +++ b/android/src/main/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManager.kt @@ -121,7 +121,14 @@ class BottomSheetViewManager : @ReactProp(name = "index") override fun setIndex(view: BottomSheetView, index: Int) { - view.setIndex(index) + // Props within an update arrive in no particular order, so apply the index after the update's + // detents, as iOS does. Otherwise an index that targets a newly added detent is dropped. + view.pendingIndex = index + } + + override fun onAfterUpdateTransaction(view: BottomSheetView) { + super.onAfterUpdateTransaction(view) + view.applyPendingIndex() } @ReactProp(name = "animateIn") diff --git a/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerCloseRequestTest.kt b/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerCloseRequestTest.kt index 6bd306ce..8b2bfacd 100644 --- a/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerCloseRequestTest.kt +++ b/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerCloseRequestTest.kt @@ -15,6 +15,7 @@ import com.facebook.react.bridge.JavaOnlyMap import com.facebook.react.bridge.ReactContext import com.facebook.react.bridge.WritableMap import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.ReactStylesDiffMap import com.facebook.react.uimanager.ThemedReactContext import com.facebook.react.uimanager.UIManagerHelper import com.facebook.react.uimanager.events.BatchEventDispatchedListener @@ -63,7 +64,8 @@ class BottomSheetViewManagerCloseRequestTest { JavaOnlyMap.of("value", 300.0, "kind", "points", "programmatic", false), ), ) - manager.setIndex(sheet, 1) + // The manager applies index at the end of a props update. + manager.updateProperties(sheet, ReactStylesDiffMap(JavaOnlyMap.of("index", 1.0))) activity.setContentView(sheet) layout(sheet) reactContext.onHostResume(activity) diff --git a/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerIndexTest.kt b/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerIndexTest.kt new file mode 100644 index 00000000..3321b318 --- /dev/null +++ b/android/src/test/java/com/swmansion/reactnativebottomsheet/BottomSheetViewManagerIndexTest.kt @@ -0,0 +1,141 @@ +package com.swmansion.reactnativebottomsheet + +import android.graphics.Color +import android.os.Looper +import android.view.MotionEvent +import android.view.View +import androidx.activity.ComponentActivity +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.facebook.react.bridge.JavaOnlyArray +import com.facebook.react.bridge.JavaOnlyMap +import com.facebook.react.bridge.ReadableMap +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.ReactStylesDiffMap +import java.util.AbstractMap +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config + +// Fabric applies an update's props in the order of its folly::dynamic map, which is not the +// order they were written in JS, so these updates are fed to the manager in an explicit order. +@RunWith(AndroidJUnit4::class) +@Config(sdk = [35]) +class BottomSheetViewManagerIndexTest { + @Before + fun useLocalReactNativeFeatureFlags() { + ReactNativeFeatureFlagsForTests.setUp() + } + + @Test + fun `index targeting a detent added in the same update is applied in either prop order`() { + for (indexFirst in listOf(true, false)) { + withSheet(detents(0.0, 300.0), index = 0) { manager, sheet, _ -> + val grow = listOf("detents" to detents(0.0, 300.0, 600.0), "index" to 2.0) + manager.updateProperties(sheet, props(if (indexFirst) grow.reversed() else grow)) + + assertTrue("indexFirst=$indexFirst", host(sheet).isCloseRequestTargetResolvedAndOpen) + } + } + } + + @Test + fun `a later update without index does not replay the previous index`() { + withSheet(detents(0.0, 300.0), index = 1) { manager, sheet, listener -> + manager.updateProperties( + sheet, + props(listOf("index" to 2.0, "detents" to detents(0.0, 300.0, 600.0))), + ) + tapScrim(sheet) + assertEquals(listOf(0), listener.indexChanges) + + manager.updateProperties(sheet, props(listOf("animateContentHeight" to false))) + + assertFalse(host(sheet).isCloseRequestTargetResolvedAndOpen) + } + } + + private fun withSheet( + initialDetents: JavaOnlyArray, + index: Int, + block: (BottomSheetViewManager, BottomSheetView, IndexRecordingListener) -> Unit, + ) { + val controller = Robolectric.buildActivity(ComponentActivity::class.java).setup() + val manager = BottomSheetViewManager() + val listener = IndexRecordingListener() + val sheet = BottomSheetView(controller.get()).apply { this.listener = listener } + try { + manager.setScrimColor(sheet, Color.BLACK) + manager.updateProperties( + sheet, + props( + listOf( + "modal" to true, + "animateIn" to false, + "detents" to initialDetents, + "index" to index.toDouble(), + ) + ), + ) + controller.get().setContentView(sheet) + layout(sheet) + block(manager, sheet, listener) + } finally { + sheet.destroy() + shadowOf(Looper.getMainLooper()).idle() + controller.close() + } + } + + private fun host(sheet: BottomSheetView) = sheet.getChildAt(0) as BottomSheetHostView + + private fun detents(vararg values: Double) = + JavaOnlyArray.from( + values.map { JavaOnlyMap.of("value", it, "kind", "points", "programmatic", false) } + ) + + private fun props(entries: List>) = ReactStylesDiffMap(OrderedProps(entries)) + + private fun tapScrim(sheet: BottomSheetView) { + val down = MotionEvent.obtain(1_000L, 1_000L, MotionEvent.ACTION_DOWN, 10f, 100f, 0) + val up = MotionEvent.obtain(1_000L, 1_010L, MotionEvent.ACTION_UP, 10f, 100f, 0) + sheet.dispatchTouchEvent(down) + sheet.dispatchTouchEvent(up) + down.recycle() + up.recycle() + } + + private fun layout(view: View) { + view.measure( + View.MeasureSpec.makeMeasureSpec(1080, View.MeasureSpec.EXACTLY), + View.MeasureSpec.makeMeasureSpec(1920, View.MeasureSpec.EXACTLY), + ) + view.layout(0, 0, 1080, 1920) + shadowOf(Looper.getMainLooper()).idle() + } +} + +private class OrderedProps(private val entries: List>) : + ReadableMap by JavaOnlyMap.of(*entries.flatMap { it.toList() }.toTypedArray()) { + override val entryIterator: Iterator> + get() = entries.map { AbstractMap.SimpleEntry(it.first, it.second) }.iterator() +} + +private class IndexRecordingListener : BottomSheetViewListener { + val indexChanges = mutableListOf() + + override fun onIndexChange(index: Int) { + indexChanges.add(index) + } + + override fun onSettle(index: Int) = Unit + + override fun onPositionChange(position: Double, index: Double) = Unit + + override fun onCloseRequest() = Unit +}