fix: prevent page scroll when switching images with keyboard #493
Conversation
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
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在 Preview 组件的键盘处理里移除对 KeyCode 的依赖，改为基于 Changes
Sequence Diagram(s)（不生成序列图 — 更改限于单组件内部键盘处理，参与方少于 3 个） Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 分钟 Possibly related PRs
Suggested reviewers
兔兔诗歌
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 @cactuser-Lu, 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! This pull request addresses a user experience issue within the image preview component by preventing default browser actions when specific keyboard keys are pressed. Previously, using keys like 'Escape' or arrow keys for navigation could inadvertently trigger page scrolling. The changes ensure that these keyboard inputs are exclusively handled by the image preview, providing a smoother and more predictable interaction for users. 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. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes the issue of page scrolling when switching images using the keyboard by adding event.preventDefault(). The change is straightforward and effective. I've also added a suggestion to refactor the keyboard event handling logic using a switch statement, which could improve code readability and maintainability. Overall, great job on fixing this bug.
src/Preview/index.tsx
Outdated
| if (keyCode === KeyCode.ESC) { | ||
| event.preventDefault(); | ||
| onClose?.(); | ||
| } | ||
|
|
||
| if (showLeftOrRightSwitches) { | ||
| if (keyCode === KeyCode.LEFT) { | ||
| event.preventDefault(); | ||
| onActive(-1); | ||
| } else if (keyCode === KeyCode.RIGHT) { | ||
| event.preventDefault(); | ||
| onActive(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
While the current implementation is correct, you could refactor this block to use a switch statement. This would make the code more structured, readable, and easier to maintain, especially if more keyboard shortcuts are added in the future. It also helps to reduce some code duplication by grouping the LEFT and RIGHT arrow key handling.
switch (keyCode) {
case KeyCode.ESC:
event.preventDefault();
onClose?.();
break;
case KeyCode.LEFT:
case KeyCode.RIGHT:
if (showLeftOrRightSwitches) {
event.preventDefault();
onActive(keyCode === KeyCode.LEFT ? -1 : 1);
}
break;
default:
break;
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
==========================================
- Coverage 99.41% 99.02% -0.39%
==========================================
Files 17 17
Lines 509 515 +6
Branches 152 155 +3
==========================================
+ Hits 506 510 +4
- Misses 3 5 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where using keyboard shortcuts (ESC, LEFT, RIGHT arrows) in the image preview component would trigger unwanted page scrolling. The fix adds event.preventDefault() calls to the keyboard event handler to prevent default browser behavior while maintaining the intended preview functionality.
Key Changes
- Added
event.preventDefault()for ESC key to prevent default behavior when closing preview - Added
event.preventDefault()for LEFT and RIGHT arrow keys to prevent page scrolling when switching between images
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Preview/index.tsx
Outdated
| event.preventDefault(); | ||
| onClose?.(); | ||
| } | ||
|
|
||
| if (showLeftOrRightSwitches) { | ||
| if (keyCode === KeyCode.LEFT) { | ||
| event.preventDefault(); | ||
| onActive(-1); | ||
| } else if (keyCode === KeyCode.RIGHT) { | ||
| event.preventDefault(); | ||
| onActive(1); |
There was a problem hiding this comment.
The keyboard event handling behavior, including the preventDefault calls, lacks test coverage. Consider adding tests to verify that:
- ESC key closes the preview and prevents default browser behavior
- LEFT/RIGHT arrow keys switch between images when showLeftOrRightSwitches is true
- preventDefault is properly called to prevent page scrolling
This would help ensure the fix for issue #56290 is properly validated and prevent regressions.
|
最好是等我这个pr #494 后再做你这个问题的修复吧，因为涉及到了portal侧的改动 @cactuser-Lu |
![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 (1)
src/Preview/index.tsx (1)
336-342: 建议将preventDefault()调用移到onActive()之前。当前实现在调用
onActive()之后才调用event.preventDefault()。虽然这样可以正常工作（因为onActive只是更新状态），但更好的做法是先调用preventDefault()，这样可以：
- 更早地阻止默认滚动行为
- 使代码意图更清晰
- 符合事件处理的最佳实践
🔎 建议的调整
if (keyCode === KeyCode.LEFT) { + event.preventDefault(); onActive(-1); - event.preventDefault(); } else if (keyCode === KeyCode.RIGHT) { + event.preventDefault(); onActive(1); - event.preventDefault(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/Preview/index.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Preview/index.tsx (1)
335-335: 考虑在执行操作之前调用 preventDefault。将
event.preventDefault()移至onActive()之前可以更清晰地表达意图：先阻止默认行为，再执行自定义操作。虽然当前代码功能正常，但这样的顺序更符合最佳实践。🔎 建议的调整
if (showLeftOrRightSwitches) { if (event.key === 'ArrowLeft') { - onActive(-1); event.preventDefault(); + onActive(-1); } else if (event.key === 'ArrowRight') { - onActive(1); event.preventDefault(); + onActive(1); } }Also applies to: 338-338
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Preview/index.tsx
🔇 Additional comments (1)
src/Preview/index.tsx (1)
330-342: Portal 组件可能不支持onEsc属性。根据对 @rc-component/portal 官方文档的调查，该库的公开 API 仅包含
open、getContainer和autoLock属性，不包括onEsc。代码中使用的onEsc: PortalProps['onEsc']属性可能基于未文档化或不存在的 API，存在以下问题：
- @rc-component/portal 官方文档中无
onEsc属性支持- package.json 中指定的版本 "^2.1.2" 在 npm 注册表中不存在（最新版本为 v2.0.0）
- 当前实现可能依赖于不受支持或不稳定的 API
建议检查是否需要在组件内部自行处理 ESC 键事件，而不是依赖 Portal 组件提供此功能。
Likely an incorrect or invalid review comment.
| if (event.key === 'ArrowLeft') { | ||
| onActive(-1); | ||
| } else if (keyCode === KeyCode.RIGHT) { | ||
| event.preventDefault(); | ||
| } else if (event.key === 'ArrowRight') { | ||
| onActive(1); | ||
| event.preventDefault(); | ||
| } |
There was a problem hiding this comment.
在可编辑元素中时，应跳过方向键处理。
当前实现在按下方向键时总是会切换图片，即使用户正在输入框、文本域或其他可编辑元素中输入内容。这会导致用户在编辑文本时意外触发图片切换，影响用户体验。
建议在处理方向键之前检查事件目标是否为可编辑元素：
🔎 建议的修复方案
在文件顶部添加辅助函数：
+const isEditableTarget = (target: EventTarget | null): boolean => {
+ if (!target || !(target instanceof Element)) return false;
+ const tagName = target.tagName;
+ if (tagName === 'INPUT' || tagName === 'TEXTAREA' || tagName === 'SELECT') {
+ return true;
+ }
+ if (target instanceof HTMLElement && target.isContentEditable) {
+ return true;
+ }
+ return false;
+};
+
const Preview: React.FC<PreviewProps> = props => {然后在键盘事件处理中使用：
const onKeyDown = useEvent((event: KeyboardEvent) => {
if (open) {
+ if (isEditableTarget(event.target)) {
+ return;
+ }
if (showLeftOrRightSwitches) {
if (event.key === 'ArrowLeft') {
onActive(-1);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Preview/index.tsx around lines 333 to 339, the ArrowLeft/ArrowRight
handler always changes images even when the user is focused in an editable
element; add a small helper (e.g., isEditableTarget) near the top of the file
that returns true for INPUT, TEXTAREA, elements with contentEditable="true" (and
optionally selects, or elements with role="textbox"), and update the keyboard
handler to short-circuit and do nothing when isEditableTarget(event.target) is
true before calling onActive and preventDefault.
3 participants
fix: ant-design/ant-design#56290
Summary by CodeRabbit
发布说明
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.