fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

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

Open CampbellOwen opened this issue 1 year ago • 3 comments

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.

CampbellOwen avatar Jun 25 '24 00:06 CampbellOwen

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).
      `);

smhigley avatar Jun 25 '24 00:06 smhigley

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.

CampbellOwen avatar Jun 25 '24 00:06 CampbellOwen

Interested in this fix ;)

ayblanchet avatar Jun 25 '24 13:06 ayblanchet

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

miroslavstastny avatar Jul 08 '24 14:07 miroslavstastny

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.

kresli avatar Jul 09 '24 14:07 kresli