microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.46k stars 2.72k forks source link

[Bug]: Error - Dialog should have at least one focusable element inside DialogSurface #28209

Open rosergeev opened 1 year ago

rosergeev commented 1 year ago

Library

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

System Info

System:
    OS: Windows 10 10.0.22621
    CPU: (4) x64 Intel(R) Xeon(R) CPU E5-2680 v4 @ 2.40GHz
    Memory: 3.67 GB / 16.00 GB
  Browsers:
    Edge: Spartan (44.22621.1555.0), Chromium (112.0.1722.58)
    Internet Explorer: 11.0.22621.1

Are you reporting Accessibility issue?

None

Reproduction

https://codesandbox.io/s/sad-resonance-tlv9rv?file=/src/App.tsx

Bug Description

Actual Behavior

I copied code for the Dialog component from react.fluentui.dev from the stories section, particular for "Controlling Open And Close". And changed the initial state of the dialog to true: const [open, setOpen] = React.useState(true);

In console I can see warnings stating the following: "@fluentui/react-dialog: a Dialog should have at least one focusable element inside DialogSurface. Please add at least a close button either on DialogTitle action slot or inside DialogActions in Dialog (created by App) in App "

Actually I see this behavior in my tests while rendering this component while I use control for open/close dialog. And I like to have test run to be clear, without any warnings.

Expected Behavior

As I have some buttons inside DialogActions section I shouldn't see this warning

Logs

No response

Requested priority

Normal

Products/sites affected

ImageMaster OfficeClient Add-In

Are you willing to submit a PR to fix?

no

Validations

rosergeev commented 1 year ago

Any news about the issue?

Cam-Bloom commented 1 year ago

I am also having same issue testing with react testing library and jest, my component work correctly without any warnings within browser however when running tests I get warning which creates noise when running my tests

image

bsunderhus commented 1 year ago

This seems to be solved in the latest release versions https://codesandbox.io/s/sad-resonance-tlv9rv?file=/src/App.tsx

Can you confirm @rosergeev ?

rosergeev commented 1 year ago

Good afternoon, @bsunderhus! It seems that in my reproduction example it's not reproduced anymore, but in my production code it's still there. So I've changed reproduction example to be close to my code. https://codesandbox.io/s/late-glade-qrn674?file=/src/Confirmation.test.tsx But I'm not familiar with test in codesandbox, so my test is not working there, but actual result is the following: image

bsunderhus commented 1 year ago

ok, thanks for the feedback @rosergeev, I'll try to reproduce it locally!

rosergeev commented 1 year ago

Thank you

bsunderhus commented 1 year ago

Ok, I was able to reproduce, it seems to be working fine in production environment also, but in jest environment this fails. Looks like @fluentui/react-tabster useFocusFinders().findFirstFocusable returns null while it should return the first focusable element.

I'll follow up with a PR to add proper tests

cc: @ling1726 @mshoho

rosergeev commented 1 year ago

Thank you for your attention

15MariamS commented 1 year ago

👋 Having the same issue with the warning thrown in my tests (in prod, everything looks fine)

layershifter commented 1 year ago

@bsunderhus can we temporary add a condition to not emit warnings in test env?

bsunderhus commented 1 year ago

@bsunderhus can we temporary add a condition to not emit warnings in test env?

Yes we can @layershifter. I'll see to open a PR for it.

I'll keep this issue open nonetheless as this doesn't solve the problem of tabster failing in JSDOM environment.

sjwilczynski commented 1 year ago

Related issue in Tabster itself. As a side note, we would love to have Tabster working in JSDOM but for know to have coverage of focus related functionality, we rely on Storybook + play function

microsoft-github-policy-service[bot] commented 3 months ago

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes. Still require assistance? Please add comment - "keep open".