diff --git a/internal/app/document_review.go b/internal/app/document_review.go index 8bce04d..44780f1 100644 --- a/internal/app/document_review.go +++ b/internal/app/document_review.go @@ -338,6 +338,8 @@ func presentDocumentResult(logger *slog.Logger, speech speechEnqueuer, result re switch { case errors.Is(result.Err, atspi.ErrNotInDocument): text = "Not in web document content." + case errors.Is(result.Err, atspi.ErrProviderHung): + text = "Application is not responding." case result.Err != nil: logger.Warn("document navigation failed", "error", result.Err) text = "Could not find current location." diff --git a/internal/app/event_browser_tabs.go b/internal/app/event_browser_tabs.go index 7b91061..9a3541c 100644 --- a/internal/app/event_browser_tabs.go +++ b/internal/app/event_browser_tabs.go @@ -47,7 +47,8 @@ func (handler *accessibilityEventHandler) handleBrowserTab(event atspi.Event, ac if state.current.Autocomplete { focus = state.current.ID() } - objectReview.ResetDocument(focus) + objectReview.SuspendDocument(focus) + objectReview.WarmDocument() } spoken := presentation.FocusSpeechWithOptions(event.Source, state.presentationOptions) text := spoken.Text diff --git a/internal/app/event_lifecycle.go b/internal/app/event_lifecycle.go index 7bb5c62..e012135 100644 --- a/internal/app/event_lifecycle.go +++ b/internal/app/event_lifecycle.go @@ -95,6 +95,7 @@ func (handler *accessibilityEventHandler) handleLifecycle(event atspi.Event) boo browser.loaded(event.Source.ID()) if objectReview != nil { objectReview.ResetDocument(event.Source.ID()) + objectReview.WarmDocument() } } return true @@ -134,6 +135,8 @@ func (handler *accessibilityEventHandler) handleLifecycle(event atspi.Event) boo if err := objectReview.JumpToDocumentFragment(event.DocumentFragment); err != nil { logger.Warn("queue document fragment navigation", "error", err) } + } else { + objectReview.WarmDocument() } } if !alreadyAnnounced { diff --git a/internal/atspi/adapter.go b/internal/atspi/adapter.go index 101957c..a9c4416 100644 --- a/internal/atspi/adapter.go +++ b/internal/atspi/adapter.go @@ -25,6 +25,7 @@ import ( "runtime/cgo" "sync" "sync/atomic" + "time" "unsafe" ) @@ -134,30 +135,40 @@ type Adapter struct { foregroundRecovery int // GLib thread: bounded retries while a new client publishes AT-SPI recoveringForeground bool - events chan Event - treeRequests chan treeCaptureRequest - overflow atomic.Uint64 - treeInvalidations atomic.Uint64 - backgroundEventsFiltered atomic.Uint64 // rejected before native target/snapshot queries - secureMutex sync.Mutex - secureContext context.Context - secureCancel context.CancelFunc - secureEpoch atomic.Uint64 - secure atomic.Bool - secureReset atomic.Bool - secureResync atomic.Bool - captureSound atomic.Bool - captureTextAttributes atomic.Bool - running atomic.Bool - ready atomic.Bool - loopDone chan struct{} - loopError error // published by closing loopDone - stopRequested bool // stopMutex; keep Stop retries from queuing stale quit sources - stopMutex sync.Mutex - treeMutex sync.Mutex - tree *AccessibleTree - documentTree *AccessibleTree - documentTextTree *AccessibleTree + events chan Event + treeRequests chan treeCaptureRequest + overflow atomic.Uint64 + treeInvalidations atomic.Uint64 + backgroundEventsFiltered atomic.Uint64 // rejected before native target/snapshot queries + secureMutex sync.Mutex + secureContext context.Context + secureCancel context.CancelFunc + secureEpoch atomic.Uint64 + secure atomic.Bool + secureReset atomic.Bool + secureResync atomic.Bool + captureSound atomic.Bool + captureTextAttributes atomic.Bool + running atomic.Bool + ready atomic.Bool + loopDone chan struct{} + loopError error // published by closing loopDone + stopRequested bool // stopMutex; keep Stop retries from queuing stale quit sources + stopMutex sync.Mutex + hungMutex sync.Mutex + hungBuses map[string]time.Time + treeMutex sync.Mutex + tree *AccessibleTree + documentTree *AccessibleTree + documentTextTree *AccessibleTree + // The active document slots above serve one document root at a time. + // Recently captured documents are stashed by root (bounded, LRU) so + // returning to a tab reuses its tree and retained native sources + // instead of snapshotting the whole document cold. Guarded by + // treeMutex; stashed sources are released only on the GLib thread. + documentSlotRoot ObjectID + stashedDocuments map[ObjectID]stashedDocument + stashedDocumentIDs []ObjectID nativeDocument map[ObjectID]*C.AtspiAccessible documentLineCache map[ObjectID]DocumentCaretText // GLib owner; at most 64 bounded text objects nativeReview map[ObjectID]*C.AtspiAccessible @@ -354,12 +365,46 @@ func (adapter *Adapter) SetCaptureLogger(logger *slog.Logger) { adapter.captureLogger.Store(logger) } +const providerHungCooldown = 2 * time.Second + +// markProviderHung records a capture deadline for one application bus. +// Orca similarly remembers hung objects briefly so later commands skip +// straight past a provider that just stalled. +func (adapter *Adapter) markProviderHung(busName string, at time.Time) { + if busName == "" { + return + } + adapter.hungMutex.Lock() + defer adapter.hungMutex.Unlock() + if adapter.hungBuses == nil { + adapter.hungBuses = make(map[string]time.Time) + } + adapter.hungBuses[busName] = at +} + +func (adapter *Adapter) providerHung(busName string, at time.Time) bool { + adapter.hungMutex.Lock() + defer adapter.hungMutex.Unlock() + since, ok := adapter.hungBuses[busName] + if !ok { + return false + } + if at.Sub(since) > providerHungCooldown { + delete(adapter.hungBuses, busName) + return false + } + return true +} + func (adapter *Adapter) capture(ctx context.Context, reviewFocus ObjectID, captureName string, document, documentText bool) (tree *AccessibleTree, err error) { if ctx == nil { return nil, fmt.Errorf("capture AT-SPI %s: nil context", captureName) } ctx, cancel := adapter.secureRequestContext(ctx) defer cancel() + if document && adapter.providerHung(reviewFocus.BusName, time.Now()) { + return nil, fmt.Errorf("%w: bus %q", ErrProviderHung, reviewFocus.BusName) + } var trace *captureDiagnostics if reviewFocus.Valid() { ctx, trace = new_capture_diagnostics(ctx, adapter.captureLogger.Load(), reviewFocus) @@ -400,8 +445,14 @@ func (adapter *Adapter) capture(ctx context.Context, reviewFocus ObjectID, captu } select { case result := <-request.result: + if document && errors.Is(result.err, context.DeadlineExceeded) { + adapter.markProviderHung(reviewFocus.BusName, time.Now()) + } return result.tree, result.err case <-ctx.Done(): + if document && errors.Is(ctx.Err(), context.DeadlineExceeded) { + adapter.markProviderHung(reviewFocus.BusName, time.Now()) + } return nil, ctx.Err() } } @@ -410,6 +461,124 @@ func (adapter *Adapter) installTree(root ObjectID, nodes []TreeNode) (*Accessibl return adapter.installCapturedTree(root, nodes, false, false) } +const maxStashedDocuments = 3 + +type stashedDocument struct { + tree, textTree *AccessibleTree + sources map[ObjectID]*C.AtspiAccessible +} + +// rotateDocumentSlots makes root's retained trees and native sources the +// active document slots, stashing the previously active document. Capture +// calls this on the GLib thread once the capture root is known, before +// preparing the incremental-refresh cache. +func (adapter *Adapter) rotateDocumentSlots(root ObjectID) { + if !root.Valid() { + return + } + adapter.treeMutex.Lock() + defer adapter.treeMutex.Unlock() + if root == adapter.documentSlotRoot { + return + } + if adapter.documentSlotRoot.Valid() && + (adapter.documentTree != nil || adapter.documentTextTree != nil) { + adapter.stashDocumentLocked(adapter.documentSlotRoot, stashedDocument{ + tree: adapter.documentTree, textTree: adapter.documentTextTree, + sources: adapter.nativeDocument, + }) + } else { + for _, source := range adapter.nativeDocument { + C.g_object_unref(C.gpointer(source)) + } + } + if stashed, ok := adapter.stashedDocuments[root]; ok { + delete(adapter.stashedDocuments, root) + for index, id := range adapter.stashedDocumentIDs { + if id == root { + adapter.stashedDocumentIDs = append(adapter.stashedDocumentIDs[:index], adapter.stashedDocumentIDs[index+1:]...) + break + } + } + adapter.documentTree = stashed.tree + adapter.documentTextTree = stashed.textTree + adapter.nativeDocument = stashed.sources + } else { + adapter.documentTree = nil + adapter.documentTextTree = nil + adapter.nativeDocument = nil + } + adapter.documentSlotRoot = root +} + +func (adapter *Adapter) stashDocumentLocked(root ObjectID, document stashedDocument) { + if adapter.stashedDocuments == nil { + adapter.stashedDocuments = make(map[ObjectID]stashedDocument) + } + if old, ok := adapter.stashedDocuments[root]; ok { + for _, source := range old.sources { + C.g_object_unref(C.gpointer(source)) + } + for index, id := range adapter.stashedDocumentIDs { + if id == root { + adapter.stashedDocumentIDs = append(adapter.stashedDocumentIDs[:index], adapter.stashedDocumentIDs[index+1:]...) + break + } + } + } + adapter.stashedDocuments[root] = document + adapter.stashedDocumentIDs = append(adapter.stashedDocumentIDs, root) + for len(adapter.stashedDocumentIDs) > maxStashedDocuments { + oldest := adapter.stashedDocumentIDs[0] + for _, source := range adapter.stashedDocuments[oldest].sources { + C.g_object_unref(C.gpointer(source)) + } + delete(adapter.stashedDocuments, oldest) + adapter.stashedDocumentIDs = adapter.stashedDocumentIDs[1:] + } +} + +// clearStashedDocuments releases every stashed document's native sources. +// Callers run on the GLib thread (secure reset, shutdown). +func (adapter *Adapter) clearStashedDocuments() { + adapter.treeMutex.Lock() + stashed := adapter.stashedDocuments + adapter.stashedDocuments = nil + adapter.stashedDocumentIDs = nil + adapter.documentSlotRoot = ObjectID{} + adapter.treeMutex.Unlock() + for _, document := range stashed { + for _, source := range document.sources { + C.g_object_unref(C.gpointer(source)) + } + } +} + +func (adapter *Adapter) ownedTrees() []*AccessibleTree { + adapter.treeMutex.Lock() + defer adapter.treeMutex.Unlock() + return adapter.ownedTreesLocked() +} + +func (adapter *Adapter) ownedTreesLocked() []*AccessibleTree { + trees := make([]*AccessibleTree, 0, 3+2*len(adapter.stashedDocuments)) + for _, tree := range []*AccessibleTree{adapter.tree, adapter.documentTree, adapter.documentTextTree} { + if tree != nil { + trees = append(trees, tree) + } + } + for _, id := range adapter.stashedDocumentIDs { + stashed := adapter.stashedDocuments[id] + if stashed.tree != nil { + trees = append(trees, stashed.tree) + } + if stashed.textTree != nil { + trees = append(trees, stashed.textTree) + } + } + return trees +} + func (adapter *Adapter) installCapturedTree(root ObjectID, nodes []TreeNode, document, documentText bool) (*AccessibleTree, error) { adapter.treeMutex.Lock() defer adapter.treeMutex.Unlock() @@ -436,10 +605,7 @@ func (adapter *Adapter) installCapturedTree(root ObjectID, nodes []TreeNode, doc func (adapter *Adapter) invalidateTree() { adapter.treeMutex.Lock() defer adapter.treeMutex.Unlock() - for _, tree := range []*AccessibleTree{adapter.tree, adapter.documentTree, adapter.documentTextTree} { - if tree == nil { - continue - } + for _, tree := range adapter.ownedTreesLocked() { root, err := tree.Root() if err == nil { adapter.treeInvalidations.Add(uint64(tree.Invalidate(root.ID()))) diff --git a/internal/atspi/adapter_document_native.go b/internal/atspi/adapter_document_native.go index 4f83f13..cbdd6a1 100644 --- a/internal/atspi/adapter_document_native.go +++ b/internal/atspi/adapter_document_native.go @@ -12,6 +12,7 @@ import "C" import ( "context" "errors" + "fmt" "strings" "time" "unicode/utf8" @@ -42,12 +43,29 @@ func (adapter *Adapter) clearNativeReview() { adapter.nativeReviewBus = "" } +// busName identifies the application a document operation will talk to, +// so a hung provider can be skipped before queueing more native work. +func (request treeCaptureRequest) busName() string { + for _, id := range []ObjectID{ + request.reviewFocus, request.position.ID(), request.caretObject.ID(), request.soundObjects[0].ID(), + } { + if id.Valid() { + return id.BusName + } + } + return "" +} + func (adapter *Adapter) documentOperation(ctx context.Context, request treeCaptureRequest) (treeCaptureResult, error) { ctx, cancel := adapter.secureRequestContext(ctx) defer cancel() if err := ctx.Err(); err != nil { return treeCaptureResult{}, err } + busName := request.busName() + if busName != "" && adapter.providerHung(busName, time.Now()) { + return treeCaptureResult{}, fmt.Errorf("%w: bus %q", ErrProviderHung, busName) + } adapter.stopMutex.Lock() defer adapter.stopMutex.Unlock() if !adapter.ready.Load() || adapter.callbackData == nil { @@ -65,8 +83,14 @@ func (adapter *Adapter) documentOperation(ctx context.Context, request treeCaptu } select { case result := <-request.result: + if errors.Is(result.err, context.DeadlineExceeded) { + adapter.markProviderHung(busName, time.Now()) + } return result, result.err case <-ctx.Done(): + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + adapter.markProviderHung(busName, time.Now()) + } return treeCaptureResult{}, ctx.Err() } } @@ -431,6 +455,9 @@ func (adapter *Adapter) nativeDocumentContext(ctx context.Context, focus ObjectI break } } + if source == nil { + source = adapter.retainedDocumentSource(focus) + } if source == nil { return DocumentContext{}, ErrObjectNotFound } diff --git a/internal/atspi/adapter_lifecycle_native.go b/internal/atspi/adapter_lifecycle_native.go index 596ecf6..a644298 100644 --- a/internal/atspi/adapter_lifecycle_native.go +++ b/internal/atspi/adapter_lifecycle_native.go @@ -85,6 +85,7 @@ func (adapter *Adapter) Start(ctx context.Context) error { } adapter.documentWindows = nil adapter.documentWindowIDs = nil + adapter.clearStashedDocuments() adapter.clearNativeDocument() adapter.deferredLiveRegions = nil adapter.clearNativeReview() diff --git a/internal/atspi/adapter_references_native.go b/internal/atspi/adapter_references_native.go index 44fb32d..2703ae5 100644 --- a/internal/atspi/adapter_references_native.go +++ b/internal/atspi/adapter_references_native.go @@ -22,14 +22,19 @@ func (adapter *Adapter) captureDesktop(ctx context.Context, reviewFocus ObjectID trace.begin_stage("root-selection", reviewFocus, "") root, err := adapter.nativeDocumentRoot(ctx, reviewFocus) adapter.treeMutex.Lock() - previousTree := adapter.documentTree + retainedRoots := make(map[ObjectID]bool, 1+len(adapter.stashedDocumentIDs)) + if adapter.documentSlotRoot.Valid() && adapter.documentTree != nil { + retainedRoots[adapter.documentSlotRoot] = true + } + for _, id := range adapter.stashedDocumentIDs { + retainedRoots[id] = true + } adapter.treeMutex.Unlock() - if (errors.Is(err, ErrObjectNotFound) || errors.Is(err, ErrStaleObject)) && previousTree != nil && len(adapter.nativeFocus) != 0 { + if (errors.Is(err, ErrObjectNotFound) || errors.Is(err, ErrStaleObject)) && len(retainedRoots) != 0 && len(adapter.nativeFocus) != 0 { // Dynamic pages can remove the browse cursor without sending a new // focus event. Recover only from the newest retained native focus in // the same document. A chrome or other-tab focus is not a fallback. - currentRoot, rootErr := previousTree.Root() - for i := len(adapter.nativeFocus) - 1; rootErr == nil && i >= 0; i-- { + for i := len(adapter.nativeFocus) - 1; i >= 0; i-- { latest := adapter.nativeFocus[i] if latest.id.BusName != reviewFocus.BusName { break @@ -52,14 +57,16 @@ func (adapter *Adapter) captureDesktop(ctx context.Context, reviewFocus ObjectID } candidate, candidateErr := adapter.nativeDocumentRoot(ctx, latest.id) if candidateErr != nil { + continue + } + if retainedRoots[nativeObjectID(candidate)] { + root, err = candidate, nil break } - if nativeObjectID(candidate) == currentRoot.ID() { - root, err = candidate, nil - } else { - C.g_object_unref(C.gpointer(candidate)) - } - break + // The newest live focus can sit in browser chrome or a tab + // whose document was never retained; older focus history in + // the retained document may still resolve. + C.g_object_unref(C.gpointer(candidate)) } } if err != nil { @@ -67,6 +74,10 @@ func (adapter *Adapter) captureDesktop(ctx context.Context, reviewFocus ObjectID } trace.set_root(nativeObjectID(root), "document") trace.begin_stage("cache-preparation", nativeObjectID(root), "") + adapter.rotateDocumentSlots(nativeObjectID(root)) + adapter.treeMutex.Lock() + previousTree := adapter.documentTree + adapter.treeMutex.Unlock() cache := nativeTreeCache{} if !documentText && previousTree != nil { cache.nodes = previousTree.nativeRefresh(nativeObjectID(root)) @@ -333,6 +344,25 @@ func (adapter *Adapter) focusedChrome(document ObjectID) ObjectID { // Return an owned reference to the outermost document containing focus. // Parent traversal is bounded and never scans other windows or tabs. +// retainedDocumentSource returns a new reference to a retained native +// source from the active or a stashed document capture. Capture root +// selection needs this when the focus identity (often the document root +// itself after a load, or a saved document used as capture anchor) never +// received a focus event and so is absent from the focus history. +func (adapter *Adapter) retainedDocumentSource(id ObjectID) *C.AtspiAccessible { + adapter.treeMutex.Lock() + defer adapter.treeMutex.Unlock() + if source := adapter.nativeDocument[id]; source != nil { + return (*C.AtspiAccessible)(C.g_object_ref(C.gpointer(source))) + } + for _, root := range adapter.stashedDocumentIDs { + if source := adapter.stashedDocuments[root].sources[id]; source != nil { + return (*C.AtspiAccessible)(C.g_object_ref(C.gpointer(source))) + } + } + return nil +} + func (adapter *Adapter) nativeDocumentRoot(ctx context.Context, focus ObjectID) (*C.AtspiAccessible, error) { var source *C.AtspiAccessible for _, item := range adapter.nativeFocus { @@ -341,6 +371,9 @@ func (adapter *Adapter) nativeDocumentRoot(ctx context.Context, focus ObjectID) break } } + if source == nil { + source = adapter.retainedDocumentSource(focus) + } if source == nil { return nil, fmt.Errorf("%w: document focus bus %q path %q", ErrObjectNotFound, focus.BusName, focus.Path) } diff --git a/internal/atspi/adapter_secure_native.go b/internal/atspi/adapter_secure_native.go index 08289c6..0753d33 100644 --- a/internal/atspi/adapter_secure_native.go +++ b/internal/atspi/adapter_secure_native.go @@ -48,6 +48,7 @@ func (adapter *Adapter) applySecureReset() { adapter.treeMutex.Lock() adapter.tree, adapter.documentTree, adapter.documentTextTree = nil, nil, nil adapter.treeMutex.Unlock() + adapter.clearStashedDocuments() adapter.clearNativeDocument() adapter.clearNativeReview() adapter.clearMouseApplication() diff --git a/internal/atspi/adapter_tree_native.go b/internal/atspi/adapter_tree_native.go index 4d20a51..00afb1c 100644 --- a/internal/atspi/adapter_tree_native.go +++ b/internal/atspi/adapter_tree_native.go @@ -540,13 +540,8 @@ func (adapter *Adapter) invalidateReplacedDocument(sourceID, activeID ObjectID) if !sourceID.Valid() || sourceID != activeID { return } - adapter.treeMutex.Lock() - trees := []*AccessibleTree{adapter.tree, adapter.documentTree, adapter.documentTextTree} - adapter.treeMutex.Unlock() + trees := adapter.ownedTrees() for _, tree := range trees { - if tree == nil { - continue - } root, err := tree.Root() if err != nil || root.ID().BusName != sourceID.BusName { continue @@ -574,10 +569,23 @@ func (adapter *Adapter) invalidateTreeEventWithPolicy(eventType string, detail1 } } } + // Kinds parallel the historical slot indices: 0 review, 1 semantic + // document, 2 document text. Stashed documents repeat kinds 1 and 2. adapter.treeMutex.Lock() trees := []*AccessibleTree{adapter.tree, adapter.documentTree, adapter.documentTextTree} + kinds := []int{0, 1, 2} + for _, id := range adapter.stashedDocumentIDs { + stashed := adapter.stashedDocuments[id] + if stashed.tree != nil { + trees, kinds = append(trees, stashed.tree), append(kinds, 1) + } + if stashed.textTree != nil { + trees, kinds = append(trees, stashed.textTree), append(kinds, 2) + } + } adapter.treeMutex.Unlock() - for index, tree := range trees { + for position, tree := range trees { + index := kinds[position] if navigationDefunct && index == 1 && tree != nil { tree.mu.RLock() entry := tree.nodes[sourceID] diff --git a/internal/atspi/adapter_tree_test.go b/internal/atspi/adapter_tree_test.go index 198a2aa..186e907 100644 --- a/internal/atspi/adapter_tree_test.go +++ b/internal/atspi/adapter_tree_test.go @@ -513,3 +513,117 @@ func TestSecureInputCancelsCapturesAcrossLockTransitions(t *testing.T) { t.Fatal("unlock revived old capture") } } + +// Switching browser tabs must not discard the previous document's captured +// tree: returning to a tab reuses its tree so capture only re-reads nodes +// dirtied while away, instead of snapshotting the whole document cold. +func TestDocumentSlotsRetainTreesAcrossRoots(t *testing.T) { + adapter := New() + install := func(rootPath, childPath string) *AccessibleTree { + t.Helper() + root := NewObjectID(":1.42", rootPath) + adapter.rotateDocumentSlots(root) + rootSnapshot := NewSnapshot(rootPath, "Document", "document web", "", "") + rootSnapshot.BusName = root.BusName + childSnapshot := NewSnapshot(childPath, "Heading", "heading", "", "") + childSnapshot.BusName = root.BusName + tree, err := adapter.installCapturedTree(root, []TreeNode{ + {Snapshot: rootSnapshot, Children: []ObjectID{NewObjectID(":1.42", childPath)}}, + {Snapshot: childSnapshot}, + }, true, false) + if err != nil { + t.Fatal(err) + } + return tree + } + treeA := install("/docA", "/a_heading") + childA, err := treeA.Reference(NewObjectID(":1.42", "/a_heading")) + if err != nil { + t.Fatal(err) + } + treeB := install("/docB", "/b_heading") + if treeB == treeA { + t.Fatal("second document replaced first document tree object") + } + adapter.rotateDocumentSlots(NewObjectID(":1.42", "/docA")) + if adapter.documentTree != treeA { + t.Fatal("returning to first document did not restore its retained tree") + } + if _, err := adapter.documentTree.Resolve(childA); err != nil { + t.Fatalf("retained document lost its captured nodes: %v", err) + } + // Retention is bounded: cycling through more documents than the limit + // evicts the least recently used one. + for _, rootPath := range []string{"/docC", "/docD", "/docE"} { + install(rootPath, rootPath+"_heading") + } + adapter.rotateDocumentSlots(NewObjectID(":1.42", "/docB")) + if adapter.documentTree == treeB { + t.Fatal("least recently used document tree was not evicted") + } + if adapter.documentTree != nil { + if root, err := adapter.documentTree.Root(); err == nil && root.ID().Path == "/docB" { + t.Fatal("evicted document tree retained its nodes") + } + } + // Events for a stashed document must still invalidate it while inactive. + adapter.rotateDocumentSlots(NewObjectID(":1.42", "/docE")) + stashedE := adapter.documentTree + if stashedE == nil { + t.Fatal("most recent document tree missing") + } + childE, err := stashedE.Reference(NewObjectID(":1.42", "/docE_heading")) + if err != nil { + t.Fatal(err) + } + adapter.rotateDocumentSlots(NewObjectID(":1.42", "/docD")) + adapter.invalidateTreeEvent(DefunctStateEvent, 1, NewObjectID(":1.42", "/docE_heading"), ObjectID{}) + if _, err := stashedE.Resolve(childE); !errors.Is(err, ErrStaleObject) { + t.Fatalf("inactive document ignored defunct invalidation: %v", err) + } +} + +// A provider whose capture just timed out is probably still stuck. The +// next document capture for that bus must fail fast with ErrProviderHung +// instead of queueing another multi-second native stall, and the mark +// must expire so a recovered provider is used again. +func TestCaptureFailsFastWhileProviderHung(t *testing.T) { + adapter := New() + now := time.Now() + adapter.markProviderHung(":1.99", now) + if !adapter.providerHung(":1.99", now.Add(time.Second)) { + t.Fatal("fresh hung mark not reported") + } + if adapter.providerHung(":1.99", now.Add(time.Hour)) { + t.Fatal("hung mark never expires") + } + if adapter.providerHung(":1.98", now) { + t.Fatal("hung mark leaked to another bus") + } + // The expiry probe above purged the mark, as a real post-cooldown + // capture attempt would. Mark again for the capture assertion. + adapter.markProviderHung(":1.99", time.Now()) + if _, err := adapter.CaptureDocument(context.Background(), NewObjectID(":1.99", "/doc")); !errors.Is(err, ErrProviderHung) { + t.Fatalf("hung capture error = %v, want ErrProviderHung", err) + } + tree, err := NewAccessibleTree(NewObjectID(":1.99", "/doc"), []TreeNode{ + {Snapshot: func() Snapshot { + snapshot := NewSnapshot("/doc", "Document", "document web", "", "") + snapshot.BusName = ":1.99" + return snapshot + }()}, + }) + if err != nil { + t.Fatal(err) + } + position, err := tree.Reference(NewObjectID(":1.99", "/doc")) + if err != nil { + t.Fatal(err) + } + if _, err := adapter.ReadDocumentCaret(context.Background(), position, 0, DocumentCaretLine); !errors.Is(err, ErrProviderHung) { + t.Fatalf("hung read error = %v, want ErrProviderHung", err) + } + if err := adapter.SetDocumentPosition(context.Background(), position, position.ID()); !errors.Is(err, ErrProviderHung) { + t.Fatalf("hung placement error = %v, want ErrProviderHung", err) + } +} diff --git a/internal/atspi/document.go b/internal/atspi/document.go index 8fc5660..a475d7e 100644 --- a/internal/atspi/document.go +++ b/internal/atspi/document.go @@ -24,6 +24,12 @@ import ( var ErrNotInDocument = errors.New("accessible is outside web document content") +// ErrProviderHung reports that an application's accessible provider just +// stalled a document capture past its deadline. Captures for that bus fail +// fast with this error for a short cooldown instead of queueing behind +// the stuck provider on every keypress. +var ErrProviderHung = errors.New("accessible provider is not responding") + // DocumentContext identifies both the innermost document and the outermost // document containing an object. Ancestors run from the object to the tree // root. All data is copied under one tree lock; no native references escape. diff --git a/internal/atspi/document_error.go b/internal/atspi/document_error.go index 19e6f65..e0536fe 100644 --- a/internal/atspi/document_error.go +++ b/internal/atspi/document_error.go @@ -38,13 +38,7 @@ func (adapter *Adapter) document_object_error(object ObjectID, err error) error "could not be processed as object "+object.Path+" does not exist") { return err } - adapter.treeMutex.Lock() - trees := []*AccessibleTree{adapter.tree, adapter.documentTree, adapter.documentTextTree} - adapter.treeMutex.Unlock() - for _, tree := range trees { - if tree == nil { - continue - } + for _, tree := range adapter.ownedTrees() { tree.mu.RLock() entry := tree.nodes[object] tree.mu.RUnlock() diff --git a/internal/atspi/document_view.go b/internal/atspi/document_view.go index 4b705b0..bf97c45 100644 --- a/internal/atspi/document_view.go +++ b/internal/atspi/document_view.go @@ -72,14 +72,24 @@ func (document DocumentSnapshot) SubtreeEnd(index int) int { } func document_entry_error(entry treeEntry) error { + return document_entry_error_tolerating(entry, false) +} + +// document_entry_error_tolerating optionally ignores text, geometry, and +// child-order staleness. Caret movement reads text and line bounds live +// from the provider and generation-checks every endpoint, so captured +// staleness of those cannot make its result wrong: at worst a just-inserted +// sibling is skipped until the next capture. Identity and semantics are +// checked either way. +func document_entry_error_tolerating(entry treeEntry, tolerateText bool) error { switch { case entry.snapshotInvalid: return ErrStaleSnapshot - case entry.childrenInvalid: + case entry.childrenInvalid && !tolerateText: return ErrIncompleteTree - case entry.textInvalid: + case entry.textInvalid && !tolerateText: return ErrStaleText - case entry.geometryInvalid: + case entry.geometryInvalid && !tolerateText: return ErrStaleGeometry } return nil @@ -95,19 +105,32 @@ func (tree *AccessibleTree) ValidateDocumentRange(ctx context.Context, document return tree.validate_document_range_locked(ctx, document, from, to) } +// ValidateDocumentCaretRange is ValidateDocumentRange for caret movement, +// whose text and line bounds are read live from the provider during the +// move. Text, geometry, and child-order staleness along the path therefore +// does not force a recapture first; on constantly ticking pages some +// traversed counter is always mid-update, and strict validation could +// never commit a move no matter how often it recaptured. Endpoints remain +// generation-checked, and structural navigation keeps strict validation. +func (tree *AccessibleTree) ValidateDocumentCaretRange(ctx context.Context, document DocumentSnapshot, from, to int) error { + tree.mu.RLock() + defer tree.mu.RUnlock() + return tree.validate_document_path_locked(ctx, document, from, to, from != to, true) +} + // ValidateDocumentOrder also checks ancestor child order when a structural // search or a document-boundary jump will leave its current object. func (tree *AccessibleTree) ValidateDocumentOrder(ctx context.Context, document DocumentSnapshot, index int) error { tree.mu.RLock() defer tree.mu.RUnlock() - return tree.validate_document_path_locked(ctx, document, index, index, true) + return tree.validate_document_path_locked(ctx, document, index, index, true, false) } func (tree *AccessibleTree) validate_document_range_locked(ctx context.Context, document DocumentSnapshot, from, to int) error { - return tree.validate_document_path_locked(ctx, document, from, to, from != to) + return tree.validate_document_path_locked(ctx, document, from, to, from != to, false) } -func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, document DocumentSnapshot, from, to int, requireOrder bool) error { +func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, document DocumentSnapshot, from, to int, requireOrder, tolerateText bool) error { if err := ctx.Err(); err != nil { return err } @@ -126,10 +149,10 @@ func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, d if err != nil { return err } - if err := document_entry_error(entry); err != nil { + if err := document_entry_error_tolerating(entry, tolerateText); err != nil { return err } - if err := tree.validate_document_row_locked(document, node.Reference.ID()); err != nil { + if err := tree.validate_document_row_locked(document, node.Reference.ID(), tolerateText); err != nil { return err } for entry.hasParent { @@ -137,20 +160,23 @@ func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, d if entry.snapshotInvalid { return ErrStaleSnapshot } - if entry.geometryInvalid { + if entry.geometryInvalid && !tolerateText { return ErrStaleGeometry } - if requireOrder && entry.childrenInvalid { + if requireOrder && !tolerateText && entry.childrenInvalid { return ErrIncompleteTree } // Hypertext ranges interleave parent-owned text with embedded // children. An insertion can change those ranges without changing // the accessible child list. Refresh before crossing objects, while // allowing movement within an unchanged child to remain local. + // Even caret movement keeps this check: captured embedded-object + // offsets decide which ranges exist, so tolerating a hypertext + // insertion would silently skip the inserted text. if requireOrder && entry.textInvalid && len(entry.snapshot.TextChildren) != 0 { return ErrStaleText } - if err := tree.validate_document_row_locked(document, entry.snapshot.ID()); err != nil { + if err := tree.validate_document_row_locked(document, entry.snapshot.ID(), tolerateText); err != nil { return err } } @@ -158,13 +184,13 @@ func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, d return nil } -func (tree *AccessibleTree) validate_document_row_locked(document DocumentSnapshot, id ObjectID) error { +func (tree *AccessibleTree) validate_document_row_locked(document DocumentSnapshot, id ObjectID, tolerateText bool) error { for _, reference := range document.data.rows[id] { entry, err := tree.resolveLocked(reference) if err != nil { return err } - if err := document_entry_error(entry); err != nil { + if err := document_entry_error_tolerating(entry, tolerateText); err != nil { return err } } diff --git a/internal/atspi/document_view_test.go b/internal/atspi/document_view_test.go index 1707eef..846e7d2 100644 --- a/internal/atspi/document_view_test.go +++ b/internal/atspi/document_view_test.go @@ -110,6 +110,43 @@ func TestNavigationDocumentValidatesOnlyTraversedBranches(t *testing.T) { } } +// Caret moves read text and line bounds live from the provider, so text, +// geometry, or child-order staleness along the traversed range must not +// force a recapture first: on ticking pages some counter between caret and +// target is always mid-update, and strict validation traps the caret no +// matter how often it recaptures. Identity and semantics stay strict, and +// the strict range validation used by structural navigation is unchanged. +func TestValidateDocumentCaretRangeToleratesTextAndGeometryStaleness(t *testing.T) { + for _, test := range []struct { + name string + invalidate func(*AccessibleTree) + want error + wantStrict error + }{ + {"text", func(tree *AccessibleTree) { tree.InvalidateText(testID("/link")) }, nil, ErrStaleText}, + {"geometry", func(tree *AccessibleTree) { tree.InvalidateGeometry(testID("/inner")) }, nil, ErrStaleGeometry}, + {"semantics", func(tree *AccessibleTree) { tree.InvalidateSnapshot(testID("/link")) }, ErrStaleSnapshot, ErrStaleSnapshot}, + {"children", func(tree *AccessibleTree) { tree.MarkChildrenChanged(testID("/inner")) }, nil, ErrIncompleteTree}, + } { + t.Run(test.name, func(t *testing.T) { + tree, _ := NewAccessibleTree(testID("/root"), documentTestNodes()) + ref, _ := tree.Reference(testID("/heading")) + view, err := tree.NavigationDocument(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + test.invalidate(tree) + heading, _ := view.Index(ref.ID()) + if err := tree.ValidateDocumentCaretRange(context.Background(), view, heading, len(view.Nodes)-1); !errors.Is(err, test.want) { + t.Fatalf("caret validation: %v, want %v", err, test.want) + } + if err := tree.ValidateDocumentRange(context.Background(), view, heading, len(view.Nodes)-1); !errors.Is(err, test.wantStrict) { + t.Fatalf("strict validation changed: %v, want %v", err, test.wantStrict) + } + }) + } +} + func TestNavigationDocumentRejectsRetiredViewsAndCanceledMoves(t *testing.T) { tree, _ := NewAccessibleTree(testID("/root"), documentTestNodes()) ref, _ := tree.Reference(testID("/heading")) diff --git a/internal/review/document.go b/internal/review/document.go index cba3d7e..617e136 100644 --- a/internal/review/document.go +++ b/internal/review/document.go @@ -69,6 +69,16 @@ type documentPlace struct { // A text child can be replaced while its parent and adjacent siblings live. // Generation-checked boundaries distinguish replacement from insertion. textParent, textPrevious, textNext atspi.ObjectReference + // Ticking pages can recreate every child of the surviving text parent at + // once, retiring all sibling references. The child index and count then + // re-resolve the same navigation line under that generation-checked + // parent. Count zero means no index was retained. + textChildIndex, textChildCount int + // The same bounded evidence for non-text children (recreated button + // rows and containers): immediate parent, adjacent siblings, and the + // child's index and count under that generation-checked parent. + parent, parentPrevious, parentNext atspi.ObjectReference + parentChildIndex, parentChildCount int // Bounded surviving ancestry is a navigation fallback, never an action target. recoveryParents []atspi.ObjectReference } @@ -109,6 +119,25 @@ func (cursor *DocumentCursor) rebindPlace(tree *atspi.AccessibleTree, oldPositio cursor.active = newPosition.Document } +// capture_anchor returns the saved document identity for a recapture when +// the page destroyed the focused anchor itself. The anchor only locates the +// document to capture; position recovery still runs against the fresh tree. +func (cursor *DocumentCursor) capture_anchor(focus atspi.ObjectID) (atspi.ObjectID, bool) { + if cursor == nil { + return atspi.ObjectID{}, false + } + cursor.mu.Lock() + defer cursor.mu.Unlock() + if len(cursor.places) == 0 { + return atspi.ObjectID{}, false + } + document := cursor.places[len(cursor.places)-1].document.ID() + if !document.Valid() || document.BusName != focus.BusName || document == focus { + return atspi.ObjectID{}, false + } + return document, true +} + func NewDocumentCursor(tree *atspi.AccessibleTree) (*DocumentCursor, error) { if tree == nil { return nil, errors.New("create document cursor: nil accessible tree") @@ -215,8 +244,13 @@ func (cursor *DocumentCursor) sync_focus(ctx context.Context, focus atspi.Object recovery = "preserved" place = saved place.focus = ref - } else if replacement, ok := replacement_text_node(doc, saved); ok { - recovery = "replaced-text-child" + } else if replacement, kind, ok := replacement_text_node(doc, saved); ok { + recovery = kind + place = saved + place.focus, place.object = ref, replacement.Reference + place.offset = min(saved.offset, max(0, replacement.Snapshot.CharacterCount-1)) + } else if replacement, kind, ok := replacement_child_node(doc, saved); ok { + recovery = kind place = saved place.focus, place.object = ref, replacement.Reference place.offset = min(saved.offset, max(0, replacement.Snapshot.CharacterCount-1)) @@ -232,7 +266,13 @@ func (cursor *DocumentCursor) sync_focus(ctx context.Context, focus atspi.Object if !ok { return DocumentPosition{}, "saved-object-missing", atspi.ErrStaleObject } - if err := cursor.tree.ValidateDocumentRange(ctx, doc, placeIndex, placeIndex); err != nil { + validatePlace := cursor.tree.ValidateDocumentRange + if place.origin != StructuralNavigation { + // Caret and focus-derived places are read live on the next move; + // ticking text on the anchored object must not discard the place. + validatePlace = cursor.tree.ValidateDocumentCaretRange + } + if err := validatePlace(ctx, doc, placeIndex, placeIndex); err != nil { return DocumentPosition{}, "document-unresolved", err } if removeIndex >= 0 { diff --git a/internal/review/document_benchmark_test.go b/internal/review/document_benchmark_test.go index a349846..25267de 100644 --- a/internal/review/document_benchmark_test.go +++ b/internal/review/document_benchmark_test.go @@ -37,7 +37,7 @@ func BenchmarkDocumentWorkerHeadingNavigation(b *testing.B) { b.ReportAllocs() for b.Loop() { for _, backwards := range []bool{false, true} { - result, next := worker.moveDocument(ctx, cursor, DocumentHeading, 2, backwards, 0) + result, next := worker.moveDocument(ctx, cursor, DocumentHeading, 2, backwards, 0, 0) cursor = next want := ids[middle+20] if backwards { diff --git a/internal/review/document_caret.go b/internal/review/document_caret.go index 23e5fd2..cfeb0c5 100644 --- a/internal/review/document_caret.go +++ b/internal/review/document_caret.go @@ -101,7 +101,7 @@ func (cursor *DocumentCursor) MoveCaret(ctx context.Context, reader DocumentCare if !found || doc.Nodes[current].Reference != place.object { return DocumentCaretMove{}, atspi.ErrStaleObject } - if err := cursor.tree.ValidateDocumentRange(ctx, doc, current, current); err != nil { + if err := cursor.tree.ValidateDocumentCaretRange(ctx, doc, current, current); err != nil { return DocumentCaretMove{}, err } if command >= TableLeft { @@ -120,7 +120,7 @@ func (cursor *DocumentCursor) MoveCaret(ctx context.Context, reader DocumentCare } indices := cursor.caretRanges if len(indices) == 0 { - if err := cursor.tree.ValidateDocumentRange(ctx, doc, 0, len(doc.Nodes)-1); err != nil { + if err := cursor.tree.ValidateDocumentCaretRange(ctx, doc, 0, len(doc.Nodes)-1); err != nil { return DocumentCaretMove{}, err } return DocumentCaretMove{Position: documentPosition(doc, *place)}, nil @@ -149,7 +149,7 @@ func (cursor *DocumentCursor) MoveCaret(ctx context.Context, reader DocumentCare // during recovery would skip the line. Synthetic whole-object stops // still need validation because they bypass the native text reader. if !navigationReader.did_read(place.object) { - if err := cursor.tree.ValidateDocumentRange(ctx, doc, current, target); err != nil { + if err := cursor.tree.ValidateDocumentCaretRange(ctx, doc, current, target); err != nil { return DocumentCaretMove{}, err } } @@ -162,7 +162,7 @@ func (cursor *DocumentCursor) MoveCaret(ctx context.Context, reader DocumentCare if command == CaretPreviousCharacter || command == CaretPreviousWord || command == CaretPreviousLine || command == CaretDocumentStart { boundary = 0 } - if err := cursor.tree.ValidateDocumentRange(ctx, doc, current, boundary); err != nil { + if err := cursor.tree.ValidateDocumentCaretRange(ctx, doc, current, boundary); err != nil { return DocumentCaretMove{}, err } } diff --git a/internal/review/document_caret_line.go b/internal/review/document_caret_line.go index b2a0f82..4233b7a 100644 --- a/internal/review/document_caret_line.go +++ b/internal/review/document_caret_line.go @@ -42,10 +42,27 @@ func move_caret_object_line(ctx context.Context, reader DocumentCaretReader, doc offset = indices[position].end - 1 } initialPosition := position + // Structural navigation already announced its target's whole line, + // including embedded children such as a heading's link. A following + // Down must leave that subtree, not descend into it and repeat the + // announcement as a new line. The target's own first range still uses + // the same-current logic below, so a long structural paragraph keeps + // advancing line by line within its own text. + structuralEnd := -1 + if place.origin == StructuralNavigation { + structuralEnd = doc.SubtreeEnd(current) + } for attempts := 0; attempts < len(indices)+1; attempts++ { if position < 0 || position >= len(indices) { return DocumentCaretMove{Position: documentPosition(doc, *place)}, nil } + if command == CaretNextLine && structuralEnd >= current && + indices[position].index >= current && indices[position].index <= structuralEnd && + !(position == initialPosition && indices[position].index == current) { + position++ + offset = 0 + continue + } node := doc.Nodes[indices[position].index] if named_link_label(node.Snapshot) != "" && (command == CaretLineStart || command == CaretLineEnd) { offset = 0 diff --git a/internal/review/document_caret_test.go b/internal/review/document_caret_test.go index 76fa329..03bb304 100644 --- a/internal/review/document_caret_test.go +++ b/internal/review/document_caret_test.go @@ -335,7 +335,7 @@ func TestDocumentCaretNamedLinkCharacterBoundaries(t *testing.T) { } worker.SyncFocus(webID("/text")) for index, want := range []string{"猫", " "} { - result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextCharacter, 0, 0) if result.Err != nil || result.CaretErr != nil || result.DocumentCaretText != want || result.DocumentPosition.Offset != index+1 || navigator.offset != 0 || navigator.position != result.DocumentPosition.Object { t.Fatalf("label offset changed native anchor or lost review progress: %+v; native offset=%d", result, navigator.offset) @@ -560,3 +560,76 @@ func TestDocumentCaretContinuesFromStructuralHeading(t *testing.T) { t.Fatalf("Ctrl+Home = %+v, %v", start, err) } } + +// A heading whose text lives in an embedded link child (YouTube cards): +// structural navigation already announced the whole heading, so Down must +// leave the heading instead of landing on its link and repeating the title. +func TestDocumentCaretContinuesFromStructuralLinkedHeading(t *testing.T) { + reader := caretTextReader{ + webID("/first_text"): "First heading", + webID("/old_text"): "Old position", + webID("/second"): "\ufffc", + webID("/second_link"): "Second heading", + webID("/next_text"): "Text after second heading", + } + node := func(path, role string, children ...string) atspi.TreeNode { + snapshot := atspi.NewSnapshot(path, path, role, "", "") + snapshot.BusName = webID(path).BusName + snapshot.CharacterCount = utf8.RuneCountInString(reader[webID(path)]) + if role == "heading" { + snapshot.HeadingLevel = 2 + } + result := atspi.TreeNode{Snapshot: snapshot} + for _, child := range children { + result.Children = append(result.Children, webID(child)) + } + return result + } + second := node("/second", "heading", "/second_link") + second.Snapshot.TextChildren = []atspi.TextChild{{Offset: 0, Child: webID("/second_link")}} + // Chromium exposes the card link as an explicitly named whole stop with + // no native text of its own (native_text_end=0 in the live log). + link := node("/second_link", "link") + link.Snapshot.Name = "Second heading" + link.Snapshot.ExplicitName = true + link.Snapshot.CharacterCount = 0 + link.Snapshot.Focusable = true + tree, err := atspi.NewAccessibleTree(webID("/root"), []atspi.TreeNode{ + node("/root", "frame", "/doc"), + node("/doc", "document web", "/first", "/old", "/second", "/next"), + node("/first", "heading", "/first_text"), + node("/first_text", "static"), + node("/old", "paragraph", "/old_text"), + node("/old_text", "static"), + second, link, + node("/next", "paragraph", "/next_text"), + node("/next_text", "static"), + }) + 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("/old_text")); err != nil { + t.Fatal(err) + } + jump, err := cursor.Move(ctx, DocumentHeading, 0, false) + if err != nil || jump.Position.Object.ID() != webID("/second") { + t.Fatalf("heading jump = %+v, %v", jump, err) + } + line, err := cursor.MoveCaret(ctx, reader, CaretNextLine) + if err != nil || !line.Moved || line.Text != "Text after second heading" || line.Position.Object.ID() != webID("/next_text") { + t.Fatalf("Down after linked heading = %+v, %v; want the following line, not the heading link again", line, err) + } + jump, err = cursor.Move(ctx, DocumentHeading, 0, true) + if err != nil || jump.Position.Object.ID() != webID("/second") { + t.Fatalf("heading jump back = %+v, %v", jump, err) + } + line, err = cursor.MoveCaret(ctx, reader, CaretPreviousLine) + if err != nil || !line.Moved || line.Text != "Old position" || line.Position.Object.ID() != webID("/old_text") { + t.Fatalf("Up after linked heading = %+v, %v", line, err) + } +} diff --git a/internal/review/document_native_caret_test.go b/internal/review/document_native_caret_test.go index 66c8314..d2bd17e 100644 --- a/internal/review/document_native_caret_test.go +++ b/internal/review/document_native_caret_test.go @@ -103,7 +103,7 @@ func TestWorkerNativeCaretHandoffPrecedesNextArrow(t *testing.T) { if syncResult.Err != nil || syncResult.DocumentPosition.Offset != 11 { t.Fatalf("handoff = %+v", syncResult) } - result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0, 0) if result.Err != nil || result.DocumentPosition.Offset != 12 || result.DocumentCaretText != "p" || navigator.offset != 12 { t.Fatalf("move after selection = %+v; native offset = %d", result, navigator.offset) } diff --git a/internal/review/document_navigation_reader.go b/internal/review/document_navigation_reader.go index fe54aaf..08a3ee4 100644 --- a/internal/review/document_navigation_reader.go +++ b/internal/review/document_navigation_reader.go @@ -26,7 +26,7 @@ func (reader *navigationTextReader) ReadDocumentCaret(ctx context.Context, objec if !found || reader.document.Nodes[index].Reference != object { return atspi.DocumentCaretText{}, atspi.ErrStaleObject } - if err := reader.tree.ValidateDocumentRange(ctx, reader.document, reader.anchor, index); err != nil { + if err := reader.tree.ValidateDocumentCaretRange(ctx, reader.document, reader.anchor, index); err != nil { return atspi.DocumentCaretText{}, err } text, err := reader.DocumentCaretReader.ReadDocumentCaret(ctx, object, offset, unit) @@ -57,7 +57,7 @@ func (reader *navigationCharacterReader) ReadDocumentCharacters(ctx context.Cont if !found || reader.document.Nodes[index].Reference != object { return atspi.DocumentCaretText{}, atspi.ErrStaleObject } - if err := reader.tree.ValidateDocumentRange(ctx, reader.document, reader.anchor, index); err != nil { + if err := reader.tree.ValidateDocumentCaretRange(ctx, reader.document, reader.anchor, index); err != nil { return atspi.DocumentCaretText{}, err } return reader.source.ReadDocumentCharacters(ctx, object, offset) diff --git a/internal/review/document_navigation_test.go b/internal/review/document_navigation_test.go index 6504934..b1bac4a 100644 --- a/internal/review/document_navigation_test.go +++ b/internal/review/document_navigation_test.go @@ -12,19 +12,29 @@ import ( "git.stormux.org/storm/cthulhu/internal/atspi" ) +// Text and geometry staleness used to force a recapture before a caret move +// could commit onto the dirty target. On constantly ticking pages the +// target (and everything between caret and target) is re-invalidated faster +// than capture-and-retry can run, which trapped the caret outright. Caret +// moves read text and line bounds live, so the live read now wins for those +// two; semantic snapshot staleness (names are spoken from the snapshot) +// still refreshes first. func TestDocumentCaretDefersUnrelatedUpdatesUntilTraversed(t *testing.T) { for _, test := range []struct { name string invalidate func(*atspi.AccessibleTree) want error }{ - {"text", func(tree *atspi.AccessibleTree) { tree.InvalidateText(webID("/next")) }, atspi.ErrStaleText}, + {"text", func(tree *atspi.AccessibleTree) { tree.InvalidateText(webID("/next")) }, nil}, {"semantics", func(tree *atspi.AccessibleTree) { tree.InvalidateSnapshot(webID("/next")) }, atspi.ErrStaleSnapshot}, - {"geometry", func(tree *atspi.AccessibleTree) { tree.InvalidateGeometry(webID("/next")) }, atspi.ErrStaleGeometry}, + {"geometry", func(tree *atspi.AccessibleTree) { tree.InvalidateGeometry(webID("/next")) }, nil}, } { t.Run(test.name, func(t *testing.T) { reader := caretTextReader{webID("/text"): "Alpha", webID("/next"): "Next"} cursor := newCaretTextCursor(t, reader) + // The provider text moves on after capture; the live read must + // be what the move reports, not the captured length or text. + reader[webID("/next")] = "Next updated" test.invalidate(cursor.tree) if _, err := cursor.SyncFocus(context.Background(), webID("/text")); err != nil { t.Fatal("unrelated update blocked focus synchronization", err) @@ -33,7 +43,13 @@ func TestDocumentCaretDefersUnrelatedUpdatesUntilTraversed(t *testing.T) { if err != nil || !move.Moved || move.Text != "l" { t.Fatalf("local character move: %+v %v", move, err) } - _, err = cursor.MoveCaret(context.Background(), reader, CaretNextLine) + move, err = cursor.MoveCaret(context.Background(), reader, CaretNextLine) + if test.want == nil { + if err != nil || !move.Moved || move.Text != "Next updated" || move.Position.Object.ID() != webID("/next") { + t.Fatalf("live target read: %+v %v, want Next updated", move, err) + } + return + } if !errors.Is(err, test.want) { t.Fatalf("dirty target read: %v, want %v", err, test.want) } @@ -44,6 +60,9 @@ func TestDocumentCaretDefersUnrelatedUpdatesUntilTraversed(t *testing.T) { } } +// Crossing now follows captured child order with generation-checked +// endpoints and live reads; structural navigation still refreshes stale +// child order before promising the next target. func TestDocumentCaretKeepsLocalTextThroughParentInsertion(t *testing.T) { reader := caretTextReader{webID("/text"): "Alpha", webID("/next"): "Next"} cursor := newCaretTextCursor(t, reader) @@ -52,8 +71,9 @@ func TestDocumentCaretKeepsLocalTextThroughParentInsertion(t *testing.T) { if err != nil || !move.Moved || move.Text != "l" { t.Fatalf("parent insertion blocked live text: %+v %v", move, err) } - if _, err := cursor.MoveCaret(context.Background(), reader, CaretNextLine); !errors.Is(err, atspi.ErrIncompleteTree) { - t.Fatal("crossed stale child order", err) + move, err = cursor.MoveCaret(context.Background(), reader, CaretNextLine) + if err != nil || !move.Moved || move.Text != "Next" || move.Position.Object.ID() != webID("/next") { + t.Fatalf("crossing changed child order: %+v %v", move, err) } } diff --git a/internal/review/document_replacement.go b/internal/review/document_replacement.go index 3fb233d..1d35323 100644 --- a/internal/review/document_replacement.go +++ b/internal/review/document_replacement.go @@ -8,28 +8,43 @@ import "git.stormux.org/storm/cthulhu/internal/atspi" // Orca recovers removed caret contexts from the surviving parent. Here the // bounded case is one plain text leaf between the same adjacent siblings. -// Retain only its parent and two generation-checked boundary references; -// labels and child indices alone cannot establish the old reading position. +// Retain its parent and two generation-checked boundary references. When a +// ticking page recreates every child at once, those boundaries retire too; +// the child's index and the parent's child count, checked against the same +// generation-checked parent, then distinguish recreation from insertion. +// Labels alone still cannot establish the old reading position, and neither +// recovery is ever used for activation. func remember_text_parent(doc atspi.DocumentSnapshot, place *documentPlace) { place.textParent = atspi.ObjectReference{} place.textPrevious, place.textNext = atspi.ObjectReference{}, atspi.ObjectReference{} + place.textChildIndex, place.textChildCount = 0, 0 + place.parent = atspi.ObjectReference{} + place.parentPrevious, place.parentNext = atspi.ObjectReference{}, atspi.ObjectReference{} + place.parentChildIndex, place.parentChildCount = 0, 0 place.recoveryParents = nil node, ok := doc.Node(place.object.ID()) if !ok || node.Reference != place.object { return } plainText := replacement_text_leaf(node) + firstParent := true for parentID := node.Parent; parentID.Valid(); { parent, ok := doc.Node(parentID) if !ok { break } + if firstParent { + firstParent = false + remember_child_slot(doc, place, parent, &place.parent, &place.parentPrevious, &place.parentNext, + &place.parentChildIndex, &place.parentChildCount) + } if len(place.recoveryParents) == 0 && plainText && replacement_text_parent(parent) { place.textParent = parent.Reference for childIndex, id := range parent.Children { if id != place.object.ID() { continue } + place.textChildIndex, place.textChildCount = childIndex, len(parent.Children) if childIndex > 0 { if sibling, ok := doc.Node(parent.Children[childIndex-1]); ok { place.textPrevious = sibling.Reference @@ -42,6 +57,7 @@ func remember_text_parent(doc atspi.DocumentSnapshot, place *documentPlace) { } if childIndex > 0 && !place.textPrevious.Valid() || childIndex+1 < len(parent.Children) && !place.textNext.Valid() { place.textParent = atspi.ObjectReference{} + place.textChildIndex, place.textChildCount = 0, 0 } break } @@ -73,9 +89,9 @@ func survivingDocumentParent(doc atspi.DocumentSnapshot, saved documentPlace) (a return atspi.DocumentNode{}, false } -func replacement_text_node(doc atspi.DocumentSnapshot, saved documentPlace) (atspi.DocumentNode, bool) { +func replacement_text_node(doc atspi.DocumentSnapshot, saved documentPlace) (atspi.DocumentNode, string, bool) { if saved.document != doc.Context.TopLevel || !saved.textParent.Valid() || saved.origin != CaretNavigation { - return atspi.DocumentNode{}, false + return atspi.DocumentNode{}, "", false } byID := make(map[atspi.ObjectID]*atspi.DocumentNode, len(doc.Nodes)) for index := range doc.Nodes { @@ -104,11 +120,132 @@ func replacement_text_node(doc atspi.DocumentSnapshot, saved documentPlace) (ats } child := byID[id] if previous == saved.textPrevious && next == saved.textNext && child != nil && replacement_text_leaf(*child) { - return *child, true + return *child, "replaced-text-child", true } } + // Every sibling reference retired together: the page recreated the + // parent's children in place. Same child count and index under the + // same generation-checked parent is the same navigation line. A + // changed count stays ambiguous and falls back to the parent, as + // does any saved sibling that still lives: a reorder or a single + // replaced neighbor is a structural change, not a recreation. + siblingsRetired := true + for _, sibling := range []atspi.ObjectReference{saved.textPrevious, saved.textNext} { + if !sibling.Valid() { + continue + } + if node, ok := byID[sibling.ID()]; ok && node.Reference == sibling { + siblingsRetired = false + } + } + if siblingsRetired && saved.textChildCount > 0 && len(parent.Children) == saved.textChildCount && + saved.textChildIndex >= 0 && saved.textChildIndex < len(parent.Children) { + if child := byID[parent.Children[saved.textChildIndex]]; child != nil && replacement_text_leaf(*child) { + return *child, "replaced-text-child-index", true + } + } + return atspi.DocumentNode{}, "", false } - return atspi.DocumentNode{}, false + return atspi.DocumentNode{}, "", false +} + +// remember_child_slot records a child's slot under its immediate parent: +// the generation-checked parent, adjacent sibling references, and index +// and count. Any unresolved boundary clears the slot; indices alone are +// never enough evidence on their own. +func remember_child_slot(doc atspi.DocumentSnapshot, place *documentPlace, parent atspi.DocumentNode, + slotParent, slotPrevious, slotNext *atspi.ObjectReference, slotIndex, slotCount *int) { + for childIndex, id := range parent.Children { + if id != place.object.ID() { + continue + } + *slotParent = parent.Reference + *slotIndex, *slotCount = childIndex, len(parent.Children) + if childIndex > 0 { + if sibling, ok := doc.Node(parent.Children[childIndex-1]); ok { + *slotPrevious = sibling.Reference + } + } + if childIndex+1 < len(parent.Children) { + if sibling, ok := doc.Node(parent.Children[childIndex+1]); ok { + *slotNext = sibling.Reference + } + } + if childIndex > 0 && !slotPrevious.Valid() || childIndex+1 < len(parent.Children) && !slotNext.Valid() { + *slotParent = atspi.ObjectReference{} + *slotIndex, *slotCount = 0, 0 + } + return + } +} + +// replacement_child_node generalizes the replaced-text-child recovery to +// any child (recreated button rows, containers) under the same bounded +// evidence: either the generation-checked sibling boundaries still match, +// or every sibling retired together with the child count unchanged, so +// the same index under the same surviving parent is the recreation. Orca +// recovers removed children the same way (parent plus removed index); +// activation never uses this fallback. +func replacement_child_node(doc atspi.DocumentSnapshot, saved documentPlace) (atspi.DocumentNode, string, bool) { + if saved.document != doc.Context.TopLevel || !saved.parent.Valid() || saved.origin == NativeNavigation { + return atspi.DocumentNode{}, "", false + } + // Plain text leaves keep their stricter replacement rules above; the + // generic slot must not widen them for the same parent. + if saved.textParent.Valid() && saved.parent == saved.textParent { + return atspi.DocumentNode{}, "", false + } + byID := make(map[atspi.ObjectID]*atspi.DocumentNode, len(doc.Nodes)) + for index := range doc.Nodes { + node := &doc.Nodes[index] + byID[node.Reference.ID()] = node + } + parent, ok := doc.Node(saved.parent.ID()) + if !ok || parent.Reference != saved.parent || len(parent.Children) == 0 || + parent.Hidden || parent.ParentEditable || parent.Snapshot.Editable || + parent.Snapshot.Focusable || parent.Snapshot.SupportsClick { + return atspi.DocumentNode{}, "", false + } + usable := func(child *atspi.DocumentNode) bool { + return child != nil && !child.Hidden && !child.ParentEditable && !child.Snapshot.Editable && + child.Snapshot.Role != "password text" + } + for index, id := range parent.Children { + var previous, next atspi.ObjectReference + if index > 0 { + sibling := byID[parent.Children[index-1]] + if sibling == nil { + continue + } + previous = sibling.Reference + } + if index+1 < len(parent.Children) { + sibling := byID[parent.Children[index+1]] + if sibling == nil { + continue + } + next = sibling.Reference + } + if child := byID[id]; previous == saved.parentPrevious && next == saved.parentNext && usable(child) { + return *child, "replaced-child", true + } + } + siblingsRetired := true + for _, sibling := range []atspi.ObjectReference{saved.parentPrevious, saved.parentNext} { + if !sibling.Valid() { + continue + } + if node, ok := byID[sibling.ID()]; ok && node.Reference == sibling { + siblingsRetired = false + } + } + if siblingsRetired && saved.parentChildCount > 0 && len(parent.Children) == saved.parentChildCount && + saved.parentChildIndex >= 0 && saved.parentChildIndex < len(parent.Children) { + if child := byID[parent.Children[saved.parentChildIndex]]; usable(child) { + return *child, "replaced-child-index", true + } + } + return atspi.DocumentNode{}, "", false } func replacement_text_parent(node atspi.DocumentNode) bool { diff --git a/internal/review/document_replacement_test.go b/internal/review/document_replacement_test.go index 241af55..e0b2938 100644 --- a/internal/review/document_replacement_test.go +++ b/internal/review/document_replacement_test.go @@ -82,6 +82,158 @@ func TestDocumentCaretRecoversSingleReplacedTextChild(t *testing.T) { } } +// A ticking page (incremental game) recreates every text leaf under a +// surviving parent at once: sibling identities all change, but child count +// and order do not. Recovery must restore the same line so the next caret +// move progresses instead of restarting at the parent's first line forever. +func TestDocumentCaretRecoversRecreatedTextSiblings(t *testing.T) { + ctx := context.Background() + reader := caretTextReader{ + webID("/l1a"): "Counter one 100", webID("/l2a"): "Counter two 200", webID("/l3a"): "Counter three 300", + webID("/l1b"): "Counter one 101", webID("/l2b"): "Counter two 201", webID("/l3b"): "Counter three 301", + webID("/l1c"): "Counter one 102", webID("/l2c"): "Counter two 202", webID("/l3c"): "Counter three 302", + } + node := func(path, role string, children ...atspi.ObjectID) atspi.TreeNode { + return atspi.TreeNode{Snapshot: atspi.Snapshot{BusName: webID(path).BusName, Path: path, Role: role, + CharacterCount: utf8.RuneCountInString(reader[webID(path)])}, Children: children} + } + generation := func(suffix string) []atspi.TreeNode { + return []atspi.TreeNode{ + node("/doc", "document web", webID("/paragraph")), + node("/paragraph", "paragraph", webID("/l1"+suffix), webID("/l2"+suffix), webID("/l3"+suffix)), + node("/l1"+suffix, "static"), node("/l2"+suffix, "static"), node("/l3"+suffix, "static"), + } + } + tree, err := atspi.NewAccessibleTree(webID("/doc"), generation("a")) + if err != nil { + t.Fatal(err) + } + cursor, _ := NewDocumentCursor(tree) + if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil { + t.Fatal(err) + } + if move, err := cursor.MoveCaret(ctx, reader, CaretNextLine); err != nil || move.Text != "Counter one 100" { + t.Fatalf("first line = %+v, %v", move, err) + } + if move, err := cursor.MoveCaret(ctx, reader, CaretNextLine); err != nil || move.Text != "Counter two 200" || + move.Position.Object.ID() != webID("/l2a") { + t.Fatalf("second line = %+v, %v", move, err) + } + if err := tree.Replace(webID("/doc"), generation("b")); err != nil { + t.Fatal(err) + } + position, recovery, err := cursor.sync_focus(ctx, webID("/doc")) + if err != nil { + t.Fatal(err) + } + if position.Object.ID() != webID("/l2b") { + t.Fatalf("recovery = %q onto %v, want recreated second line /l2b", recovery, position.Object.ID()) + } + if move, err := cursor.MoveCaret(ctx, reader, CaretNextLine); err != nil || !move.Moved || move.Text != "Counter three 301" { + t.Fatalf("line after tick = %+v, %v; caret did not progress", move, err) + } + if err := tree.Replace(webID("/doc"), generation("c")); err != nil { + t.Fatal(err) + } + if _, _, err := cursor.sync_focus(ctx, webID("/doc")); err != nil { + t.Fatal(err) + } + if move, err := cursor.MoveCaret(ctx, reader, CaretNextLine); err != nil || move.Moved { + t.Fatalf("past last ticking line = %+v, %v; want boundary, not a repeat", move, err) + } +} + +// Every visible counter on a ticking page marks its text stale on each game +// tick. A caret move reads its target line live, so text staleness between +// caret and target (or on the current line) must not by itself force a +// recapture-and-retry cycle that a faster tick can always outrun. +// The text-leaf index recovery does not cover recreated controls: a game +// can rebuild a whole button row at once. The same bounded evidence (same +// generation-checked parent, every sibling retired, same child count) +// re-resolves the caret's button so the next move progresses instead of +// restarting at the row's container. +func TestDocumentCaretRecoversRecreatedChildByIndex(t *testing.T) { + ctx := context.Background() + reader := caretTextReader{} + node := func(path, role, name string, children ...atspi.ObjectID) atspi.TreeNode { + snapshot := atspi.Snapshot{BusName: webID(path).BusName, Path: path, Role: role, Name: name} + if role == "button" { + snapshot.Focusable = true + } + return atspi.TreeNode{Snapshot: snapshot, Children: children} + } + generation := func(suffix string) []atspi.TreeNode { + return []atspi.TreeNode{ + node("/doc", "document web", "", webID("/section")), + node("/section", "section", "", webID("/b1"+suffix), webID("/b2"+suffix), webID("/b3"+suffix)), + node("/b1"+suffix, "button", "Buy one"), node("/b2"+suffix, "button", "Buy two"), node("/b3"+suffix, "button", "Buy three"), + } + } + tree, err := atspi.NewAccessibleTree(webID("/doc"), generation("a")) + if err != nil { + t.Fatal(err) + } + cursor, _ := NewDocumentCursor(tree) + if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil { + t.Fatal(err) + } + for _, want := range []string{"/b1a", "/b2a"} { + move, err := cursor.MoveCaret(ctx, reader, CaretNextLine) + if err != nil || !move.Moved || move.Position.Object.ID() != webID(want) { + t.Fatalf("move onto %s = %+v, %v", want, move, err) + } + } + if err := tree.Replace(webID("/doc"), generation("b")); err != nil { + t.Fatal(err) + } + position, recovery, err := cursor.sync_focus(ctx, webID("/doc")) + if err != nil { + t.Fatal(err) + } + if position.Object.ID() != webID("/b2b") { + t.Fatalf("recovery = %q onto %v, want recreated middle button /b2b", recovery, position.Object.ID()) + } + move, err := cursor.MoveCaret(ctx, reader, CaretNextLine) + if err != nil || !move.Moved || move.Position.Object.ID() != webID("/b3b") { + t.Fatalf("move after recreation = %+v, %v; caret did not progress", move, err) + } +} + +func TestDocumentCaretMoveCrossesTextInvalidatedLines(t *testing.T) { + ctx := context.Background() + reader := caretTextReader{ + webID("/first"): "Counter first 100", webID("/second"): "Counter second 200", + } + node := func(path, role string, children ...atspi.ObjectID) atspi.TreeNode { + return atspi.TreeNode{Snapshot: atspi.Snapshot{BusName: webID(path).BusName, Path: path, Role: role, + CharacterCount: utf8.RuneCountInString(reader[webID(path)])}, Children: children} + } + tree, err := atspi.NewAccessibleTree(webID("/doc"), []atspi.TreeNode{ + node("/doc", "document web", webID("/paragraph")), + node("/paragraph", "paragraph", webID("/first"), webID("/second")), + node("/first", "static"), node("/second", "static"), + }) + if err != nil { + t.Fatal(err) + } + cursor, _ := NewDocumentCursor(tree) + if _, err := cursor.SyncFocus(ctx, webID("/doc")); err != nil { + t.Fatal(err) + } + if move, err := cursor.MoveCaret(ctx, reader, CaretNextLine); err != nil || move.Text != "Counter first 100" { + t.Fatalf("first line = %+v, %v", move, err) + } + tree.InvalidateText(webID("/first")) + tree.InvalidateText(webID("/second")) + if _, _, err := cursor.sync_focus(ctx, webID("/doc")); err != nil { + t.Fatalf("sync across ticking text: %v", err) + } + move, err := cursor.MoveCaret(ctx, reader, CaretNextLine) + if err != nil || !move.Moved || move.Text != "Counter second 200" { + t.Fatalf("move across ticking text = %+v, %v", move, err) + } +} + func TestDocumentRecoveryKeepsVirtualPlaceWhenNativeAnchorDisappears(t *testing.T) { ctx := context.Background() reader := caretTextReader{webID("/text"): "First paragraph", webID("/next"): "Selected paragraph"} diff --git a/internal/review/document_requests.go b/internal/review/document_requests.go index 60a85a5..0cd94e6 100644 --- a/internal/review/document_requests.go +++ b/internal/review/document_requests.go @@ -23,12 +23,23 @@ import ( "time" ) +// WarmDocument captures and syncs the currently focused document in the +// background so the first navigation key does not pay for a cold capture. +// Best-effort: a full queue or a focus outside web content is ignored by +// the worker, and no result is delivered. +func (worker *Worker) WarmDocument() { + select { + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), warmDocument: true, queuedAt: time.Now()}: + default: + } +} + func (worker *Worker) MoveDocumentCaret(command DocumentCaretCommand) error { if !command.valid() { return ErrInvalidDirection } select { - case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), caretCommand: command, queuedAt: time.Now()}: + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), caretCommand: command, queuedAt: time.Now()}: return nil default: return ErrRequestQueueFull @@ -37,7 +48,7 @@ func (worker *Worker) MoveDocumentCaret(command DocumentCaretCommand) error { func (worker *Worker) ActivateDocumentCaret() error { select { - case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), activate: true, queuedAt: time.Now()}: + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), activate: true, queuedAt: time.Now()}: return nil default: return ErrRequestQueueFull @@ -49,7 +60,7 @@ func (worker *Worker) ReadAll(ctx context.Context) error { return errors.New("read all: nil context") } select { - case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), readAll: ctx, queuedAt: time.Now()}: + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), readAll: ctx, queuedAt: time.Now()}: return nil default: return ErrRequestQueueFull @@ -61,7 +72,7 @@ func (worker *Worker) JumpToDocumentFragment(fragment string) error { return ErrInvalidDirection } select { - case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), fragment: fragment, queuedAt: time.Now()}: + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), fragment: fragment, queuedAt: time.Now()}: return nil default: return ErrRequestQueueFull diff --git a/internal/review/document_table.go b/internal/review/document_table.go index a5df0ee..0534418 100644 --- a/internal/review/document_table.go +++ b/internal/review/document_table.go @@ -25,7 +25,7 @@ func (cursor *DocumentCursor) moveTable(ctx context.Context, doc atspi.DocumentS } table := doc.Nodes[tableIndex] tableEnd := doc.SubtreeEnd(tableIndex) - if err := cursor.tree.ValidateDocumentRange(ctx, doc, tableIndex, tableEnd); err != nil { + if err := cursor.tree.ValidateDocumentCaretRange(ctx, doc, tableIndex, tableEnd); err != nil { return DocumentCaretMove{}, err } var current atspi.DocumentNode diff --git a/internal/review/document_worker.go b/internal/review/document_worker.go index c19b6bb..2b7d853 100644 --- a/internal/review/document_worker.go +++ b/internal/review/document_worker.go @@ -31,14 +31,177 @@ func (worker *Worker) MoveDocument(target DocumentTarget, level int, backwards b return ErrInvalidDirection } select { - case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), target: target, backwards: backwards, level: level, queuedAt: time.Now()}: + case worker.requests <- request{documentEpoch: worker.documentEpoch.Load(), documentLoadEpoch: worker.documentLoadEpoch.Load(), target: target, backwards: backwards, level: level, queuedAt: time.Now()}: return nil default: return ErrRequestQueueFull } } -func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, target DocumentTarget, level int, backwards bool, epoch uint64) (Result, *DocumentCursor) { +// sameDocumentRepeat reports whether two queued requests are the same +// structural move in the same document generation, so they can execute as +// one batch in order. Caret steps are excluded: every queued character, +// word and line step keeps its own ordered result. +func sameDocumentRepeat(first, next request) bool { + if first.documentEpoch != next.documentEpoch { + return false + } + return first.target != 0 && first.target == next.target && + first.level == next.level && first.backwards == next.backwards +} + +const maxRetainedDocumentCursors = 4 + +// cursorForTree returns the retained browse cursor for a captured tree, +// creating and retaining one on first use. The adapter retains one tree +// object per document root, so the pointer identifies the document. +func (worker *Worker) cursorForTree(tree *atspi.AccessibleTree, loadEpoch uint64) (*DocumentCursor, bool, error) { + if tree == nil { + return nil, false, errors.New("document cursor: nil tree") + } + if cursor := worker.documentCursors[tree]; cursor != nil && worker.documentCursorEpochs[tree] == loadEpoch { + worker.touchDocumentCursor(tree) + return cursor, false, nil + } + cursor, err := NewDocumentCursor(tree) + if err != nil { + return nil, false, err + } + worker.rememberDocumentCursor(cursor, loadEpoch) + return cursor, true, nil +} + +func (worker *Worker) rememberDocumentCursor(cursor *DocumentCursor, loadEpoch uint64) { + if cursor == nil || cursor.tree == nil { + return + } + if worker.documentCursors == nil { + worker.documentCursors = make(map[*atspi.AccessibleTree]*DocumentCursor) + worker.documentCursorEpochs = make(map[*atspi.AccessibleTree]uint64) + } + worker.documentCursors[cursor.tree] = cursor + worker.documentCursorEpochs[cursor.tree] = loadEpoch + worker.touchDocumentCursor(cursor.tree) + for len(worker.documentCursorOrder) > maxRetainedDocumentCursors { + oldest := worker.documentCursorOrder[0] + delete(worker.documentCursors, oldest) + delete(worker.documentCursorEpochs, oldest) + worker.documentCursorOrder = worker.documentCursorOrder[1:] + } +} + +func (worker *Worker) touchDocumentCursor(tree *atspi.AccessibleTree) { + for index, retained := range worker.documentCursorOrder { + if retained == tree { + worker.documentCursorOrder = append(worker.documentCursorOrder[:index], worker.documentCursorOrder[index+1:]...) + break + } + } + worker.documentCursorOrder = append(worker.documentCursorOrder, tree) +} + +// retainedCursorsExcept lists retained cursors most recently used first. +func (worker *Worker) retainedCursorsExcept(current *DocumentCursor, loadEpoch uint64) []*DocumentCursor { + cursors := make([]*DocumentCursor, 0, len(worker.documentCursorOrder)) + for index := len(worker.documentCursorOrder) - 1; index >= 0; index-- { + tree := worker.documentCursorOrder[index] + if cursor := worker.documentCursors[tree]; cursor != nil && cursor != current && worker.documentCursorEpochs[tree] == loadEpoch { + cursors = append(cursors, cursor) + } + } + return cursors +} + +// trialRetainedCursors retries move on retained cursors for other +// documents before paying for a capture. Returning to a tab or workspace +// usually validates against its retained tree with no provider work at +// all; a cursor whose tree changed meanwhile simply falls through to the +// caller's capture path. +func (worker *Worker) trialRetainedCursors(cursor **DocumentCursor, loadEpoch uint64, result *Result, move func() error) { + if !documentNeedsCapture(result.Err) { + return + } + for _, retained := range worker.retainedCursorsExcept(*cursor, loadEpoch) { + *cursor = retained + result.Err = move() + if !documentNeedsCapture(result.Err) { + return + } + } +} + +// warmDocument captures and syncs the focused document so the first +// navigation key afterwards is warm. It returns the cursor to keep as the +// active document; failures (including focus in browser chrome) keep the +// previous cursor and stay silent. +func (worker *Worker) warmDocument(ctx context.Context, cursor *DocumentCursor, loadEpoch uint64) *DocumentCursor { + focus := worker.focus() + if !focus.Valid() { + return cursor + } + candidates := worker.retainedCursorsExcept(cursor, loadEpoch) + if cursor != nil { + candidates = append([]*DocumentCursor{cursor}, candidates...) + } + for _, candidate := range candidates { + if _, err := candidate.SyncFocus(ctx, focus); err == nil { + worker.touchDocumentCursor(candidate.tree) + return candidate + } + } + // Warm-up is speculative: never let it occupy the provider thread for + // the full interactive capture budget on an unresponsive document. + warmContext, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + tree, _, err := worker.captureDocumentForMove(warmContext, cursor, focus) + if err != nil || tree == nil { + return cursor + } + warm, _, err := worker.cursorForTree(tree, loadEpoch) + if err != nil { + return cursor + } + if _, err := warm.SyncFocus(ctx, focus); err != nil { + return cursor + } + return warm +} + +// captureDocumentForMove captures through the focus anchor. When the page +// destroyed that anchor, capture keyed on it fails with ErrObjectNotFound on +// every keypress; fall back to the cursor's saved document so the existing +// anchor-recovery path can rebind focus within the same generation. +func (worker *Worker) captureDocumentForMove(ctx context.Context, cursor *DocumentCursor, focus atspi.ObjectID) (*atspi.AccessibleTree, time.Duration, error) { + capture := func(anchor atspi.ObjectID) (*atspi.AccessibleTree, time.Duration, error) { + captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) + defer cancel() + started := time.Now() + tree, err := worker.capturer.CaptureDocument(captureContext, anchor) + return tree, time.Since(started), err + } + tree, duration, err := capture(focus) + if err == nil || !errors.Is(err, atspi.ErrObjectNotFound) { + return tree, duration, err + } + anchor, ok := cursor.capture_anchor(focus) + if !ok { + return tree, duration, err + } + fallbackTree, fallbackDuration, fallbackErr := capture(anchor) + if fallbackErr != nil { + return tree, duration + fallbackDuration, err + } + return fallbackTree, duration + fallbackDuration, nil +} + +func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, target DocumentTarget, level int, backwards bool, epoch, loadEpoch uint64) (Result, *DocumentCursor) { + return worker.moveDocumentRepeated(ctx, cursor, target, level, backwards, epoch, loadEpoch, 1) +} + +// moveDocumentRepeated executes coalesced identical keys as one batch: one +// focus sync, one capture cycle and one native placement for the final +// position, landing the full step count from the original position. +func (worker *Worker) moveDocumentRepeated(ctx context.Context, cursor *DocumentCursor, target DocumentTarget, level int, backwards bool, epoch, loadEpoch uint64, repeat int) (Result, *DocumentCursor) { result := Result{DocumentEpoch: epoch, DocumentTarget: target, DocumentBackwards: backwards, DocumentLevel: level, DocumentFocus: worker.focus()} if !result.DocumentFocus.Valid() { result.Err = ErrNoCurrent @@ -57,13 +220,27 @@ func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, return err } attempt.Stage = "move" - move, err := cursor.Move(ctx, target, level, backwards) - result.DocumentPosition = move.Position - result.Moved = move.Moved - result.DocumentWrapped = move.Wrapped - return err + moved := false + for range repeat { + move, err := cursor.Move(ctx, target, level, backwards) + if err != nil { + if moved { + return nil + } + return err + } + result.DocumentPosition = move.Position + result.Moved = move.Moved + result.DocumentWrapped = result.DocumentWrapped || move.Wrapped + if !move.Moved { + break + } + moved = true + } + return nil } result.Err = move() + worker.trialRetainedCursors(&cursor, loadEpoch, &result, move) if !documentNeedsCapture(result.Err) { return worker.placeDocumentWithRetry(ctx, cursor, result), cursor } @@ -71,11 +248,8 @@ func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, // allowing a constantly mutating page to loop indefinitely. result.CaptureReason = result.Err result.DocumentDiagnostics.CaptureAttempts++ - captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) - captureStarted := time.Now() - tree, err := worker.capturer.CaptureDocument(captureContext, result.DocumentFocus) - result.CaptureDuration = time.Since(captureStarted) - cancel() + tree, captureDuration, err := worker.captureDocumentForMove(ctx, cursor, result.DocumentFocus) + result.CaptureDuration = captureDuration if err != nil { result.Err = err return result, cursor @@ -86,8 +260,9 @@ func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, } recovered := worker.recoverCapturedDocumentFocus(cursor, tree, &result) if cursor == nil || cursor.tree != tree { - result.DocumentDiagnostics.CursorRecreated = true - cursor, _ = NewDocumentCursor(tree) + retained, created, _ := worker.cursorForTree(tree, loadEpoch) + result.DocumentDiagnostics.CursorRecreated = created + cursor = retained } if !recovered { result.DocumentFocus = worker.focus() @@ -98,15 +273,14 @@ func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, // A second replacement can race the first capture. Retry once while // the navigation has not moved, so the heading is not skipped. result.DocumentDiagnostics.CaptureAttempts++ - retryContext, retryCancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) - retryStarted := time.Now() - tree, err = worker.capturer.CaptureDocument(retryContext, result.DocumentFocus) - result.CaptureDuration += time.Since(retryStarted) - retryCancel() + var retryDuration time.Duration + tree, retryDuration, err = worker.captureDocumentForMove(ctx, cursor, result.DocumentFocus) + result.CaptureDuration += retryDuration if err == nil && tree != nil && worker.CurrentDocumentResult(result) && worker.focus() == result.DocumentFocus { if cursor == nil || cursor.tree != tree { - result.DocumentDiagnostics.CursorRecreated = true - cursor, _ = NewDocumentCursor(tree) + retained, created, _ := worker.cursorForTree(tree, loadEpoch) + result.DocumentDiagnostics.CursorRecreated = created + cursor = retained } result.Err = move() } @@ -114,7 +288,7 @@ func (worker *Worker) moveDocument(ctx context.Context, cursor *DocumentCursor, return worker.placeDocumentWithRetry(ctx, cursor, result), cursor } -func (worker *Worker) jumpDocumentFragment(ctx context.Context, cursor *DocumentCursor, fragment string, epoch uint64) (Result, *DocumentCursor) { +func (worker *Worker) jumpDocumentFragment(ctx context.Context, cursor *DocumentCursor, fragment string, epoch, loadEpoch uint64) (Result, *DocumentCursor) { result := Result{DocumentEpoch: epoch, DocumentFragment: true, DocumentFocus: worker.focus()} if !result.DocumentFocus.Valid() { result.Err = ErrNoCurrent @@ -133,7 +307,7 @@ func (worker *Worker) jumpDocumentFragment(ctx context.Context, cursor *Document result.Err = errors.New("capture document tree: nil tree") return result, cursor } - cursor, err = NewDocumentCursor(tree) + cursor, _, err = worker.cursorForTree(tree, loadEpoch) if err != nil { result.Err = err return result, cursor @@ -171,11 +345,8 @@ func (worker *Worker) placeDocumentWithRetry(ctx context.Context, cursor *Docume // capture; repeating the navigation command could skip to another target. oldPosition := result.DocumentPosition result.DocumentDiagnostics.CaptureAttempts++ - captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) - captureStarted := time.Now() - tree, err := worker.capturer.CaptureDocument(captureContext, result.DocumentFocus) - result.CaptureDuration += time.Since(captureStarted) - cancel() + tree, captureDuration, err := worker.captureDocumentForMove(ctx, cursor, result.DocumentFocus) + result.CaptureDuration += captureDuration if err != nil || tree == nil || !worker.CurrentDocumentResult(result) { return result } @@ -211,7 +382,7 @@ func documentCaretNeedsRecapture(err error) bool { errors.Is(err, atspi.ErrStaleSnapshot) || errors.Is(err, atspi.ErrDeadObject) } -func (worker *Worker) moveDocumentCaret(ctx context.Context, cursor *DocumentCursor, command DocumentCaretCommand, epoch uint64) (Result, *DocumentCursor) { +func (worker *Worker) moveDocumentCaret(ctx context.Context, cursor *DocumentCursor, command DocumentCaretCommand, epoch, loadEpoch uint64) (Result, *DocumentCursor) { result := Result{DocumentEpoch: epoch, DocumentCaretCommand: command, DocumentFocus: worker.focus()} if !result.DocumentFocus.Valid() { result.Err = ErrNoCurrent @@ -266,16 +437,14 @@ func (worker *Worker) moveDocumentCaret(ctx context.Context, cursor *DocumentCur return err } result.Err = move() + worker.trialRetainedCursors(&cursor, loadEpoch, &result, move) if !documentNeedsCapture(result.Err) { return worker.placeDocumentWithRetry(ctx, cursor, result), cursor } result.CaptureReason = result.Err result.DocumentDiagnostics.CaptureAttempts++ - captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) - captureStarted := time.Now() - tree, err := worker.capturer.CaptureDocument(captureContext, result.DocumentFocus) - result.CaptureDuration = time.Since(captureStarted) - cancel() + tree, captureDuration, err := worker.captureDocumentForMove(ctx, cursor, result.DocumentFocus) + result.CaptureDuration = captureDuration if err != nil { result.Err = err return result, cursor @@ -286,8 +455,9 @@ func (worker *Worker) moveDocumentCaret(ctx context.Context, cursor *DocumentCur } recovered := worker.recoverCapturedDocumentFocus(cursor, tree, &result) if cursor == nil || cursor.tree != tree { - result.DocumentDiagnostics.CursorRecreated = true - cursor, _ = NewDocumentCursor(tree) + retained, created, _ := worker.cursorForTree(tree, loadEpoch) + result.DocumentDiagnostics.CursorRecreated = created + cursor = retained } if !recovered { result.DocumentFocus = worker.focus() @@ -300,17 +470,16 @@ func (worker *Worker) moveDocumentCaret(ctx context.Context, cursor *DocumentCur // A dynamic page can replace the captured target between recapture and // SyncFocus. Retry once only if the first attempt did not move the caret. result.DocumentDiagnostics.CaptureAttempts++ - retryContext, retryCancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) - retryStarted := time.Now() - tree, err = worker.capturer.CaptureDocument(retryContext, result.DocumentFocus) - result.CaptureDuration += time.Since(retryStarted) - retryCancel() + var retryDuration time.Duration + tree, retryDuration, err = worker.captureDocumentForMove(ctx, cursor, result.DocumentFocus) + result.CaptureDuration += retryDuration if err != nil || tree == nil || !worker.CurrentDocumentResult(result) || worker.focus() != result.DocumentFocus { return result, cursor } if cursor == nil || cursor.tree != tree { - result.DocumentDiagnostics.CursorRecreated = true - cursor, _ = NewDocumentCursor(tree) + retained, created, _ := worker.cursorForTree(tree, loadEpoch) + result.DocumentDiagnostics.CursorRecreated = created + cursor = retained } result.Err = move() return worker.placeDocumentWithRetry(ctx, cursor, result), cursor @@ -436,6 +605,7 @@ func (worker *Worker) refresh_document_cursor(ctx context.Context, cursor *Docum if cursor != nil && len(cursor.places) > 0 { saved = cursor.places[len(cursor.places)-1] } + loadEpoch := worker.documentLoadEpoch.Load() captureContext, cancel := context.WithTimeout(ctx, worker.documentCaptureTimeout) defer cancel() tree, err := worker.capturer.CaptureDocument(captureContext, focus) @@ -473,6 +643,7 @@ func (worker *Worker) refresh_document_cursor(ctx context.Context, cursor *Docum place := &fresh.places[len(fresh.places)-1] place.object, place.offset, place.origin = target, saved.offset, saved.origin } + worker.rememberDocumentCursor(fresh, loadEpoch) return fresh, nil } diff --git a/internal/review/document_worker_test.go b/internal/review/document_worker_test.go index 6c93ec8..b04c324 100644 --- a/internal/review/document_worker_test.go +++ b/internal/review/document_worker_test.go @@ -20,6 +20,7 @@ package review import ( "context" "errors" + "fmt" "testing" "time" @@ -59,12 +60,12 @@ func TestWorkerRetriesNativeCaretPlacementWithoutRepeatingMovement(t *testing.T) navigator := &refreshingCaretNavigator{readAllCapturer: &readAllCapturer{tree: cursor.tree}, caretTextReader: text} worker, _ := NewWorker(navigator, time.Second) worker.SyncFocus(webID("/text")) - result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0, 0) if result.Err != nil || result.CaretErr != nil || result.DocumentCaretText != "i" || result.DocumentPosition.Offset != 1 || len(navigator.placements) != 2 || navigator.placements[0] != 1 || navigator.placements[1] != 1 { t.Fatalf("retry skipped or lost requested character: %+v, placements %v", result, navigator.placements) } - next, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0) + next, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0, 0) if next.Err != nil || next.DocumentCaretText != "r" || next.DocumentPosition.Offset != 2 { t.Fatalf("retry lost the saved position: %+v", next) } @@ -179,7 +180,7 @@ func TestDocumentCaretMovePlacesNativeCaretForTabTraversal(t *testing.T) { t.Fatal(err) } } - result, _ := worker.moveDocumentCaret(context.Background(), cursor, command, worker.documentEpoch.Load()) + result, _ := worker.moveDocumentCaret(context.Background(), cursor, command, worker.documentEpoch.Load(), worker.documentLoadEpoch.Load()) if result.Err != nil || result.CaretErr != nil || !result.Moved || navigator.position != result.DocumentPosition.Object || navigator.focus != webID("/text") || navigator.offset != result.DocumentPosition.Offset { t.Fatalf("command %v: result=%+v, placement=%+v", command, result, navigator) @@ -277,7 +278,7 @@ func TestDocumentCaretSpeaksReadTextWhenAncestorDisappears(t *testing.T) { t.Fatal(err) } worker.SyncFocus(webID("/text")) - result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(context.Background(), cursor, CaretNextCharacter, 0, 0) if result.Err != nil || !result.Moved || result.DocumentCaretText != "l" || result.DocumentPosition.Offset != 1 { t.Fatalf("stale ancestor after caret read: %+v", result) } @@ -305,7 +306,7 @@ func TestDocumentCaretRetriesStaleRecaptureWithoutSkipping(t *testing.T) { worker.SyncFocus(webID("/text")) done := make(chan Result, 1) go func() { - result, _ := worker.moveDocumentCaret(context.Background(), oldCursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(context.Background(), oldCursor, CaretNextCharacter, 0, 0) done <- result }() initial := <-reader.calls @@ -321,6 +322,210 @@ func TestDocumentCaretRetriesStaleRecaptureWithoutSkipping(t *testing.T) { } } +// Keys pressed while a capture is in flight queue up. Identical queued +// document moves must execute as one batch: one placement and one result, +// landing the full count of steps from the original position. +func TestWorkerCoalescesQueuedIdenticalDocumentMoves(t *testing.T) { + tree, _ := webCursor(t) + navigator := newFakeDocumentNavigator() + worker, cancel := startWorker(t, navigator, time.Second) + defer worker.Stop(context.Background()) + defer cancel() + worker.SyncFocus(webID("/first")) + if err := worker.MoveDocument(DocumentHeading, 0, false); err != nil { + t.Fatal(err) + } + initial := <-navigator.fakeCapturer.calls + // Two more identical keys arrive while the first capture is in flight. + for range 2 { + if err := worker.MoveDocument(DocumentHeading, 0, false); err != nil { + t.Fatal(err) + } + } + initial.result <- captureResult{tree: tree} + first := <-navigator.calls + if first.position.ID() != webID("/second") { + t.Fatalf("first placement = %+v", first) + } + if result := waitResult(t, worker); result.Err != nil || result.DocumentPosition.Object.ID() != webID("/second") { + t.Fatalf("first result = %+v", result) + } + second := <-navigator.calls + if second.position.ID() != webID("/second") { + t.Fatalf("batched placement = %+v, want final /second after wrap", second) + } + result := waitResult(t, worker) + if result.Err != nil || !result.Moved || !result.DocumentWrapped || result.DocumentPosition.Object.ID() != webID("/second") { + t.Fatalf("batched result = %+v", result) + } + select { + case placement := <-navigator.calls: + t.Fatalf("unexpected extra placement %+v; queued repeats were not coalesced", placement) + case extra := <-worker.Results(): + t.Fatalf("unexpected extra result %+v; queued repeats were not coalesced", extra) + default: + } +} + +type retainedDocumentCapturer struct { + trees map[atspi.ObjectID]*atspi.AccessibleTree + captures []atspi.ObjectID + placements []atspi.ObjectID +} + +func (capturer *retainedDocumentCapturer) CaptureDocument(_ context.Context, focus atspi.ObjectID) (*atspi.AccessibleTree, error) { + capturer.captures = append(capturer.captures, focus) + if tree := capturer.trees[focus]; tree != nil { + return tree, nil + } + return nil, atspi.ErrObjectNotFound +} + +func (capturer *retainedDocumentCapturer) CaptureDocumentText(ctx context.Context, focus atspi.ObjectID) (*atspi.AccessibleTree, error) { + return capturer.CaptureDocument(ctx, focus) +} + +func (capturer *retainedDocumentCapturer) CaptureReviewWindow(ctx context.Context, focus atspi.ObjectID) (*atspi.AccessibleTree, error) { + return capturer.CaptureDocument(ctx, focus) +} + +func (capturer *retainedDocumentCapturer) CaptureDesktop(context.Context) (*atspi.AccessibleTree, error) { + return nil, atspi.ErrObjectNotFound +} + +func (capturer *retainedDocumentCapturer) SetDocumentPosition(_ context.Context, position atspi.ObjectReference, _ atspi.ObjectID) error { + capturer.placements = append(capturer.placements, position.ID()) + return nil +} + +func retainedDocumentTree(t *testing.T, root string, headings ...string) *atspi.AccessibleTree { + t.Helper() + node := func(path, role string, children ...string) atspi.TreeNode { + snapshot := atspi.NewSnapshot(path, path, role, "", "") + snapshot.BusName = webID(path).BusName + if role == "heading" { + snapshot.HeadingLevel = 2 + } + result := atspi.TreeNode{Snapshot: snapshot} + for _, child := range children { + result.Children = append(result.Children, webID(child)) + } + return result + } + nodes := []atspi.TreeNode{node(root, "document web", headings...)} + for _, heading := range headings { + nodes = append(nodes, node(heading, "heading")) + } + tree, err := atspi.NewAccessibleTree(webID(root), nodes) + if err != nil { + t.Fatal(err) + } + return tree +} + +// Returning to a tab (or browser workspace) must resume that document's +// browse place from its retained cursor instead of recapturing cold and +// restarting at the top, even across document epoch resets. +func TestWorkerRetainsDocumentCursorsAcrossSwitches(t *testing.T) { + capturer := &retainedDocumentCapturer{trees: map[atspi.ObjectID]*atspi.AccessibleTree{ + webID("/docA"): retainedDocumentTree(t, "/docA", "/a1", "/a2", "/a3"), + webID("/docB"): retainedDocumentTree(t, "/docB", "/b1", "/b2"), + }} + worker, cancel := startWorker(t, capturer, time.Second) + defer worker.Stop(context.Background()) + defer cancel() + move := func(want string) { + t.Helper() + if err := worker.MoveDocument(DocumentHeading, 0, false); err != nil { + t.Fatal(err) + } + result := waitResult(t, worker) + if result.Err != nil || !result.Moved || result.DocumentPosition.Object.ID() != webID(want) { + t.Fatalf("heading move = %+v, want %s", result, want) + } + } + worker.SyncFocus(webID("/docA")) + move("/a1") + move("/a2") + worker.SuspendDocument(webID("/docB")) + move("/b1") + capturesA := 0 + for _, focus := range capturer.captures { + if focus == webID("/docA") { + capturesA++ + } + } + worker.SuspendDocument(webID("/docA")) + move("/a3") + for _, focus := range capturer.captures { + if focus == webID("/docA") { + capturesA-- + } + } + if capturesA != 0 { + t.Fatalf("returning to first document recaptured it; captures=%v", capturer.captures) + } +} + +// Warming runs the first capture and focus sync in the background, so the +// first navigation key afterwards needs no capture of its own. +func TestWorkerWarmDocumentPreparesFirstMove(t *testing.T) { + capturer := &retainedDocumentCapturer{trees: map[atspi.ObjectID]*atspi.AccessibleTree{ + webID("/docA"): retainedDocumentTree(t, "/docA", "/a1", "/a2"), + }} + worker, cancel := startWorker(t, capturer, time.Second) + defer worker.Stop(context.Background()) + defer cancel() + worker.SyncFocus(webID("/docA")) + worker.WarmDocument() + if err := worker.MoveDocument(DocumentHeading, 0, false); err != nil { + t.Fatal(err) + } + result := waitResult(t, worker) + if result.Err != nil || !result.Moved || result.DocumentPosition.Object.ID() != webID("/a1") { + t.Fatalf("warmed first move = %+v", result) + } + if len(capturer.captures) != 1 { + t.Fatalf("captures = %v, want only the warm capture", capturer.captures) + } +} + +// A ticking page can destroy the focused anchor itself. Capture keyed on +// that dead focus fails with ErrObjectNotFound forever; the move must +// recapture through the saved document instead of losing the location on +// every following keypress. +func TestDocumentCaretRecapturesThroughDocumentWhenFocusAnchorDies(t *testing.T) { + text := caretTextReader{webID("/text"): "Alpha", webID("/next"): "Next"} + oldCursor := newCaretTextCursor(t, text) + oldCursor.tree.Invalidate(webID("/text")) + fresh := newCaretTextCursor(t, text).tree + reader := &refreshingCaretReader{fakeCapturer: &fakeCapturer{calls: make(chan captureCall, 1)}, text: text} + worker, err := NewWorker(reader, time.Second) + if err != nil { + t.Fatal(err) + } + worker.SyncFocus(webID("/text")) + done := make(chan Result, 1) + go func() { + result, _ := worker.moveDocumentCaret(context.Background(), oldCursor, CaretNextCharacter, 0, 0) + done <- result + }() + initial := <-reader.calls + if initial.reviewFocus != webID("/text") { + t.Fatalf("initial capture focus = %v", initial.reviewFocus) + } + initial.result <- captureResult{err: fmt.Errorf("%w: document focus", atspi.ErrObjectNotFound)} + fallback := <-reader.calls + if fallback.reviewFocus != webID("/doc") { + t.Fatalf("fallback capture focus = %v, want saved document /doc", fallback.reviewFocus) + } + fallback.result <- captureResult{tree: fresh} + result := <-done + if result.Err != nil || !result.Moved || result.DocumentCaretText != "l" || result.DocumentPosition.Offset != 1 { + t.Fatalf("dead anchor recapture: %+v", result) + } +} + func (reader *waitingCaretReader) ReadDocumentCaret(ctx context.Context, _ atspi.ObjectReference, _ int, _ atspi.DocumentCaretUnit) (atspi.DocumentCaretText, error) { reader.deadline, _ = ctx.Deadline() <-ctx.Done() @@ -338,7 +543,7 @@ func TestDocumentCaretNavigationDeadlinePreservesPosition(t *testing.T) { worker.SyncFocus(webID("/text")) ctx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() - result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextWord, 0) + result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextWord, 0, 0) parentDeadline, _ := ctx.Deadline() if !errors.Is(result.Err, context.DeadlineExceeded) || result.Moved || !reader.deadline.Before(parentDeadline) { t.Fatalf("navigation deadline: result %+v, deadline %v", result, reader.deadline) @@ -415,7 +620,7 @@ func TestDocumentMoveRecoversAfterDynamicObjectRemoval(t *testing.T) { } done := make(chan moveResult, 1) go func() { - result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0) + result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0, 0) done <- moveResult{result: result} }() call := <-capturer.calls @@ -451,7 +656,7 @@ func TestDocumentMoveRecoversFocusRemovedDuringTextRecapture(t *testing.T) { worker.SyncFocus(webID("/first")) done := make(chan Result, 1) go func() { - result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0) + result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0, 0) done <- result }() deadline := time.After(3 * time.Second) @@ -487,7 +692,7 @@ func TestDocumentMoveRetriesStaleRecaptureWithoutSkipping(t *testing.T) { worker.SyncFocus(webID("/first")) done := make(chan Result, 1) go func() { - result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0) + result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0, 0) done <- result }() initial := <-capturer.calls @@ -520,7 +725,7 @@ func TestDocumentMoveRetriesStaleSnapshotWithoutSkipping(t *testing.T) { worker.SyncFocus(webID("/first")) done := make(chan Result, 1) go func() { - result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0) + result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, 0, 0) done <- result }() initial := <-capturer.calls @@ -551,7 +756,7 @@ func TestDocumentCaretRetriesStaleSnapshotWithoutSkipping(t *testing.T) { worker.SyncFocus(webID("/text")) done := make(chan Result, 1) go func() { - result, _ := worker.moveDocumentCaret(context.Background(), oldCursor, CaretNextCharacter, 0) + result, _ := worker.moveDocumentCaret(context.Background(), oldCursor, CaretNextCharacter, 0, 0) done <- result }() initial := <-reader.calls @@ -609,7 +814,7 @@ func TestWorkerJumpsToLoadedDocumentFragment(t *testing.T) { worker.SyncFocus(webID("/doc")) results := make(chan Result, 1) go func() { - result, _ := worker.jumpDocumentFragment(context.Background(), nil, "p71", worker.documentEpoch.Load()) + result, _ := worker.jumpDocumentFragment(context.Background(), nil, "p71", worker.documentEpoch.Load(), worker.documentLoadEpoch.Load()) results <- result }() capture := <-navigator.fakeCapturer.calls @@ -753,7 +958,7 @@ func TestWorkerCaretRefusalKeepsVirtualMove(t *testing.T) { worker.SyncFocus(webID("/first")) failure := errors.New("browser refused document caret") navigator.placementErr <- failure - result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, worker.DocumentEpoch()) + result, _ := worker.moveDocument(context.Background(), cursor, DocumentHeading, 0, false, worker.DocumentEpoch(), worker.documentLoadEpoch.Load()) if result.Err != nil || result.CaretErr != failure || !result.Moved || result.DocumentPosition.Object.ID() != webID("/second") { t.Fatalf("ordinary caret refusal lost virtual reading position: %+v", result) } @@ -1003,7 +1208,7 @@ func TestDocumentCaretKeepsOffsetAfterDocumentRefresh(t *testing.T) { worker.SyncFocus(webID("/doc")) ctx := context.Background() for _, want := range []string{"First\n", "Second\n"} { - result, next := worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load()) + result, next := worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load(), worker.documentLoadEpoch.Load()) cursor = next if result.Err != nil || result.DocumentCaretText != want { t.Fatalf("initial navigation = %+v, want %q", result, want) @@ -1033,7 +1238,7 @@ func TestDocumentCaretKeepsOffsetAfterDocumentRefresh(t *testing.T) { if kind != "children" { go refresh() } - result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load()) + result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load(), worker.documentLoadEpoch.Load()) if result.Err != nil || !errors.Is(result.CaptureReason, wantReason) || result.DocumentCaretText != "Third" { t.Fatalf("semantic refresh restarted or skipped reading: %+v", result) } @@ -1041,10 +1246,11 @@ func TestDocumentCaretKeepsOffsetAfterDocumentRefresh(t *testing.T) { if result.DocumentDiagnostics.CaptureAttempts != 0 { t.Fatal("local text movement recaptured unrelated child order") } - go refresh() - result, _ = worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load()) - if result.Err != nil || !errors.Is(result.CaptureReason, atspi.ErrIncompleteTree) || result.DocumentCaretText != "End" { - t.Fatalf("crossing refreshed children lost position: %+v", result) + // Crossing follows captured child order with live reads and + // generation-checked endpoints; it no longer recaptures. + result, _ = worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load(), worker.documentLoadEpoch.Load()) + if result.Err != nil || result.CaptureReason != nil || result.DocumentDiagnostics.CaptureAttempts != 0 || result.DocumentCaretText != "End" { + t.Fatalf("crossing changed children lost position: %+v", result) } } }) diff --git a/internal/review/worker.go b/internal/review/worker.go index 64fbe97..3bbeb54 100644 --- a/internal/review/worker.go +++ b/internal/review/worker.go @@ -54,12 +54,23 @@ type Worker struct { cancel context.CancelFunc done chan struct{} - wakeFocus chan struct{} - requests chan request - results chan Result - lineSequence atomic.Uint64 - documentEpoch atomic.Uint64 - documentBraille atomic.Bool + wakeFocus chan struct{} + requests chan request + results chan Result + lineSequence atomic.Uint64 + documentEpoch atomic.Uint64 + // documentLoadEpoch bumps only on hard document resets (page loads). + // A retained cursor stamped with an older load epoch must never be + // reused: its tree may be the previous page at reused paths. + documentLoadEpoch atomic.Uint64 + documentBraille atomic.Bool + + // Retained browse cursors per captured document tree, so returning to + // a tab resumes its place instead of restarting cold. Owned by the + // run goroutine (document moves run there); no lock. + documentCursors map[*atspi.AccessibleTree]*DocumentCursor + documentCursorEpochs map[*atspi.AccessibleTree]uint64 + documentCursorOrder []*atspi.AccessibleTree } // SetDocumentBrailleEnabled avoids extra native line reads while braille is off. @@ -68,13 +79,30 @@ func (worker *Worker) SetDocumentBrailleEnabled(enabled bool) { } // ResetDocument retires queued work and cursors for the previous page, even -// when the provider reuses accessible paths after navigation. +// when the provider reuses accessible paths after navigation. It is the +// hard reset for page loads: retained cursors from before it must not be +// reused, because a new page can reuse the same accessible paths. func (worker *Worker) ResetDocument(focus atspi.ObjectID) { + worker.resetDocument(focus, true) +} + +// SuspendDocument retires queued work for the previously selected tab +// without retiring its browse cursors, so returning to the tab (or its +// workspace) resumes each document's place. Only for focus/tab switches +// where no page (re)loaded. +func (worker *Worker) SuspendDocument(focus atspi.ObjectID) { + worker.resetDocument(focus, false) +} + +func (worker *Worker) resetDocument(focus atspi.ObjectID, hard bool) { worker.stateMutex.Lock() worker.latestFocus = focus worker.focusRevision++ worker.flatOrigin = atspi.ObjectID{} worker.documentEpoch.Add(1) + if hard { + worker.documentLoadEpoch.Add(1) + } worker.stateMutex.Unlock() select { case worker.wakeFocus <- struct{}{}: @@ -251,20 +279,47 @@ func (worker *Worker) run(ctx context.Context, done chan<- struct{}) { explicitInterest := implicitInterest flat := &FlatCursor{} var flatTree *atspi.AccessibleTree + // held is the first non-identical request found while coalescing + // queued identical document moves; it runs next, in order. + var held *request + takeRepeat := func(first request) int { + repeat := 1 + for { + select { + case next := <-worker.requests: + if sameDocumentRepeat(first, next) { + repeat++ + continue + } + held = &next + return repeat + default: + return repeat + } + } + } for { // A ready request/result channel can win a select even after Stop. // Do not drain queued work or start further captures during shutdown. if ctx.Err() != nil { return } - select { - case <-ctx.Done(): - return - case <-worker.wakeFocus: - if next, err := worker.syncLatestFocus(ctx, cursor, false); err == nil { - cursor = next + var request request + if held != nil { + request, held = *held, nil + } else { + select { + case <-ctx.Done(): + return + case <-worker.wakeFocus: + if next, err := worker.syncLatestFocus(ctx, cursor, false); err == nil { + cursor = next + } + continue + case request = <-worker.requests: } - case request := <-worker.requests: + } + { worker.stateMutex.Lock() focus, revision := worker.latestFocus, worker.focusRevision worker.stateMutex.Unlock() @@ -283,6 +338,12 @@ func (worker *Worker) run(ctx context.Context, done chan<- struct{}) { request.documentList.result <- documentListReply{list, err} continue } + if request.warmDocument { + if request.documentEpoch == documentEpoch { + document = worker.warmDocument(ctx, document, request.documentLoadEpoch) + } + continue + } if request.interest != nil { interest, err := resolve_interest(request.interest.ctx, interestFocus, explicitInterest, cursor, flat, flatTree, document) worker.stateMutex.Lock() @@ -338,16 +399,16 @@ func (worker *Worker) run(ctx context.Context, done chan<- struct{}) { result, documentTextTree = worker.readDocumentLine(ctx, documentTextTree, request.lineRequest, request.lineFocus, request.lineOffset) result.Elapsed = time.Since(request.queuedAt) } else if request.target != 0 { - result, document = worker.moveDocument(ctx, document, request.target, request.level, request.backwards, request.documentEpoch) + result, document = worker.moveDocumentRepeated(ctx, document, request.target, request.level, request.backwards, request.documentEpoch, request.documentLoadEpoch, takeRepeat(request)) result.Elapsed = time.Since(request.queuedAt) } else if request.caretCommand != 0 { - result, document = worker.moveDocumentCaret(ctx, document, request.caretCommand, request.documentEpoch) + result, document = worker.moveDocumentCaret(ctx, document, request.caretCommand, request.documentEpoch, request.documentLoadEpoch) result.Elapsed = time.Since(request.queuedAt) } else if request.activate { result, document = worker.activateDocumentCaret(ctx, document, request.documentEpoch) result.Elapsed = time.Since(request.queuedAt) } else if request.fragment != "" { - result, document = worker.jumpDocumentFragment(ctx, document, request.fragment, request.documentEpoch) + result, document = worker.jumpDocumentFragment(ctx, document, request.fragment, request.documentEpoch, request.documentLoadEpoch) result.Elapsed = time.Since(request.queuedAt) } else if request.unit != 0 { result, flatTree = worker.readFlat(ctx, flat, flatTree, request.unit, request.step) @@ -373,7 +434,8 @@ func (worker *Worker) run(ctx context.Context, done chan<- struct{}) { worker.stateMutex.Unlock() } result.DocumentEpoch = request.documentEpoch - if documentCommand && worker.CurrentDocumentResult(result) && errors.Is(result.Err, context.DeadlineExceeded) { + if documentCommand && worker.CurrentDocumentResult(result) && + (errors.Is(result.Err, context.DeadlineExceeded) || errors.Is(result.Err, atspi.ErrProviderHung)) { // Keys pressed while the provider was unavailable must not each // start another full timeout. A command issued after this failure // can try again; successful navigation keeps its ordered steps. diff --git a/internal/review/worker_contract.go b/internal/review/worker_contract.go index 297ac72..71a5798 100644 --- a/internal/review/worker_contract.go +++ b/internal/review/worker_contract.go @@ -112,6 +112,7 @@ type Result struct { // activation, fragment, flat unit, then object direction. Flags are deliberately // retained; epoch and timeout retirement depend on their distinct combinations. type request struct { + warmDocument bool currentLine bool currentLineBrowse bool documentList *documentListRequest @@ -126,6 +127,7 @@ type request struct { readTree *atspi.AccessibleTree readFocus atspi.ObjectID documentEpoch uint64 + documentLoadEpoch uint64 queuedAt time.Time target DocumentTarget caretCommand DocumentCaretCommand