-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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 ResizeObserver causing infinite selectionchange
loop
#45351
base: master
Are you sure you want to change the base?
Conversation
2f579d4
to
7dfb4fe
Compare
Netlify deploy previewhttps://deploy-preview-45351--material-ui.netlify.app/ Bundle size report |
@DiegoAndai I think this is the most reasonable compromise between both the original RO error and the new LMK if you think this is the right way to go, if so I'll polish this a bit (and figure out how to test it 🤔 ) |
Thanks for jumping on this @mj12albert! 🙌🏼
I'm curious: why does the event fire?
To check I understand correctly: The compromise is that the "unwanted" |
@DiegoAndai TBH I don't know 😓 but just observing console.logs I'm quite certain this is the case...
I think typically this would be whenever the width changes, e.g. resizing the browser, but also contrived ways like in repro from the prior issue: React.useEffect(() => {
setTimeout(() => {
if (textareaWrapperRef.current != null) {
textareaWrapperRef.current.style.width = '200px';
}
}, 2000);
}, []);
Yeah, the compromise is that it doesn't seem like there is anything we can do about this, and it was happening before the latest change I did anyway I wonder if this ever happened in @aaw5017's real use-case, like if there is a fixed width in CSS maybe it won't fire the RO callback 🤔 |
@mj12albert, thanks for the explanation. Yes, let's move forward with this fix. |
fcec848
to
92026df
Compare
await render(<App />); | ||
// tick 1 second | ||
await sleep(1000); | ||
expect(handleSelectionChange.callCount).to.lessThanOrEqual(3); |
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.
before applying the fix this would fire 61 times in browser tests
after the fix, it's 3 in browser tests, 2 when I test in the browser console, and it doesn't work in the unit tests...
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.
expect(handleSelectionChange.callCount).to.lessThanOrEqual(3)
I'm not really sure what the best way to test this is, just trying to avoid making it slow or potentially flaky, do you think this is good enough? @DiegoAndai
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.
Yes, I think this should be enough to catch it if it comes back 👍🏼
e8cddfe
to
318e505
Compare
@mj12albert let me know if this is ready for review 😊 |
318e505
to
187d697
Compare
Fixes #45307
Demo: https://codesandbox.io/p/devbox/runtime-wood-v4mn8t
The issue is that:
.observe()
s, (the global, uncancellable)selectionchange
event firesAfter the fix: