Repository navigation
Conversation
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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Android, a render that adds a detent and moves
indexonto it in the same update can leave the sheet where it was. The example app already does this: in "Dynamic detent updates", tap "Shorten detents and select last index" and then "Restore content detent and select it". That second button callssetIndex(2)and brings back the'content'detent in the same render. On Android the sheet can stay on the middle detent while the screen reportsindex: 2.The cause is prop order. Fabric gives a props update to the view manager in the key order of the update's
folly::dynamicobject, and that is not the order the props were written in JS.BottomSheetViewManager.setIndexhands the value straight toBottomSheetHostView.setIndex, which ignores an index outside the current detents:So when
indexcomes beforedetents, index 2 is checked against the old two detents and dropped. Thedetentssetter that runs next keeps the old target. Nothing applies the index again until another update includesindex.I checked the order against folly itself (Homebrew folly 2026.01.12, the same
F14NodeMapthat backsdynamic::object). I inserted keys the wayjsi::dynamicFromValuedoes, in JS property order:In an NDEBUG build, a small update like these comes out of
items()in reverse insertion order, soindexcomes beforedetents. Without NDEBUG, F14 shuffles the order on purpose, and it changed from run to run, so debug builds hit this only some of the time.getPropsinFabricMountingManager.cppsendsnewProps->rawPropsas is,ReadableNativeMap.importKeyswalksitems(), andViewManager.updatePropertiescalls the setters in that order.iOS is not affected, because
BottomSheetComponentView updateProps:sets detents and then index explicitly. This change gives Android the same order.setIndexnow only records the value, andonAfterUpdateTransactionapplies it once the whole update has been processed.onAfterUpdateTransactionruns after every props batch, on both the create path and the update path. React Native's ownBaseViewManageruses the same hook for transform props, whose setters also depend on each other's order. The pending value is cleared once it is applied, so a later update withoutindexdoes not apply it again.Tests: the new
BottomSheetViewManagerIndexTestpasses updates throughViewManager.updatePropertieswith an explicit key order, becauseJavaOnlyMapis aHashMapand happens to putdetentsfirst. The first test grows[0, 300]at index 0 to[0, 300, 600]at index 2, withindexfirst and then withdetentsfirst, and checks that the target is now an open detent. Without the fix it fails forindexFirst=true. The second test covers the cleanup. After that kind of update, a scrim tap closes the sheet (onIndexChange(0)). A later update that contains onlyanimateContentHeightmust not reopen it.BottomSheetViewManagerCloseRequestTestcalledmanager.setIndexdirectly, outside a props update, so it now sends that one prop throughupdateProperties. #56 also edits that test file, so whichever lands second will have a one-line conflict there. The Android unit suite goes from 76 to 78 tests, all passing.assembleDebugAndroidTest, lint and typecheck pass.One behavior change to be aware of: when an update shortens
detentsand sets an index equal to the clamped target, Android now does what iOS does. The detent refresh clamps the target and animates to it, and theindexthat follows is a no-op. Before this change, a release build appliedindexfirst. By my reading of the code it also emitted anonSettlefor that case, and that no longer happens. I have not run this on a device or emulator (none available here). The evidence is the folly ordering above, the code path, and the unit tests.