[core] fix(Popover): work around react-popper bug, fix React 18 compat #6458
Conversation
[core] fix(Popover): work around react-popper bug, React 18 compatBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| // if we're provided a ref to the child already, we don't need to attach one ourselves | ||
| if (this.props.targetRef !== undefined) { | ||
| return onlyChild; | ||
| } |
There was a problem hiding this comment.
it turns out that this change to accept both RefObject and RefCallback broke all Popovers with children that are not DOM elements, such as <Suggest> which renders an <InputGroup> child. here's what happens:
- ResizeSensor gets a ref callback as its
targetRef - ResizeSensor is not able to peek into this ref callback (like it could with
.currenton ref objects) to get the DOM element, so it must create its own ref - ResizeSensor's own ref can only attach to its direct child, which may not be a native DOM element
- ResizeSensor returns this non-DOM element value to
Popover#targetRefat runtime, thereby settingPopover#targetElementto an invalid type (e.g.InputGroupinside aSuggest, instead ofHTMLInputElement) - the page crashes because:
i.Popovercode which expectsthis.targetElementto be a DOM node fails (e.g.element.closest())
ii. popper.js fails because it doesn't have access to a valid DOM node
thus, targetRef can only accept a RefObject type, which must be provided if the child is a non-native DOM element node
Fix tests, add commentsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix tests, only create ResizeObserver on mountBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| @@ -89,6 +90,7 @@ export class ResizeSensor extends AbstractPureComponent<ResizeSensorProps> { | |||
| } | |||
|
|
|||
| public componentDidMount() { | |||
| this.observer = new ResizeObserver(entries => this.props.onResize?.(entries)); | |||
There was a problem hiding this comment.
this caused a runtime error in some test environments like jsdom where ResizeObserver is undefined. apparently components still get mounted in jsdom, whoops. I will fix the regression ASAP
Fixes #6203
Checklist
Changes proposed in this pull request:
fix(
Popover): adjust target ref behavior to work around react-popperinnerRefbug, fixing React 18 strict mode compatibilityReviewers should focus on:
No regressions in Popover, table draggable interactions, ResizeSensor
Screenshot