fix(input): held modifiers on mouse events + Ctrl+click→Cmd+click remap

[antonmos]

Problem

Synthesized mouse events never carried the held-modifier state. button() and post_move() created their CGEvents but never called set_flags, so a click/drag while a modifier was held arrived with empty modifierFlags:

• a genuine Cmd+click didn't open a link in a new tab,
• Shift+click didn't range-select,
• Ctrl+click wasn't a secondary click.

The keyboard path (key()) has always set_flags for this reason; the mouse path just never did.

Change

• button() / post_move() now stamp the current modifier state onto every mouse event via a new mouse_event_flags() helper.
• Under --map-ctrl-to-cmd, a plain Ctrl+click is delivered as Cmd+click (open link in a new tab) instead of a secondary/context click — the mouse analogue of the existing keyboard Ctrl→Cmd remap. It reuses a shared ctrl_to_cmd_flags() bit-swap (refactored out of post_ctrl_as_cmd), gated identically (Ctrl held, no Cmd/Alt, non-excluded app). The frontmost_is_excluded() check sits behind the cheap Ctrl-held guards, so an ordinary unmodified drag (hundreds of post_move calls/s) never pays for it.

Testing

• cargo build + cargo test input:: (9 passed).
• Live-verified on real Windows App for macOS over RDP: Ctrl+click opens a link in a new tab.
• Note on that client: it forwards the Command/Windows key as an instantaneous tap, not a held modifier, so a genuine Cmd+click can't be made to work server-side there — the Ctrl+click remap is the reliable gesture. (Documented in the quirk note.)

🤖 Generated with Claude Code

[clintcan]

Thanks @antonmos — the underlying diagnosis is right and I hadn't realised this
was broken. macOS genuinely does not fold session modifier state into a
synthesized mouse event, so a Cmd+click or Shift+click over RDP has been
arriving bare this whole time. ax_press_spotlight already clears flags
explicitly for exactly this reason, so the codebase half-knew. The
ctrl_to_cmd_flags extraction is clean and I like that the expensive frontmost
check sits behind the cheap Ctrl-held guards.

I'd like one thing resolved before this merges, plus three smaller fixes.

Blocking: a stuck modifier now costs the user their mouse, and reconnecting doesn't fix it

Before this PR button() ignored self.mods entirely, so a modifier key-up
that never arrived — the classic case being the client losing focus while Ctrl
is held — was a keyboard-only annoyance. With set_flags on the click
(src/input.rs:1404), l_ctrl stuck at true means macOS turns every left
click into a secondary click.

Three things make the blast radius bigger than it first looks:

• synchronize() (src/input.rs:779) reconciles caps_lock and nothing else,
  and the MS-RDPBCGR Synchronize PDU only carries lock keys (Scroll/Num/Caps/
  Kana) anyway — so there is no protocol-level path to reconcile a held Ctrl.
• Nothing resets .mods anywhere in the file.
• inner is a plain field of MacInputHandler, constructed once at
  src/main.rs:2518. Modifier state is process-lifetime, so this survives
  disconnect and reconnect and clears only when macrdp itself restarts.

To be clear, this PR doesn't create the stuck-modifier bug — it removes the
buffer that was hiding it. But "every click is a right-click until you restart
the server" is a bad failure mode for the one channel the user has, and it's
the same shape as #179/#180, where a change made something latent reachable.

The awkward part is that RdpServerInputHandler only has keyboard/mouse,
so there's no natural per-connection hook to reset on. Options I see, in
increasing order of effort: clear mods the first time a connection delivers
input after an idle gap; add a reset seam driven from the server's
connect/disconnect edge; or treat a mouse event with no recent key traffic as
a resync opportunity. Happy with any of them — mostly I want the decision made
deliberately rather than inherited.

Should fix

1. The remap verdict can flip between down and up (src/input.rs:1404).
mouse_event_flags() is evaluated independently for the down and the up, and
the down itself fires update_focus_from_click (src/input.rs:1413), which
refreshes LAST_FOCUS_BUNDLE off-thread. Ctrl+clicking into an excluded app
can therefore post a Cmd-flagged down and a Ctrl-flagged up — a mismatched pair,
and the "excluded apps get a real Ctrl+click" guarantee breaks on the event that
actually drives the context menu. Latching the decision at button-down (a
remapped_buttons set mirroring remapped_keys) and reusing it for the up
fixes this, and also kills the per-move cost below.

2. The swap hits right- and middle-click too (src/input.rs:1404).
set_flags(self.mouse_event_flags()) is unconditional, regardless of which
CGMouseButton is in play, so under --map-ctrl-to-cmd a Ctrl+right-click
becomes Cmd+right-click. The docs in this PR only promise Ctrl+click → Cmd+click
for the primary button. Gating the swap on CGMouseButton::Left would match the
stated intent.

3. scroll() wasn't updated (src/input.rs:1417), but the docs say it was.
docs/features.md now reads "held modifiers now also ride every synthesized
mouse event", and new_scroll_event is posted with no set_flags — so
Shift+scroll (horizontal in most apps) and Cmd+scroll (zoom) still arrive bare.
Either extend the same call to the scroll path or narrow the wording.

Minor

Holding Ctrl while moving makes every motion PDU pay for
frontmost_is_excluded() — two mutex locks, a format!("{entry}.") allocation
per candidate bundle, a debug!, and a synchronous AXUIElementCopy...
round-trip on the input thread whenever LAST_FOCUS_BUNDLE is still None
(i.e. before the first click of a session). The comment at src/input.rs:1049 is
accurate that an unmodified drag never pays, but a Ctrl-held drag does.
Latching per button-down (Should-fix 1) removes this too.

Also: no tests. I realise mouse_event_flags() leans on AX and process globals
so it isn't trivially pure, but if the gating predicate were split out from the
flag construction, the decision table (remap on/off × ctrl × cmd × alt ×
excluded) would unit-test the way map_client_to_display does.

CI is green on all three jobs and the refactor itself is good — this is
really about the modifier-reset question.

[antonmos]

Thanks for the thorough review — all of it landed in c8712e9. Taking the blocking item first since you asked for the decision to be made deliberately.

Modifier reset — decision: both a connection-edge reset AND an idle-gap resync

I went with two of your three options rather than one, because they cover different failure shapes:

• Connection edge (deterministic). capture.rs's per-connection ScreenCaptureUpdates::start now calls input::request_modifier_reset() — the same seam that already resets display_suppressed per connection. So a reconnect is a clean slate; the "survives disconnect and reconnect" property is gone.
• Idle gap (the case that has no disconnect at all). The classic failure — client loses focus mid-session, Ctrl is released where we can't see it — never involves a reconnect, so the edge alone doesn't fix it. resync_modifiers_if_stale (top of keyboard()/mouse()) clears held non-lock modifiers after MACRDP_MODS_RESYNC_IDLE_MS (default 10 s, 0 disables the idle trigger).

The threshold is generous on purpose and the cost asymmetry is the argument: a false clear costs one keystroke (re-press the modifier), a missed one costs every click until restart. Any event — a mouse move included — refreshes the timer, so the only false-positive shape is a genuine 10-second hold with zero input in between. It also posts one FlagsChanged with the emptied set (macOS derives release from the flags diff) and drops any outstanding remapped_keys/click latch. Caps Lock is deliberately preserved — it's a toggle synchronize() owns, and clearing it here would fight that path; there's a test pinning that.

You're right that this PR didn't create the stuck-modifier bug, only removed the buffer hiding it — but I agree "every click is a right-click until you restart the server" is not an acceptable failure mode for the one channel the user has.

Should-fix

1. Latched at button-down. New left_click_remapped (the mouse analogue of remapped_keys). The verdict is decided once on left-down and reused for the drag and the up, so the pair can never disagree — and frontmost_is_excluded() is off the drag path entirely, which also resolves the minor per-motion-cost point.
2. Primary button only. Right/middle carry the real held modifiers; a Ctrl+right-click no longer becomes Cmd+right-click. Docs narrowed to match.
3. scroll() now carries modifiers (Shift+scroll, Cmd+scroll), so the "every synthesized mouse event" wording is now true rather than narrowed. Deliberately no Ctrl→Cmd swap on scroll — Ctrl+scroll is macOS's own screen-zoom accessibility gesture and silently rewriting it would hijack a system binding.

Tests

