Fluentui: Double popup on <TooltipHost>

Created on 1 Nov 2018  路  13Comments  路  Source: microsoft/fluentui

Environment Information

  • __Package version(s)__: 6.92.0
  • __Browser and OS versions__: Windows

Please provide a reproduction of the bug in a codepen:

https://codepen.io/anon/pen/oaapVo



Actual behavior:

Move to "Some Rather pretty Long name" and see a pop up, move to the "Pretty..." above to see another popup.

Expected behavior:

There is only 1 pop up.

Priorities and help requested:

Are you willing to submit a PR to fix? (Yes, No) No

Requested priority: (Blocking, High, Normal, Low) Normal

Products/sites affected: (if applicable) PowerApps

Tooltip Type

Most helpful comment

I think a better design is to have TooltipHosts be aware of its children (or that the children TooltipHosts are aware of its parent) whether or not a tooltip is currently shown in that subtree.

One way of tackling this is with context. We can have a state that is contextual for TooltipHosts only that would prevent showing double popups.

All 13 comments

This seems to have something to do with this code:

https://github.com/OfficeDev/office-ui-fabric-react/blob/a19b4559d8c8daf16f80e31d95d3eb7c82a28b4d/packages/office-ui-fabric-react/src/components/Tooltip/TooltipHost.base.tsx#L134

By examining the React virtual dom, I can see that any descendent text section within a <TooltipHost> are converted to a <TooltipHostBase>, so each descendent text section will go through that code. Interestingly, the descendent text section has "overflowMode" turned on even though no client has ever requested a overflowMode to be applied to that text section.

I found this issue when investigating a weird bug which reproduces on IE/Edge only. Even without any overflow, sometimes the clientWidth is 1px smaller than scrollWidth(which seems to be a IE quirk). So this code hasOverflow will return true. When overflowMode is turned on, the pertinent text section will then have another <Popup> show up.

On the codepen website, I cannot reproduce a case that under IE/Edge the wrapper element is 1px smaller in clientWidth than scrollWidth. So I just set a max-width to reproduce the bug in another way. But the idea is similar: we shouldn't rely on the overflowMode logic above to determine whether any descendent text section should have a pop up shown. The requirement is clear: user only want to show a pop up where a <TooltipHost> is explicitly specified.

Pretty easy to see the repro with the provided CodePen. I don't see anything obviously wrong with the CodePen example itself, so I agree this appears to be a TooltipHost issue. Thanks for the CodePen and thorough analysis!

Hey @changsi-an! Have you tried using the onRenderPrimaryText prop? The logic of the Persona component includes a tooltip for any text that overflows here:

https://github.com/OfficeDev/office-ui-fabric-react/blob/af3ca4d877de3b225819ba0607c9bf0f8f1a663a/packages/office-ui-fabric-react/src/components/Persona/Persona.base.tsx#L146-L159

If you want to override that, you can write a custom render method for your primary text, like in this CodePen.

@mtennoe @jakob101 is including the Tooltip for all text that overflows part of the design of Persona?

@natalieethell - I would personally expect that at least, as all text fields in the Persona control can overflow, and having tooltips for only parts of them would be inconsistent (and potentially frustrating for the user)

@mtennoe makes sense. @changsi-an given what Marius said, it seems that the tooltip on overflow is by design and I would recommend using onRenderPrimaryText, onRenderSecondaryText, onRenderTertiaryText, and/or onRenderOptionalText to custom render your text without a tooltip. I'll close this but feel free to re-open it if you have any other concerns.

Thanks for the feedback. But I don't think this is ideal: it does not provide a way to always show the tooltip. It puts unnecessary burden on the consumer side to consider how to implement onRenderPrimaryText

@mtennoe @natalieethell

@mtennoe what do you think, should we leave this as is?

It sounds like one potential option would be to add another prop to control the tooltip. The default behavior would remain the same, but you could specify whether you want tooltips in your Persona.

Agree that overriding the inRenderText functions is fiddly and cumbersome, so we can definitely find a better solution for component consumers. It does introduce some complexity in the interface to the component, with need for backwards compatibility in the future etc, but this is something that can be added fairly easily with an optional boolen property (or similar), and I think will be worth it for component consumers

It opens up the question though: What about other components that have tooltips? Do we want to make it possible to disable tooltips in all components, or only a select few? I guess we can start with this one only to avoid overengineering

Thanks for looking into this again.

I think a Boolean/Enum flag might be a good idea. If <ToolTipHost> is allowed to have a overflowMode, it might equally justify that the <Persona> can have a tooltipMode as well.

I think a better design is to have TooltipHosts be aware of its children (or that the children TooltipHosts are aware of its parent) whether or not a tooltip is currently shown in that subtree.

One way of tackling this is with context. We can have a state that is contextual for TooltipHosts only that would prevent showing double popups.

@changsi-an of course, thanks for bringing this to our attention.

@kenotron that sounds like a good approach, and handles all other components that have tooltips rendered within them, as you mentioned, @mtennoe. I'll look into this.

@kenotron - Really like that idea as well!

:tada:This issue was addressed in #7105, which has now been successfully released as [email protected].:tada:

Handy Links:

Was this page helpful?
0 / 5 - 0 ratings

Related issues

just-joshing picture just-joshing  路  35Comments

lukasbash picture lukasbash  路  31Comments

just-joshing picture just-joshing  路  35Comments

Nimelrian picture Nimelrian  路  38Comments

ryancole picture ryancole  路  39Comments