Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: ion-activated class removed when contextmenu event #25544

Closed
4 of 7 tasks
Klapik opened this issue Jun 28, 2022 · 7 comments · Fixed by #25551
Closed
4 of 7 tasks

bug: ion-activated class removed when contextmenu event #25544

Klapik opened this issue Jun 28, 2022 · 7 comments · Fixed by #25551
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@Klapik
Copy link

Klapik commented Jun 28, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

When I hold element with ion-activatable class, hover only appears for a moment. Class ion-activated is removed after a while. It's because changes in tap-click.ts file - event contextmenu is fired on longpress and after the changes, the hover breaks.
link to changes

Expected Behavior

When I hold element with ion-activatable class, hover should stay as long as the hold is taking.

Steps to Reproduce

  1. Add an element with a class to the view, e.g. <div class="ion-activatable">test</div>.
  2. Hold element on mobile device.

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.18.1 (/usr/local/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 6.1.11
@angular-devkit/build-angular : 13.3.3
@angular-devkit/schematics : 13.3.3
@angular/cli : 13.3.3
@ionic/angular-toolkit : 6.1.0

Capacitor:

Capacitor CLI : 3.4.3
@capacitor/android : 3.4.3
@capacitor/core : 3.4.3
@capacitor/ios : 3.4.3

Cordova:

Cordova CLI : 10.0.0
Cordova Platforms : not available
Cordova Plugins : not available

Utility:

cordova-res : not installed globally
native-run : 1.5.0

System:

NodeJS : v16.14.2 (/usr/local/bin/node)
npm : 8.5.0
OS : macOS Big Sur

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jun 28, 2022
@liamdebeasi liamdebeasi self-assigned this Jun 28, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. The code in 2c07a15#diff-a2c042e751c5816179d1f73c952a6828a85b41cdf1f31d8d27f78222f7d4769fR163 was added to account for the .ion-activated class not being removed when right clicking an element when developing. We found this while trying to inspect elements with developer tools.

This change had the unintended side effect of causing .ion-activated to be removed when long pressing on a button on Android as the contextmenu event is fired there too. We were likely testing on iOS at the time, and iOS does not fire the contextmenu event at the moment (https://bugs.webkit.org/show_bug.cgi?id=213953).

In terms of fixing this, we can likely do the following:

  1. Remove the contextmenu event listener.
  2. Add code to the onMouseDown to exclude right clicks. This will result in the ripple effect/activated effect not being added when right clicking with a mouse.

@Klapik
Copy link
Author

Klapik commented Jun 28, 2022

In terms of fixing this, we can likely do the following:

  1. Remove the contextmenu event listener.
  2. Add code to the onMouseDown to exclude right clicks. This will result in the ripple effect/activated effect not being added when right clicking with a mouse.

Thanks for the quick reply, the repair proposal sounds very good

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Jun 28, 2022
@ionitron-bot ionitron-bot bot removed the triage label Jun 28, 2022
@liamdebeasi
Copy link
Contributor

Can you give this dev build a try and let me know if it resolves the issue on your end?

npm install @ionic/[email protected]

@liamdebeasi liamdebeasi removed their assignment Jun 28, 2022
@Klapik
Copy link
Author

Klapik commented Jun 28, 2022

In @ionic/[email protected] When I hold element with ion-activatable class, hover stays as long as the hold is taking, good job!

One more thing that i found is the class ion-activated is removed on Android on pointercancel event (fired on touchmove) - this is not blocker because it works much better anyway. (i think there is no need to create new issue for this)
On ios, hover stays despite the touchmove. I also checked the application with the Ionic 5 version and on Android also the hover remains despite the touchmove. I found the reason behind this is the last fix for ripple effect when scrolling.

I think we can live with the difference and your fix for contextevent is sufficient. Good job and thanks!

@liamdebeasi
Copy link
Contributor

Glad to hear the issue is resolved.

The other behaviors you noted align more closely with how native iOS and Android apps behave. The activated state should be removed when scrolling (This is what pointercancel controls). There is a bug in Safari where the pointercancel event does not fire under certain scenarios, but I believe that behavior existed even before the fix in 0b275af was implemented.

#25352 (comment) has sample videos if you are interested in all the details.

I will keep this issue open until the fix has merged into main. Thanks for the report!

@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #25551, and a fix will be available in an upcoming release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Jul 30, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants