Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ButtonBase] Fix when changing enableRipple prop from false to true #19667

Merged

Conversation

dmtrKovalenko
Copy link
Member

@dmtrKovalenko dmtrKovalenko commented Feb 12, 2020

Fixing crash if ref.current is null. We always need to check that ref.current is available before use

Reproduce the crash:

  1. Visit https://material-ui-pickers-git-feature-accessibility.mui-org.now.sh/demo/datepicker#responsiveness
  2. In the 3d dropdown select a year in the past (2000)
  3. Try to open second with only keyboard (tab and space on the calendar button)
  4. Observe the exception

@dmtrKovalenko dmtrKovalenko added the bug 🐛 Something doesn't work label Feb 12, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 12, 2020

Details of bundle changes.

Comparing: 9532f26...1f124be

bundle Size Change Size Gzip Change Gzip
@material-ui/core[umd] -- 318 kB ▼ -1 B (-0.00% ) 92.2 kB
ButtonGroup -- 83.4 kB ▼ -1 B (-0.00% ) 25.6 kB
CardActionArea -- 75.3 kB ▼ -1 B (-0.00% ) 23.8 kB
Checkbox -- 83.1 kB ▼ -1 B (-0.00% ) 26.3 kB
docs.main -- 601 kB ▼ -1 B (-0.00% ) 195 kB
Fab -- 77.1 kB ▼ -1 B (-0.00% ) 24.1 kB
StepButton -- 82.5 kB ▲ +1 B (0.00% ) 26.2 kB
Tab -- 76.6 kB ▼ -1 B (-0.00% ) 24.3 kB
Tabs -- 85.5 kB ▲ +1 B (0.00% ) 27.2 kB
@material-ui/core -- 359 kB -- 98.9 kB
@material-ui/lab -- 196 kB -- 58.1 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.5 kB -- 4.32 kB
Alert -- 83.6 kB -- 26.4 kB
AlertTitle -- 64.5 kB -- 20.3 kB
AppBar -- 64.3 kB -- 20.2 kB
Autocomplete -- 132 kB -- 41.5 kB
Avatar -- 65.5 kB -- 20.7 kB
AvatarGroup -- 62.7 kB -- 19.7 kB
Backdrop -- 68.2 kB -- 21.1 kB
Badge -- 65.6 kB -- 20.4 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
BottomNavigationAction -- 75.8 kB -- 24 kB
Box -- 72.2 kB -- 21.9 kB
Breadcrumbs -- 80.6 kB -- 25.4 kB
Button -- 80 kB -- 24.6 kB
ButtonBase -- 74.3 kB -- 23.4 kB
Card -- 63.2 kB -- 19.8 kB
CardActions -- 62.4 kB -- 19.6 kB
CardContent -- 62.3 kB -- 19.5 kB
CardHeader -- 65.4 kB -- 20.6 kB
CardMedia -- 62.7 kB -- 19.7 kB
Chip -- 82.8 kB -- 25.4 kB
CircularProgress -- 64.4 kB -- 20.3 kB
ClickAwayListener -- 3.84 kB -- 1.54 kB
Collapse -- 68.3 kB -- 21.2 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 62.3 kB -- 19.6 kB
Dialog -- 83.2 kB -- 25.9 kB
DialogActions -- 62.4 kB -- 19.6 kB
DialogContent -- 62.5 kB -- 19.6 kB
DialogContentText -- 64.4 kB -- 20.2 kB
DialogTitle -- 64.6 kB -- 20.3 kB
Divider -- 63 kB -- 19.8 kB
docs.landing -- 56.6 kB -- 15.6 kB
Drawer -- 84.9 kB -- 25.9 kB
ExpansionPanel -- 72.6 kB -- 22.7 kB
ExpansionPanelActions -- 62.4 kB -- 19.6 kB
ExpansionPanelDetails -- 62.3 kB -- 19.5 kB
ExpansionPanelSummary -- 78.3 kB -- 24.8 kB
Fade -- 23.6 kB -- 8.01 kB
FilledInput -- 73.9 kB -- 23 kB
FormControl -- 64.7 kB -- 20.2 kB
FormControlLabel -- 65.8 kB -- 20.7 kB
FormGroup -- 62.3 kB -- 19.5 kB
FormHelperText -- 63.7 kB -- 20 kB
FormLabel -- 63.8 kB -- 19.8 kB
Grid -- 65.4 kB -- 20.5 kB
GridList -- 62.8 kB -- 19.7 kB
GridListTile -- 64 kB -- 20.1 kB
GridListTileBar -- 63.5 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.22 kB
Hidden -- 66.3 kB -- 20.8 kB
Icon -- 63.1 kB -- 19.8 kB
IconButton -- 76.4 kB -- 23.9 kB
Input -- 72.9 kB -- 22.8 kB
InputAdornment -- 65.4 kB -- 20.6 kB
InputBase -- 71 kB -- 22.3 kB
InputLabel -- 65.6 kB -- 20.5 kB
LinearProgress -- 65.6 kB -- 20.5 kB
Link -- 66.9 kB -- 21.1 kB
List -- 62.7 kB -- 19.6 kB
ListItem -- 77.3 kB -- 24.3 kB
ListItemAvatar -- 62.4 kB -- 19.6 kB
ListItemIcon -- 62.5 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.5 kB
ListItemText -- 65.3 kB -- 20.5 kB
ListSubheader -- 63.1 kB -- 19.8 kB
Menu -- 88.8 kB -- 27.5 kB
MenuItem -- 78.4 kB -- 24.6 kB
MenuList -- 66.3 kB -- 20.8 kB
MobileStepper -- 68.1 kB -- 21.4 kB
Modal -- 14.3 kB -- 5.03 kB
NativeSelect -- 77.1 kB -- 24.4 kB
NoSsr -- 2.17 kB -- 1.03 kB
OutlinedInput -- 74.9 kB -- 23.4 kB
Pagination -- 85.3 kB -- 26.3 kB
PaginationItem -- 81 kB -- 25 kB
Paper -- 62.7 kB -- 19.6 kB
Popover -- 83.2 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.87 kB -- 1.29 kB
Radio -- 84.1 kB -- 26.6 kB
RadioGroup -- 64.7 kB -- 20.1 kB
Rating -- 70.7 kB -- 22.7 kB
RootRef -- 4.21 kB -- 1.63 kB
ScopedCssBaseline -- 63.1 kB -- 19.9 kB
Select -- 116 kB -- 34.7 kB
Skeleton -- 63.3 kB -- 20 kB
Slide -- 25.6 kB -- 8.74 kB
Slider -- 76.9 kB -- 24.2 kB
Snackbar -- 75.6 kB -- 23.7 kB
SnackbarContent -- 63.8 kB -- 20.1 kB
SpeedDial -- 86.4 kB -- 27.3 kB
SpeedDialAction -- 118 kB -- 37.6 kB
SpeedDialIcon -- 64.9 kB -- 20.3 kB
Step -- 63 kB -- 19.8 kB
StepConnector -- 63 kB -- 19.8 kB
StepContent -- 69.5 kB -- 21.8 kB
StepIcon -- 64.9 kB -- 20.3 kB
StepLabel -- 68.9 kB -- 21.7 kB
Stepper -- 65.2 kB -- 20.6 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB
SvgIcon -- 63.4 kB -- 19.8 kB
SwipeableDrawer -- 92.3 kB -- 28.9 kB
Switch -- 82.3 kB -- 26 kB
Table -- 62.9 kB -- 19.7 kB
TableBody -- 62.4 kB -- 19.5 kB
TableCell -- 64.3 kB -- 20.3 kB
TableContainer -- 62.3 kB -- 19.5 kB
TableFooter -- 62.4 kB -- 19.5 kB
TableHead -- 62.4 kB -- 19.5 kB
TablePagination -- 143 kB -- 42 kB
TableRow -- 62.8 kB -- 19.7 kB
TableSortLabel -- 77.6 kB -- 24.4 kB
TextareaAutosize -- 5.19 kB -- 2.17 kB
TextField -- 125 kB -- 36.7 kB
ToggleButton -- 76.4 kB -- 24.2 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.7 kB -- 19.7 kB
Tooltip -- 102 kB -- 32.4 kB
TreeItem -- 75.5 kB -- 23.9 kB
TreeView -- 68.9 kB -- 21.6 kB
Typography -- 64 kB -- 20 kB
useAutocomplete -- 14.7 kB -- 5.33 kB
useMediaQuery -- 2.56 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.13 kB

