fix: after selecting and dragging text, the table still scrolls even after releasing the mouse. #328
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The package version was changed from 3.19.2 to 3.19.1, possibly to correct a versioning error or revert a previous update.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthroughๅจ Changes
Sequence Diagram(s)sequenceDiagram
participant User as ็จๆท
participant Document as Document
participant Hook as useScrollDrag
User->>Document: mousedown / ้ไธญๆๆฌ
Document->>Hook: onMouseDown / onMouseMove (ไฟๆๅ่กไธบ)
Note right of Hook #DFF2E1: mouseDownLock ๅฏ่ฝไธบ true\nๆปๅจๅฏ่ฝๅจ่ฟ่ก
User->>Document: dragstart
Document->>Hook: dragstart -> clearDragState()
Note right of Hook #FCE8D6: clearDragState ๅๆญขๆปๅจๅจ็ป\nๅนถๅฐ mouseDownLock ่ฎพไธบ false
User->>Document: drop / dragend / mouseup
Document->>Hook: dragend / mouseup -> clearDragState()
Estimated code review effort๐ฏ 3 (Moderate) | โฑ๏ธ ~20 minutes Possibly related PRs
Poem
Tip ๐ Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. โจ Finishing Touches
๐งช Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ๐ชง TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Reportโ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #328 +/- ##
==========================================
+ Coverage 97.85% 97.86% +0.01%
==========================================
Files 19 19
Lines 794 798 +4
Branches 193 193
==========================================
+ Hits 777 781 +4
Misses 17 17 โ View full report in Codecov by Sentry. ๐ New features to boost your workflow:
|
Introduces a test to verify that the list does not scroll when table text is dragged and dropped, ensuring onScroll is not triggered during drag-and-drop interactions.
|
ๆๆฒกๆๅคงไฝฌreviewๅ |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where text dragging in a table continues to trigger scrolling after the mouse is released. The issue occurred when users selected text, dragged it outside the table, and released the mouse - subsequent mouse movements would still cause unwanted table scrolling.
- Adds native drag event listeners to properly reset drag state during native drag operations
- Introduces a centralized
clearDragStatefunction to handle state cleanup consistently - Adds comprehensive test coverage for the text dragging scenario
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| src/hooks/useScrollDrag.ts | Implements drag event handlers and centralizes state cleanup logic |
| tests/scroll.test.js | Adds test case to verify scrolling doesn't occur after text drag operations |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
๐งน Nitpick comments (3)
tests/scroll.test.js (3)
767-772: ้ๅค่ฐ็จ selectElementText๏ผๆ ๅฎ้ ๆไนselectElementText ๅจ่ฟๅ ฅ if ๅๆฏๅๅทฒๆง่กไธๆฌก๏ผๅๆฏๅ ๅๆฌก่ฐ็จๅนถไธๆนๅ่กไธบ๏ผๅปบ่ฎฎๅป้คไปฅ้ฟๅ ๆททๆทใ
- if (targetItem && listHolder) { - selectElementText(targetItem); + if (targetItem && listHolder) {
735-735: ็จไพๆ ้ขๅปบ่ฎฎไปโtableโๆนไธบโlistโๆฌไปๅบๅบๆฏๆฏ่ๆๅ่กจ๏ผๆ ้ขไธญ็ โtableโ ๅฎนๆๅผ่ตท่ฏฏ่งฃ๏ผๅปบ่ฎฎๆดๅ็กฎ่กจ่พพใ
-it('should not scroll after drop table text', () => { +it('should not scroll after dropping selected list text', () => {
761-761: ้ๆฉๅจไธๆธฒๆ็ปๆ็่ฆๅๅปบ่ฎฎ้ไฝไธบไบ้ๆฉ็ฎๆ ๅ ็ด ๏ผ่ฟ้็ป li ไบบไธบๅ ไธไบ fixed-item ็ฑปใ่ฝ็ถ่ฝๅทฅไฝ๏ผไฝไผ่ฎฉๆต่ฏๅฏน็ฑปๅๆไพ่ตใไนๅฏไปฅ็ดๆฅ้ๆฉ็ฌฌไธไธช li ๅ ็ด ๏ผ่ดด่ฟ็ๅฎๆธฒๆ็ปๆ๏ผ้ไฝ่ฆๅใ
- {({ id }) => <li className="fixed-item">{id}</li>} + {({ id }) => <li>{id}</li>}ๅๆถๆๅ็ปญๆฅ่ฏขๆนไธบ๏ผ
- const fixedItems = container.querySelectorAll('.fixed-item'); + const fixedItems = container.querySelectorAll('li');
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ก Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
๐ Files selected for processing (2)
src/hooks/useScrollDrag.ts(3 hunks)tests/scroll.test.js(1 hunks)
๐ง Files skipped from review as they are similar to previous changes (1)
- src/hooks/useScrollDrag.ts
Comment on lines
+735
to
+801
| it('should not scroll after drop table text', () => { | ||
|
|
||
| // Helper function to select text content of an element | ||
| const selectElementText = (element) => { | ||
| const range = document.createRange(); | ||
| range.selectNodeContents(element); | ||
| const selection = window.getSelection(); | ||
| selection.removeAllRanges(); | ||
| selection.addRange(range); | ||
| }; | ||
|
|
||
| const onScroll = jest.fn(); | ||
| const onDragStart = jest.fn(); | ||
| const onDragEnd = jest.fn(); | ||
| document.addEventListener('dragstart', onDragStart); | ||
| document.addEventListener('dragend', onDragEnd); | ||
|
|
||
| const { container } = render( | ||
| <List | ||
| component="ul" | ||
| itemKey="id" | ||
| itemHeight={20} | ||
| height={100} | ||
| data={genData(200)} | ||
| onScroll={onScroll} | ||
| > | ||
| {({ id }) => <li className="fixed-item">{id}</li>} | ||
| </List>, | ||
| ); | ||
| const fixedItems = container.querySelectorAll('.fixed-item'); | ||
| const targetItem = fixedItems[0]; | ||
| if (targetItem) { | ||
| selectElementText(targetItem); | ||
| } | ||
| const listHolder = container.querySelector('.rc-virtual-list-holder'); | ||
| if (targetItem && listHolder) { | ||
| selectElementText(targetItem); | ||
|
|
||
| fireEvent.dragStart(targetItem, { bubbles: true }); | ||
|
|
||
| const rect = listHolder.getBoundingClientRect(); | ||
| fireEvent.dragOver(listHolder, { | ||
| clientY: rect.bottom + 10, | ||
| bubbles: true, | ||
| }); | ||
|
|
||
| fireEvent.drop(listHolder, { | ||
| clientY: rect.bottom + 10, | ||
| bubbles: true, | ||
| }); | ||
|
|
||
| fireEvent.dragEnd(targetItem, { bubbles: true }); | ||
| } | ||
| expect(onScroll).not.toHaveBeenCalled(); | ||
| expect(onDragStart).toHaveBeenCalled(); | ||
| expect(onDragEnd).toHaveBeenCalled(); | ||
|
|
||
| if (listHolder) { | ||
| const rect = listHolder.getBoundingClientRect(); | ||
| const mouseMoveEvent = new MouseEvent('mousemove', { | ||
| bubbles: true, | ||
| clientY: rect.top - 10, | ||
| }); | ||
| listHolder.dispatchEvent(mouseMoveEvent); | ||
| } | ||
| expect(onScroll).not.toHaveBeenCalled(); | ||
|
|
There was a problem hiding this comment.
ๆต่ฏๆช่งฆๅโๆไฝๆๆฝๆปๅจโ๏ผๆญ่จๅฏ่ฝๆไธบไผช้ณๆง๏ผ็ผบๅฐ mousedownโmousemove ๅฏผ่ดๅๅงๆปๅจ๏ผ
ๅฝๅ็จไพไป ๆจกๆไบ้ไธญๆๆฌ + dragStart/dragOver/drop/dragEnd๏ผ็ถๅๅจๆๅๅไธๆฌก mousemove ๅนถๆญ่จ onScroll ๆช่ขซ่งฆๅใไฝ็ฑไบไปๆช่ฟๅ ฅ useScrollDrag ็โๆไฝๆๅจๆปๅจโ็ถๆ๏ผๆช่งฆๅ mousedownโmousemove๏ผ๏ผๅณไฝฟๆฒกๆๆญคๆฌกไฟฎๅค๏ผๆๅ้ฃๆฌก mousemove ้ๅธธไนไธไผ่งฆๅๆปๅจ๏ผๅ ๆญค่ฏฅๆต่ฏๅฏ่ฝๅจๆช่ฆ็็ๅฎ็ผบ้ท้พ่ทฏ็ๆ ๅตไธ้่ฟใ
ๅปบ่ฎฎๅจ dragStart ไนๅๅ ็จ mousedown + mousemove ่งฆๅๅฑ็คบไธญ็ๆปๅจ๏ผๅนถ้่ฟๆจ่ฟ่ฎกๆถๅจ่ฎฉ onScroll ่ณๅฐ่ฐ็จไธๆฌก๏ผ๏ผ็ถๅๅจ dragStart/dragEnd ๆธ ็ๅๅๆฌก mousemove๏ผๆญ่จ onScroll ่ฎกๆฐไธๅๅขๅ ๏ผ่ฟๆ ทๆ่ฝ้ช่ฏโ้ๆพๅไธไผ็ปง็ปญๆปๅจโ็ๅ ณ้ฎ่กไธบใ
ๅฏๅ่ๅฆไธ่กฅไธไปฅๅขๅผบ็จไพ็ๅฎๆดๆงไธ็จณๅฎๆง๏ผ
-it('should not scroll after drop table text', () => {
+it('should not scroll after dropping selected list text', () => {
@@
- const onScroll = jest.fn();
+ const onScroll = jest.fn();
const onDragStart = jest.fn();
const onDragEnd = jest.fn();
document.addEventListener('dragstart', onDragStart);
document.addEventListener('dragend', onDragEnd);
@@
- {({ id }) => <li className="fixed-item">{id}</li>}
+ {({ id }) => <li className="fixed-item">{id}</li>}
</List>,
);
const fixedItems = container.querySelectorAll('.fixed-item');
const targetItem = fixedItems[0];
if (targetItem) {
selectElementText(targetItem);
}
- const listHolder = container.querySelector('.rc-virtual-list-holder');
+ const listHolder = container.querySelector('.rc-virtual-list-holder');
if (targetItem && listHolder) {
- selectElementText(targetItem);
+ // ๅ
่ฟๅ
ฅโๆไฝๆๆฝๆปๅจโ็ถๆ๏ผ็กฎไฟ onScroll ่ณๅฐ่งฆๅไธๆฌก
+ fireEvent.mouseDown(targetItem, { button: 0 });
+ const preMove = createEvent.mouseMove(targetItem);
+ preMove.pageY = 100;
+ fireEvent(targetItem, preMove);
+ act(() => {
+ jest.advanceTimersByTime(100);
+ });
+ expect(onScroll).toHaveBeenCalled();
+ const scrollCallCountBeforeDrop = onScroll.mock.calls.length;
fireEvent.dragStart(targetItem, { bubbles: true });
const rect = listHolder.getBoundingClientRect();
fireEvent.dragOver(listHolder, {
clientY: rect.bottom + 10,
bubbles: true,
});
fireEvent.drop(listHolder, {
clientY: rect.bottom + 10,
bubbles: true,
});
fireEvent.dragEnd(targetItem, { bubbles: true });
- }
- expect(onScroll).not.toHaveBeenCalled();
+ // drag ็ปๆๅๅๆฌก็งปๅจ้ผ ๆ ๏ผไธๅบๅ็ปง็ปญ่งฆๅๆปๅจ
+ const afterRect = listHolder.getBoundingClientRect();
+ const mouseMoveEvent = new MouseEvent('mousemove', {
+ bubbles: true,
+ clientY: afterRect.top - 10,
+ });
+ listHolder.dispatchEvent(mouseMoveEvent);
+ act(() => {
+ jest.advanceTimersByTime(100);
+ });
+ expect(onScroll.mock.calls.length).toBe(scrollCallCountBeforeDrop);
+ }
+ // ไปไฟๆๅฏน drag ไบไปถๆฏๅฆ่งฆๅ็ๆ ก้ช
+ expect(onScroll).toHaveBeenCalled();
expect(onDragStart).toHaveBeenCalled();
expect(onDragEnd).toHaveBeenCalled();
- if (listHolder) {
- const rect = listHolder.getBoundingClientRect();
- const mouseMoveEvent = new MouseEvent('mousemove', {
- bubbles: true,
- clientY: rect.top - 10,
- });
- listHolder.dispatchEvent(mouseMoveEvent);
- }
- expect(onScroll).not.toHaveBeenCalled();
+ // ๆธ
็้ๆฉ
+ const sel = window.getSelection();
+ sel && sel.removeAllRanges();
document.removeEventListener('dragstart', onDragStart);
document.removeEventListener('dragend', onDragEnd);
});๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should not scroll after drop table text', () => { | |
| // Helper function to select text content of an element | |
| const selectElementText = (element) => { | |
| const range = document.createRange(); | |
| range.selectNodeContents(element); | |
| const selection = window.getSelection(); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| }; | |
| const onScroll = jest.fn(); | |
| const onDragStart = jest.fn(); | |
| const onDragEnd = jest.fn(); | |
| document.addEventListener('dragstart', onDragStart); | |
| document.addEventListener('dragend', onDragEnd); | |
| const { container } = render( | |
| <List | |
| component="ul" | |
| itemKey="id" | |
| itemHeight={20} | |
| height={100} | |
| data={genData(200)} | |
| onScroll={onScroll} | |
| > | |
| {({ id }) => <li className="fixed-item">{id}</li>} | |
| </List>, | |
| ); | |
| const fixedItems = container.querySelectorAll('.fixed-item'); | |
| const targetItem = fixedItems[0]; | |
| if (targetItem) { | |
| selectElementText(targetItem); | |
| } | |
| const listHolder = container.querySelector('.rc-virtual-list-holder'); | |
| if (targetItem && listHolder) { | |
| selectElementText(targetItem); | |
| fireEvent.dragStart(targetItem, { bubbles: true }); | |
| const rect = listHolder.getBoundingClientRect(); | |
| fireEvent.dragOver(listHolder, { | |
| clientY: rect.bottom + 10, | |
| bubbles: true, | |
| }); | |
| fireEvent.drop(listHolder, { | |
| clientY: rect.bottom + 10, | |
| bubbles: true, | |
| }); | |
| fireEvent.dragEnd(targetItem, { bubbles: true }); | |
| } | |
| expect(onScroll).not.toHaveBeenCalled(); | |
| expect(onDragStart).toHaveBeenCalled(); | |
| expect(onDragEnd).toHaveBeenCalled(); | |
| if (listHolder) { | |
| const rect = listHolder.getBoundingClientRect(); | |
| const mouseMoveEvent = new MouseEvent('mousemove', { | |
| bubbles: true, | |
| clientY: rect.top - 10, | |
| }); | |
| listHolder.dispatchEvent(mouseMoveEvent); | |
| } | |
| expect(onScroll).not.toHaveBeenCalled(); | |
| it('should not scroll after dropping selected list text', () => { | |
| // Helper function to select text content of an element | |
| const selectElementText = (element) => { | |
| const range = document.createRange(); | |
| range.selectNodeContents(element); | |
| const selection = window.getSelection(); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| }; | |
| const onScroll = jest.fn(); | |
| const onDragStart = jest.fn(); | |
| const onDragEnd = jest.fn(); | |
| document.addEventListener('dragstart', onDragStart); | |
| document.addEventListener('dragend', onDragEnd); | |
| const { container } = render( | |
| <List | |
| component="ul" | |
| itemKey="id" | |
| itemHeight={20} | |
| height={100} | |
| data={genData(200)} | |
| onScroll={onScroll} | |
| > | |
| {({ id }) => <li className="fixed-item">{id}</li>} | |
| </List>, | |
| ); | |
| const fixedItems = container.querySelectorAll('.fixed-item'); | |
| const targetItem = fixedItems[0]; | |
| if (targetItem) { | |
| selectElementText(targetItem); | |
| } | |
| const listHolder = container.querySelector('.rc-virtual-list-holder'); | |
| if (targetItem && listHolder) { | |
| // ๅ ่ฟๅ ฅโๆไฝๆๆฝๆปๅจโ็ถๆ๏ผ็กฎไฟ onScroll ่ณๅฐ่งฆๅไธๆฌก | |
| fireEvent.mouseDown(targetItem, { button: 0 }); | |
| const preMove = createEvent.mouseMove(targetItem); | |
| preMove.pageY = 100; | |
| fireEvent(targetItem, preMove); | |
| act(() => { | |
| jest.advanceTimersByTime(100); | |
| }); | |
| expect(onScroll).toHaveBeenCalled(); | |
| const scrollCallCountBeforeDrop = onScroll.mock.calls.length; | |
| fireEvent.dragStart(targetItem, { bubbles: true }); | |
| const rect = listHolder.getBoundingClientRect(); | |
| fireEvent.dragOver(listHolder, { | |
| clientY: rect.bottom + 10, | |
| bubbles: true, | |
| }); | |
| fireEvent.drop(listHolder, { | |
| clientY: rect.bottom + 10, | |
| bubbles: true, | |
| }); | |
| fireEvent.dragEnd(targetItem, { bubbles: true }); | |
| // drag ็ปๆๅๅๆฌก็งปๅจ้ผ ๆ ๏ผไธๅบๅ็ปง็ปญ่งฆๅๆปๅจ | |
| const afterRect = listHolder.getBoundingClientRect(); | |
| const mouseMoveEvent = new MouseEvent('mousemove', { | |
| bubbles: true, | |
| clientY: afterRect.top - 10, | |
| }); | |
| listHolder.dispatchEvent(mouseMoveEvent); | |
| act(() => { | |
| jest.advanceTimersByTime(100); | |
| }); | |
| expect(onScroll.mock.calls.length).toBe(scrollCallCountBeforeDrop); | |
| } | |
| // ไปไฟๆๅฏน drag ไบไปถๆฏๅฆ่งฆๅ็ๆ ก้ช | |
| expect(onScroll).toHaveBeenCalled(); | |
| expect(onDragStart).toHaveBeenCalled(); | |
| expect(onDragEnd).toHaveBeenCalled(); | |
| // ๆธ ็้ๆฉ | |
| const sel = window.getSelection(); | |
| sel && sel.removeAllRanges(); | |
| document.removeEventListener('dragstart', onDragStart); | |
| document.removeEventListener('dragend', onDragEnd); | |
| }); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
๐ค This is a ...
๐ Related Issues
ant-design/ant-design#53924
ant-design/ant-design#54548
๐ก Background and Solution
้ๆฉๆๆฌ๏ผ้ผ ๆ ๆไฝๆๆฌๆๆฝๅฐ่กจๆ ผๅค่พน๏ผๆพๅผ้ผ ๆ ๏ผ้ผ ๆ ไธไธ็งปๅจไผ่งฆๅ่กจๆ ผๆปๅจ
467032935-9bec2f1c-866f-40d0-be9e-b6e19b6c0be8.mov
rc-virtual-list ็ useScrollDrag hook ็ๅฌ mouseup
๐ Change Log
Summary by CodeRabbit
Bug Fixes
Tests