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

[material-ui] Revert visual regressions from #42283 #43364

Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Aug 19, 2024

Closes #43185

The regression was introduced in this change in PR #42283, where width: 100% was replaced with flexGrow: 1 in InputBase affecting the TextField component. The replacement was correct because flexGrow: 1 allows the input to take up remaining space in the flexbox layout when there are start or end adornments. The original width: 100% was a workaround for IE compatibility.

However, removing width: 100% caused the TextField to overflow its container, as it no longer adhered to the container's width. Earlier, with width: 100% on input, the custom container with a defined width rendered by the user became the closest ancestor for input width calculation, leading to it adjusting according to the container width.

Solution: Adding maxWidth: 100% to the TextField root ensures it stays within its container's boundaries without overflowing when the container is smaller. In case where the container is larger than the TextField, the TextField won't expand to fill the container and for that the fullWidth prop can be used when the TextField should occupy the full container width.

I've also added a visual regression test to prevent similar issues in the future.

Before: CodeSandbox
After Fix: CodeSandbox

Edit: See the discussion in PR. We are reverting some changes done in #42283

@ZeeshanTamboli ZeeshanTamboli added component: text field This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Aug 19, 2024
@mui-bot
Copy link

mui-bot commented Aug 19, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 7f80d96

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work regression A bug, but worse labels Aug 19, 2024
@@ -37,7 +37,9 @@ const TextFieldRoot = styled(FormControl, {
name: 'MuiTextField',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})({});
})({
maxWidth: '100%',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reasoning behind adding this to TextField vs. FormControl vs. InputBase? I'm not fully familiar with this abstraction hierarchy.

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want maxWidth: 100% to be applied directly to the TextField root because it's the component that's immediately impacted by the container's size. While you might consider applying it to FormControl, which TextField inherits, FormControl is primarily responsible for form-related context (like labels and helper text) which Text field also supports and not for the direct sizing or spacing of TextField. Adding maxWidth: 100% to FormControl could create unintended side effects for other form controls.

As for InputBase, it's used by other components like Select. Applying maxWidth: 100% there would impact all components that rely on InputBase, potentially causing layout issues where this behavior isn't needed. And it won't even fix the issue because it is not a direct child of the container.

TextField is a higher-level component that combines InputBase and FormControl, providing a simplified API for common use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation

@aarongarciah
Copy link
Member

@ZeeshanTamboli the after fix codesanbox seems to be private

@ZeeshanTamboli
Copy link
Member Author

@ZeeshanTamboli the after fix codesanbox seems to be private

As usual, forgot to make it public 😓. Should be visible now.

@siriwatknp
Copy link
Member

siriwatknp commented Aug 20, 2024

To be honest, I'd revert some of the changes in #42283 (InputBase and StepLabel).

Because it's hard to justify which one is a bug. You might say this PR is a fix but it could be a regression for users who want to control the width larger than the container. Now the code below does not work due to max-width: 100%.

<div style={{ width: 200 }}>
  <TextField sx={{ width: 300 }} />
</div>

or we don't merge this PR and add to the migration guide that the TextField has an implicit width. To make it follow the container, add sx={{ width: 100% }}.

@mnajdova
Copy link
Member

To be honest, I'd revert some of the changes in #42283 (InputBase and StepLabel).

Agree, if these changes introduced any perceived changes for people we should revert them.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Aug 20, 2024

but it could be a regression for users who want to control the width larger than the container.

I think such cases would be rare, but you might be right. Users can override it with max-width: none:

<div style={{ width: 200 }}>
  <TextField sx={{ width: 300, maxWidth: 'none' }} />
</div>

However, this adds an extra step, which could make them see it as a regression. I believe width: 100% on the inner input base is incorrect. Adding this to the migration doc might be better than reverting the changes. We shouldn't add workaround code to support IE. What do you think? It's a tough decision.

@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 20, 2024

I agree with @siriwatknp and @mnajdova, let's revert the changes. The intent of the PR was to remove IE-11-only code, but if that creates changes in other browsers, then it shouldn't be there. I understand we could discuss if "width: 100% on the inner input base is incorrect", but this is out of scope for v6, especially since it is so close to stable. Besides, v7 will be a significant visual change, so there's even less incentive for CSS changes like this one.

@siriwatknp why do you think we should also include the StepLabel one? Is there any reported issue with it?

@siriwatknp
Copy link
Member

why do you think we should also include the StepLabel one? Is there any reported issue with it?

The removal of flexShrink: 0, definitely changes the behavior of the StepLabel when the container's width is less than the StepLabel.

There is no reported issue yet, just to prevent it.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Aug 20, 2024

I agree with @siriwatknp and @mnajdova, let's revert the changes. The intent of the PR was to remove IE-11-only code, but if that creates changes in other browsers, then it shouldn't be there. I understand we could discuss if "width: 100% on the inner input base is incorrect", but this is out of scope for v6, especially since it is so close to stable. Besides, v7 will be a significant visual change, so there's even less incentive for CSS changes like this one.

Alright. I agree, it shouldn't block the release. I'll let you handle this PR since you pushed the commit.

@DiegoAndai DiegoAndai changed the title [material-ui][TextField] Fix Text Field overflowing container [material-ui] Revert visual regressions from https://github.com/mui/material-ui/pull/42283 Aug 20, 2024
@DiegoAndai DiegoAndai changed the title [material-ui] Revert visual regressions from https://github.com/mui/material-ui/pull/42283 [material-ui] Revert visual regressions from #42283 Aug 20, 2024
@DiegoAndai
Copy link
Member

@aarongarciah @siriwatknp ready for review

@ZeeshanTamboli ZeeshanTamboli added the component: stepper This is the name of the generic UI component, not the React module! label Aug 20, 2024
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: stepper This is the name of the generic UI component, not the React module! component: text field This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[material-ui][TextField] Text Fields are suddenly overflowing when using versions newer than 6.0.0-alpha.9
6 participants