material-ui: [BottomNavigation] onClick does not fire if tapped while scrolling
When i try to tap (touch click) on a BottomNavigationAction while the page is currently scrolling, the onClick event handler is never called. The TouchRipple effect however runs flawlessly, which makes me believe that this is an issue with how touch events are handled in ButtonBase. If i change my onClick for a onTouchStart it is called as expected, but of course this is not a viable solution, as it does not take swiping into account.
- The issue is present in the latest release.
- I have searched the issues of this repository and believe that this is not a duplicate.
Current Behavior 😯
If i press the BottomNavigationAction while the page is scrolling, the page stops scrolling and the TouchRipple effect runs. If i press again, now that the page has stopped scrolling, the onClick event handler is called.
Expected Behavior 🤔
When i press the BottomNavigationAction while the page is scrolling, the TouchRipple effect runs and my onClick handler is called.
Steps to Reproduce 🕹
Steps:
- Go to a page where you have a BottomNavigation, where you can scroll vertically
- Swipe up
- While the phone is scrolling, tap the BottomNavigationAction
Context 🔦
When you scroll on mobile, it takes a while for it to stop scrolling, if you do it fast enough. Therefore i want it to be possible, to click the BottomNavigationAction while the phone is scrolling.
Your Environment 🌎
I experience the issue when running mobile emulation in Chrome Devtools. A tester is also experiencing this issue on an iPhone 11 Pro on Safari.
| Tech | Version |
|---|---|
| Material-UI | v4.9.1 |
| React | v16.12.x |
| Browser | Chrome |
| TypeScript | v3.9.3 |
About this issue
- Original URL
- State: open
- Created 4 years ago
- Reactions: 9
- Comments: 28 (25 by maintainers)
I have investigated the issue discussed in #27664. It seems to be a timing issue, where the time used by Firefox to fire the onClick event exceeds the 10ms used in the touch timeout here: https://github.com/mui-org/material-ui/pull/22524/files#diff-9bd1b68cd4c10ba7a546c4e18fb32dd88299f9a9de53f2efe599f54cdf24ae92R112.
If i increase the timeout, then i can no longer reproduce the issue in Firefox, using the demo from #27664 (i checked out a commit before the author changed it to use a button element).
The 10ms delay were taken without additional thought, from this PoC from @oliviertassinari, but i can see that the ripple uses an 80ms delay (https://github.com/mui-org/material-ui/blob/a9194b570c7384ae869023020d084229fdc0dc32/packages/material-ui/src/ButtonBase/TouchRipple.js#L12). I imagine that it is that high, due to the exact same issue as this. Thus, using the same delay would resolve this issue.
Of course this does not resolve the question of functionality raised by @eps1lon, but it fixes the bug reported in #27664 without removing behaviour that i, and users of mine, regard as expected. 😄
@eps1lon I disagree, the ripple starting, when clicking - not when doing a gesture/swipe, indicates to the user (at least any user who does not know how MUI works) that the button has been clicked. When the ripple starts, the user then expects the appropriate action to occur. If the given action does not occur, the user will interpret this as an error. The whole reason i opened this issue in the first place, is because a (non-technical) customer believed this to be a bug in the software we built for them.
When one person interpret this as a bug, there must be a whole lot more who just haven’t said anything.
I also do not necessarily agree on this. If you are scrolling around on a page at a certain pace, you would not have a 100% hit rate, when trying to press a given button element. No matter how fast you scroll though, a fixed bottomnavigationaction will always be the same place, and you will likely always hit it whenever you try, as navigation quickly becomes muscle memory. Therefore a user would expect it to fire an action, no matter how fast the page is scrolling.
@oliviertassinari Please reopen this issue and reign in your trigger-happy support bot. This is a bug in MUI without a doubt.
👋 Thanks for using Material-UI!
We use GitHub issues exclusively as a bug and feature requests tracker, however, this issue appears to be a support request.
For support, please check out https://material-ui.com/getting-started/support/. Thanks!
If you have a question on StackOverflow, you are welcome to link to it here, it might help others. If your issue is subsequently confirmed as a bug, and the report follows the issue template, it can be reopened.
To expand on what the 80ms of https://github.com/mui-org/material-ui/blob/a9194b570c7384ae869023020d084229fdc0dc32/packages/material-ui/src/ButtonBase/TouchRipple.js#L12
is about. It’s about being long enough to know if a click event is likely going to happen. It’s also about being short enough to make the ripple feel instant.
The problem we face here seems to strick for the same tradeoff. We need to wait long enough to not fire two-click events. We also need not wait too long to make the UI feel responsive (the click handler).
Agree, I think that we could increase the timeout from 10 ms to 80ms. It’s still enough to make the UI feel responsive.
@rxdrag Could you expand on how the double click event is problematic in your use case? I get that it’s not ideal but if the annoyance is only about calling a “cheap” (from a runtime perspective) method twice. It sounds like an interesting tradeoff to take this double click pain in and get the UI feel more responsive on iOS in exchange, until we know more better.
Re-opening since the fix for this issue (https://github.com/mui-org/material-ui/pull/22524) causes another bug: https://github.com/mui-org/material-ui/issues/27664#issuecomment-896575078.
BottomNavigationActionshould not have different click behavior compared to any other button-like element. The ripple starting is not an indication that a click will be dispatched. It’s just indicating that the button is “arming”.The question that’s currently unanswered: How would a native
<button />,<Button />or any other clickable behave under the same circumstances. They should behave just like the<BottmonNavigationAction />.@oliviertassinari I have the implementation and testing done, i just need to throw together the demo you wanted. I’m planning on doing it after work today 😃 If you have any comments for the implementation/testing, they are more than welcome.
@oliviertassinari Nice,
fireEventwas just what i wanted to do, but couldn’t figure out how. I’ll use that instead, and make sure to forward the events.@EliasJorgensen Great. One thing I could see: if a parent provides onTouchStart/onTouchEnd the event is not forwarded.
We can’t move forward with an approach that leverages the ref to trigger the action. However, you can apply the approach we use in the other components, e.g.: https://github.com/mui-org/material-ui/blob/ceb813e855778dc544b1056c600633c4d2aa27b3/packages/material-ui/src/Slider/Slider.test.js#L138
Now, I don’t think that this feature can “truly” (as end-to-end) be tested with our stack. It relies on the fact that if a scroll is in progress the onClick event is never triggered. But I guess it’s still interesting to add one 😃
@oliviertassinari Sure, i’d like to work on a pull request. What do you want the demo to showcase? I’m also unsure how i would write a test for this scenario. Do you have anything similar in the codebase i can look at for a rough reference?
@oliviertassinari How is this support? I am reporting unexpected behaviour? Surely that must be a bug.