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]: Memory Leak when we remove sp-tooltip(self-managed) from dom but not its parent #2359

Closed
1 task done
maheshwari-ashutosh opened this issue Jun 14, 2022 · 3 comments · Fixed by #3456
Closed
1 task done
Labels

Comments

@maheshwari-ashutosh
Copy link

maheshwari-ashutosh commented Jun 14, 2022

Code of conduct

  • I agree to follow this project's code of conduct.

Impacted component(s)

sp-tooltip

Expected behavior

When client conditionally render the sp-tooltip, when its removed from DOM it should remove the event listener from the parent. so that when we hover on the parent the active-overlay should not be added on the DOM.
See : https://studio.webcomponents.dev/edit/dcmsShnRpGqlmmaBvPWy/src/index.ts?p=stories

When the second time user hovers for 1.5secs on the card, it should look like this
Screenshot 2022-06-14 at 9 51 23 PM

Actual behavior

When the second time user hovers for 1.5secs on the card, it looks like this
Screenshot 2022-06-14 at 9 49 13 PM

Screenshots

No response

What browsers are you seeing the problem in?

Chrome, Safari

How can we reproduce this issue?

  1. Go to '...'
  2. Click on '....'
  3. Scroll to '....'
  4. Check console
  5. See error

Sample code that illustrates the problem

Please check the link in expected behaviour section

Logs taken while reproducing problem

No response

@maheshwari-ashutosh maheshwari-ashutosh added bug Something isn't working triage An issue needing triage labels Jun 14, 2022
@Westbrook
Copy link
Contributor

Westbrook commented Jun 14, 2022

Does it work to leverage the tooltip like this? https://studio.webcomponents.dev/edit/zig2Krhi5Ovi6vSrUMcz/src/index.ts?p=stories

In your example you duplicating features of the tooltip within your implementation of the Card that you can omit all together.

@maheshwari-ashutosh
Copy link
Author

It works functionality wise but lets say we have many cards in a single page, then it will always be in the DOM for each and every card element even when its not visible thus increasing the DOM size.

@Westbrook
Copy link
Contributor

I'd love to see the stats at which point you actually see a perf drop off here. Very interesting area, but not something that benefits from optimizations that aren't supported by measurement.

Regardless, the self-manged attribute is not designed to support this sort of use case. "Self managed" meaning very explicitly, "it does it for itself". There is lots of clean up that can only be done when an overlay is closed and removing the element from the DOM before that time means you're breaking a contract with the element. This can cause not only the situations you've listed here can also skip expected animations and events that could be required by other parts of an application. It may be something we look at supporting in the future, but it is fairly low in our priorities right now.

When you're ready to jump to some level of optimization, you've got a couple of options available to you:

  • You can choose to throw a dynamic tooltip this leverages the whole of the Overlay API to manage the tooltip in you custom manner. This presents a chance that your overlay here will diverge from other overlays overtime as while these are the APIs that SWC uses internally the style and configuration of that usage could change in a way that would be otherwise non-breaking to the library. It does however, align to give you a chance to choose whether to throw a tooltip based on the length of the headline that you are supporting.
  • You could leverage a tooltip directive. This is a little nicer version of the above and locally it would exhibit all the same issues, however we are discussing this implementation with other teams right now. While it's not 100% clear when it would become a priority/be ready to ship, the idea that this aligns with your desires here might be of interest to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants