Repository navigation
feat(server)!: give mouse button events a position, fix X1/X2 and middle-button/hwheel handling - #1769
Conversation
…dle-button/hwheel handling MouseEvent's button variants were position-less, so From<MousePdu> and From<MouseXPdu> silently discarded the wire PDU's x/y whenever a button flag was set (position only survived for a pure Move). Clients that send a tap as a single button PDU with no preceding Move (confirmed: Windows App on iOS/iPadOS, touch mode) click at the stale cursor position rather than where they tapped. Tracked as issue Devolutions#1466. While fixing that, found the same conversion layer has two more bugs, confirmed against MS-RDPBCGR: - From<MousePdu> never checked PointerFlags::MIDDLE_BUTTON_OR_WHEEL or ::HORIZONTAL_WHEEL, so middle-click and horizontal wheel silently fell through to the Move fallback (using position the spec says the server should ignore for wheel events). xrdp's own source has a maintainer comment confirming real clients (mstsc) exercise this: "As mstsc does MOUSE not MOUSEX for horizontal scrolling, PTRFLAGS_HWHEEL must be handled here." - From<MouseXPdu> mapped PointerXFlags::BUTTON1/BUTTON2 to Left/Right. Per MS-RDPBCGR 2.2.8.1.2.2.4 these are "Extended mouse button 1 (also referred to as button 4)" and "...2 (also...button 5)", the X1/X2 side buttons, not primary left/right. MouseRelPdu's own sibling From impl already distinguishes these correctly via XBUTTON1/XBUTTON2, so this was an inconsistency, not a deliberate choice. Checked FreeRDP, xrdp, KDE krdp, and GNOME Remote Desktop: all four handle explicit MOVE flags (never implicit else-fallback), all four handle middle button and both wheel flags, none conflate X1/X2 with primary buttons. This redesign follows that shared shape rather than inventing something new. MouseEvent::Button { x, y, button, pressed } replaces the ten position-less LeftPressed/LeftReleased/.../Button5Released variants, scoped to the three PDU types that actually carry absolute position (MousePdu, MouseXPdu, ainput::MousePdu, which already carried x/y on every event but never applied it to buttons either). MouseRelPdu gets MouseEvent::ButtonRel { button, pressed } instead, since it only has relative deltas, no absolute position exists to give it. Both share a new MouseButton enum (Left/Right/Middle/X1/X2). Added MouseEvent::HorizontalScroll to pair with the existing VerticalScroll. Move detection is now an explicit PointerFlags::MOVE-adjacent check everywhere it can be (MouseXPdu's pointerFlags has no move/wheel bits per spec, so its implicit fallback was already correct). Breaking anyway, so MouseEvent and MouseButton are both #[non_exhaustive]. There's independent, unmerged prior art for the middle-button half of this on branch probakowski/xrdp (commit e924272, ad hoc xrdp-interop debugging, never opened as a PR) that arrived at the same MIDDLE_BUTTON_OR_WHEEL fix.
|
Thanks for running with it, Greg Lamberson (@glamberson) — no problem that you took it; the broader scope you found is exactly why it's better in your hands than a narrow patch from me. Corroborating from the downstream side (macrdp is where #1466 came from): our vendored fix deliberately scoped to LEFT/RIGHT only (the reported iOS touch-tap, a left-click), but our divergence note explicitly flagged that middle-click and the MouseX (X1/X2) buttons have the same latent coord-drop, left unaddressed — so fixing all the button paths uniformly matches what we'd independently seen, and it's the right call over a per-button patch. The two extra bugs you folded in ( Happy to validate the button behavior end-to-end on macrdp (mstsc + Windows App, incl. middle / X-buttons) once the shape settles, if a real-client check helps. |
MouseEvent::Button and MouseEvent::ButtonRel both carry a MouseButton, but the type itself was never re-exported from the crate root, so no external consumer could actually name it to construct or match on those variants. Missed this in the same commit that introduced the type.
MS-RDPBCGR 2.2.8.1.1.3.1.1.7 defines PTRFLAGS_DOWN as the button press occurring at the position produced by applying xDelta/yDelta to the previous position, not a separate move. The button branches of From<MouseRelPdu> were dropping those deltas entirely, so a client combining motion with a button transition in one relative event lost the motion. ButtonRel now carries x/y like RelMove does, populated in all five button branches instead of only the fallback move case.
e9c5b1c
into
Devolutions:master
…e public API) Six upstream commits since the last sync, in crates/ironrdp-server: Devolutions#1242 (typed ServerError, breaking), Devolutions#1417 (rdpeusb server integration), Devolutions#1773 (RDPEI wiring), Devolutions#1737 (RTT baseline), Devolutions#1769 (mouse position, no server.rs footprint), Devolutions#1754 (test relocation, no server.rs footprint). Only one real conflict, in run()'s loop -- the same class as prior syncs: HEAD's preemption race vs upstream's plain accept-loop tail. Resolved by taking HEAD's side (a strict superset). The change that needed real attention, not just conflict resolution: Devolutions#1242 makes run/run_connection/run_connection_with return ServerResult (a new typed error), keeping the anyhow-returning bodies as private _inner methods -- exactly the same wrap-and-rename shape Devolutions#1721 already used for run_connection_with (now a third layer: run_connection_with -> run_connection_with_inner [Devolutions#1242, does the Devolutions#1721 static-channels reset] -> run_connection_inner [the real body, PendingConnection-based here]). Upstream's own accept loop switched to calling run_connection_with_inner directly instead of the now-ServerResult-returning public wrapper, since its `conn`-equivalent needs anyhow::Error for on_disconnected and a consistent type across match arms. This branch's preemption race has the identical two call sites (Entry::Fresh's stream, in both the preempt-enabled race and the plain-queue-behind fallback) that were still calling the public run_connection -- which would have silently started returning the wrong error type (ServerResult vs serve_negotiated's anyhow Result, breaking the match arms' common type) had the merge not been caught by an actual build. Switched both to run_connection_with_inner(stream, TransportTls::Managed), matching upstream's own fix exactly, with a comment explaining why (this is not a stylistic change -- it's required for the code to typecheck at all once the two arms feeding into `conn` must share one Result type). Devolutions#1417 (USB) and Devolutions#1773 (RDPEI)'s server.rs footprint is additive and, for USB, feature-gated (`#[cfg(feature = "usb")]`) so it's a no-op in every build this branch's own verification exercises; Devolutions#1754 relocated the autodetect unit tests out of this crate into ironrdp-testsuite-core, so ironrdp-server's own suite drops from 24 to 14 tests -- confirmed by grep that they still exist there, not lost. 14/14 tests pass in both default and --features egfx (down from 24, accounted for above, not a regression), clippy --all-targets clean in both, fmt clean, full cargo build --workspace green.
Summary
MouseEvent's button variants were position-less, soFrom<MousePdu>andFrom<MouseXPdu>silently discarded the wire PDU's x/y whenever a button flag was set (position only survived for a pure Move). A client that sends a tap as a single button PDU with no preceding Move (confirmed: Windows App on iOS/iPadOS, touch mode) clicks at the stale cursor position rather than where it tapped.From<MousePdu>never checkedMIDDLE_BUTTON_OR_WHEELorHORIZONTAL_WHEEL, so middle-click and horizontal wheel silently fell through to the Move fallback.From<MouseXPdu>mappedPointerXFlags::BUTTON1/BUTTON2to Left/Right, but per 2.2.8.1.2.2.4 these are "Extended mouse button 1 (also referred to as button 4)" and "...2 (...button 5)", the X1/X2 side buttons.MouseRelPdu's own siblingFromimpl already distinguishes these correctly viaXBUTTON1/XBUTTON2, so this was an inconsistency, not a deliberate choice.MouseEvent::Button { x, y, button, pressed }replaces the ten position-lessLeftPressed/LeftReleased/.../Button5Releasedvariants, scoped to the PDU types that actually carry absolute position (MousePdu,MouseXPdu,ainput::MousePdu, which already carried x/y on every event but never applied it to buttons either).MouseRelPdugetsMouseEvent::ButtonRel { button, pressed }instead, since it only has relative deltas. Both share a newMouseButtonenum (Left/Right/Middle/X1/X2). AddedMouseEvent::HorizontalScrollto pair with the existingVerticalScroll. Breaking anyway, soMouseEventandMouseButtonare both#[non_exhaustive].probakowski/xrdp(commite924272, ad hoc xrdp-interop debugging, never opened as a PR) that arrived at the sameMIDDLE_BUTTON_OR_WHEELfix. Crediting it here since I found it while researching this.Validation
cargo xtask check fmt/lints/tests/typos/locksall pass.Notes
Single file,
crates/ironrdp-server/src/handler.rs. Grepped the whole workspace forMouseEvent::usage; this file is the only one that constructs or matches it, no other call sites need updating for this PR. Downstream, I'm updating my ownlamco-rdp-input/lamco-rdp-serverconsumers to the new shape as a follow-up.