Skip to content

[BottomSheetBehavior] Check for shouldSkipHalfExpandedWhenDragging after nested scrolling #2876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OxygenCobalt
Copy link
Contributor

@OxygenCobalt OxygenCobalt commented Jul 29, 2022

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.

… nested scrolling implementation

Make `onStopNestedScroll` check to `shouldSkipHalfExpandedWhenDragging`
before trying to set it's state to `STATE_EXPANDED`.

This prevents the STATE_EXPANDED state from appearing even when
`shouldSkipHalfExpandedWhenDragging` is set to true.
@OxygenCobalt OxygenCobalt changed the title [BottomSheetBehavior] Check for shouldSkipHalfExpandedWhenDragging during nested scrolling [BottomSheetBehavior] Check for shouldSkipHalfExpandedWhenDragging after nested scrolling Jul 29, 2022
Clean up nested ifs in onStopNestedScroll where an OR would suffice.
@OxygenCobalt
Copy link
Contributor Author

Okay, simplified those if statements @drchen.

@drchen drchen added the Reviewing Internally An internal change has been created and sent for review. label Jul 29, 2022
@OxygenCobalt
Copy link
Contributor Author

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

@drchen
Copy link
Contributor

drchen commented Sep 3, 2022

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

@OxygenCobalt
Copy link
Contributor Author

OxygenCobalt commented Sep 3, 2022

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.

@drchen
Copy link
Contributor

drchen commented Sep 6, 2022

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.

@OxygenCobalt
Copy link
Contributor Author

OxygenCobalt commented Sep 6, 2022

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
Copy link
Contributor Author

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

@drchen
Copy link
Contributor

drchen commented Feb 24, 2023

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.

@hellosagar
Copy link

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

@OxygenCobalt
Copy link
Contributor Author

OxygenCobalt commented Oct 23, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Internally An internal change has been created and sent for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants