Make menubar select first item, like context menus

This also tidies up the logic for changing the selected element from
context menus to the menubar, fixing a bug where the menubar forgets
which element was selected before it, and not re-introducing the bug
which the complex logic fixed in the first place (which was the menubar
seeing the context menu as the previously selected element, when the
menu will be destroyed by the time the menubar restores its selection).

2 files changed, 15 insertions, 18 deletions

diff --git a/todo.txt b/todo.txt
index 8a86e5f..e95a8e7 100644
--- a/todo.txt
+++ b/todo.txt
@@ -420,3 +420,4 @@ TODO: Investigate menubar UX - now that it uses KeyboardSelector, it's very
       comparable in interaction to ContextMenus. To match context menus,
       should we make its selection index reset to zero (i.e. the 'mtui' option)
       whenever it's selected (via the keyboard)?
+      (Done! Decided yes.)
diff --git a/ui.js b/ui.js
index 7037e87..674541d 100644
--- a/ui.js
+++ b/ui.js
@@ -1144,6 +1144,14 @@ class AppElement extends FocusElement {
       if (this.menubar.isSelected) {
         this.menubar.restoreSelection()
       } else {
+        // If we've got a menu open, close it, restoring selection to the
+        // element selected before the menu was opened, so the menubar will
+        // see that as the previously selected element (instead of the context
+        // menu - which will be closed irregardless and gone when the menubar
+        // tries to restore the selection).
+        if (this.menuLayer.children[0]) {
+          this.menuLayer.children[0].close()
+        }
         this.menubar.select()
       }
     } else if (keyBuf.equals(Buffer.from([5]))) { // Ctrl-E
@@ -3499,17 +3507,7 @@ class ContextMenu extends FocusElement {
     if (this.visible) {
       const newlySelected = this.root.selectedElement
       this.close()
-
-      // Bad hack: if the menubar is the selected thing, directly select it
-      // (hence setting its previously selected element to whatever we just
-      // restored the selection to as the context menu).
-      const menubar = newlySelected.directAncestors.find(
-        el => el instanceof Menubar)
-      if (menubar) {
-        menubar.select()
-      } else {
-        this.selectedBefore.root.select(newlySelected)
-      }
+      this.selectedBefore.root.select(newlySelected)
     }
   }
 
@@ -3694,18 +3692,16 @@ class Menubar extends ListScrollForm {
   }
 
   select() {
-    // The context menu disappears when it's deselected, so we really want to
-    // use whatever element was selected before the menu was opened.
+    // When the menubar is selected from the menubar's context menu, the UI
+    // looks like it's "popping" a state, so don't reset the selected index to
+    // the start - something we only do when we "newly" select the menubar.
     if (this.contextMenu && this.contextMenu.isSelected) {
-      // ...Unless it was the menubar that was already selected.
-      if (!this.contextMenu.selectedBefore.directAncestors.includes(this)) {
-        this.selectedBefore = this.contextMenu.selectedBefore
-      }
+      this.root.select(this)
     } else {
       this.selectedBefore = this.root.selectedElement
+      this.firstInput()
     }
 
-    this.root.select(this)
     this.keyboardSelector.reset()
   }