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

[TextareaAutosize] Fix wrong height measurement #37185

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

theweipeng
Copy link
Contributor

if the textarea style contain paddingRight and value has multi line, the shadow textarea can't follow the textarea corrently. so we can't overwrite the paddingLeft and paddingRight

if the textarea style contain paddingRight and value contain multi line, shadow textarea can't follow the textarea corrently. 

Signed-off-by: weipeng <pythonvoid@outlook.com>
@mui-bot
Copy link

mui-bot commented May 6, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against fe2956b

@zannager zannager added the component: TextareaAutosize The React component. label May 8, 2023
@zannager zannager requested a review from michaldudak May 8, 2023 07:46
@michaldudak
Copy link
Member

Please provide a before/after reproduction so we can verify if it's indeed a bug and then test it.
Also, for the future, it's better to open an issue describing the problem first.

@theweipeng
Copy link
Contributor Author

theweipeng commented May 12, 2023

Please provide a before/after reproduction so we can verify if it's indeed a bug and then test it. Also, for the future, it's better to open an issue describing the problem first.

sorry,I will open an issue next time.

@michaldudak

Reproducing

code

create a TextareaAutosize which paddingRight 100 and boxSizing: 'border-box'

function App() {
  const [value, setValue] = useState("1");
  return (
    <div className="App">
      <div>value of TextareaAutosize: </div>
      <div>{value}</div>
      <div>TextareaAutosize paddingRight 100</div>
      <TextareaAutosize
        onChange={(e) => setValue(e.target.value)}
        value={value}
        style={{ paddingRight: 100, width: 200, boxSizing: "border-box", overflow: 'scroll' }}
      />
    </div>
  );
}

snapshot

we can find TextareaAutosize can't measure the height correctly。
in the file "packages/mui-base/src/TextareaAutosize/TextareaAutosize.tsx" we use "inputShallow.style.width = computedStyle.width;" to set the width of the shadow textarea. but if the textarea's boxSizing is 'border-box' and paddingRight greate than 0, the shadow textarea's width would set a incorrect value

image

textarea size info

both textarea has same width, but handle textarea has paddingRight
image
image

After fix

if we use textarea's paddingLeft and paddingRight , it would be work fine

image

@michaldudak
Copy link
Member

Looks good! Could you please add a test verifying the fix?

@ZeeshanTamboli ZeeshanTamboli added the PR: needs test The pull request can't be merged label Jun 19, 2023
@theweipeng
Copy link
Contributor Author

Looks good! Could you please add a test verifying the fix?

Certainly! I added a test to verify the fix.

@michaldudak michaldudak changed the title [TextareaAutosize] fix height measure wrong [TextareaAutosize] Fix wrong height measurement Jun 28, 2023
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Thanks! I've got some stylistic remarks. Besides that it's OK!

@michaldudak
Copy link
Member

Thanks for working on this!

@michaldudak michaldudak merged commit 2fa8d6c into mui:master Jun 29, 2023
18 checks passed
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work and removed PR: needs test The pull request can't be merged labels Jun 30, 2023
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: TextareaAutosize The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants