Add focus restoration helper to menu component #3793
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughAdds focus-restoration logic for menu and popover components, introduces a reusable focusTriggerElement utility, and expands end-to-end tests to verify focus behavior across triggers, selections, Escape handling, and nested sub-menus. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (click/keyboard)
participant Trigger as Trigger element
participant Component as Menu/Popover
participant Document as Document/Browser
User->>Trigger: click / keypress (open)
Trigger->>Component: open()
Component-->>Trigger: set aria attributes
Note right of Component: User navigates/selects or presses Escape
User->>Component: select item / press Escape / click outside
Component->>Component: requestCloseAndRestoreFocus() / emit close
Component->>Trigger: (after short delay) focusTrigger()
Trigger->>Document: focused element updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Kiarokh
force-pushed
the
menu-set-focus-back-on-trigger
branch
from
7e22e57 to
e63e8de
Compare
coderabbitai
bot
changed the title
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3793/ |
![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
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.e2e.ts`:
- Around line 141-149: The test "restores focus to the trigger" currently uses a
1ms sleep which is flaky; replace the waitForTimeout(1) with a
wait-for-condition that polls until document.activeElement?.getAttribute('slot')
=== 'trigger' (e.g., use page.waitForFunction or equivalent) and then assert the
slot is 'trigger'; apply the same change to the other focus-restore tests in
this file (the tests that currently use waitForTimeout with tiny delays) so each
waits for the activeElement slot condition instead of relying on a fixed short
timeout.
472a8d7 to
f104322
Compare
Kiarokh
commented
Jan 23, 2026
Focuses the first focusable element inside a trigger element. Supports custom elements by searching both the element's shadow root and its light DOM.
…a selection So the user can easily press Enter to open it again.
…a dismiss e.g. pressing `Esc` button; so the user can easily press Enter to open it again.
… wait-for-condition
Kiarokh
force-pushed
the
menu-set-focus-back-on-trigger
branch
from
78732d2 to
0bc2c1b
Compare
Contributor
Author
|
@coderabbitai so full review again please. Commits have now changed and things are restructured |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/popover/popover.e2e.ts`:
- Around line 3-38: Extract the duplicated polling helpers into a shared test
utilities module: move waitForCondition, waitForActiveElementId, and
waitForPopoverOpenState into a new test-helpers file and export them; update
this popover e2e test to import those functions instead of defining them inline;
do the same for the similar helpers waitForActiveElementSlot and
waitForMenuOpenState from the menu e2e test so both tests import the shared
implementations and share consistent timeout/polling behavior; ensure function
signatures and behavior remain identical and update any import paths in the test
files.
In `@src/components/popover/popover.tsx`:
- Around line 192-207: The focusTrigger implementation duplicates logic found
elsewhere; refactor by extending focusTriggerElement to accept a slot (or add a
new helper like focusTriggerFromSlot) and update this.focusTrigger to call that
helper with this.triggerSlot; reference the existing focusTrigger method, the
triggerSlot property, and focusTriggerElement (or new focusTriggerFromSlot) so
both popover.tsx and menu.tsx can delegate to the shared helper and remove the
assignedElements()/[0] boilerplate.
In `@src/util/focus-trigger-element.ts`:
- Around line 1-2: FOCUSABLE_SELECTOR currently omits contenteditable elements,
so add [contenteditable] to the comma-separated list in the FOCUSABLE_SELECTOR
constant to ensure contenteditable elements are treated as focusable triggers;
keep the existing [tabindex]:not([tabindex="-1"]) clause as-is to continue
matching elements with explicit tabindex values.
Comment on lines
+3
to
+38
| const waitForCondition = async ( | ||
| condition: () => Promise<boolean>, | ||
| errorMessage: string | ||
| ) => { | ||
| const timeoutMs = 2000; | ||
| const start = Date.now(); | ||
|
|
||
| while (Date.now() - start < timeoutMs) { | ||
| if (await condition()) { | ||
| return; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
|
|
||
| throw new Error(errorMessage); | ||
| }; | ||
|
|
||
| const waitForActiveElementId = async (page: any, id: string) => { | ||
| await waitForCondition(async () => { | ||
| const activeId = await page.evaluate(() => document.activeElement?.id); | ||
| return activeId === id; | ||
| }, `Timed out waiting for focus on #${id}`); | ||
| }; | ||
|
|
||
| const waitForPopoverOpenState = async (page: any, isOpen: boolean) => { | ||
| await waitForCondition( | ||
| async () => { | ||
| const open = await page.evaluate(() => { | ||
| const popover = document.querySelector('limel-popover') as any; | ||
| return Boolean(popover?.open); | ||
| }); | ||
| return open === isOpen; | ||
| }, | ||
| `Timed out waiting for popover open=${String(isOpen)}` | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared test utilities to reduce duplication.
The waitForCondition and related helpers here are similar in pattern to waitForActiveElementSlot and waitForMenuOpenState in menu.e2e.ts. Consider extracting these into a shared test utilities file (e.g., src/util/test-helpers.ts) to reduce duplication and ensure consistent timeout/polling behavior across e2e tests.
🤖 Prompt for AI Agents
In `@src/components/popover/popover.e2e.ts` around lines 3 - 38, Extract the
duplicated polling helpers into a shared test utilities module: move
waitForCondition, waitForActiveElementId, and waitForPopoverOpenState into a new
test-helpers file and export them; update this popover e2e test to import those
functions instead of defining them inline; do the same for the similar helpers
waitForActiveElementSlot and waitForMenuOpenState from the menu e2e test so both
tests import the shared implementations and share consistent timeout/polling
behavior; ensure function signatures and behavior remain identical and update
any import paths in the test files.
Comment on lines
+192
to
+207
| private readonly requestCloseAndRestoreFocus = () => { | ||
| this.shouldRestoreFocusOnClose = true; | ||
| this.close.emit(); | ||
| }; | ||
|
|
||
| private readonly setTriggerRef = (elm?: HTMLSlotElement) => { | ||
| this.triggerSlot = elm; | ||
| }; | ||
|
|
||
| private readonly focusTrigger = () => { | ||
| const trigger = this.triggerSlot?.assignedElements()?.[0] as | ||
| | HTMLElement | ||
| | undefined; | ||
|
|
||
| focusTriggerElement(trigger); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating the focusTrigger method pattern.
The focusTrigger method implementation (lines 201-207) is nearly identical to the one in menu.tsx (lines 810-816). While the duplication is minimal, consider whether a higher-order helper or a slight refactor of focusTriggerElement could accept the slot directly to reduce this boilerplate.
💡 Alternative approach
You could extend focusTriggerElement to optionally accept a slot element:
// In focus-trigger-element.ts
export const focusTriggerFromSlot = (slot?: HTMLSlotElement | null): boolean => {
const trigger = slot?.assignedElements()?.[0] as HTMLElement | undefined;
return focusTriggerElement(trigger);
};Then both components could use:
private readonly focusTrigger = () => focusTriggerFromSlot(this.triggerSlot);🤖 Prompt for AI Agents
In `@src/components/popover/popover.tsx` around lines 192 - 207, The focusTrigger
implementation duplicates logic found elsewhere; refactor by extending
focusTriggerElement to accept a slot (or add a new helper like
focusTriggerFromSlot) and update this.focusTrigger to call that helper with
this.triggerSlot; reference the existing focusTrigger method, the triggerSlot
property, and focusTriggerElement (or new focusTriggerFromSlot) so both
popover.tsx and menu.tsx can delegate to the shared helper and remove the
assignedElements()/[0] boilerplate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix https://github.com/Lundalogik/crm-client/issues/646
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: