Merge pull request #1454 from toaster/bugfix/814

Bugfix: #814

internal/driver/glfw/canvas.go

@@ -22,12 +22,13 @@ var _ fyne.Canvas = (*glCanvas)(nil)
 type glCanvas struct {
 	sync.RWMutex
 
-	content  fyne.CanvasObject
-	menu     fyne.CanvasObject
-	overlays *overlayStack
-	padded   bool
-	size     fyne.Size
-	focusMgr *app.FocusManager
+	content         fyne.CanvasObject
+	contentFocusMgr *app.FocusManager
+	menu            fyne.CanvasObject
+	menuFocusMgr    *app.FocusManager
+	overlays        *overlayStack
+	padded          bool
+	size            fyne.Size
 
 	onTypedRune func(rune)
 	onTypedKey  func(*fyne.KeyEvent)
@@ -253,6 +254,8 @@ func (c *glCanvas) Unfocus() {
 }
 
 func (c *glCanvas) buildMenu(w *window, m *fyne.MainMenu) {
+	c.Lock()
+	defer c.Unlock()
 	c.setMenuOverlay(nil)
 	if m == nil {
 		return
@@ -264,6 +267,10 @@ func (c *glCanvas) buildMenu(w *window, m *fyne.MainMenu) {
 	}
 }
 
+func (c *glCanvas) RemoveShortcut(shortcut fyne.Shortcut) {
+	c.shortcut.RemoveShortcut(shortcut)
+}
+
 // canvasSize computes the needed canvas size for the given content size
 func (c *glCanvas) canvasSize(contentSize fyne.Size) fyne.Size {
 	canvasSize := contentSize.Add(fyne.NewSize(0, c.menuHeight()))
@@ -274,10 +281,6 @@ func (c *glCanvas) canvasSize(contentSize fyne.Size) fyne.Size {
 	return canvasSize
 }
 
-func (c *glCanvas) RemoveShortcut(shortcut fyne.Shortcut) {
-	c.shortcut.RemoveShortcut(shortcut)
-}
-
 func (c *glCanvas) contentPos() fyne.Position {
 	contentPos := fyne.NewPos(0, c.menuHeight())
 	if c.Padded() {
@@ -358,7 +361,10 @@ func (c *glCanvas) focusManager() *app.FocusManager {
 	if focusMgr := c.overlays.TopFocusManager(); focusMgr != nil {
 		return focusMgr
 	}
-	return c.focusMgr
+	if c.isMenuActive() {
+		return c.menuFocusMgr
+	}
+	return c.contentFocusMgr
 }
 
 func (c *glCanvas) isDirty() bool {
@@ -368,20 +374,20 @@ func (c *glCanvas) isDirty() bool {
 	return c.dirty
 }
 
+func (c *glCanvas) isMenuActive() bool {
+	return c.menu != nil && c.menu.(*MenuBar).IsActive()
+}
+
 func (c *glCanvas) menuHeight() int {
-	switch c.menuOverlay() {
+	switch c.menu {
 	case nil:
 		// no menu or native menu -> does not consume space on the canvas
 		return 0
 	default:
-		return c.menuOverlay().MinSize().Height
+		return c.menu.MinSize().Height
 	}
 }
 
-func (c *glCanvas) menuOverlay() fyne.CanvasObject {
-	return c.menu
-}
-
 func (c *glCanvas) objectTrees() []fyne.CanvasObject {
 	trees := make([]fyne.CanvasObject, 0, len(c.Overlays().List())+2)
 	trees = append(trees, c.content)
@@ -428,7 +434,7 @@ func (c *glCanvas) paint(size fyne.Size) {
 func (c *glCanvas) setContent(content fyne.CanvasObject) {
 	c.content = content
 	c.contentTree = &renderCacheTree{root: &renderCacheNode{obj: c.content}}
-	c.focusMgr = app.NewFocusManager(c.content)
+	c.contentFocusMgr = app.NewFocusManager(c.content)
 }
 
 func (c *glCanvas) setDirty(dirty bool) {
@@ -438,11 +444,10 @@ func (c *glCanvas) setDirty(dirty bool) {
 	c.dirty = dirty
 }
 
-func (c *glCanvas) setMenuOverlay(b fyne.CanvasObject) {
-	c.Lock()
+func (c *glCanvas) setMenuOverlay(b *MenuBar) {
 	c.menu = b
 	c.menuTree = &renderCacheTree{root: &renderCacheNode{obj: c.menu}}
-	c.Unlock()
+	c.menuFocusMgr = app.NewFocusManager(c.menu)
 }
 
 func (c *glCanvas) setupThemeListener() {

internal/driver/glfw/canvas_other_test.go

@@ -0,0 +1,62 @@
+// +build !ci
+// +build !mobile
+// +build !darwin no_native_menus
+
+package glfw
+
+import (
+	"testing"
+
+	"fyne.io/fyne"
+	"fyne.io/fyne/container"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGlCanvas_FocusHandlingWhenActivatingOrDeactivatingTheMenu(t *testing.T) {
+	w := createWindow("Test")
+	w.SetPadded(false)
+	w.SetMainMenu(
+		fyne.NewMainMenu(
+			fyne.NewMenu("test", fyne.NewMenuItem("item", func() {})),
+		),
+	)
+	c := w.Canvas().(*glCanvas)
+
+	ce1 := &focusable{id: "ce1"}
+	ce2 := &focusable{id: "ce2"}
+	content := container.NewVBox(ce1, ce2)
+	w.SetContent(content)
+
+	assert.Nil(t, c.Focused())
+	m := c.menu.(*MenuBar)
+	assert.False(t, m.IsActive())
+
+	c.FocusPrevious()
+	assert.Equal(t, ce2, c.Focused())
+	assert.True(t, ce2.focused)
+
+	m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
+	assert.True(t, m.IsActive())
+	ctxt := "activating the menu changes focus handler but does not remove focus from content"
+	assert.True(t, ce2.focused, ctxt)
+	assert.Nil(t, c.Focused(), ctxt)
+
+	c.FocusNext()
+	// TODO: changes menu focus as soon as menu has focus support
+	ctxt = "changing focus with active menu does not affect content focus"
+	assert.True(t, ce2.focused, ctxt)
+	assert.Nil(t, c.Focused(), ctxt)
+
+	m.Items[0].(*menuBarItem).Tapped(&fyne.PointEvent{})
+	assert.False(t, m.IsActive())
+	// TODO: does not remove focus from focused menu item as soon as menu has focus support
+	ctxt = "deactivating the menu restores focus handler from content"
+	assert.True(t, ce2.focused, ctxt)
+	assert.Equal(t, ce2, c.Focused())
+
+	c.FocusPrevious()
+	assert.Equal(t, ce1, c.Focused(), ctxt)
+	assert.True(t, ce1.focused, ctxt)
+	assert.False(t, ce2.focused, ctxt)
+}

internal/driver/glfw/driver_test.go

@@ -56,7 +56,7 @@ func Test_gLDriver_AbsolutePositionForObject(t *testing.T) {
 	// 0 is the shadow
 	// 1 is the menu bar’s background
 	// 2 is the container holding the items
-	mbarCont := cache.Renderer(movl.(fyne.Widget)).Objects()[2].(*fyne.Container)
+	mbarCont := cache.Renderer(movl).Objects()[2].(*fyne.Container)
 	m2 := mbarCont.Objects[1]
 
 	tests := map[string]struct {

internal/driver/glfw/menu.go

@@ -4,7 +4,7 @@ import (
 	"fyne.io/fyne"
 )
 
-func buildMenuOverlay(menus *fyne.MainMenu, c fyne.Canvas) fyne.CanvasObject {
+func buildMenuOverlay(menus *fyne.MainMenu, c fyne.Canvas) *MenuBar {
 	if len(menus.Items) == 0 {
 		fyne.LogError("Main menu must have at least one child menu", nil)
 		return nil

internal/driver/glfw/menu_bar.go

@@ -51,6 +51,12 @@ func (b *MenuBar) CreateRenderer() fyne.WidgetRenderer {
 	}
 }
 
+// IsActive returns whether the menu bar is active or not.
+// An active menu bar shows the current selected menu and should have the focus.
+func (b *MenuBar) IsActive() bool {
+	return b.active
+}
+
 // Hide hides the menu bar.
 //
 // Implements: fyne.Widget

internal/driver/glfw/window.go

@@ -643,7 +643,7 @@ func (w *window) mouseOut() {
 }
 
 func (w *window) mouseClicked(_ *glfw.Window, btn glfw.MouseButton, action glfw.Action, mods glfw.ModifierKey) {
-	co, pos, layer := w.findObjectAtPositionMatching(w.canvas, w.mousePos, func(object fyne.CanvasObject) bool {
+	co, pos, _ := w.findObjectAtPositionMatching(w.canvas, w.mousePos, func(object fyne.CanvasObject) bool {
 		switch object.(type) {
 		case fyne.Tappable, fyne.SecondaryTappable, fyne.DoubleTappable, fyne.Focusable, fyne.Draggable, desktop.Mouseable, desktop.Hoverable:
 			return true
@@ -675,12 +675,10 @@ func (w *window) mouseClicked(_ *glfw.Window, btn glfw.MouseButton, action glfw.
 		}
 	}
 
-	if layer != 1 { // 0 - overlay, 1 - menu, 2 - content
-		if wid, ok := co.(fyne.Focusable); ok {
-			w.canvas.Focus(wid)
-		} else {
-			w.canvas.Unfocus()
-		}
+	if wid, ok := co.(fyne.Focusable); ok {
+		w.canvas.Focus(wid)
+	} else {
+		w.canvas.Unfocus()
 	}
 
 	if action == glfw.Press {