search

patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
797 stars 356 forks source link

Bug - Tooltip - removeFindDomNode causes validateDomNesting warning #8481

Closed starpit closed 1 year ago

starpit commented 1 year ago

Describe the problem When removeFindDomNode is enabled on <Tooltip>, a validateDomNesting warning can result. If the tooltipped element is nested in a <p>, then Tooltip's use of <div> as the Popper-wrapper causes this warning.

This only occurs when removeFindDomNode is supplied, due to https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/helpers/Popper/Popper.tsx#L411

Since this comes from the generic Popper helper, this probably affects other components, beyond Tooltip.

How do you reproduce the problem? Wrap a Tooltip'd element in a <p>, and provide removeFindDomNode to the <Tooltip>.

Expected behavior The Popper wrapper should work in any nesting.

Is this issue blocking you? The workaround is to avoid React StrictMode.

Screenshots n/a

What is your environment?

What is your product and what release date are you targeting? Kui

Any other information?

kmcfaul commented 1 year ago

Will be addressed by this issue - https://github.com/patternfly/patternfly-react/issues/8666

nicolethoen commented 1 year ago

reference could be renamed to triggerRef. popperRef is not exposed to other components that use Popper.

If we want users to utilize the alternate strict mode warning resolution (passing references and avoiding the wrapping div), then we should make sure components that use Popper have these props available and support them (as the trigger would have be rendered on the component side rather than passed to Popper).

kmcfaul commented 1 year ago

@mcoker Would a span cause less issues overall as far as DOM nesting? The wrapping element is only there to contain the Popper references it requires.

I will still be updating Popper consuming components to allow them to utilize the other strict-mode enabling method which requires refs.

starpit commented 1 year ago

span should be fine. i think the only restriction is <p> under <p>

mcoker commented 1 year ago

Sorry I could be off base, but does this wrap the popper contents? If so, it's worth noting only phrasing content can be placed inside of a <span> - would adding a list or div or some non-phrasing element potentially throw a validateDomNesting warning in popper's <span>, too?

kmcfaul commented 1 year ago

The wrapping elements wrap the trigger and the popper. We could leave the popper wrapper as div and only update the trigger to span maybe.