ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.89k stars 13.52k forks source link

bug: tap click cancels gesture when used on button #22491

Open infacto opened 3 years ago

infacto commented 3 years ago

Bug Report

Ionic version: [x] 5.x (5.4.4)

Current behavior: If the GesturesController is used on an Ionic button. The ripple-effect does not work anymore.

Expected behavior: The ripple-effect should work in this case.

Steps to reproduce: In my case: Create a directive with the Ionic GesturesController and assign it to a ion-button or ion-fab-button.

Related code:

import { GestureController } from "@ionic/angular";
/* ... */
constructor (private gestureCtrl: GestureController) {}
/* ... */
  this.gestureCtrl.create({
      gestureName: "long-press",
      el: this.nativeElement,
      threshold: 0,
      onStart: detail => {
        console.log("onStart", detail);
        this.onStart.emit(detail.event);
      },
      onEnd: detail => {
        console.log("onEnd", detail);
        this.onEnd.emit(detail.event);
      }
    });
  <ion-fab-button
    long-press
    (onStart)="log('fab:onStart')"
    (onEnd)="log('fab:onEnd')"
  >
    <ion-icon name="bug"></ion-icon>
  </ion-fab-button>

Stackblitz Demo

gestures-ripple-issue

Other information:

I guess it's about event propagation. The ripple effects does not get any clicks. But how to fix this? I already tested some options like passive.

https://ionicframework.com/docs/utilities/gestures

liamdebeasi commented 3 years ago

Thanks for the issue. It looks like when we capture a gesture, we cancel any active ripple effect logic: https://github.com/ionic-team/ionic-framework/blob/master/core/src/utils/tap-click.ts#L150 This is probably to prevent the ripple effect from firing when you are using ion-reorder or other components.

infacto commented 3 years ago

Hmm 🤔 any idea for a workaround? It's a really bad UX right now. I would request higher priority for this issue.

liamdebeasi commented 3 years ago

I don't think there's a good workaround right now. I was thinking of a solution and it might make sense to only call cancelActive if the ionGestureCaptured event comes from certain components that would benefit from having cancelActive called.

For example, it doesn't really make a difference to cancel the gesture on an ion-button, but an ion-reorder would get messed up with conflicting gestures.

I likely will not have time to tackle this soon, but feel free to take a look at it yourself -- I'm happy to review a PR for it. The change probably needs to be made in this file: https://github.com/ionic-team/ionic-framework/blob/master/core/src/utils/tap-click.ts#L150. I imagine it would look something like this:

const allowList = ['ion-button'];
doc.addEventListener('ionGestureCaptured', (event) => {
  // if event target is NOT in allow list of components
    // call cancelActive()
  // otherwise return
});

This pseudo code would only call cancelActive if the gesture was not captured on ion-button. There may be other components to add to the allowList, but we can add them on an as needed basis.

Since ionGestureCaptured is a custom event, we might need to pass in the element target here: https://github.com/ionic-team/ionic-framework/blob/b40f7d36d5802251a0b44a7dd2599f19dbe977d5/core/src/utils/gesture/gesture-controller.ts#L44.

ionitron-bot[bot] commented 3 years ago

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

infacto commented 3 years ago

This issue becomes attentions when the Ionic team implements the gestures like tap and press (long press) to the buttons, etc. like in Ionic 3. What I sorely miss in Ionic 4+. The reason I've created an own directive with Ionic Gestures. Anyway...

In this case the ion-reorder wants to disable the ripple effect. In other cases you want to keep the ripple effect. For example, when Ionic official supports the tap and press events again.

TL;DR: Another suggestion is to set this as an option. Disable ripple effect (boolean) in the GesturesController. Would this possible? It's maybe more simpler and controllable as the allow list in the custom captured event.

import { Gesture, GestureController } from '@ionic/angular';
/* ... */
constructor(private gestureCtrl: GestureController) {
  const gesture: Gesture = this.gestureCtrl.create({
    el: this.element.nativeElement,
    threshold: 15,
    gestureName: 'my-gesture',
    onMove: ev => this.onMoveHandler(ev),
    disableRippleEffect: true /* <<< */
  }, true);
}

