material-components-android
material-components-android copied to clipboard
[BottomSheetBehavior] Check for `shouldSkipHalfExpandedWhenDragging` after nested scrolling
Make BottomSheetBehavior check the experimental shouldSkipHalfExpandedWhenDragging flag before setting the state to HALF_EXPANDED.
The code changes the following behaviors in onStopNestedScroll:
- In the case of
dy > 0, the state will be set toSTATE_EXPANDEDinstead ofSTATE_HALF_EXPANDEDwhen the flag is enabled. - In the case of
dy == 0, the state will be set toSTATE_EXPANDEDwhen above the half-expanded offset, andSTATE_COLLAPSEDwhen below it when the flag is enabled. - In the state of
dy < 0, the state will be set toSTATE_COLLAPSEDinstead ofSTATE_HALF_EXPANDEDwhen the flag is enabled.
Please let me know if the behavior here should be tweaked. It was difficult to understand the logic of the method so I may have made a mistake.
Also see #2874.
Okay, simplified those if statements @drchen.
Completely forgot about this PR. Any updates on this too? @drchen
We found some problems during the internal review.
I need to take more time to understand the relevant logic and figure out what's the cause. : )
Could you elaborate on the issue @drchen? I'm using this fix in my app, so it might affect me. Also, if I know the issue, I could try to figure it out on my end.
My teammate patched and tested the PR and said:
""" ...the scroll behavior is almost completely broken for this case -- it doesn't expand upwards when you try to drag it up. it looks like this is because it only behaves properly when shouldExpandOnUpwardsDrag() is also set to true. wdyt about removing that flag? i can't imagine a scenario where we'd want it set to false. also, that shouldExpandOnUpwardsDrag() only takes effect when used with shouldSkipHalfExpandedStateWhenDragging(), which is pretty unexpected. i think we should delete that flag and just add another shouldSkipHalfExpandedStateWhenDragging() check next to the fitToContents offset check in onViewReleased(). """
Haven't really got time to check it yet.
Oh yeah, I already know about that bug @drchen. It was present before my fix (Discovered it while trying to fit the bottom sheet into my app), and I'm really not sure what the point behind that function was.
I could go look into it and try to remove that function. Could I append it to this PR or should I go and make a new one?
Any updates here @drchen? Should I make another PR to remove shouldExpandOnUpwardsDrag so this can get unblocked?
Hi the change is pretty difficult to land the change due to all the corner cases we found during running internal tests.
I'll need a less busy timespan to go back working on this. Sorry for the long delay.
@OxygenCobalt if Ive to use it in my app, can you refer me here?
Sorry for taking awhile to respond @hellosagar. I don't really mind if you copy this changeset into your own vendored bottom sheet. Give credit if you want to be nice.