fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: Using a non-html element in `as` props of ThemeProvider cause useFocusRect to crash

Open weichensw opened this issue 2 years ago • 2 comments

Library

React / v8 (@fluentui/react)

System Info

npx: installed 1 in 2.252s

  System:
    OS: Windows 10 10.0.22621
    CPU: (20) x64 Intel(R) Core(TM) i9-10900X CPU @ 3.70GHz
    Memory: 23.51 GB / 31.72 GB
  Browsers:
    Edge: Spartan (44.22621.1702.0), Chromium (113.0.1774.57)
    Internet Explorer: 11.0.22621.1

Are you reporting Accessibility issue?

no

Reproduction

https://codepen.io/xbtsw/pen/bGmJeod

Bug Description

Actual Behavior

useFocusRect will crash because it attempt to call addEventListener on a non-HTML element.

Expected Behavior

The focus rect should be on the window object, as per design.

A screenshot to assist with understanding (this is not from the minimal repro as that stack is minified) image

Logs

No response

Requested priority

Blocking

Products/sites affected

No response

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.

weichensw avatar May 26 '23 22:05 weichensw

Yeah, i believe this is intentional. The as prop is typically used to change the DOM element rendered at the root (span or section etc)

It was never expected to take any functional component. I'll update the prop description.

micahgodbolt avatar May 30 '23 18:05 micahgodbolt

Hi @micahgodbolt, thanks for the investigation. I don't think we should redefine the prop here because:

  • The type is acceptable of a non-html element (React.ElementType instead of HTMLElement)
  • The description is allowing passing of non-html element
  • This was working as expected for the past 1.5 years (at least until 8.77.0, further version we haven't verified).
  • Redefining this prop as such requires everyone to move their ThemeProvider (if they can) or introduce an originally unnecessary HTML element on the top level of their app, which is a very disruptive change for complex apps.

So I think this is a breaking change instead of a doc fix.

weichensw avatar May 31 '23 04:05 weichensw

Not sure why bot closed it

weichensw avatar Jun 03 '23 07:06 weichensw

I dug in and it looks like it broke at 8.84.0, while it works in 8.83.1

https://codepen.io/micahgodbolt/pen/ZEmQGNO?editors=1011

This is the main change between those two versions. So i'm guessing it has to do with this.

  • fix: Scope focus visible classnames and event listeners to ThemeProvider by default, and to the window body as a fallback. (PR #24025 by [email protected])

micahgodbolt avatar Jun 14 '23 15:06 micahgodbolt

:tada:This issue was addressed in #28342, which has now been successfully released as @fluentui/[email protected].:tada:

Handy links:

msft-fluent-ui-bot avatar Jun 28 '23 07:06 msft-fluent-ui-bot