fix(context-menu): context menu positioning#4446
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughMoved ID attributes from cloned target/menu elements into their surrounding wrapper divs in ContextMenu; focus logic now finds the target wrapper by Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
461-474:⚠️ Potential issue | 🟡 MinorStale type cast and test gap in
focusTarget()testTwo issues with this test:
Stale cast (line 470):
document.getElementById(instance.menuTargetID)now returns the wrapper<div>, not the<button>. The castas HTMLButtonElementshould beas HTMLDivElementto reflect the changed DOM structure.Test does not detect the focus regression: Mocking
.focus()on the wrapper div and asserting it was called provesfocusTarget()attempted a focus call, but says nothing about whether focus was actually restored to an interactive element. If/whenfocusTarget()is fixed (see the comment onContextMenu.tsxabove), the test should additionally assert the focusable descendant (button) received focus rather than the wrapper container.💡 Suggested test update (after fixing `focusTarget()`)
describe('focusTarget()', () => { test('should focus the menu target when called', () => { const onFocusTargetSpy = jest.fn(); wrapper = mountToBody( <ContextMenu> <FakeButton /> <FakeMenu /> </ContextMenu>, ); const instance = wrapper.instance() as ContextMenu; - const menuTargetEl = document.getElementById(instance.menuTargetID) as HTMLButtonElement; - menuTargetEl.focus = onFocusTargetSpy; + // The wrapper div contains the button; focusTarget() should focus the button child + const wrapperDiv = document.getElementById(instance.menuTargetID) as HTMLDivElement; + const buttonEl = wrapperDiv.querySelector('button') as HTMLButtonElement; + buttonEl.focus = onFocusTargetSpy; instance.focusTarget(); expect(onFocusTargetSpy).toHaveBeenCalled(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/context-menu/__tests__/ContextMenu.test.tsx` around lines 461 - 474, Update the test for ContextMenu.focusTarget(): change the stale cast of document.getElementById(instance.menuTargetID) from HTMLButtonElement to HTMLDivElement to match the wrapper div, then adjust the assertions so the test spies on the actual focusable descendant (e.g., the FakeButton inside the wrapper) instead of the wrapper container; specifically, locate the wrapper div via instance.menuTargetID, find its focusable descendant (querySelector('button') or the FakeButton element), mock that element's focus and assert it was called (and optionally assert document.activeElement is that button) to ensure focus is restored to the interactive element when focusTarget() runs.src/components/context-menu/ContextMenu.tsx (1)
91-97:⚠️ Potential issue | 🟠 Major
focusTarget()now focuses a non-focusable<div>, breaking keyboard focus restorationMoving
id={this.menuTargetID}from the cloned element to the wrapper<div>meansdocument.getElementById(this.menuTargetID)now returns the outer<div>. Since that<div>has notabIndex, calling.focus()on it is a silent no-op in every browser — keyboard focus is never returned to the trigger element after the menu closes. This breaks WCAG 2.1 SC 2.1.1 and 2.4.3.The existing test in
focusTarget()(line 470) masks this by mocking.focus()on whatever elementgetElementByIdreturns; it does not validate that the element is actually focusable.Fix
focusTarget()to query the first focusable descendant inside the wrapper:♿ Proposed fix
focusTarget = () => { - // breaks encapsulation but the only alternative is passing a ref to an unknown child component - const menuTargetEl = document.getElementById(this.menuTargetID); + const menuTargetWrapper = document.getElementById(this.menuTargetID); + // Find the first focusable descendant; fall back to the wrapper itself (programmatic focus via tabIndex=-1) + const menuTargetEl = + menuTargetWrapper?.querySelector<HTMLElement>( + 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])', + ) ?? menuTargetWrapper; if (menuTargetEl) { menuTargetEl.focus(); } };Also applies to: 173-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/context-menu/ContextMenu.tsx` around lines 91 - 97, focusTarget() currently calls focus() on document.getElementById(this.menuTargetID) which now returns a non-focusable wrapper <div>; update focusTarget to (1) obtain the element with getElementById(this.menuTargetID), (2) if that element is focusable (native focusable tag or has a non-negative tabIndex), call focus() on it, otherwise (3) query the first focusable descendant inside that wrapper using a selector like "a, button, input, textarea, select, [tabindex]:not([tabindex='-1']), [contenteditable='true']" and call focus() on the first match; ensure you still short-circuit if no element found. Apply the same fix to the other occurrence noted (around lines 173-175) and update the tests that mock focus so they assert that a real focusable descendant gets focused rather than stubbing focus() on the wrapper.
🧹 Nitpick comments (1)
src/components/context-menu/ContextMenu.tsx (1)
173-175: Wrapper<div>may disturb flex/grid layout of the trigger elementThe new block-level
<div>becomes the flex/grid item whenContextMenuis used inside a flex or grid container, which can alter the button's sizing/alignment. Considerdisplay: contentson the wrapper div (supported in all modern browsers) to make it a phantom container that doesn't introduce a new layout box, preserving the original rendering behaviour.♻️ Proposed refinement
-<div ref={ref} id={this.menuTargetID}> +<div ref={ref} id={this.menuTargetID} style={{ display: 'contents' }}> {React.cloneElement(menuTarget, menuTargetProps)} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/context-menu/ContextMenu.tsx` around lines 173 - 175, The wrapper <div> around the cloned menu target (the element using ref, id={this.menuTargetID}, and React.cloneElement(menuTarget, menuTargetProps) in ContextMenu.tsx) can become a layout box inside flex/grid parents and change sizing/alignment; change the wrapper to use display: contents (e.g., style or class that sets display: contents) so it becomes a phantom container and does not affect the layout of the menuTarget, while preserving the ref/id/clone behavior (add a fallback approach if display: contents is undesirable on some targets).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/context-menu/__tests__/ContextMenu.test.tsx`:
- Around line 461-474: Update the test for ContextMenu.focusTarget(): change the
stale cast of document.getElementById(instance.menuTargetID) from
HTMLButtonElement to HTMLDivElement to match the wrapper div, then adjust the
assertions so the test spies on the actual focusable descendant (e.g., the
FakeButton inside the wrapper) instead of the wrapper container; specifically,
locate the wrapper div via instance.menuTargetID, find its focusable descendant
(querySelector('button') or the FakeButton element), mock that element's focus
and assert it was called (and optionally assert document.activeElement is that
button) to ensure focus is restored to the interactive element when
focusTarget() runs.
In `@src/components/context-menu/ContextMenu.tsx`:
- Around line 91-97: focusTarget() currently calls focus() on
document.getElementById(this.menuTargetID) which now returns a non-focusable
wrapper <div>; update focusTarget to (1) obtain the element with
getElementById(this.menuTargetID), (2) if that element is focusable (native
focusable tag or has a non-negative tabIndex), call focus() on it, otherwise (3)
query the first focusable descendant inside that wrapper using a selector like
"a, button, input, textarea, select, [tabindex]:not([tabindex='-1']),
[contenteditable='true']" and call focus() on the first match; ensure you still
short-circuit if no element found. Apply the same fix to the other occurrence
noted (around lines 173-175) and update the tests that mock focus so they assert
that a real focusable descendant gets focused rather than stubbing focus() on
the wrapper.
---
Nitpick comments:
In `@src/components/context-menu/ContextMenu.tsx`:
- Around line 173-175: The wrapper <div> around the cloned menu target (the
element using ref, id={this.menuTargetID}, and React.cloneElement(menuTarget,
menuTargetProps) in ContextMenu.tsx) can become a layout box inside flex/grid
parents and change sizing/alignment; change the wrapper to use display: contents
(e.g., style or class that sets display: contents) so it becomes a phantom
container and does not affect the layout of the menuTarget, while preserving the
ref/id/clone behavior (add a fallback approach if display: contents is
undesirable on some targets).
Lindar90
force-pushed
the
context-menu-tether-migration-fix
branch
from
8a4fc81 to
a09a526
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Lindar90
force-pushed
the
context-menu-tether-migration-fix
branch
from
a09a526 to
4692fa1
Compare
Merge Queue StatusRule:
This pull request spent 6 seconds in the queue, with no time running CI. Required conditions to merge
|
5 participants
BEFORE:
AFTER:
The Problem
When react-tether was upgraded from v1 to v3, the API changed to use renderTarget and renderElement render props. The migration added wrapper
The Solution
I moved the id attributes from the inner elements to the wrapper divs (src/components/context-menu/ContextMenu.tsx:174 and :181). Now:
Summary by CodeRabbit
Refactor
Tests