[material-ui][Modal] Apply aria-hidden to the correct elements when Modals are mounted to other places than document.body

[Gr3q]

Fixes https://github.com/mui/material-ui/issues/19450

Lets do https://github.com/mui/material-ui/pull/34165 again. Before if someone used disablePortal and opened a modal the whole page became inaccessible to Screen Readers because aria-hidden only got applied to siblings of document.body, in this case all of them. This PR fixes that.

Changes:

• Make sure we pass in the correct container if disablePortal is used.
• If the modal container is deep in the tree we need to make sure every sibling in every ancestor level is aria-hidden and every aria-hidden we added gets cleaned up properly on unmount.
• In addition because we don't differentiate between aria-hidden tags that got added by modals and added by devs, we need to pierce through (remove) every aria-hidden attribute in every ancestor of the last modal container; this is to prevent no accessible elements on a page when multiple modals are present.

Known problems:

• You will have a bad time if you put your dialog in a container next to 1000s of siblings - bad idea before, bad idea now.
• Previous bug still stands when you mess with aria-hidden states in the tree outside your modal, when the modal gets removed your changes won't be preserved (because the modal will restore the state when it was added)

I'll port this to next too when I have time.

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

[mui-bot]

Netlify deploy preview

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

Bundle size report

@mui/material parsed: 🔺+320B^(+0.06%) gzip: 🔺+160B^(+0.10%) @mui/lab parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/system parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/utils parsed: 0B^(0.00%) gzip: 0B^(0.00%)

Show details for 100 more bundles (86 more not shown)

@mui/material/Dialog parsed: 🔺+326B^(+0.34%) gzip: 🔺+135B^(+0.40%) @mui/material/Modal parsed: 🔺+326B^(+0.37%) gzip: 🔺+131B^(+0.41%) @mui/material/Menu parsed: 🔺+323B^(+0.31%) gzip: 🔺+153B^(+0.42%) @mui/material/Popover parsed: 🔺+323B^(+0.33%) gzip: 🔺+143B^(+0.41%) @mui/material/Select parsed: 🔺+323B^(+0.23%) gzip: 🔺+143B^(+0.30%) @mui/material/SwipeableDrawer parsed: 🔺+322B^(+0.31%) gzip: 🔺+136B^(+0.37%) @mui/material/Drawer parsed: 🔺+321B^(+0.33%) gzip: 🔺+122B^(+0.35%) @mui/material/TextField parsed: 🔺+321B^(+0.21%) gzip: 🔺+132B^(+0.26%) @mui/material/TablePagination parsed: 🔺+320B^(+0.18%) gzip: 🔺+140B^(+0.25%) @mui/lab/AdapterDateFns parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/AdapterDayjs parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/AdapterLuxon parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/AdapterMoment parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/CalendarPicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/CalendarPickerSkeleton parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/ClockPicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DatePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DateRangePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DateRangePickerDay parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DateTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DesktopDatePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DesktopDateRangePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DesktopDateTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/DesktopTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/LoadingButton parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/LocalizationProvider parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/Masonry parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/MobileDatePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/MobileDateRangePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/MobileDateTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/MobileTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/MonthPicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/PickersDay parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/StaticDatePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/StaticDateRangePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/StaticDateTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/StaticTimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TabContext parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TabList parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TabPanel parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/Timeline parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineConnector parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineContent parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineDot parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineItem parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineOppositeContent parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimelineSeparator parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TimePicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TreeItem parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/TreeView parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/useAutocomplete parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/lab/YearPicker parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Accordion parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AccordionActions parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AccordionDetails parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AccordionSummary parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Alert parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AlertTitle parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AppBar parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Autocomplete parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Avatar parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/AvatarGroup parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Backdrop parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Badge parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/BottomNavigation parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/BottomNavigationAction parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Box parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Breadcrumbs parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Button parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/ButtonBase parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/ButtonGroup parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Card parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CardActionArea parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CardActions parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CardContent parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CardHeader parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CardMedia parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Checkbox parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Chip parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CircularProgress parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/ClickAwayListener parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Collapse parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Container parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/CssBaseline parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/DefaultPropsProvider parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/DialogActions parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/DialogContent parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/DialogContentText parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/DialogTitle parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Divider parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Fab parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Fade parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FilledInput parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FormControl parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FormControlLabel parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FormGroup parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FormHelperText parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/FormLabel parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/GlobalStyles parsed: 0B^(0.00%) gzip: 0B^(0.00%) @mui/material/Grid parsed: 0B^(0.00%) gzip: 0B^(0.00%)

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 069631770fec95a40adfe7bcfe76eacc4102f5a1

