-
Notifications
You must be signed in to change notification settings - Fork 8
fix: Fixes auto-scroll releasing when transaction commits #367
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 94.98% 95.40% +0.41%
==========================================
Files 67 67
Lines 3250 3263 +13
Branches 692 703 +11
==========================================
+ Hits 3087 3113 +26
+ Misses 163 150 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| document.activeElement.scrollIntoView?.({ behavior: "smooth", block: "nearest" }); | ||
| } | ||
| this.scrollRepeat(); |
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.
Doesn't scrollIntoView already scroll? Why do we need this.scrollRepeat()?
If there is no activeElement in the document, where is this supposed to scroll?
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.
The scrollRepeat repeatedly checks for the cursor position and if it is on edge - it will scroll automatically with small time increments.
The scheduleActiveElementScrollIntoView is an action which is called on-demand. To have no conflict with the repeated scroll, it reuses the same timeout reference. First, it clears the timeout which is normally set every 10ms by scroll repeat. Then, it performs its own scrolling after a delay. Finally, it has to restart the repeated auto-scroll, as otherwise it won't work anymore after scheduleActiveElementScrollIntoView() is called.
| expect(scroll1.top).toBe(0); | ||
|
|
||
| // This should cancel the operation. | ||
| await page.mouseMove(0, 255); |
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.
Does this go beyond the window and cause scrolling?
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.
This goes to the bottom edge of the window, which is enough. The auto-scrolling activates when the cursor is within 50px from either top or bottom screen edge.
Description
Fixes a bug when auto-scrolling would not stop after transaction commits. Logically, when the transaction is submitted or discarded, the auto-scroll handlers should be removed, which is supposed to prevent the scroll. However, this does not happen due to a rather complex lifecycle involved. Removing unneeded React state and making the event listeners added and removed only once per component mount/unmount fixes the problem.
Before:
Screen.Recording.2025-08-25.at.18.15.39.mov
After:
Screen.Recording.2025-08-25.at.18.16.07.mov
Rel: AWSUI-61150, similar to: #368
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.