From 290b5fe82163ccfa4b85413e8c0e50ab9432d6ec Mon Sep 17 00:00:00 2001
From: Veikko Sariola <5684185+vsariola@users.noreply.github.com>
Date: Wed, 16 Aug 2023 16:15:14 +0300
Subject: [PATCH] widget: click button only if key pressed and released

This commit fixes the non-intuitive behaviour, where hitting return or
space with a button focused, then tabbing to another button and
releasing the key causes the second button to trigger. It feels wrong,
as the "gesture" was never initiated on the second button. The fix makes
widget.Clickable track which key was pressed, in a variable called
pressedKey, and only considers a key release if the released key matches
the pressed key. Finally, if the widget loses focus, pressedKey is
cleared.

Fixes: https://todo.sr.ht/~eliasnaur/gio/525
Signed-off-by: Veikko Sariola <5684185+vsariola@users.noreply.github.com>
---
 widget/button.go      |  24 ++++++++--
 widget/button_test.go | 104 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 5 deletions(-)
 create mode 100644 widget/button_test.go

diff --git a/widget/button.go b/widget/button.go
index dcd81180..410ba201 100644
--- a/widget/button.go
+++ b/widget/button.go
@@ -28,6 +28,7 @@ type Clickable struct {
 	keyTag       struct{}
 	requestFocus bool
 	focused      bool
+	pressedKey   string
 }
 
 // Click represents a click.
@@ -178,17 +179,30 @@ func (b *Clickable) update(gtx layout.Context) {
 		switch e := e.(type) {
 		case key.FocusEvent:
 			b.focused = e.Focus
+			if !b.focused {
+				b.pressedKey = ""
+			}
 		case key.Event:
-			if !b.focused || e.State != key.Release {
+			if !b.focused {
 				break
 			}
 			if e.Name != key.NameReturn && e.Name != key.NameSpace {
 				break
 			}
-			b.clicks = append(b.clicks, Click{
-				Modifiers: e.Modifiers,
-				NumClicks: 1,
-			})
+			switch e.State {
+			case key.Press:
+				b.pressedKey = e.Name
+			case key.Release:
+				if b.pressedKey != e.Name {
+					break
+				}
+				// only register a key as a click if the key was pressed and released while this button was focused
+				b.pressedKey = ""
+				b.clicks = append(b.clicks, Click{
+					Modifiers: e.Modifiers,
+					NumClicks: 1,
+				})
+			}
 		}
 	}
 }
diff --git a/widget/button_test.go b/widget/button_test.go
new file mode 100644
index 00000000..9bb712f5
--- /dev/null
+++ b/widget/button_test.go
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: Unlicense OR MIT
+
+package widget_test
+
+import (
+	"image"
+	"testing"
+
+	"gioui.org/io/key"
+	"gioui.org/io/router"
+	"gioui.org/io/system"
+	"gioui.org/layout"
+	"gioui.org/op"
+	"gioui.org/widget"
+)
+
+func TestClickable(t *testing.T) {
+	var (
+		ops op.Ops
+		r   router.Router
+		b1  widget.Clickable
+		b2  widget.Clickable
+	)
+	gtx := layout.NewContext(&ops, system.FrameEvent{Queue: &r})
+	layout := func() {
+		b1.Layout(gtx, func(gtx layout.Context) layout.Dimensions {
+			return layout.Dimensions{Size: image.Pt(100, 100)}
+		})
+		// buttons are on top of each other but we only use focus and keyevents, so this is fine
+		b2.Layout(gtx, func(gtx layout.Context) layout.Dimensions {
+			return layout.Dimensions{Size: image.Pt(100, 100)}
+		})
+	}
+	frame := func() {
+		ops.Reset()
+		layout()
+		r.Frame(gtx.Ops)
+	}
+	// frame: request focus for button 1
+	b1.Focus()
+	frame()
+	// frame: gain focus for button 1
+	frame()
+	if !b1.Focused() {
+		t.Error("button 1 did not gain focus")
+	}
+	if b2.Focused() {
+		t.Error("button 2 should not have focus")
+	}
+	// frame: press & release return
+	r.Queue(
+		key.Event{
+			Name:  key.NameReturn,
+			State: key.Press,
+		},
+		key.Event{
+			Name:  key.NameReturn,
+			State: key.Release,
+		},
+	)
+	frame()
+	if !b1.Clicked() {
+		t.Error("button 1 did not get clicked when it got return press & release")
+	}
+	if b2.Clicked() {
+		t.Error("button 2 got clicked when it did not have focus")
+	}
+	// frame: press return down
+	r.Queue(
+		key.Event{
+			Name:  key.NameReturn,
+			State: key.Press,
+		},
+	)
+	frame()
+	if b1.Clicked() {
+		t.Error("button 1 got clicked, even if it only got return press")
+	}
+	// frame: request focus for button 2
+	b2.Focus()
+	frame()
+	// frame: gain focus for button 2
+	frame()
+	if b1.Focused() {
+		t.Error("button 1 should not have focus")
+	}
+	if !b2.Focused() {
+		t.Error("button 2 did not gain focus")
+	}
+	// frame: release return
+	r.Queue(
+		key.Event{
+			Name:  key.NameReturn,
+			State: key.Release,
+		},
+	)
+	frame()
+	if b1.Clicked() {
+		t.Error("button 1 got clicked, even if it had lost focus")
+	}
+	if b2.Clicked() {
+		t.Error("button 2 should not have been clicked, as it only got return release")
+	}
+}