Skip to content

fix: escape propagation from RuiMenu and first-segment focus on an empty picker - #566

Merged
kelsos merged 3 commits into
rotki:mainfrom
kelsos:fix/menu-escape-and-picker-caret
Jul 31, 2026
Merged

fix: escape propagation from RuiMenu and first-segment focus on an empty picker#566
kelsos merged 3 commits into
rotki:mainfrom
kelsos:fix/menu-escape-and-picker-caret

Conversation

@kelsos

@kelsos kelsos commented Jul 31, 2026

Copy link
Copy Markdown
Member

Two fixes found while wiring RuiDateTimePicker into an editor in rotki/rotki.

1. RuiMenu swallowed Escape even while closed

The root wrapper and the teleported popover both bound @keydown.esc.stop. withKeys only runs the handler for Escape, but when it did run, .stop fired regardless of whether the menu was open, so an ancestor listener never saw the key: not on the first press, not on any later one.

onLeave already calls stopPropagation itself, and only when the menu is open, so the fix is to pass it the event and drop .stop. Escape is swallowed while the menu is open (it closes the menu) and forwarded while it is closed.

Measured before the fix, with a keydown listener on an ancestor of the menu:

key menu ancestor saw it (before) (after)
Escape closed no yes
Escape open no no
any other open yes yes

2. Focusing an empty picker put the caret after the format tokens

An untouched field holds no value: the tokens are shown through the placeholder, and formattedDisplay only expands them into the value once the field is focused. That value write lands after handleFocus has run its setSelectionRange, and writing an input's value drops the selection, so the caret ended up at the end (position 16 of DD/MM/YYYY HH:mm) instead of on DD.

A filled field was never affected: it already selects the first segment (0-2). Now an empty one re-selects the first segment once the tokens are actually in the field, so both start in the same place.

Testing

  • RuiMenu.spec.ts: escape forwarded while closed, swallowed while open, other keys always forwarded.
  • RuiDateTimePicker.spec.ts: first segment highlighted on autofocus, for a filled and for an empty field. The empty-field test fails on main (expected 16 to be +0).
  • Full suite: pnpm run test:run (1351 passing), pnpm run typecheck, pnpm run lint with no new warnings.

kelsos added 2 commits July 31, 2026 15:34
The root wrapper and the popover both bound `@keydown.esc.stop`, so every
Escape that passed through a menu had its propagation stopped, whether or
not the menu was open. A consumer that closes its own editor on Escape
therefore never saw the key, not even after the menu had closed.

`onLeave` already stops the event itself, and only when the menu is open,
so passing the event to it and dropping `.stop` gives the intended
behaviour: swallowed while open, forwarded while closed.
Focusing an untouched field expands the format tokens into the input's
value. That write lands after `handleFocus` has selected the first segment
and clears the selection with it, leaving the caret after the tokens
instead of on DD.

Re-select the first segment once the tokens are in the field, so focusing
an empty picker starts on the first segment like a filled one does.
@kelsos
kelsos requested a review from a team as a code owner July 31, 2026 13:37
@codecov-commenter

codecov-commenter commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.05%. Comparing base (63521f3) to head (06a4375).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
+ Coverage   87.03%   87.05%   +0.02%     
==========================================
  Files         159      159              
  Lines        6307     6311       +4     
  Branches     1939     1940       +1     
==========================================
+ Hits         5489     5494       +5     
+ Misses        818      817       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Nothing in the example app opened a menu from inside another one, so the
one-level-at-a-time escape behaviour had no coverage. The new section on
the menus page opens an inner menu from the outer menu's content, and the
e2e test walks both levels back with two Escape presses.
@kelsos
kelsos merged commit 06a4375 into rotki:main Jul 31, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants