Fix Thunderbird deselection and review test synchronization

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
 	}

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)}

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")
 	}
 }