Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Commit

Permalink
fix(tooltip): corrected md-tooltip positioning when scrolled
Browse files Browse the repository at this point in the history
There are several issues out there (e.g. #2406) that point to tooltip positioning when the page is scrolled and
getNearestContentElement that suggests that the enclosed loop is supposed to stop at md-content but does not.
Adding that condition to the loop fixed the positioning issue for me.

Fixes #2406. Closes #5161.
  • Loading branch information
kasiarachwal authored and ThomasBurleson committed Oct 14, 2015
1 parent c9f822d commit f62f693
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/components/tooltip/tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function MdTooltipDirective($timeout, $window, $$rAF, $document, $mdUtil, $mdThe
function getNearestContentElement () {
var current = element.parent()[0];
// Look for the nearest parent md-content, stopping at the rootElement.
while (current && current !== $rootElement[0] && current !== document.body) {
while (current && current !== $rootElement[0] && current !== document.body && current.nodeName !== 'MD-CONTENT') {
current = current.parentNode;
}
return current;
Expand Down

17 comments on commit f62f693

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Nov 19, 2015

Choose a reason for hiding this comment

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

@kasiarachwal @ThomasBurleson This broke all my tooltips and it appears to haven broken several others too. See #5161 (comment)

@kasiarachwal Do you have a demo of what situation this fixed tooltips for? In my case I have fab buttons and speed dials inside of md-content using tooltips. This also broke regular tooltips just inside of md-content.


image

PS this also caused another issue with tooltips seeming to have the effects of a lower z-index then tabs. However the z-index isn't different and I couldn't find a way using css to fix this(I think it has to deal with where the tooltip is inserted?).

image

This also doesn't fix #2406. For me it was working with 0.11.2 and 0.11.4 at least. I'm not sure exactly when it was fixed though.

@ThomasBurleson Can you please revert this change until a better fix can be found? I'm also not sure if one is even needed as it appears to be working fine in 0.11.4. But a demo from @kasiarachwal may show that we do indeed need one.

@kasiarachwal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps the difference is that I have a left hand sidenav (it's already off screen here - about half a page scrolled). With the code change, when I scroll, my tooltips are correctly positioned (although I have since modified the styling).
image

When I put it back the way it was before the change, and I scroll, the same tooltip is not where it should be. It is roughly the sidenav's width left of where it should be and the vertical position gets worse as you scroll farther down.
image

I have several config screens that are arranged this way, with a left sidenav and a vertical series of collapsing/expanding card forms. This change was the only thing that allowed the tooltips to be positioned in the correct place on the screen when scrolling since 11.0.

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Nov 20, 2015

Choose a reason for hiding this comment

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

@kasiarachwal looks like the fix for #5609 will fix all of my issues.

I'm not sure if yours will work anymore though as right now getNearestContentElement is always selecting the root element. I'm gussing that when the sidenav is open you just need to make sure it selects the root element instead of the md-content(or maybe md-contents parent?).

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Nov 20, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Nov 30, 2015

Choose a reason for hiding this comment

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

@kasiarachwal can you provide a plunkr for your setup?

The .toUpperCase() fix didn't work. I found out why and I need to make a pr to fix it for both our usecases but I'd like to be able t confirm it works with your setup.

As for the why it doesn't work. With/without the toUpperCase() to && current.nodeName !== 'MD-CONTENT' essentially always skips md-content and uses a higher(body or html tag?) tag for the root to attatch the tooltip too. This is what should happen with your specific case(I'm gussesing whenever a sidenav is open) but when you do not have a sidenav open it should use the md-content as the root like it did before.

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Dec 1, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 1, 2015

Choose a reason for hiding this comment

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

@kasiarachwal I tried to replicate your setup but I don't think I have it right as they are all broken in 1.0.0-rc1+ but working in 0.11.4(your change was added just after this) just as before. I couldn't find your case in 0.11.4 where it was broken.

Demo with right sidenav locked open and tooltips on the input and the toggle right sidenav button.
http://codepen.io/anon/pen/YwKXKp?editors=101
image

Here is the same demo using 0.11.4(version just before this change was added). It seems to function perfectly. Both tooltips work correctly when scrolled. Also I don't see the bug you mentioned with the tooltip being moved
http://codepen.io/anon/pen/zrOGYO?editors=101

Here is another copy of the last demo with just a sidenav on the left(to make sure the two sidenavs weren't canceling each other out by moving the tooltip back and forth).
http://codepen.io/anon/pen/rxBVNE?editors=101

Is your setup different then this? Can you update one of them if it is?

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Dec 1, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 2, 2015

Choose a reason for hiding this comment

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

Here is the last one with 1.0.0-rc5

http://codepen.io/anon/pen/QyLEJK?editors=101

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Dec 2, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 2, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 5, 2015

Choose a reason for hiding this comment

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

@kasiarachwal any chance I can get access somehow? Still waiting to fix this.

Several more people seem to have run into broken tooltips due to this too.

@ThomasBurleson do you want to roll this back for now? It'd close quite a few issues for many people.

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Dec 7, 2015 via email

Choose a reason for hiding this comment

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

@kasiarachwal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moot point as of 1.0.0-rc7 since the enclosing function is gone and parent is always body. Back to being broken for me.

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 9, 2015

Choose a reason for hiding this comment

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

Well 1.0.0-rc7 fixed my issue.

@kasiarachwal if you can get a demo of your issue I'd be more then happy to help you find a fix that works with both use cases.

@kasiarachwal
Copy link
Contributor Author

@kasiarachwal kasiarachwal commented on f62f693 Dec 9, 2015 via email

Choose a reason for hiding this comment

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

@epelc
Copy link
Contributor

@epelc epelc commented on f62f693 Dec 9, 2015

Choose a reason for hiding this comment

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

Well that's good to hear it's not an angular-material issue. I ran into a similar problem with d3 breaking svg icons a while back.

Please sign in to comment.