material-components-android icon indicating copy to clipboard operation
material-components-android copied to clipboard

[BottomSheetBehavior] Check for `shouldSkipHalfExpandedWhenDragging` after nested scrolling

Open OxygenCobalt opened this issue 2 years ago • 6 comments

Make BottomSheetBehavior check the experimental shouldSkipHalfExpandedWhenDragging flag before setting the state to HALF_EXPANDED.

The code changes the following behaviors in onStopNestedScroll:

  1. In the case of dy > 0, the state will be set to STATE_EXPANDED instead of STATE_HALF_EXPANDED when the flag is enabled.
  2. In the case of dy == 0, the state will be set to STATE_EXPANDED when above the half-expanded offset, and STATE_COLLAPSED when below it when the flag is enabled.
  3. In the state of dy < 0, the state will be set to STATE_COLLAPSED instead of STATE_HALF_EXPANDED when 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.

OxygenCobalt avatar Jul 29 '22 18:07 OxygenCobalt

Okay, simplified those if statements @drchen.

OxygenCobalt avatar Jul 29 '22 18:07 OxygenCobalt

Completely forgot about this PR. Any updates on this too? @drchen

OxygenCobalt avatar Sep 02 '22 20:09 OxygenCobalt

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. : )

drchen avatar Sep 03 '22 01:09 drchen

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.

OxygenCobalt avatar Sep 03 '22 15:09 OxygenCobalt

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.

drchen avatar Sep 06 '22 14:09 drchen

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?

OxygenCobalt avatar Sep 06 '22 14:09 OxygenCobalt

Any updates here @drchen? Should I make another PR to remove shouldExpandOnUpwardsDrag so this can get unblocked?

OxygenCobalt avatar Feb 23 '23 19:02 OxygenCobalt

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.

drchen avatar Feb 24 '23 01:02 drchen

@OxygenCobalt if Ive to use it in my app, can you refer me here?

hellosagar avatar Mar 22 '23 14:03 hellosagar

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.

OxygenCobalt avatar Oct 23 '23 18:10 OxygenCobalt