[Gr3q]

Your argos check seems to be borked especially I made no changed that would affect visuals.

[Gr3q]

Should I not touch the unstable folder in the future? Could you explain why does it exist?

[ZeeshanTamboli]

Should I not touch the unstable folder in the future? Could you explain why does it exist?

I reverted the changes in Base UI (mui-base directory). It included the unstable folder because Base UI was still in beta. We no longer maintain Base UI in this repository—it's now legacy and will be removed in the future (timeline unknown). Material UI used to depend on Base UI, but we've since separated the code, causing duplication. We don't plan to make any further changes to Base UI here. The issue is related to Material UI's Modal. Base UI with headless components is now being developed in a separate repo: mui/base-ui, which will be integrated into Material UI once stable.

[Gr3q]

In addition because we don't differentiate between aria-hidden tags that got added by modals and added by devs, we need to pierce through (remove) every aria-hidden attribute in every ancestor of the last modal container; this is to prevent no accessible elements on a page when multiple modals are present.

I didn't understand this. Can you please explain more clearly with an example?

I think I explained the technicality of it here but basically someone can put a Modal with disablePortal inside an element that is already aria-hidden and should be aria-hidden, then opens said Modal. In that case we can't afford not to remove that aria-hidden tag from the ancestor because we cannot allow to be the whole UI inaccessible, that's the absolute worst case.

If you have multiple modals that function that gets aria-hidden elements currently cannot differentiate between if it's actually a dev mistake (and warn) or it was ModalManager that added that prop.

Previous bug still stands when you mess with aria-hidden states in the tree outside your modal, when the modal gets removed your changes won't be preserved (because the modal will restore the state when it was added)

What bug is this? Can you give a bug reproduction in the form of StackBlitz or CodeSandbox?

We get the original elements aria-hidden states to restore (were they originally hidden or not) at modal mount time. When we come back and restore them on modal unmount, someone might've changed one of those elements while the modal was mounted. In the end we restore them to a state that the dev changed since then. This I think this can only be partially fixed and it would be some effort to implement.

We also need to add a test case for the Modal component with the disablePortal prop to verify that aria-hidden is handled correctly

I'll add some more tests now.

[Gr3q]

test added

[Gr3q]

@Gr3q, it would’ve been great to use aria-modal instead of managing aria-hidden ourselves. That would have allowed us to remove all this logic. Unfortunately, I guess it's not widely supported yet, and also ariaModal API isn't available on Firefox versions below 119, while we still support Firefox from version 115. Although we don't use ariaModal property, do you think we can use aria-modal attribute now?

even though screenreader support is not as good it can still be added. But the docs https://w3c.github.io/aria/#aria-modal implies that there should be only 1 aria-modal at a time, so some logic would need to be written to remove it from all modals except the toplevel one.

Plus according the docs we can't escape implementing this same logic because all other elements bust be marked as inert (instead of aria-hidden). Technically it's not a MUST, but by their spec a modal doesn't enforce focus either.

Quote from the docs

When a modal element is displayed, authors SHOULD mark all other contents as inert (such as "inert subtrees" in HTML) if the ability to do so exists in the host language.

[Gr3q]

That's the problem of the developer mounting the Modal on the wrong place. We shouldn't handle this and remove aria-hidden for them. They should be able to find out that they are mounting the Modal in the wrong container or remove aria-hidden themselves. Could you remove this part of the logic and the relevant test for it (if any).

I was a little too verbose on the dev errors. Taking away dev errors, ModalManager adds (and removes) aria-hidden tags itself.

Just using that as baseline in this example scenario:

1. User is interacting with a dialog with disablePortal enabled
2. The UI has a dialog that opens when it loses connection to the backend without disablePortal (so it mounts next to root). The frontend loses connection and the dialog opens while the first dialog was open.

In this case the whole page becomes inaccessible because the dialog the user was using hid the parent element of the lost connection dialog beforehand (only if we remove that part of the code as you suggested).

This is a common enough scenario to handle and even if it isn't I can't guarantee there isn't another edge-case caused by ModalManager manipulating aria-hidden tags unless you remove disablePortal and container support from it altogether. So that code must stay.

[Gr3q]

I noticed that in the Menu component (which uses Modal), the scroll isn’t locked when the disablePortal prop is used. Here's a video showing the behavior in this PR vs production

