diff --git a/distro-packages/Arch-Linux/cthulhu-git/PKGBUILD b/distro-packages/Arch-Linux/cthulhu-git/PKGBUILD index ba8b8e8..7628fc8 100644 --- a/distro-packages/Arch-Linux/cthulhu-git/PKGBUILD +++ b/distro-packages/Arch-Linux/cthulhu-git/PKGBUILD @@ -4,7 +4,7 @@ pkgbase=cthulhu-git pkgname=cthulhu-git _pkgname=cthulhu -pkgver=2026.10.24.r75.g8a560a2 +pkgver=2026.10.24.r77.g133953e pkgrel=1 pkgdesc="Desktop-agnostic screen reader with speech, Braille, and Tentacle support" url="https://git.stormux.org/storm/cthulhu" diff --git a/docs/testing/benchmarks.md b/docs/testing/benchmarks.md index 4554c5d..ccc53a3 100644 --- a/docs/testing/benchmarks.md +++ b/docs/testing/benchmarks.md @@ -31,6 +31,20 @@ text, with two objects on each visual row. Each benchmark iteration performs: construction and navigation bookkeeping. - `BenchmarkDocumentLineAssembly`: one line lookup in the middle of a document, combining the two text objects on that row. This includes snapshot construction. +- `BenchmarkDocumentCaretNavigation`: a character forward/backward round trip + within the same text object. Reusable caret ranges are warmed before timing. +- `BenchmarkDocumentWorkerHeadingNavigation`: a heading forward/backward round + trip through the review worker, including focus synchronization, diagnostics + and placement dispatch, with an in-memory provider. It performs no browser IPC. + +Include the additional cases with +`-bench '^BenchmarkDocument(Snapshot|HeadingNavigation|LineAssembly|CaretNavigation|WorkerHeadingNavigation)$'`. +Navigation shares an immutable indexed view; `BenchmarkDocumentSnapshot` +continues to measure the independently owned node copy returned by `Document`. +The first view construction is included in snapshot and line measurements and +amortized over subsequent iterations. A short run can therefore overstate the +steady allocation rate. See [document navigation validation](browser-navigation-performance.md) +for cache ownership, invalidation and real-browser checks. Fixture construction and initial focus synchronization are outside the timed loops. The benchmarks fail on errors, missing work or invalid result shapes. diff --git a/docs/testing/browser-navigation-performance.md b/docs/testing/browser-navigation-performance.md new file mode 100644 index 0000000..38520fb --- /dev/null +++ b/docs/testing/browser-navigation-performance.md @@ -0,0 +1,114 @@ +# Document navigation performance and validation + +Browse navigation reuses an immutable document view and an object-to-index +lookup owned by `AccessibleTree`. `DocumentCursor` reuses the view's caret ranges +and visual-line ownership map. Focus synchronization, heading/link searches, +position lookup, linked-heading lookup and replacement-parent bookkeeping use +that index instead of rebuilding maps or scanning the entire document per key. + +`Replace`, object removal and destructive child invalidation retire the cached +view. Pending semantic, text, geometry and child-order changes keep the view's +immutable data but mark the live tree entries dirty. Before navigation reads or +selects content, it validates the objects it traverses and their relevant +ancestors. Dirty skipped candidates are validated too, so navigation cannot +silently omit newly readable content. A local move within one live text object +can proceed through a parent's unrelated insertion; crossing objects requires +fresh child order. Table-row summaries also validate their contributing sibling +cells using the same bounded dependency list used to derive the row text. + +Failed validation preserves the reading position and uses the existing bounded +recapture/retry path. Successful native text remains valid for the requested +announcement if the provider removes its object immediately afterwards; retrying +that completed move would skip text. Activation, element lists, native selection +resolution and captured-line assembly retain complete-document validation. +`Document` still returns an independently owned node slice; callers of +`DocumentView` and `NavigationDocument` must treat the shared data as read-only. +Tree access and cache publication use the tree mutex, and native object lifetime +and GLib ownership stay with the adapter. + +On 2026-10-24, three sequential 200 ms runs on an AMD Ryzen 9 PRO 5945 with +Go 1.27.3 compared the unchanged `133953e` source through a Go overlay with +these changes. The same benchmark source ran against both versions. Median +results for the 10,000-content-object fixture were: + +| Operation | Before | After | Bytes allocated before | Bytes allocated after | +| --- | ---: | ---: | ---: | ---: | +| Worker heading forward/back pair | 87.476 ms | 0.053 ms | 276,775,952 | 51,650 | +| Character forward/back pair | 53.536 ms | 0.014 ms | 124,261,980 | 16,072 | +| Heading forward/back pair | 43.129 ms | 0.035 ms | 142,691,897 | 18,080 | +| Heading wrapping pair | 44.895 ms | 0.032 ms | 142,687,798 | 18,080 | +| Owned document snapshot | 14.747 ms | 5.421 ms | 54,801,125 | 11,723,115 | +| Captured line assembly | 15.297 ms | 3.095 ms | 54,810,445 | 182,019 | + +These measure owned Go data and fake-provider calls, excluding native browser +IPC, initial capture, speech delivery and audible latency. Allocation figures +are traffic per operation, not retained process memory. Snapshot/line results +include an amortized first index build; navigation setup precedes timing. +Raw outputs are `/tmp/cthulhu-web-performance-before.txt` and +`/tmp/cthulhu-web-performance-after.txt`; medians are retained in +`/tmp/cthulhu-web-performance-medians.json`. Performance remains informational, +with no timing gate added to tests. + +Run the affected correctness and race checks in a private session: + +```sh +python3 tests/native_test_session.py env GOCACHE=/tmp/cthulhu-cache go test ./internal/atspi ./internal/review ./internal/app -count=1 +python3 tests/native_test_session.py env GOCACHE=/tmp/cthulhu-cache go test -race ./internal/atspi ./internal/review ./internal/app -count=1 +go vet ./internal/atspi ./internal/review ./internal/app +``` + +The tests cover cached-view ownership, concurrent replacement, retired views, +cancellation, unrelated dirty branches, stale traversal and unchanged reading +positions after failures. Existing regressions also cover replacement recovery, +generation-checked activation, table navigation, native selections, passwords, +inline text, named links and browser-chrome round trips. Two existing recapture +tests now distinguish continued reading within a live leaf from crossing dirty +child order, rather than requiring a recapture for every document mutation. + +Run the existing browser fixtures and actual-reader navigation fixture: + +```sh +CTHULHU_NATIVE_BROWSER_TESTS=replaced-feed,generatedlinks,replacement,fragments,tabs,namedlinks,linkedheadings,tables,dynamic,document,editing,selection,modes python3 tests/native_browser.py +go build -o /tmp/cthulhu-browser-performance ./cmd/cthulhu +CTHULHU_BINARY=/tmp/cthulhu-browser-performance python3 tests/native_browser_navigation.py +``` + +The actual-reader fixture checks 76 navigation commands while page text changes, +including forward/reverse headings, line/character/word movement and custom +activation. It records speech protocol requests in a private X11/browser session. +It does not establish audible output or physical keyboard/Braille behavior. +An initial optimized run passed the entire fixture. A final-binary run completed +all 76 navigation commands and both custom activations, then failed its final +pointer-handoff assertion: the click at screen coordinates 0,0 produced no +observed shared AT-SPI pointer press. The focused native pointer test passed. +Keep this separate from the verified navigation sequence; it remains an open +harness/delivery check. The unchanged original binary reproduced the same final +pointer failure after completing its navigation sequence, so this failure +predates the performance changes. Evidence is retained in +`/tmp/cthulhu-browser-navigation-56h2cw3n` (final binary), +`/tmp/cthulhu-browser-navigation-i2jxxobv` (original binary), and +`/tmp/cthulhu-browser-navigation-mduikajr` (initial complete optimized pass). + +The browser `modes` fixture remains an intermittent gate. During this change it +passed in an isolated optimized run, but mixed runs failed once on page return +and once while expecting typed chrome text. The original source passed isolated +and tables/selection/modes comparison runs; these comparisons do not conclusively +attribute the intermittent failures. Preserve their logs and report the open +gate rather than describing the whole browser suite as consistently passing. + +For human acceptance, use the actual reported Reddit view and a long updating +page. Navigate headings in both directions through wrapping, read post/comment +text with arrows and word/character commands, expand/collapse comments, load +more content, type and select in an editor, activate links, and return from the +address bar and another tab. Check that stale/replaced targets never activate a +different control. On a table, change a sibling cell and verify full-row speech +and Braille refresh. Check Steam browse navigation and search/Tab behavior after +an application/workspace round trip. Listen for skipped/repeated text and verify +physical Braille where available. Preserve the current log and obtain approval +before replacing the active reader. + +The first native document capture still enumerates the captured tree, the +30,000-object safety limit still applies, and generic named-clickable filtering +is unchanged. These performance changes do not establish a fix for Reddit's +specific reported content problem. Review the diff and complete the live checks +before submitting under the project's AI-assisted contribution policy. diff --git a/internal/atspi/document.go b/internal/atspi/document.go index c145cac..8fc5660 100644 --- a/internal/atspi/document.go +++ b/internal/atspi/document.go @@ -93,14 +93,48 @@ type DocumentNode struct { type DocumentSnapshot struct { Context DocumentContext Nodes []DocumentNode + data *documentData } // Document copies the top-level document, including embedded documents but // excluding browser chrome and other tabs. A stale/incomplete capture fails // atomically. It does not use flat review's viewport-limited text ranges. func (tree *AccessibleTree) Document(ctx context.Context, reference ObjectReference) (DocumentSnapshot, error) { - tree.mu.RLock() - defer tree.mu.RUnlock() + view, err := tree.DocumentView(ctx, reference) + if err != nil { + return DocumentSnapshot{}, err + } + view.Nodes = append([]DocumentNode(nil), view.Nodes...) + view.data = nil + return view, nil +} + +// DocumentView validates the complete document atomically and shares its +// immutable data. Like NavigationDocument, the returned Nodes are read-only. +func (tree *AccessibleTree) DocumentView(ctx context.Context, reference ObjectReference) (DocumentSnapshot, error) { + tree.mu.Lock() + defer tree.mu.Unlock() + view, err := tree.navigation_document_locked(ctx, reference) + if err != nil { + return DocumentSnapshot{}, err + } + if err := tree.validate_document_range_locked(ctx, view, 0, len(view.Nodes)-1); err != nil { + return DocumentSnapshot{}, err + } + return view, nil +} + +// NavigationDocument shares immutable document data until Replace or removal +// changes the tree. Callers must not modify Nodes or their nested slices. Dirty +// entries remain in the view: navigation validates the path it actually uses +// with ValidateDocumentRange rather than recapturing unrelated page updates. +func (tree *AccessibleTree) NavigationDocument(ctx context.Context, reference ObjectReference) (DocumentSnapshot, error) { + tree.mu.Lock() + defer tree.mu.Unlock() + return tree.navigation_document_locked(ctx, reference) +} + +func (tree *AccessibleTree) navigation_document_locked(ctx context.Context, reference ObjectReference) (DocumentSnapshot, error) { if err := ctx.Err(); err != nil { return DocumentSnapshot{}, err } @@ -108,7 +142,14 @@ func (tree *AccessibleTree) Document(ctx context.Context, reference ObjectRefere if err != nil { return DocumentSnapshot{}, err } - result := DocumentSnapshot{Context: owner} + if data := tree.documents[owner.TopLevel.ID()]; data != nil { + return DocumentSnapshot{Context: owner, Nodes: data.nodes, data: data}, nil + } + capacity := 128 + if owner.TopLevel.ID() == tree.root { + capacity = len(tree.nodes) + } + result := DocumentSnapshot{Context: owner, Nodes: make([]DocumentNode, 0, capacity)} type pendingNode struct { id ObjectID hidden bool @@ -121,19 +162,10 @@ func (tree *AccessibleTree) Document(ctx context.Context, reference ObjectRefere last := len(pending) - 1 next := pending[last] pending = pending[:last] - entry := tree.nodes[next.id] - if entry.snapshotInvalid { - return DocumentSnapshot{}, ErrStaleSnapshot - } - if entry.childrenInvalid { + entry, present := tree.nodes[next.id] + if !present { return DocumentSnapshot{}, ErrIncompleteTree } - if entry.textInvalid { - return DocumentSnapshot{}, ErrStaleText - } - if entry.geometryInvalid { - return DocumentSnapshot{}, ErrStaleGeometry - } hidden := next.hidden || entry.geometry.VisibilityKnown && !entry.geometry.Visible parentEditable := false if entry.hasParent { @@ -153,6 +185,11 @@ func (tree *AccessibleTree) Document(ctx context.Context, reference ObjectRefere pending = append(pending, pendingNode{entry.children[i], hidden}) } } + result.data = new_document_data(result.Nodes) populate_document_rows(&result) + if tree.documents == nil { + tree.documents = make(map[ObjectID]*documentData) + } + tree.documents[owner.TopLevel.ID()] = result.data return result, nil } diff --git a/internal/atspi/document_line.go b/internal/atspi/document_line.go index 2a6a353..e221dca 100644 --- a/internal/atspi/document_line.go +++ b/internal/atspi/document_line.go @@ -27,7 +27,7 @@ type DocumentLine struct { } func (tree *AccessibleTree) DocumentLineAtOffset(ctx context.Context, reference ObjectReference, offset int) (DocumentLine, error) { - document, err := tree.Document(ctx, reference) + document, err := tree.DocumentView(ctx, reference) if err != nil { return DocumentLine{}, err } diff --git a/internal/atspi/document_rows.go b/internal/atspi/document_rows.go index e6bd03c..b63f9a6 100644 --- a/internal/atspi/document_rows.go +++ b/internal/atspi/document_rows.go @@ -12,10 +12,7 @@ import ( // Derive rows from the already captured document. This performs no native // queries and keeps large tables bounded to 32 cells and 4096 bytes per row. func populate_document_rows(document *DocumentSnapshot) { - nodes := make(map[ObjectID]int, len(document.Nodes)) - for index, node := range document.Nodes { - nodes[node.Reference.ID()] = index - } + nodes := document.data.indices type rowKey struct { table ObjectID row int @@ -55,10 +52,19 @@ func populate_document_rows(document *DocumentSnapshot) { owners[index] = key } texts := make(map[rowKey]string, len(rows)) + dependencies := make(map[rowKey][]ObjectReference, len(rows)) for key, indices := range rows { sort.SliceStable(indices, func(a, b int) bool { return document.Nodes[indices[a]].Snapshot.TableColumnIndex < document.Nodes[indices[b]].Snapshot.TableColumnIndex }) + // Validation follows the same bounded set used to derive row text. + // Include cells past the byte limit too: shorter updated labels could + // make those cells newly contribute to the next row summary. + references := make([]ObjectReference, len(indices)) + for index, member := range indices { + references[index] = document.Nodes[member].Reference + } + dependencies[key] = references var parts []string length := 0 for _, index := range indices { @@ -78,7 +84,11 @@ func populate_document_rows(document *DocumentSnapshot) { } texts[key] = strings.Join(parts, ", ") } + if len(owners) > 0 { + document.data.rows = make(map[ObjectID][]ObjectReference, len(owners)) + } for index, key := range owners { document.Nodes[index].Snapshot.TableRowText = texts[key] + document.data.rows[document.Nodes[index].Reference.ID()] = dependencies[key] } } diff --git a/internal/atspi/document_view.go b/internal/atspi/document_view.go new file mode 100644 index 0000000..04601dc --- /dev/null +++ b/internal/atspi/document_view.go @@ -0,0 +1,165 @@ +// Copyright (c) 2026 Storm Dragon +// Authors: Storm Dragon, Angus Kola +// SPDX-License-Identifier: GPL-3.0-or-later + +package atspi + +import "context" + +// Owned by AccessibleTree under its mutex. Published data is immutable and can +// outlive a replacement; ValidateDocumentRange rejects retired views. Dirty +// flags stay in treeEntry, so an unrelated invalidation need not rebuild this. +type documentData struct { + nodes []DocumentNode + indices map[ObjectID]int + ends []int + rows map[ObjectID][]ObjectReference +} + +func new_document_data(nodes []DocumentNode) *documentData { + data := &documentData{nodes: nodes, indices: make(map[ObjectID]int, len(nodes)), ends: make([]int, len(nodes))} + for index, node := range nodes { + data.indices[node.Reference.ID()] = index + data.ends[index] = index + } + for index := len(nodes) - 1; index > 0; index-- { + parent := data.indices[nodes[index].Parent] + data.ends[parent] = max(data.ends[parent], data.ends[index]) + } + return data +} + +// SameContent identifies a shared immutable view, not merely matching object +// paths. A replacement with surviving identities still has different content. +func (document DocumentSnapshot) SameContent(other DocumentSnapshot) bool { + return document.data != nil && document.data == other.data +} + +func (document DocumentSnapshot) Index(id ObjectID) (int, bool) { + if document.data != nil { + index, ok := document.data.indices[id] + return index, ok + } + for index, node := range document.Nodes { + if node.Reference.ID() == id { + return index, true + } + } + return 0, false +} + +func (document DocumentSnapshot) Node(id ObjectID) (DocumentNode, bool) { + index, ok := document.Index(id) + if !ok { + return DocumentNode{}, false + } + return document.Nodes[index], true +} + +func (document DocumentSnapshot) SubtreeEnd(index int) int { + if document.data != nil { + return document.data.ends[index] + } + end := index + for next := index + 1; next < len(document.Nodes); next++ { + parent, ok := document.Index(document.Nodes[next].Parent) + if !ok || parent < index { + break + } + end = next + } + return end +} + +func document_entry_error(entry treeEntry) error { + switch { + case entry.snapshotInvalid: + return ErrStaleSnapshot + case entry.childrenInvalid: + return ErrIncompleteTree + case entry.textInvalid: + return ErrStaleText + case entry.geometryInvalid: + return ErrStaleGeometry + } + return nil +} + +// ValidateDocumentRange checks traversed objects, including skipped/hidden +// candidates, before committing a move. Ancestor metadata always matters; +// ancestor child order matters when crossing objects. A character move within +// one live leaf therefore survives insertions elsewhere in its parent. +func (tree *AccessibleTree) ValidateDocumentRange(ctx context.Context, document DocumentSnapshot, from, to int) error { + tree.mu.RLock() + defer tree.mu.RUnlock() + return tree.validate_document_range_locked(ctx, document, from, to) +} + +// 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) +} + +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) +} + +func (tree *AccessibleTree) validate_document_path_locked(ctx context.Context, document DocumentSnapshot, from, to int, requireOrder bool) error { + if err := ctx.Err(); err != nil { + return err + } + if document.data == nil || tree.documents[document.Context.TopLevel.ID()] != document.data { + return ErrStaleObject + } + if from < 0 || to < 0 || from >= len(document.Nodes) || to >= len(document.Nodes) { + return ErrStaleObject + } + for index := min(from, to); index <= max(from, to); index++ { + if err := ctx.Err(); err != nil { + return err + } + node := document.Nodes[index] + entry, err := tree.resolveLocked(node.Reference) + if err != nil { + return err + } + if err := document_entry_error(entry); err != nil { + return err + } + if err := tree.validate_document_row_locked(document, node.Reference.ID()); err != nil { + return err + } + for entry.hasParent { + entry = tree.nodes[entry.parent] + if entry.snapshotInvalid { + return ErrStaleSnapshot + } + if entry.geometryInvalid { + return ErrStaleGeometry + } + if requireOrder && entry.childrenInvalid { + return ErrIncompleteTree + } + if err := tree.validate_document_row_locked(document, entry.snapshot.ID()); err != nil { + return err + } + } + } + return nil +} + +func (tree *AccessibleTree) validate_document_row_locked(document DocumentSnapshot, id ObjectID) 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 { + return err + } + } + return nil +} diff --git a/internal/atspi/document_view_test.go b/internal/atspi/document_view_test.go new file mode 100644 index 0000000..1707eef --- /dev/null +++ b/internal/atspi/document_view_test.go @@ -0,0 +1,163 @@ +// Copyright (c) 2026 Storm Dragon +// Authors: Storm Dragon, Angus Kola +// SPDX-License-Identifier: GPL-3.0-or-later + +package atspi + +import ( + "context" + "errors" + "sync" + "testing" +) + +func TestNavigationDocumentConcurrentReplacementKeepsViewsImmutable(t *testing.T) { + tree, _ := NewAccessibleTree(testID("/root"), documentTestNodes()) + ref, _ := tree.Reference(testID("/heading")) + var workers sync.WaitGroup + workers.Add(1) + go func() { + defer workers.Done() + for range 100 { + nodes := documentTestNodes() + for index := range nodes { + nodes[index].Snapshot.Name = "Replacement" + } + if err := tree.Replace(testID("/root"), nodes); err != nil { + t.Error(err) + return + } + } + }() + defer workers.Wait() + for range 100 { + view, err := tree.NavigationDocument(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + replaced := view.Context.Ancestors[0].Name == "Replacement" + for _, node := range view.Nodes { + if (node.Snapshot.Name == "Replacement") != replaced { + t.Fatal("mixed replacement data in shared view") + } + } + if err := tree.ValidateDocumentRange(context.Background(), view, 0, len(view.Nodes)-1); err != nil && !errors.Is(err, ErrStaleObject) { + t.Fatal(err) + } + } +} + +func TestNavigationDocumentSharesOnlyImmutableData(t *testing.T) { + tree, _ := NewAccessibleTree(testID("/root"), documentTestNodes()) + ref, _ := tree.Reference(testID("/heading")) + first, err := tree.NavigationDocument(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + second, err := tree.NavigationDocument(context.Background(), ref) + if err != nil || !first.SameContent(second) || &first.Nodes[0] != &second.Nodes[0] { + t.Fatal("unchanged document data was rebuilt", err) + } + copy, err := tree.Document(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + copy.Nodes[0].Snapshot.Name = "Changed copy" + if first.Nodes[0].Snapshot.Name == "Changed copy" { + t.Fatal("owned snapshot modified shared navigation data") + } + for index, node := range first.Nodes { + if got, ok := first.Index(node.Reference.ID()); !ok || got != index { + t.Fatalf("index for %v = %d, %v", node.Reference.ID(), got, ok) + } + if end := first.SubtreeEnd(index); end < index || end >= len(first.Nodes) { + t.Fatalf("invalid subtree end %d", end) + } + } +} + +func TestNavigationDocumentValidatesOnlyTraversedBranches(t *testing.T) { + for _, test := range []struct { + name string + invalidate func(*AccessibleTree) + want error + }{ + {"text", func(tree *AccessibleTree) { tree.InvalidateText(testID("/link")) }, ErrStaleText}, + {"semantics", func(tree *AccessibleTree) { tree.InvalidateSnapshot(testID("/link")) }, ErrStaleSnapshot}, + {"geometry", func(tree *AccessibleTree) { tree.InvalidateGeometry(testID("/inner")) }, ErrStaleGeometry}, + {"children", func(tree *AccessibleTree) { tree.MarkChildrenChanged(testID("/inner")) }, 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) + current, err := tree.NavigationDocument(context.Background(), ref) + if err != nil || !current.SameContent(view) { + t.Fatal("unrelated dirty branch rebuilt document", err) + } + heading, _ := view.Index(ref.ID()) + if err := tree.ValidateDocumentRange(context.Background(), view, heading, heading); err != nil { + t.Fatal("unrelated update blocked current heading", err) + } + if err := tree.ValidateDocumentRange(context.Background(), view, heading, len(view.Nodes)-1); !errors.Is(err, test.want) { + t.Fatalf("traversed stale branch: %v, want %v", err, test.want) + } + }) + } +} + +func TestNavigationDocumentRejectsRetiredViewsAndCanceledMoves(t *testing.T) { + tree, _ := NewAccessibleTree(testID("/root"), documentTestNodes()) + ref, _ := tree.Reference(testID("/heading")) + view, _ := tree.NavigationDocument(context.Background(), ref) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := tree.ValidateDocumentRange(ctx, view, 0, 0); !errors.Is(err, context.Canceled) { + t.Fatal(err) + } + if err := tree.Replace(testID("/root"), documentTestNodes()); err != nil { + t.Fatal(err) + } + if err := tree.ValidateDocumentRange(context.Background(), view, 0, 0); !errors.Is(err, ErrStaleObject) { + t.Fatal("replacement retained old view", err) + } + fresh, _ := tree.NavigationDocument(context.Background(), ref) + if fresh.SameContent(view) { + t.Fatal("replacement reused navigation data") + } + tree.Invalidate(testID("/link")) + if err := tree.ValidateDocumentRange(context.Background(), fresh, 0, 0); !errors.Is(err, ErrStaleObject) { + t.Fatal("removal retained old topology", err) + } +} + +func TestNavigationDocumentValidatesDerivedRowDependencies(t *testing.T) { + node := func(path, role string, children ...ObjectID) TreeNode { + n := testNode(path, path, children...) + n.Snapshot.Role = role + return n + } + nodes := []TreeNode{node("/doc", "document web", testID("/table")), node("/table", "table", testID("/first"), testID("/second")), node("/first", "table cell", testID("/text")), node("/text", "static"), node("/second", "table cell")} + for _, index := range []int{2, 4} { + nodes[index].Snapshot.TablePositionKnown = true + nodes[index].Snapshot.TableColumnIndex = index/2 - 1 + } + tree, err := NewAccessibleTree(testID("/doc"), nodes) + if err != nil { + t.Fatal(err) + } + ref, _ := tree.Reference(testID("/text")) + view, err := tree.NavigationDocument(context.Background(), ref) + if err != nil { + t.Fatal(err) + } + index, _ := view.Index(ref.ID()) + tree.InvalidateText(testID("/second")) + if err := tree.ValidateDocumentRange(context.Background(), view, index, index); !errors.Is(err, ErrStaleText) { + t.Fatal("cached full row ignored a changed sibling cell", err) + } +} diff --git a/internal/atspi/tree.go b/internal/atspi/tree.go index d5181a9..577c667 100644 --- a/internal/atspi/tree.go +++ b/internal/atspi/tree.go @@ -88,6 +88,7 @@ type AccessibleTree struct { root ObjectID nodes map[ObjectID]treeEntry nextGeneration uint64 + documents map[ObjectID]*documentData } func NewAccessibleTree(root ObjectID, nodes []TreeNode) (*AccessibleTree, error) { @@ -126,6 +127,7 @@ func (tree *AccessibleTree) Replace(root ObjectID, nodes []TreeNode) error { } tree.root = root tree.nodes = next + tree.documents = nil return nil } @@ -352,6 +354,7 @@ func (tree *AccessibleTree) Invalidate(id ObjectID) int { if !ok { return 0 } + tree.documents = nil if entry.hasParent { parent := tree.nodes[entry.parent] for index, childID := range parent.children { @@ -392,6 +395,7 @@ func (tree *AccessibleTree) InvalidateChildren(parentID ObjectID) int { if !ok { return 0 } + tree.documents = nil pending := append([]ObjectID(nil), parent.children...) parent.children = nil parent.childrenInvalid = true diff --git a/internal/review/document.go b/internal/review/document.go index e74190f..8daa629 100644 --- a/internal/review/document.go +++ b/internal/review/document.go @@ -79,10 +79,13 @@ type documentPlace struct { // capture/recapture lifecycle. Methods serialize cursor state and do no native // work, key handling, focus changes, or speech. type DocumentCursor struct { - mu sync.Mutex - tree *atspi.AccessibleTree - places []documentPlace - active atspi.ObjectReference + mu sync.Mutex + tree *atspi.AccessibleTree + places []documentPlace + active atspi.ObjectReference + caretDocument atspi.DocumentSnapshot + caretRanges []caretRange + lineOwners map[int]atspi.ObjectID } // rebindPlace keeps the selected target after a semantic refresh retires its @@ -155,7 +158,7 @@ func (cursor *DocumentCursor) ActivationTarget() (atspi.ObjectReference, error) current = parent } if place.origin == StructuralNavigation && selectedHeading { - doc, err := cursor.tree.Document(context.Background(), place.object) + doc, err := cursor.tree.DocumentView(context.Background(), place.object) if err != nil { return atspi.ObjectReference{}, err } @@ -190,12 +193,13 @@ func (cursor *DocumentCursor) sync_focus(ctx context.Context, focus atspi.Object if err != nil { return DocumentPosition{}, "focus-unresolved", err } - doc, err := cursor.tree.Document(ctx, ref) + doc, err := cursor.tree.NavigationDocument(ctx, ref) if err != nil { return DocumentPosition{}, "document-unresolved", err } place := documentPlace{document: doc.Context.TopLevel, object: ref, focus: ref} recovery := "new-document" + removeIndex := -1 for index, saved := range cursor.places { if saved.document != place.document { if saved.document.ID() == place.document.ID() { @@ -220,9 +224,19 @@ func (cursor *DocumentCursor) sync_focus(ctx context.Context, focus atspi.Object place.object = parent.Reference } } - cursor.places = append(cursor.places[:index], cursor.places[index+1:]...) + removeIndex = index break } + placeIndex, ok := doc.Index(place.object.ID()) + if !ok { + return DocumentPosition{}, "saved-object-missing", atspi.ErrStaleObject + } + if err := cursor.tree.ValidateDocumentRange(ctx, doc, placeIndex, placeIndex); err != nil { + return DocumentPosition{}, "document-unresolved", err + } + if removeIndex >= 0 { + cursor.places = append(cursor.places[:removeIndex], cursor.places[removeIndex+1:]...) + } remember_text_parent(doc, &place) if len(cursor.places) == 16 { cursor.places = cursor.places[1:] @@ -242,7 +256,7 @@ func (cursor *DocumentCursor) JumpToFragment(ctx context.Context, fragment strin return DocumentPosition{}, false, ErrNoCurrent } place := &cursor.places[len(cursor.places)-1] - doc, err := cursor.tree.Document(ctx, place.object) + doc, err := cursor.tree.DocumentView(ctx, place.object) if err != nil { return DocumentPosition{}, false, err } @@ -334,7 +348,7 @@ func (cursor *DocumentCursor) Move(ctx context.Context, target DocumentTarget, l return DocumentMoveResult{}, ErrNoCurrent } place := &cursor.places[len(cursor.places)-1] - doc, err := cursor.tree.Document(ctx, place.object) + doc, err := cursor.tree.NavigationDocument(ctx, place.object) if err != nil { return DocumentMoveResult{}, err } @@ -343,86 +357,64 @@ func (cursor *DocumentCursor) Move(ctx context.Context, target DocumentTarget, l if doc.Context.TopLevel != place.document { return DocumentMoveResult{}, atspi.ErrStaleObject } - index := -1 - for i, node := range doc.Nodes { - if node.Reference == place.object { - index = i - break - } - } - if index < 0 { + index, found := doc.Index(place.object.ID()) + if !found || doc.Nodes[index].Reference != place.object { return DocumentMoveResult{}, atspi.ErrStaleObject } - // Python treats a focused descendant as being within its nearest matching - // ancestor. Anchor the search there so previous navigation does not select - // and reannounce the containing link, heading, or other target. - indexByID := make(map[atspi.ObjectID]int, len(doc.Nodes)) - parentByID := make(map[atspi.ObjectID]atspi.ObjectID, len(doc.Nodes)) - for candidateIndex, node := range doc.Nodes { - indexByID[node.Reference.ID()] = candidateIndex - for _, child := range node.Children { - parentByID[child] = node.Reference.ID() - } + if err := cursor.tree.ValidateDocumentOrder(ctx, doc, index); err != nil { + return DocumentMoveResult{}, err } - for parent := place.object.ID(); parent.Valid(); parent = parentByID[parent] { - candidateIndex, ok := indexByID[parent] + // Anchor at the nearest matching ancestor without rebuilding parent maps. + for parent := place.object.ID(); parent.Valid(); { + candidate, ok := doc.Node(parent) if !ok { break } - if documentTargetMatches(target, doc.Nodes[candidateIndex], level) { - index = candidateIndex + if documentTargetMatches(target, candidate, level) { + index, _ = doc.Index(parent) break } + parent = candidate.Parent + } + commit := func(targetIndex int, wrapped bool) (DocumentMoveResult, error) { + if err := cursor.tree.ValidateDocumentRange(ctx, doc, targetIndex, doc.SubtreeEnd(targetIndex)); err != nil { + return DocumentMoveResult{}, err + } + place.tableCell = atspi.ObjectID{} + place.object, place.origin, place.offset = doc.Nodes[targetIndex].Reference, StructuralNavigation, 0 + remember_text_parent(doc, place) + return DocumentMoveResult{Position: documentPosition(doc, *place), Moved: true, Wrapped: wrapped}, nil } step := 1 if backwards { step = -1 } + // Validate every traversed node, including nonmatches and hidden nodes: a + // dirty candidate must be refreshed before it can be skipped safely. for i := index + step; i >= 0 && i < len(doc.Nodes); i += step { - node := doc.Nodes[i] - if node.Hidden { - continue + if err := cursor.tree.ValidateDocumentRange(ctx, doc, i-step, i); err != nil { + return DocumentMoveResult{}, err } - match := documentTargetMatches(target, node, level) - if match { - place.tableCell = atspi.ObjectID{} - place.object = node.Reference - place.origin = StructuralNavigation - place.offset = 0 - remember_text_parent(doc, place) - return DocumentMoveResult{Position: documentPosition(doc, *place), Moved: true}, nil + node := doc.Nodes[i] + if !node.Hidden && documentTargetMatches(target, node, level) { + return commit(i, false) } } - firstMatch, lastMatch := -1, -1 - for i := 0; i < len(doc.Nodes); i++ { - node := doc.Nodes[i] - if node.Hidden { - continue - } - if !documentTargetMatches(target, node, level) { - continue - } - if firstMatch < 0 { - firstMatch = i - } - lastMatch = i - } - wrappedIndex := firstMatch + start, end := 0, index if backwards { - wrappedIndex = lastMatch + start, end = len(doc.Nodes)-1, index } - if wrappedIndex >= 0 { - node := doc.Nodes[wrappedIndex] - place.tableCell = atspi.ObjectID{} - place.object = node.Reference - place.origin = StructuralNavigation - place.offset = 0 - remember_text_parent(doc, place) - return DocumentMoveResult{ - Position: documentPosition(doc, *place), - Moved: true, - Wrapped: true, - }, nil + for i := start; ; i += step { + if err := cursor.tree.ValidateDocumentRange(ctx, doc, i, i); err != nil { + return DocumentMoveResult{}, err + } + node := doc.Nodes[i] + if !node.Hidden && documentTargetMatches(target, node, level) { + return commit(i, true) + } + if i == end { + break + } } return DocumentMoveResult{Position: documentPosition(doc, *place)}, nil } @@ -536,12 +528,11 @@ func documentChunkRole(snapshot atspi.Snapshot) bool { } func documentObject(doc atspi.DocumentSnapshot, ref atspi.ObjectReference) (atspi.Snapshot, bool) { - for _, node := range doc.Nodes { - if node.Reference == ref { - return node.Snapshot, true - } + node, ok := doc.Node(ref.ID()) + if !ok || node.Reference != ref { + return atspi.Snapshot{}, false } - return atspi.Snapshot{}, false + return node.Snapshot, true } func documentPosition(doc atspi.DocumentSnapshot, place documentPlace) DocumentPosition { @@ -560,26 +551,19 @@ func documentPosition(doc atspi.DocumentSnapshot, place documentPlace) DocumentP // readable caret position, matching Python's heading navigation. A heading // starting with plain text has no unambiguous link target. func linkedHeadingNode(doc atspi.DocumentSnapshot, heading atspi.ObjectReference) (atspi.DocumentNode, bool, bool) { - byID := make(map[atspi.ObjectID]atspi.DocumentNode, len(doc.Nodes)) - parents := make(map[atspi.ObjectID]atspi.ObjectID, len(doc.Nodes)) - for _, node := range doc.Nodes { - byID[node.Reference.ID()] = node - for _, child := range node.Children { - parents[child] = node.Reference.ID() - } - } - root, ok := byID[heading.ID()] - if !ok || root.Snapshot.Role != "heading" { + root, ok := doc.Node(heading.ID()) + if !ok || root.Reference != heading || root.Snapshot.Role != "heading" { return atspi.DocumentNode{}, false, false } - for parent := parents[heading.ID()]; parent.Valid(); parent = parents[parent] { - node, found := byID[parent] + for parent := root.Parent; parent.Valid(); { + node, found := doc.Node(parent) if !found || atspi.IsWebDocumentRole(node.Snapshot.Role) { break } if node.Snapshot.Role == "link" { return node, true, false } + parent = node.Parent } var match atspi.DocumentNode count := 0 @@ -587,7 +571,7 @@ func linkedHeadingNode(doc atspi.DocumentSnapshot, heading atspi.ObjectReference for len(pending) != 0 && count < 2 { id := pending[len(pending)-1] pending = pending[:len(pending)-1] - node, found := byID[id] + node, found := doc.Node(id) if !found || node.Hidden || node.Snapshot.Role == "heading" { continue } @@ -607,7 +591,7 @@ func linkedHeadingNode(doc atspi.DocumentSnapshot, heading atspi.ObjectReference for len(pending) != 0 { id := pending[len(pending)-1] pending = pending[:len(pending)-1] - node, found := byID[id] + node, found := doc.Node(id) if !found || node.Hidden { continue } @@ -622,7 +606,7 @@ func linkedHeadingNode(doc atspi.DocumentSnapshot, heading atspi.ObjectReference continue } var firstLink atspi.DocumentNode - for ; id.Valid(); id = parents[id] { + for id.Valid() { if id == heading.ID() { if firstLink.Reference.Valid() { // Gecko may omit plain text between child accessibles. @@ -636,13 +620,17 @@ func linkedHeadingNode(doc atspi.DocumentSnapshot, heading atspi.ObjectReference } break } - node := byID[id] + node, found := doc.Node(id) + if !found { + break + } if node.Snapshot.Role == "heading" || atspi.IsWebDocumentRole(node.Snapshot.Role) { break } if node.Snapshot.Role == "link" && !firstLink.Reference.Valid() { firstLink = node } + id = node.Parent } // Only the heading's starting caret position can select a link. break diff --git a/internal/review/document_benchmark_test.go b/internal/review/document_benchmark_test.go index d07db22..a349846 100644 --- a/internal/review/document_benchmark_test.go +++ b/internal/review/document_benchmark_test.go @@ -8,11 +8,87 @@ import ( "context" "fmt" "testing" + "time" "unicode/utf8" "git.stormux.org/storm/cthulhu/internal/atspi" ) +// Include focus synchronization and worker bookkeeping, which the primitive +// heading benchmark intentionally excludes. Native provider work is separate. +func BenchmarkDocumentWorkerHeadingNavigation(b *testing.B) { + for _, contentNodes := range []int{100, 1000, 10000} { + b.Run(fmt.Sprintf("nodes-%d", contentNodes), func(b *testing.B) { + tree, ids := benchmark_document_tree(b, contentNodes) + cursor, err := NewDocumentCursor(tree) + if err != nil { + b.Fatal(err) + } + worker, err := NewWorker(&fakeCapturer{}, time.Second) + if err != nil { + b.Fatal(err) + } + middle := contentNodes / 40 * 20 + worker.SyncFocus(ids[middle]) + ctx := context.Background() + if _, err := cursor.SyncFocus(ctx, ids[middle]); err != nil { + b.Fatal(err) + } + b.ReportAllocs() + for b.Loop() { + for _, backwards := range []bool{false, true} { + result, next := worker.moveDocument(ctx, cursor, DocumentHeading, 2, backwards, 0) + cursor = next + want := ids[middle+20] + if backwards { + want = ids[middle] + } + if result.Err != nil || !result.Moved || result.DocumentPosition.Object.ID() != want || result.DocumentDiagnostics.CaptureAttempts != 0 { + b.Fatalf("worker move: moved=%v error=%v", result.Moved, result.Err) + } + } + } + }) + } +} + +func BenchmarkDocumentCaretNavigation(b *testing.B) { + for _, contentNodes := range []int{100, 1000, 10000} { + b.Run(fmt.Sprintf("nodes-%d", contentNodes), func(b *testing.B) { + tree, ids := benchmark_document_tree(b, contentNodes) + cursor, err := NewDocumentCursor(tree) + if err != nil { + b.Fatal(err) + } + middle := contentNodes/2 + 2 + ctx := context.Background() + if _, err := cursor.SyncFocus(ctx, ids[middle]); err != nil { + b.Fatal(err) + } + reader := caretTextReader{ids[middle]: fmt.Sprintf("Café sample %d", middle)} + // Build the view's reusable ranges before measuring repeated keys. + for _, command := range []DocumentCaretCommand{CaretNextCharacter, CaretPreviousCharacter} { + if _, err := cursor.MoveCaret(ctx, reader, command); err != nil { + b.Fatal(err) + } + } + b.ReportAllocs() + for b.Loop() { + for _, command := range []DocumentCaretCommand{CaretNextCharacter, CaretPreviousCharacter} { + move, err := cursor.MoveCaret(ctx, reader, command) + want := 1 + if command == CaretPreviousCharacter { + want = 0 + } + if err != nil || !move.Moved || move.Position.Object.ID() != ids[middle] || move.Position.Offset != want { + b.Fatalf("caret move: moved=%v error=%v", move.Moved, err) + } + } + } + }) + } +} + // benchmark_document_tree models an already captured page, not native AT-SPI // capture. Each section has twenty content objects: a heading, a link, and // ordinary text. Pairs of objects share a visual row, including Unicode text. diff --git a/internal/review/document_braille_routing.go b/internal/review/document_braille_routing.go index 880fce7..e69ccc0 100644 --- a/internal/review/document_braille_routing.go +++ b/internal/review/document_braille_routing.go @@ -72,7 +72,7 @@ func (worker *Worker) route_document_braille(ctx context.Context, cursor *Docume result.Err = atspi.ErrNoDocumentCaret return } - doc, err := cursor.tree.Document(ctx, target) + doc, err := cursor.tree.DocumentView(ctx, target) if err != nil { result.Err = err return diff --git a/internal/review/document_caret.go b/internal/review/document_caret.go index ed42bb3..d1cc6f5 100644 --- a/internal/review/document_caret.go +++ b/internal/review/document_caret.go @@ -83,37 +83,85 @@ func (cursor *DocumentCursor) MoveCaret(ctx context.Context, reader DocumentCare if !cursor.active.Valid() || len(cursor.places) == 0 { return DocumentCaretMove{}, ErrNoCurrent } - place := &cursor.places[len(cursor.places)-1] - doc, err := cursor.tree.Document(ctx, place.object) + saved := &cursor.places[len(cursor.places)-1] + next := *saved + place := &next + doc, err := cursor.tree.NavigationDocument(ctx, place.object) if err != nil { return DocumentCaretMove{}, err } if doc.Context.TopLevel != place.document { return DocumentCaretMove{}, atspi.ErrStaleObject } - if command >= TableLeft { - return cursor.moveTable(ctx, doc, place, command) - } - place.tableCell = atspi.ObjectID{} - _, current := caret_node_indices(doc.Nodes, place.object) - indices := caretNodeRanges(doc.Nodes) - if current < 0 { + current, found := doc.Index(place.object.ID()) + if !found || doc.Nodes[current].Reference != place.object { return DocumentCaretMove{}, atspi.ErrStaleObject } + if err := cursor.tree.ValidateDocumentRange(ctx, doc, current, current); err != nil { + return DocumentCaretMove{}, err + } + if command >= TableLeft { + // Table coordinates and row/header presentation consume the complete + // table, unlike ordinary caret movement's local text path. + if err := cursor.tree.ValidateDocumentRange(ctx, doc, 0, len(doc.Nodes)-1); err != nil { + return DocumentCaretMove{}, err + } + movement, err := cursor.moveTable(ctx, doc, place, command) + if err == nil && movement.Moved { + *saved = next + } + return movement, err + } + place.tableCell = atspi.ObjectID{} + if !doc.SameContent(cursor.caretDocument) { + cursor.caretDocument = doc + cursor.caretRanges = caretNodeRanges(doc.Nodes) + cursor.lineOwners = visual_line_owners(doc.Nodes) + } + indices := cursor.caretRanges if len(indices) == 0 { + if err := cursor.tree.ValidateDocumentRange(ctx, doc, 0, len(doc.Nodes)-1); err != nil { + return DocumentCaretMove{}, err + } return DocumentCaretMove{Position: documentPosition(doc, *place)}, nil } var movement DocumentCaretMove + navigationReader := &navigationTextReader{DocumentCaretReader: reader, tree: cursor.tree, document: doc, anchor: current} + reader = navigationReader switch command { case CaretNextWord, CaretPreviousWord, CaretNextCharacter, CaretPreviousCharacter: movement, err = moveCaretText(ctx, reader, doc, place, indices, current, command == CaretPreviousWord || command == CaretPreviousCharacter, command == CaretNextWord || command == CaretPreviousWord) default: - movement, err = move_caret_line(ctx, reader, cursor.tree, doc, place, indices, current, command) + movement, err = move_caret_line(ctx, reader, cursor.tree, doc, place, indices, current, command, cursor.lineOwners) } if err == nil && movement.Moved { + target, found := doc.Index(place.object.ID()) + if !found { + return DocumentCaretMove{}, atspi.ErrStaleObject + } + // Successful text already belongs to this move even if the provider + // removes its ancestor immediately afterwards. Repeating that move + // 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 { + return DocumentCaretMove{}, err + } + } remember_text_parent(doc, place) + *saved = next + } else if err == nil { + // Reaching a boundary must not silently skip newly readable text in a + // dirty object which had no caret range in the captured view. + boundary := len(doc.Nodes) - 1 + if command == CaretPreviousCharacter || command == CaretPreviousWord || command == CaretPreviousLine || command == CaretDocumentStart { + boundary = 0 + } + if err := cursor.tree.ValidateDocumentRange(ctx, doc, current, boundary); err != nil { + return DocumentCaretMove{}, err + } } return movement, err } diff --git a/internal/review/document_lists.go b/internal/review/document_lists.go index d68e5b2..a295044 100644 --- a/internal/review/document_lists.go +++ b/internal/review/document_lists.go @@ -115,7 +115,7 @@ func (worker *Worker) listDocument(ctx context.Context, cursor *DocumentCursor, if err != nil { return list, err } - doc, err := tree.Document(captureContext, ref) + doc, err := tree.DocumentView(captureContext, ref) if err != nil { return list, err } @@ -206,7 +206,7 @@ func (worker *Worker) selectDocumentList(ctx context.Context, cursor *DocumentCu result.Err = err return result, cursor } - doc, err := list.tree.Document(ctx, position.Object) + doc, err := list.tree.DocumentView(ctx, position.Object) if err != nil { result.Err = err return result, cursor diff --git a/internal/review/document_native_caret.go b/internal/review/document_native_caret.go index 4e6e5fd..e099d7e 100644 --- a/internal/review/document_native_caret.go +++ b/internal/review/document_native_caret.go @@ -24,7 +24,7 @@ func (cursor *DocumentCursor) syncNativeCaret(ctx context.Context, reader Docume cursor.mu.Lock() defer cursor.mu.Unlock() place := &cursor.places[len(cursor.places)-1] - doc, err := cursor.tree.Document(ctx, place.focus) + doc, err := cursor.tree.DocumentView(ctx, place.focus) if err != nil { return err } @@ -40,11 +40,7 @@ func (cursor *DocumentCursor) syncNativeCaret(ctx context.Context, reader Docume } func native_caret_node(ctx context.Context, reader DocumentCaretReader, doc atspi.DocumentSnapshot, focus atspi.ObjectID, offset int) (atspi.DocumentNode, int, error) { - nodes := make(map[atspi.ObjectID]atspi.DocumentNode, len(doc.Nodes)) - for _, node := range doc.Nodes { - nodes[node.Reference.ID()] = node - } - node := nodes[focus] + node, _ := doc.Node(focus) if !node.Reference.Valid() || offset < 0 || offset > node.Snapshot.CharacterCount || node.Snapshot.Editable || node.Snapshot.Role == "password text" { return atspi.DocumentNode{}, 0, atspi.ErrNoDocumentCaret } @@ -60,7 +56,7 @@ func native_caret_node(ctx context.Context, reader DocumentCaretReader, doc atsp if !embedded.Valid() { return node, offset, nil } - child, ok := nodes[embedded] + child, ok := doc.Node(embedded) if !ok || child.Hidden || child.Snapshot.Editable || child.Snapshot.Role == "password text" { return atspi.DocumentNode{}, 0, atspi.ErrNoDocumentCaret } @@ -70,7 +66,7 @@ func native_caret_node(ctx context.Context, reader DocumentCaretReader, doc atsp if len(node.Children) != 1 { return atspi.DocumentNode{}, 0, atspi.ErrNoDocumentCaret } - child, ok := nodes[node.Children[0]] + child, ok := doc.Node(node.Children[0]) if !ok || child.Hidden || child.Snapshot.Editable || child.Snapshot.CharacterCount != node.Snapshot.CharacterCount { return atspi.DocumentNode{}, 0, atspi.ErrNoDocumentCaret } diff --git a/internal/review/document_navigation_reader.go b/internal/review/document_navigation_reader.go new file mode 100644 index 0000000..47fc453 --- /dev/null +++ b/internal/review/document_navigation_reader.go @@ -0,0 +1,46 @@ +// Copyright (c) 2026 Storm Dragon +// Authors: Storm Dragon, Angus Kola +// SPDX-License-Identifier: GPL-3.0-or-later + +package review + +import ( + "context" + + "git.stormux.org/storm/cthulhu/internal/atspi" +) + +// One move owns this wrapper. Validate the path before native reads so stale +// range lengths and omitted candidates trigger recovery without consuming a +// different object's text or advancing the retained cursor. +type navigationTextReader struct { + DocumentCaretReader + tree *atspi.AccessibleTree + document atspi.DocumentSnapshot + anchor int + readObjects []atspi.ObjectReference +} + +func (reader *navigationTextReader) ReadDocumentCaret(ctx context.Context, object atspi.ObjectReference, offset int, unit atspi.DocumentCaretUnit) (atspi.DocumentCaretText, error) { + index, found := reader.document.Index(object.ID()) + 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 { + return atspi.DocumentCaretText{}, err + } + text, err := reader.DocumentCaretReader.ReadDocumentCaret(ctx, object, offset, unit) + if err == nil { + reader.readObjects = append(reader.readObjects, object) + } + return text, err +} + +func (reader *navigationTextReader) did_read(object atspi.ObjectReference) bool { + for _, read := range reader.readObjects { + if read == object { + return true + } + } + return false +} diff --git a/internal/review/document_navigation_test.go b/internal/review/document_navigation_test.go new file mode 100644 index 0000000..6504934 --- /dev/null +++ b/internal/review/document_navigation_test.go @@ -0,0 +1,83 @@ +// Copyright (c) 2026 Storm Dragon +// Authors: Storm Dragon, Angus Kola +// SPDX-License-Identifier: GPL-3.0-or-later + +package review + +import ( + "context" + "errors" + "testing" + + "git.stormux.org/storm/cthulhu/internal/atspi" +) + +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}, + {"semantics", func(tree *atspi.AccessibleTree) { tree.InvalidateSnapshot(webID("/next")) }, atspi.ErrStaleSnapshot}, + {"geometry", func(tree *atspi.AccessibleTree) { tree.InvalidateGeometry(webID("/next")) }, atspi.ErrStaleGeometry}, + } { + t.Run(test.name, func(t *testing.T) { + reader := caretTextReader{webID("/text"): "Alpha", webID("/next"): "Next"} + cursor := newCaretTextCursor(t, reader) + test.invalidate(cursor.tree) + if _, err := cursor.SyncFocus(context.Background(), webID("/text")); err != nil { + t.Fatal("unrelated update blocked focus synchronization", err) + } + move, err := cursor.MoveCaret(context.Background(), reader, CaretNextCharacter) + if err != nil || !move.Moved || move.Text != "l" { + t.Fatalf("local character move: %+v %v", move, err) + } + _, err = cursor.MoveCaret(context.Background(), reader, CaretNextLine) + if !errors.Is(err, test.want) { + t.Fatalf("dirty target read: %v, want %v", err, test.want) + } + if place := cursor.places[0]; place.object.ID() != webID("/text") || place.offset != 1 { + t.Fatalf("failed move changed position: %+v", place) + } + }) + } +} + +func TestDocumentCaretKeepsLocalTextThroughParentInsertion(t *testing.T) { + reader := caretTextReader{webID("/text"): "Alpha", webID("/next"): "Next"} + cursor := newCaretTextCursor(t, reader) + cursor.tree.MarkChildrenChanged(webID("/doc")) + move, err := cursor.MoveCaret(context.Background(), reader, CaretNextCharacter) + 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) + } +} + +func TestDocumentStructuralMoveDefersUnrelatedBranch(t *testing.T) { + nodes := []atspi.TreeNode{ + {Snapshot: atspi.Snapshot{BusName: webID("/doc").BusName, Path: "/doc", Role: "document web"}, Children: []atspi.ObjectID{webID("/one"), webID("/two"), webID("/later")}}, + {Snapshot: atspi.Snapshot{BusName: webID("/one").BusName, Path: "/one", Role: "heading", Name: "First"}}, + {Snapshot: atspi.Snapshot{BusName: webID("/two").BusName, Path: "/two", Role: "heading", Name: "Second"}}, + {Snapshot: atspi.Snapshot{BusName: webID("/later").BusName, Path: "/later", Role: "static", Name: "Later"}}, + } + tree, err := atspi.NewAccessibleTree(webID("/doc"), nodes) + if err != nil { + t.Fatal(err) + } + cursor, _ := NewDocumentCursor(tree) + if _, err := cursor.SyncFocus(context.Background(), webID("/one")); err != nil { + t.Fatal(err) + } + tree.InvalidateText(webID("/later")) + move, err := cursor.Move(context.Background(), DocumentHeading, 0, false) + if err != nil || !move.Moved || move.Position.Object.ID() != webID("/two") { + t.Fatalf("unrelated update blocked adjacent heading: %+v %v", move, err) + } + if _, err := cursor.Move(context.Background(), DocumentHeading, 0, false); !errors.Is(err, atspi.ErrStaleText) { + t.Fatal("wrapped across dirty content", err) + } +} diff --git a/internal/review/document_replacement.go b/internal/review/document_replacement.go index 4d6298b..3fb233d 100644 --- a/internal/review/document_replacement.go +++ b/internal/review/document_replacement.go @@ -14,50 +14,43 @@ func remember_text_parent(doc atspi.DocumentSnapshot, place *documentPlace) { place.textParent = atspi.ObjectReference{} place.textPrevious, place.textNext = atspi.ObjectReference{}, atspi.ObjectReference{} place.recoveryParents = nil - // Nodes are in provider preorder. Walking backwards follows the parent - // chain in one scan, without rescanning all nodes for each ancestor. - var parentID atspi.ObjectID - plainText := false - for index := len(doc.Nodes) - 1; index >= 0; index-- { - node := doc.Nodes[index] - if node.Reference == place.object { - parentID, plainText = node.Parent, replacement_text_leaf(node) - continue + node, ok := doc.Node(place.object.ID()) + if !ok || node.Reference != place.object { + return + } + plainText := replacement_text_leaf(node) + for parentID := node.Parent; parentID.Valid(); { + parent, ok := doc.Node(parentID) + if !ok { + break } - if !parentID.Valid() || node.Reference.ID() != parentID { - continue - } - if len(place.recoveryParents) == 0 && plainText && replacement_text_parent(node) { - place.textParent = node.Reference - for childIndex, id := range node.Children { + 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 } - if len(node.Children) == 1 { - break - } - for _, sibling := range doc.Nodes { - if childIndex > 0 && sibling.Reference.ID() == node.Children[childIndex-1] { + if childIndex > 0 { + if sibling, ok := doc.Node(parent.Children[childIndex-1]); ok { place.textPrevious = sibling.Reference } - if childIndex+1 < len(node.Children) && sibling.Reference.ID() == node.Children[childIndex+1] { + } + if childIndex+1 < len(parent.Children) { + if sibling, ok := doc.Node(parent.Children[childIndex+1]); ok { place.textNext = sibling.Reference } - if (childIndex == 0 || place.textPrevious.Valid()) && (childIndex+1 == len(node.Children) || place.textNext.Valid()) { - break - } } - if childIndex > 0 && !place.textPrevious.Valid() || childIndex+1 < len(node.Children) && !place.textNext.Valid() { + if childIndex > 0 && !place.textPrevious.Valid() || childIndex+1 < len(parent.Children) && !place.textNext.Valid() { place.textParent = atspi.ObjectReference{} } break } } - if node.Reference == place.document || len(place.recoveryParents) == 32 { + if parent.Reference == place.document || len(place.recoveryParents) == 32 { break } - place.recoveryParents = append(place.recoveryParents, node.Reference) - parentID = node.Parent + place.recoveryParents = append(place.recoveryParents, parent.Reference) + parentID = parent.Parent } } diff --git a/internal/review/document_visual_line.go b/internal/review/document_visual_line.go index 64efaf5..b94f0ba 100644 --- a/internal/review/document_visual_line.go +++ b/internal/review/document_visual_line.go @@ -48,9 +48,8 @@ func (reader *visualLineReader) ReadDocumentCaret(ctx context.Context, object at return text, err } -func move_caret_line(ctx context.Context, source DocumentCaretReader, tree *atspi.AccessibleTree, doc atspi.DocumentSnapshot, place *documentPlace, indices []caretRange, current int, command DocumentCaretCommand) (DocumentCaretMove, error) { +func move_caret_line(ctx context.Context, source DocumentCaretReader, tree *atspi.AccessibleTree, doc atspi.DocumentSnapshot, place *documentPlace, indices []caretRange, current int, command DocumentCaretCommand, owners map[int]atspi.ObjectID) (DocumentCaretMove, error) { reader := &visualLineReader{DocumentCaretReader: source, cache: make(map[visualLineKey]atspi.DocumentCaretText)} - owners := visual_line_owners(doc.Nodes) next := *place // Move beyond the entire current line, even when character/word movement // left the cursor in a middle fragment or structural navigation chose its diff --git a/internal/review/document_worker_test.go b/internal/review/document_worker_test.go index f96c881..6c93ec8 100644 --- a/internal/review/document_worker_test.go +++ b/internal/review/document_worker_test.go @@ -870,7 +870,10 @@ func TestWorkerDocumentRecapturePreservesPosition(t *testing.T) { call := <-capturer.calls call.result <- captureResult{tree: tree} waitResult(t, worker) - tree.InvalidateText(webID("/link")) + // Only a traversed branch requires recapture now. The hidden heading is + // between the current and requested positions; it cannot be skipped until + // its invalidated data has been refreshed. + tree.InvalidateText(webID("/hidden")) worker.MoveDocument(DocumentHeading, 0, true) call = <-capturer.calls nodes := webNodes() @@ -889,7 +892,7 @@ func TestWorkerDocumentRecapturePreservesPosition(t *testing.T) { t.Fatalf("recapture diagnostics = %+v", diagnostics) } failed, recovered := diagnostics.Attempts[0], diagnostics.Attempts[1] - if failed.Stage != "focus" || failed.Recovery != "document-unresolved" || failed.Error == "" || + if failed.Stage != "move" || failed.Recovery != "preserved" || failed.Error == "" || recovered.Recovery != "preserved" || recovered.Error != "" || recovered.Before.Object != webID("/second") || recovered.Synced.Object != webID("/second") || recovered.After.Object != webID("/first") { t.Fatalf("recapture lost diagnostic history: %+v", diagnostics) @@ -1015,22 +1018,35 @@ func TestDocumentCaretKeepsOffsetAfterDocumentRefresh(t *testing.T) { for _, node := range doc.Nodes { nodes = append(nodes, atspi.TreeNode{Snapshot: node.Snapshot, Children: node.Children, Geometry: node.Geometry, Text: node.Text}) } - wantReason := atspi.ErrStaleSnapshot + var wantReason error = atspi.ErrStaleSnapshot if kind == "children" { cursor.tree.MarkChildrenChanged(webID("/doc")) - wantReason = atspi.ErrIncompleteTree + wantReason = nil } else { cursor.tree.InvalidateSnapshot(webID("/text")) } - go func() { + refresh := func() { call := <-navigator.calls err := cursor.tree.Replace(webID("/doc"), nodes) call.result <- captureResult{tree: cursor.tree, err: err} - }() + } + if kind != "children" { + go refresh() + } result, _ := worker.moveDocumentCaret(ctx, cursor, CaretNextLine, worker.documentEpoch.Load()) if result.Err != nil || !errors.Is(result.CaptureReason, wantReason) || result.DocumentCaretText != "Third" { t.Fatalf("semantic refresh restarted or skipped reading: %+v", result) } + if kind == "children" { + 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) + } + } }) } }