-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[base-ui][TextareaAutosize] Fix resizing instability #41638
[base-ui][TextareaAutosize] Fix resizing instability #41638
Conversation
Netlify deploy previewhttps://deploy-preview-41638--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
Thanks for working on this @ZeeshanTamboli. The after demo looks like it's working properly 🎉. I read mui/base-ui#168 (comment) and reviewed the code, but I failed to understand the issue and solution. May I ask you to explain it to me? Thanks in advance. |
@DiegoAndai Sure. Quoting mui/base-ui#168 (comment):
The Previously, we were updating the
This indicates that previously, when state was utilized (before I removed it in #40789), the So the solution here I believe is to store the height in a ref and update it only when it is different from the previous value, as mentioned in mui/base-ui#189 (comment). Marija added the "PR: needs test" label. I'm uncertain about how to add a test for this. I expressed the same concern in mui/base-ui#189 (comment). Do you have any suggestions? I hope it helps. |
Thanks for the detailed explanation @ZeeshanTamboli. The solution makes more sense now 👍🏼 Part of the confusion was due to the Something I noticed is that with this change, the Screen.Recording.2024-03-27.at.09.16.04.movWhich was not the case previously. Although I think this isn't correct either: Screen.Recording.2024-03-27.at.09.16.29.movI would handle this in a separate issue/PR
By providing a ref to the component, would it be possible to spy on how many times and with which values the Heads up: I'll be on vacation starting tomorrow (March 28th) until April 8th, so I won't attend to notifications during this time. To move this PR forward, please reach out to @michaldudak 😊 From my perspective, this fix makes sense, and it should be merged. Let's try to figure out a way to test it first. |
Correct. We can do this in a follow-up PR.
If resizing is done by dragging, which is a native feature of the textarea HTML element, it shouldn't consider maxRows or minRows. Essentially, it behaves like a regular
I believe we should find a way to test resizing the component by dragging and then verify that it doesn't glitch by ensuring the height value isn't being set each time. |
This makes sense 👍🏼
Have you been able to explore testing this fix? |
No, I haven't. I'm not sure how to simulate dragging the textarea programmatically in a test. |
We could try this: https://testing-library.com/docs/user-event/pointer#moving-a-pointer |
But how do we target the bottom-right dragger of the textarea? Strangely, I haven't come across any test case examples for textarea dragging online. |
If we know the textarea's width and height we should be able to move the cursor to the bottom-right corner, right? (I've never tested this either, just throwing ideas 😅) |
I've attempted the following approach: it.only('should not glitch when resizing textarea', function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}
const { container } = render(<TextareaAutosize />);
const textarea = container.querySelector<HTMLTextAreaElement>('textarea')!;
console.log('before textarea.style.height: ', textarea.style.height);
// Get the element's dimensions
const { top, left, width, height } = textarea.getBoundingClientRect();
// Calculate coordinates of bottom-right corner
const bottomRightX = left + width;
const bottomRightY = top + height;
fireEvent.mouseDown(textarea, { clientX: bottomRightX, clientY: bottomRightY });
fireEvent.mouseMove(textarea, { clientX: bottomRightX + 50, clientY: bottomRightY + 50 });
fireEvent.mouseUp(textarea);
console.log('after textarea.style.height: ', textarea.style.height);
}); But despite these efforts, the height remains unchanged. I'm testing it in the browser rather than JSDOM. I've also reached out for assistance on Stack Overflow: https://stackoverflow.com/questions/78319726/how-to-resize-textarea-by-dragging-in-react-testing-library. |
The code you shared was exactly what I imagined 👌🏼 let's wait for a while to see if we get any luck on Stack Overflow. Did you test with user event instead of fire event? https://testing-library.com/docs/user-event/pointer |
Tried with it but getting the same results. |
@DiegoAndai I tested it by adding an E2E test using Playwright! I think an E2E test is more suitable here for this case and gives higher confidence. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work figuring the test out @ZeeshanTamboli!
A couple minor questions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @ZeeshanTamboli 🙌🏼
Similar to mui/base-ui#189. This issue needs fixing here as it's a regression. It also affects the
TextField
component with themultiline
prop which uses TextareaAutosize. Additionally, it needs to be released from the master branch under v5.It doesn't need to be synced with
base-ui
repo since it will be deprecated there - see mui/base-ui#189 (comment). I'll close that PR once this gets merged.Before: https://mui.com/base-ui/react-textarea-autosize/
After: https://deploy-preview-41638--material-ui.netlify.app/base-ui/react-textarea-autosize/