fix: close tooltip on Escape - #540
Conversation
|
@nrps9909 is attempting to deploy a commit to the afc163's projects 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 Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughTooltip 统一管理 ChangesTooltip 可见状态与 Escape 关闭
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change adds Escape dismissal for tooltips while preserving controlled visibility behavior, with reported tests and validation checks passing; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Trigger
participant Tooltip
participant TargetWindow
Trigger->>Tooltip: 传入 mergedVisible 和 onInternalVisibleChange
Tooltip->>TargetWindow: 注册 keydown 监听器
TargetWindow-->>Tooltip: 发送 Escape 事件
Tooltip->>Trigger: 请求可见状态变为 false
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
yoyo837
left a comment
yoyo837
left a comment
There was a problem hiding this comment.
A few focused issues around Escape handling and visibility ownership.
| } | ||
| }; | ||
|
|
||
| targetWindow?.addEventListener('keydown', onKeyDown); |
There was a problem hiding this comment.
Trigger/Portal already handles Escape with a topmost-overlay guard. This listener runs for every visible tooltip, so one keypress can close background tooltips and call onVisibleChange(false) twice in controlled mode. Could we reuse Trigger's existing Escape path?
|
|
||
| const targetWindow = triggerRef.current?.nativeElement?.ownerDocument.defaultView; | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') { |
There was a problem hiding this comment.
This bypasses Portal's IME guards (event.isComposing and the post-compositionend lock), so Escape used to cancel CJK composition also closes the tooltip.
| popupAlign={align} | ||
| getPopupContainer={getTooltipContainer} | ||
| onOpenChange={onVisibleChange} | ||
| popupVisible={mergedVisible} |
There was a problem hiding this comment.
Always passing popupVisible forces Trigger into controlled mode. With unique and enter/leave delays, Trigger delays before onOpenChange, then UniqueProvider applies the delay again, effectively doubling it.
| extraProps.popupVisible = props.visible; | ||
| } | ||
| React.useEffect(() => { | ||
| if (!mergedVisible) { |
There was a problem hiding this comment.
While disabled, Trigger hides the popup but preserves its raw open state. This effect still listens because mergedVisible is true, so Escape clears that state and the popup no longer returns when re-enabled.
|
Thank you for catching these ownership and interaction issues. I verified the current Trigger and Portal path already owns Escape ordering, IME guards, controlled-state requests, disabled-state preservation, and UniqueProvider timing. Signed commit a005347 fully restores the upstream implementation; the resulting branch has no diff from the exact base and the focused 27-test file passes. I am closing this PR rather than keeping a redundant listener that would weaken those guarantees. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 36 36
Branches 14 14
=========================================
Hits 36 36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Summary
Escape, matching the ARIA tooltip keyboard interactionfalsethroughonVisibleChangewhile leaving the rendered state under the caller's controlValidation
falsecallback afterEscapepnpm tscgit diff --checkFixes #510.
AI assistance disclosure: Codex was used to trace rc-trigger visibility ownership, construct the exact-base keyboard regression, and run validation. The submitted behavior and test results were verified locally.
Summary by CodeRabbit
新功能
测试