Repository navigation
fix(app): keep desktop submenus open across parent items - #59625
chemarma28 wants to merge 4 commits into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the PR. It helps, but it doesn't fully fix #59613 yet.
I ran the issue's Playwright reproduction on the Windows menu test page in Chromium, on the base and with this branch. On the base, moving diagonally from File to its last item closes the File submenu, and your new regression test fails, which is good. With this branch, the same diagonal works when the pointer moves fast (about 70–160 ms). At a normal hand speed (about 400 ms, the issue's default), it still fails. The pointer rests on View for about 60 ms on its way, Kobalte opens View's submenu, it replaces File's, and Close Window is never highlighted. The change makes Kobalte's close requests wait, but it doesn't stop a sibling from opening while the pointer is heading into the open submenu. Details are inline.
In my runs a slow, deliberate hover onto a sibling still switched submenus, with only one submenu open. A quick keyboard check failed once in three runs: after ArrowDown then ArrowRight, Edit's submenu didn't open. I haven't confirmed whether that comes from this change or from my test, so it's worth a quick check, since closing and opening now also depend on the new lastInput flag.
Public API: unchanged.
The new e2e test moves the pointer with no pauses, so it passes even though a realistic move still fails (see inline).
The screenshots show a fast move on the base and with this branch, then a move at the issue's default speed (about 400 ms) with this branch, where View's submenu has replaced File's.
| <DesktopMenuSubmenu | ||
| label={language.t(menu.labelKey)} | ||
| open={activeSubmenu() === menu.id} | ||
| onOpenChange={(open) => (open ? openSubmenu(menu.id) : requestSubmenuClose(menu.id))} |
There was a problem hiding this comment.
When the pointer stays on a sibling for Kobalte's open delay, Kobalte still calls onOpenChange(true) for it. openSubmenu then switches activeSubmenu to that sibling, even while the pointer is heading into the open submenu. Using the issue's diagonal path at about 400 ms (25 steps × 16 ms), aria-expanded moves from File to View after about 60 ms over View, and the last File item is never reached.
To fix #59613, the open request also needs a check: ignore it while the pointer is inside the triangle from the trigger to the open submenu (or moving toward it), and accept it once the pointer settles on the sibling. Then a deliberate hover onto a sibling still switches.
There was a problem hiding this comment.
Added an intent-area check plus a short settle delay on the sibling.
| const y = last.y + last.height / 2 | ||
|
|
||
| await page.mouse.move(x, trigger.y + trigger.height / 2) | ||
| await page.mouse.move(x, y, { steps: 10 }) |
There was a problem hiding this comment.
mouse.move(..., { steps: 10 }) with no waits finishes in a few milliseconds, faster than Kobalte's sibling-open delay, so this test passes even though a move at normal speed still fails. Please make it follow the issue's path and speed: one straight diagonal from the trigger to the last item, about 16 ms per step. Then it catches the case that's still broken. Keep the closing check for when the pointer leaves the menu.
There was a problem hiding this comment.
The e2e now uses ~400 ms diagonal path and also checks a deliberate sibling hover.
There was a problem hiding this comment.
The new commit only merges dev into the branch. The changes to windows-menu.tsx and the e2e test are the same as before, so both earlier points are still open:
- A sibling's submenu still opens when the pointer moves diagonally toward the open submenu at a normal speed.
- The regression test moves the pointer with no pauses between steps, so it doesn't catch that case.
Public API: unchanged.
f6ec957 to
dd3e23d
Compare
dd3e23d to
3c7bda7
Compare
There was a problem hiding this comment.
Thanks, this now fixes #59613 in my runs, and both earlier points are addressed.
On the base, the issue's Playwright reproduction (a straight diagonal from File to its last item, 25 steps × 16 ms) closes the File submenu, and the new regression test fails. With this branch, the same reproduction passes at about 70, 400 and 600 ms moves. A deliberate hover onto a sibling still switches submenus, arrow keys and Escape still work, and the regression test passes 3/3. tsc -b passes. Public API: unchanged.
Not blocking: this safe-path check lives only in windows-menu.tsx. Other menus built on the shared Menu.Sub, such as the new branch and path submenus in session-identity-header.tsx, don't get it. If the same problem shows up there, the shared menu component in packages/ui may be a better place for it.
Issue for this PR
Closes #59613
Type of change
What does this PR do?
The desktop titlebar menu uses Kobalte dropdown submenus. Kobalte closes a submenu when focus moves to another parent item, and its pointer grace area only covers a direct path into the submenu. Moving down the parent menu to the last item therefore closed the submenu before the pointer reached it.
The submenu now stays open while the pointer is over the parent menu or the submenu, closes 300 ms after the pointer leaves both.
How did you verify your code works?
packages/app/e2e/regression/tabs.spec.tsusing the existing Windows menu fixture. It failed before the fix and passes after.bun run check,tsc -b, and oxlint pass.bun run dev:desktopon Windows in the actual Electron app: the submenu stays open while the pointer moves from its trigger to the last item, switches correctly when hovering another parent item.Screenshots / recordings
submenu-close-grace-fix.mp4
Checklist