Maybe another name or inverted. But I hope I was able to explain my proposal well. So with this option you (Ionic internal and the user) have the control over it. It's more related... But I'm not fully involved to the Ionic sources. What do you think about?

liamdebeasi commented 3 years ago

Just to be clear, the Ionic team did not implement tap and press. Those were provided by Hammer.js, and Ionic v3 bundled Hammer.js with all Ionic apps.

Thanks for the suggestion on how to resolve the issue! Unfortunately, I do not think that adding an API to the gesture utility is a good idea. As I mentioned in https://github.com/ionic-team/ionic-framework/issues/22491#issuecomment-726797867 the problem is not with the gesture utility but with how the framework's other utilities respond to gestures being captured. I am not 100% set on the my initial solution, but something along those lines probably would resolve this issue while still being fairly flexible towards other components that have gestures.

infacto commented 3 years ago

I know that Hammer.js was used. But has been removed from Ionic anyway. So it makes no difference. But if I understood it correctly, it is planned to implement such events in the future. I can also understand why the decision was made to kick Hammer.js. It's obsolete and may not fit in optimally. All fine... But it would be nice if Ionic already provided these events. That was just an unpleasant surprise for the Ionic 3 migration. ...

Back to the issue, I wonder what happens if the event ionGestureCaptured is ignored? Especially for the ion-reorder. What was the issue? What's wrong to trigger ripple effect in this case? Perhaps there are cases where the ripple effect is desired and other cases it is not. It depends on the gesture type like tap, press or move etc. If we now determine which elements allow this and which do not, it may not always be intended that way. ... Anyway, elements like buttons (ion-fab-button, ion-button) should never disable it. 🤔 So your solution would resolve it. 🙏 Hopefully as soon as possible. Meanwhile, I'm still looking for a workaround. 🔧

Update: My current workaround is to add following in e.g. main.ts:

window.addEventListener(
  'ionGestureCaptured',
  (event) => {
    event.stopPropagation();
  },
  true
);

Demo - It just captures the event and stops propagation to its childs. So, the document never receives the event anymore. Works 👍 (I don't use ion-reorder or elements who should listen to it yet.)

liamdebeasi commented 3 years ago

Back to the issue, I wonder what happens if the event ionGestureCaptured is ignored? Especially for the ion-reorder. What was the issue? What's wrong to trigger ripple effect in this case?

In the case of ion-reorder, the ripple effect should only be applied after a tap/click completion. The problem is that between mobile and desktop taps and clicks are handled slightly differently. This is where our tap click utility comes into play and helps bridge the gap between the two: https://github.com/ionic-team/ionic-framework/blob/master/core/src/utils/tap-click.ts. If we did not call cancelActive when ionGestureCaptured is fired, then you would see the ripple effect applied to ion-reorder when you click and drag it which is not correct.

Anyway, elements like buttons (ion-fab-button, ion-button) should never disable it.

Agreed, ion-fab-button should be on the list of excluded components when it comes to calling cancelActive.

Demo - It just captures the event and stops propagation to its childs. So, the document never receives the event anymore. Works 👍

Nice! Glad you were able to find a workaround.

mp035 commented 1 year ago

I came across this rather annoying bug whilst trying to allow long-taps on a button to trigger the click event. Thanks @infacto for the work around. I thought I'd add a little tweak which allows you to filter out the added touch events, and leave others active (if you have controls that need it). Adjust the gesture name to match that of the touch gesture that you create and add the snippet to main.ts

window.addEventListener(
  'ionGestureCaptured',
  (event: any) => {
    if (event.detail?.gestureName === 'your-custom-gesture-name-goes-here'){
      event.stopPropagation();
    }
  },
  true
);
mikelhamer commented 1 month ago

Is this still relevant?

mp035 commented 1 month ago

I still have the fix in place, but my project is running ionic 7.0.14 so I can't confirm or deny. Sorry.

mp035 commented 1 month ago

Update: looks like the workaround is in the project, but unused, so my best guess is that it's no longer a problem.