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: content global click listener causing issues at scale #24359

Closed
6 tasks done
tlaverdure opened this issue Dec 9, 2021 · 5 comments · Fixed by #24360
Closed
6 tasks done

bug: content global click listener causing issues at scale #24359

tlaverdure opened this issue Dec 9, 2021 · 5 comments · Fixed by #24360
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@tlaverdure
Copy link
Contributor

Prerequisites

Ionic Framework Version

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

Current Behavior

The ion-content component uses a click event listener to prevent clicks while scrolling.

@Listen('click', { capture: true })
onClick(ev: Event) {
if (this.isScrolling) {
ev.preventDefault();
ev.stopPropagation();
}
}

This was added in version 4.x to fix an issue described as "tap-click deadlock". I'm not exactly sure what that means. The PR doesn't really include a useful description or tests to explain what the functionality adds/fixes. However, there is a list of issues in the PR that are related to click functionality misbehaving while scrolling that were closed in effect:

fix(content): tap-click deadlock #17170

I've found that this event listener drastically decreases click performance with increasing amounts of DOM nodes within the ion-content component. Particularly in Webkit browsers which seem to handle event listeners that originate from a parent element differently than Chrome. Even with virtual scrolling added, the UI just feels slow and buggy when clicking. The only real solution that I've found is to not use ion-content but that would mean losing all the benefits of that component. Such as tapping the nav to scroll to the top of the page or using other components that should be children of ion-content like ion-refresher and ion-infinite-scroll.

Expected Behavior

If this code is no longer needed for current browser engines it should be removed, or allow developers to control this behavior with a setting in IonicConfig. For my purposes, I don't really need to block clicks while scrolling.

Steps to Reproduce

You can test these this StackBlitz links in Chrome and Safari. Both browsers are slower with ion-content but Safari is much worse.

With Ion Content

https://angular-ivy-63drzo.stackblitz.io/

Screen.Recording.2021-12-09.at.2.24.05.PM.mov

Without Ion Content

https://angular-ivy-zugcxz.stackblitz.io

Screen.Recording.2021-12-09.at.2.24.54.PM.mov

Code Reproduction URL

https://angular-ivy-63drzo.stackblitz.io/

Ionic Info

I'm using the latest version.

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Dec 9, 2021
@liamdebeasi liamdebeasi self-assigned this Dec 9, 2021
@liamdebeasi liamdebeasi changed the title bug: ion-content click performance bug: content global click listener causing issues at scale Dec 9, 2021
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Dec 9, 2021
@ionitron-bot ionitron-bot bot removed the triage label Dec 9, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the issue. I can reproduce this behavior. The code in question has its roots in the early Ionic v3 days, nearly 7 years ago: ionic-team/ionic-v3@43b8b4a#diff-99a1d9c14c8cea5a04cc5f1f7f7d688e924009325721f7ce17dd125bb425a0a3R38-R39

It appears that this was an attempt to prevent clicks from propagating while scrolling or other gestures were taking place. At the time this code was written, Chrome 43 was the latest Chrome and UIWebView was used instead of WKWebView. While I do not know the full context of this change, I think it is likely that this change was made to work around bugs in old browsers/webviews.

For most developers, the body of this onClick handler never executes as this.isScrolling is only modified when scrollEvents is true. This property defaults to false. As a result, I think it is fine to remove this listener altogether. Given 7 years of browser improvements, I think it is likely that whatever bug this code worked around has been resolved.


I removed the handler and tested the update in your application and it seems to help performance. There is definitely still an overhead from Angular/Zone.js, but the overhead from ion-content seems to have been resolved. Can you give the following dev build a try?

npm install @ionic/[email protected]

@liamdebeasi
Copy link
Contributor

Hey there,

I merged in a fix for this via #24360. Please feel free to continue testing and let me know if you run into any issues. Thanks again!

@tlaverdure
Copy link
Contributor Author

Gonna test it out in a sec, thanks for the quick response!

@tlaverdure
Copy link
Contributor Author

tlaverdure commented Dec 10, 2021

Hey @liamdebeasi this fix has provided some improvement. However, I'm still experiencing some sluggishness while clicking, even with zone.js disabled.

https://stackblitz.com/edit/angular-ivy-dmvpqp

Looking at things simply, when I don't use ion-content I can use hundreds of thousands of components in an application without issue. The only thing I can think of now is Safari not performing well with a ton of nodes within shadow DOM.

I found a couple links on bugs.webkit.org that you were involved in but haven't gotten anywhere 😢

https://bugs.webkit.org/show_bug.cgi?id=222187
https://bugs.webkit.org/show_bug.cgi?id=226330

From my own testing I can see that any type of user interaction with components within the shadow DOM tree causes rendering events labeled as "Styles Invalidated" which subsequently trigger "Styles Recalculated" events. The recalculations are taking > 400ms which seems to be blocking the UI.

Screen Shot 2021-12-10 at 2 19 55 PM

Do you know if there is a way to disable shadow DOM on the stencil components or is that only available during compilation of the core components?

If that isn't an option, my next course of action would be to implement my own content component for situations where I need to use a lot of components. Leaning this way would introduce challenges with integrating other ionic components though as they have hard assumptions around using ion-content1 (ex: querySelector('ion-content') 🤔

https://github.com/ionic-team/ionic-framework/search?q=%22an+ion-content%22&type=code

@ionitron-bot
Copy link

ionitron-bot bot commented Jan 12, 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 Jan 12, 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
2 participants