Tooltip chartContainer defaults to document.body, which breaks tooltips with other chartContainers

[foolmoron]

Apologies if this has been reported already. There's a lot of tooltip bugs, but I don't think this particular one is listed.

Charts without useInteractiveGuideline use tooltips without giving them a chartContainer, which defaults to document.body. This is a problem because in initTooltip, there is a line d3.select(container).selectAll('.nvtooltip'), which selects all the tooltips on the page, including ones from other charts, and the rest of the function breaks the other tooltips in unpredictable ways.

Charts with useInteractiveGuideline set the chartContainer to parentNode, which works perfectly fine with any other chart. But if we don't want useInteractiveGuideline, we have to manually set the tooltip chartContainer in order for tooltips not to break.

Why not just make all the tooltips default their chartContainer to parentNode?

[liquidpele]

I think this might be fixed in some of the latest work done by @RenatoUtsch ...

[RenatoUtsch]

Yeah, what I did was to actually put all the tooltips in document.body and remove the chartContainer property, because after some time working on the tooltips I reached the conclusion that there is no way to use that property and make the tooltips work efficiently.

So this issue can be closed, it was already fixed and should be ok in the next version.

On Sun, Jul 3, 2016, 10:15 AM liquidpele notifications@github.com wrote:

I think this might be fixed in some of the latest work done by @RenatoUtsch https://github.com/RenatoUtsch ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/novus/nvd3/issues/1680#issuecomment-230164111, or mute the thread https://github.com/notifications/unsubscribe/AAzC7eOlLPQMmT7gM5KyqDapKqQvCHPXks5qR-4ggaJpZM4IrywP .

[foolmoron]

As long as the new tooltip code allows for many charts on the same page that should be okay. The main problem was that the previous tooltip accidentally clobbered other tooltips that shared the same container.

[RenatoUtsch]

I think it should be fine, the dom element is saved with the current code.

On Tue, Jul 5, 2016, 11:39 AM Momin Khan notifications@github.com wrote:

As long as the new tooltip code allows for many charts on the same page that should be okay. The main problem was that the previous tooltip accidentally clobbered other tooltips that shared the same container.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/novus/nvd3/issues/1680#issuecomment-230564844, or mute the thread https://github.com/notifications/unsubscribe/AAzC7ezq5wJw2dL5DSEh9HUUELpYcdyqks5qSqTJgaJpZM4IrywP .

[Fireworks]

The hard deprecation of chartContainer breaks our use case for a customized static tooltip inside a donut chart. While slightly hacky, the only way I could figure out a solution for what I wanted to achieve was to utilize a custom tooltip and anchor it inside the donut chart container to position it staticly. It surprised me to find this breaking change in a minor version bump.

@RenatoUtsch

[Fireworks]

@RenatoUtsch @liquidpele Pinging for visibility. I'm sure we will be fine locked to the lower version for awhile, but want to get an idea if this is a permanent change.

[RenatoUtsch]

Hi @Fireworks, this will probably be a permanent change. Any other solution would either break tooltip positioning in some cases or break other elements in the page due to changes in parent elements that we do not own.

You can still have a static tooltip inside the donut chart, you just have to replace the tooltip.position() function with a function that returns the position of the chart relative to the viewport (Element.getBoundingClientRect()) plus some offset to position it in the right place at the chart.

I agree with you, though, that this change should not have been done in a patch version bump, it should have been a minor version bump. Sorry about that.

[liquidpele]

I'm fine with putting chartContainer back and just setting it so that the assumption is that no parent containers are using translate... as long as the default is document.body then I din't see a reason to not let people override that if they know what they're doing. Just add info about the limitation to documentation.html file for that option.

[cubissimo]

I'm facing various tooltip's bugs with angular material... maybe putting chartContainer back is the way to go... i think give the choice to override to developer give us better control

[liquidpele]

I'm going to make a new minor version soon, so if someone wants this to be fixed in the next version please send me a pull request :)

[RenatoUtsch]

@cubissimo what kind of bugs are you facing? Can you give an example?

This was indeed a breaking change that should not have gone in a minor version, but the only real complain to this change was easily fixable by overriding the position() function, which is a better way because changing the chartContainer() will break the rest of the tooltip positioning code.

If @cubissmo shows a buggy example I can take a look and see how to fix it.

[Fireworks]

@RenatoUtsch I don't have the time at the moment to write out a full example of the use case I was describing earlier, but what I can say is that overriding position() was not a solution for me. I needed a way to anchor the tooltips to the chartContainer so that I could render them staticly in the center of a donut chart. I am not sure how I would achieve this simply with the position function...

A bit confused why having the option for overriding is a big deal, since if we know what we doing in the override, it would only give more control to the developer.

[RenatoUtsch]

The thing is, if you change the chartContainer you'll also have to change the position() function, as it assumes that the chartContainer is the body element.

You can get a position statically in the center of a donut chart by getting the position of the chart relative to the viewport and adding half the width and height of the chart to position them in the center. So something like this would position the tooltip in the middle of your chart:

tooltip.position = function() {
    var brect = chart.getBoundingClientRect();

    return {
        top: brect.top + brect.height / 2,
        left: brect.left + brect.width / 2
    };
}

Now, I guess that if there was documentation explaining that if you change chartContainer() you also will have to change position(), it would be ok to put it back.

[cubissimo]

@RenatoUtsch i'll post here a plunker asap. ty

[mathewcst]

@RenatoUtsch the problem that I'm facing is: using nvd3 inside md-grid-title (angular material) the Tooltip goes to the top left corner of the page instead of following the mouse in bar charts, for example.

I'm not fiding any way to make it work.

[nicysneiros]

I'm also trying to place the tooltip in the center of the donut and I was able to accomplish this using @RenatoUtsch workaround. However I'm facing a scrolling issue, specially in mobile: the tooltip does not scroll with the pie chart, because it is placed in the body instead of in the chartContainer.

One fix that I can think of to avoid the "floating" tooltip, would be to hide it on scroll event. But this is not very good for performance. The chartContainer variable provided a much cleaner solution for this... Can you guys think of any other option for this issue?

[RenatoUtsch]

This is surprisingly not an easy problem to solve. I've been trying to fix this for years in different ways because my use cases (and others) were broken, and with each change that I thought would solve the problem a different use case would break.

@nicysneiros how do you scroll the pie chart? The current implementation was supposed to work even with scrolling inside divs. Can you provide a jsfiddle that reproduces the issue? Even if the tooltip is on body, the position function should position the tooltip based on the chart's position relative to the viewport.

I can think of only one way to create a real, definitive solution that covers all use cases. However, this would need to break the current API. The solution is to put the chart inside a div that is managed by nvd3, and not by the user of the library. If we have control of the div, we can properly (and easily) place the tooltip together with the chart. However, as right now the chart can be anywhere, we can't make any assumptions and change the div's CSS, because we might break the user's page.

By the way, I don't think nvd3 was made with mobile browsers in mind, you'll probably have some problems while using it in mobile.

[liquidpele]

I thought the plan was to keep it on body, and deprecate the tooltipContainer option or make it step all the way up through the dom to calculate position?

Also, I did test on an ipad and android a while back, it should work for anything current... although it doesn't utilize the touch actions the browsers still handle it pretty well.

[RenatoUtsch]

Yeah, but it seems that it didn't work? A few people popped up complaining that it isn't working. I have yet to see a real example of issues, but if it's true maybe we have to change this again.

Oh, and good to know, last time I tried it was a bit buggy but that was a long time ago.

On Wed, Jan 11, 2017, 22:49 liquidpele notifications@github.com wrote:

I thought the plan was to keep it on body, and deprecate the tooltipContainer option or make it step all the way up through the dom to calculate position?

Also, I did test on an ipad and android a while back, it should work for anything current... although it doesn't utilize the touch actions the browsers still handle it pretty well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/novus/nvd3/issues/1680#issuecomment-272043199, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzC7epWWx1vWbTAScXNk40l__5UbY7wks5rRXiPgaJpZM4IrywP .

[nicysneiros]

Hey @RenatoUtsch, I found it better to show you with a screencast of the behaviour:

• Version without chartContainer:
• Version with chartContainer:

For now, I chose to use the old version that supports chartContainer, since I need to deliver this feature. As @liquidpele, I also didn't have any trouble using nvd3 for mobile, except for this one. Maybe this issue is more related to adding support for touch events than to chartContainer attribute. I couldn't figure this out 😞

[RenatoUtsch]

Could you show the code you used to place the tooltip?

On Thu, Jan 12, 2017, 12:31 Nicolle Cysneiros notifications@github.com wrote:

Hey @RenatoUtsch https://github.com/RenatoUtsch, I found it better to show you with a screencast of the behaviour:

• Version without chartContainer: [image: screen capture on 2017-01-12 at 11-12-44] https://cloud.githubusercontent.com/assets/3246075/21893604/6b84038c-d8ba-11e6-95c5-1a8ee6ec9b41.gif
• Version with chartContainer: [image: screen capture on 2017-01-12 at 11-14-27] https://cloud.githubusercontent.com/assets/3246075/21893608/6f875038-d8ba-11e6-9336-ae130036db6f.gif

For now, I chose to use the old version that supports chartContainer, since I need to deliver this feature. As @liquidpele https://github.com/liquidpele, I also didn't have any trouble using nvd3 for mobile, except for this one. Maybe this issue is more related to adding support for touch events than to chartContainer attribute. I couldn't figure this out 😞

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/novus/nvd3/issues/1680#issuecomment-272177271, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzC7V4D0wxHqKnXhjwZkjuM5qKRwxpKks5rRjkugaJpZM4IrywP .

[nicysneiros]

@RenatoUtsch, here is a snippet:

• Without chartContainer in the chart object:

  ...
  tooltip: {
  position: function() {
  var brect = d3.select('.activity-score-chart g.nv-pie:not(nv-wrap)')[0][0].getBoundingClientRect();
  var tooltipRect = d3.select('.nvtooltip')[0][0].getBoundingClientRect();
  
  return {
    top: brect.top + (brect.height / 2) - (tooltipRect.height/3),
    left: brect.left + (brect.width / 2) - (tooltipRect.width/1.5)
  };
  }
  }
  ...

• With chartContainer in the chart object:

  ...
  tooltip: {
  chartContainer: '.activity-score-chart',
  position: function () { return { top: 100 } }
  }
  ...

[RenatoUtsch]

I'm sorry, but the code you provided should not result in the GIF you sent. The only possibility is that the "brect" bounding rect is not the chart element, but something else that is not being moved when you scroll.

Please make sure that the selection is done using the SVG, not the enclosing div:

tooltip.position = function() {
  var svgRect = d3.select("#svgElementOfTheChart").node().getBoundingClientRect();
  var tooltipRect = d3.select('.nvtooltip').node().getBoundingClientRect();

  return {
    top: svgRect.top + (svgRect.height / 2) - (tooltipRect.height / 3),
    left: svgRect.left + (svgRect.width / 2) - (tooltipRect.width / 1.5),
  };
};