[Select][material] Fix select menu moving on scroll when disableScrollLock is true #37773

VishruthR · 2023-07-01T22:20:50Z

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

Solves bug where if a user has a Select component with a MenuList that has disableScrollLock set to true, the MenuList will scroll with the screen.

Demo of bug: https://codesandbox.io/s/bitter-violet-wpfhc4?file=/src/App.js

Closes #37752

mui-bot · 2023-07-01T22:23:36Z

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8adf73f

VishruthR · 2023-07-09T03:16:34Z

@michaldudak @gzrae Right now the ci/codesandbox build is failing and I'm not sure why. When I run yarn build:codesandbox locally it works fine. Do you have any idea what might be wrong?

VishruthR · 2023-07-17T21:29:03Z

@michaldudak @gzrae Right now the ci/codesandbox build is failing and I'm not sure why. When I run yarn build:codesandbox locally it works fine. Do you have any idea what might be wrong?

Hi, just wanted to follow up on this question and see if you guys had any thoughts?

DiegoAndai

Hey @VishruthR, thanks for working on this! 😊 And sorry for the late response.

I think the solution works 🚀 I added two comments to improve it, let me know what you think

packages/mui-material/src/Popover/Popover.js

Signed-off-by: Vishruth Raj <42901020+VishruthR@users.noreply.github.com>

VishruthR · 2023-07-30T17:37:54Z

docs/data/joy/customization/theme-typography/NewTypographyLevel.js

