Fixed double ripple due to compat mouse down (issue 1563)

[owencm]

This change fixes issue 1563. For compatibility reasons, browsers generate a mouse down event after touch end. This change prevents us from showing a ripple for the first mouse down event after we've seen a touch start event, the same solution applied in Material Design Lite.

The old code intended to solve this, but the ripple created by the touch events had already been removed from ripples by the time the mouse down event was fired, so the check if (ripples[i].props.touchGenerated) return; would never detect the earlier touch generated ripple, even though it was still being displayed.

An alternative solution would be to ensure ripples always includes all currently visible ripples, but since today the ripples set a timeout of 2000ms before allowing themselves to be removed we'd need to either duplicate that logic hackily here, or have the ripples fire some event back to touch-ripple.jsx to indicate they're done, which seems like an necessarily complex solution. I worry the current method of keeping ripples on screen after they've been removed from the parent will create more edge case bugs like this, so perhaps in the long run we should find some solution where the parent of the ripples has more control and visibility into their lifecycle, but in the meanwhile this patch seems like a good solution.

[owencm]

This is my first pull request to material-ui by the way. Any feedback on my code or process of submitting patches is very much appreciated.

[oliviertassinari]

@owencm Thanks for working on this issue. There is no particular process for submitting patches.

[oliviertassinari]

Using setState will trigger a rerendering.
I think that it would be better to have this ignoreMouseDown property outside of the react state.
I would just use the react state for property that have an effect on the virtual dom we render.
For instance https://github.com/callemall/material-ui/blob/master/src/left-nav.jsx#L301

[owencm]

"an unnecessarily"*

@olivertassinari good to know, thanks!

[owencm]

@oliviertassinari That's true that it would re-render, which isn't ideal. I'm still pretty new to React, but it seems to me that the idiomatic solution would be to keep it in state and implement a shouldComponentUpdate method which returns false if the only change is to ignoreMouseDown. That avoids having some ugly state floating around outside of this.state, although it would be slightly less performant due to comparing the old and new props and state in shouldComponentUpdate.

What do you think? I'm happy to update to whichever solution you think works best. :)

[oliviertassinari]

@owencm I have just tried the fix. It's working fine for a single tap 👍.

I worry the current method of keeping ripples on screen after they've been removed from the parent will create more edge case bugs like this, so perhaps in the long run we should find some solution where the parent of the ripples has more control and visibility into their lifecycle, but in the meanwhile this patch seems like a good solution

With a double tap, we can see three ripples, it seems to be an edge case you described.
Can't we just ignore the onMouseDown, onMouseUp and onMouseLeave events if we detect a touch device?

it seems to me that the idiomatic solution would be to keep it in state

I remember an older discussion regarding this same topic and we decided to not use the react state.
As you said, it was for performance reason. I don't know what the facebook react team says about this.
So I would say, let's not use the state.

[owencm]

@oliviertassinari can you double check that you are seeing three ripples? I've tried a couple of devices with touch and non-touch and can't reproduce.

Unfortunately we can't simply ignore ouse events on touch devices as many laptops these days have touch screens so the user may either use a mouse or touch input regardless of the device supporting touch.

And that sounds good, I'll move to using a local variable in that style rather than using React state.

[oliviertassinari]

Unfortunately we can't simply ignore ouse events on touch devices as many laptops these days have touch screens so the user may either use a mouse or touch input regardless of the device supporting touch.

I knew something was wrong. It's just too simple.

can you double check that you are seeing three ripples

I'm not using a real device, I'm testing with the Chrome mobile emulator.

I've tried a couple of devices

I trust you with this, we should be good then.

An alternative solution would be to ensure ripples always includes all currently visible ripples, but since today the ripples set a timeout of 2000ms before allowing themselves to be removed we'd need to either duplicate that logic hackily here, or have the ripples fire some event back to touch-ripple.jsx to indicate they're done, which seems like an necessarily complex solution

For now, I would say, let's keep this fix as it's simpler and should answers 80% of our use case.
Can you squash down the commit to one? I will merge then 👍.

[oliviertassinari]

Side note: @vjeux wasn't using the react state in this article (http://blog.vjeux.com/2013/javascript/scroll-position-with-react.html) to store properties needed outise the render method.
I think we are fine here.

[owencm]

That sounds good. I chatted to a FB engineer today and he confirmed they do that approach for non-re-rendering state too so LGTM.

I've merged the commits and think everything is good to go, although I'm new to screwing around with rebasing etc so let me know if I did something wrong and I'm happy to correct!

Time to go pick up some other issue :)

[hilkeheremans]

@owencm Thanks for the PR! As you mention, it will probably cause some unwanted side effects but I can personally live with those. ;-)

I agree it should be OK to store some state on the component instance outside of setState, specifically in the case when state change should not risk triggering a potentially costly re-render.

[oliviertassinari]

I've merged the commits and think everything is good to go, although I'm new to screwing around with rebasing etc so let me know if I did something wrong and I'm happy to correct!

There is an additional commit in this one. I have created a PR with just your commit that keep the creditential (#2216).

@owencm Thanks!

[owencm]

Thanks @oliviertassinari! And sorry about the extra commit, still getting to grips with this rebasing and commit squashing business. Hopefully I'll get that first time on my next PR!

[oliviertassinari]

Hopefully I'll get that first time on my next PR!

We are looking forward to see it 😄.

Regarding this rebasing issue. I can expose how I'm handling it.
I have a fork of the official repository with two remotes, my fork as the main remote origin and the official repository upstream.
Then, when I need to rebase (assuming i'm on the right branch), I'm running those commands.

git fetch upstream master
git rebase upstream/master -i