[Dialog] Remove scroll prop #14717
[Dialog] Remove scroll prop #14717
Conversation
eps1lon
added
the
component: dialog
| @@ -7,7 +7,6 @@ import Fade from '../Fade'; | |||
| export const styles = { | |||
| /* Styles applied to the root element. */ | |||
| root: { | |||
| zIndex: -1, | |||
There was a problem hiding this comment.
I'm probably overly naive here. But wouldn't you want the backdrop to block the main document? Therefore putting it behind sounds wrong. Is this required to make it work with other components?
There was a problem hiding this comment.
Try the scroll body demo. You should see a problem with the scroll.
There was a problem hiding this comment.
https://deploy-preview-14717--material-ui.netlify.com/demos/dialogs/#scrolling-long-content ? I don't "see a problem". Could explain what's expected, what actually happens and why this should happen?
There was a problem hiding this comment.
You can't scroll when hovering the backdrop.
There was a problem hiding this comment.
jQuery ui doesn't have a backdrop: https://codesandbox.io/s/7wx2k0mmrq. The rest of the document isn't inert i.e. if I scroll both body and modal scroll. The argument doesn't apply.
There was a problem hiding this comment.
@oliviertassinari I'm not aiming for some gotcha here. This is me legit asking where you've seen scrollbars that are detached from the content they scroll and if you have some resources that describe this UX pattern.
Sorry, I have updated the comment replacing "ask for" with "need".
If they justify that need sure. It doesn't look that way though looking through the linked issues/prs.
There was a problem hiding this comment.
Please, let's move to a different topic, we should keep the scroll body. I don't want to get into the fundamental why, I doubt there is any non-opinionated A + B = C type of logical discussion we can get on the subject. I would rather save time by benchmarking what the others are doing. It's well accepted in literature and real world applications. There were smarter people than me that came to this conclusion
- It's the default behavior of Bootstrap, Ant Design, jQuery UI & more. However, it's not our default behavior. Maybe it should?
- +100,000 views on https://stackoverflow.com/questions/10476632/how-to-scroll-the-page-when-a-modal-dialog-is-longer-than-the-screen
There was a problem hiding this comment.
+100,000 views on stackoverflow.com/questions/10476632/how-to-scroll-the-page-when-a-modal-dialog-is-longer-than-the-screen
Accepted answer implements scroll=paper
Completely covering the background window also hides background movement that occurs on some mobile devices when scrolling content inside the dialog.
describes scroll=paper doesn't implement it though.
real world applications
Trello does it, accepted.
google products don't as far as I can tell.
Backdrop click bug is still an issue though
There was a problem hiding this comment.
google products don't as far as I can tell.
Yes. I know that Gmail doesn't do it & Keep doesn't do it. I haven't look deeper.
Backdrop click bug is still an issue though
Yeah, it was the cost of supporting this feature. @joshwooding & I couldn't find a better solution 😢. We have copied Bootstrap strategy.
Details of bundle changes.Comparing: ecc2c85...6902c6f
|
eps1lon
force-pushed
the
refactor/Dialog/backdrop
branch
from
6902c6f to
5f4d8a4
Compare
Had a hard time finding a proper way to test this behavior on a mounted component. Looks like this implements the same feature while getting rid of some internal gotchas (onBackdropClick not actually being fired from the Backdrop, opinionated
role="document"that looked like it was only attached to appease the linter etc.@joshwooding did most of the work to fix #13648. Could you take a look if this is still good?
If this is accepted I'll improve the tests later to use
mount. Don't think the tests give use much confidence here anyway since it relies heavily on event bubbling/capturing. Would be better to have a real e2e test here.