ButtonBase ownerdocument error

[joacub]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Current Behavior 😯

Steps to Reproduce 🕹

Link:

Context 🔦

Your Environment 🌎

Tech	Version
Material-UI	next
React	Last
Browser	All
TypeScript
etc.

Error getting ownerdocument when component is different to the default

[joshwooding]

Please fill out the issue template. We will need more information to look at this.

[joacub]

Here is the problem, in beta1 you quit the helper for get the owner document and right now if is null get the node get an error obiusly

componentDidMount() {
    prepareFocusVisible(this.getButtonNode().ownerDocument);
    if (this.props.action) {
      this.props.action({
        focusVisible: () => {
          this.setState({ focusVisible: true });
          this.getButtonNode().focus();
        },
      });
    }
  }

[HaNdTriX]

@joacub I believe you are using the component prop in your code. Please note that this must be able to hold refs.

- <ButtonBase component={(props) => <a {...props} />}>
+ <ButtonBase component={React.forwardRef((props, ref) => <a ref={ref} {...props} />)}>

Docs

• https://next.material-ui.com/guides/composition/#caveat-with-refs
• https://next.material-ui.com/api/button-base

[joacub]

@joacub I believe you are using the component prop in your code. Please note that this must be able to hold refs.

- <ButtonBase component={(props) => <a {...props} />}>
+ <ButtonBase component={React.forwardRef((props, ref) => <a ref={ref} {...props} />)}>

Docs

• https://next.material-ui.com/guides/composition/#caveat-with-refs
• https://next.material-ui.com/api/button-base

Yeah you are right I’m using the component prop, and is running right before this new version I catch

[ryancogswell]

@joacub It would be helpful to see the code of the component you are specifying via the component prop. I can reproduce the error you describe with the following code:

import React from "react";
import ReactDOM from "react-dom";

import Button from "@material-ui/core/Button";

const RenderNull = React.forwardRef(function RenderNull(props, ref) {
  return null;
});

