Repository navigation
fix(menu): keep a click on a menu item inside its own panel - #8685
talissoncosta wants to merge 1 commit into
Conversation
A menu portalled to the body is not a DOM descendant of whatever opened it, so a click on an item reached the outside-click watchers and closed the panel on the very item you picked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds the Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The dropdown containment change is ready to merge after normal checks.
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 |
github-actions
Bot
added
front-end
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21246 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #21246 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21246 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21246 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression20 screenshots compared. See report for details. |
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
| const node = ref.current | ||
| if (!active || !node) return | ||
| const stop = (e: Event) => e.stopPropagation() | ||
| node.addEventListener('mouseup', stop) |
There was a problem hiding this comment.
🟠 Major · ⚡ Quick win
Add a regression test for the containment contract.
Observed: This hook changes mouseup and touchend propagation, but neither it nor DropdownMenu has automated coverage. Predicted: A later change to the portal, event type, or listener phase could reintroduce the modal-closing bug without CI detecting it. Cover an InlineModal with a portalled DropdownMenu, asserting a menu selection keeps the modal open while a real outside click closes it.
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The containment itself is aligned with the document-level outside-click listener and completed CI checks are successful or neutral. The new propagation contract has no regression test, so the modal-closing case this change is intended to prevent can silently return.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Automate: Keep the modal-plus-portalled-menu interaction in the unit suite. Product take: This is useful groundwork for the accessible tag panel, but its user-facing benefit depends on an event-ordering edge case that needs a durable test. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A small portal tether, but it needs a regression test before it becomes a safety rope · reviewed at a8455cf |
|
Closing. This is not a bug on The failure needs one panel's outside-click watcher wrapping a different component's portalled menu. Every component on useOutsideClick(dropDownRef, onOutsideClick)
return createPortal(<div ref={dropDownRef} …>, …)Watcher and content are the same subtree, so a click inside is never outside. No nesting, nothing to fix. The only place the condition arises is the new tag panel, which puts a #8613 listed this under "pre-existing bugs fixed on the way", which was wrong: the panel introduces the condition and fixes it in the same change. |
This branch was successfully deployed
2 participants
docs/if required so people know about the feature.Changes
Contributes to #8465
1/5, splitting #8613 into reviewable pieces.
A menu portalled to the body is not a DOM descendant of whatever opened it, so a click on one of its items reaches the outside-click watcher of the panel it was opened from, and that panel closes on the very item you picked.
useContainClicksstops the click at the menu.Native listeners rather than React's
onMouseUp, because those sit belowdocument, which is whereuseOutsideClicklistens.No panel on
maincurrently holds aDropdownMenu, so there is nothing to reproduce today. This lands ahead of the tag panel, which does.How did you test this code?
Typecheck at main's baseline, 916 errors both sides.
Regression only. Each of the five
DropdownMenucall sites should still open, fire an item, and close on both Escape and an outside click: