fix(ripple): move 'up events' to window object - fixes #1124 #1461

Spetnik · 2017-10-22T22:20:47Z

This moves the mouseup, pointerup, and touchend events from the ripple-activated element to the window object. This ~~fixes #1328~~ fixes #1124 by allowing the ripple to deactivate if the mouse/finger moves off of the element before the mouse button is released or the finger is lifted. Additionally, a delay (equal to the animation length) is added to the deactivation call to ensure that the animation is not played twice during a touch event (otherwise a quick tap causes a double-ripple when the mouse and touch events fire sequentially).

N.B. This is my first contribution to this repo. I read the submission guidelines and I hope that I followed them properly. If I did not, please let me know - and I apologize in advance.
Edit: I inadvertently used the "update branch" button a couple times in the PR, not realizing it would create merge commits on the branch. I probably should have manually re-based and pushed instead - sorry!

'Up events' don't fire if the cursor or finger leaves the calling element 'Up events' include 'mouseup', 'pointerup', and 'touchend' events - move 'up event' listeners from the element to the window object - add delay to deactivation to ensure ripples only get fired once Fixes material-components#1328

…ial-components-web into fix/ripple-mouseup

googlebot · 2017-10-22T22:20:50Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Spetnik · 2017-10-22T22:22:21Z

I signed it!

googlebot · 2017-10-22T22:22:23Z

CLAs look good, thanks!

Passing util.applyPassive() instead of {passive: util.applyPassive()}

Spetnik · 2017-10-22T22:53:50Z

The IE11 test is failing on array.find() - I'll fix that tomorrow.

But the messages about "Unsatisfied verification on test double .removeClass." - I can use some help there. Does that have something to do with me adding a delay to the deactivation (mdc-ripple/foundation.js - line 422)? Or possibly because it will deactivate on both 'mouseup' and 'touchend' (line 410)? As the comment indicates, I added 'touchend' because without it if your finger slides off the element before being lifted, the ripple effect doesn't go away.

(also - I apologize for not doing automatic testing locally - sauce labs kept timing out when I tried it)

- IE11 doesn't support array.prototype.find() so manually code loop - no need to override existing activation - just allow touch to re-activate Fixes material-components#1328

Spetnik · 2017-10-23T10:16:49Z

Okay, I fixed the IE11 compatibility issue.

I believe at least one the failing tests is failing because of a false assumption.
In foundation-deactivation.test.js (https://github.com/material-components/material-components-web/blob/65a5936a3e4ec82b09fd620d2053f761536916d1/test/unit/mdc-ripple/foundation-deactivation.test.js) it states (line 345):

// At this point, the deactivation UX should have run, since the initial activation was triggered by
// a pointerdown event.

However, this is not always the case. If there is a touchstart event on the element, the mousedown and pointerdown will fire first. If the finger is then swiped away from the element before being lifted, the mouseup and pointerup events will not fire - only the touchend event will fire. Therefore we need to let touchstart trigger the activation and that's why the pointerup won't trigger deactivation, since we need to wait for touchend.

As for the second failure (same file, line 227), I am guessing this is failing because of the change starting at line 419 at mdc-ripple/foundation.js (https://github.com/Spetnik/material-components-web/blob/5aa2b6b7d9e1dcc5383ae5bd90d4861d9b9dae31/packages/mdc-ripple/foundation.js). I added a setTimeout() here because otherwise a touch event on the element would cause a double animation (because pointerdown, pointerup, touchstart, and touchend would all file sequentially, causing it to activate and deactivate twice - because of the issue mentioned in the previous paragraph we need to activate on touchstart as well as pointerdown). The setTimeout() prevents the immediate deactivation, which in turn prevents a second touch from re-activating within the DEACTIVATION_TIMEOUT_MS. As a result, the BG_ACTIVE_FILL and FG_ACTIVATION classes are only removed twice in this test scenario. Maybe we should change clock.tick(20) to clock.tick(DEACTIVATION_TIMEOUT_MS) for this test?

- 'keyup' also causes the same issue as the mouse events - there is no need to rerun the activation code on 'touchstart'

kfranqueiro · 2017-10-25T18:28:23Z

packages/mdc-ripple/index.js

-        instance.root_.addEventListener(evtType, handler, util.applyPassive()),
-      deregisterInteractionHandler: (evtType, handler) =>
-        instance.root_.removeEventListener(evtType, handler, util.applyPassive()),
+      registerInteractionHandler: (evtType, handler) => {


We should probably opt to add e.g. [de]registerBodyInteractionHandler methods rather than make this method "smarter" (see also mdc-slider which has separate methods for this).

Also, I'm actually not sure whether all of these events technically exist on window according to spec, so I think either document.body or document.documentElement may be a better choice.

Most importantly, though, I have reservations about immediately hooking up this many handlers on a root element of the document for each ripple instance on the page (there could be a lot of them). MDC Slider has code to hook up event handlers for the release only when a press occurs, which I think might be a better idea here as well (though be advised there may be caveats to that; see #1192 and #1484).

At a glance it looks like WHATWG supports mouseup and keyup on the window, but not pointerup or touchend. I chose to use window and not documentElement or body because I believe that's the only way to get the event to fire if the mouse leaves the window boundaries. But being that pointerup and touchend are not supported on the window it might just make sense to ignore such a scenario rather than to split the "up events" (although I'm guessing here on how the project looks at such an issue).

Edit: I understand your concern. I'll look at that issue and PR that you referenced.

@kfranqueiro What do you think about provisionally adding the events to window for the browsers that support it, while also adding it to the documentElement to comply with spec? This way we can handle the cursor leaving the window boundaries on browsers that support it.

Sorry for the slow response here. I'm reaching out to designers before answering this, because I had a thought about a possible alternative solution which might require less effort than managing document/window handlers, but I want to see which way seems correct to them first.

Okay, I've received design feedback, and I think we are going down the correct path (in fact we may need to do more work in addition to what this PR addresses, but we can get to the rest later).

My thoughts (feel free to comment if it seems like I've left something out or messed up):

I don't think keyup on the window should matter, since we're concerned with a bug related to events with pointing devices

We will want 4 new adapter APIs: registerWindowInteractionHandler, registerDocumentElementInteractionHandler, and their deregister equivalents

When a ripple is activated, we will register mouseup on window, and all up events on documentElement

May want to test whether we need mouseup on both, or whether mouseup on window is sufficient for all supported browsers

Within the up event callback, we will deregister all of the up event handlers

Again, there may be useful reference material for this in mdc-slider's foundation, though that simply listens for everything on body.

kfranqueiro · 2017-10-25T18:31:27Z

packages/mdc-ripple/foundation.js

@@ -404,7 +418,10 @@ class MDCRippleFoundation extends MDCFoundation {
      }

      if (needsActualDeactivation) {
-        this.activationState_ = this.defaultActivationState_();
+        // Prevents double-animation when 'mouse' and 'touch' events fire sequentially


Is this issue caused by the change above? I am a little concerned that the change at lines 408-410 is undermining what that code was initially designed to do, but I probably am not fully understanding what's being fixed.

Yes. The double-animation happens because of the 408-410 change. The reason for the 408-410 change is because when touching an element, mousedown, pointerdown, and touchstart all fire, but if the finger slides off the element before being lifted only touchend fires, but mouseup and pointerup never get fired (I didn't check the specs but I wonder if it's a browser bug). As a result, in such a case the ripple won't be deactivated by a mouseup so we need to check for a touchend as well. The problem is that then in a normal touch scenario (when the finger doesn't move) we would get a double activation/deactivation for both the mouse and touch events. That's why I added a setTimeout to delay the deactivation until after the animation period, which in turn prevents the second event from activating (since we don't allow an active ripple to be activated again).

Thanks for the information. This raises another concern, however, now that you explained this: this means that ripples no longer re-trigger if they are legitimately re-activated while the previous ripple is still animating. (E.g., click or keydown on a button twice relatively fast; in the existing implementation, the ripple will restart, but due to this fix it will no longer do that, which may make the component seem unresponsive, when it in fact was activated again.)

Interesting - I didn't realize that. What if we only allow a double animation if the second animation is fired by the same event (e.g. 'mousedown + mousedown' as opposed to 'mousedown + touchstart')? I think it would be rare that someone would use two different methods of activation in such a quick timeframe, no?

That thought occurred to me; I'm also wondering if it'd be feasible to use a tighter timeout to avoid this specific issue? i.e. when the sequential events occur due to one user input, do they actually occur nearly simultaneously, or is this a case where there's a tap delay?

I would need to test more thoroughly. When I lowered the delay to 100ms I think it still happened, but I'll check again and try to nail down the delay range.

kfranqueiro · 2017-10-25T19:11:48Z

packages/mdc-ripple/foundation.js

@@ -404,7 +418,10 @@ class MDCRippleFoundation extends MDCFoundation {
      }

      if (needsActualDeactivation) {
-        this.activationState_ = this.defaultActivationState_();
+        // Prevents double-animation when 'mouse' and 'touch' events fire sequentially
+        setTimeout(()=>{


Nit: add spaces around =>

'Up events' don't fire if the cursor or finger leaves the calling element 'Up events' include 'mouseup', 'pointerup', and 'touchend' events - move 'up event' listeners from the element to the window object - add delay to deactivation to ensure ripples only get fired once Fixes material-components#1328

Passing util.applyPassive() instead of {passive: util.applyPassive()}

- IE11 doesn't support array.prototype.find() so manually code loop - no need to override existing activation - just allow touch to re-activate Fixes material-components#1328

- 'keyup' also causes the same issue as the mouse events - there is no need to rerun the activation code on 'touchstart'

…ial-components-web into fix/ripple-mouseup

touficbatache · 2017-10-31T10:42:24Z

Looks like the Travis CI build failed. Does this also fix #809?

Spetnik · 2017-10-31T10:51:26Z

@touficbatache no it does not. I wasn't sure if that was by design or not. I can technically include it here or create a new PR for it.
See my comments above concerning the failures.

touficbatache · 2017-10-31T10:52:46Z

Oh, do you know what the issue is for #809? Or do you even have an idea on how to fix it? I would be willing to help you!

Spetnik · 2017-10-31T11:43:14Z

I have a guess based on my work in this PR. If I'm right, then it's related. I'll take a look and see if we can kill two birds with one stone.

touficbatache · 2017-10-31T11:49:19Z

😂 Okkay, I think it's better if you do it in another PR. Did you see my last comment on #809? It might help you!

kfranqueiro · 2017-10-31T13:45:40Z

Please leave #809 separate. There's also a chance that #809 may end up obsoleted by new things that design is experimenting with.

touficbatache · 2017-11-01T13:24:03Z

Hey @Spetnik, do you think you could fix #1080? The problem is that when you scroll with your finger on touch devices, elements with ripples get activated accidentally. I think touchstart is causing this issue. Setting needsDeactivationUX to true fixes the problem. However, the deactivation is not fully complete: you have to click again on the element to make it finish the deactivation process.

kfranqueiro · 2017-11-01T13:26:52Z

Please keep #1080 separate from this PR. FWIW I believe there is a design requirement around removing the ripple if a scroll is detected within ~80ms.

touficbatache · 2017-11-01T13:42:58Z

@Spetnik Oh, just found the issue for #809: expectedActivationType is touchstart when expectedActivationType should be is mousedown!

Let me explain, the ripple calls deactivation if the actual activation type (actualActivationType) is the expected activation type (expectedActivationType). expectedActivationType for mobile is touchstart strangely: so when touchstart fires, the ripple calls deactivation when it should call it when mousedown fires.

kfranqueiro · 2017-12-21T13:28:04Z

I've opened PR #1800 to attempt to resolve the issues originally approached in this PR. The discussion here certainly helped arrive at that, so thanks for the contribution! You're welcome to comment on #1800 if interested.

Spetnik · 2017-12-21T19:14:08Z

@kfranqueiro thanks - I apologize for not finishing this. Things got busy! I'll take a look at #1800

Spetnik added 3 commits October 22, 2017 16:16

Merge branch 'fix/ripple-mouseup' of https://github.com/Spetnik/mater…

5be6a44

…ial-components-web into fix/ripple-mouseup

Spetnik added 2 commits October 22, 2017 17:35

fix(ripple): fix util.applyPassive() parameter

4747d76

Passing util.applyPassive() instead of {passive: util.applyPassive()}

fix(ripple): remove rogue console.log()

967a524

fix(ripple): remove array.find() and enhance foundation code

5aa2b6b

- IE11 doesn't support array.prototype.find() so manually code loop - no need to override existing activation - just allow touch to re-activate Fixes material-components#1328

Spetnik added 3 commits October 23, 2017 15:06

Merge branch 'master' into fix/ripple-mouseup

aaff49d

Merge branch 'master' into fix/ripple-mouseup

a9100b8

fix(ripple): add 'keyup' to UP_EVENTS and remove unnecessary activation

55061c6

- 'keyup' also causes the same issue as the mouse events - there is no need to rerun the activation code on 'touchstart'

Spetnik mentioned this pull request Oct 24, 2017

Dragging the mouse causes ripple elements to retain highlighting #1328

Closed

kfranqueiro self-requested a review October 24, 2017 21:12

kfranqueiro suggested changes Oct 25, 2017

View reviewed changes

Spetnik added 7 commits October 25, 2017 14:40

fix(ripple): fix util.applyPassive() parameter

b74aee8

Passing util.applyPassive() instead of {passive: util.applyPassive()}

fix(ripple): remove rogue console.log()

2593cbf

fix(ripple): remove array.find() and enhance foundation code

a93310b

- IE11 doesn't support array.prototype.find() so manually code loop - no need to override existing activation - just allow touch to re-activate Fixes material-components#1328

fix(ripple): add 'keyup' to UP_EVENTS and remove unnecessary activation

ca683e0

- 'keyup' also causes the same issue as the mouse events - there is no need to rerun the activation code on 'touchstart'

Merge branch 'fix/ripple-mouseup' of https://github.com/Spetnik/mater…

ac30820

…ial-components-web into fix/ripple-mouseup

fix(ripple): added spaces around arrow

523e76a

Spetnik changed the title ~~fix(ripple): move 'up events' to window object #1328~~ fix(ripple): move 'up events' to window object - fixes #1124 Oct 31, 2017

lynnmercier assigned kfranqueiro Nov 1, 2017

kfranqueiro mentioned this pull request Dec 18, 2017

fix(ripple): Listen for up events at document level #1800

Merged

kfranqueiro closed this Dec 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ripple): move 'up events' to window object - fixes #1124 #1461

fix(ripple): move 'up events' to window object - fixes #1124 #1461

Spetnik commented Oct 22, 2017 •

edited

Loading

googlebot commented Oct 22, 2017

Spetnik commented Oct 22, 2017

googlebot commented Oct 22, 2017

Spetnik commented Oct 22, 2017 •

edited

Loading

Spetnik commented Oct 23, 2017 •

edited

Loading

kfranqueiro Oct 25, 2017

Spetnik Oct 25, 2017 •

edited

Loading

Spetnik Oct 31, 2017

kfranqueiro Oct 31, 2017

kfranqueiro Oct 31, 2017

kfranqueiro Oct 25, 2017

Spetnik Oct 25, 2017

kfranqueiro Oct 25, 2017 •

edited

Loading

Spetnik Oct 25, 2017

kfranqueiro Oct 25, 2017

Spetnik Oct 25, 2017

kfranqueiro Oct 25, 2017

touficbatache commented Oct 31, 2017

Spetnik commented Oct 31, 2017

touficbatache commented Oct 31, 2017

Spetnik commented Oct 31, 2017

touficbatache commented Oct 31, 2017

kfranqueiro commented Oct 31, 2017

touficbatache commented Nov 1, 2017

kfranqueiro commented Nov 1, 2017

touficbatache commented Nov 1, 2017

kfranqueiro commented Dec 21, 2017

Spetnik commented Dec 21, 2017

fix(ripple): move 'up events' to window object - fixes #1124 #1461

fix(ripple): move 'up events' to window object - fixes #1124 #1461

Conversation

Spetnik commented Oct 22, 2017 • edited Loading

googlebot commented Oct 22, 2017

Spetnik commented Oct 22, 2017

googlebot commented Oct 22, 2017

Spetnik commented Oct 22, 2017 • edited Loading

Spetnik commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Spetnik Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touficbatache commented Oct 31, 2017

Spetnik commented Oct 31, 2017

touficbatache commented Oct 31, 2017

Spetnik commented Oct 31, 2017

touficbatache commented Oct 31, 2017

kfranqueiro commented Oct 31, 2017

touficbatache commented Nov 1, 2017

kfranqueiro commented Nov 1, 2017

touficbatache commented Nov 1, 2017

kfranqueiro commented Dec 21, 2017

Spetnik commented Dec 21, 2017

Spetnik commented Oct 22, 2017 •

edited

Loading

Spetnik commented Oct 22, 2017 •

edited

Loading

Spetnik commented Oct 23, 2017 •

edited

Loading

Spetnik Oct 25, 2017 •

edited

Loading

kfranqueiro Oct 25, 2017 •

edited

Loading