[Popper] Fix to defer setting of exited state to Transition component

[Sharakai]

• I have followed (at least) the PR section of the contributing guide.

Fixes #15180

• Added 3 additional Popper keepMounted prop behaviour tests, with failing test case for the fixed issue.
• Fixed by removing the getDerivedStateFromProps and deferring the exited: false state setting to the Transition component

This'll be followed up by an additional PR to include a failing test case, and fix, for the Modal component - it suffers from the same bug.

Test Failure on next (sans-fix):

(Note that I only ran the prop: keepMounted tests to generate the screenshot)

[mui-pr-bot]

Details of bundle changes.

Comparing: 1fa8f94...3b5d8e0

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.01%	-0.02%	347,568	347,548	89,893	89,872
@material-ui/core/Paper	0.00%	0.00%	68,404	68,404	20,053	20,053
@material-ui/core/Paper.esm	0.00%	0.00%	60,735	60,735	18,785	18,785
@material-ui/core/Popper	-0.06%	-0.08%	34,930	34,910	11,917	11,907
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,877	17,877	5,962	5,962
@material-ui/core/useMediaQuery	0.00%	0.00%	2,463	2,463	1,044	1,044
@material-ui/lab	-0.01%	-0.02%	148,119	148,099	43,913	43,904
@material-ui/styles	0.00%	0.00%	53,143	53,143	15,443	15,443
@material-ui/system	0.00%	0.00%	17,132	17,132	4,519	4,519
Button	0.00%	0.00%	88,669	88,669	26,423	26,423
Modal	0.00%	0.00%	82,688	82,688	24,802	24,802
colorManipulator	0.00%	0.00%	3,745	3,745	1,537	1,537
docs.landing	0.00%	0.00%	49,820	49,820	10,807	10,807
docs.main	-0.00%	-0.01%	644,630	644,610	200,877	200,866
packages/material-ui/build/umd/material-ui.production.min.js	-0.01%	-0.00%	296,257	296,237	82,811	82,809

Generated by 🚫 dangerJS against 3b5d8e0

[Sharakai]

Although partially off-topic for this PR, I've noted that the Popper and Modal component not aligned in how they handle Transition children. Is this intentional, or should they be closer aligned?

For instance:

1. Popper uses a transition prop to tell it to attach transition-related handlers, whereas Modal infers it from whether children.props has an in property - Why not change Popper to the same as Modal and remove the transition prop (or perhaps have transition has an override)?
2. Popper (now) attaches in, onEntered, and onExited, whereas Modal only attaches onEntered and onExited - Why not in?

Just something to keep in mind. Happy to action either if either are desired, albeit will raise a separate issue & PR.

[oliviertassinari]

A bug fix with code removal, that's awesome 👍

A follow-up on the Modal component would be greatly appreciated!

[Sharakai]

A bug fix with code removal, that's awesome 👍

A follow-up on the Modal component would be greatly appreciated!

👍 Am working on the Modal component, but applying the same fix corrects the issue for Modal, but breaks 9 other test cases across Drawer, SwipeableDrawer, and others :/

[oliviertassinari]

@Sharakai Oh, we would need to check if these fails are legitimate on the Modal before moving forward with the Popper. Man, it's not an easy topic you got yourself into!

[Sharakai]

@oliviertassinari
I'll raise the Modal PR soon with the test failures, so you can see what tests are failing and whether I'm correct in my assertions. From what I've found so far, though:

1. here in Drawer.test.js
   • Drawer uses the Modal component, which defaults keepMounted to false. It looks like this test was "exploiting" this bug as the child component is still mounted when it, in fact, should not be.
2. here in SwipeableDrawer
   • This test actually fails in the SwipeableDrawer code here as this.backdropRef is undefined or null.
   • This test renders the Drawer > Modal open once, then back to closed. This calls the handleBodyTouchEnd function, which calls this.setState here - This synchronously renders before continuing with the rest of the handleBodyTouchEnd function, meaning the Modal content (and Backdrop) are unmounted, and their ref's become undefined.
   • Guarding against this.backdropRef being falsey at SwipeableDrawer.js#L133 obviously works around this error, and may even be correct as we don't want to apply transitions to something that doesn't (didn't) visibly exist.
3. <Menu> integration (2 test timeout errors - not yet investigated)
4. <Select> integration (2 test timeout errors - not yet investigated)

[oliviertassinari]

@Sharakai I'm having a look.

[eps1lon]

I still don't understand this test. It's described under prop: keepMounted but this prop is not used in the test. It also seems like that this is a step back. Why would I want the children to be hidden after I set the open={true}?