Skip to content

Commit c3fffb5

Browse files
committed
fix(v3): name only presses that can actually be bound, on macOS
Addresses both review comments on #6032, each confirmed with a test that fails against the previous code first. A press with no name was still named after its modifiers alone, producing strings like "cmd" that no binding can match and that parseAccelerator rejects outright - so each one was reported as an error rather than ignored. The name is now empty in that case, and -keyDown: does not send an empty name on, which would only have moved the error rather than removed it. -performKeyEquivalent: already declined to. F21 to F35 and the keypad's Clear key had no name at all. namedKeys accepts f21 through f35 and numlock, so bindings can be written for them, but nothing on macOS ever produced those names: the keys exist only on extended keyboards, where AppKit reports them through characters rather than through a key code this build knows. They are named from their characters now - NSF21FunctionKey and consecutive, and NSClearLineFunctionKey for numlock. Neither was introduced by this pull request; both were carried over from the Objective-C the branch replaced. Also verified against a running application: bindings still fire, and no accelerator now fails to parse. Claude-Session: https://claude.ai/code/session_01WP7MWnMpQDT2WB7CPLWbqr
1 parent f62a500 commit c3fffb5

3 files changed

Lines changed: 94 additions & 10 deletions

File tree

v3/pkg/application/accelerator_darwin.go

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,30 @@ var macCharacterNames = map[rune]string{
6565
'\x0e': "page up",
6666
'\x01': "home",
6767
'\x04': "end",
68+
69+
// F21 to F35 exist only on extended keyboards, and AppKit reports them
70+
// through these characters rather than through a key code this build
71+
// knows. namedKeys accepts their names, so without this a binding written
72+
// for one could never fire. NSF21FunctionKey and consecutive.
73+
'\uf718': "f21",
74+
'\uf719': "f22",
75+
'\uf71a': "f23",
76+
'\uf71b': "f24",
77+
'\uf71c': "f25",
78+
'\uf71d': "f26",
79+
'\uf71e': "f27",
80+
'\uf71f': "f28",
81+
'\uf720': "f29",
82+
'\uf721': "f30",
83+
'\uf722': "f31",
84+
'\uf723': "f32",
85+
'\uf724': "f33",
86+
'\uf725': "f34",
87+
'\uf726': "f35",
88+
89+
// NSClearLineFunctionKey - the key a Mac keypad labels Clear, and which
90+
// namedKeys calls numlock.
91+
'\uf739': "numlock",
6892
}
6993

7094
// macKeyName names the key that was pressed, or returns an empty string for a
@@ -83,6 +107,14 @@ func macKeyName(keyCode uint16, character rune, hasCharacter bool) string {
83107
// modifiers held, in a fixed order, then the key, joined with "+". Empty if
84108
// the key has no name, since a binding could not refer to it.
85109
func macAccelerator(keyCode uint16, modifiers uint, character rune, hasCharacter bool) string {
110+
// Without a key there is nothing to bind to, and naming the press after
111+
// its modifiers alone produces something like "cmd" that parseAccelerator
112+
// rejects - which is what every modifier press used to be reported as.
113+
key := macKeyName(keyCode, character, hasCharacter)
114+
if key == "" {
115+
return ""
116+
}
117+
86118
var parts []string
87119
if modifiers&macModifierShift != 0 {
88120
parts = append(parts, "shift")
@@ -96,8 +128,6 @@ func macAccelerator(keyCode uint16, modifiers uint, character rune, hasCharacter
96128
if modifiers&macModifierCommand != 0 {
97129
parts = append(parts, "cmd")
98130
}
99-
if key := macKeyName(keyCode, character, hasCharacter); key != "" {
100-
parts = append(parts, key)
101-
}
131+
parts = append(parts, key)
102132
return strings.Join(parts, "+")
103133
}

v3/pkg/application/accelerator_darwin_test.go

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22

33
package application
44

5-
import "testing"
5+
import (
6+
"strconv"
7+
"testing"
8+
)
69

710
// With Control held, macOS collapses letters to their control codes: Control-A
811
// produces U+0001, which is also what an older reading of this took to mean
@@ -81,14 +84,60 @@ func TestModifiersAreNamedInOrder(t *testing.T) {
8184
}
8285
}
8386

84-
// A key with no name cannot be bound, so it is named as nothing rather than as
85-
// a bare list of modifiers.
87+
// A key with no name cannot be bound to, so the press is named as nothing at
88+
// all. Naming it after the modifiers alone produces something like "cmd",
89+
// which no binding can match and which parseAccelerator rejects - so every
90+
// press of an unnamed key with a modifier held was reported as an error.
8691
func TestAnUnknownKeyHasNoName(t *testing.T) {
87-
if got := macAccelerator(999, macModifierCommand, 0, false); got != "cmd" {
88-
t.Errorf("an unknown key with Command held was named %q, not %q", got, "cmd")
92+
for _, modifiers := range []uint{0, macModifierCommand, macModifierShift | macModifierControl} {
93+
if got := macAccelerator(999, modifiers, 0, false); got != "" {
94+
t.Errorf("an unknown key with modifiers %#x was named %q, not %q", modifiers, got, "")
95+
}
96+
}
97+
}
98+
99+
// The names this produces are handed to parseAccelerator, so anything it
100+
// produces for a real press has to survive that. A bare list of modifiers does
101+
// not.
102+
func TestNothingProducedIsRejectedByTheParser(t *testing.T) {
103+
presses := []struct {
104+
name string
105+
keyCode uint16
106+
modifiers uint
107+
character rune
108+
hasCharacter bool
109+
}{
110+
{"an unknown key held with Command", 999, macModifierCommand, 0, false},
111+
{"a key with no name held with Shift", 56, macModifierShift, 0, false},
112+
{"Control-A", 0, macModifierControl, '\x01', true},
113+
{"F12", 111, 0, 0, false},
114+
}
115+
for _, p := range presses {
116+
got := macAccelerator(p.keyCode, p.modifiers, p.character, p.hasCharacter)
117+
if got == "" {
118+
continue // Nothing is dispatched for an unnamed press.
119+
}
120+
if _, err := parseAccelerator(got); err != nil {
121+
t.Errorf("%s was named %q, which the parser rejects: %s", p.name, got, err)
122+
}
123+
}
124+
}
125+
126+
// namedKeys accepts these, so a binding can be written for them; they have to
127+
// be reachable from a real press or the binding can never fire. AppKit reports
128+
// them only through the characters they produce.
129+
func TestExtendedFunctionKeysAreNamed(t *testing.T) {
130+
// NSF21FunctionKey is 0xF718, and they run consecutively to F35.
131+
for i := 0; i <= 35-21; i++ {
132+
character := rune(0xF718 + i)
133+
want := "f" + strconv.Itoa(21+i)
134+
if got := macAccelerator(0xFFFF, 0, character, true); got != want {
135+
t.Errorf("the key producing %U was named %q, not %q", character, got, want)
136+
}
89137
}
90-
if got := macAccelerator(999, 0, 0, false); got != "" {
91-
t.Errorf("an unknown key was named %q, not %q", got, "")
138+
// NSClearLineFunctionKey. Wails calls this key numlock.
139+
if got := macAccelerator(0xFFFF, 0, 0xF739, true); got != "numlock" {
140+
t.Errorf("the clear key was named %q, not %q", got, "numlock")
92141
}
93142
}
94143

v3/pkg/application/webview_window_darwin.m

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,11 @@ - (WebviewWindow*) initWithContentRect:(NSRect)contentRect styleMask:(NSUInteger
9090
}
9191
- (void)keyDown:(NSEvent *)event {
9292
NSString *keyEventString = acceleratorStringFromKeyEvent(event);
93+
// A press with no name cannot match a binding, and the parser rejects the
94+
// empty string, so sending it on only produces an error for every one.
95+
if (keyEventString.length == 0) {
96+
return;
97+
}
9398
WebviewWindowDelegate *delegate = (WebviewWindowDelegate*)self.delegate;
9499
processWindowKeyDownEvent(delegate.windowId, [keyEventString UTF8String]);
95100
}

0 commit comments

Comments
 (0)