mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.31k stars 32.12k forks source link

[transitions] "Cannot read property `scrollTop` of null" #27154

Open siriwatknp opened 3 years ago

siriwatknp commented 3 years ago

Current Behavior ๐Ÿ˜ฏ

If the child of components does not forwardRef to DOM, it cause this error.

Screen Shot 2564-07-07 at 09 24 14

Expected Behavior ๐Ÿค”

The error should be more informative about how to fix it.

Steps to Reproduce ๐Ÿ•น

Steps:

  1. open this sandbox https://codesandbox.io/s/transition-v5-error-dide8?file=/src/App.tsx
  2. change material-ui/core to v5.beta
  3. will see the error

Context ๐Ÿ”ฆ

Is it better to display warning (in console) instead of Error?

Here is what I think about the dev experience.

then, 3 possibilities

Actions

Your Environment ๐ŸŒŽ

`npx @material-ui/envinfo` ``` Don't forget to mention which browser you used. Output from `npx @material-ui/envinfo` goes here. ```
thovden commented 3 years ago

Thanks - this saved me some debugging. I had this construct:

<Zoom in timeout={200}>
  <React.Fragment>

Fixed by adding the actual components as the only child, e.g.,

<Zoom in timeout={200}>
  <Fab>
oliviertassinari commented 3 years ago

I'm linking #17119 too for context when a similar symptom impacts the tests (but with a different root cause).

With v5, the first two warnings in the console are about how to fix the issue https://codesandbox.io/s/transition-v5-error-forked-x66jg

Capture dโ€™eฬcran 2021-07-14 aฬ€ 18 31 14

and then, about the exception that is thrown. The problem is very close to #18119 and #17480.

codingedgar commented 2 years ago

I'm checking the example at https://mui.com/pt/guides/migration-v4/#cannot-read-property-scrolltop-of-null that linked here, but I get this error while using styled and wrapping it in a forwardRef (like this https://github.com/DefinitelyTyped/DefinitelyTyped/issues/28884#issuecomment-983243411) does not solve it.

I have a while trying to find and tinkering how to do this right but honestly haven't found anything, do you have an example of how to use Fade with styled component as a child?

siriwatknp commented 2 years ago

I have a while trying to find and tinkering how to do this right but honestly haven't found anything, do you have an example of how to use Fade with styled component as a child?

Please provide a CodeSandbox that describes your issue.

codingedgar commented 2 years ago

https://codesandbox.io/s/simplefade-material-demo-forked-9gm3tp?file=/demo.tsx:0-2357

just in case here is the code:

import React from "react";
import {
  InputAdornment,
  Input,
  InputProps,
  IconButton,
  styled
} from "@mui/material";
import { Visibility, VisibilityOff } from "@mui/icons-material";

export type PasswordInputProps = InputProps & {
  showPassword: boolean;
  onToggleShowPassword: (
    event: React.MouseEvent<HTMLButtonElement, MouseEvent>
  ) => void;
};

export const PasswordInput = styled(
  ({ showPassword, onToggleShowPassword, ...props }: PasswordInputProps) => (
    <Input
      placeholder="Link Password"
      type={showPassword ? "test" : "password"}
      endAdornment={
        // eslint-disable-next-line react/jsx-wrap-multilines
        <InputAdornment position="end">
          <IconButton
            aria-label="toggle password visibility"
            edge="end"
            onMouseDown={(e) => {
              e.preventDefault();
            }}
            onClick={onToggleShowPassword}
          >
            {showPassword ? (
              <VisibilityOff fontSize="small" />
            ) : (
              <Visibility fontSize="small" />
            )}
          </IconButton>
        </InputAdornment>
      }
      {...props}
    />
  )
)({
  backgroundColor: "var(--color-3)",
  color: "var(--color-1)",
  fontSize: 12,
  borderRadius: "5px",
  paddingLeft: "10px",
  alignItems: "center",
  "&:focus": {
    backgroundColor: "var(--color-3)"
  },
  width: "250px",
  height: "30px",
  "&:after": {
    borderColor: "var(--color-3)"
  },
  "&.MuiInput-underline:hover:not(.Mui-disabled):before": {
    borderBottom: "none"
  },
  "&.MuiInput-underline:before": {
    borderBottom: "none"
  },
  "& .MuiInput-input": {
    "&[type=password]:not(:placeholder-shown)": {
      fontSize: 17,
      letterSpacing: -2,
      fontFamily: "monospace",
      padding: 0
    }
  },
  "& .MuiInputAdornment-root": {
    color: "var(--color-1)",
    margin: 0,
    padding: "0 10px",
    backgroundColor: "var(--color-3)",
    "& button": {
      color: "var(--color-1)"
    }
  }
});

// from: https://github.com/DefinitelyTyped/DefinitelyTyped/issues/28884#issuecomment-983243411
// the only diff is that i do not use <Omit> as forwardRef already does that
export const PasswordInputRef = React.forwardRef<
  typeof Input,
  PasswordInputProps
>((props, ref) => <PasswordInput ref={ref} {...props} />);
import * as React from "react";
import Box from "@mui/material/Box";
import Switch from "@mui/material/Switch";
import Fade from "@mui/material/Fade";
import FormControlLabel from "@mui/material/FormControlLabel";
import { PasswordInputRef, PasswordInput } from "./PasswordInput";
import Input from "@mui/material/Input";

export default function SimpleFade() {
  const [checked0, setChecked0] = React.useState(false);
  const [checked1, setChecked1] = React.useState(false);
  const [checked2, setChecked2] = React.useState(false);
  const [checked3, setChecked3] = React.useState(false);
  const [showPassword, toggleShowPassword] = React.useState(false);

  const handleChange0 = () => {
    setChecked0((prev) => !prev);
  };

  const handleChange1 = () => {
    setChecked1((prev) => !prev);
  };

  const handleChange2 = () => {
    setChecked2((prev) => !prev);
  };

  const handleChange3 = () => {
    setChecked3((prev) => !prev);
  };

  return (
    <Box sx={{ height: 180 }}>
      <FormControlLabel
        control={<Switch checked={checked0} onChange={handleChange0} />}
        label="Show0"
      />
      <FormControlLabel
        control={<Switch checked={checked1} onChange={handleChange1} />}
        label="Show1"
      />
      <FormControlLabel
        control={<Switch checked={checked2} onChange={handleChange2} />}
        label="Show2"
      />
      <FormControlLabel
        control={<Switch checked={checked3} onChange={handleChange3} />}
        label="Show3"
      />
      <Box sx={{ display: "flex" }}>
        <Fade in={checked0}>
          <Input />
        </Fade>
        <Fade in={checked1}>
          <PasswordInputRef
            showPassword={showPassword}
            onToggleShowPassword={() => {
              toggleShowPassword((x) => !x);
            }}
          />
        </Fade>
        <Fade in={checked2}>
          <PasswordInput
            showPassword={showPassword}
            onToggleShowPassword={() => {
              toggleShowPassword((x) => !x);
            }}
          />
        </Fade>
        <Fade in={checked3}>
          <div>
            <PasswordInput
              showPassword={showPassword}
              onToggleShowPassword={() => {
                toggleShowPassword((x) => !x);
              }}
            />
          </div>
        </Fade>
      </Box>
    </Box>
  );
}

which returns

TypeError
Cannot read properties of null (reading 'scrollTop')

for check1 and check2, my workaround has been check3 but I wish to know why or how styled does not forward the ref to Input, or if I'm seeing this wrong? as check0 which is the Input without styled wrap, does work fine.

siriwatknp commented 2 years ago

@codingedgar I think the problem is in the PasswordInput. You have to forward the ref to the Input.

export const PasswordInput = styled(
  React.forwardRef(({ showPassword, onToggleShowPassword, ...props }: PasswordInputProps, ref) => (
    <Input
      ref={ref}
      placeholder="Link Password"
      type={showPassword ? "test" : "password"}
      endAdornment={
        // eslint-disable-next-line react/jsx-wrap-multilines
        <InputAdornment position="end">
          <IconButton
            aria-label="toggle password visibility"
            edge="end"
            onMouseDown={(e) => {
              e.preventDefault();
            }}
            onClick={onToggleShowPassword}
          >
            {showPassword ? (
              <VisibilityOff fontSize="small" />
            ) : (
              <Visibility fontSize="small" />
            )}
          </IconButton>
        </InputAdornment>
      }
      {...props}
    />
  ))
)
codingedgar commented 2 years ago

@codingedgar I think the problem is in the PasswordInput. You have to forward the ref to the Input.

export const PasswordInput = styled(
  React.forwardRef(({ showPassword, onToggleShowPassword, ...props }: PasswordInputProps, ref) => (
    <Input
      ref={ref}
      placeholder="Link Password"
      type={showPassword ? "test" : "password"}
      endAdornment={
        // eslint-disable-next-line react/jsx-wrap-multilines
        <InputAdornment position="end">
          <IconButton
            aria-label="toggle password visibility"
            edge="end"
            onMouseDown={(e) => {
              e.preventDefault();
            }}
            onClick={onToggleShowPassword}
          >
            {showPassword ? (
              <VisibilityOff fontSize="small" />
            ) : (
              <Visibility fontSize="small" />
            )}
          </IconButton>
        </InputAdornment>
      }
      {...props}
    />
  ))
)

Thank you so much! It does work, was unsure on how to use styled and forwardRef together.

totszwai commented 2 years ago

Also facing the same problem... so are you saying that if the child is a styled-component and does not have a forwardRef it would not work...? Shouldn't the Fade (or all transitions) work with or without a ref?

We have many components that are just plain styled-components without a ref (because there were simply no need to use a ref) but now we have to add it everywhere just to get transition to work??

Perhaps all transitions should wraps the child with a plain div instead?

The workaround I had to do, was to manually wrap the children with another div... ๐Ÿ‘Ž

      <Fade in={visible} timeout={1000}>
        <div>{children}</div>
      </Fade>
chuckwired commented 2 years ago

Thanks @totszwai ! I can confirm I'm experiencing the same on <Fade> upgrading my codebase from v4 to v5, and wrapping with a div circumvents the issue.

katherinedavenia commented 2 years ago

Thanks @totszwai . I faced the same problem and your solution works. I need to wrap the children with a div tag.

relativeflux commented 2 years ago

@totszwai Can confirm this solution of wrapping in a div works. Can you please look into this MUI team? We shouldn't have to resort to this workaround.

sin-to-jin commented 2 years ago

It might be to differ a behavior, but I tried to take to use <Collapse> instead of <Fade>, then it had no error.

// TypeError: Cannot read properties of null (reading 'scrollTop').
import { Fade } from '@mui/material';
const Component = () => {
  const [checked, setChecked] = React.useState(false);
  return (
    <>
      <button onClick={() => setChecked(!checked)}>test</button>
      <Fade in={checked}>
        <div>hoge</div>
      </Fade>
    </>
  );
}
// no errors
import { Collapse } from '@mui/material';
const Component = () => {
  const [checked, setChecked] = React.useState(false);
  return (
    <>
      <button onClick={() => setChecked(!checked)}>test</button>
      <Collapse in={checked}>
        <div>hoge</div>
      </Collapse>
    </>
  );
}
bendras commented 1 year ago

Observed similar issue in Tabs /> component. SEE: https://github.com/mui/material-ui/issues/17119#issuecomment-1282478695

allicanseenow commented 1 year ago

This issue is still existing and was not available with MUI 4. This can cause the a project with hundreds of components to crash after the upgrade to MUI 5 and it's difficult to fix because the stack trace or the logged error message is not helpful at all as it doesn't mention where or which MUI component is causing the issue.

dberardo-com commented 1 year ago

yeah so ... how to use transitions on mounting / rerendering components with MUI5 then ? any solution? any workaround ? or is this only affecting Fade ?

shughes-uk commented 7 months ago

Had the same issue, for me I was using forwardRef wrong with a custom Card

Here's what I was doing wrong (pseduocode)

// INCORRECT
forwardRef(({ ref, ...props }) => <Card ref={ref} ...props>blah</Card>
// CORRECT
forwardRef((props, ref) => <Card ref={ref} ...props>blah</Card>
1mehdifaraji commented 2 months ago

I had the same error when I tried to use the <Fade> component from material-ui and found a workaround like this:

<Fade in timeout={1000}> 
   <div>
       <Component />
   </div>
</Fade>

Had to use a div element wrapped around my component and the error was gone.

chiaberry commented 3 weeks ago

I am getting this error, but not on a custom component but rather

TypeError: Cannot set properties of null (setting 'scrollTop')
    at GridVirtualScrollbar.js:77:1
    at HTMLDivElement.<anonymous> (useEventCallback.js:7:1)

It seems to happen when I've scrolled to the middle of my table, but then do some filtering (not with data grid's filters, but server side with our own components), and if the results are less than a full view of the table, the error shows up)

oliviertassinari commented 3 weeks ago

@chiaberry Your issue is with the data grid, not the transition. If you have a minimal reproduction, please open an issue at https://github.com/mui/mui-x.

ddouangkesone commented 2 weeks ago

I had the same error when I tried to use the <Fade> component from material-ui and found a workaround like this:

<Fade in timeout={1000}> 
   <div>
       <Component />
   </div>
</Fade>

Had to use a div element wrapped around my component and the error was gone.

Wooow this fixed my issue! A little silly that wrapping it in a div fixes the issue.