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

When the tooltip is shown and for any reasons the target goes away from the DOM, the tooltip won't go away and it will be frozen on the screen. So, I added and binded DOMNodeRemovedFromDocument event (just 2 lines of code). #259

Merged
merged 3 commits into from
Mar 23, 2017

Conversation

antoniogiordano
Copy link
Contributor

…d and the tooltip is still visible

@huumanoid
Copy link
Contributor

Well, the problem indeed exists.
And this problem is extremely sucks!
Thank you for your patch!

For everyone who wants to reproduce the problem:
Check it out there http://hmnid.ru:8090/dom-remove.html
Here are the sources: https://github.com/huumanoid/react-tooltip-issue-test/blob/master/src/dom-remove.jsx

However, it seems that DOMNodeRemovedFromDocument is deprecated. Is it acceptable, or it's better to use MutationObserver?

@huumanoid huumanoid self-requested a review March 20, 2017 14:05
@huumanoid
Copy link
Contributor

Hello again, @antoniogiordano!

Could you remove e79f781f4eba1ed50a02de13bbfc635bcd113aa4 from this PR, please?

Also, I think this fix is not enough.
Now, tooltip is hidden when any target element has been removed, not only current.
Look:
vokoscreen-2017-03-20_17-25-37

I think we should check if the target of DOMNodeRemovedFromDocument equals to current target (this.state.currentTarget).
The example above is posted here.

Copy link
Contributor

@huumanoid huumanoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After cleanup and fixing the issue described here this PR can be merged, I guess.

@antoniogiordano
Copy link
Contributor Author

Hi Huumanoid! I removed the commit, now you can merge it.
I will also give a look to fix that issue

@antoniogiordano
Copy link
Contributor Author

I just added a check as you suggested, with your example it seems to work :-)

@huumanoid
Copy link
Contributor

I've just checked: it's awesome! Thank you a lot!
Note: you don't need to add hasTarget in checkSameTarget signature. hasTarget is needed for static methods. Probably, you should remove it for clarity.

@wwayne, please, take a look!

@antoniogiordano
Copy link
Contributor Author

I removed the option, check if it is fine ;-)

@huumanoid huumanoid merged commit 6a56af2 into ReactTooltip:master Mar 23, 2017
@huumanoid
Copy link
Contributor

Hey @antoniogiordano, please, check is this thing works in firefox?

@huumanoid
Copy link
Contributor

I'm afraid but it seems like the problem is more difficult than we expected:
https://github.com/huumanoid/react-tooltip/blob/fix-track-removal-firefox/src/decorators/trackRemoval.js

npm install github:huumanoid/react-tooltip#fix-track-removal-firefox-dist

Please, check it.

@antoniogiordano
Copy link
Contributor Author

Hi huumanoid, sure, I will give a look to MutationObserver to fix for firefox as well ASAP!

@huumanoid
Copy link
Contributor

huumanoid commented Mar 31, 2017

I'm sorry but it's already done
#278
Anyway, take a look, it's an interesting thing.
This MDN page states that

This feature has been removed from the Web standards. Though some browsers may still support it, it is in the process of being dropped. Avoid using it and update existing code if possible.

Adding DOM mutation listeners to a document profoundly degrades the performance of further DOM modifications to that document (making them 1.5 - 7 times slower!). Moreover, removing the listeners does not reverse the damage.

Thank you for your ideas and efforts, I hope it's over with this bug!

@rajatkb-typito
Copy link

rajatkb-typito commented Jan 11, 2022

Hi , @huumanoid I was actually working on some performance fixes in our application where we use React Tool Tip extensively, and I saw bunch of micro task being fired coming from this exact code that was merged here. I understand the use case. But is there anyway we can batch it under one Mutation Observer in anyway. Our application does have a few ReactToolTip enabled at same time , and by the looks of the code from what I understand the MutationObserver and it's function callback is unique to each ToolTip instances (correct me If did get that wrong , it seems the code is using decorators as way of merging the objects after creation).

I was thinking if somehow we can combine them i.e batch them, the micro task duration and number of fires can be controlled. Basically have one MutationObserver do the task of listening and then have every new instance of React tooltip hook onto the observer , since every tool tip instance is anyway listening to document modification only.

PS: The reason I am convinced about batching behaviour is because the multiple micro task that are fired have a self time of 0ms in the profiler. It's the Run Microtask phase which is filled with React Tool Tip's hide callback , which takes up most of the time. Also for most applications these would not matter but since we deal primarily with Canvas painting so getting the frame timing correct is important when done with user events.

Let me know if this makes sense would like to take up the PR for this if we are on same page 😅

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

Successfully merging this pull request may close these issues.

3 participants