Tooltips: nvtooltip-pending-removal div is not removed on multiBarChart

[robinfhu]

On the development branch, when I open the file examples/multiBarChart.html, I mouse over the chart and I see the following:

When I inspect the DOM, there are lots of divs with 'nvtooltip-pending-removal' classed to them.

[robinfhu]

@petersondrew can you please take a look?

[RenatoUtsch]

I know that the bar charts use the old tooltip code, not the code that is used by the charts with interactive guideline (like the line chart).

[petersondrew]

Hmm, I'll take a look when I get into the office tomorrow.

[robinfhu]

Thanks. This is happening for pretty much any chart with the regular hover tooltip.

[petersondrew]

Alright, either I'm going crazy, or we're not comparing apples to apples. I just synced roadranger/nvd3/development with novus/nvd3/development, and when I open the multiBarChartTest.html under test I do not get this behavior at all, on chrome, ff, or IE11.

What browser are you testing in? Do you have any local changes, particularly with the nvd3 css file, that may be contributing?

[petersondrew]

Just in case it's somehow relevant, I appear to be on d3 3.5.4 locally.

[robinfhu]

@petersondrew , is it possible you have changes in roadranger/nvd3/development that were not merged in to our branch? You may have a different environment.

[petersondrew]

I don't believe so. I forked to roadranger/nvd3 yesterday and only committed the changes for #765. This morning I did a pull request from novus/nvd3 to sync my development branch with upstream.

I'm going to pull novus/nvd3 down as well and test there.

[petersondrew]

@robinfhu I just pulled novus/nvd3 and on development branch I do not see any problems with the tooltips on the multibarchart.html page. I tried under /examples and /test.

Can you please clone development into a clean folder, do a bower install, and see if you see the same issues?

[robinfhu]

@petersondrew , the reason is because you need to run 'grunt' once before loading the test pages.

Grunt creates build/nv.d3.js. In development, we don't checkin changes to that file, so you are using an older version of that.

[petersondrew]

Yeah, sorry. I just noticed that. I thought it would have been updated after 1.7.1.

[petersondrew]

@robinfhu I found the problem. The default task for grunt does not copy css. Try grunt production and it will copy nv.d3.css over to build/, which has the proper css for nvtooltip-pending-removal.

We really should remove build from the repo and add it to the.gitignore to avoid all this confusion.

[RenatoUtsch]

Wait, this is very important. Grunt needs to copy the css when doing its default task.

[robinfhu]

Yeah I'll change the grunt file

[robinfhu]

Good catch.

[RenatoUtsch]

I am copying the CSS but I don't see the div being removed...

[robinfhu]

@RenatoUtsch check again, this should work on development branch. Run 'grunt' before opening examples/multiBarChart

[RenatoUtsch]

@robinfhu I did it, and I am on the development branch.

Try checking out a clean copy and doing that.

I am using Chrome and OS X Yosemite.

[robinfhu]

@RenatoUtsch I can't reproduce your issue. Please upload a copy of your nv.d3.js and CSS file to JS fiddle and post it. Thanks.

[robinfhu]

Hold on, I see what you are saying. You need to be more specific. Do you mean that the actual DIVs are not being remove from the page (they are just set to opacity 0)

[petersondrew]

@robinfhu Ah, I see. So with the old style tooltips it doesn't re-use the container.

[robinfhu]

@petersondrew , I was confused. I thought @RenatoUtsch had the same visual issue above. What he REALLY meant is that the div element is just hidden, never physically removed.

[robinfhu]

@petersondrew sorry to bother you about this again, can you find a fix?

[petersondrew]

So, is it better to have a single "remove" method that just hides the div, and force the old tooltip code to re-use the container, or do we change the remove method so that it removes the div and introduce a new function that hides the re-usable container specifically for the interactive layer tooltips?

[robinfhu]

Yeah. Also I think there are advantages to not creating a reusable container for tooltips. For example, you may want to show tooltips simultaneously in two different charts side by side.

[liquidpele]

Oh, are they shared across charts?? Yea, we should probably not do that...

[robinfhu]

Yeah I'm now concerned about the reusable interactiveGuideline tooltip container. In our software, we have situations where tooltips show up for two charts at the same time. Wonder if this change would break that...

[petersondrew]

@robinfhu Do you mean "this change" as in what we're discussing to address removing these leftover tooltip container divs, or referring to the original change I made to make the container re-usable? If the latter, the pending-removal class (which in hindsight I should have used a new name) is only applied to the div owned by the interactive layer, so multiple interactive layers on a page will not interfere with one another (it uses the element id which is pseudo-randomly generated if I recall correctly).