function App() {
  return (
    <div className="App">
      <Button component={RenderNull}>Hello World!</Button>
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

but this is rather odd usage. It would be helpful to see what your exact scenario is so that the appropriate resolution can be determined.

[joacub]

@ryancogswell the issue is becouse the prop component not is with React.forwardRef and the component not have the ref , my fault becouse the new code change

[joacub]

@joacub It would be helpful to see the code of the component you are specifying via the component prop. I can reproduce the error you describe with the following code:

import React from "react";
import ReactDOM from "react-dom";

import Button from "@material-ui/core/Button";

const RenderNull = React.forwardRef(function RenderNull(props, ref) {
  return null;
});

function App() {
  return (
    <div className="App">
      <Button component={RenderNull}>Hello World!</Button>
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

but this is rather odd usage. It would be helpful to see what your exact scenario is so that the appropriate resolution can be determined.

yes, if the return is nulled or function or fragment get an error

[oliviertassinari]

Should we introduce a new warning to handle this edge case or ignore it as unlikely to affect many people?

@joacub Do you have a minimal reproduction code example to share?

[apanizo]

I am facing this issue without using component prop. The environment where I can reproduce the error is using JEST for generating snapshots of storybook stories. The project is just a create-react-app.

I am wondering if I am missing something because this should be a pretty common scenario, not an edge case.

If I rollback dependencies to alpha.6 the problem disappears, so that means this a ButtonBase's component problem.

console.error ../../node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [TypeError: Cannot read property 'ownerDocument' of null]
        at reportException (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
        at invokeEventListeners (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:209:9)
        at HTMLUnknownElementImpl._dispatch (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
        at HTMLUnknownElementImpl.dispatchEvent (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
        at HTMLUnknownElementImpl.dispatchEvent (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
        at HTMLUnknownElement.dispatchEvent (/Users/apanizo/git/iov/ponferrada/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
        at Object.invokeGuardedCallbackDev (/Users/apanizo/git/iov/ponferrada/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2427:16)
        at invokeGuardedCallback (/Users/apanizo/git/iov/ponferrada/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:2480:31)
        at commitRoot (/Users/apanizo/git/iov/ponferrada/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:11041:7)
        at /Users/apanizo/git/iov/ponferrada/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12492:5 TypeError: Cannot read property 'ownerDocument' of null
        at ButtonBase.componentDidMount (/Users/apanizo/git/iov/ponferrada/node_modules/@material-ui/core/ButtonBase/ButtonBase.js:230:54)
        at commitLifeCycles (/Users/apanizo/git/iov/ponferrada/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9432:22)
....

console.error ../../node_modules/react-test-renderer/cjs/react-test-renderer.development.js:9215
    The above error occurred in the <ButtonBase> component:
        in ButtonBase (created by ForwardRef(ButtonBase))
        in ForwardRef(ButtonBase) (created by WithStyles(ForwardRef(ButtonBase)))
        in WithStyles(ForwardRef(ButtonBase)) (created by ForwardRef(Button))
        in ForwardRef(Button) (created by WithStyles(ForwardRef(Button)))
        in WithStyles(ForwardRef(Button)) (at Button/index.stories.tsx:17)

And the script executed is: "test": "CI=true react-scripts test --env=jsdom"

What do you guys think?

[eps1lon]

Issue was likely introduced by #15484.

[joacub]

Should we introduce a new warning to handle this edge case or ignore it as unlikely to affect many people?

@joacub Do you have a minimal reproduction code example to share?

The null example is valid I have same code

[fzaninotto]

I'm experiencing this problem as well with a <MenuItem component={NavLink}>, which is a pretty common use case of using react-router in a material-ui menu. It used to work fine in v3.

[ryancogswell]

@eps1lon Do you think the following is what we want?

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
index 8486c972df6..44c96ba32db 100644
--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -112,7 +112,10 @@ class ButtonBase extends React.Component {
   }
 
   getButtonNode() {
-    return ReactDOM.findDOMNode(this.buttonRef.current);
+    if (this.buttonRef.current) {
+      return ReactDOM.findDOMNode(this.buttonRef.current);
+    }
+    return ReactDOM.findDOMNode(this);
   }
 
   onRippleRef = node => {

[oliviertassinari]

@ryancogswell We will soon move the ButtonBase from the classes to the hooks. We can't rely on ReactDOM.findDOMNode(this) with the hooks API. We would need reproduction examples to make sure we are going in the right direction. So far I have heard 2 different problems:

• @joacub and @fzaninotto that have a ref forwarding issue. React router doesn't do it correctly yet. Maybe in v6:
  https://github.com/mui-org/material-ui/blob/bb5b5b494096e3f379564a97706ebc3386c112e8/docs/src/pages/style/links/LinkRouter.js#L7-L9
• @apanizo has an issue in Jest.

[ryancogswell]

We will soon move the ButtonBase from the classes to the hooks.

@oliviertassinari Are you planning to still do this yet in v4? This seems like a pretty big breaking change to not yet have in the beta.

[eps1lon]

It seems like this issue is caused for two reasons:

1. The component passed via component does not forward its ref.
   We already warn against this. The general advise is to fix any warnings before filing an issue. Since this warning will now always lead to an error we should probably catch the error and rethrow it with a more meaningful message. The correct react-router integration is documented in https://next.material-ui.com/demos/buttons/#third-party-routing-library
2. unknown issue in jest (very likely also a ref forwarding issue)

[fzaninotto]

Using the documented react-router integration works, thanks. As it's a BC break compared to v3, I'd expect that information to appear in the upgrade guide - otherwise, it's really hard to find.

[eps1lon]

Using the documented react-router integration works, thanks. As it's a BC break compared to v3, I'd expect that information to appear in the upgrade guide - otherwise, it's really hard to find.

Do you have a reproduction? There should be a warning in your console linking to an upgrade path.

[fzaninotto]

The reproduction is <MenuItem component={NavLink}>, as I said earlier. I don't have the console log at hand (I've fixed the problem since then).

But from your question, I'm understanding that you consider that a warning in the console replaces a mention in the Upgrade guide, am I right? If so, let me express my preference for a mention in the upgrade guide, which lets me fix the problem before users encounter it in production.

[oliviertassinari]

@eps1lon What do you think of a note like this regarding the forwardRef https://next.material-ui.com/guides/migration-v3/#dialog?

[eps1lon]

The migration notes are just incomplete. It's missing the forwardRef requirement from the findDOMNode changes. It just has to link to https://next.material-ui.com/guides/composition/#caveat-with-refs which has extensive information about the change.

[bretep]

I just upgraded to the latest @next today and started to experience this issue. Before finding this github issue, I bisected the commits and have pinned the error down to this commit #15484

Before I upgraded to the latest @next I was using the following, which seems to be the suggested fix?

component={React.forwardRef((props, ref) => NavLink(props, ref))}

I tried changing it to:

component={React.forwardRef((props, ref) => <NavLink innerRef={ref} {...props} />)}

Same error.

Since it seems I'm already doing what is suggested above, I don't think this is the correct fix. Am I missing something?

[eps1lon]

@bretep Could you isolate the issue in a codesandbox? Without knowing where NavLink (implementation, package, version etc) I can't really help you.

[bretep]

Working on it now. Are you guys in any chat channel or just github issues?

[bretep]

@eps1lon
Here is the codesandbox: https://codesandbox.io/embed/y5xm1yv3j and works as expected. Thank you for your quick reply.

My browser was on the wrong route and did not hit the new code path. 😔

Summary
Changning the following fixed the problem:

From:

component={React.forwardRef((props, ref) => NavLink(props, ref))}

To:

component={React.forwardRef((props, ref) => <NavLink innerRef={ref} {...props} />)}

[eps1lon]

For anybody having this issue with react-test-renderer I suggest leveraging the createNodeMock option. A minimal version for this particular issues should match the following implementation:

// test utils
function createNodeMock() {
  return {
    nodeType: 1, // to trick findDOMNode
    // for focus-visible polyfill
    ownerDocument: {
      addEventListener: () => {},
      removeEventListener: () => {}
    },
  };
}

// test
it("can mount a button", () => {
  const renderer = TestRenderer.create(<Button>test</Button>, {
    createNodeMock
  });
  expect(renderer.toJSON()).toMatchSnapshot();
});

Though it is very likely that you encounter issues with other components as well since we still have a dependency on the DOM. Though I encourage everyone to use as little snapshot tests as possible I will work on a more extensive createNodeMock that covers more components since this seems like an important issue.

[medmunds]

To help others searching, this can also show up as "Error: Material-UI: expected an Element but found null" in ButtonBase.componentDidMount. The createNodeMock above solves this.

(Perhaps it would be helpful to add info about mocking to that error message when running in a test environment?)

For people using Storybook snapshots, createNodeMock is a Storyshots init option.

[eps1lon]

This issue will be fixed incidentally when converting ButtonBase to a function component. Preparation of the focus visible polyfill will happen in the ref phase. In that phase we have to guard against null anyway (ref cleanup) which means test environments without mocked nodes won't see an error.

I'm currently in the state where I think that any reading of ref.current in cDM only is a bug anyway so we might reduce the number of required createMockNodes anyway.

It's not ideal since it might hide an a11y issue but we it will already log 2 warnings so I guess this is ok.

(Perhaps it would be helpful to add info about mocking to that error message when running in a test environment?)

Sounds like a good idea to add this to https://next.material-ui.com/. Basically explaining that we have a dependency on the DOM and that you should mock this accordingly.