[Bug]: react-dialog breaking changes to type of children

CampbellOwen commented 3 months ago

Library

React Components / v9 (@fluentui/react-components)

System Info

System:
    OS: macOS 14.5
    CPU: (14) arm64 Apple M3 Max
    Memory: 93.72 MB / 36.00 GB
    Shell: 5.9 - /bin/zsh
  Browsers:
    Chrome: 126.0.6478.115
    Edge: 126.0.2592.68
    Safari: 17.5

Are you reporting Accessibility issue?

no

Reproduction

https://stackblitz.com/edit/zekrtd?file=src%2Fexample.tsx

Bug Description

Actual Behavior

When using a Dialog component, the new addition of react-motion means the child element must accept a ref. If you wrap all the usual Dialog children elements with React.Suspense (which does not accept a ref, there are errors rendering

Expected Behavior

This change in behaviour should either be a breaking change, or re-enable the previous behaviour of allowing children which do not accept a ref.

Logs

chunk-MDTLSHSV.js?v=9f4e6bb3:19411 
 Uncaught 
Error: @fluentui/react-motion: Invalid child element.
Motion factories require a single child element to be passed. That element element should support ref forwarding i.e. it should be either an intrinsic element (e.g. div) or a component that uses React.forwardRef().
    at getChildElement (@fluentui_react-comp…?v=9f4e6bb3:59258:9)
    at Presence (@fluentui_react-comp…v=9f4e6bb3:59364:19)
    at renderWithHooks (chunk-MDTLSHSV.js?v=9f4e6bb3:11546:26)
    at mountIndeterminateComponent (chunk-MDTLSHSV.js?v=9f4e6bb3:14924:21)
    at beginWork (chunk-MDTLSHSV.js?v=9f4e6bb3:15912:22)
    at beginWork$1 (chunk-MDTLSHSV.js?v=9f4e6bb3:19751:22)
    at performUnitOfWork (chunk-MDTLSHSV.js?v=9f4e6bb3:19196:20)
    at workLoopSync (chunk-MDTLSHSV.js?v=9f4e6bb3:19135:13)
    at renderRootSync (chunk-MDTLSHSV.js?v=9f4e6bb3:19114:15)
    at recoverFromConcurrentError (chunk-MDTLSHSV.js?v=9f4e6bb3:18734:28)
getChildElement @   @fluentui_react-comp…js?v=9f4e6bb3:59258
Presence    @   @fluentui_react-comp…js?v=9f4e6bb3:59364
renderWithHooks @   chunk-MDTLSHSV.js?v=9f4e6bb3:11546
mountIndeterminateComponent @   chunk-MDTLSHSV.js?v=9f4e6bb3:14924
beginWork   @   chunk-MDTLSHSV.js?v=9f4e6bb3:15912
beginWork$1 @   chunk-MDTLSHSV.js?v=9f4e6bb3:19751
performUnitOfWork   @   chunk-MDTLSHSV.js?v=9f4e6bb3:19196
workLoopSync    @   chunk-MDTLSHSV.js?v=9f4e6bb3:19135
renderRootSync  @   chunk-MDTLSHSV.js?v=9f4e6bb3:19114
recoverFromConcurrentError  @   chunk-MDTLSHSV.js?v=9f4e6bb3:18734
performConcurrentWorkOnRoot @   chunk-MDTLSHSV.js?v=9f4e6bb3:18682
workLoop    @   chunk-MDTLSHSV.js?v=9f4e6bb3:195
flushWork   @   chunk-MDTLSHSV.js?v=9f4e6bb3:174
performWorkUntilDeadline    @   chunk-MDTLSHSV.js?v=9f4e6bb3:382

Requested priority

High

Products/sites affected

Loop

Are you willing to submit a PR to fix?

no

Validations

[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

smhigley commented 3 months ago

This does work if Suspense is a child of DialogSurface instead of a wrapping control for both the trigger + content.

@layershifter since I heard you already may have looked at this -- is this a valid format for Dialog's children? We do have this console warning, but it only appears if the number of children is wrong, and not if the children themselves don't have a ref 😅:

console.warn(/* #__DE-INDENT__ */ `
        @fluentui/react-dialog [useDialog]:
        Dialog must contain at least one child <DialogSurface/>,
        and at most two children <DialogTrigger/> <DialogSurface/> (in this order).
      `);

CampbellOwen commented 3 months ago

I mentioned this to @smhigley offline, but the partner team where this is a "regression" does have exactly 2 children in the Dialog, but they're both React.Suspense so they haven't been seeing that warning.

ayblanchet commented 3 months ago

Interested in this fix ;)

miroslavstastny commented 3 months ago

As a workaround, wrap just the child of DialogSurface with the Suspense as smhigley suggested.

kresli commented 2 months ago

As a workaround, wrap just the child of DialogSurface with the Suspense as smhigley suggested.

The workaround doesn't really work as DialogTrigger is never shown.

microsoft / fluentui