[robinfhu]

@petersondrew I am referring to the big change you made originally. So if I understand correctly, each chart's tooltip operates independently, so two charts can show tooltips at the same time?

[petersondrew]

That is correct, I modified the interactive layer to locate its tooltip container by id, which is randomly generated per interactive layer instance, rather than by class which was the old behavior.

Play around with it and let me know if it breaks your scenario, but it should just work. 😄

[robinfhu]

@petersondrew thanks. Once this pending-removal bug is fixed I hope tooltips are in a good state.

[RenatoUtsch]

@robinfhu Sorry that I wasn't specific enough.

The DIVs used to be removed by the old nv.tooltip.cleanup() code, but after that big change @petersondrew made, the divs are now only hidden.

I am having a problem with this find by id thing. If, for example, the interactive guideline tooltips can't find the tooltip by ID, it creates a new one, resulting in the following:

The old tooltip code has the same behaviour in this case, but it hides any other existing tooltip DIVs before showing a new one:

[robinfhu]

@RenatoUtsch whoa, what do you have to do to get that first screenshot's situation?

[RenatoUtsch]

@robinfhu you have to put the chart inside a Shadow DOM:
https://github.com/RenatoUtsch/polynvd3

[petersondrew]

@RenatoUtsch I think locating a div by its id is an acceptable practice. Any issues created by doing so when it comes to virtual/shadom dom is far beyond the scope of this issue (and would need to be addressed solely in your polynvd3 fork, rather than in nvd3).

I'm more than happy to come up with a good way to reuse tooltips outside of the interactive layer, so that we don't orphan tons of divs. However I don't think that abandoning the use of getElementById to support shadom dom, at the expense of the performance gains we've made, is a good idea.

[liquidpele]

Weren't we selecting them by class before? Why would that work in shadowdom and selecting by ID wouldn't?

[RenatoUtsch]

By making a small change in the selection by class I can make the selection look inside all shadow doms.

Because you can have the same ids in different shadow doms, this feature is not available when selecting by ID, as conflicts can arise.

I think the ideal would be for nvd3 to support its usage, as it is something that will be really popular in the future, but I can make changes on my fork only.

[RenatoUtsch]

Oh and yes, the selection used to be by class.

[joshjordan]

Does that imply that getElementById is basically useless when using a
shadow DOM? That doesn't really seem like a reasonable requirement to place
on a project.

Is there a better way to choose when PR which DOM to apply the id selector
to?

On Fri, Feb 20, 2015, 10:10 AM Renato Utsch notifications@github.com
wrote:

Oh and yes, the selection used to be by class.

—
Reply to this email directly or view it on GitHub
#771 (comment).

[joshjordan]

When or which*

On Fri, Feb 20, 2015, 10:14 AM josh.jordan@gmail.com josh.jordan@gmail.com
wrote:

Does that imply that getElementById is basically useless when using a
shadow DOM? That doesn't really seem like a reasonable requirement to place
on a project.

Is there a better way to choose when PR which DOM to apply the id selector
to?

On Fri, Feb 20, 2015, 10:10 AM Renato Utsch notifications@github.com
wrote:

Oh and yes, the selection used to be by class.

—
Reply to this email directly or view it on GitHub
#771 (comment).

[liquidpele]

shadow dom doesn't really look like something worth supporting yet...

http://caniuse.com/#feat=shadowdom

[RenatoUtsch]

http://webcomponents.org/polyfills/

@joshjordan You can apply the ID selector inside each shadow DOM, but not through all of them.

[robinfhu]

@petersondrew any updates on the fix?

[robinfhu]

Since I would like this fixed, I am going to look into a solution.

[robinfhu]

The nv.tooltip.cleanup function isn't removing the tooltips at all.

[RenatoUtsch]

Yes, thats it. I brought that back but someone rolled it back.

The one thing I still don't understand is the delay when removing the tooltips. In the original code the delay was 500ms, in your code is 200ms, what's with this delay?

[robinfhu]

@RenatoUtsch I don't know, it was written by @bobmonteverde I believe, but I think it has to do with making the tooltip removal a non-blocking action. I think the timeout is arbitrary which is why I shortened it.

[liquidpele]

I wonder if a better solution is to save the tooltip object outside the tooltip call function, and then when we need to get it again, just do tooltip.node()... that would avoid all this class and ID selection stuff.

[RenatoUtsch]

YES, this would work with shadow dom too, and probably be faster.

[liquidpele]

This should be fixed in my PR: https://github.com/novus/nvd3/pull/851so closing this, let me know if the PR doesn't fix it!