Generated by 🚫 dangerJS against 1f124be

@eps1lon eps1lon added the PR: needs test The pull request can't be merged label Feb 12, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can reproduce as described. We still need a reduced example that is testable. It's not entirely clear why this wouldn't be set.

The issue is that we might miss pulsating.

@dmtrKovalenko
Copy link
Member Author

Yeah, but it is anyway the bad practice to not check ref.current. Especially if you are calling it in useEffect, because it can be called when the element is actually unmounted

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

Yeah, but it is anyway the bad practice to not check ref.current.

Not really no. It might make more sense to throw a more descriptive error, sure. But if you expect the ref to be set, then a defensive check might hide actual issues.

That's why I'm asking for a test: To be sure that this is a legitimate state our component can be in or if there's a user mistake and we should throw a descriptive error.

@dmtrKovalenko
Copy link
Member Author

No, it is a bad practice – any code that can adjust null in this particular moment should be catched for nulls. If you will use typescript for Button.js you will get an error that ref.current can be nullable.
There is a clear thought about "why it is crashing" but it is better to net let it crash if you can

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

Could you engage with the point that we might want to crash because of invalid input?

@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Feb 12, 2020

It took me most of the day, but I found the reason https://codesandbox.io/s/wandering-night-ff46i

The problem is effect dependency disableRipple when it is changing from false to true useEffect is triggering and calling rippleRef.current.focus() but there is no mounted ripple dom element. BTW it is working only if we have several buttons mounted 🤔 and moving focus from one to another

