mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Ripple stays if multiclicked fast #7537

Closed NonameSLdev closed 5 years ago

NonameSLdev commented 6 years ago

If someone clicks in about >9 clicks per second for a little while (1-2 seconds) the ripples don't leave the button and it stays with the color. Small demonstration:

You're welcome to try it yourself: https://material-ui.com/demos/buttons/#text-buttons

EDIT: This comment describes the issue - there are more mouse downs than ups and the ripple isn't released.

oliviertassinari commented 6 years ago

What browser are you using? I can't reproduce it with Chrome 59.

kgregory commented 6 years ago

Unable to reproduce in:

NonameSLdev commented 6 years ago

@oliviertassinari @kgregory I'm on Windows 10, Firefox 54.0.1. I'm using my touchpad on my laptop to do it in a faster speed with both fingers... I guess that's what's different because with one finger I can't manage to do it. It's not an urgent bug but it could happen.

Dattaya commented 6 years ago

I couldn't reproduce either (Windows 10, Firefox 54.0.1) but in Chrome and with a right (secondary) mouse button it does behave like that.

kgregory commented 6 years ago

@NonameSLdev I'm around 9 clicks per second and can't reproduce it in that version of Firefox.

@Dattaya are you repeatedly clicking with the right mouse button to reproduce this in Chrome?

kgregory commented 6 years ago

Nevermind @Dattaya, I reproduced in Chrome by semi-rapidly clicking left and right mouse button simultaneously about ten times.

oliviertassinari commented 6 years ago

I'm wondering should we have a ripple with a right mouse click? I would say no. So maybe the right fix is to disable them.

kgregory commented 6 years ago

@oliviertassinari good thought, but there will probably be some other combination of ripple-worthy user interactions that lead to this behavior. Have you seen anything in the standards about ink ripples on right-clicks (I'm guessing there's nothing there)? In Chrome, there are no ripples on the bookmark bar when a bookmark is right-clicked. Maybe this is enough evidence 😄

I've looked at this for a little while and I'm not exactly sure why these ripples are being stranded in the TouchRipple's state (ripple array).

NonameSLdev commented 6 years ago

At 9cps I found something interesting: On firefox the ripples don't leave, on chrome they leave extremely slowly. BUT, on each click on firefox on a button that already has ripples that didn't leave another ripple shows up then leaves and takes a ripple with it. Odd behaviour indeed

oliviertassinari commented 6 years ago

The issue might have been fixed by #7575. I would say 50/50 as I haven't been able to reproduce

kgregory commented 6 years ago

Just pulled and reinstalled all dependencies, still able to reproduce by clicking left and right mouse buttons simultaneously at a semi-rapid pace.

oliviertassinari commented 6 years ago

@kgregory I guess it's an issue at the ButtonBase level in that case. Thanks for trying it out.

oliviertassinari commented 6 years ago

This issue is driving me crazy, no matter what I try, I can't reproduce. I have added the good first issue tag, if someone else want to address it, great, otherwise, it will stay unresolved.

stavlocker commented 6 years ago

@oliviertassinari I tested it around a few. I can only reproduce it with my laptop's touchpad, even at 3cps.

AndreasZeissner commented 6 years ago

Hi guys, at first, nice work. A bit rough to keep up with beta, but it is getting more awesome on a daily basis.

@oliviertassinari we use mui an have some strange errors concerning this issue, maybe this helps to fix this: On a Mac, using chrome you cannot reproduce this issue and everything works just fine. On a Linux machine, with chrome, rippling buttons get darker and darker when you click it at something at 3cps. Clicking it fast does not reproduce this. Remarkable is, that turning of fastclick https://github.com/ftlabs/fastclick, we use, will fix this, and buttons will handle the events right. Pressing the buttons on the mui-doc page also does not have this effect, as i assume, there are no libraries having any side effects on material-ui. Maybe it helps to reproduce or delimit this issue.

ssalka commented 6 years ago

I'm working on a cordova iOS app (with FastClick to avoid 300ms delay) using material-ui@next (1.0 v23) and I'm getting the same behavior. Removing FastClick seems to fix the touch ripples building up, but results in poor UX due to the 300ms delay.

@oliviertassinari I noticed you have an app called SplitMe that's using material-ui@next + cordova, do you know of a way to avoid this touch ripple bug when used in tandem with FastClick? Or does SplitMe also have the 300ms delay?

P.S. before I had been using material-ui@0.19 with FastClick and had no touch ripple issues

oliviertassinari commented 6 years ago

Or does SplitMe also have the 300ms delay?

@ssalka Can you notice the delay in the documentation? Is it a Cordova specific issue? I haven't been doing much work on SplitMe for a long time. As far as I know, the delay can be removed with the viewport meta. To be confirmed.

ssalka commented 6 years ago

I think it is a Cordova issue (more specifically, the iOS UIWebView). I actually couldn't find SplitMe in the app store, and unfortunately I have no way to check whether the documentation loads in my project, as CORS is disabled and HTML links to other domains open up in Safari (tried an iframe, no luck). All I can say for certain is that the touch ripples work fine on v0.x, and build up in v1.

