Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app,io,widget: [android/macos] fix focus issues #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

app,io,widget: [android/macos] fix focus issues #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion app/os_macos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,11 @@ static void invalidateCharacterCoordinates(CFTypeRef viewRef) {
}
}
}

static void setFocus(CFTypeRef viewRef) {
NSView *view = (__bridge NSView *)viewRef;
[view.window makeFirstResponder:view];
}
*/
import "C"

Expand Down Expand Up @@ -514,7 +519,11 @@ func (w *window) EditorStateChanged(old, new editorState) {
}
}

func (w *window) ShowTextInput(show bool) {}
func (w *window) ShowTextInput(show bool) {
if show && !w.config.Focused {
C.setFocus(w.view)
}
}
Comment on lines +522 to +526
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is macOS, which doesn't have SoftwareKeyboard. However, I didn't find any specific function to request focus, and adding a new function will require changes on multiple OSes, even as no-op.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you need this here and not on other desktop platforms. Why can't C.setFocus be called automatically in the cases where focus was lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to call it when the GioView is clicked, and doesn't have focus. I don't think Gio should immediate request for Focus, because it's possible to have multiple non-Gio View in the same Window.


func (w *window) SetInputHint(_ key.InputHint) {}

Expand Down
20 changes: 16 additions & 4 deletions app/os_macos.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,17 @@ - (void)windowDidChangeScreen:(NSNotification *)notification {
}
- (void)windowDidBecomeKey:(NSNotification *)notification {
NSWindow *window = (NSWindow *)[notification object];
GioView *view = (GioView *)window.contentView;
gio_onFocus(view.handle, 1);
GioView *view = (GioView *)window.contentView;
if ([window firstResponder] == view) {
gio_onFocus(view.handle, 1);
}
}
- (void)windowDidResignKey:(NSNotification *)notification {
NSWindow *window = (NSWindow *)[notification object];
GioView *view = (GioView *)window.contentView;
gio_onFocus(view.handle, 0);
GioView *view = (GioView *)window.contentView;
if ([window firstResponder] == view) {
gio_onFocus(view.handle, 0);
}
Comment on lines 48 to +60
Copy link
Contributor Author

@inkeliz inkeliz Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason to change: This is relative to the window, not the view. If you have multiple views in the same window, it used to incorrectly report that Gio had the focus.

}
@end

Expand Down Expand Up @@ -205,6 +209,14 @@ - (void)applicationDidHide:(NSNotification *)notification {
- (void)dealloc {
gio_onDestroy(self.handle);
}
- (BOOL) becomeFirstResponder {
gio_onFocus(self.handle, 1);
return [super becomeFirstResponder];
}
- (BOOL) resignFirstResponder {
gio_onFocus(self.handle, 0);
return [super resignFirstResponder];
}
@end

// Delegates are weakly referenced from their peers. Nothing
Expand Down
8 changes: 8 additions & 0 deletions io/input/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,14 @@ func (q *Router) changeState(e event.Event, state inputState, evts []taggedEvent
e.event = de
}
}
for i := range evts {
e := &evts[i]
if fe, ok := e.event.(key.FocusEvent); ok {
if !fe.Focus {
state.keyState.focus = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me this should be done in Router.processEvent, near the lines

	case key.EditEvent, key.FocusEvent, key.SelectionEvent:
		var evts []taggedEvent
		if f := state.focus; f != nil {
			evts = append(evts, taggedEvent{tag: f, event: e})
		}
		q.changeState(e, state, evts)

But I'm not perfectly sure what use-case you're fixing. Can you add a test to io/input/key_test.go for my understanding and to guard against regressions?

Copy link
Contributor Author

@inkeliz inkeliz Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm fixing is reseting the focus when receiving key.FocusEvent from "the OS".

Consider that you have one FocusCmd{Tag: TextArea}. If Gio gets one FocusEvent from "the OS" (say: macos), it will not remove the focus from TextArea. That is the issue here.

So, this change changes the state (state.keyState.focus) to nil when the FocusEvent is false. Since it's messing with state I use the changeState function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changeState doesn't change state per se, it adds it to the list of states. A better name would be addStateChange.

See https://github.com/gioui/gio/pull/138/files/101cf1aa478601e9c823f86a86b652b09d768a56#diff-c04c544d7677b4f40b53717995fb8f2e3d67212768e651e229ef2c020fc0d357L434 where state is changed by processEvent, in response to a pointer.Event.

}
}
}
// Initialize the first change to contain the current state
// and events that are bound for the current frame.
if len(q.changes) == 0 {
Expand Down
5 changes: 4 additions & 1 deletion widget/editor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ func (e *Editor) processPointerEvent(gtx layout.Context, ev event.Event) (Editor
Y: int(math.Round(float64(evt.Position.Y))),
})
gtx.Execute(key.FocusCmd{Tag: e})
if !e.ReadOnly {
gtx.Execute(key.SoftKeyboardCmd{Show: true})
}
Comment on lines +292 to +294
Copy link
Contributor Author

@inkeliz inkeliz Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't make sense to request SoftKeyboard when it's ReadOnly. Maybe I need to do it on a separated commit?

if e.scroller.State() != gesture.StateFlinging {
e.scrollCaret = true
}
Expand Down Expand Up @@ -395,7 +398,7 @@ func (e *Editor) processKey(gtx layout.Context) (EditorEvent, bool) {
case key.FocusEvent:
// Reset IME state.
e.ime.imeState = imeState{}
if ke.Focus {
if ke.Focus && !e.ReadOnly {
gtx.Execute(key.SoftKeyboardCmd{Show: true})
}
case key.Event:
Expand Down