Split the gate into the pure should_remap_click(remap_on, ctrl, cmd, alt, excluded); click_remap_active gathers the impure inputs and runs the cheap flags through the table before paying for the frontmost lookup (kept as a two-step call so the expensive argument isn't evaluated eagerly). Added:

• mouse_remap_decision_table — the full 32-row table, asserting exactly one row remaps;
• mouse_remap_guards_each_hold_independently — one named assertion per guard;
• clear_non_lock_drops_held_keys_but_preserves_caps_lock.

Also fixed a doc comment my earlier refactor had glued onto the wrong fn. cargo clippy --all-targets -- -D warnings, cargo fmt --check, and the full suite (208 passed) are clean.

One honest caveat: the idle-gap resync is verified by unit test and reasoning, not yet by a live focus-loss repro against a real client.

[clintcan]

Thanks @antonmos — this is a really good response. Two triggers is the right
call, the cost-asymmetry argument for the generous idle threshold is exactly
right, and splitting out should_remap_click with the full decision table is
what I was after — the guard-independence test is a nice addition. Appreciated
the honest caveat about the live repro, too.

The blocker is resolved. I went through c8712e9 and found two small things in
the new code, plus one pre-existing hazard that your new reset turns out to be
the natural fix for. The fixes for #2 and #3 both go in
resync_modifiers_if_stale and #1 is about where the reset is requested from,
so they fit in one follow-up.

1. The "connection edge" reset also fires on every reactivation

request_modifier_reset() is called from ScreenCaptureUpdates::start
(src/capture.rs:1221), and the comment at src/capture.rs:629 says
RdpServerDisplay::updates "runs at connect AND after every
deactivation-reactivation". The chain is updates() → build_updates() (806) →
ScreenCaptureUpdates::start (813) → the reset. So a held modifier is also
cleared on every live resize, and on blank recovery, which fires on its own
with no user action.

The cost is one re-press, so it's small today — but it means "the only
false-positive shape is a genuine 10-second hold" isn't quite true, and it
matters more once #3 lands, since a reactivation in the middle of a drag would
then break the drag.

One thing to avoid: on_accept looks like the obvious per-connection hook, but
it runs on preemption candidates too, while the live connection is still
being served (vendor/ironrdp-server/src/server.rs:516) — so moving the reset
there would let a mere connection attempt clear the live session's modifiers.
The seam you want is once per served connection: the top of run_connection
(server.rs:1567) and serve_negotiated (1547). Each calls accept_finalize
once and reactivations loop inside it, so a signal there never fires for a
reactivation or a losing candidate — and it'd follow the existing set_*_handle
pattern on the server.

2. On reconnect, the previous connection's latch survives

resync_modifiers_if_stale returns early at src/input.rs:955 when
clear_non_lock() reports nothing was held — before remapped_keys.clear()
and left_click_remapped = false at 970–971. So if a connection ends with the
modifier already released but a remapped key-up or the left-up still
outstanding, that state carries straight into the next connection.

3. Pre-existing: a disconnect mid-drag leaves a phantom drag

Not caused by this PR — main has the same button tracking — but your reset is
the right place to fix it. left_down and right_down are only ever written by
a real button event (src/input.rs:1550–1551), they live in the same
process-lifetime handler as mods, and post_move chooses LeftMouseDragged
from left_down (1508).

So if a connection drops between a left-down and its up, every mouse move on the
next connection is posted as LeftMouseDragged until the user next clicks —
plain pointer movement arrives at macOS as a drag. Blank recovery's fallback drop followed by the ARC
auto-reconnect is a plausible way to hit it, and with #2 that phantom drag would
also carry Cmd.

Putting #2 and #3 together

I'd split the reset by trigger:

• Connection edge: clear everything per-connection, unconditionally —
  modifiers, the latch, remapped_keys, and button-down state. Nothing from the
  previous connection is live, so none of it is worth preserving.
• Idle gap: leave it as it is. A click or drag in progress inside a live
  connection is legitimate state. A drag held still sends no events at all, so
  clearing button state here would drop it; and clearing the latch here would
  undo your own down/up fix — Ctrl+down, release Ctrl, hold still past the
  threshold, release, and the up posts plain after a Cmd-flagged down.

Either way, only post the FlagsChanged when a modifier was actually held.

Once these are in I think this is good to go.

[antonmos]

All three in 01e86a8 — and thanks for the pointer to run_connection/serve_negotiated; the reactivation loop living inside accept_finalize was the fact I'd missed.

1. Reset moved to a once-per-served-connection seam

New vendored divergence (24): RdpServer::set_input_reset_handle(Arc<AtomicBool>), raised to true at the top of both run_connection and serve_negotiated, placed right next to divergence 13's auto_reconnect_sent = false — same lifecycle, so it fires exactly once per connection that's actually served and never for a reactivation. capture.rs no longer touches it; main.rs installs the handle via input::modifier_reset_handle(). Deliberately not on_accept, per your warning — it runs for preemption candidates while the live session is still served. Both rejected seams are written up in the divergence entry and the quirk note so nobody re-tries them. Off by default (None), so the upstream-shaped path is byte-identical unless macrdp installs the handle.

2. Latch surviving a reconnect

Right — the early return on clear_non_lock() == false skipped the latch/remapped_keys clears. Restructured: the connection-edge clears are unconditional now, and the FlagsChanged is the only thing gated on "a modifier was actually held".

3. Phantom drag

Took this one step further than clearing the tracking: on the connection edge, any button still down is released — a synthetic Up posted through button() — before the modifiers are cleared. Reasoning: we posted the Down and the connection died before the Up, so macOS itself still believes the button is held; merely zeroing left_down would stop our phantom LeftMouseDragged but leave the OS in a drag, and the user's next real click would post a second Down with no Up in between. Going through button() means the Up carries the same latched flags the Down did (it consumes the latch via mem::take) and the click-count bookkeeping stays paired. Ordered before the modifier clear so the pair can't disagree.

Trigger split

Exactly as you laid out: connection edge clears everything (modifiers, latch, remapped_keys, and releases held buttons); idle gap clears modifiers only — a click/drag in progress inside a live connection is legitimate state, and clearing the latch there would undo the down/up fix. Both cases are spelled out in the fn doc.

clippy --all-targets -- -D warnings, fmt --check, full suite (208 passed) clean. Same live caveat as before: the connection-edge path is verified by construction, not yet by a mid-drag disconnect against a real client.

[clintcan]

Thanks @antonmos — I went through 01e86a8 against the current head and all three hold up. Raising the flag right after auto_reconnect_sent = false and before accept_finalize in both run_connection and serve_negotiated is exactly the right seam, and the unconditional connection-edge clears with only the FlagsChanged gated read well.

Two things, neither a blocker:

1. Where the synthetic button-up lands. resync_modifiers_if_stale runs at the top of both mouse() and keyboard(), before the incoming event updates the cursor, and button() posts at self.last_x/self.last_y. So the release goes out at the pre-disconnect cursor position, on the new connection's first input — a keystroke counts. If the link dropped mid-way through dragging a file in Finder, that release completes the drag as a drop at the old position: into whatever folder was under the cursor, or onto the Trash in the Dock.

I agree the double-press problem you describe is real, and I don't see a clean way out of it: Escape would cancel a drag, but outside one it dismisses dialogs and exits full screen in plenty of apps. So I'd keep your approach, and ask for two things: a line in the quirk note stating that an interrupted drag completes as a drop at the last cursor position, and, when you do the live mid-drag disconnect, drag an actual file over a folder so we see what macOS really does.

2. Tests. 01e86a8 doesn't add any, so the connection-edge path is covered only by construction. The decision part — which buttons get released, the unconditional latch and remapped_keys clear on reconnect, and posting FlagsChanged only when a modifier was held — could be split out pure the way you did should_remap_click, and table-tested.

Numbering: #182 also claims vendored divergence (24), and so does my mic branch. Since this one looks closest to landing, it keeps (24); I'll ask #182 to take (25), and the mic work will take (26).

With the quirk-note line and a test, I think this is good to go. I'll review #184 once this lands, since it's stacked on top.

[clintcan]

Approving. The design holds up and CI is green on the rebased head. The drag-completes-as-a-drop caveat I'll add to the quirk note on main myself; tests for the reconnect reset can come as a follow-up.

[clintcan]

Merged — thanks @antonmos. I added the interrupted-drag caveat to the quirk note on main (b43a047).

One follow-up when you have time: a small test for the reconnect reset — which buttons get released, the latch and remapped_keys cleared on reconnect, and FlagsChanged posted only when a modifier was held — split out pure the way should_remap_click is. And if you get to the live mid-drag disconnect with a real file over a folder, I'd like to note what macOS actually does.