Open antonis opened 3 years ago
antonis
commented
3 years ago I attempted to resolve this by configuring the BottomSheetBehavior. A more radical approach that would remove the dependency on the BottomSheetDialogFragment seems necessary to resolve this.
mkevins
commented
2 years ago I attempted to resolve this by configuring the
BottomSheetBehavior. A more radical approach that would remove the dependency on theBottomSheetDialogFragmentseems necessary to resolve this.
I attempted to resolve this as well, with a different approach (inspired by some answers here): adding a BottomSheetCallback to the behavior to track the offset with onSlide, and intercept state changes with onStateChanged. The idea was to only dismiss when the offset was beyond some custom threshold. This did not work, sadly, but it did offer a better means of logging / inspecting why the dismissal is so "sensitive".
My first approach used the following changes:
With this code, I opened the page picker via tapping: Fab -> New Page (from the My Site screen), followed by a very brief tap in a slightly downward "angle", and observed the following relevant logs in logcat:
From the logs, it is clear that the state is being set to hidden, even after we successfully set the state to be expanded. A little bit of digging reveals where this issue originates, and why it can be difficult to work around it.
The BottomSheetBehavior utilizes an internal ViewDragHelper, with a callback to override onViewReleased. There is a lot of logic within the implementation to handle various scenarios with nested ScrollViews, however, the onViewReleased method only has the x and y velocity from which to make decisions about what kind of gesture the user intended with their input. Notably lacking is the position of the start of the touch, nor the total distance traveled, which is typically used as a threshold to help distinguish between a tap and a fling or swipe. Instead, the implementation relies solely on two conditions: that the magnitude of the y velocity is greater than that of the x velocity, and that the y velocity is greater than a hard-coded constant called SIGNIFICANT_VEL_THRESHOLD (500 pixels / second).
The problem with this is that a tap can have a very short duration, and even a small change in pixels can lead to a very large velocity when the denominator is also very small. For example, in my testing, with just a few small taps, I was seeing velocities greater than 1500 pixels / second, even though my finger moved less than a few millimeters on the physical device.
Next, startSettlingAnimation is called with the targetState set to hidden, which posts a SettleRunnable, which handles the animation, deferring callbacks. While it isn't totally clear to me why setting the state manually does not intervene on the currently running animation, I suspect it is because the underlying view(s) that were driving the settling state are not considered in a state transition at the behavior level.
Another attempt I briefly tried was to disable the skipCollapsed behavior, but this would mean that the BottomSheet would have an intermediate (partially collapsed) state. I eliminated this "manually" via similar interception mechanisms, but the resulting UX is quite janky, imo, since the animation must complete before the state transition continues to fully hidden or fully expanded.
Since we are considering a new UI for this flow in the near future, and fixing this issue seems fairly non-trivial, perhaps it will become a moot point. I have not exhausted the possibilities for attempting to work around this (for example, further investigation could be explored to answer conclusively why the animation for hiding the view is not interrupted by a subsequent manual state change). However, as this appears to be a bug in the library code (imo), I'm not sure if the further time investment is worth the return. :man_shrugging: Wdyt @antonis ?
mkevins
commented
2 years ago Note: this is also an open issue on the library repository.
antonis
commented
2 years ago Thank you for investigating this and trying different approaches @mkevins 🙇
Since we are considering a new UI for this flow in the near future, and fixing this issue seems fairly non-trivial, perhaps it will become a moot point.
I agree Matthew. Since this screen might be deprecated (ref `pc8HXX-6Z-p2) it doesn't make sense to put more effort on this now 👍
Note: this is also an open issue on the library repository.
Great finding. Probably a new material library version will fix this 🤞
Expected behavior
The home page picker does not dismiss on slight downward gestures.
Actual behavior
The preview screen seems very sensitive to any slight downward gesture. That makes it easy to unintentionally dismiss the screen.
Steps to reproduce the behavior
The same behavior can be observed in the Page Layout Chooser screen.
Internal ref:
p5T066-1K5-p2#comment-6937Tested on Google Pixel 2 XL, Android 11, WPAndroid 16.6