From 7752c4e566c04a24567e14be79581a8336218de9 Mon Sep 17 00:00:00 2001 From: Angus Kola Date: Sat, 7 Nov 2026 15:57:33 -0500 Subject: [PATCH] Fix Thunderbird deselection and review test synchronization --- docs/testing/thunderbird-workflows.md | 34 ++++++++++++++++- internal/app/event_focus.go | 5 ++- internal/app/event_focus_test.go | 43 ++++++++++++++++++++++ internal/review/worker_test.go | 53 ++++++++++++++++++--------- 4 files changed, 115 insertions(+), 20 deletions(-) diff --git a/docs/testing/thunderbird-workflows.md b/docs/testing/thunderbird-workflows.md index e0c7e77..3560907 100644 --- a/docs/testing/thunderbird-workflows.md +++ b/docs/testing/thunderbird-workflows.md @@ -106,10 +106,42 @@ with 60 workflow checks in each format. The tests disable Braille; the event-loop tests separately check paragraph Braille cursor and routing metadata. +### Regression review against 2026.10.31 + +The November review reproduced missing attachment deselection speech twice on +`testing` at `e7d02b1`. The same local-mail harness passed both composition +formats against `2026.10.31`. After adding an attachment to an unsent message, +Escape cleared its native selection but the current reader omitted "not +selected". A focus event could capture the already-cleared selection before +the selected-state event arrived. The shared duplicate filter discarded that +state event even though row focus announces only positive selection. + +The filter now suppresses only positive selected-state announcements already +covered by row focus. Deselection remains audible when focus arrives first; +the existing rejection of deselection from the previous managed row remains. +This fixes the observed current-build failure without application matching or +new retained state. The underlying duplicate check predates the tag; this +comparison establishes the workflow regression, not its introducing commit. + +`TestProcessEventsPresentsDeselectionAfterRowFocus` replays that event order for +list and tree items, including duplicate focus and positive selection events. +It fails before the fix. The rebuilt reader passes the complete HTML and +plain-text local-mail workflows and the Firefox/Thunderbird reading baseline. +These isolated Xorg checks record speech requests and native focus/selection; +audible output and physical Braille still require the live checks below. +GTK/Qt ownership checks, focused app race checks, vet and the pre-push speech +checks also pass. The full app race suite passed in 233 seconds with a ten-minute +allowance; its Tentacle package fixtures exceed the former two-minute suite +limit. AT-SPI and presentation also passed. A separate flaky review test could +wait forever for a capture after a pending focus update repaired its cursor. +That test now establishes stale state before starting the move and bounds its +capture/result waits; 100 race-detector repetitions and the full review package +pass. This changes test synchronization, not runtime cursor behavior. + Run the shared regression checks as well: ```sh -go test -race ./internal/atspi ./internal/app ./internal/review ./internal/presentation -count=1 -timeout=120s +go test -race ./internal/atspi ./internal/app ./internal/review ./internal/presentation -count=1 -timeout=10m go vet ./internal/atspi ./internal/app ./internal/review ./internal/presentation python3 tests/native_firefox.py python3 tests/check_speech.py diff --git a/internal/app/event_focus.go b/internal/app/event_focus.go index f0d6dca..d0d3081 100644 --- a/internal/app/event_focus.go +++ b/internal/app/event_focus.go @@ -108,7 +108,10 @@ func (handler *accessibilityEventHandler) acceptFocusedEvent(event atspi.Event, logger.Debug("skipped event outside focused object", "type", event.Type, "role", event.Source.Role) return true } - if event.Type == atspi.SelectedStateEvent && sameSelectionItem(event.Source, state.current) { + // Row focus announces selection only when selected. A focus snapshot can + // already contain the cleared state (for example Escape on a Mozilla + // attachment), but it has not announced that deselection to the user. + if event.Type == atspi.SelectedStateEvent && event.Source.Selected && sameSelectionItem(event.Source, state.current) { logger.Debug("skipped selected state already presented by focus", "role", event.Source.Role) return true } diff --git a/internal/app/event_focus_test.go b/internal/app/event_focus_test.go index ea75c52..649aebc 100644 --- a/internal/app/event_focus_test.go +++ b/internal/app/event_focus_test.go @@ -113,6 +113,49 @@ func TestProcessEventsSuppressesQtPopupSelectionRepeats(t *testing.T) { } } +func TestProcessEventsPresentsDeselectionAfterRowFocus(t *testing.T) { + for _, role := range []string{"list item", "tree item"} { + t.Run(role, func(t *testing.T) { + speech := &fakeSpeechQueue{spoken: make(chan string, 8)} + adapter := &fakeAdapter{events: make(chan atspi.Event, 8)} + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + processEvents(ctx, slog.New(slog.DiscardHandler), speech, adapter, 0) + close(done) + }() + defer func() { cancel(); <-done }() + + row := atspi.Snapshot{BusName: ":1.7", Path: "/attachments/1", Name: "Attachment", + Role: role, Focused: true, Selected: true} + adapter.events <- atspi.NewEvent(atspi.FocusEvent, row) + adapter.events <- atspi.NewEvent(atspi.SelectedStateEvent, row) + // Mozilla can reassert focus with the new selection state before + // emitting selected:false. Focus omits the negative state, so the + // subsequent deselection still needs its own announcement. + row.Selected = false + adapter.events <- atspi.NewEvent(atspi.FocusEvent, row) + focused := atspi.NewEvent(atspi.FocusedStateEvent, row) + focused.Detail1 = 1 + adapter.events <- focused + adapter.events <- atspi.NewEvent(atspi.SelectedStateEvent, row) + adapter.events <- atspi.NewEvent(atspi.FocusEvent, + atspi.Snapshot{BusName: row.BusName, Path: "/done", Name: "Done", Role: "button"}) + for _, want := range []string{"Attachment, " + role + ", selected", "Attachment, " + role, + "not selected", "Done, button"} { + select { + case got := <-speech.spoken: + if got != want { + t.Fatalf("speech = %q, want %q", got, want) + } + case <-time.After(time.Second): + t.Fatalf("timed out waiting for %q", want) + } + } + }) + } +} + func TestProcessEventsSuppressesUnfocusedNameChangesDuringFileNavigation(t *testing.T) { speech := &fakeSpeechQueue{spoken: make(chan string, 8)} adapter := &fakeAdapter{events: make(chan atspi.Event, 8)} diff --git a/internal/review/worker_test.go b/internal/review/worker_test.go index 0529044..85224d2 100644 --- a/internal/review/worker_test.go +++ b/internal/review/worker_test.go @@ -197,30 +197,47 @@ func TestWorkerUsesNewestFocusAfterConcurrentCapture(t *testing.T) { } func TestWorkerRecapturesAndRepairsStaleCursor(t *testing.T) { - capturer := &fakeCapturer{calls: make(chan captureCall, 2)} - worker, cancel := startWorker(t, capturer, time.Second) - defer cancel() - + capturer := &fakeCapturer{calls: make(chan captureCall, 1)} + worker, err := NewWorker(capturer, time.Second) + if err != nil { + t.Fatal(err) + } firstTree := threeNodeTree(t) - worker.SyncFocus(objectID("/first")) - if err := worker.Move(NextSibling); err != nil { + cursor, err := NewCursor(firstTree) + if err != nil { t.Fatal(err) } - firstCall := <-capturer.calls - firstCall.result <- captureResult{tree: firstTree} - if result := waitResult(t, worker); result.Err != nil || result.Position.Current.Name != "Second" { - t.Fatalf("initial result = %#v", result) + focus := objectID("/first") + worker.SyncFocus(focus) + if _, err := cursor.SyncFocus(focus); err != nil { + t.Fatal(err) } + if position, moved, err := cursor.Move(NextSibling); err != nil || !moved || position.Current.Name != "Second" { + t.Fatalf("initial move = %#v, %v, %v", position, moved, err) + } + // Establish stale state before starting the move. A pending wakeFocus in + // a running worker can otherwise repair this cursor during invalidation, + // so the queued move legitimately needs no capture and the test hangs. firstTree.Invalidate(objectID("/second")) - - if err := worker.Move(NextSibling); err != nil { - t.Fatal(err) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + results := make(chan moveResult, 1) + go func() { results <- worker.move(ctx, cursor, NextSibling) }() + select { + case call := <-capturer.calls: + call.result <- captureResult{tree: threeNodeTree(t)} + case result := <-results: + t.Fatalf("stale cursor did not recapture: %#v", result) + case <-time.After(time.Second): + t.Fatal("timed out waiting for stale cursor capture") } - secondCall := <-capturer.calls - secondCall.result <- captureResult{tree: threeNodeTree(t)} - result := waitResult(t, worker) - if result.Err != nil || !result.Moved || result.Position.Current.Name != "Second" { - t.Fatalf("recovered result = %#v, want Second", result) + select { + case result := <-results: + if result.Err != nil || !result.Moved || result.Position.Current.Name != "Second" { + t.Fatalf("recovered result = %#v, want Second", result) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for recovered move") } }