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

[Tooltip] custom flip behavior causing placement issues due to margin #12084

Closed
2 tasks done
Sharakai opened this issue Jul 8, 2018 · 4 comments
Closed
2 tasks done
Assignees
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)

Comments

@Sharakai
Copy link
Contributor

Sharakai commented Jul 8, 2018

  • This is a v1.x issue (v0.x is no longer maintained).
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

When the Tooltip is provided with a custom flip behavior (via PopperProps, it should be positioned correctly, in the order specified, and with correct separation from the target to the tooltip.

Current Behavior

If the custom flip behavior causes the tooltip to go from right/left to top/bottom, then the tooltip pops in and out at different places as it is opening/closing, and it does not position correctly causing it to overlap the target.

This is because there are multiple updates occurring within react-popper (at least one as a result of the current material-ui implementation), and the current Tooltip styles apply only a left-right margin (if placement is "left" or "right") or a top-bottom margin (if placement is "top" or "bottom").

  1. On the very initial calculation, the "placement" is to the right (as that is where I have defined it to start at with the placement prop).
  2. MUI applies a 24px/14px margin to the left and right of the tooltip (https://github.com/mui-org/material-ui/blob/v1.3.1/packages/material-ui/src/Tooltip/Tooltip.js#L65).
  3. popper.js determines that the tooltip doesn't fit to the right, but DOES fit at the top.
  4. MUI applies a 24px/14px margin to the top and bottom of the tooltip (https://github.com/mui-org/material-ui/blob/v1.3.1/packages/material-ui/src/Tooltip/Tooltip.js#L72), removing the left-right margin
  5. popper.js determines that the tooltip doesn't fit at the right or top (because of the additional margin), but does to the left.
  6. Repeat from step 2 for as many renders / updates there are.

Steps to Reproduce (for bugs)

You can see this in the following codesandbox, by hovering over the red "Tile" and watching the tooltip "pop" in at the top before settling at the top-left,
https://codesandbox.io/s/l464lv7wv9?module=%2Fsrc%2Fpages%2Findex.js
(See src/pages/index.js for the implementation)

If you uncomment the margin: 14px !important on line 17, you'll see that the issue is fixed as the tooltip will now have a constant size regardless of placement. This will now mean, however, that the tooltip cannot push up against the edge of the viewport, and will be (at a minimum) 14px from the edge (but perhaps this can be countered by a negative position, e.g. top -14px?).

Context

We have a tile that can be dragged around the screen, with a large tooltip that provides helpful information on hover. When the user drags the tile to the top right, we get this issue.

Your Environment

Tech Version
Material-UI v1.3.1 (although originally found on v1.2.0)
React v16.4.1
browser Chrome 67
etc
@Sharakai Sharakai changed the title [Tooltip] [Tooltip] custom flip behavior causing issues with placement due to margin Jul 8, 2018
@Sharakai Sharakai changed the title [Tooltip] custom flip behavior causing issues with placement due to margin [Tooltip] custom flip behavior causing placement issues due to margin Jul 8, 2018
@oliviertassinari oliviertassinari added the component: tooltip This is the name of the generic UI component, not the React module! label Jul 8, 2018
@oliviertassinari oliviertassinari self-assigned this Jul 8, 2018
@Sharakai
Copy link
Contributor Author

Sharakai commented Jul 8, 2018

For what it's worth, this issue is slightly exacerbated by the use of an inline arrow-function in MUI's Tooltip render function (https://github.com/mui-org/material-ui/blob/v1.3.1/packages/material-ui/src/Tooltip/Tooltip.js#L348).

react-popper schedules an extra update on every Tooltip render, as it sees nextProps.children !== this.props.children (https://github.com/FezVrasta/react-popper/blob/v0.10.4/src/Popper.js#L64)

For every update on popper.js, it seems to re-run the Tooltip's render function, thus re-applying MUI's margin styling resulting in the placement changing again.

@oliviertassinari
Copy link
Member

@Sharakai I can't find of any better workaround that applying the same margin to the tooltip, no matter the placement. Unfortunately, we can't move this logic into the core as it's breaking the -start and -end placement. Are you happy with this story? Can we close the issue?

On a related topic, we might have the same issue with the auto placement. But it's not something we officially support. At least yet.

@oliviertassinari oliviertassinari self-assigned this Jul 10, 2018
@Sharakai
Copy link
Contributor Author

@oliviertassinari I presume, then, that any consumer defining a custom flip.behavior akin to ['right', 'top', 'left', 'bottom'] will need to override the tooltipPlacementX styles in order to apply a standard margin?
This would, of course, be at expense of it not working with -start and -end placements, nor with it aligning correctly along the edge of the viewport.

Happy to close if this is considered un-fixable at present. Thanks for looking at it nevertheless 👍
I'll keep investigating when I get time and see if there is a novel solution to it.

@oliviertassinari oliviertassinari added the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Jul 10, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 10, 2018

@Sharakai Yes, I think that it's structural. I can't see how we could make both works at the same time. We have to prioritize, start/end feature wins.

Be aware that we have reworked the Tooltip implementation. Your following code sample will no longer work as expected:

  <Tooltip
    title="My Tooltip"
    placement="right"
    classes={{ tooltip: props.classes.tooltip }}
    PopperProps={{
      modifiers: {
        flip: {
          behavior: ["right", "top", "left", "bottom"]
        }
      }
    }}
  >

I'm working on fixing this breaking change. Hold on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)
Projects
None yet
Development

No branches or pull requests

2 participants