fix: prevent scrolling to top when clicking node#1018
Conversation
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough引入鼠标交互追踪（ Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tree as Tree Component
participant NodeList as NodeList / VirtualList
participant External as External Callback
User->>NodeList: mouseDown on node
NodeList->>Tree: onMouseDown event propagated
Tree->>Tree: set focusedByMouse = true (start raf timer)
Tree->>External: call props.onMouseDown?()
Note over Tree: If focus event occurs while focusedByMouse === true\nonFocus ignores activate-like behavior
User->>NodeList: mouseUp on node
NodeList->>Tree: onMouseUp event propagated
Tree->>External: call props.onMouseUp?()
Tree->>Tree: onBlur -> focusedByMouse = false (clear timer)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求修复了一个在点击树节点时可能导致树滚动到顶部的错误。该问题源于之前对树的可访问性重构，当树没有选中节点时， Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1018 +/- ##
=======================================
Coverage 99.84% 99.84%
=======================================
Files 14 14
Lines 1293 1303 +10
Branches 396 396
=======================================
+ Hits 1291 1301 +10
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Tree.spec.tsx (1)
1415-1433: 测试逻辑正确，建议添加 spy 清理。测试用例正确模拟了问题场景：鼠标点击触发的 focus 不应引发 scrollTo。建议在测试结束时恢复 spy，避免潜在的测试污染：
♻️ 建议添加 spy 清理
// Simulate pointer focus without existing selectedKeys fireEvent.mouseDown(treeContainer); fireEvent.focus(treeContainer); expect(scrollToSpy).not.toHaveBeenCalled(); + + scrollToSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Tree.spec.tsx` around lines 1415 - 1433, The test creates a spy on treeRef.current.scrollTo (stored as scrollToSpy) but never restores it, which can leak into other tests; update the test file (tests/Tree.spec.tsx) to restore the spy after use—either call scrollToSpy.mockRestore() at the end of this it block or add a global afterEach that calls jest.restoreAllMocks()—so that the scrollTo spy is cleaned up and does not affect other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Tree.tsx`:
- Line 1540: Add an explicit onMouseDown prop to the NodeListProps interface to
restore type safety: declare onMouseDown?:
React.MouseEventHandler<HTMLDivElement>; in the NodeListProps definition
(alongside existing onKeyDown/onFocus/onBlur), and ensure it is forwarded via
domProps to VirtualList where onMouseDown={this.onMouseDown} is used so the
component and callers get proper typings.
---
Nitpick comments:
In `@tests/Tree.spec.tsx`:
- Around line 1415-1433: The test creates a spy on treeRef.current.scrollTo
(stored as scrollToSpy) but never restores it, which can leak into other tests;
update the test file (tests/Tree.spec.tsx) to restore the spy after use—either
call scrollToSpy.mockRestore() at the end of this it block or add a global
afterEach that calls jest.restoreAllMocks()—so that the scrollTo spy is cleaned
up and does not affect other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d482dc13-e90f-4ca5-943c-890a53bfd16f
📒 Files selected for processing (2)
src/Tree.tsxtests/Tree.spec.tsx
| it('should not scroll to top when click node and tree is focused', () => { | ||
| jest.useFakeTimers(); | ||
| const data = [ | ||
| { key: '0', title: '0' }, | ||
| { key: '1', title: '1' }, | ||
| { key: '2', title: '2' }, | ||
| ]; | ||
| const treeRef = React.createRef<any>(); | ||
| const { container } = render(<Tree ref={treeRef} treeData={data} />); | ||
| const treeContainer = container.querySelector('.rc-tree-list'); | ||
| const scrollToSpy = jest.spyOn(treeRef.current, 'scrollTo'); | ||
|
|
||
| // Simulate pointer focus without existing selectedKeys | ||
| fireEvent.mouseDown(treeContainer); | ||
| expect(treeRef.current.focusedByMouse).toBe(true); | ||
| fireEvent.focus(treeContainer); | ||
|
|
||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
|
|
||
| expect(treeRef.current.focusedByMouse).toBe(false); | ||
| expect(scrollToSpy).not.toHaveBeenCalled(); | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
jsdom 里模拟不了真实滚动行为，所以这里的 case 测了内部的实现细节
| const { activeKey, selectedKeys, flattenNodes } = this.state; | ||
|
|
||
| if (!disabled && activeKey === null) { | ||
| if (!this.focusedByMouse && !disabled && activeKey === null) { |
There was a problem hiding this comment.
如果点击事件区分才执行下面逻辑的话，看起来下面逻辑就是不充分的。那岂不也可以直接去掉交给点击和键盘自行操作么
… raf Made-with: Cursor
|
同意用
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/examples/basic.jsx (1)
103-109: 建议把回归场景放到独立示例里，不要改写simple示例语义。这里给第一个
Tree直接加height={150}，会把“simple”示例也切成虚拟滚动版本；但它仍然带着defaultSelectedKeys，并不对应#1017的“初始无选中节点”前提。更稳妥的是保留这个基础示例不变，另外新增一个专门的 virtual/no-selected 示例来承载这次回归场景。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples/basic.jsx` around lines 103 - 109, The first Tree in the "simple" example was changed to use height={150}, turning it into a virtualized tree while still carrying defaultSelectedKeys which violates the regression test premise; revert the height={150} addition in the existing Tree (the one using ref={this.setTreeRef}, className="myCls", showLine, checkable, defaultExpandAll, and any defaultSelectedKeys) to preserve the original "simple" semantics, and instead add a new dedicated example (e.g., virtual-no-selected) that renders a Tree with height={150} (and omits defaultSelectedKeys) to reproduce the regression scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/examples/basic.jsx`:
- Around line 103-109: The first Tree in the "simple" example was changed to use
height={150}, turning it into a virtualized tree while still carrying
defaultSelectedKeys which violates the regression test premise; revert the
height={150} addition in the existing Tree (the one using ref={this.setTreeRef},
className="myCls", showLine, checkable, defaultExpandAll, and any
defaultSelectedKeys) to preserve the original "simple" semantics, and instead
add a new dedicated example (e.g., virtual-no-selected) that renders a Tree with
height={150} (and omits defaultSelectedKeys) to reproduce the regression
scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d649eee-f814-407b-b7e9-ccb1c2ff6a3a
📒 Files selected for processing (4)
docs/examples/basic.jsxsrc/NodeList.tsxsrc/Tree.tsxtests/Tree.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Tree.spec.tsx
- src/Tree.tsx
|
这 cursor 怎么未经允许擅自 commit 并且评论…🌚 |
之前在这个 pr 里重构了一下 tree 的 a11y 实现，a11y规范中有个约定就是：tree 接收到焦点时，focus ring 给到被选中的第一个节点，如果没有被选中的节点则 fallback 到第一个节点。然后这里就出问题了：初始态一棵无选中节点的树 -> 用户点击节点，触发容器 focus ->
onFocus中根据selectedKey判断是否要 fallback，但此时还没触发点击事件，selectedKey为null，于是 fallback 到第一个树节点，onActiveChange会触发scrollTo({ firstNodeKey })-> 滚动到顶部解法就是利用 mousedown 判断一下焦点触发来源，鼠标点击触发的 focus 不走 a11y 焦点规范。
btw,TDD 是对的
update：目前 tree 的 a11y 还有点小缺陷，比如：
不支持 type-ahead，这个不知道中文场景下会不会有特殊 case想起来 antd 本身是支持 search 功能的，type-ahead 在使用的功能性上还是太弱了，而且 w3c 官方对它的实现规范目前其实还有不明确的地方，比如这个 issue。综上，个人感觉 search 是 type-ahead 的 promax 版，所以没必要提供 type-ahead 能力了，鸡肋先 mark 一下，后面有空了一个个补上。都做完了的话 tree 的 a11y 应该基本上是完善了（大概
Summary by CodeRabbit
新功能
问题修复
测试
文档