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

Fix tooltip re-position after content resize & respect scroll bars #936

Merged

Conversation

chandlerprall
Copy link
Contributor

fast follow for #924

  • EuiTooltip did not clear its previous knowledge of the tooltip content's size so a window resize & re-hover of the anchor would cause the new tooltip to be influenced by the previous instance's positioning
  • small adjustment (1px) to tooltip's arrow position
  • Popover service did not take scroll bars into account when finding available window size
  • Popover service did not take buffer into account when calculating the content's fit value

@cchaos would you mind testing the visuals again to see if you can find any circumstances where tooltip breaks / is malformed?

@@ -371,7 +371,7 @@ describe('popover_positioning', () => {

// give the container limited space on both left and top, forcing to bottom-right
const container = document.createElement('div');
container.getBoundingClientRect = () => makeBB(100, 300, 768, 30);
container.getBoundingClientRect = () => makeBB(50, 300, 768, 30);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is in response to buffer now being used to compute fit

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

This is really awesome man! Nice job fixing this. I found one bug though. Horizontal centering looks fine if there are scrollbars visible, but if there aren't then it looks off-center (Chrome, OS X):

image

Also, I somehow managed to break things really badly by changing the size of the container element. Here's a screenshot showing the bottom tooltip, but the others are also fubared:

image

@cchaos
Copy link
Contributor

cchaos commented Jun 21, 2018

I am able to reproduce those issues that @cjcenizal found. Here are some more details.


The first one seems to have to only occur when the page reaches it's max-width. Here you can see without scrollbars it's still fine but the screen is smaller than the max-width.

screen shot 2018-06-21 at 09 57 42 am

And here you can see that once it's reached the max-width the positioning becomes slightly off.

screen shot 2018-06-21 at 10 09 23 am


The second one also seems to have to do with the max-width on the page. Here are two screenshots of giant width on the guide, one with the max-width and the other without.

screen shot 2018-06-21 at 10 15 19 am

screen shot 2018-06-21 at 10 15 40 am

@chandlerprall
Copy link
Contributor Author

@cchaos @cjcenizal tooltip creation now configured to avoid creating scrollbars while determining tooltip placement, this fixes the slightly-misaligned tooltip when no scrollbar is visible.

The second problem where the arrow and tooltip are separated is caused by the relationship between the anchor, tooltip, and page body. If you force the anchor to be rendered outside of the page body's bounds (by, saying, giving guide a width of 5000px while body maxes out around 1200px), then the anchor & tooltip position are correctly computed, but then the browser tries to render the tooltip (whose parent is body) outside of body's bounds, correctly determines the tooltip's width doesn't fit in the parent's bounds, and does its best by inserting a ton of line breaks. The "fix" for this is don't have content that flows outside of the body element. If we absolutely need to support this case there is a way but I strongly suspect we don't need to.

@snide
Copy link
Contributor

snide commented Jun 21, 2018

don't have content that flows outside of the body element

I can't think of a time when we'd need this. The body should be the visible page. At least that's how we've been using it. I think we're OK not worrying too much about this.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these!

@chandlerprall chandlerprall merged commit d9a2a1b into elastic:master Jun 22, 2018
@chandlerprall chandlerprall deleted the bug/tooltip-position-after-resize branch June 22, 2018 16:23
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.

4 participants