[BottomNavigation] onClick does not fire if tapped while scrolling

[EliasJorgensen]

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:

1. Go to a page where you have a BottomNavigation, where you can scroll vertically
2. Swipe up
3. 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

[support]

👋 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.

[EliasJorgensen]

@oliviertassinari How is this support? I am reporting unexpected behaviour? Surely that must be a bug.

[soerenBoisen]

@oliviertassinari Please reopen this issue and reign in your trigger-happy support bot. This is a bug in MUI without a doubt.

[oliviertassinari]

A couple of thoughts:

• A reproduction of the issue https://codesandbox.io/s/material-demo-forked-w444e?file=/demo.tsx.
• The first click attempt will stop the scroll, the second click attempt will go through. It's how the web works.
• The visible ripple originates from a tradeoff. You can check the source of TouchRipple.js and its history to gain more information about it. I think that it's a dead end, nothing to improve there.
• I have tried to create a reproduction with ionic to see how they would behave. I couldn't, their documentation was missing too much information for doing such. I have hatted the experience. I think that it would be great to add a new demo, using the previous codesandbox as inspiration as well as https://material-ui.com/components/app-bar/#bottom-app-bar
• On Android and iOS a touch start doesn't stop the scroll, on the web, it does, I don't think that we can do anything about it.
• On Android and iOS a touch end, if it completes on the same element that the touch start was while scrolling, it will trigger a click, on the web it doesn't.

This POC seems to fill the task just right:

diff --git a/packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js b/packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js
index a1956c844d..020b125b30 100644
--- a/packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js
+++ b/packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js
@@ -69,6 +69,9 @@ const BottomNavigationAction = React.forwardRef(function BottomNavigationAction(
   } = props;

   const handleChange = (event) => {
+    clearTimeout(timer.current);
+    console.log('click')
+
     if (onChange) {
       onChange(event, value);
     }
@@ -78,6 +81,9 @@ const BottomNavigationAction = React.forwardRef(function BottomNavigationAction(
     }
   };

+  const start = React.useRef();
+  const timer = React.useRef();
+
   return (
     <ButtonBase
       ref={ref}
@@ -89,6 +95,25 @@ const BottomNavigationAction = React.forwardRef(function BottomNavigationAction(
         },
         className,
       )}
+      onTouchStart={(event) => {
+        const { clientX, clientY } = event.touches[0];
+
+        start.current = {
+          clientX,
+          clientY,
+        }
+      }}
+      onTouchEnd={(event) => {
+        const target = event.target;
+        const { clientX, clientY } = event.changedTouches[0];
+
+        if(Math.abs(clientX - start.current.clientX) < 10 && Math.abs(clientY - start.current.clientY) < 10) {
+          timer.current = setTimeout(() => {
+            alert('foo')
+            target.dispatchEvent(new Event("click", { bubbles: true }));
+          }, 10);
+        }
+      }}
       focusRipple
       onClick={handleChange}
       {...other}

Could you try it out in your application? Do you want to work on a pull request? (we would need to add a new demo, add tests, and cleanup the POC). Unfortunately, if there is a native <a> used for navigation, it won't trigger it.

[EliasJorgensen]

@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]

@EliasJorgensen We have no prior-art in messing with the onclick, so nothing we can directly use as inspiration. For the demos, we have seen developers ask: how do I correctly position the bottom navigation a couple of time? We could solve this question with a new demo at the end of the page, similar to my reproduction.

[EliasJorgensen]

@oliviertassinari Alright cool, i'll take a look at it next week 👌

[EliasJorgensen]

@oliviertassinari i have implemented your PoC, cleaned it up, and added a single unit test for it, to show how i solved testing it. The only way i could figure out testing the onTouchStart/End handling of the component, was by adding a useImperativeHandle to the BottomNavigationAction component, in order to call the handlers directly. This however hijacks the ref, as far as i can tell anyway, such that it never reaches the ButtonBase like it normally does. I'm not sure how we can test the touch handling without doing this however. Is this problem something you have experienced before?

You can take a look at what i mean here: EliasJorgensen@cc507f1

[oliviertassinari]

i have implemented your PoC, cleaned it up, and added a single unit test for it

@EliasJorgensen Great. One thing I could see: if a parent provides onTouchStart/onTouchEnd the event is not forwarded.

I'm not sure how we can test the touch handling without doing this.

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

[EliasJorgensen]

@oliviertassinari Nice, fireEvent was 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]

@oliviertassinari In your PoC, what is the point of the timout when dispatching the click event?

[oliviertassinari]

@EliasJorgensen Wait for a potential click event to fire, if a native click fires, we need to cancel the synthetic click event. Most of the time, the native click should fire, it's only when there is an inertial scrolling that it doesn't, we don't want to fire a click event twice.

[EliasJorgensen]

@oliviertassinari Ah i see, so we create the timeout, and if we receive a native click event, the handleChange function clears the timeout. Gotcha. Thanks for clearing it up 👍

[oliviertassinari]

@EliasJorgensen How is it going? Are the changes ready for a pull request :)?

[EliasJorgensen]

@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.

[eps1lon]

Re-opening since the fix for this issue (#22524) causes another bug: #27664 (comment).

BottomNavigationAction should 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 />.

[EliasJorgensen]

@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.

[EliasJorgensen]

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 />.

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.

[EliasJorgensen]

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. 😄

[oliviertassinari]

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.

[soerenBoisen]

@eps1lon I would be careful about requiring that BottomNavigationActions and buttons act exactly the same. The crucial difference here is that BottomNavigationActions always remain static as the user scrolls, whereas buttons are usually embedded in the scrolling content.

It is natural for a user to expect to be able to interact with static elements before other content has stopped scrolling. But as a user, I would not be surprised that I cannot activate a button as it scrolls by. In fact I would stop the scroll before making the attempt to make sure I hit the right button.

[eps1lon]

whereas buttons are usually embedded in the scrolling content.

They can be static just like the BottomNavigationAction.

@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.

The ripple starting never indicated that a click happened but that it will happen (under certain conditions). So if you want to propose a change in behavior to the ripple, I'm all ears. Otherwise there's not much to disagree since your statement is factually incorrect.

Right now the current solution is based on false assumptions (how the ripple works) and therefore needs to be revisited to avoid unrelated fallout.

[oliviertassinari]

how the ripple works

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

[oliviertassinari]

@EliasJorgensen Do you want to work on a PR based on #22368 (comment)?

[EliasJorgensen]

@oliviertassinari Sure. What do you want done? Just changing the timeout? :)

[oliviertassinari]

Mostly to make sure we don't regress with #27664 and if we do, that it's not frequent.

[EliasJorgensen]

@oliviertassinari I have reapplied the changes that were reverted in #27690 and changed the 10ms delay, to use DELAY_RIPPLE imported from TouchRipple.js instead.
As mentioned in #22368 (comment), that delay fixed the issue in Firefox for me, and i could no longer reproduce it.
Should i just open a PR with this, or is there anything else you wanted me to look into? :)

[EliasJorgensen]

@oliviertassinari I will just submit a PR, then you can comment if there's anything else you would like done 😄