@@ -6,11 +6,10 @@ import Typography from '@mui/joy/Typography';
 const customTheme = extendTheme({
 typography: {
 kbd: {
- background:
- 'linear-gradient(to top, var(--joy-palette-background-level2), var(--joy-palette-background-surface))',
+ backgroundColor: 'var(--joy-palette-background-surface)',


When I ran yarn docs:typescript:formatted this file changed

This is related to a Joy UI default theme update: https://github.com/mui/material-ui/pull/36843/files#diff-300d121fc27c24b580132ae34564f8ac77dfb0aeaeaa3dcbcce03c3ff31b7011

Can you try merging the latest master to your branch and see if this change goes away?

Git seems to think my branch is up to date with master so merging does nothing.

DiegoAndai

Thanks for working on the feedback 😊

A couple more polish comments 🙌🏼

packages/mui-material/src/Popover/Popover.js

DiegoAndai · 2023-07-31T15:57:26Z

packages/mui-material/src/Popover/Popover.js

 ...other
 } = props;

+ const disableScrollLock = props.disableScrollLock ?? false;


This can be added to the props destructure:

// ... disableMarginThreshold = false, disableScrollLock = false, ...other

We would have to explicitly add it to the Root props as it won't go inside the other props anymore, so on line 416:

const { slotProps: rootSlotPropsProp, ...rootProps } = useSlotProps({ elementType: RootSlot, externalSlotProps: slotProps?.root || {}, - externalForwardedProps: other, + externalForwardedProps: { + disableScrollLock, + ...other + }, additionalProps: { ref, slotProps: { backdrop: { invisible: true } }, container, open, }, ownerState, className: clsx(classes.root, className), });

It seems like adding this line caused me to fail a unit test. I'm not too sure what this error is saying exactly so any guidance would be great!

1) <Menu /> MUI component API allows overriding the root slot with an element using the slots.root prop: Expected test not to call console.error() but instead received 1 calls. If the warning is expected, test for it explicitly by using the toErrorDev() matcher. Test location: /tmp/material-ui/packages/mui-material/src/Menu/Menu.test.js console.error message #1: Warning: React does not recognize the `disableScrollLock` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `disablescrolllock` instead. If you accidentally passed it from a parent component, remove it from the DOM element. at i at Popover (/tmp/material-ui/packages/mui-material/src/Popover/Popover.js:4389:71) at /tmp/material-ui/node_modules/@emotion/react/dist/emotion-element-48d2c2e4.cjs.dev.js:62:23 at Menu (/tmp/material-ui/packages/mui-material/src/Menu/Menu.js:1761:70) at Wrapper (/tmp/material-ui/test/utils/createRenderer.tsx:403:7)

DiegoAndai · 2023-07-31T16:01:46Z

docs/data/joy/customization/theme-typography/NewTypographyLevel.js

@@ -6,11 +6,10 @@ import Typography from '@mui/joy/Typography';
 const customTheme = extendTheme({
 typography: {
 kbd: {
- background:
- 'linear-gradient(to top, var(--joy-palette-background-level2), var(--joy-palette-background-surface))',
+ backgroundColor: 'var(--joy-palette-background-surface)',


This is related to a Joy UI default theme update: https://github.com/mui/material-ui/pull/36843/files#diff-300d121fc27c24b580132ae34564f8ac77dfb0aeaeaa3dcbcce03c3ff31b7011

Can you try merging the latest master to your branch and see if this change goes away?

packages/mui-material/src/Popover/Popover.js

DiegoAndai · 2023-08-07T19:42:08Z

Here's the original repro codesandbox: https://codesandbox.io/s/bitter-violet-wpfhc4?file=/src/App.js:0-704

Here's the codesandbox showcasing the new functionality by using disableLockScroll and disableMarginThreshold: https://codesandbox.io/s/scrollable-select-vnppl4

@michaldudak may I ask a review from you here?

packages/mui-material/src/Popover/Popover.d.ts

packages/mui-material/src/Popover/Popover.js

michaldudak · 2023-08-21T09:46:58Z

packages/mui-material/src/Popover/Popover.test.js

 const positioningStyle = getElementStyleOfOpenPopover(mockedAnchor);

 expect(positioningStyle.top).to.equal(`${marginThreshold}px`);
 expect(positioningStyle.left).to.equal(`${marginThreshold}px`);
- expect(positioningStyle.transformOrigin).to.match(/0px 1px( 0px)?/);
+ expect(positioningStyle.transformOrigin).to.match(/0px -1px( 0ms)?/);


Pretty sure it is a typo 🤔, it's present in master: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Popover/Popover.test.js#L890

Fixed it

michaldudak

👍

bnussman-akamai · 2023-08-30T23:21:14Z

Thank you for fixing this! I'm seeing that on MacOS, the bouncy-scroll does not play well with this. Is there anything that can be done about this?

Screen.Recording.2023-08-30.at.7.20.09.PM.mov

michaldudak · 2023-08-31T07:10:31Z

@bnussman-akamai, could you please open a new issue so we can track this problem?

VishruthR added 2 commits July 1, 2023 12:30

Fix scrolling bug

d97378c

Remove console log

33bcb3b

VishruthR changed the title ~~[Select][material] Fix select menu moving on scroll~~ [Select][material] Fix select menu moving on scroll when disableScrollLock is false Jul 1, 2023

VishruthR changed the title ~~[Select][material] Fix select menu moving on scroll when disableScrollLock is false~~ [Select][material] Fix select menu moving on scroll when disableScrollLock is true Jul 1, 2023

VishruthR and others added 2 commits July 2, 2023 10:34

Fix unit tests for Popover and Menu

d40a457

Merge branch 'mui:master' into text-field-select-scroll-bug

f4a34fc

zannager added the component: select This is the name of the generic UI component, not the React module! label Jul 3, 2023

zannager requested a review from michaldudak July 3, 2023 13:00

DiegoAndai reviewed Jul 26, 2023

View reviewed changes

packages/mui-material/src/Popover/Popover.js Outdated Show resolved Hide resolved

packages/mui-material/src/Popover/Popover.js Outdated Show resolved Hide resolved

DiegoAndai added the package: material-ui Specific to @mui/material label Jul 26, 2023

VishruthR added 5 commits July 29, 2023 14:03

Address PR comments

d7853e1

Update props description

a67dc1f

Update proptypes

7247a03

Update popover docs

0ddca1f

Update translations docs for popover

b641aec

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2023

Merge branch 'master' into text-field-select-scroll-bug

3b3f9c0

Signed-off-by: Vishruth Raj <42901020+VishruthR@users.noreply.github.com>

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 30, 2023

VishruthR added 2 commits July 30, 2023 10:05

Fix typing

01718e7

Fix NewTypographyLevel

f037234

VishruthR commented Jul 30, 2023

View reviewed changes

VishruthR requested a review from DiegoAndai July 30, 2023 17:38

DiegoAndai reviewed Jul 31, 2023

View reviewed changes

DiegoAndai reviewed Aug 1, 2023

View reviewed changes

packages/mui-material/src/Popover/Popover.js Outdated Show resolved Hide resolved

packages/mui-material/src/Popover/Popover.js Outdated Show resolved Hide resolved

VishruthR and others added 3 commits August 2, 2023 16:22

Update Popover

d346411

Merge branch 'master' into text-field-select-scroll-bug

c9abe65

Remove unwanted joy docs change

a596e1d

Add and fix tests

5271ff1

michaldudak reviewed Aug 8, 2023

View reviewed changes

packages/mui-material/src/Popover/Popover.d.ts Outdated Show resolved Hide resolved

packages/mui-material/src/Popover/Popover.js Outdated Show resolved Hide resolved

bnussman-akamai mentioned this pull request Aug 14, 2023

refactor: [M3-7022] - Make <ActionMenu /> use MUI instead of Reach UI linode/manager#9540

Merged

DiegoAndai added 2 commits August 16, 2023 15:05

replace disableMarginThreshold w/ null marginThreshold

1944b4e

Add "prop: " to marginThreshold test

8b1ee79

michaldudak reviewed Aug 21, 2023

View reviewed changes

DiegoAndai self-assigned this Aug 21, 2023

Fix marginThreshold test transformOrigin typo

8adf73f

michaldudak approved these changes Aug 23, 2023

View reviewed changes

DiegoAndai merged commit c54ad8f into mui:master Aug 24, 2023
21 checks passed

bnussman-akamai mentioned this pull request Aug 29, 2023

chore: Update Material UI and Emotion linode/manager#9603

Merged

DiegoAndai mentioned this pull request Aug 29, 2023

[Select][material-ui] Don't lock the body scroll and make it non-modal select #17353

Open

michaldudak mentioned this pull request Aug 30, 2023

[Select] Menu opens above input with lots of options in v5 without a maxHeight #36776

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select][material] Fix select menu moving on scroll when disableScrollLock is true #37773

[Select][material] Fix select menu moving on scroll when disableScrollLock is true #37773

VishruthR commented Jul 1, 2023 •

edited

Loading

mui-bot commented Jul 1, 2023 •

edited

Loading

VishruthR commented Jul 9, 2023

VishruthR commented Jul 17, 2023

DiegoAndai left a comment

VishruthR Jul 30, 2023

DiegoAndai Jul 31, 2023

VishruthR Aug 4, 2023

DiegoAndai left a comment

DiegoAndai Jul 31, 2023

VishruthR Aug 4, 2023

DiegoAndai Jul 31, 2023

DiegoAndai commented Aug 7, 2023

michaldudak Aug 21, 2023

DiegoAndai Aug 21, 2023

michaldudak left a comment

bnussman-akamai commented Aug 30, 2023

michaldudak commented Aug 31, 2023

[Select][material] Fix select menu moving on scroll when disableScrollLock is true #37773

[Select][material] Fix select menu moving on scroll when disableScrollLock is true #37773

Conversation

VishruthR commented Jul 1, 2023 • edited Loading

mui-bot commented Jul 1, 2023 • edited Loading

Netlify deploy preview

Bundle size report

VishruthR commented Jul 9, 2023

VishruthR commented Jul 17, 2023

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldudak left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Aug 30, 2023

michaldudak commented Aug 31, 2023

VishruthR commented Jul 1, 2023 •

edited

Loading

mui-bot commented Jul 1, 2023 •

edited

Loading