WIP: Keyboard only operation #1074

jkvatne · 2020-06-03T09:48:20Z

Description:

This PR implements keyboard only operation of most but not all widgets. This will improve usability on desktop computers, allowing operation without a mouse.

It passes all tests except the tests where images are compared. The reason is that I had to modify the coloring sheme in order to distinguish between focused and primary colors. The problem is similar to the ongoing changes to implement button animation. What I have called PressedColor should perhaps be named TappedColor.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style.
Any breaking changes have a deprecation path or have been discussed.

andydotxyz · 2020-06-03T10:33:33Z

Just looking at Travis it seems like failures are formatting, test failures and linting - I'd recommend the git hook we use https://github.com/fyne-io/fyne/wiki/GitHooks

andydotxyz · 2020-06-03T10:59:23Z

cmd/fyne_demo/screens/window.go

@@ -187,7 +187,12 @@ func loadWindowGroup() fyne.Widget {
 		}),
 		widget.NewButton("Fixed size window", func() {
 			w := fyne.CurrentApp().NewWindow("Fixed")
-			w.SetContent(fyne.NewContainerWithLayout(layout.NewCenterLayout(), widget.NewLabel("Hello World!")))
+			w.SetContent(fyne.NewContainerWithLayout(


I'm not sure what this is demoing?

It was introduced to test keyboard operation in a second window. There was no extra windows with any focusable widgets, so I had to introduce some buttons to test it. Of cause it failed the first time I tested it, but should be working ok now.

All windows should behave the same though?

andydotxyz · 2020-06-03T11:01:01Z

internal/app/focus.go

@@ -13,7 +13,12 @@ type FocusManager struct {
 func (f *FocusManager) nextInChain(current fyne.Focusable) fyne.Focusable {
 	var first, next fyne.Focusable
 	found := current == nil // if we have no starting point then pretend we matched already
-	driver.WalkVisibleObjectTree(f.canvas.Content(), func(obj fyne.CanvasObject, _ fyne.Position, _ fyne.Position, _ fyne.Size) bool {
+	//OBS driver.WalkVisibleObjectTree(f.cavas.Content(), func(obj fyne.CanvasObject, _ fyne.Position, _ fyne.Position, _ fyne.Size) bool {


OBS? try not to commit commented out code

This may relate to bug #814

I use //OBS to easily do a global search for comments that should be removed before commit. This fell through the cracks.
As for but #814, I belive the added lines of code will be a fix for the bug. I am not able to reproduce it.

I agree. In the future it's good to have a bug fix in separate commit - possibly even PR - than a big feature addition

andydotxyz · 2020-06-03T11:04:57Z

internal/driver/glfw/menu_bar_item.go

 		i.Parent.activateChild(i)
+		for j := 0; j < len(i.Parent.Items); j++ {


I don't understand this loop...
i.Parent.Items[i.Parent.index] is i so the item that has just been activated is being defocussed (or "upped").
Possibly this is because I don't understand what index is

I have changed the name "index" to "currentItem", as it indicates the menu item selected. There is unfortunately a mix of "focused". The focused item receiveing the keyboard data is the MenuBar, while the indicated (focus-colored) item is one of the menu items in the list. Perhaps each menu item should recieve keyboard input?

THat might be cleaner, would probably help with code encapsulation, that said left/right may still make sense at the top level.

andydotxyz · 2020-06-03T11:05:22Z

internal/driver/glfw/menu_bar.go

-	Items []fyne.CanvasObject
-
+	Items      []fyne.CanvasObject
+	index      int


index is quite generic, not clear what this means

index is the currently focused item on the menu bar. Using arrow keys, the focus is moved and index is updated. This is also used on toolbars and tabs.

So maybe activeIndex or currentMenu naming then?

Just changed it to currentItem

andydotxyz · 2020-06-03T11:06:31Z

internal/driver/glfw/menu_bar.go

+}
+
+// HandleKey will process keys pressed when menu is open
+func (b *MenuBar) HandleKey(key fyne.KeyName) {


This should not be an exported API

By way of explanation here (sorry it was lacking before) we don't add a new exported API unless there is a solid use-case for developers.

andydotxyz · 2020-06-03T11:47:41Z

widget/tabcontainer.go

@@ -20,6 +20,8 @@ type TabContainer struct {
 	OnChanged   func(tab *TabItem)
 	current     int
 	tabLocation TabLocation
+	focused     bool
+	renderer    tabContainerRenderer


This is a no-no - a widget must not retain a reference to it's current renderer

This is one of the difficult, deep issues. I could not find any other way to do it.

From the code that uses it it's hard to see the intent, was it to change buttons or highlight that one had been pressed? if it's the latter then maybe tabButton is the place for that code? otherwise the keyboard handling code can change selected item that the render then updates to reflect.

Actualy, on the toolbar, this functionality is not needed and has been removed. But on the Toolbar, there is a similar use of the renderer.
The problem is that the toolbar must update the state of the boolbar buttons, as it receives key-presses. But it has no reference to its buttons. To avoid the renderer reference, I have instead added a buttons array in the toolbar.

I guess this is a little better than holding the renderer, but given that a renderer's lifecycle can be much shorter than a widget it still seems a little risky - you are capturing an internal detail of the renderer could will become invalid if the renderer is destroyed.

Renderers are only guaranteed to remain as long as the widget is visible - if it is hidden for any reason the renderer could be detached.

Oops. Is there a way to force detachement of all renderers, so this can be tested?

Currently we do not have that test capability, but it’s a good suggestion.

andydotxyz · 2020-06-03T11:49:54Z

widget/tabcontainer.go

+}
+
+func (b *TabContainer) KeyUp(key *fyne.KeyEvent) {
+	if b.renderer.tabBar==nil {


Sorry, no - a widget must not manually manipulate the state of it's renderer.
Widgets manage state - renderers draw it.
Surely calling SelectTab or similar helper is the right thing to do?

This has been fixed now

andydotxyz · 2020-06-03T11:50:19Z

widget/tabcontainer.go

@@ -463,15 +531,6 @@ func (b *tabButton) Tapped(e *fyne.PointEvent) {
 	b.OnTap()
 }

-func (b *tabButton) setText(text string) {


Please don't rearrange methods unless they are in the wrong order.

I have tried to change as little as possible on the existing code, but is there a description of what is correct order?
Anyway, I have moved setText() back to where if was.

I don't think we have it written up the wiki yet, the information is in issue #827

andydotxyz · 2020-06-03T11:51:03Z

widget/toolbar.go

-	button := NewButtonWithIcon("", t.Icon, t.OnActivated)
-	button.HideShadow = true
-
+	//button := NewButtonWithIcon("", t.Icon, t.OnActivated)


Please don't commit commented out code - we have version control to track changes.

I know. I just missed it while cleaning up.

andydotxyz · 2020-06-03T11:52:48Z

widget/toolbar.go

@@ -68,14 +68,102 @@ func NewToolbarSeparator() ToolbarItem {
 // Toolbar widget creates a horizontal list of tool buttons
 type Toolbar struct {
 	BaseWidget
-	Items []ToolbarItem
+	Items    []ToolbarItem
+	focused  bool


I don't understand why a toolbarButton should not be focused but the toolbar can be?
This code generally seems strange - why would a bar track which of it's non-focusable children is focused?

In the various apps I have tried it seems that toolbars are not focusable.
If that's not right then we can come back to it.

You are right, but it makes for a lot of Tab presses to navigate through the tab bar. If a toolbar has 10 toolbar buttons, you must press Tab 10 times to go to the first entry in the form. I like it much better with the chosen approach, where the Tab continues to the next object and arrow keys are used to select the actual toolbar button. It is very similar to the tabbed container.

Looking at more applications I see that toolbars are not focusable at all.
It's like they only focus "input controls" and not every element of the interface.
I don't know if that is right either - but if we are to focus toolbar not items then it should probably appeare focused rather than an item in it?

Usualy the toolbar buttons are associated with keyboard shortcuts, and then there is no need to focus them. To do this we need an obligatory shortcut for each toolbar button. I leave this for future updates.

That is a good idea, apart from the mandatory part. Some work for Shortcuts is in place, hopwfully we can make use of that.

IMHO, a toolbar (or containing buttons) never should be focusable. Toolbars are for easy clicking with the mouse. All commands available in a toolbar should also be available as a menu item in the menu bar - hence keyboard-only users already use the menu or its accelerator.

I agree. But it depends on having keyboard shortcuts. And the menu must be focusable.

It would be great to get this split into smaller pieces so we can see some of this functionality land - the conflict list is probably too long to get this PR landed in one go.

andydotxyz · 2020-06-07T11:56:57Z

Looks like there is still a format issue in there, plus lots of test failures.

Some 70 png files had to be changed to implement the new coloring sheme with differnce between focused and primary colors. Modified theme/theme_test.go to check focus color Modified widget/select_entry.go to focus first entry Modified internal/driver/glfw/window.go to not focus buttons on mouse click. Modified: internal/driver/glfw/menu_bar_test.go to unfocus button on start of each test.

andydotxyz · 2020-06-10T08:39:52Z

Hi @jkvatne I think this would be much easier to review and discuss in detail if you broke it into smaller PRs, this currently contains a lot of different areas of change and in our experience such changes can take exponentially longer than items that focus on one area at a time. There may be a dependency ordering consideration as well.
What do you think?

jkvatne · 2020-06-10T09:09:01Z

Sure, you are correct. I will try to split it more. I was a bit optimistic, but now all tests passes so it is not so bad.
There is one thing that should be descided soon, and that is the coloring sheme to differentiate between primary color and focused color. Also a "tapped" or "pressed" color should be introduced. This relates very much to WIP - Full material design #970, so I will try to look into what steveoc64 has done.

andydotxyz · 2020-06-10T21:27:10Z

Please see my proposed new theme API for 2.0. I think this should facilitate the improvements that you reckon we need :)

Theme-API-2

andydotxyz · 2020-06-10T21:30:16Z

Also issue #885 is probably quite relevant

andydotxyz · 2020-09-04T10:19:05Z

This PR has a growing list of conflicts as the codebase moves on - what should we do with it @jkvatne ?
It might be easiest if smaller pieces were added - perhaps addressing one widget at a time?

toaster · 2020-09-07T05:21:28Z

It might be easiest if smaller pieces were added - perhaps addressing one widget at a time?

@andydotxyz I think that would definitely the way to go.
Any bugfix/refactoring for keyboard handling in overlays should be separated, too.
As I am beginning to work on #814, I think it would even be a good idea to have such fixes extracted first :).

toaster · 2020-09-07T05:25:22Z

internal/driver/glfw/window.go

@@ -15,6 +15,7 @@ import (
 	"fyne.io/fyne/internal/cache"
 	"fyne.io/fyne/internal/driver"
 	"fyne.io/fyne/internal/painter/gl"
+	"fyne.io/fyne/widget"


We must not add dependencies from widget here.

jkvatne · 2020-09-07T07:03:05Z

I am sorry, but I do not have much time now for this prosject. I am an independant consultant, and have suddenly a lot of paying customers that demands my time. Perhaps a bit later.
But one of the big problems I find, is that the coloring scheme is insufficient. The primary color is used both for focused items and for primary buttons. You need to differentiate between primary/secondary and focused/hovered/nonfocused. Google uses overlays with different shades to indicate hovered/focused/pressed/seelcted, so this might be a way of doing it.

But buttons might be the simplest items. All other widgets that have manual interaction must also be focus-able, and indicate state. Menus also.

So I would like you to first find a way to calculate colors/shades that is widget independant, and with consistent coloring for different widgets. This is beyond my ability. I tried, but you didn't like it.

andydotxyz · 2020-09-07T22:07:31Z

Yes making such an addition will break the theme API so we need to hold it back for 2.0 - but then we can get alt-background and highlighted colours etc

andydotxyz · 2020-10-17T10:50:13Z

Radio (-> RadioGroup) and Select widgets have been taken care of by other work.

andydotxyz · 2020-11-04T10:42:50Z

There is a lot of good work here and we should take it all into account as we move forward.
However there is a lot of conflict against base now and many widgets have had keyboard operation added separately.

I'm going to close this, if you find that time becomes available then let's look at how to fill in the blanks of the updated keyboard support.
Mostly I think we could do with feed-in on the theme updates. They are proposed on the wiki at Theme2 and should cover things like the shade code added here, but built into the API for 2.0.

jkvatne added 2 commits June 3, 2020 11:04

Keyboard navigation

ed04aa9

Merge branch 'develop' into feature/keyboardoperation

0b06b60

jkvatne marked this pull request as draft June 3, 2020 10:29

andydotxyz requested changes Jun 3, 2020

View reviewed changes

jkvatne added 3 commits June 4, 2020 15:22

Updated to reflect comments from Andy

5bb4487

Fixed lint and fmt errors. Still fails test

07c8cbf

Fixed lint and fmt errors. Still fails test

c32a82c

jkvatne added 6 commits June 8, 2020 20:38

Fix toolbar regeneration and focus update of buttons

c416a0d

Fixed stray spaces in select_entry

c8052c9

Fixed stray spaces in select_entry

b61755f

Make first menu entry focuse when Alt key is used to open menu

13ec8ff

Merge branch 'develop' into feature/keyboardoperation

0a75eb7

jkvatne mentioned this pull request Jul 15, 2020

Support Focus Traversal in Dialogs #948

Closed

toaster reviewed Sep 7, 2020

View reviewed changes

andydotxyz mentioned this pull request Oct 8, 2020

Fyne-demo bugs (Windows 10) #1372

Closed

andydotxyz closed this Nov 4, 2020

		i.Parent.activateChild(i)
		for j := 0; j < len(i.Parent.Items); j++ {

WIP: Keyboard only operation #1074

WIP: Keyboard only operation #1074

Conversation

jkvatne commented Jun 3, 2020 • edited

Description:

Checklist:

Where applicable:

andydotxyz commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Jun 7, 2020

andydotxyz commented Jun 10, 2020

jkvatne commented Jun 10, 2020

andydotxyz commented Jun 10, 2020

andydotxyz commented Jun 10, 2020

andydotxyz commented Sep 4, 2020

toaster commented Sep 7, 2020

Choose a reason for hiding this comment

jkvatne commented Sep 7, 2020

andydotxyz commented Sep 7, 2020

andydotxyz commented Oct 17, 2020

andydotxyz commented Nov 4, 2020

jkvatne commented Jun 3, 2020 •

edited