Recover browser list selection on ticking pages

2026-11-11 10:39:09 -05:00
1 parent ac7f7acc43
commit 0d5a5f7ac1
@@ -46,7 +46,11 @@ The native document capture limit also bounds lists. Core navigation lists can
 contain more than 256 items; external Tentacle forms retain their existing
 256-item limit. List selections retain exact accessible references and page
 ownership, so removed targets, replaced documents and changed focus cannot
-select a different item. A changed page can require reopening the list.
+select a different item. If a ticking page invalidates accessible semantics
+while the list is open, selection may refresh the same retained tree up to
+twice and retry only that exact generation-checked reference; it never
+re-finds an item by label, path or position. A changed page can require
+reopening the list.

 Automated checks:

@@ -62,6 +66,10 @@ empty image list with actual X11 grabs and AT-SPI data. These checks inspect
 speech requests and synthetic input. They do not establish audible speech or
 physical keyboard/Braille behavior.

+Go regressions also cover exact-target selection after a ticking semantic
+refresh, the two-recapture bound, and rejection when recapture returns a
+different tree.
+
 Before submission, review the diff and test the rebuilt reader on a real page:

 1. Open links with Alt+Shift+K. Move with arrows, type a name, select
@@ -44,6 +44,11 @@ focus anchor so recapture itself failed with `ErrObjectNotFound` forever.
  one batch (one sync, capture cycle, placement and result, full step
  count). Caret steps are excluded: queued character/word/line steps keep
  their own ordered results.
+- Browser-list selection keeps its exact generation-checked reference, but
+  bounded same-tree recapture (at most two refreshes) now retries when a
+  tick invalidates semantics while the list is open. A different captured
+  tree, a removed or reused target, focus changes, and page reloads still
+  reject the selection instead of substituting another item.

 ## Caret validation on ticking text

@@ -14,7 +14,8 @@ import (
 )

 // DocumentListSnapshot retains exact generation-checked references. Opening a list
-// never moves the cursor. Selection cannot recapture or substitute an item.
+// never moves the cursor. Selection may refresh that same retained tree when
+// ticking semantics go stale, but it cannot substitute another item.
 type DocumentListSnapshot struct {
 	Entries  []DocumentListEntry
 	Selected int
@@ -230,23 +231,56 @@ func (worker *Worker) currentDocumentList(list DocumentListSnapshot) bool {
 func (worker *Worker) selectDocumentList(ctx context.Context, cursor *DocumentCursor, selection *documentListSelection) (Result, *DocumentCursor) {
 	list := selection.list
 	result := Result{DocumentEpoch: list.epoch, DocumentTarget: list.Target, DocumentLevel: list.Level, DocumentFocus: list.focus}
-	if !worker.currentDocumentList(list) {
-		result.Err = atspi.ErrStaleObject
-		return result, cursor
-	}
 	position := list.Entries[selection.index]
+	candidate := cursor
+	var doc atspi.DocumentSnapshot
+	for recaptures := 0; ; recaptures++ {
+		var retryable bool
+		var err error
+		doc, candidate, retryable, err = worker.validateDocumentListSelection(ctx, list, position, candidate)
+		if err == nil {
+			break
+		}
+		if !retryable || recaptures == 2 {
+			result.Err = err
+			return result, cursor
+		}
+		if result.CaptureReason == nil {
+			result.CaptureReason = err
+		}
+		result.DocumentDiagnostics.CaptureAttempts++
+		if captureErr := worker.recaptureDocumentList(ctx, list, &result); captureErr != nil {
+			result.Err = captureErr
+			return result, cursor
+		}
+	}
+	cursor = candidate
+	cursor.mu.Lock()
+	place := &cursor.places[len(cursor.places)-1]
+	place.object, place.offset, place.origin = position.Object, 0, StructuralNavigation
+	place.tableCell = atspi.ObjectID{}
+	remember_text_parent(doc, place)
+	result.DocumentPosition = documentPosition(doc, *place)
+	cursor.mu.Unlock()
+	result.Moved = true
+	// Use the exact validated target. A same-tree refresh may update its
+	// semantics, but selection must never choose a replacement item.
+	return worker.placeDocument(ctx, cursor, result), cursor
+}
+
+func (worker *Worker) validateDocumentListSelection(ctx context.Context, list DocumentListSnapshot, position DocumentListEntry, cursor *DocumentCursor) (atspi.DocumentSnapshot, *DocumentCursor, bool, error) {
+	if !worker.currentDocumentList(list) {
+		return atspi.DocumentSnapshot{}, cursor, false, atspi.ErrStaleObject
+	}
 	if _, err := list.tree.Resolve(position.Object); err != nil {
-		result.Err = err
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, documentNeedsCapture(err), err
 	}
 	doc, err := list.tree.DocumentView(ctx, position.Object)
 	if err != nil {
-		result.Err = err
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, documentNeedsCapture(err), err
 	}
 	if doc.Context.TopLevel != position.Document {
-		result.Err = atspi.ErrStaleObject
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, false, atspi.ErrStaleObject
 	}
 	found := false
 	for _, node := range doc.Nodes {
@@ -256,29 +290,37 @@ func (worker *Worker) selectDocumentList(ctx context.Context, cursor *DocumentCu
 		}
 	}
 	if !found {
-		result.Err = atspi.ErrStaleObject
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, false, atspi.ErrStaleObject
 	}
 	if cursor == nil || cursor.tree != list.tree {
 		cursor, _ = NewDocumentCursor(list.tree)
 	}
 	if _, err := cursor.SyncFocus(ctx, list.focus); err != nil {
-		result.Err = err
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, documentNeedsCapture(err), err
 	}
 	if !worker.currentDocumentList(list) {
-		result.Err = atspi.ErrStaleObject
-		return result, cursor
+		return atspi.DocumentSnapshot{}, cursor, false, atspi.ErrStaleObject
 	}
-	cursor.mu.Lock()
-	place := &cursor.places[len(cursor.places)-1]
-	place.object, place.offset, place.origin = position.Object, 0, StructuralNavigation
-	place.tableCell = atspi.ObjectID{}
-	remember_text_parent(doc, place)
-	result.DocumentPosition = documentPosition(doc, *place)
-	cursor.mu.Unlock()
-	result.Moved = true
-	// Use the exact validated target; retry/recapture belongs to ordinary
-	// navigation and must not choose a replacement for a menu selection.
-	return worker.placeDocument(ctx, cursor, result), cursor
+	return doc, cursor, false, nil
+}
+
+// recaptureDocumentList refreshes dirty semantics in the list's retained
+// tree. A different tree cannot prove that the list's generation-checked
+// references still name the same objects, so it is not a substitute.
+func (worker *Worker) recaptureDocumentList(ctx context.Context, list DocumentListSnapshot, result *Result) error {
+	captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout)
+	defer cancel()
+	started := time.Now()
+	tree, err := worker.capturer.CaptureDocument(captureContext, list.focus)
+	result.CaptureDuration += time.Since(started)
+	if err != nil {
+		return err
+	}
+	if tree == nil {
+		return errors.New("capture document list: nil tree")
+	}
+	if tree != list.tree {
+		return atspi.ErrStaleObject
+	}
+	return nil
 }
@@ -6,6 +6,7 @@ package review

 import (
 	"context"
+	"errors"
 	"testing"
 	"time"

@@ -81,6 +82,149 @@ func TestDocumentListRejectsChangedTargetsAndOwnership(t *testing.T) {
 	}
 }

+type refreshingListCapturer struct {
+	*readAllCapturer
+	captures   int
+	refresh    bool
+	dirtyPath  string
+	captureErr error
+}
+
+func (capturer *refreshingListCapturer) CaptureDocument(ctx context.Context, focus atspi.ObjectID) (*atspi.AccessibleTree, error) {
+	capturer.captures++
+	if capturer.refresh && capturer.captureErr != nil {
+		return nil, capturer.captureErr
+	}
+	if capturer.refresh {
+		nodes := webNodes()
+		nodes[4].Geometry = atspi.Geometry{VisibilityKnown: true}
+		nodes[5].Geometry = atspi.Geometry{VisibilityKnown: true, Visible: true}
+		if err := capturer.tree.Replace(webID("/root"), nodes); err != nil {
+			return nil, err
+		}
+		if capturer.dirtyPath != "" {
+			capturer.tree.InvalidateSnapshot(webID(capturer.dirtyPath))
+		}
+	}
+	return capturer.readAllCapturer.CaptureDocument(ctx, focus)
+}
+
+func TestDocumentListSelectsExactTargetAfterTickingRefresh(t *testing.T) {
+	for _, stalePath := range []string{"/first", "/button"} {
+		t.Run(stalePath, func(t *testing.T) {
+			tree, cursor := webCursor(t)
+			ctx := context.Background()
+			if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil {
+				t.Fatal(err)
+			}
+			capturer := &refreshingListCapturer{readAllCapturer: &readAllCapturer{tree: tree}}
+			worker, _ := NewWorker(capturer, time.Second)
+			worker.SyncFocus(webID("/doc"))
+			list, err := worker.listDocument(ctx, cursor, &documentListRequest{ctx: ctx, target: DocumentHeading})
+			if err != nil {
+				t.Fatal(err)
+			}
+			target := list.Entries[0].Object
+			capturer.refresh = true
+			if tree.InvalidateSnapshot(webID(stalePath)) != 1 {
+				t.Fatalf("did not invalidate %s", stalePath)
+			}
+			result, _ := worker.selectDocumentList(ctx, cursor, &documentListSelection{list: list, index: 0})
+			if result.Err != nil || !result.Moved || result.DocumentPosition.Object != target {
+				t.Fatalf("selection after ticking refresh: %+v", result)
+			}
+			if capturer.captures != 2 {
+				t.Fatalf("captures = %d, want opening capture plus one recapture", capturer.captures)
+			}
+			if result.DocumentDiagnostics.CaptureAttempts != 1 {
+				t.Fatalf("capture attempts = %d, want 1", result.DocumentDiagnostics.CaptureAttempts)
+			}
+		})
+	}
+}
+
+func TestDocumentListTickingRefreshIsBounded(t *testing.T) {
+	tree, cursor := webCursor(t)
+	ctx := context.Background()
+	if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil {
+		t.Fatal(err)
+	}
+	capturer := &refreshingListCapturer{readAllCapturer: &readAllCapturer{tree: tree}, dirtyPath: "/button"}
+	worker, _ := NewWorker(capturer, time.Second)
+	worker.SyncFocus(webID("/doc"))
+	list, err := worker.listDocument(ctx, cursor, &documentListRequest{ctx: ctx, target: DocumentHeading})
+	if err != nil {
+		t.Fatal(err)
+	}
+	capturer.refresh = true
+	tree.InvalidateSnapshot(list.Entries[0].Object.ID())
+	result, _ := worker.selectDocumentList(ctx, cursor, &documentListSelection{list: list, index: 0})
+	if result.Err == nil || result.Moved {
+		t.Fatalf("perpetually ticking selection succeeded: %+v", result)
+	}
+	if capturer.captures != 3 {
+		t.Fatalf("captures = %d, want opening capture plus two recaptures", capturer.captures)
+	}
+	if result.DocumentDiagnostics.CaptureAttempts != 2 {
+		t.Fatalf("capture attempts = %d, want 2", result.DocumentDiagnostics.CaptureAttempts)
+	}
+	if !errors.Is(result.CaptureReason, atspi.ErrStaleSnapshot) {
+		t.Fatalf("capture reason = %v, want stale snapshot", result.CaptureReason)
+	}
+}
+
+func TestDocumentListRecaptureFailureReportsCaptureError(t *testing.T) {
+	tree, cursor := webCursor(t)
+	ctx := context.Background()
+	if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil {
+		t.Fatal(err)
+	}
+	capturer := &refreshingListCapturer{readAllCapturer: &readAllCapturer{tree: tree}}
+	worker, _ := NewWorker(capturer, time.Second)
+	worker.SyncFocus(webID("/doc"))
+	list, err := worker.listDocument(ctx, cursor, &documentListRequest{ctx: ctx, target: DocumentHeading})
+	if err != nil {
+		t.Fatal(err)
+	}
+	capturer.refresh = true
+	capturer.captureErr = atspi.ErrProviderHung
+	tree.InvalidateSnapshot(list.Entries[0].Object.ID())
+	result, returned := worker.selectDocumentList(ctx, cursor, &documentListSelection{list: list, index: 0})
+	if !errors.Is(result.Err, atspi.ErrProviderHung) || result.Moved {
+		t.Fatalf("recapture failure was masked: %+v", result)
+	}
+	if returned != cursor {
+		t.Fatal("failed selection replaced the original cursor")
+	}
+	if !errors.Is(result.CaptureReason, atspi.ErrStaleSnapshot) {
+		t.Fatalf("capture reason = %v, want stale snapshot", result.CaptureReason)
+	}
+	if capturer.captures != 2 {
+		t.Fatalf("captures = %d, want opening capture plus failed recapture", capturer.captures)
+	}
+}
+
+func TestDocumentListRecaptureRejectsDifferentTree(t *testing.T) {
+	tree, cursor := webCursor(t)
+	ctx := context.Background()
+	if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil {
+		t.Fatal(err)
+	}
+	worker, _ := NewWorker(&readAllCapturer{tree: tree}, time.Second)
+	worker.SyncFocus(webID("/doc"))
+	list, err := worker.listDocument(ctx, cursor, &documentListRequest{ctx: ctx, target: DocumentHeading})
+	if err != nil {
+		t.Fatal(err)
+	}
+	fresh, _ := webCursor(t)
+	worker.capturer = &readAllCapturer{tree: fresh}
+	tree.InvalidateSnapshot(list.Entries[0].Object.ID())
+	result, _ := worker.selectDocumentList(ctx, cursor, &documentListSelection{list: list, index: 0})
+	if result.Err == nil || result.Moved {
+		t.Fatalf("selection substituted a target from a different tree: %+v", result)
+	}
+}
+
 func TestDocumentListDescendantLabelsExcludeHiddenAndPasswordText(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}