Skip to content

fix keyboard_listener escape loop - #6719

Open
OwenMcDonnell wants to merge 3 commits into
mainfrom
keyboard-listener-fix
Open

fix keyboard_listener escape loop#6719
OwenMcDonnell wants to merge 3 commits into
mainfrom
keyboard-listener-fix

Conversation

@OwenMcDonnell

@OwenMcDonnell OwenMcDonnell commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description

Fix a bug with keyboard listener. Flutter sends a synthetic key up event for logical escape (command+.) on macOS which creates a loop as flet responds to the event.

Summary by Sourcery

Handle macOS Escape key events without creating a synthetic key-up loop in the keyboard listener.

Bug Fixes:

  • Prevent infinite event loops caused by Flutter's synthetic Escape key-up event for Command+Period on macOS.

Enhancements:

  • Route keyboard events through a Focus widget and introduce escape-specific handling to distinguish physical, synthetic, and Command+Period Escape events.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploying flet-website-v2 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 356b7c5
Status: ✅  Deploy successful!
Preview URL: https://6d864c5f.flet-website-v2.pages.dev
Branch Preview URL: https://keyboard-listener-fix.flet-website-v2.pages.dev

View logs

@FeodorFitsner FeodorFitsner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this - the underlying problem is real, but I think the fix has a regression for ordinary Escape usage on every platform.

I extracted the state machine from this PR into a throwaway flutter test and replayed key sequences through it, so the traces below are actual output rather than a read-through.

Blocking: fast double-Escape drops key_down and emits an unmatched key_up

single escape        -> [key_down:Escape, key_up:Escape]                    OK
fast double escape   -> [key_down:Escape, key_up:Escape, key_up:Escape]     BUG
esc, a, esc          -> [key_down:Escape, key_up:Escape, key_down:A,
                         key_up:A, key_down:Escape, key_up:Escape]          OK

The cause is that _lastSynthesizedEscapeUpAt is set on every physical-Escape key-up, not only synthesized ones:

if (keyEvent is KeyUpEvent && isPhysicalEscape) {
  _lastSynthesizedEscapeUpAt = DateTime.now();   // <- unconditional
  if (keyEvent.synthesized) { ... }
}

so the next block swallows a genuine Escape press within 300 ms of a genuine Escape release. Two consequences:

  1. The app never sees the second key_down, but the fall-through in build still calls _triggerKeyEvent("key_up", ...) unconditionally (only _triggerEscapeKeyUp guards on _escapeDownSent), so Python receives a key_up with no matching key_down.
  2. That swallowed press returns KeyEventResult.handled, so it also stops propagating - a second Escape inside 300 ms won't dismiss a dialog or pop a route.

None of this is gated on macOS, so Windows/Linux/web users get the regression to fix a macOS-only quirk. The esc, a, esc case happens to work only because !isEscape resets the timestamp.

Suggested alternative: symmetric bookkeeping instead of a time window

The root issue is that the control forwards raw KeyEvents with no pairing, so a duplicate or synthesized up is indistinguishable from a real one. Tracking which keys we've actually reported down fixes it generally - no timer, no magic constant, no macOS special-casing, and no change to event propagation:

final Set<LogicalKeyboardKey> _downKeys = {};

// key_down:   emit only if _downKeys.add(keyEvent.logicalKey)
// key_up:     emit only if _downKeys.remove(keyEvent.logicalKey)
// key_repeat: unchanged

For Cmd+. this yields key_down:Escape followed by exactly one key_up:Escape (from the synthesized up, which is semantically correct - the key is released), and any further duplicate ups are dropped. Double-Escape stays intact. It also keeps KeyEventResult.ignored throughout, so KeyboardListener need not be swapped for Focus at all.

If you'd rather keep the current approach, at minimum:

  • only stamp _lastSynthesizedEscapeUpAt when keyEvent.synthesized is true;
  • route the fall-through key_up for Escape through _triggerEscapeKeyUp() so it can't fire unpaired;
  • gate the whole workaround behind defaultTargetPlatform == TargetPlatform.macOS.

Smaller points

  • Stray blank line added at line 1, before the imports. dart analyze in CI only covers the extension packages, so nothing catches it.
  • The Focus swap is behaviorally equivalent to KeyboardListener - I checked the Flutter 3.44.8 source, and KeyboardListener.build is exactly this Focus with an onKeyEvent returning ignored. So the swap itself is safe; what changes semantics is returning handled.
  • _triggerKeyEvent clears _escapeDownSent on repeats: _escapeDownSent = eventName == "key_down" sets it false for a key_repeat of Escape. Held-Escape traces fine today ([key_down, key_repeat, key_up]) because the fall-through up path doesn't consult the flag, but it's an accidental coupling.
  • Hardcoded "Escape" in _triggerEscapeKeyUp duplicates LogicalKeyboardKey.escape.keyLabel; prefer the constant.
  • isMetaUp checks both physical metaLeft/metaRight and three logical variants - redundant, harmless, but worth trimming.
  • No tests. The escape logic is pure state plus input events; if it were pulled into a plain class (as I did to verify this), it would be directly unit-testable without a Control/FletBackend mock. Worth adding given the number of edge cases here.
  • Could you link the upstream Flutter issue and a minimal repro in the description? "Creates a loop" isn't precise enough to tell whether the fix targets the actual mechanism - duplicate synthesized ups, or genuinely unbounded event emission from the engine.

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