[base-ui][material-ui][joy-ui][Modal] Add scrollLockContainer prop

[Jack-Works]

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

close #34513

[mui-bot]

Netlify deploy preview

https://deploy-preview-34697--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d399d5d

[Jack-Works]

hello, eslint is emitting errors on code generated by yarn proptypes. I don't know how to handle this.

[michaldudak]

Thanks for your work!

Sometimes the autogenerated prop types have to be adjusted. Changing them to scrollLockContainer: HTMLElementType should help (they won't be overridden the next time yarn proptypes is run).

Could you add a test verifying the new feature?

[Jack-Works]

I added the test case but it does not work correctly... btw it works well in codesandbox

https://github.com/mui/material-ui/pull/34697/files#diff-512815f88107e3b8b7093f4e725b2be98da8ac1f7fda1683334a394d643da96fR146-R147

[Jack-Works]

hello, is there any chance to let this continue?

[michaldudak]

Could you please address Olivier's comment?
Also, please merge in the latest master.

[Jack-Works]

Could you please address Olivier's comment? Also, please merge in the latest master.

ok

[Jack-Works]

About CI and test errors:

Fake browser and real browser tests:

  1) ModalManager
       overflow
         should handle the scroll on custom scroll target:

      AssertionError: expected '32px' to equal '1057px'
      + expected - actual

      -32px
      +1057px
      
      at Context.<anonymous> (packages/mui-base/src/Modal/ModalManager.test.ts:158:42)
      at processImmediate (internal/timers.js:464:21)

  2) ModalManager
       overflow
         should restore styles correctly if none existed before:

      AssertionError: expected '' to equal 'hidden'
      + expected - actual

      +hidden
      
      at Context.<anonymous> (packages/mui-base/src/Modal/ModalManager.test.ts:191:44)
      at processImmediate (internal/timers.js:464:21)

Chrome Headless 113.0.5672.53 (Linux x86_64) ModalManager overflow should handle the scroll on custom scroll target FAILED
	AssertionError: expected '32px' to equal '33px'
	    at Context.<anonymous> (/tmp/_karma_webpack_291811/commons.js:47622:53)
Chrome Headless 113.0.5672.53 (Linux x86_64) ModalManager overflow should handle the scroll on custom scroll target FAILED
	AssertionError: expected '32px' to equal '33px'
	    at Context.<anonymous> (/tmp/_karma_webpack_291811/commons.js:47622:53)
.
Chrome Headless 113.0.5672.53 (Linux x86_64) ModalManager overflow should restore styles correctly if none existed before FAILED
	AssertionError: expected '' to equal 'hidden'
	    at Context.<anonymous> (/tmp/_karma_webpack_291811/commons.js:47651:55)
Chrome Headless 113.0.5672.53 (Linux x86_64) ModalManager overflow should restore styles correctly if none existed before FAILED
	AssertionError: expected '' to equal 'hidden'
	    at Context.<anonymous> (/tmp/_karma_webpack_291811/commons.js:47651:55)

I have no idea why this appears because it works in the playground. You can try it with the following playground content:

import { Box, Button, Dialog, DialogActions, DialogTitle } from "@mui/material";
import { useState } from "react";

export default function () {
  const [o, s] = useState(false)
  return <>
    <Button onClick={() => {
      const style = document.querySelector('html')!.style;
      if (style.paddingRight === "344px") style.paddingRight = ""
      else style.paddingRight = "344px"
    }}>Add/remove padding</Button>
    <Button variant="contained" onClick={() => s(true)}>Open</Button>
    AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA AAAAAAAAAAAA
    <br />
    <Box height="6474px"></Box>
    <Dialog open={o} scrollLockContainer={globalThis?.document?.querySelector('html')!}>
      <DialogTitle>Hello</DialogTitle>
      <DialogActions>
        <Button onClick={() => s(false)}>Close</Button>
      </DialogActions>
    </Dialog>
  </>
}

[michaldudak]

I cannot add types to the code above because it is generated with yarn proptypes

In some cases, the autogenerated types need to be corrected.
If you overwrite the autogenerated code with scrollLockContainer: HTMLElementType, it won't get overwritten again the next time yarn proptypes is executed.

I have no idea why this appears because it works in the playground. You can try it with the following playground content

Perhaps, as @oliviertassinari, it would be better to test the whole Modal instead of just the ModalManager.

[Jack-Works]

it would be better to test the whole Modal instead of just the ModalManager

do you mean write test in the Modal instead of ModalManager?

[michaldudak]

Yes, to test the whole thing, not just the ModalManager in isolation.

[Jack-Works]

Sorry I'm still interested in this PR but recently I don't have time to work on it!

[Jack-Works]

@ZeeshanTamboli Hi I fixed the test in the real browser (headless browser). I need to disable the still-failing tests in fake browser test because they cannot calculate height/scrollbar stuff, how can I do that?

[ZeeshanTamboli]

@ZeeshanTamboli Hi I fixed the test in the real browser (headless browser). I need to disable the still-failing tests in fake browser test because they cannot calculate height/scrollbar stuff, how can I do that?

@Jack-Works You can bypass jsdom (fake browser) at the beginning of the test with the following code:

if (/jsdom/.test(window.navigator.userAgent)) {
  this.skip();
}

An example of this can be found here.

[Jack-Works]

@ZeeshanTamboli thanks! now all test passed

[ZeeshanTamboli]

Looks good. Just left a few comments.

[ZeeshanTamboli]

@Jack-Works Overall, it looks good to me. However, I noticed an issue (#17353) regarding not locking the scroll for Select, particularly highlighted in this comment. Although it pertains to Select, but since Select inherits from Modal, this property will also be available on Select.

I'll defer the final decision and review/approval of this PR to other members like @DiegoAndai and @michaldudak, as they may better assess whether to introduce this new feature in v5 or consider it for v6 or v7.

In my opinion, I think this feature is useful and suitable for the Modal component because it involves having a custom container for scroll lock with the use case mentioned in the related issue. It's not about having the need to not lock the scroll like #17353. This looks good to me.

[DiegoAndai]

Thanks for working on this! I agree that the feature is useful.

I wonder how this fits with the future of our floating components like the Popup and Modal. The new Popup component is based on Floating UI. Should the Modal be based on it as well? Floating UI does have a dialog guide.

I ask this because, right now, we lack a vision of how these components should behave and be used. IMO, all of these should share APIs and mental models. For example, if you know how to modify the scroll behavior on the Popup, you should be able to transfer that learning to the Modal. Basing all of them on the same engine, Floating UI, would help us achieve it.

Sorry if this seems like a tangent, but I think it's relevant as I wouldn't like to introduce an API that we would like to change one or two majors down the road.

@michaldudak.

[michaldudak]

Yes, ideally we use the same building blocks where applicable, so I'd be keen to see the Modal use Floating UI under the hood. It won't happen until at least v7, though.

[DiegoAndai]

Yes, ideally we use the same building blocks where applicable

I suggest putting API changes to the modal (and floating components), like this one, on hold until we have a clearer vision of the future of these components. I wouldn't like to add a prop that would change in the next year. What do you think @michaldudak? We could add it to v7's milestone.

[michaldudak]

Yes, these changes could potentially be breaking, so adding them to v7 seems like a good direction.

[Jack-Works]

emm, isn't v7 too far away from us? We're currently using a patched version of mui because we need this feature.

I wouldn't like to add a prop that would change in the next year

or can you add it as an experimental API and tell developers this will have breaking change?

[DiegoAndai]

Hi @Jack-Works

isn't v7 too far away from us?

We're targeting v7 for the end of this year, so it's far away, but not too much.

The problem with introducing APIs is that we add complexity to the components. This is not to say that we should never do it, but that we should be careful and thoughtful about doing it.

An experimental API also adds complexity and is a promise. I would strive only to add experimental APIs if we plan to make them stable. Users who use these APIs are left stranded if the API is unstable forever or abandoned.

Your PR seems like a good addition. The problem is that we already have a lot of complexity regarding the floating components: different APIs and conventions. We also have an ongoing migration to a new engine: Floating UI. For this case, one API change is better than multiple ones, as the multiple uncoordinated changes got us here in the first place. Working on this in v7 seems like the opportunity to do it properly and thoughtfully so we can develop a consistent and ergonomic API.

Hopefully, this is not a massive inconvenience for you. If your current solution is not satisfactory, we might be able to help improve it until we release v7.

cc: @michaldudak

[Jack-Works]

oh, if we can use scrollbar-gutter and dialog element to build popups, we may no longer need this PR

[DiegoAndai]

Closing as stale, but I added #34513 to the "Material UI with Base UI" milestone to revisit when Base UI's floating components are ready.