In general it looks like all the major browsers (even Safari for iOS!) have implemented a fix using the viewport meta tag, like you said, but it is still present in the UIWebView used internally by Cordova. I wouldn't count Cordova as a major browser/platform though, so I'll understand if this is not counted as a priority issue.

Thanks for the quick response!

mbrookes commented 6 years ago

@ssalka Have you tried https://github.com/apache/cordova-plugin-wkwebview-engine?

oliviertassinari commented 6 years ago

I confirm it's much better. I'm using this plugin on SplitMe.

oliviertassinari commented 5 years ago

@NonameSLdev Can you still reproduce the issue? The issue hasn't made much progress in a year. I think that it will be safe to close it.

stavlocker commented 5 years ago

Yes, same behaviour - ripple stays and on each hover some of it loses of itself.

It isn't really a major issue, but it does exist.

UPDATE: Wait. Just got an idea. What if this is about more mouse downs (a.k.a clicks) than mouse ups? Because using the track pad that can happen as I'm using two fingers for pressing repeatedly.

I just tested this theory and it seems to be true: When multiclicking like I do to create the ripple I got 21 mouse down events and just 11 mouse up events. I think that this is the reason. #TookUsLongEnough

gtsafas commented 5 years ago

@stavlocker @oliviertassinari Ive been encountering this issue for awhile now. For my case I was finally able to track it down to '-webkit-app-region': 'no-drag' removing the property completely fixed the issue for me. I suspect this may be similar to the referenced issue #11696

Just FYI I am using openfin on chrome 61

stavlocker commented 5 years ago

@gtsafas Perhaps this could fix it, but as I said on my comment (above yours) it's because there are more mouse downs than ups. Without looking at the code, I can say that ripple is created on mouse down and removed on mouse up - but that mouse up never arrived because there are more downs than ups. Your fix is good but it would only work in webkit browsers.

Two solutions as I see it:

gtsafas commented 5 years ago

@stavlocker yeah I wasn't proposing it as the fix. But I was saying that the other issue that causes this is dragging images. For me it was not specifically images but something that interacts with dragging. Something was preventing the mouseup from occuring. At least in my case it wasn't related to users with different click speeds, it happened for everyone while that css was there.

We can put a timer or mouse up but these are just hiding the problem. I think we need to know why mouse up isn't firing before patching it.

Does this happen to you on every material-ui site?

stavlocker commented 5 years ago

@gtsafas That's interesting. But it's not a MUI bug that the mouse up is not firing. In my case it's because with my trackpad I can use two fingers to click. My thoughts are that because I use two fingers which can trigger another mouse down when there's already one down, sometimes I lift my other finger while the second one is pressed which the trackpad doesn't detect as a mouse up because there's something pressed down. This causes more downs than ups therefore the ripple stays.

gtsafas commented 5 years ago

@stavlocker ok then that's different. While trying to debug this (before I realized it was -webkit-app-region) I added logging around mouse down/up and also observed more downs than ups.

So if you plug in a normal mouse it works fine?

kgregory commented 5 years ago

@gtsafas you can reproduce the behavior with a mouse by simultaneously clicking the left and right buttons.

ejnu commented 5 years ago

we had this problem too. you probably preventing somewere else mouse events... check your code for something like:

document.addEventListener("mousedown", myScript); // should be document.addEventListener("mousedown", myScript, false);
document.addEventListener("mouseup", myScript); // should be document.addEventListener("mouseup", myScript, false);

//jquery example: 
        // (...)
        documentElement.on({
            'mouseup': (e) => {
                e.preventDefault(); // WRONG! cut this off
                isDrag = false
            },
        // (...)
        });
oliviertassinari commented 5 years ago

@ejnu We should be prevent default invariant. Something might be wrong.

joshwooding commented 5 years ago

@kgregory @oliviertassinari I'm not sure if this is right but here is what I've found after a little testing: Somehow this happens when a click interacts with the context menu in a weird way. I tested this by failing to have the ripple persist with: onContextMenu={e => {e.preventDefault()}} on ButtonBase. It doesn't look like you can detect when a click happens on the context menu but I think I've managed to fix this by stopping the ripple on the context menu event: #13740

   handleTouchMove = createRippleHandler(this, 'TouchMove', 'stop');

+  handleContextMenu = createRippleHandler(this, 'ContextMenu', 'stop');

   handleBlur = createRippleHandler(this, 'Blur', 'stop', () => {
    clearTimeout(this.focusVisibleTimeout);
    if (this.state.focusVisible) {

        onTouchEnd={this.handleTouchEnd}
        onTouchMove={this.handleTouchMove}
        onTouchStart={this.handleTouchStart}
+       onContextMenu={this.handleContextMenu}
        ref={buttonRef}
        tabIndex={disabled ? '-1' : tabIndex}
        {...buttonProps}

Can anyone confirm?

IssueHuntBot commented 5 years ago

@0maxxam0 funded this issue with $40. See it on IssueHunt

oliviertassinari commented 5 years ago

It should be solved with #13740. If you can reproduce the issue with the latest version, let us know!

stavlocker commented 5 years ago

@oliviertassinari @kgregory I can still reproduce, as I already said on #13740. This PR only fixes the context menu spamming, but not the trackpad spamming. This comment describes the issue I'm suffering from and offers two solutions.

Ways to reproduce:

  1. Go to the button component demos
  2. Use your trackpad to click the button a bunch of times

I'm using a Lenovo Yoga 700 running Windows 10, can reproduce in Chrome 71, Firefox 64 (Quantum), Edge 42.

I'm certain this can be reproduced on different computers, as the problem is that the trackpad registers multiple fingers and sometimes doesn't register as much mouse ups as mouse downs, as I've concluded in my previous testing.

petercutting commented 5 years ago

I am seeing cumulative button ripple shading in my phonegap, react mui app on an iPad. on Android and Web there is no problem. Everytime I click the button it gets darker (or lighter) until it saturates.

CaptainN commented 5 years ago

I can confirm this issue on Safari/iOS as well - it doesn't even take tapping fast, the dark/light overlay simply never goes away after tapping it.

CaptainN commented 5 years ago

@oliviertassinari @kgregory I can still reproduce, as I already said on #13740. This PR only fixes the context menu spamming, but not the trackpad spamming. This comment describes the issue I'm suffering from and offers two solutions.

Ways to reproduce:

1. Go to the [button component demos](https://material-ui.com/demos/buttons/)

2. Use your trackpad to click the button a bunch of times

I'm using a Lenovo Yoga 700 running Windows 10, can reproduce in Chrome 71, Firefox 64 (Quantum), Edge 42.

I'm certain this can be reproduced on different computers, as the problem is that the trackpad registers multiple fingers and sometimes doesn't register as much mouse ups as mouse downs, as I've concluded in my previous testing.

If the problem is that there are more mousedown events fired than mouseup, we should look at whether there is a corresponding number of mouseup outside that element. One trick for these kinds of things you can add a mouse up handler (which would remove itself) to the root DOM element on mouse down, then the mouse up will always fire, no matter where mousedown was captured. I suspect there is always some event that fires to trigger cleanup, but it may not be on the originating element.

joshwooding commented 5 years ago

@CaptainN What iOS/Safari are you reproducing this on. I can’t recreate on iOS 12.1.2

CaptainN commented 5 years ago

iOS 11.3.1 - mostly I see it on IconButton components, but also on Fab.

CaptainN commented 5 years ago

It looks like it's supposed to:

  1. Animate the circle from touch point out.
  2. Fade out (alpha) while animating.
  3. I assume get removed and cleaned up after the animation is complete.

Steps 2 and 3 for me are not happening on my iPad (11.3.1) an older iPhone (11.4.1) or iPhone 8 (12.1.2).

I'll see if I can dig into the code at some point, but can't make any promises about timing.

mbrookes commented 5 years ago

@CaptainN Is this in the docs demos, or your own code? I can't reproduce it with the docs on an iPhone 10 with iOS 12.1, or an older iPad with 12.1.3, so I'm wondering if there are confounding factors at play?

Not saying it isn't a bug, but there may be more steps required to reproduce it.

CaptainN commented 5 years ago

It's in my own app, which is a medium sized app built with Meteor, React and Material-UI. It'll actually be public soon, so I can share a link.

I'll both look for the cause in code at some point, and if I can't find it, try to produce a reduction.

CaptainN commented 5 years ago

I'm also using SSR - I do get some warnings about mismatched style attributes, usually the server or client doesn't agree on the -webkit- prefix - could that have something do with it? I'm actually not really sure how material-ui applies prefixes (autoprefixer) - are there docs on that?

mbrookes commented 5 years ago

This might help: https://material-ui.com/getting-started/supported-platforms/#css-prefixing

zuus-keith commented 5 years ago

I am also experiencing this issue in my own environment using Meteor, React and Material-UI and only on iOS. I've been able to use Chrome's device emulator and only to get this bug to trigger when emulating iOS devices and and not Android devices. I can't seem to replicate this with the demo sandboxes yet but it might stem from the ButtonBase component because I seeing the issue with buttons and tabs.

CaptainN commented 5 years ago

I'm now seeing this even in Chrome on Windows with mobile mode enabled in Dev Tools. On this app: https://www.pixstoriplus.com/

joshwooding commented 5 years ago

@CaptainN I’ll have a look later. Just for clarity sake can you post your reproduction steps. What Chrome are you using?

CaptainN commented 5 years ago

It was doing it quite consistently before, but now that I'm trying to reproduce it in Chrome, nothing. Something must have put it into an error state of some kind.

On iOS, you can just load that app up and see the problem pretty easily. Tap back and forth between home and search buttons in the bottom nav for the easiest example. Or tap search, then back to home, and press home a few times.

joshwooding commented 5 years ago

I can recreate it on iOS on the bottom navigation.

zuus-keith commented 5 years ago

I was able to get more consistent behaviour if you reload the page after switching from regular Chrome to device mode.