From e9cb0b326dcb1a41028ba3ab50fa951dcbcf8438 Mon Sep 17 00:00:00 2001
From: Chris Waldon <christopher.waldon.dev@gmail.com>
Date: Fri, 18 Aug 2023 10:57:55 -0400
Subject: [PATCH] io/router: fix system action routing logic

When running ActionAt, the router used to only consider the topmost clip area, even
if that clip area had no input handlers attached whatsoever. This change updates the
logic for that test to use the same traversal as normal event handling, ensuring that
action inputs behave intuitively like any other pointer input area. Included is a test
catching the problematic behavior that prompted this change.

Signed-off-by: Chris Waldon <christopher.waldon.dev@gmail.com>
---
 io/router/pointer.go      | 36 +++++++++++++++++++++++-------------
 io/router/pointer_test.go | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/io/router/pointer.go b/io/router/pointer.go
index e95bc6e7..1e92483a 100644
--- a/io/router/pointer.go
+++ b/io/router/pointer.go
@@ -432,17 +432,15 @@ func (q *pointerQueue) semanticIDFor(content semanticContent) SemanticID {
 	return id.id
 }
 
-func (q *pointerQueue) ActionAt(pos f32.Point) (system.Action, bool) {
-	for i := len(q.hitTree) - 1; i >= 0; i-- {
-		n := &q.hitTree[i]
-		hit, _ := q.hit(n.area, pos)
-		if !hit {
-			continue
-		}
+func (q *pointerQueue) ActionAt(pos f32.Point) (action system.Action, hasAction bool) {
+	q.hitTest(pos, func(n *hitNode) {
 		area := q.areas[n.area]
-		return area.action, area.action != 0
-	}
-	return 0, false
+		if area.action != 0 {
+			action = area.action
+			hasAction = true
+		}
+	})
+	return action, hasAction
 }
 
 func (q *pointerQueue) SemanticAt(pos f32.Point) (SemanticID, bool) {
@@ -461,10 +459,14 @@ func (q *pointerQueue) SemanticAt(pos f32.Point) (SemanticID, bool) {
 	return 0, false
 }
 
-func (q *pointerQueue) opHit(pos f32.Point) ([]event.Tag, pointer.Cursor) {
+// hitTest searches the hit tree for nodes matching pos. Any node matching pos will
+// have the onNode func invoked on it to allow the caller to extract whatever information
+// is necessary for further processing. Providing this algorithm in this generic way
+// allows normal event routing and system action event routing to share the same traversal
+// logic even though they are interested in different aspects of hit nodes.
+func (q *pointerQueue) hitTest(pos f32.Point, onNode func(*hitNode)) pointer.Cursor {
 	// Track whether we're passing through hits.
 	pass := true
-	hits := q.scratch[:0]
 	idx := len(q.hitTree) - 1
 	cursor := pointer.CursorDefault
 	for idx >= 0 {
@@ -483,12 +485,20 @@ func (q *pointerQueue) opHit(pos f32.Point) ([]event.Tag, pointer.Cursor) {
 		} else {
 			idx = n.next
 		}
+		onNode(n)
+	}
+	return cursor
+}
+
+func (q *pointerQueue) opHit(pos f32.Point) ([]event.Tag, pointer.Cursor) {
+	hits := q.scratch[:0]
+	cursor := q.hitTest(pos, func(n *hitNode) {
 		if n.tag != nil {
 			if _, exists := q.handlers[n.tag]; exists {
 				hits = addHandler(hits, n.tag)
 			}
 		}
-	}
+	})
 	q.scratch = hits[:0]
 	return hits, cursor
 }
diff --git a/io/router/pointer_test.go b/io/router/pointer_test.go
index 9decd302..46516c58 100644
--- a/io/router/pointer_test.go
+++ b/io/router/pointer_test.go
@@ -14,6 +14,7 @@ import (
 	"gioui.org/io/event"
 	"gioui.org/io/key"
 	"gioui.org/io/pointer"
+	"gioui.org/io/system"
 	"gioui.org/io/transfer"
 	"gioui.org/op"
 	"gioui.org/op/clip"
@@ -221,6 +222,30 @@ func TestPointerTypes(t *testing.T) {
 	assertEventPointerTypeSequence(t, r.Events(handler), pointer.Cancel, pointer.Press, pointer.Release)
 }
 
+func TestPointerSystemAction(t *testing.T) {
+	t.Run("simple", func(t *testing.T) {
+		var ops op.Ops
+		r1 := clip.Rect(image.Rect(0, 0, 100, 100)).Push(&ops)
+		system.ActionInputOp(system.ActionMove).Add(&ops)
+		r1.Pop()
+
+		var r Router
+		r.Frame(&ops)
+		assertActionAt(t, r, f32.Pt(50, 50), system.ActionMove)
+	})
+	t.Run("covered by another clip", func(t *testing.T) {
+		var ops op.Ops
+		r1 := clip.Rect(image.Rect(0, 0, 100, 100)).Push(&ops)
+		system.ActionInputOp(system.ActionMove).Add(&ops)
+		clip.Rect(image.Rect(0, 0, 100, 100)).Push(&ops).Pop()
+		r1.Pop()
+
+		var r Router
+		r.Frame(&ops)
+		assertActionAt(t, r, f32.Pt(50, 50), system.ActionMove)
+	})
+}
+
 func TestPointerPriority(t *testing.T) {
 	handler1 := new(int)
 	handler2 := new(int)
@@ -1231,6 +1256,17 @@ func assertScrollEvent(t *testing.T, ev event.Event, scroll f32.Point) {
 	}
 }
 
+// assertActionAt checks that the router has a system action of the expected type at point.
+func assertActionAt(t *testing.T, q Router, point f32.Point, expected system.Action) {
+	t.Helper()
+	action, ok := q.ActionAt(point)
+	if !ok {
+		t.Errorf("expected action %v at %v, got no action", expected, point)
+	} else if action != expected {
+		t.Errorf("expected action %v at %v, got %v", expected, point, action)
+	}
+}
+
 func BenchmarkRouterAdd(b *testing.B) {
 	// Set this to the number of overlapping handlers that you want to
 	// evaluate performance for. Typical values for the example applications