Fix menu behaviour (#253)

crates/kas-core/src/core/data.rs

@@ -156,12 +156,6 @@ pub trait LayoutData {
 /// A pop-up widget's rect is not contained by its parent, therefore the parent
 /// must not call any [`Layout`] methods on the pop-up (whether or not it is
 /// visible). The window is responsible for calling these methods.
-///
-/// Other methods on the pop-up, including event handlers, should be called
-/// normally, with one exception: after calling an event handler on the pop-up,
-/// the parent should invoke [`Manager::pop_action`] and handle the action
-/// itself, where possible (using [`Manager::close_window`] to close it).
-/// Remaining actions should be added back to the [`Manager`].
 //
 // NOTE: it's tempting to include a pointer to the widget here. There are two
 // options: (a) an unsafe aliased pointer or (b) Rc<RefCell<dyn WidgetConfig>>.

crates/kas-core/src/dir.rs

@@ -5,6 +5,8 @@
 
 //! Direction types
 
+use std::fmt;
+
 /// Trait over directional types
 ///
 /// This trait has a variable implementation, [`Direction`], and several fixed
@@ -92,6 +94,21 @@ pub enum Direction {
     Up = 3,
 }
 
+impl fmt::Display for Direction {
+    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        write!(
+            f,
+            "{}",
+            match self {
+                Direction::Right => "Right",
+                Direction::Down => "Down",
+                Direction::Left => "Left",
+                Direction::Up => "Up",
+            }
+        )
+    }
+}
+
 impl Directional for Direction {
     type Flipped = Self;
     type Reversed = Self;

crates/kas-core/src/event/events.rs

@@ -13,7 +13,7 @@ use super::{GrabMode, Manager, Response, SendEvent}; // for doc-links
 use super::{MouseButton, UpdateHandle, VirtualKeyCode};
 
 use crate::geom::{Coord, DVec2, Offset};
-use crate::{WidgetId, WindowId};
+use crate::{dir::Direction, WidgetId, WindowId};
 
 /// Events addressed to a widget
 #[non_exhaustive]
@@ -153,12 +153,6 @@ pub enum Event {
     /// A user-defined payload is passed. Interpretation of this payload is
     /// user-defined and unfortunately not type safe.
     HandleUpdate { handle: UpdateHandle, payload: u64 },
-    /// Notification that a new popup has been created
-    ///
-    /// This is sent to the parent of each open popup when a new popup is
-    /// created. This enables parents to close their popups when the new popup
-    /// is not a descendant of itself. The `WidgetId` is that of the popup.
-    NewPopup(WidgetId),
     /// Notification that a popup has been destroyed
     ///
     /// This is sent to the popup's parent after a popup has been removed.
@@ -388,6 +382,17 @@ impl Command {
         use Command::*;
         matches!(self, Escape | Cut | Copy | Deselect)
     }
+
+    /// Convert arrow keys to a direction
+    pub fn as_direction(self) -> Option<Direction> {
+        match self {
+            Command::Left => Some(Direction::Left),
+            Command::Right => Some(Direction::Right),
+            Command::Up => Some(Direction::Up),
+            Command::Down => Some(Direction::Down),
+            _ => None,
+        }
+    }
 }
 
 /// Source of `EventChild::Press`

crates/kas-core/src/event/manager.rs

@@ -307,7 +307,7 @@ impl<'a> Manager<'a> {
             return;
         } else if vkey == VK::Escape {
             if let Some(id) = self.state.popups.last().map(|(id, _, _)| *id) {
-                self.close_window(id);
+                self.close_window(id, true);
                 return;
             }
         } else if !self.state.char_focus {
@@ -390,7 +390,7 @@ impl<'a> Manager<'a> {
             let last = self.state.popups.len() - 1;
             for i in 0..n {
                 let id = self.state.popups[last - i].0;
-                self.close_window(id);
+                self.close_window(id, false);
             }
         }
 
@@ -516,7 +516,7 @@ impl<'a> Manager<'a> {
                 Response::Unhandled => (),
                 _ => return,
             }
-            self.close_window(wid);
+            self.close_window(wid, false);
         }
         self.send_event(widget, id, event);
     }

crates/kas-core/src/event/manager/mgr_pub.rs

@@ -216,17 +216,6 @@ impl<'a> Manager<'a> {
         self.action |= action;
     }
 
-    /// Get the current [`TkAction`], replacing with `None`
-    ///
-    /// The caller is responsible for ensuring the action is handled correctly;
-    /// generally this means matching only actions which can be handled locally
-    /// and downgrading the action, adding the result back to the [`Manager`].
-    pub fn pop_action(&mut self) -> TkAction {
-        let action = self.action;
-        self.action = TkAction::empty();
-        action
-    }
-
     /// Add an overlay (pop-up)
     ///
     /// A pop-up is a box used for things like tool-tips and menus which is
@@ -266,8 +255,15 @@ impl<'a> Manager<'a> {
     }
 
     /// Close a window or pop-up
+    ///
+    /// In the case of a pop-up, all pop-ups created after this will also be
+    /// removed (on the assumption they are a descendant of the first popup).
+    ///
+    /// If `restore_focus` then navigation focus will return to whichever widget
+    /// had focus before the popup was open. (Usually this is true excepting
+    /// where focus has already been changed.)
     #[inline]
-    pub fn close_window(&mut self, id: WindowId) {
+    pub fn close_window(&mut self, id: WindowId, restore_focus: bool) {
         if let Some(index) =
             self.state.popups.iter().enumerate().find_map(
                 |(i, p)| {
@@ -279,18 +275,24 @@ impl<'a> Manager<'a> {
                 },
             )
         {
-            let (_, popup, old_nav_focus) = self.state.popups.remove(index);
-            self.state.popup_removed.push((popup.parent, id));
+            let mut old_nav_focus = None;
+            while self.state.popups.len() > index {
+                let (wid, popup, onf) = self.state.popups.pop().unwrap();
+                self.state.popup_removed.push((popup.parent, wid));
+                self.shell.close_window(wid);
+                old_nav_focus = onf;
+            }
 
+            if !restore_focus {
+                old_nav_focus = None
+            }
             if let Some(id) = old_nav_focus {
                 self.set_nav_focus(id, true);
             }
+            // TODO: if popup.id is an ancestor of self.nav_focus then clear
+            // focus if not setting (currently we cannot test this)
         }
 
-        // For popups, we need to update mouse/keyboard focus.
-        // (For windows, focus gained/lost events do this job.)
-        self.state.send_action(TkAction::REGION_MOVED);
-
         self.shell.close_window(id);
     }
 
@@ -625,6 +627,7 @@ impl<'a> Manager<'a> {
             self.redraw(id);
         }
         self.state.nav_focus = None;
+        self.clear_char_focus();
         trace!("Manager: nav_focus = None");
     }
 

crates/kas-core/src/event/manager/mgr_shell.rs

@@ -240,22 +240,6 @@ impl ManagerState {
         trace!("Manager::region_moved");
         // Note: redraw is already implied.
 
-        self.nav_focus = self
-            .nav_focus
-            .and_then(|id| widget.find_leaf(id).map(|w| w.id()));
-        if let Some(id) = self.sel_focus {
-            if let Some(new_id) = widget.find_leaf(id).map(|w| w.id()) {
-                self.sel_focus = Some(new_id);
-            } else {
-                if self.char_focus {
-                    self.pending.push(Pending::LostCharFocus(id));
-                    self.char_focus = false;
-                }
-                self.pending.push(Pending::LostSelFocus(id));
-                self.sel_focus = None;
-            }
-        }
-
         // Update hovered widget
         let hover = widget.find_id(self.last_mouse_coord);
         self.with(shell, |mgr| mgr.set_hover(widget, hover));
@@ -313,14 +297,17 @@ impl ManagerState {
             mgr.send_event(widget, parent, Event::PopupRemoved(wid));
         }
         while let Some(id) = mgr.state.new_popups.pop() {
-            for parent in mgr
-                .state
-                .popups
-                .iter()
-                .map(|(_, popup, _)| popup.parent)
-                .collect::<SmallVec<[WidgetId; 16]>>()
-            {
-                mgr.send_event(widget, parent, Event::NewPopup(id));
+            while let Some((_, popup, _)) = mgr.state.popups.last() {
+                if widget
+                    .find_leaf(popup.parent)
+                    .map(|w| w.is_ancestor_of(id))
+                    .unwrap_or(false)
+                {
+                    break;
+                }
+                let (wid, popup, _old_nav_focus) = mgr.state.popups.pop().unwrap();
+                mgr.send_event(widget, popup.parent, Event::PopupRemoved(wid));
+                // Don't restore old nav focus: assume new focus will be set by new popup
             }
         }
 
@@ -457,6 +444,12 @@ impl<'a> Manager<'a> {
                     }
                 }
             }
+            Focused(false) => {
+                // Window focus lost: close all popups
+                while let Some(id) = self.state.popups.last().map(|(id, _, _)| *id) {
+                    self.close_window(id, true);
+                }
+            }
             KeyboardInput {
                 input,
                 is_synthetic,

crates/kas-core/src/text/selection.rs

@@ -34,6 +34,11 @@ impl SelectionHelper {
         }
     }
 
+    /// Reset to the default state
+    pub fn clear(&mut self) {
+        *self = Self::default();
+    }
+
     /// True if the edit pos equals the selection pos
     pub fn is_empty(&self) -> bool {
         self.edit_pos == self.sel_pos

crates/kas-core/src/toolkit.rs

@@ -80,7 +80,7 @@ bitflags! {
         ///
         /// [`WidgetId`]: crate::WidgetId
         const RECONFIGURE = 1 << 16;
-        /// The current window or pop-up should be closed
+        /// The current window should be closed
         const CLOSE = 1 << 30;
         /// Close all windows and exit
         const EXIT = 1 << 31;

crates/kas-widgets/src/combobox.rs

@@ -273,7 +273,7 @@ impl<M: 'static> ComboBox<M> {
             Response::Focus(x) => Response::Focus(x),
             Response::Update | Response::Select => {
                 if let Some(id) = self.popup_id {
-                    mgr.close_window(id);
+                    mgr.close_window(id, true);
                 }
                 if let Some(index) = self.popup.inner.find_child_index(id) {
                     if index != self.active {
@@ -290,7 +290,7 @@ impl<M: 'static> ComboBox<M> {
             Response::Msg((index, ())) => {
                 *mgr |= self.set_active(index);
                 if let Some(id) = self.popup_id {
-                    mgr.close_window(id);
+                    mgr.close_window(id, true);
                 }
                 if let Some(ref f) = self.on_select {
                     Response::update_or_msg((f)(mgr, index))
@@ -299,9 +299,6 @@ impl<M: 'static> ComboBox<M> {
                 }
             }
         }
-        // NOTE: as part of the Popup API we are expected to trap
-        // TkAction::CLOSE here, but we know our widget doesn't generate
-        // this action.
     }
 }
 
@@ -322,7 +319,7 @@ impl<M: 'static> event::Handler for ComboBox<M> {
         match event {
             Event::Activate => {
                 if let Some(id) = self.popup_id {
-                    mgr.close_window(id);
+                    mgr.close_window(id, true);
                 } else {
                     open_popup(self, mgr, true);
                 }
@@ -340,7 +337,7 @@ impl<M: 'static> event::Handler for ComboBox<M> {
                     }
                 } else {
                     if let Some(id) = self.popup_id {
-                        mgr.close_window(id);
+                        mgr.close_window(id, false);
                     }
                     return Response::Unhandled;
                 }
@@ -376,15 +373,7 @@ impl<M: 'static> event::Handler for ComboBox<M> {
                     }
                 }
                 if let Some(id) = self.popup_id {
-                    mgr.close_window(id);
-                }
-            }
-            Event::NewPopup(id) => {
-                // For a ComboBox, for any new Popup we should close self
-                if id != self.popup.id() {
-                    if let Some(id) = self.popup_id {
-                        mgr.close_window(id);
-                    }
+                    mgr.close_window(id, true);
                 }
             }
             Event::PopupRemoved(id) => {

crates/kas-widgets/src/editbox.rs

@@ -1044,6 +1044,7 @@ impl<G: EditGuard> HasString for EditField<G> {
         }
 
         self.text.set_string(string);
+        self.selection.clear();
         if kas::text::fonts::fonts().num_faces() > 0 {
             if let Some(req) = self.text.prepare() {
                 self.required = req.into();

crates/kas-widgets/src/menu.rs

@@ -33,7 +33,13 @@ pub trait Menu: Widget {
     /// are menus, these should open.
     ///
     /// `target == None` implies that all menus should close.
-    fn set_menu_path(&mut self, _mgr: &mut Manager, _target: Option<WidgetId>) {}
+    ///
+    /// When opening menus and `set_focus` is true, the first navigable child
+    /// of the newly opened menu will be given focus. This is used for keyboard
+    /// navigation only.
+    fn set_menu_path(&mut self, mgr: &mut Manager, target: Option<WidgetId>, set_focus: bool) {
+        let _ = (mgr, target, set_focus);
+    }
 }
 
 impl<M: 'static> WidgetCore for Box<dyn Menu<Msg = M>> {
@@ -156,8 +162,8 @@ impl<M: 'static> Menu for Box<dyn Menu<Msg = M>> {
     fn menu_is_open(&self) -> bool {
         self.deref().menu_is_open()
     }
-    fn set_menu_path(&mut self, mgr: &mut Manager, target: Option<WidgetId>) {
-        self.deref_mut().set_menu_path(mgr, target)
+    fn set_menu_path(&mut self, mgr: &mut Manager, target: Option<WidgetId>, set_focus: bool) {
+        self.deref_mut().set_menu_path(mgr, target, set_focus)
     }
 }
 

crates/kas-widgets/src/menu/menu_entry.rs

@@ -33,9 +33,6 @@ impl<M: Clone + Debug + 'static> WidgetConfig for MenuEntry<M> {
     fn key_nav(&self) -> bool {
         true
     }
-    fn hover_highlight(&self) -> bool {
-        true
-    }
 }
 
 impl<M: Clone + Debug + 'static> Layout for MenuEntry<M> {