spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

[Bug]: Memory Leak when we remove sp-tooltip(self-managed) from dom but not its parent

Open maheshwari-ashutosh opened this issue 3 years ago • 3 comments

Code of conduct

  • [x] 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 avatar Jun 14 '22 16:06 maheshwari-ashutosh

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.

Westbrook avatar Jun 14 '22 16:06 Westbrook

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.

maheshwari-ashutosh avatar Jun 14 '22 17:06 maheshwari-ashutosh

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.

Westbrook avatar Jun 14 '22 18:06 Westbrook