feat: add focusTrap prop to preview config#507
feat: add focusTrap prop to preview config#507zombieJ merged 1 commit intoreact-component:masterfrom
Conversation
Add `focusTrap` option to `InternalPreviewConfig` (default `true`). When set to `false`, the preview will not trap focus via `useLockFocus`. This aligns with rc-dialog and rc-drawer which both support controlling focus trapping behavior. The main use case is rendering an always-open preview inline (e.g. for documentation semantic demos), where the global focus lock incorrectly prevents keyboard interaction with the rest of the page.
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
概览此变更向 Preview 组件添加可选的 变更
估计代码审查工作量🎯 2 (Simple) | ⏱️ ~8 分钟 可能相关的 PR
建议的审查者
诗歌
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 17 17
Lines 538 541 +3
Branches 161 165 +4
=======================================
+ Hits 535 538 +3
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a focusTrap property to the Preview component, allowing users to toggle focus trapping behavior, which defaults to true. The changes include updating the useLockFocus hook and adding a test case to verify that focus is not trapped when the property is false. Feedback suggests that for a consistent non-modal experience, other behaviors like the aria-modal attribute and scroll locking should also respect the focusTrap setting.
| }, [open]); | ||
|
|
||
| useLockFocus(open && portalRender, () => wrapperRef.current); | ||
| useLockFocus(focusTrap && open && portalRender, () => wrapperRef.current); |
There was a problem hiding this comment.
While this change correctly disables the focus lock via useLockFocus, the Preview component still maintains other modal-like behaviors that should ideally respect the focusTrap prop for a consistent non-modal experience:
- Accessibility: The
aria-modal="true"attribute (line 450) should be set tofalse(or matchfocusTrap) to correctly inform assistive technologies that the rest of the page remains interactive when the preview is open. - Scroll Locking: The scroll lock logic (line 368) should be bypassed when
focusTrapisfalse, allowing users to scroll the page while the preview is open (especially useful for the "inline" documentation use case mentioned in the PR).
Since these lines are not part of the current diff hunks, they couldn't be included in a code suggestion, but they are important for the completeness of this feature.
Summary
Add
focusTrapoption toInternalPreviewConfig(defaulttrue). When set tofalse, the preview will not trap focus viauseLockFocus.Problem
When a preview is rendered with
open: trueinline (e.g. in antd's semantic documentation demos),useLockFocusactivates a globalfocusinlistener that prevents all other elements on the page from receiving keyboard focus. This breaks keyboard accessibility for the entire page.Solution
Add a
focusTrapprop (consistent withrc-dialogandrc-drawernaming) to allow consumers to opt out of focus trapping:rc-dialog already guards focus lock with
isFixedPosandfocusTrap:rc-drawer uses
focusTrapprop:rc-image previously had no guard:
Changes
InternalPreviewConfig: addfocusTrap?: boolean(defaulttrue)Previewcomponent: passfocusTrapcondition touseLockFocusfocusTrap: falseTest plan
focusTrapisfalsefocusTrap: falseon the semantic demo's always-open previewSummary by CodeRabbit
发布说明
新功能
测试