Fix:The dropdown list is not following the highlighted item when navigating through the result using up or down arrow after entering keyword #575
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough本次更新调整了 Changes
Sequence Diagram(s)sequenceDiagram
participant C as Column组件
participant E as useEffect
participant D as document.querySelector
participant S as scrollIntoView
C->>E: activeValue 更新
E->>D: 构造选择器查询 DOM
D-->>E: 返回对应元素 (或 null)
E->>S: 调用 scrollIntoView（若元素存在）
sequenceDiagram
participant T as 测试用例
participant C as Cascader组件
participant M as 活跃菜单项DOM
T->>C: 发送键盘导航事件
C->>M: 激活对应菜单项
M->>M: 调用 scrollIntoView 方法
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/OptionList/Column.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
src/OptionList/List.tsx
Outdated
| @@ -186,9 +186,6 @@ const RawOptionList = React.forwardRef<RefOptionListProps, RawOptionListProps>(( | |||
|
|
|||
| // >>>>> Active Scroll | |||
| React.useEffect(() => { | |||
| if (searchValue) { | |||
There was a problem hiding this comment.
看了下之前的PR：#532
如果searchValue 有值，会阻止滚动，这个行为是不合理的，不应该阻止滚动。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
=======================================
Coverage 99.83% 99.83%
=======================================
Files 23 23
Lines 610 610
Branches 186 186
=======================================
Hits 609 609
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/OptionList/Column.tsx (1)
103-114:⚠️ Potential issue良好的滚动实现，但存在安全问题
这段代码实现了当活跃选项改变时自动滚动到视图中的功能，这对于键盘导航体验来说是一个很好的改进。
但是，我注意到第104行中的字符串转义可能不完整：
const escapedValue = String(activeValue).replace(/"/g, '\\"');仅替换双引号是不够的，如果
activeValue包含其他特殊字符（如\、'、(、)等），可能会导致CSS选择器注入风险。建议使用更全面的转义方法：
-const escapedValue = String(activeValue).replace(/"/g, '\\"'); +const escapedValue = CSS.escape(String(activeValue));
CSS.escape()方法可以正确转义所有CSS选择器中的特殊字符，提供更好的安全保障。
🧹 Nitpick comments (3)
src/OptionList/Column.tsx (3)
103-114: 考虑添加性能优化当前实现在每次
activeValue变化时都会查询整个文档，这在选项很多时可能会影响性能。建议通过以下方式优化：
- 使用组件内的引用来限制查询范围
+ const columnRef = React.useRef<HTMLUListElement>(null); React.useEffect(() => { const escapedValue = String(activeValue).replace(/"/g, '\\"'); const selector = `[data-path-key="${escapedValue}"]`; - const activeElement = document.querySelector<HTMLElement>(selector); + const activeElement = columnRef.current?.querySelector<HTMLElement>(selector); if (activeElement) { activeElement.scrollIntoView({ block: 'nearest', inline: 'nearest', }); } }, [activeValue]); // 在返回的JSX中添加ref return ( - <ul className={menuPrefixCls} role="menu"> + <ul className={menuPrefixCls} role="menu" ref={columnRef}>
- 考虑添加防抖或节流，特别是在快速导航时
103-114: 添加注释说明修复的问题这段代码解决了PR描述中提到的问题，但代码中没有解释这个实现的目的。
建议添加简短注释：
React.useEffect(() => { + // 确保当使用键盘导航时，活跃项会滚动到视图中 const escapedValue = String(activeValue).replace(/"/g, '\\"'); const selector = `[data-path-key="${escapedValue}"]`; const activeElement = document.querySelector<HTMLElement>(selector);
103-114: 考虑添加错误处理当前实现没有处理可能的异常情况，如无效的
activeValue值或DOM操作失败。建议添加简单的错误处理：
React.useEffect(() => { + if (activeValue === undefined || activeValue === null) return; + const escapedValue = String(activeValue).replace(/"/g, '\\"'); const selector = `[data-path-key="${escapedValue}"]`; - const activeElement = document.querySelector<HTMLElement>(selector); + try { + const activeElement = document.querySelector<HTMLElement>(selector); + + if (activeElement) { + activeElement.scrollIntoView({ + block: 'nearest', + inline: 'nearest', + }); + } + } catch (error) { + console.error('Failed to scroll active item into view:', error); + } - if (activeElement) { - activeElement.scrollIntoView({ - block: 'nearest', - inline: 'nearest', - }); - } }, [activeValue]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OptionList/Column.tsx(1 hunks)tests/index.spec.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/index.spec.tsx
src/OptionList/Column.tsx
Outdated
| @@ -100,6 +100,22 @@ export default function Column<OptionType extends DefaultOptionType = DefaultOpt | |||
| [options, checkedSet, fieldNames, halfCheckedSet, loadingKeys, prevValuePath], | |||
| ); | |||
|
|
|||
| React.useEffect(() => { | |||
| const escapedValue = String(activeValue) | |||
| .replace(/\\/g, '\\\\') | |||
There was a problem hiding this comment.
有没有更好的方式来搜索对应的 ele？感觉这个 escape 有点太黑了
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OptionList/Column.tsx (1)
103-112: 建议优化查找活动元素的方法当前实现直接使用
document.querySelector查找活动元素可能存在以下问题：
- 绕过了React的虚拟DOM流程进行直接DOM操作
- 没有处理
menuItemPrefixCls中可能包含的特殊字符- 如果页面上有多个Cascader实例，可能会选中错误的元素
- 在整个文档范围内查询可能影响性能
以下是改进建议：
- React.useEffect(() => { - const activeElement = document.querySelector<HTMLElement>(`.${menuItemPrefixCls}-active`); - - if (activeElement) { - activeElement.scrollIntoView({ - block: 'nearest', - inline: 'nearest', - }); - } - }, [activeValue]); + // 使用ref引用列表容器元素 + const containerRef = React.useRef<HTMLUListElement>(null); + + React.useEffect(() => { + if (!containerRef.current || !activeValue) return; + + // 在容器范围内查找活动元素，避免全局查询 + const activeElement = containerRef.current.querySelector<HTMLElement>( + `.${menuItemPrefixCls}-active` + ); + + if (activeElement) { + activeElement.scrollIntoView({ + block: 'nearest', + inline: 'nearest', + }); + } + }, [activeValue, menuItemPrefixCls]);然后在返回的JSX中添加ref:
- return ( - <ul className={menuPrefixCls} role="menu"> + return ( + <ul ref={containerRef} className={menuPrefixCls} role="menu">这种方法可以:
- 限制查询范围在当前组件内
- 避免与其他Cascader实例冲突
- 提高性能
- 确保使用正确的
menuItemPrefixCls值
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OptionList/Column.tsx(2 hunks)
🔇 Additional comments (2)
src/OptionList/Column.tsx (2)
46-46: 新增 menuRef 用于获取 DOM 元素，逻辑清晰添加对 DOM 节点的引用将有助于实现滚动功能，是实现需求的必要步骤。
120-120: 正确添加了 ref 属性到 ul 元素将 menuRef 正确绑定到 ul 元素上，使得 DOM 操作逻辑能够正常工作。
| React.useEffect(() => { | ||
| if (menuRef.current) { | ||
| const selector = `.${menuItemPrefixCls}-active`; | ||
| const activeElement = menuRef.current.querySelector<HTMLElement>(selector); | ||
|
|
||
| if (activeElement) { | ||
| activeElement.scrollIntoView({ | ||
| block: 'nearest', | ||
| inline: 'nearest', | ||
| }); | ||
| } | ||
| } | ||
| }, [activeValue, menuItemPrefixCls]); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
实现了滚动高亮项到视图功能，但存在潜在安全隐患
通过监听 activeValue 变更并调用 scrollIntoView 的方式实现了下拉列表跟随高亮项的功能，解决了原有的键盘导航问题。
然而，构建选择器时未对 menuItemPrefixCls 进行转义处理，若其中包含 CSS 选择器的特殊字符（如 .、#、:等），可能导致选择器失效或存在潜在的注入风险。
建议按照下面的方式进行修改，使用 data 属性替代类选择器，避免潜在问题：
React.useEffect(() => {
if (menuRef.current) {
- const selector = `.${menuItemPrefixCls}-active`;
- const activeElement = menuRef.current.querySelector<HTMLElement>(selector);
+ const activeElement = menuRef.current.querySelector<HTMLElement>('[data-active="true"]');
if (activeElement) {
activeElement.scrollIntoView({
block: 'nearest',
inline: 'nearest',
});
}
}
}, [activeValue, menuItemPrefixCls]);同时，需要调整 <li> 元素的属性：
<li
key={fullPathKey}
className={classNames(menuItemPrefixCls, {
[`${menuItemPrefixCls}-expand`]: !isMergedLeaf,
[`${menuItemPrefixCls}-active`]:
activeValue === value || activeValue === fullPathKey,
[`${menuItemPrefixCls}-disabled`]: isOptionDisabled(disabled),
[`${menuItemPrefixCls}-loading`]: isLoading,
})}
+ data-active={activeValue === value || activeValue === fullPathKey ? 'true' : undefined}
style={dropdownMenuColumnStyle}
role="menuitemcheckbox"
...关键安全隐患：建议使用 data-active 属性替换基于 menuItemPrefixCls 构造的动态类名选择器
- 当前
useEffect中使用 `.${menuItemPrefixCls}-active` 构造 CSS 选择器直接查找高亮项，这在菜单项的前缀值含有特殊字符（如.、#、:等）时可能导致选择器失效，甚至引发潜在的注入风险。 - 建议改用 data 属性来标识高亮状态，这样既能避免字符串拼接带来的安全隐患，也能提高代码的可维护性和健壮性。
具体建议修改如下：
React.useEffect(() => {
if (menuRef.current) {
- const selector = `.${menuItemPrefixCls}-active`;
- const activeElement = menuRef.current.querySelector<HTMLElement>(selector);
+ const activeElement = menuRef.current.querySelector<HTMLElement>('[data-active="true"]');
if (activeElement) {
activeElement.scrollIntoView({
block: 'nearest',
inline: 'nearest',
});
}
}
}, [activeValue, menuItemPrefixCls]);同时，需要在 <li> 元素上添加 data-active 属性以标识高亮项：
<li
key={fullPathKey}
className={classNames(menuItemPrefixCls, {
[`${menuItemPrefixCls}-expand`]: !isMergedLeaf,
[`${menuItemPrefixCls}-active`]:
activeValue === value || activeValue === fullPathKey,
[`${menuItemPrefixCls}-disabled`]: isOptionDisabled(disabled),
[`${menuItemPrefixCls}-loading`]: isLoading,
})}
+ data-active={activeValue === value || activeValue === fullPathKey ? 'true' : undefined}
style={dropdownMenuColumnStyle}
role="menuitemcheckbox"
...请尽快按照上述建议进行修正，以确保下拉列表在键盘导航时能够安全准确地滚动显示高亮项。
📝 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.
| React.useEffect(() => { | |
| if (menuRef.current) { | |
| const selector = `.${menuItemPrefixCls}-active`; | |
| const activeElement = menuRef.current.querySelector<HTMLElement>(selector); | |
| if (activeElement) { | |
| activeElement.scrollIntoView({ | |
| block: 'nearest', | |
| inline: 'nearest', | |
| }); | |
| } | |
| } | |
| }, [activeValue, menuItemPrefixCls]); | |
| // In src/OptionList/Column.tsx | |
| // Updated useEffect for scrolling active element into view: | |
| React.useEffect(() => { | |
| if (menuRef.current) { | |
| - const selector = `.${menuItemPrefixCls}-active`; | |
| - const activeElement = menuRef.current.querySelector<HTMLElement>(selector); | |
| + const activeElement = menuRef.current.querySelector<HTMLElement>('[data-active="true"]'); | |
| if (activeElement) { | |
| activeElement.scrollIntoView({ | |
| block: 'nearest', | |
| inline: 'nearest', | |
| }); | |
| } | |
| } | |
| }, [activeValue, menuItemPrefixCls]); | |
| // Updated <li> element with data-active attribute: | |
| <li | |
| key={fullPathKey} | |
| className={classNames(menuItemPrefixCls, { | |
| [`${menuItemPrefixCls}-expand`]: !isMergedLeaf, | |
| [`${menuItemPrefixCls}-active`]: | |
| activeValue === value || activeValue === fullPathKey, | |
| [`${menuItemPrefixCls}-disabled`]: isOptionDisabled(disabled), | |
| [`${menuItemPrefixCls}-loading`]: isLoading, | |
| })} | |
| + data-active={activeValue === value || activeValue === fullPathKey ? 'true' : undefined} | |
| style={dropdownMenuColumnStyle} | |
| role="menuitemcheckbox" | |
| ... | |
| > | |
| {/* ... rest of the <li> content ... */} | |
| </li> |
issuse:ant-design/ant-design#53050
Summary by CodeRabbit
新功能
测试
scrollIntoView方法的模拟实现，以便于测试。