fix(Android): clamp unmapped sheet states instead of throwing - #4530
Open
Bram-dc wants to merge 1 commit into
Open
fix(Android): clamp unmapped sheet states instead of throwing#4530Bram-dc wants to merge 1 commit into
Bram-dc wants to merge 1 commit into
Conversation
detentIndexFromSheetState throws IllegalArgumentException when a stable BottomSheetBehavior state has no slot in the current detents array. That is reachable from ordinary prop updates: shrinking sheetAllowedDetents while the sheet rests in a detent that no longer exists leaves Material reporting the old state until the sheet is re-settled, so [0.5, 1] -> [1] while collapsed delivers STATE_COLLAPSED with a detentCount of 1. The throw happens on the main thread inside a BottomSheetBehavior callback, so it takes down the whole app. Clamp to the top-most valid detent and log a warning instead. The rewritten form sheet already treats an unmapped state as non-fatal via FORM_SHEET_UNKNOWN_DETENT_INDEX; -1 cannot be reused in this mapping because it already means STATE_HIDDEN, and reporting the sheet as dismissed would be worse than reporting the nearest detent.
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.
Description
SheetUtils.detentIndexFromSheetStatethrowsIllegalArgumentExceptionwhen a stableBottomSheetBehaviorstate has no slot in the current detents array. Because it is called from aBottomSheetBehaviorcallback on the main thread, the throw takes the whole app down.This is not a transient-state problem.
onSheetStateChangedalready filters withSheetUtils.isStateStable(newState), andSTATE_COLLAPSEDis stable. The mismatch is between the state Material still holds and a detents array that has since shrunk:sheetAllowedDetentsgoes from[0.5, 1]to[1]while the sheet is resting collapsedSTATE_COLLAPSEDuntil the sheet is re-settleddetentCount == 1branch only mapsSTATE_HIDDENandSTATE_EXPANDED, so it throwsUpdating detents in response to content height is a normal thing to do, which makes this reachable without the user doing anything unusual.
Worth noting: the rewritten form sheet has already reached the same conclusion.
FormSheetBehaviorController.mapStateToDetentIndexreturns aFORM_SHEET_UNKNOWN_DETENT_INDEXsentinel for exactly these combinations rather than throwing. This PR brings the legacy path in line with that.Reported (against the react-navigation repo, but the stack trace is this function) in react-navigation/react-navigation#12374.
Changes
detentIndexFromSheetStatenow routes every unmapped state through a new privateunmappedDetentIndex, which logs a warning and clamps to the top-most valid detent instead of throwing.@throwsbehaviour.I did not reuse
-1the way the form sheet controller does, because this mapping already spends-1onSTATE_HIDDEN. Returning it would report the sheet as dismissed, which is worse than reporting the nearest detent. Clamping todetentCount - 1keeps the sheet visible and matches where it physically is: with a single detent the only detent is the expanded one, andSTATE_HALF_EXPANDEDagainst two detents is likewise closest to the top.sheetStateFromDetentIndexis left alone. Its throws are fed by our own index arithmetic rather than by Material's state machine, so they are genuine programming errors.Before & after - visual documentation
No visual changes. The sheet renders the same; it stops crashing.
Test plan
There are no Kotlin unit tests in the repo, so this was verified by hand.
Repro, from the linked issue:
Open the form sheet, leave it at the collapsed detent, then let
needExpandflip so the detents array shrinks to[1]. Onmainthe app dies with theIllegalArgumentExceptionabove. With this change the sheet stays up, reports detent index 0, and logs:I have been running this patched into 4.26.2 in production for several weeks. It removed the crash with no behavioural fallout, which matters more than usual for us because the app hosts an AccessibilityService in the same process, so this exception was taking that down with it.
Formatting checked with
ktlint 1.3.0(the versionandroid/spotless.gradlepins) against the changed file.Checklist