I don't know why it was reproducing for me only after space or enter but the problem was pretty same - changing disableRipple.

I know that it is a edge-edge-edge case, but we don't need to crash even in such cases

@dmtrKovalenko
Copy link
Member Author

@eps1lon how I can write a test for this case? I expect this test to be written as e2e test, that running in real browser. But cannot figure out how can I write steps to control karma tests, could you help here?

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

The problem is effect dependency disableRipple when it is changing from false to true useEffect is triggering

I'm a bit lost about what's happening here. The if-block is only entered when disableRipple={false}. So the issue can't be from false->true but true->false.

I seem to remember some issue with overlapping ref and effect phases when looking at parents and their children. This might be the issue here. In that case we can only add a warning as far as I can tell.

I expect this test to be written as e2e test, that running in real browser.

Let's see if this only happens in a browser first. I don't think we need one.

@oliviertassinari
Copy link
Member

It seems to be an issue with a delay NoSsr introduces. Remove the component and it works OK.

@oliviertassinari
Copy link
Member

What about the following: we replace the NoSsr usage with a custom hook? Adding a defensive check would "hide" this underlying issue.

@oliviertassinari oliviertassinari changed the title [bug] Fix crashing if rippleRef is null [ButtonBase] Fix crashing if rippleRef is null Feb 12, 2020
@dmtrKovalenko
Copy link
Member Author

dmtrKovalenko commented Feb 12, 2020

I am not sure what you want me to do. I can add a warning and test using testing-lib, but are you sure it is a right idea trying to test dom .focus() without dom?
We 100% need to prevent crashing

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

but are you sure it is a right idea trying to test dom .focus() without dom?

JSDOM is close enough for focus managment. What are you missing from JSDOM regarding .focus()?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2020

Here is an interesting test case:

  1. It crashes on master TypeError: Cannot read property 'pulsate' of null
  2. It fails with this branch to have a length of 1 but got 0
  3. It passes without NoSsr

The challenge would be to make it pass while keeping the SSR boost :)?

      it.only('--', () => {
        function App() {
          const [enableRipple, setRipple] = React.useState(false);

          return (
            <div>
              <button
                data-testid="trigger"
                onClick={() => {
                  setRipple(true);
                }}
              >
                Trigger crash
              </button>
              <ButtonBase
                autoFocus
                TouchRippleProps={{
                  classes: {
                    ripplePulsate: 'ripple-pulsate',
                  },
                }}
                focusRipple
                disableRipple={!enableRipple}
              >
                1
              </ButtonBase>
              <ButtonBase autoFocus focusRipple>
                2
              </ButtonBase>
            </div>
          );
        }

        const { container, getByTestId } = render(<App />);

        fireEvent.click(getByTestId('trigger'));
        expect(container.querySelectorAll('.ripple-pulsate')).to.have.lengthOf(1);
      });

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

Yeah that was it. ~NoSsr has a layout effect. When a layout effect runs every queued passive effect runs as well. ince the ref phase for parents runs after the commit phase of the children the passive effect in ButtonBase runs after the ref is attached.

The children of NoSSR will be rendered after its first layout phase. This means that if you render NoSSR in layout phase A then its children will be rendered in layout phase A + 1. If you expect the ref of the NoSSR children to be attached in layout phase A + 1 you've made a false assumption. The ref will be attached in A + 1.

My guess is that <NoSsr>{condition && element}</NoSsr> instead of {condition && <NoSsr>{element}</NoSsr>} solves this issue. Edit: Confirmed #19667 (comment)

It's the same issue with passing refs across Suspense boundaries. Technically we would need to "wait" for the ref but I haven't looked into how we could achieve this (other than putting the instance into state).

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

The test from @oliviertassinari passes with

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -302,12 +302,12 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
       {...other}
     >
       {children}
-      {!disableRipple && !disabled ? (
-        <NoSsr>
-          {/* TouchRipple is only needed client-side, x2 boost on the server. */}
+      <NoSsr>
+        {/* TouchRipple is only needed client-side, x2 boost on the server. */}
+        {!disableRipple && !disabled && (
           <TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
-        </NoSsr>
-      ) : null}
+        )}
+      </NoSsr>
     </ComponentProp>
   );
 });

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2020

@eps1lon Nice!

I wonder if this wouldn't be an opportunity to replace this NoSsr with a hook.
Does the NoSsr component has a performance implication when rendering a list of many buttons? I also wonder about simplifying the React devtools output.

@oliviertassinari oliviertassinari added the component: ButtonBase The React component. label Feb 17, 2020
@dmtrKovalenko dmtrKovalenko changed the title [ButtonBase] Fix crashing if rippleRef is null [ButtonBase] Fix when changing enableRipple prop from false to true Feb 24, 2020
@oliviertassinari
Copy link
Member

Ouch, there is a conflict, somewhere between the tests.

@dmtrKovalenko
Copy link
Member Author

@oliviertassinari trying to debug this, can you point me to the right direction?
BTW question - why are you using mocha? It has really awful API for running parts of tests

@oliviertassinari
Copy link
Member

@dmtrKovalenko I could isolate the conflict down to between 'should fire event callbacks' and 'should not crash when changes enableRipple from false to true'.

why are you using mocha? It has really awful API for running parts of tests

Which alternative do you have in mind? I believe the motivation is so we can use karma-mocha.

const [enableRipple, setRipple] = React.useState(false);

React.useEffect(() => {
if (buttonRef.current) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the type of defensive checks I was working about that TypeScript encourage and that might hide root issues. @eps1lon what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not hiding issue of test, you can check it and without changes this test will still crash

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I was wondering about the pattern in general, what should be our "baseline" (default approach) for this concern :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need this check here? What happens if App mounts but the ref isn't instantiated?

Copy link
Member Author

@dmtrKovalenko dmtrKovalenko Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is for typescript compiler, to get rid of @ts-ignore. Actually it is impossible inside the useEffect (maybe only if somebody will specifically delete dom element after react commit phase and before effects are run?)

Right here it is not needed, but I am sure that in the code we have to guard the refs, even if it is hide bugs (IMHO such guards can't hide bugs, because e.g. in this particular issue element will not gain ripple focus if check was done - it is a bug as well, but no so critical as crash)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a bug as well, but no so critical as crash)

So we agree that it shouldn't be guarded against. There was a defect in the code which we wouldn't have been able to detect with a defensive check.

As long as typescript is only a type checker + transpiler I don't care about @ts-ignore in JSX. TypeScript is particularly bad with React. If it complains, we disable it. Just like with false positives in lint rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to get rid of this check in the test or handle the else case.

Copy link
Member Author

@dmtrKovalenko dmtrKovalenko Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 ok I will add throwing error in the else branch.
Cannot agree — we would be able. If there will be a guard, when enableRipple changes we simply not starting rippleEffect. It is an issue as well that can be noticed and fixed in the same way as current one.

But if some user will notice crash in the most basic Button component — it will spoil the trust to material-ui as qualified and reliable tool.

I have strong opinion that if something can crash — you must not let it crash. Even if you will use more reliable type system like ReasonML — it will complain about refs, because we cannot trust DOM, like we cannot trust users and we cannot trust developers who uses our components.

I can propose you the following api for using in the core components.

assertReactDomRef(ref, ref => ref.focus())
// or using new asserts syntax
assertReactDomRef(ref)
ref.focus() 

Which will assert ref is not null and if it’s missing warn user and ask to raise an issue. Thus our component will even look more clever :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand the issue, this is an internal consideration. Regarding the impact userland, I think that a fast feedback loop should be preferred.

Copy link
Member

@oliviertassinari oliviertassinari Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmtrKovalenko I love the new throw error in the branch, it's a great edge in case the action prop stop working as expected, we get a clear error message in the test. Smart! In an older test, we were using // @ts-ignore, which sounds great too.

But if some user will notice crash in the most basic Button component — it will spoil the trust to material-ui as qualified and reliable tool.

From my perspective, the ref effect should always run before a layout effect which also runs before an effect. The ref should always be defined. If it's not, then there is a deeper issue, that a defensive logic will hide, make it harder to uncover.

So in userland, I would recommend the usage of // @ts-ignore or to throw, like in the test cases.

@eps1lon
Copy link
Member

eps1lon commented Feb 24, 2020

I'm not following. The test from Olivier was passing with #19667 (comment). Since this behavior was not accepted we need to add a test that fails with #19667 (comment)

Reminder: Don't use github UI for longstanding PRs 😆

About mocha: What other framework would you use? Jest is no option because it can't run in a browser.

@dmtrKovalenko
Copy link
Member Author

I meant jest as test runner, but I got that you are using karma. Only one thing I know is better than mocha (as for me) is ava. But I don't think migration worth it :)

@oliviertassinari oliviertassinari merged commit 9143f70 into mui:master Feb 26, 2020
EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: ButtonBase The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants