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

Revert tooltip observer change as it does not work properly. #2890

Merged
merged 2 commits into from
Oct 11, 2018

Conversation

JDutil
Copy link
Contributor

@JDutil JDutil commented Oct 3, 2018

fixes #2864

The change introduced by https://github.com/solidusio/solidus/pull/2421/files caused a regression in tooltips where they don't go away when the element is removed from the DOM. It appears the MutationObserver is meant for detecting child elements being removed from the target element, and it does not actually observe the target element itself being removed.
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
https://developers.google.com/web/updates/2012/02/Detect-DOM-changes-with-Mutation-Observers
https://stackoverflow.com/questions/28598340/mutation-observer-fails-to-detect-elements-dom-removal

So we can either revert the change to using MutationObserver or we would need to change it to observe a different target element, such as, the parent element of our actual target. I gave a go at wrapping the the target with a parent node, and observing the parent node and I couldn't easily get it to work either. So reverting seems easiest solution for now, and I changed from tooltip.hide() to tooltip.dispose() to unbind events as well.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Yes the (hacky) tricky here was intercepting the aria-describedby attribute change on the element itself, which seemed to happen on element removal as well on Chrome and Safari, but it's (now?) not happening on Firefox.

Wrapping into a parent div is another hack, not better than the timeout solution in my opinion, so 👍 with reverting that commit.

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Good revert. Thanks

@kennyadsl kennyadsl merged commit eb1ea02 into solidusio:master Oct 11, 2018
@JDutil JDutil deleted the hotfix/28674-dispose-tooltips branch October 11, 2018 18:01
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.

Bootstrap tooltips are broken on Firefox
3 participants