Fix bug with tooltip positioning. #1674

RenatoUtsch · 2016-05-22T00:19:54Z

This fix corrects the position of the tooltip based on the translate property of the parents.

RenatoUtsch · 2016-05-22T00:25:30Z

travis seems to be failing because of the environment the tests are running, not because of the PR. Also, all the tests passed in my machine.

workgena · 2016-05-24T11:48:30Z

I have the same issue. And this PR does not solve my problem.

Also you have this commit
RenatoUtsch@f1754e3

In my case, this commit fixes issue with tooltip positioning.

RenatoUtsch · 2016-05-24T11:50:30Z

@workgena the problem is that that commit breakes tooltip positioning in other cases.
Can you please create a jsfiddle to show your issue, so that I can work on it?

workgena · 2016-05-25T11:46:02Z

Found strong dependence on bootstrap version.

Boostrap 2.3.1
http://jsfiddle.net/pL9sdgLu/1/

Boostrap 3.3.6
http://jsfiddle.net/bg5gsm2a/7/

This is CSS issue with bootstrap v3.3.6. Tooltip position has exact offset as has modal. To see it more clearly - resize demonstration window.

This is reason for error:

.modal.fade .modal-dialog {
  -webkit-transition: -webkit-transform .3s ease-out;
       -o-transition:      -o-transform .3s ease-out;
          transition:         transform .3s ease-out;
  -webkit-transform: translate(0, -25%);
      -ms-transform: translate(0, -25%);
       -o-transform: translate(0, -25%);
          transform: translate(0, -25%);
}
.modal.in .modal-dialog {
  -webkit-transform: translate(0, 0);
      -ms-transform: translate(0, 0);
       -o-transform: translate(0, 0);
          transform: translate(0, 0);
}

RenatoUtsch · 2016-05-25T12:40:09Z

Those jsfiddles worked for me. What browser are you using?

I tested on Safari, Firefox and Chrome, and in all of them your two jsfiddles worked. I even tried resizing the windows and everything was okay with both the chart and that modal example.

workgena · 2016-05-25T13:23:06Z

OS X Chrome and Firefox

good: http://awesomescreenshot.com/0845wb86a8
with error: http://awesomescreenshot.com/03e5wb7t2c

RenatoUtsch · 2016-05-25T15:34:26Z

Okay, I saw it. Will take a look at it later.

element with a translation, if it exists.

RenatoUtsch · 2016-05-25T17:29:25Z

Thanks, I made an even better fix because of what you said. This one now will probably work in all cases. Your example is working now.

I can't find any more problems with this PR, have everyone's problems been solved?

RenatoUtsch · 2016-05-25T17:32:14Z

Again, all checks have passed in my machine, this is a problem with the travis environment.

@liquidpele

liquidpele · 2016-05-25T23:22:43Z

Cool, will look at some PRS tonight.

workgena · 2016-05-26T14:02:26Z

Applied this PR on my project and it works fine.
Checked StackedArea and MultiBar charts.
Thx @RenatoUtsch

It would be good to have nvd3 1.8.4 with this fix.

liquidpele · 2016-05-26T14:22:57Z

src/tooltip.js

+                break;
+            }
+
+            obj = obj.parentNode;


This is always going to walk up the whole object tree every time it adjusts the tooltip position... which is a lot... What was getBoundingClientRect().top and left doing that was wrong exactly? From what I can tell it is supposed to take translate into account, there's even a bugzilla bug about it that they fixed at one point.

It is always going to walk the object tree, but it will stop in the case of
a translation. The problem is that if there is a parent that has a css
translation, the fixed positioning of children will be based on the
position of this parent, not the screen. And as d3.event.client* is based
on the position of the screen, it has to take into account the translation
of this parent. The complexity of walking up the tree is not good, I agree.

I had a good idea. We can just look at the parent of the tooltip, and if
it doesn't have a translation property, we add a translate(0,0). We would
only have to take into account the immediate parent of the tooltip and it
would always work.

I'll do this later today, thanks for the good idea @liquidpele.

Em Qui, 26 de mai de 2016 11:23 AM, liquidpele notifications@github.com
escreveu:

In src/tooltip.js
#1674 (comment):

\* If a parent of the container has a translate transformation, fix the

\* positioning. The fixed position of the tooltip will be based starting

\* at this element.

*/

var fixTranslation = function(pos) {

var obj = chartContainer;

while(obj && obj.style) {

if(getComputedStyle(obj).transform != 'none') {

var rect = obj.getBoundingClientRect();

pos.left -= rect.left;

pos.top -= rect.top;

break;

}

obj = obj.parentNode;

This is always going to walk up the whole object tree every time it
adjusts the tooltip position... which is a lot... What was
getBoundingClientRect().top and left doing that was wrong exactly? From
what I can tell it is supposed to take translate into account, there's even
a bugzilla bug about it that they fixed at one point.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/novus/nvd3/pull/1674/files/7aef28714acf8368a01264f36ea7f6d6db95c912#r64754094

A less invasive approach would be to put the nvd3 chart and the tooltip
inside a new div, and add a translate(0,0) to the new div. This would
ensure that adding the additional translation wouldn't break the rest of
the code of the user that depends on fixed positioning.

What do you think?

Em Qui, 26 de mai de 2016 12:04 PM, Renato Utsch renatoutsch@gmail.com
escreveu:

It is always going to walk the object tree, but it will stop in the case
of a translation. The problem is that if there is a parent that has a css
translation, the fixed positioning of children will be based on the
position of this parent, not the screen. And as d3.event.client* is based
on the position of the screen, it has to take into account the translation
of this parent. The complexity of walking up the tree is not good, I agree.

I had a good idea. We can just look at the parent of the tooltip, and if
it doesn't have a translation property, we add a translate(0,0). We would
only have to take into account the immediate parent of the tooltip and it
would always work.

I'll do this later today, thanks for the good idea @liquidpele.

Em Qui, 26 de mai de 2016 11:23 AM, liquidpele notifications@github.com
escreveu:

In src/tooltip.js
#1674 (comment):

\* If a parent of the container has a translate transformation, fix the

\* positioning. The fixed position of the tooltip will be based starting

\* at this element.

*/

var fixTranslation = function(pos) {

var obj = chartContainer;

while(obj && obj.style) {

if(getComputedStyle(obj).transform != 'none') {

var rect = obj.getBoundingClientRect();

pos.left -= rect.left;

pos.top -= rect.top;

break;

}

obj = obj.parentNode;

This is always going to walk up the whole object tree every time it
adjusts the tooltip position... which is a lot... What was
getBoundingClientRect().top and left doing that was wrong exactly? From
what I can tell it is supposed to take translate into account, there's even
a bugzilla bug about it that they fixed at one point.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/novus/nvd3/pull/1674/files/7aef28714acf8368a01264f36ea7f6d6db95c912#r64754094

Turn off CSS transforms for the time being, with a cost of painting speed. Issue is currently being looked at on the nvd3 repo PR: novus/nvd3#1674

RenatoUtsch · 2016-06-02T01:44:14Z

From what I've researched, we have only two choices to avoid having to walk up the DOM tree:

Add a 'translate(0,0)' to the tooltip parent if it doesn't have a translate transformation, so that we only need to look at it instead of the whole tree. The problem with this is that we're changing a div that we didn't create, and this can (and probably will) break user code.
Make the tooltip div a direct child of document.body, so that we only have to look if the document.body has a transformation. The problem with this is that we're not putting the tooltip together with the SVG.

I personally prefer the 2nd choice. First, it won't break any user code (except code that called chartContainer()? But I don't see a good use for changing it except for internal nvd3 code). Second, NVD3 originally worked like that, with the tooltip directly inside document.body, so we'll just be regressing to the original behavior (but much better now, the old code was a mess).

I'll implement the 2nd choice and update this PR. Anybody against it? @liquidpele?

workgena · 2016-06-02T07:39:54Z

I prefer putting tooltip into document.body. Many similar things(popup, modals, etc.) in other js-libs work this way.

* Use react-grid-layout instead of gridster * visualizations show and resize * display slice name and description; links work * positioning of widgets to match gridster, rowHeight matches * Change margins, rowHeight, unpositioned viz, and expandedSlices to match gridster * Saving dashboard, deleting slices, formatting on slices (chart control and resize handle), expanded slices fixed. * responsiveness + use es6 classes * Minor ui fixes + linting * CSS transforms on slices messes up nvd3 tooltip positioning. Turn off CSS transforms for the time being, with a cost of painting speed. Issue is currently being looked at on the nvd3 repo PR: novus/nvd3#1674 * Remove breakpoint listener, fires when it shouldn't (i.e. too often) * resize is no longer buggy, minor cleanup * gridster class, const, landscape error * one source of data for data to front end from python

RenatoUtsch · 2016-06-03T17:38:18Z

Done. Now the tooltip is directly into document.body. All the tests passed in my machine.

The performance is not a problem anymore @liquidpele.

liquidpele · 2016-06-10T14:33:13Z

Awesome, good job!

RenatoUtsch · 2016-06-10T22:19:23Z

Maybe a new 1.8.4 release would be good now, to finally put an end to all these tooltip-related bugs =)

Fix bug with tooltip positioning.

7387c9c

Merge branch 'master' of git://github.com/novus/nvd3

9bb0a88

Change the tooltip translation fix to use the bounding rect of the first

7aef287

element with a translation, if it exists.

RenatoUtsch mentioned this pull request May 25, 2016

Tooltip Position Wrong when the chart is in a dialog #1532

Closed

liquidpele reviewed May 26, 2016
View reviewed changes

Update the tooltip to be positioned directly at document.body.

955281a

liquidpele merged commit 9444bea into novus:master Jun 10, 2016

georgeke mentioned this pull request Jul 29, 2016

Use css transforms for dashboard grid cells apache/superset#848

Merged

rey-awesense mentioned this pull request Aug 23, 2017

Update the tooltip to be positioned directly at document.body Awesense/nvd3#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with tooltip positioning. #1674

Fix bug with tooltip positioning. #1674

RenatoUtsch commented May 22, 2016 •

edited

Loading

RenatoUtsch commented May 22, 2016 •

edited

Loading

workgena commented May 24, 2016

RenatoUtsch commented May 24, 2016 •

edited

Loading

workgena commented May 25, 2016 •

edited

Loading

RenatoUtsch commented May 25, 2016 •

edited

Loading

workgena commented May 25, 2016 •

edited

Loading

RenatoUtsch commented May 25, 2016

RenatoUtsch commented May 25, 2016

RenatoUtsch commented May 25, 2016

liquidpele commented May 25, 2016

workgena commented May 26, 2016

liquidpele May 26, 2016

RenatoUtsch May 26, 2016

RenatoUtsch May 26, 2016

RenatoUtsch commented Jun 2, 2016 •

edited

Loading

workgena commented Jun 2, 2016 •

edited

Loading

RenatoUtsch commented Jun 3, 2016

liquidpele commented Jun 10, 2016

RenatoUtsch commented Jun 10, 2016

Fix bug with tooltip positioning. #1674

Fix bug with tooltip positioning. #1674

Conversation

RenatoUtsch commented May 22, 2016 • edited Loading

RenatoUtsch commented May 22, 2016 • edited Loading

workgena commented May 24, 2016

RenatoUtsch commented May 24, 2016 • edited Loading

workgena commented May 25, 2016 • edited Loading

RenatoUtsch commented May 25, 2016 • edited Loading

workgena commented May 25, 2016 • edited Loading

RenatoUtsch commented May 25, 2016

RenatoUtsch commented May 25, 2016

RenatoUtsch commented May 25, 2016

liquidpele commented May 25, 2016

workgena commented May 26, 2016

liquidpele May 26, 2016

Choose a reason for hiding this comment

RenatoUtsch May 26, 2016

Choose a reason for hiding this comment

RenatoUtsch May 26, 2016

Choose a reason for hiding this comment

RenatoUtsch commented Jun 2, 2016 • edited Loading

workgena commented Jun 2, 2016 • edited Loading

RenatoUtsch commented Jun 3, 2016

liquidpele commented Jun 10, 2016

RenatoUtsch commented Jun 10, 2016

RenatoUtsch commented May 22, 2016 •

edited

Loading

RenatoUtsch commented May 22, 2016 •

edited

Loading

RenatoUtsch commented May 24, 2016 •

edited

Loading

workgena commented May 25, 2016 •

edited

Loading

RenatoUtsch commented May 25, 2016 •

edited

Loading

workgena commented May 25, 2016 •

edited

Loading

RenatoUtsch commented Jun 2, 2016 •

edited

Loading

workgena commented Jun 2, 2016 •

edited

Loading