From ac7f7acc43d51170d74a047d1eea6b747ceffed6 Mon Sep 17 00:00:00 2001 From: Hunter Jozwiak Date: Wed, 11 Nov 2026 09:05:10 -0500 Subject: [PATCH] Fix Wayland internal UI lifecycle edge cases --- docs/testing/command-bypass.md | 26 +- internal/input/commands.go | 11 + internal/input/commands_test.go | 11 + internal/platform/wayland/keyboard.go | 1 + internal/platform/wayland/protocol.go | 43 ++-- internal/platform/wayland/protocol_test.go | 268 ++++++++++++++++++++- internal/platform/wayland/routing.go | 17 +- internal/platform/wayland/routing_test.go | 12 + internal/platform/wayland/ui.go | 15 +- internal/platform/wayland/ui_test.go | 40 +++ internal/review/document_lists.go | 6 +- internal/review/document_lists_test.go | 117 +++++---- 12 files changed, 477 insertions(+), 90 deletions(-) create mode 100644 internal/platform/wayland/ui_test.go diff --git a/docs/testing/command-bypass.md b/docs/testing/command-bypass.md index 22ab5b2..4746d99 100644 --- a/docs/testing/command-bypass.md +++ b/docs/testing/command-bypass.md @@ -22,11 +22,15 @@ review ends on entry. Secure input and XTerm handoff retain their existing precedence. Reader shutdown discards state. The Wayland backend also implements internal UI sessions through the -interface's GrabKeyboard, so the Actions list, browser lists, and the emoji -menu work there. ShowActions captures its session at the chord, Escape -reports and dismisses, and secure-input transitions revoke the session. -Local-only approval input remains unavailable on Wayland because the -protocol cannot distinguish device-backed from synthesized keys. +interface's GrabKeyboard, so the Actions list and browser lists work +there, and the emoji menu can open and navigate. Emoji delivery is +unchanged and has not been verified on Wayland: unchecked insertion still +uses AT-SPI keyboard-event generation and checked copy still uses xclip, +so the README's Xorg scope for emoji insertion remains. ShowActions +captures its session at the chord, Escape reports and dismisses, and +secure-input transitions revoke the session. Local-only approval input +remains unavailable on Wayland because the protocol cannot distinguish +device-backed from synthesized keys. ## Automated checks @@ -75,11 +79,13 @@ physical keyboard input, or physical Braille output. bypass to remain active. If Caps Lock is a configured modifier, press and release it alone during bypass: expect normal lock behavior. Turn it off again before restoring reader commands. -7. On Wayland, repeat steps 2-6 with these differences: in step 2, - Cthulhu+T during bypass must not read the time and is swallowed rather - than reaching the terminal. Also verify Cthulhu+Shift+A opens the - Actions list, arrows and Enter work inside it, and Escape returns keys - to the application. +7. On Wayland, repeat steps 2-5 and the same-application window switch + from step 6, with these differences: in step 2, Cthulhu+T during bypass + must not read the time and is swallowed rather than reaching the + terminal. Also verify Cthulhu+Shift+A opens the Actions list, arrows + and Enter work inside it, and Escape returns keys to the application. + The Caps Lock standalone-lock expectation in step 6 remains an Xorg + check until a compositor establishes it for a watched Caps Lock modifier. Report the exact speech, Braille text, focused control, and application key behavior for any mismatch. Inspect ~/devel/cthulhu-go.log after an unexpected diff --git a/internal/input/commands.go b/internal/input/commands.go index c66c71c..0272159 100644 --- a/internal/input/commands.go +++ b/internal/input/commands.go @@ -484,3 +484,14 @@ func (router *CommandRouter) ReplaceBindings(bindings CommandBindings) { router.bindings[chord] = command } } + +// ReplaceConfiguration updates the modifier set and bindings without losing +// held-key or multi-press state. Grab-only transitions need that continuity +// when capture changes while the initiating chord is still held. +func (router *CommandRouter) ReplaceConfiguration(modifiers []uint32, bindings CommandBindings) { + router.modifiers = make(map[uint32]bool, len(modifiers)) + for _, symbol := range modifiers { + router.modifiers[symbol] = true + } + router.ReplaceBindings(bindings) +} diff --git a/internal/input/commands_test.go b/internal/input/commands_test.go index 936e8e0..1b09214 100644 --- a/internal/input/commands_test.go +++ b/internal/input/commands_test.go @@ -68,6 +68,17 @@ func TestBindingTableRoutesPreferencesOnPress(t *testing.T) { } } +func TestReplaceConfigurationPreservesHeldModifier(t *testing.T) { + router := NewCommandRouter([]uint32{0xff63}, DefaultCommandBindings(DesktopKeyboardLayout)) + if consumed, command := router.Handle(0xff63, 118, 0, true); !consumed || command != "" { + t.Fatalf("modifier press = %v %q", consumed, command) + } + router.ReplaceConfiguration([]uint32{0xff63}, DefaultCommandBindings(DesktopKeyboardLayout)) + if consumed, command := router.Handle('t', 20, 0, true); !consumed || command != PresentTime { + t.Fatalf("held modifier was lost across reconfiguration: %v %q", consumed, command) + } +} + func TestApplicationPreferencesShortcut(t *testing.T) { router := NewCommandRouter([]uint32{0xff63}, DefaultCommandBindings(DesktopKeyboardLayout)) if consumed, command := router.Handle(0xff63, 90, 0, true); !consumed || command != "" { diff --git a/internal/platform/wayland/keyboard.go b/internal/platform/wayland/keyboard.go index 3d8ce3a..3fdd4fc 100644 --- a/internal/platform/wayland/keyboard.go +++ b/internal/platform/wayland/keyboard.go @@ -61,6 +61,7 @@ type Keyboard struct { active atomic.Bool learn atomic.Bool learnEcho atomic.Bool + uiEcho atomic.Bool uiSession atomic.Uint64 uiSequence atomic.Uint64 uiClose atomic.Uint64 diff --git a/internal/platform/wayland/protocol.go b/internal/platform/wayland/protocol.go index 98c03ba..7b67c2b 100644 --- a/internal/platform/wayland/protocol.go +++ b/internal/platform/wayland/protocol.go @@ -65,6 +65,8 @@ type keyboardOwner struct { captured map[uint16]bool learning bool uiSession uint64 + grabbed bool + grabKnown bool started time.Time sequence uint32 generation uint64 @@ -114,22 +116,34 @@ func (owner *keyboardOwner) configure(ctx context.Context, configuration keyboar if err := owner.call(ctx, "SetKeyGrabs", symbols, grabs); err != nil { return err } - grabKeyboard := policy.learn || policy.ui - if grabKeyboard != (owner.policy.learn || owner.policy.ui) { + grabKeyboard := policy.enabled && !policy.secure && (policy.learn || policy.ui) + if !owner.grabKnown || grabKeyboard != owner.grabbed { method := "UngrabKeyboard" if grabKeyboard { method = "GrabKeyboard" } if err := owner.call(ctx, method); err != nil { + // A timeout can mean the compositor applied the transition + // anyway. Keep the applied grab state unknown so the next + // policy sync forces the compositor back to the desired state + // instead of trusting a diff against stale policy. + owner.grabKnown = false return err } + owner.grabbed = grabKeyboard + owner.grabKnown = true } owner.configuration, owner.policy, owner.bindings = configuration, policy, bindings - owner.router = input.NewCommandRouter(symbols, bindings) + if owner.router == nil { + owner.router = input.NewCommandRouter(symbols, bindings) + } else { + owner.router.ReplaceConfiguration(symbols, bindings) + } owner.keyboard.applied.Store(policy.enabled && !policy.secure) owner.keyboard.browseApplied.Store(policy.browse && !policy.bypass && policy.enabled && !policy.secure) owner.keyboard.bypassApplied.Store(policy.bypass && policy.enabled && !policy.secure) owner.keyboard.learnEcho.Store(policy.learn) + owner.keyboard.uiEcho.Store(policy.ui && owner.grabbed) return nil } @@ -139,6 +153,7 @@ func (keyboard *Keyboard) run(ctx context.Context, configuration keyboardConfigu defer keyboard.browseApplied.Store(false) defer keyboard.bypassApplied.Store(false) defer keyboard.uiSession.Store(0) + defer keyboard.uiEcho.Store(false) defer keyboard.active.Store(false) defer keyboard.learn.Store(false) defer keyboard.learnEcho.Store(false) @@ -149,7 +164,7 @@ func (keyboard *Keyboard) run(ctx context.Context, configuration keyboardConfigu return } defer connection.Close() // Also releases grabs if setup only partly succeeded. - owner := &keyboardOwner{keyboard: keyboard, connection: connection, configuration: configuration, + owner := &keyboardOwner{keyboard: keyboard, connection: connection, configuration: configuration, grabKnown: true, held: make(map[uint16]uint32), captured: make(map[uint16]bool), started: time.Now()} deadline, cancel := context.WithTimeout(ctx, callTimeout) err = connection.BusObject().CallWithContext(deadline, "org.freedesktop.DBus.GetNameOwner", 0, managerName).Store(&owner.owner) @@ -237,15 +252,16 @@ func (owner *keyboardOwner) loop(ctx context.Context, queue *signalQueue) error if err := owner.call(ctx, method); err != nil { return err } - } - // A close request revokes the session even if its acknowledgement - // times out waiting for held keys, mirroring the Xorg owner. The - // pending update (if any) then just confirms the ungrab. - if owner.uiSession != 0 && keyboard.uiClose.Load() >= owner.uiSession { - owner.dismiss_internal_ui() - if err := owner.configure(ctx, owner.configuration, owner.requested_policy()); err != nil { + if err := owner.configure(ctx, owner.configuration, policy); err != nil { return err } + } + // A close request revokes the session identity promptly, even if + // its acknowledgement must wait for held keys. The held==0 policy + // sync below retains the compositor grab until those releases + // arrive, mirroring the Escape path and the Xorg owner. + if owner.uiSession != 0 && keyboard.uiClose.Load() >= owner.uiSession { + owner.dismiss_internal_ui() policy = owner.requested_policy() } if !stopping && len(owner.held) == 0 { @@ -266,9 +282,6 @@ func (owner *keyboardOwner) loop(ctx context.Context, queue *signalQueue) error } } update.result <- reply - if reply.err != nil && update.closeSession != 0 { - return reply.err - } } else if pending != nil { update := pending pending = nil @@ -289,7 +302,7 @@ func (owner *keyboardOwner) loop(ctx context.Context, queue *signalQueue) error keyboard.mutex.Unlock() } update.result <- err - } else if policy != owner.policy && (policy.bypass == owner.policy.bypass || len(owner.held) == 0) { + } else if policy != owner.policy || !owner.grabKnown { if err := owner.configure(ctx, owner.configuration, policy); err != nil { return err } diff --git a/internal/platform/wayland/protocol_test.go b/internal/platform/wayland/protocol_test.go index e4ac8d6..0602915 100644 --- a/internal/platform/wayland/protocol_test.go +++ b/internal/platform/wayland/protocol_test.go @@ -26,12 +26,18 @@ type MonitorSnapshot struct { // FakeKeyboardMonitor verifies the real D-Bus wire contract without pretending // to prove physical key suppression by a compositor. type FakeKeyboardMonitor struct { - connection *dbus.Conn - mutex sync.Mutex - state MonitorSnapshot - changes chan MonitorSnapshot - rejectNext bool - rejectWatch bool + connection *dbus.Conn + mutex sync.Mutex + state MonitorSnapshot + changes chan MonitorSnapshot + rejectNext bool + rejectWatch bool + failGrabNext bool + grabDelay time.Duration + grabStarted chan struct{} + grabCalls int + ungrabCalls int + setKeyGrabCalls int } func (monitor *FakeKeyboardMonitor) publish() { @@ -59,15 +65,33 @@ func (monitor *FakeKeyboardMonitor) UnwatchKeyboard() *dbus.Error { return nil } func (monitor *FakeKeyboardMonitor) GrabKeyboard() *dbus.Error { + monitor.mutex.Lock() + monitor.grabCalls++ + failApplied, delay, started := monitor.failGrabNext, monitor.grabDelay, monitor.grabStarted + monitor.failGrabNext = false + monitor.mutex.Unlock() + if started != nil { + select { + case started <- struct{}{}: + default: + } + } + if delay != 0 { + time.Sleep(delay) + } monitor.mutex.Lock() defer monitor.mutex.Unlock() monitor.state.grabbed = true monitor.publish() + if failApplied { + return dbus.NewError("org.freedesktop.DBus.Error.Timeout", []any{"grab applied but timed out"}) + } return nil } func (monitor *FakeKeyboardMonitor) UngrabKeyboard() *dbus.Error { monitor.mutex.Lock() defer monitor.mutex.Unlock() + monitor.ungrabCalls++ monitor.state.grabbed = false monitor.publish() return nil @@ -75,6 +99,7 @@ func (monitor *FakeKeyboardMonitor) UngrabKeyboard() *dbus.Error { func (monitor *FakeKeyboardMonitor) SetKeyGrabs(modifiers []uint32, grabs []keyGrab) *dbus.Error { monitor.mutex.Lock() defer monitor.mutex.Unlock() + monitor.setKeyGrabCalls++ if monitor.rejectNext { monitor.rejectNext = false return dbus.NewError("org.freedesktop.DBus.Error.Failed", []any{"grab update denied"}) @@ -84,6 +109,37 @@ func (monitor *FakeKeyboardMonitor) SetKeyGrabs(modifiers []uint32, grabs []keyG return nil } +func (monitor *FakeKeyboardMonitor) grab_state() (grabbed bool, grabCalls, ungrabCalls, keyGrabs int) { + monitor.mutex.Lock() + defer monitor.mutex.Unlock() + return monitor.state.grabbed, monitor.grabCalls, monitor.ungrabCalls, len(monitor.state.grabs) +} + +func (monitor *FakeKeyboardMonitor) fail_next_grab() { + monitor.mutex.Lock() + defer monitor.mutex.Unlock() + monitor.failGrabNext = true +} + +func (monitor *FakeKeyboardMonitor) delay_next_grab(delay time.Duration) <-chan struct{} { + started := make(chan struct{}, 1) + monitor.mutex.Lock() + defer monitor.mutex.Unlock() + monitor.grabDelay, monitor.grabStarted = delay, started + return started +} + +func wait_for(t *testing.T, check func() bool, message string) { + t.Helper() + deadline := time.Now().Add(3 * time.Second) + for !check() { + if time.Now().After(deadline) { + t.Fatal(message) + } + time.Sleep(time.Millisecond) + } +} + func start_monitor(t *testing.T) *FakeKeyboardMonitor { t.Helper() if os.Getenv("CTHULHU_NATIVE_DBUS_TEST") != "1" { @@ -417,6 +473,17 @@ func TestNativeBypassAndInternalUIGrabLifecycle(t *testing.T) { keyboard := start_keyboard(t, monitor) keyboard.SetBrowseNavigation(true) wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return len(state.grabs) != 0 }) + monitor.key(t, 'h', 43, 0, false) + await_command(t, keyboard, input.DocumentNextHeading) + keyboard.SetCommandBypass(true) + heldDeadline := time.Now().Add(75 * time.Millisecond) + for time.Now().Before(heldDeadline) { + if _, _, _, keyGrabs := monitor.grab_state(); keyGrabs == 0 { + t.Fatal("bypass was applied while a key was held") + } + time.Sleep(time.Millisecond) + } + monitor.key(t, 'h', 43, 0, true) keyboard.SetCommandBypass(true) wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return len(state.grabs) == 0 }) deadline := time.Now().Add(3 * time.Second) @@ -459,3 +526,192 @@ func TestNativeBypassAndInternalUIGrabLifecycle(t *testing.T) { t.Fatal("internal UI session survived end") } } + +func TestNativeInternalUISecureTransitionRevokesSession(t *testing.T) { + monitor := start_monitor(t) + keyboard := start_keyboard(t, monitor) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + session, err := keyboard.BeginInternalUI(ctx) + if err != nil || session == 0 { + t.Fatalf("begin internal UI: session %d, %v", session, err) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.grabbed }) + + keyboard.SetSecureInputActive(true) + if event := await_command(t, keyboard, input.InternalUIDismiss); event.UISession != session { + t.Fatalf("secure dismiss session = %d, want %d", event.UISession, session) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return !state.watching && !state.grabbed }) + if keyboard.InternalUIActive() || keyboard.InternalUISession() != 0 { + t.Fatal("internal UI session survived secure transition") + } + _, grabCalls, _, _ := monitor.grab_state() + if _, err := keyboard.BeginInternalUI(ctx); err == nil { + t.Fatal("internal UI began while secure") + } + if _, afterCalls, _, _ := monitor.grab_state(); afterCalls != grabCalls { + t.Fatal("rejected secure begin still grabbed the keyboard") + } + + keyboard.SetSecureInputActive(false) + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.watching && !state.grabbed }) + session, err = keyboard.BeginInternalUI(ctx) + if err != nil || session == 0 { + t.Fatalf("begin internal UI after unlock: session %d, %v", session, err) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.grabbed }) + monitor.key(t, 0xff1b, 9, 0, false) + event := await_command(t, keyboard, input.InternalUIKey) + if event.UISession != session || event.Chord.Symbol != 0xff1b { + t.Fatalf("internal UI Escape = %+v", event) + } + monitor.key(t, 0xff1b, 9, 0, true) + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return !state.grabbed }) + if err := keyboard.EndInternalUI(ctx, session); err != nil { + t.Fatal(err) + } +} + +func TestNativeInternalUIEndWaitsForHeldKeyRelease(t *testing.T) { + monitor := start_monitor(t) + keyboard := start_keyboard(t, monitor) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + session, err := keyboard.BeginInternalUI(ctx) + if err != nil || session == 0 { + t.Fatalf("begin internal UI: session %d, %v", session, err) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.grabbed }) + monitor.key(t, 0xff0d, 36, 0, false) + await_command(t, keyboard, input.InternalUIKey) + + closed := make(chan error, 1) + go func() { closed <- keyboard.EndInternalUI(ctx, session) }() + wait_for(t, func() bool { return !keyboard.InternalUIActive() }, "session identity was not revoked promptly") + if grabbed, _, _, _ := monitor.grab_state(); !grabbed { + t.Fatal("keyboard was ungrabbed while Enter was held") + } + if !keyboard.InternalUIEchoActive() { + t.Fatal("UI echo suppression ended before held key release") + } + select { + case err := <-closed: + t.Fatalf("EndInternalUI returned before held key release: %v", err) + default: + } + monitor.key(t, 0xff0d, 36, 0, true) + select { + case err := <-closed: + if err != nil { + t.Fatal(err) + } + case <-time.After(3 * time.Second): + t.Fatal("EndInternalUI did not return after key release") + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return !state.grabbed }) + if keyboard.InternalUIEchoActive() { + t.Fatal("UI echo suppression survived ungrab") + } +} + +func TestNativeShowActionsCapturePreservesHeldReaderModifier(t *testing.T) { + monitor := start_monitor(t) + keyboard := start_keyboard(t, monitor) + monitor.key(t, 0xff63, 118, 0, false) + monitor.key(t, 0xffe1, 50, 1, false) + monitor.key(t, 'A', 38, 1, false) + opened := await_command(t, keyboard, input.ShowActions) + if opened.UISession == 0 || opened.UISession != keyboard.InternalUISession() { + t.Fatalf("ShowActions did not capture atomically: %+v", opened) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.grabbed }) + // Insert is still held from the opening chord. Its router state must + // survive the synchronous capture so this cannot become field text. + monitor.key(t, 'x', 53, 1, false) + monitor.key(t, 'x', 53, 1, true) + monitor.key(t, 'A', 38, 1, true) + monitor.key(t, 0xffe1, 50, 0, true) + monitor.key(t, 0xff63, 118, 0, true) + monitor.key(t, 'a', 38, 0, false) + event := await_command(t, keyboard, input.InternalUIKey) + if event.UISession != opened.UISession || event.Text != "a" { + t.Fatalf("first internal UI key = %+v", event) + } + monitor.key(t, 'a', 38, 0, true) + monitor.key(t, 0xff1b, 9, 0, false) + await_command(t, keyboard, input.InternalUIKey) + monitor.key(t, 0xff1b, 9, 0, true) + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return !state.grabbed }) +} + +func TestNativeShowActionsSecureTransitionDoesNotBlockLock(t *testing.T) { + monitor := start_monitor(t) + keyboard := start_keyboard(t, monitor) + started := monitor.delay_next_grab(300 * time.Millisecond) + monitor.key(t, 0xff63, 118, 0, false) + monitor.key(t, 0xffe1, 50, 1, false) + monitor.key(t, 'A', 38, 1, false) + select { + case <-started: + case <-time.After(3 * time.Second): + t.Fatal("GrabKeyboard was not started") + } + secureDone := make(chan struct{}) + go func() { + keyboard.SetSecureInputActive(true) + close(secureDone) + }() + select { + case <-secureDone: + case <-time.After(100 * time.Millisecond): + t.Fatal("secure transition blocked behind ShowActions capture") + } + if event := await_command(t, keyboard, input.InternalUIDismiss); event.UISession == 0 { + t.Fatalf("secure dismiss event = %+v", event) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return !state.watching && !state.grabbed }) + keyboard.SetSecureInputActive(false) + deadline := time.Now().Add(3 * time.Second) + for { + monitor.mutex.Lock() + state := monitor.state + setKeyGrabCalls, snapshotQueue := monitor.setKeyGrabCalls, len(monitor.changes) + monitor.mutex.Unlock() + if state.watching && !state.grabbed { + break + } + if time.Now().After(deadline) { + select { + case err := <-keyboard.Failures(): + t.Fatalf("keyboard did not resume watching after unlock: watching=%v grabbed=%v secure=%v enabled=%v uiSession=%d setKeyGrabCalls=%d snapshotQueue=%d failure=%v", state.watching, state.grabbed, keyboard.secure.Load(), keyboard.enabled.Load(), keyboard.InternalUISession(), setKeyGrabCalls, snapshotQueue, err) + default: + t.Fatalf("keyboard did not resume watching after unlock: watching=%v grabbed=%v secure=%v enabled=%v uiSession=%d setKeyGrabCalls=%d snapshotQueue=%d", state.watching, state.grabbed, keyboard.secure.Load(), keyboard.enabled.Load(), keyboard.InternalUISession(), setKeyGrabCalls, snapshotQueue) + } + } + time.Sleep(time.Millisecond) + } +} + +func TestNativeInternalUIRecoversAmbiguousGrabFailure(t *testing.T) { + monitor := start_monitor(t) + keyboard := start_keyboard(t, monitor) + monitor.fail_next_grab() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + if session, err := keyboard.BeginInternalUI(ctx); err == nil || session != 0 { + t.Fatalf("ambiguous grab begin = session %d, %v", session, err) + } + wait_for(t, func() bool { + grabbed, _, ungrabCalls, _ := monitor.grab_state() + return !grabbed && ungrabCalls != 0 + }, "ambiguous grab was not reconciled with UngrabKeyboard") + session, err := keyboard.BeginInternalUI(ctx) + if err != nil || session == 0 { + t.Fatalf("begin after grab recovery: session %d, %v", session, err) + } + wait_snapshot(t, monitor, func(state MonitorSnapshot) bool { return state.grabbed }) + if err := keyboard.EndInternalUI(ctx, session); err != nil { + t.Fatal(err) + } +} diff --git a/internal/platform/wayland/routing.go b/internal/platform/wayland/routing.go index 7e4c338..ab164c4 100644 --- a/internal/platform/wayland/routing.go +++ b/internal/platform/wayland/routing.go @@ -291,7 +291,22 @@ func (owner *keyboardOwner) handle_key(key keyEvent) error { // Capture exclusively before reporting the command, as on Xorg. // An unavailable modal grab is a failed command, not a failed // reader: session zero makes dispatch announce unavailability. - if session, err := owner.begin_internal_ui(owner.ctx); err == nil { + // Do not hold the secure mutex across compositor IPC; a secure + // transition during the calls retires the just-created session + // instead of reporting a command from before the lock. + generation := owner.generation + keyboard.secureMutex.Unlock() + session, err := owner.begin_internal_ui(owner.ctx) + keyboard.secureMutex.Lock() + if err == nil { + if generation != keyboard.secureGeneration.Load() { + owner.retire_secure_generation() + return nil + } + if !keyboard.enabled.Load() { + owner.dismiss_internal_ui() + return nil + } uiSession = session } } diff --git a/internal/platform/wayland/routing_test.go b/internal/platform/wayland/routing_test.go index 4df0874..9963c59 100644 --- a/internal/platform/wayland/routing_test.go +++ b/internal/platform/wayland/routing_test.go @@ -367,6 +367,18 @@ func TestInternalUIKeyRoutingAndEscapeDismiss(t *testing.T) { t.Fatalf("UI tab event = %+v", event) } send_key(t, owner, 0xff09, 23, 0, true) + send_key(t, owner, 'A', 38, 1, false) + event = next_command(t, owner.keyboard, input.InternalUIKey) + if event.UISession != 7 || event.Text != "A" { + t.Fatalf("UI shifted letter event = %+v", event) + } + send_key(t, owner, 'A', 38, 1, true) + send_key(t, owner, '#', 12, 1, false) + event = next_command(t, owner.keyboard, input.InternalUIKey) + if event.UISession != 7 || event.Text != "#" { + t.Fatalf("UI shifted digit event = %+v", event) + } + send_key(t, owner, '3', 12, 0, true) // Reader chords never become field contents. send_key(t, owner, 0xff63, 118, 0, false) send_key(t, owner, 'x', 53, 0, false) diff --git a/internal/platform/wayland/ui.go b/internal/platform/wayland/ui.go index aef54d5..950d9c9 100644 --- a/internal/platform/wayland/ui.go +++ b/internal/platform/wayland/ui.go @@ -25,11 +25,10 @@ type uiUpdate struct { func (keyboard *Keyboard) InternalUISession() uint64 { return keyboard.uiSession.Load() } func (keyboard *Keyboard) InternalUIActive() bool { return keyboard.uiSession.Load() != 0 } -// InternalUIEchoActive follows the session itself; the owner retains the -// compositor grab until the session's last held key is released and the -// ungrab is applied, so echo suppression never outlives key capture by -// more than one policy cycle. -func (keyboard *Keyboard) InternalUIEchoActive() bool { return keyboard.uiSession.Load() != 0 } +// InternalUIEchoActive follows the applied compositor grab for a UI +// session. It remains active after the session identity is revoked until +// the session's last held key is released and the ungrab is applied. +func (keyboard *Keyboard) InternalUIEchoActive() bool { return keyboard.uiEcho.Load() } // BeginInternalUI acknowledges exclusive capture through the shared // interface's GrabKeyboard. EndInternalUI waits for physical releases, @@ -75,8 +74,6 @@ func (keyboard *Keyboard) update_internal_ui(ctx context.Context, closeSession u select { case result := <-update.result: return result.session, result.err - case <-ctx.Done(): - return 0, ctx.Err() case <-done: return 0, errors.New("Wayland keyboard stopped") } @@ -85,7 +82,7 @@ func (keyboard *Keyboard) update_internal_ui(ctx context.Context, closeSession u // begin_internal_ui runs on the owner goroutine. The compositor grab is // applied before the session is acknowledged, mirroring the Xorg owner. func (owner *keyboardOwner) begin_internal_ui(ctx context.Context) (uint64, error) { - if !owner.policy.enabled || owner.policy.secure || owner.learning || owner.uiSession != 0 { + if !owner.policy.enabled || owner.policy.secure || !owner.keyboard.enabled.Load() || owner.keyboard.secure.Load() || owner.learning || owner.uiSession != 0 { return 0, errors.New("internal UI input unavailable") } policy := owner.policy @@ -141,7 +138,7 @@ func (owner *keyboardOwner) handle_ui_key(key keyEvent, symbol uint32, chord inp // falls back to the keysym's Unicode mapping. Control characters never // become field text. func key_text(key keyEvent, symbol uint32) string { - for _, candidate := range []uint32{key.character, symbol_utf32(symbol), symbol_utf32(key.symbol)} { + for _, candidate := range []uint32{key.character, symbol_utf32(key.symbol), symbol_utf32(symbol)} { if candidate >= 0x20 && candidate != 0x7f && candidate <= 0x10ffff { return string(rune(candidate)) } diff --git a/internal/platform/wayland/ui_test.go b/internal/platform/wayland/ui_test.go new file mode 100644 index 0000000..e631934 --- /dev/null +++ b/internal/platform/wayland/ui_test.go @@ -0,0 +1,40 @@ +// Copyright (c) 2026 Storm Dragon +// Authors: Storm Dragon, Angus Kola +// SPDX-License-Identifier: GPL-3.0-or-later + +package wayland + +import ( + "context" + "testing" +) + +func TestUpdateInternalUIWaitsForSentUpdateResult(t *testing.T) { + keyboard := &Keyboard{ + done: make(chan struct{}), + uiUpdates: make(chan uiUpdate), + } + ctx, cancel := context.WithCancel(context.Background()) + type outcome struct { + session uint64 + err error + } + outcomes := make(chan outcome, 1) + go func() { + session, err := keyboard.update_internal_ui(ctx, 0) + outcomes <- outcome{session: session, err: err} + }() + + update := <-keyboard.uiUpdates + cancel() + select { + case got := <-outcomes: + t.Fatalf("sent update was abandoned on cancellation: %+v", got) + default: + } + update.result <- uiReply{session: 9} + got := <-outcomes + if got.err != nil || got.session != 9 { + t.Fatalf("sent update outcome = %+v", got) + } +} diff --git a/internal/review/document_lists.go b/internal/review/document_lists.go index 96f73d3..800f861 100644 --- a/internal/review/document_lists.go +++ b/internal/review/document_lists.go @@ -172,15 +172,16 @@ func (worker *Worker) listDocument(ctx context.Context, cursor *DocumentCursor, // its own name, else its text, else its children's labels in provider // order. Password text never contributes its text or descendants. func documentNodeLabel(snapshot atspi.Snapshot, childLabels []string) string { - label := snapshot.Name + label := strings.TrimSpace(snapshot.Name) if snapshot.Role != "password text" { if label == "" { - label = snapshot.Text + label = strings.TrimSpace(snapshot.Text) } if label == "" { var parts []string size := 0 for _, part := range childLabels { + part = strings.TrimSpace(part) if part != "" { parts = append(parts, part) size += len(part) @@ -192,6 +193,7 @@ func documentNodeLabel(snapshot atspi.Snapshot, childLabels []string) string { label = strings.Join(parts, " ") } } + label = strings.TrimSpace(label) runes := []rune(label) if len(runes) > 225 { label = string(runes[:225]) diff --git a/internal/review/document_lists_test.go b/internal/review/document_lists_test.go index 709d33b..f106f97 100644 --- a/internal/review/document_lists_test.go +++ b/internal/review/document_lists_test.go @@ -113,53 +113,76 @@ func TestDocumentListDescendantLabelsExcludeHiddenAndPasswordText(t *testing.T) } func TestDocumentClickableMoveNamesUnnamedSectionFromDescendants(t *testing.T) { - node := func(path, role, text string, children ...string) atspi.TreeNode { - snapshot := atspi.Snapshot{BusName: webID(path).BusName, Path: path, Role: role, Text: text} - entry := atspi.TreeNode{Snapshot: snapshot} - for _, child := range children { - entry.Children = append(entry.Children, webID(child)) - } - return entry - } - nodes := []atspi.TreeNode{ - node("/doc", "document web", "", "/start", "/clickable"), - node("/start", "static", "Before"), - node("/clickable", "section", "", "/text", "/hidden", "/password"), - node("/text", "static", "Clickable text"), - node("/hidden", "static", "Hidden content"), - node("/password", "password text", "private fixture password"), - } - nodes[2].Snapshot.SupportsClick = true - nodes[4].Geometry = atspi.Geometry{VisibilityKnown: true} - tree, err := atspi.NewAccessibleTree(webID("/doc"), nodes) - if err != nil { - t.Fatal(err) - } - cursor, err := NewDocumentCursor(tree) - if err != nil { - t.Fatal(err) - } - ctx := context.Background() - if _, err := cursor.SyncFocus(ctx, webID("/start")); err != nil { - t.Fatal(err) - } - move, err := cursor.Move(ctx, DocumentClickable, 0, false) - if err != nil || !move.Moved || move.Position.Object.ID() != webID("/clickable") { - t.Fatalf("clickable move: %+v %v", move, err) - } - if move.Position.Snapshot.Name != "Clickable text" { - t.Fatalf("landed snapshot name = %q, want descendant label", move.Position.Snapshot.Name) - } - ref, err := tree.Reference(webID("/clickable")) - if err != nil { - t.Fatal(err) - } - captured, err := tree.Resolve(ref) - if err != nil { - t.Fatal(err) - } - if captured.Name != "" { - t.Fatalf("captured snapshot mutated: name = %q", captured.Name) + for _, tc := range []struct { + name string + rootName string + rootText string + editable bool + wantName string + }{ + {name: "descendant label", wantName: "Clickable text"}, + {name: "whitespace name and text", rootName: " \t", rootText: " ", wantName: "Clickable text"}, + {name: "editable clickable", editable: true}, + } { + t.Run(tc.name, func(t *testing.T) { + node := func(path, role, text string, children ...string) atspi.TreeNode { + snapshot := atspi.Snapshot{BusName: webID(path).BusName, Path: path, Role: role, Text: text} + entry := atspi.TreeNode{Snapshot: snapshot} + for _, child := range children { + entry.Children = append(entry.Children, webID(child)) + } + return entry + } + nodes := []atspi.TreeNode{ + node("/doc", "document web", "", "/start", "/clickable"), + node("/start", "static", "Before"), + node("/clickable", "section", tc.rootText, "/text", "/hidden", "/password"), + node("/text", "static", "Clickable text"), + node("/hidden", "static", "Hidden content"), + node("/password", "password text", "private fixture password"), + } + nodes[2].Snapshot.Name = tc.rootName + nodes[2].Snapshot.SupportsClick = true + nodes[2].Snapshot.Editable = tc.editable + nodes[4].Geometry = atspi.Geometry{VisibilityKnown: true} + tree, err := atspi.NewAccessibleTree(webID("/doc"), nodes) + if err != nil { + t.Fatal(err) + } + cursor, err := NewDocumentCursor(tree) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + focused, err := cursor.SyncFocus(ctx, webID("/clickable")) + if err != nil { + t.Fatal(err) + } + if focused.Snapshot.Name != tc.rootName { + t.Fatalf("non-structural snapshot name = %q, want captured name %q", focused.Snapshot.Name, tc.rootName) + } + if _, err := cursor.SyncFocus(ctx, webID("/start")); err != nil { + t.Fatal(err) + } + move, err := cursor.Move(ctx, DocumentClickable, 0, false) + if err != nil || !move.Moved || move.Position.Object.ID() != webID("/clickable") { + t.Fatalf("clickable move: %+v %v", move, err) + } + if move.Position.Snapshot.Name != tc.wantName { + t.Fatalf("landed snapshot name = %q, want %q", move.Position.Snapshot.Name, tc.wantName) + } + ref, err := tree.Reference(webID("/clickable")) + if err != nil { + t.Fatal(err) + } + captured, err := tree.Resolve(ref) + if err != nil { + t.Fatal(err) + } + if captured.Name != tc.rootName { + t.Fatalf("captured snapshot mutated: name = %q, want %q", captured.Name, tc.rootName) + } + }) } }