I see. I'm not sure what is the optimal logic when selecting the scrollContainer in ModalManager.ts#handleContainer.

This https://github.com/mui/material-ui/pull/33168 changed it so if the modal is in shadow dom it always selects body. Maybe we need to do that in all cases?

Edit: I probably need to ~recursively~ go up the tree to select the correct scrollable ancestor.

[ZeeshanTamboli]

Edit: I probably need to ~recursively~ go up the tree to select the correct scrollable ancestor.

Yes, that seems like the right approach. You can use the isOverflowing method to check for a vertical scrollbar, and if there isn't, move up using parentElement.

[Gr3q]

I fixed it, the original code was not the best either because it had isOverflowing and containerWindow.getComputedStyle(parent).overflowY === 'scroll' that more or less do the same thing, but one is used when applies padding and the other one is used to hide the scrollbar (and they are applied to the same elements).

I could probably refactor this further to do both in 1 loop (aside from the shadow dom fix).

I don't know how to write the tests for this, the expected outcome is that the page and the modal cannot be scrolled.

• when disabledPortal is used
• when there are multiple scrollable ancestors
• when there are multiple scrollable ancestors and each of those elements have a fixed element
• when there are multiple scrollable ancestors and each of those elements have a fixed element, and the second (nested) scrollable has position:relative

The fixed elements should still look good and be cleaned up properly.

I know that the current tests are not passing.

[Gr3q]

I know that the current tests are not passing.

Those tests are verifying the wrong things, here is a snipped from one, see my comments:

     // This is wrong, this indicates nothing, the page might or might not be scrollable
      expect(document.body.parentElement.style).to.have.property('overflow', '');

      setProps({ open: true });

      // it wasn't scrollable so we didn't need to apply overflow: hidden to it, this fails because of it.
      expect(document.body.parentElement.style).to.have.property('overflow', 'hidden');

      setProps({ open: false });

      expect(document.body.parentElement.style).to.have.property('overflow', '');

I could try to fix them but I don't know what I should expect instead.

[Gr3q]

I changed the tests for Modal, as I said before they were testing the wrong things, now they are actually checking if their child is mounted or not on open/close.

The remaining tests that are failing in ModalManager because of this test (assumption) "should disable the scroll even when not overflowing". I admit I don't understand why is that necessary when the page is not scrollable. Maybe because the Modal content itself can cause an overflow? I can add some code so we always apply overflow: hidden to the container as well and those tests will most likely pass.

[Gr3q]

I fixed the behaviour now completely, you can manually add another overflow:scroll around the menu button in the docs to test nested scrollable containers. The top bar is also not moving permanently to the left anymore.

The only thing left are the new tests.

Edit: and the existing tests are breaking because for some reason the window.document.documentElement.clientWidth reports 0 in the tests for some reason.

[Gr3q]

I can do that, the question is will it work properly if we have a Menu with disablePortal inside a Dialog that has scrollable content, which is not a good pattern, but still?

[Gr3q]

I reverted back the scrollLock change and amended the tests for it. It seems to work fine for both disabledPortal and regular modals even with nested scrollbars.

Tests are passing on my end, I can commit pnpm dedupe changes but I much prefer you would do that - the tooling for this project gives me trouble on every turn for some reason.

[Gr3q]

@ZeeshanTamboli any progress on this?

[ZeeshanTamboli]

@Gr3q aria-modal property was added to Dialog in the recent release version 6.1.5 in #44118.

[Gr3q]

@Gr3q aria-modal property was added to Dialog in the recent release version 6.1.5 in https://github.com/mui/material-ui/pull/44118.

Yes...and? What do you want me to do about it?

Can I assume you tested screen-reader (including Android, iOS and Windows screen-readers) compatibility, tested multiple open modals at the same time including nested modals? It all works and usable, not just compliant?

You can take this PR or leave it, but I definitely don't have time to do any of that for you with aria-modals, that would be just starting from scratch for me with all that testing. Plus I wouldn't be able to remove this patch from our own codebase anyway when I'm done (without good reason), when this implementation passed 3 accessibility audits already.

If you take it, you can build upon it to add/remove inert props when they become widely supported (instead of aria-hidden), plus this and https://github.com/mui/material-ui/pull/44118 can exist side-by-side.

You can also leave this PR or take it over completely, I don't mind. In that case I also don't take responsibility for any of the functionality either.

[ZeeshanTamboli]

@Gr3q aria-modal property was added to Dialog in the recent release version 6.1.5 in #44118.

Yes...and? What do you want me to do about it?

Can I assume you tested screen-reader (including Android, iOS and Windows screen-readers) compatibility, tested multiple open modals at the same time including nested modals? It all works and usable, not just compliant?

You can take this PR or leave it, but I definitely don't have time to do any of that for you with aria-modals, that would be just starting from scratch for me with all that testing. Plus I wouldn't be able to remove this patch from our own codebase anyway when I'm done (without good reason), when this implementation passed 3 accessibility audits already.

If you take it, you can build upon it to add/remove inert props when they become widely supported (instead of aria-hidden), plus this and #44118 can exist side-by-side.

You can also leave this PR or take it over completely, I don't mind. In that case I also don't take responsibility for any of the functionality either.

@Gr3q, I just wanted to inform you and see if that could simplify things. No need to start from scratch.

Could you take a look at this comment: https://github.com/mui/material-ui/pull/43318#discussion_r1816582932?

[Gr3q]

@Gr3q, I just wanted to inform you and see if that could simplify things. No need to start from scratch.

If you want to keep the previous behaviour - make only the toplevel modal accessible for screenreaders and to be able to trap focus - then I can't.

But I can't say for sure without some real screenreader testing with aria-modals set, like how they handle focus, tabbing, multiple aria-modals, nested aria-modals etc. I could only simplify the code here if they are capable of doing the work for us.

[Gr3q]

This page is not scrollable: https://deploy-preview-43318--material-ui.netlify.app/material-ui/react-modal/. Maybe some logic is wrong.

I narrowed it down to the server-side modal on the page. I'll check where it's going wrong.

Edit: The server-side modal in the docs is always set to open and disableScrollLock is not set on it, so it's technically working as intended. It didn't work as intended before, so technically this is a breaking change?

[ZeeshanTamboli]

The server-side modal in the docs is always set to open and disableScrollLock is not set on it, so it's technically working as intended. It didn't work as intended before, so technically this is a breaking change?

The issue seems to be that the server-side modal has a custom container (container={() => rootRef.current!}). Previously, handleContainer considered this custom container, so overflow: hidden wasn’t applied to it (because it didn't overflow), and scrolling wasn’t locked. Now, it's always the body element, so overflow: hidden is applied, locking scrolling. This is the breaking change I was concerned about in this discussion.

[Gr3q]

The issue seems to be that the server-side modal has a custom container (container={() => rootRef.current!}). Previously, handleContainer considered this custom container, so overflow: hidden wasn’t applied to it (because it didn't overflow), and scrolling wasn’t locked. Now, it's always the body element, so overflow: hidden is applied, locking scrolling. This is the breaking change I was concerned about in https://github.com/mui/material-ui/pull/43318#discussion_r1786644290.

@ZeeshanTamboli I'm not sure I have a good answer to that.

My assumption is that disableScrollLock is made to prevent scrolling on the page when a modal is open, not on the immediate (parent) container, because that doesn't really have the desired effect (doesn't actually prevent scrolling). Maybe my assumption is wrong.

We made that change because it was not behaving as desired for disablePortal. I treat disablePortal and container the same, because what you can do with disablePortal={true} you can do it directly with container. For example in the server-side example if you remove setting container it will display exactly the same as with container.

If they are meant to be different I'm not sure what is the best course of action, container prop has nothing in it's docstring indicating that it will affect scroll locking behaviour in any way.

In short I can't come up with have any use-cases where the behaviour you are describing is useful by myself. If can provide me one or two I can start implementing something that matches those use cases too

[ZeeshanTamboli]

@ZeeshanTamboli I'm not sure I have a good answer to that.

My assumption is that disableScrollLock is made to prevent scrolling on the page when a modal is open, not on the immediate (parent) container, because that doesn't really have the desired effect (doesn't actually prevent scrolling). Maybe my assumption is wrong.

We made that change because it was not behaving as desired for disablePortal. I treat disablePortal and container the same, because what you can do with disablePortal={true} you can do it directly with container. For example in the server-side example if you remove setting container it will display exactly the same as with container.

If they are meant to be different I'm not sure what is the best course of action, container prop has nothing in it's docstring indicating that it will affect scroll locking behaviour in any way.

In short I can't come up with have any use-cases where the behaviour you are describing is useful by myself. If can provide me one or two I can start implementing something that matches those use cases too

As per the current production code, the prevention of scrolling is on the container, where the scrollContainer in the code is the resolvedContainer (either the one provided by the developer or the default body). If you inspect in https://mui.com/material-ui/react-modal/#server-side-modal, you will see overflow: hidden is applied on Box container.

You are right that the disablePortal={true} is the same as container. The container prop only has an effect when disablePortal={false}. When disablePortal={true}, the container prop is ignored since no portal is created. But I think it's about the scroll lock style getting applied on the container. What if a developer has a custom container and no disablePortal? I think we will have to revert a few commits here till this comment.

[Gr3q]

@ZeeshanTamboli I'm not sure I have a good answer to that. My assumption is that disableScrollLock is made to prevent scrolling on the page when a modal is open, not on the immediate (parent) container, because that doesn't really have the desired effect (doesn't actually prevent scrolling). Maybe my assumption is wrong. We made that change because it was not behaving as desired for disablePortal. I treat disablePortal and container the same, because what you can do with disablePortal={true} you can do it directly with container. For example in the server-side example if you remove setting container it will display exactly the same as with container. If they are meant to be different I'm not sure what is the best course of action, container prop has nothing in it's docstring indicating that it will affect scroll locking behaviour in any way. In short I can't come up with have any use-cases where the behaviour you are describing is useful by myself. If can provide me one or two I can start implementing something that matches those use cases too

As per the current production code, the prevention of scrolling is on the container, where the scrollContainer in the code is the resolvedContainer (either the one provided by the developer or the default body). If you inspect in https://mui.com/material-ui/react-modal/#server-side-modal, you will see overflow: hidden is applied on Box container.

You are right that the disablePortal={true} is the same as container. The container prop only has an effect when disablePortal={false}. When disablePortal={true}, the container prop is ignored since no portal is created. But I think it's about the scroll lock style getting applied on the container. What if a developer has a custom container and no disablePortal? I think we will have to revert a few commits here till this comment.

If you want to preserve that behaviour I agree, as long as disablePortal will work the same as now in this PR.

You did most of the changes there, would you like me to change it now or would you like to do it?

[ZeeshanTamboli]

You did most of the changes there, would you like me to change it now or would you like to do it?

Please look into it. I don't have the bandwidth for it.

[Gr3q]

As per the current production code, the prevention of scrolling is on the container, where the scrollContainer in the code is the resolvedContainer (either the one provided by the developer or the default body). If you inspect in https://mui.com/material-ui/react-modal/#server-side-modal, you will see overflow: hidden is applied on Box container.

You are right that the disablePortal={true} is the same as container. The container prop only has an effect when disablePortal={false}. When disablePortal={true}, the container prop is ignored since no portal is created. But I think it's about the scroll lock style getting applied on the container. What if a developer has a custom container and no disablePortal? I think we will have to revert a few commits here till https://github.com/mui/material-ui/pull/43318#issuecomment-2382701323.

I'm changing the code, but I want to summarize the bugs this PR had, then what you want.

Bugs:

1. scrollLock doesn't work for disablePortal on the docs page - I found out the correct behaviour is to lock the body element (just like before) because that's the only way scroll locking works. When container is provided locking was applied to the container (and that didn't work, this is the bevaiour on the main branch)
2. Because I changed (fixed) the scroll locking behaviour to always apply to body, the whole modal doc page got locked. This was due to the server-side modal always open with no disableScrollLock. disablePortal applied or not doesn't make a difference here.

So let me clarify what you want:

You want me to change it that scroll lock works peropely when:

• disablePortal is set
• disablePortal is not set

and it shouldn't work properly (aka scroll lock applies to the container that effectively does nothing to prevent page scrolling):

• container is set, regardless if disablePortal is set or not. This means even though container is not used when disablePortal is true and the modal shouldn't be mounted to the container at all, you still want the scroll lock to apply to that container.

I want you to confirm this because this is the only way bugs 1 and 2 can be resolved on the Modal docs page at the same time. (Edit: I'm asking because I will need to implement some workarounds to handle that)

[ZeeshanTamboli]

@Gr3q It's been a while, but from what I remember you're right—that's what's needed.

[Gr3q]

@ZeeshanTamboli I fixed the doc page, it does what you want (so no breaking changes).

Added a comment that technically the behaviour around the scrolllock+container is still a bug, hopefully someone will remove it when the next major version comes.

Fixed the existing tests and added new tests for this too.

So now the PR should be ready. :crossed_fingers: