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

[SwipeableDrawer] Discovery and hysteresis broken in React v18 #30414

Closed
2 tasks done
peterhirn opened this issue Dec 27, 2021 · 8 comments · Fixed by #34505
Closed
2 tasks done

[SwipeableDrawer] Discovery and hysteresis broken in React v18 #30414

peterhirn opened this issue Dec 27, 2021 · 8 comments · Fixed by #34505
Assignees
Labels
bug 🐛 Something doesn't work component: SwipeableDrawer The React component.

Comments

@peterhirn
Copy link

peterhirn commented Dec 27, 2021

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

  • Discovery opens drawer completely
  • Partially opened drawer (> hysteresis) is not opened completely

Expected behavior 🤔

No response

Steps to reproduce 🕹

https://codesandbox.io/s/unruffled-poincare-2v4pk

Context 🔦

No response

Your environment 🌎

"@emotion/react": "^11.7.1",
"@emotion/styled": "^11.6.0",
"@mui/material": "^5.2.5",
"react": "18.0.0-rc.0",
"react-dom": "18.0.0-rc.0"
@peterhirn peterhirn added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 27, 2021
@peterhirn peterhirn changed the title [SwipeableDrawer] Discovery broken in React v18 [SwipeableDrawer] Discovery and hysteresis broken in React v18 Dec 27, 2021
@mnajdova
Copy link
Member

I don't understand what is the problem. Can you expend a bit on describing the problem?

@mnajdova mnajdova added the component: SwipeableDrawer The React component. label Dec 28, 2021
@peterhirn
Copy link
Author

Sure.

  1. Discovery should open the drawer 20px (default) which works in v17 but opens the drawer completely in v18.
  2. When the drawer is dragged out < hysteresis it closes again (works in both versions). If the drawer is dragged >= hysteresis it opens fully (works in v17), in v18 it just stays partially opened.

Run my sample (or any react 18 sample) in eg. chromium and switch to responsive/mobile in the dev tools. Then click and hold within 20px of the left border of the app to reproduce.

@michaldudak michaldudak added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 12, 2022
@michaldudak
Copy link
Member

I can reproduce it. It seems like a bug indeed.

@kawork1
Copy link

kawork1 commented Apr 29, 2022

Seeing this issue as well. Any update on this?

@michaldudak
Copy link
Member

We haven't done any work on it so far. We're happy to accept contributions from the community.

@tech-meppem
Copy link
Contributor

tech-meppem commented Aug 30, 2022

I can confirm this is happening still.

My best guess at what I believe what's happening is that when rendering the drawer / paper component, it deletes/replaces the existing styles, instead of just overriding the ones that have changed.
This means that the transform style is deleted, and as such, the discovery doesn't work.
Although, that's only a guess based on logging output of styles that I've been debugging.

I've been working on my PR's unit tests, trying to figure out why they are broken for a long time now, and I now believe this to be the reason.

@mnajdova mnajdova self-assigned this Sep 26, 2022
@mnajdova
Copy link
Member

mnajdova commented Sep 27, 2022

There is a change in the behavior between React 17 and React 18 in terms of when the paper ref is being set, which makes this line https://github.com/mui/material-ui/blob/master/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js#L492 not being invoked in React 18, while it is invoked in React 17.
Edit: By using ModalProps ={{ keepMounted: true }}, this line is being invoked in React 18 too, but the issue is still happening, so we can eliminate this as the main problem.
I will need to dive a bit deeper to see why this is happening. It may be related to the Transition component which is used inside the Drawer.

React 18 codesandbox: https://codesandbox.io/s/unruffled-poincare-2v4pk
React 17 codeasndbox: https://codesandbox.io/s/upbeat-fast-5zj4rx?file=/src/index.tsx


I will try to create a simpler repro to see what may be the issue, as there are quite a lot of moving parts here: Drawer, Modal, Slider etc.

@mnajdova
Copy link
Member

I've created #34505 which is fixing the two issues mentioned here. I am not 100% happy with the fix, as it requires using keepMounted on the Modal, but let's see if other maintainers will have some suggestions around this.

Here is a sandbox with the fix: https://codesandbox.io/s/7qkr9k?file=/demo.tsx

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: SwipeableDrawer The React component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants