Fix focus handling in multi-window applications

This took some time to debug - starting with an application that opens a popup window,
wherein the user should be able to tab between items and press a key to select and close
the popup.

It turned out to be **three** separate issues:

1. `window_handle.process_central_messages()` would discard any messages for `View`s with a different root than its own, so they would never be processed.
2. `ViewId` attempted to cache the root view id, but would just cache the deepest parent found - so if you do something that generates an update message
during construction of a tree of views, the cached value would not actually be the root view for the (not yet existing) window
3. Not quite as strictly a bug, but I can't imagine why it would be by design: `FocusGained` and `FocusLost` events caused by a call to `ViewId.request_focus()`
*would* be delivered to listeners, but a `View` would never receive them via `event_before_children()` or `event_after_children()` - so this one had a
workaround, but the workaround - a view attaching listeners to itself - would be much less readable than a straightforward implementation of one of
the event handler methods

The fix for 3. I am the least sure about:

* It is a bit ugly (swapping an `EventCx` for an `UpdateCx` to call `unconditional_view_event()` and then recreating the `UpdateCx`)
* It is not clear if there are *other* kinds of events which have the same problem of non-delivery to the view itself
* `ViewId.apply_event()` might be a cleaner place to implement it (though it would mean a bunch of calling back and forth between `WindowHandle` and
`ViewId` which seems less than ideal)
* I do *not* stop propagation of events if `event_*_children()` returns `EventPropagation::Stop` - as long is this is about focus events, I think it
is probably harmful to be able to block propagation of focus events - window focus events more so, since it's unlikely that one view in a tree knows
for sure that none of its siblings need to know about the window losing focus - but the same logic applies more weakly to plain view focus events -
it's a recipe for hard-to-diagnose focus bugs.

Since I'm a proud member of the 1990's `println` school of debugging, `UpdateMessage` now implements `Debug` - needed it to prove that retained messages
really were eventually processed, and it will probably be useful in the future.

src/id.rs

@@ -22,7 +22,7 @@ use crate::{
     view::{IntoView, View},
     view_state::{ChangeFlags, StackOffset, ViewState},
     view_storage::VIEW_STORAGE,
-    window_tracking::window_id_for_root,
+    window_tracking::{is_known_root, window_id_for_root},
     ScreenLayout,
 };
 
@@ -37,6 +37,9 @@ impl ViewId {
 
     pub fn remove(&self) {
         VIEW_STORAGE.with_borrow_mut(|s| {
+            // Remove the cached root, in the (unlikely) case that this view is
+            // re-added to a different window
+            s.root.remove(*self);
             if let Some(Some(parent)) = s.parent.get(*self) {
                 if let Some(children) = s.children.get_mut(*parent) {
                     children.retain(|c| c != self);
@@ -155,11 +158,20 @@ impl ViewId {
     pub(crate) fn root(&self) -> Option<ViewId> {
         VIEW_STORAGE.with_borrow_mut(|s| {
             if let Some(root) = s.root.get(*self) {
+                // The cached value will be cleared on remove() above
                 return *root;
             }
             let root_view_id = s.root_view_id(*self);
-            s.root.insert(*self, root_view_id);
-            root_view_id
+            // root_view_id() always returns SOMETHING.  If the view is not yet added
+            // to a window, it can be itself or its nearest ancestor, which means we
+            // will store garbage permanently.
+            if let Some(root) = root_view_id {
+                if is_known_root(&root) {
+                    s.root.insert(*self, root_view_id);
+                    return Some(root);
+                }
+            }
+            None
         })
     }
 

src/lib.rs

@@ -195,6 +195,6 @@ pub use peniko;
 pub use peniko::kurbo;
 pub use screen_layout::ScreenLayout;
 pub use taffy;
-pub use view::{default_compute_layout, recursively_layout_view, AnyView, IntoView, View};
+pub use view::{recursively_layout_view, AnyView, IntoView, View};
 pub use window::{close_window, new_window};
 pub use window_id::{Urgency, WindowIdExt};

src/update.rs

@@ -96,3 +96,72 @@ pub(crate) enum UpdateMessage {
     },
     WindowVisible(bool),
 }
+
+impl std::fmt::Debug for UpdateMessage {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            UpdateMessage::Focus(id) => f.write_fmt(format_args!("Focus({:?})", id)),
+            UpdateMessage::ClearFocus(id) => f.write_fmt(format_args!("ClearFocus({:?})", id)),
+            UpdateMessage::Active(id) => f.write_fmt(format_args!("Active({:?})", id)),
+            UpdateMessage::ClearActive(id) => f.write_fmt(format_args!("ClearActive({:?})", id)),
+            UpdateMessage::WindowScale(scale) => {
+                f.write_fmt(format_args!("WindowScale({})", scale))
+            }
+            UpdateMessage::Disabled { id, is_disabled } => {
+                f.write_fmt(format_args!("Disabled({:?}:{})", id, is_disabled))
+            }
+            UpdateMessage::RequestPaint => f.write_str("RequestPaint"),
+            UpdateMessage::State { id, state: _ } => {
+                f.write_fmt(format_args!("State({:?}:???)", id))
+            }
+            UpdateMessage::KeyboardNavigable { id } => {
+                f.write_fmt(format_args!("KeyboardNavigable({:?})", id))
+            }
+            UpdateMessage::Draggable { id } => f.write_fmt(format_args!("Draggable({:?})", id)),
+            UpdateMessage::ToggleWindowMaximized => f.write_str("ToggleWindowMaximized"),
+            UpdateMessage::SetWindowMaximized(maximized) => {
+                f.write_fmt(format_args!("SetWindowMaximized({})", maximized))
+            }
+            UpdateMessage::MinimizeWindow => f.write_str("MinimizeWindow"),
+            UpdateMessage::DragWindow => f.write_str("DragWindow"),
+            UpdateMessage::DragResizeWindow(direction) => {
+                f.write_fmt(format_args!("DragResizeWindow({:?})", direction))
+            }
+            UpdateMessage::SetWindowDelta(delta) => {
+                f.write_fmt(format_args!("SetWindowDelta({}, {})", delta.x, delta.y))
+            }
+            UpdateMessage::Animation { id, animation: _ } => {
+                f.write_fmt(format_args!("Animation({:?})", id))
+            }
+            UpdateMessage::ShowContextMenu { menu: _, pos } => {
+                f.write_fmt(format_args!("ShowContextMenu({:?})", pos))
+            }
+            UpdateMessage::WindowMenu { menu: _ } => f.write_str("WindowMenu"),
+            UpdateMessage::SetWindowTitle { title } => {
+                f.write_fmt(format_args!("SetWindowTitle({:?})", title))
+            }
+            UpdateMessage::AddOverlay {
+                id,
+                position,
+                view: _,
+            } => f.write_fmt(format_args!("AddOverlay({:?} : {:?})", id, position)),
+            UpdateMessage::RemoveOverlay { id } => {
+                f.write_fmt(format_args!("RemoveOverlay({:?})", id))
+            }
+            UpdateMessage::Inspect => f.write_str("Inspect"),
+            UpdateMessage::ScrollTo { id, rect } => {
+                f.write_fmt(format_args!("ScrollTo({:?}:{:?})", id, rect))
+            }
+            UpdateMessage::FocusWindow => f.write_str("FocusWindow"),
+            UpdateMessage::SetImeAllowed { allowed } => {
+                f.write_fmt(format_args!("SetImeAllowed({})", allowed))
+            }
+            UpdateMessage::SetImeCursorArea { position, size } => {
+                f.write_fmt(format_args!("SetImeCursorArea({:?}, {:?})", position, size))
+            }
+            UpdateMessage::WindowVisible(visible) => {
+                f.write_fmt(format_args!("WindowVisible({})", visible))
+            }
+        }
+    }
+}

src/view_storage.rs

@@ -52,18 +52,18 @@ impl ViewStorage {
         }
     }
 
+    /// Returns the deepest view ID encountered traversing parents.  It does *not* guarantee
+    /// that it is a real window root; any caller should perform the same test
+    /// of `window_tracking::is_known_root()` that `ViewId.root()` does before
+    /// assuming the returned value is really a window root.
     pub(crate) fn root_view_id(&self, id: ViewId) -> Option<ViewId> {
-        let mut parent = self.parent.get(id).cloned().flatten();
-        while let Some(parent_id) = parent {
-            match self.parent.get(parent_id).cloned().flatten() {
-                Some(id) => {
-                    parent = Some(id);
-                }
-                None => {
-                    return parent;
-                }
-            }
+        // Rewritten from the original looping version - unless we anticipate view ids
+        // deeper than the possible stack depth, this should be more efficient and
+        // require less cloning.
+        if let Some(p) = self.parent.get(id).unwrap_or(&None) {
+            self.root_view_id(*p)
+        } else {
+            Some(id)
         }
-        parent
     }
 }

src/window_handle.rs

@@ -310,6 +310,9 @@ impl WindowHandle {
             }
         }
         if was_focused != cx.app_state.focus {
+            // What is this for? If you call ViewId.request_focus(), this
+            // causes focus to be set to your view, then briefly set to
+            // None here and then back.  Why?
             cx.app_state.focus_changed(was_focused, cx.app_state.focus);
         }
 
@@ -701,11 +704,28 @@ impl WindowHandle {
         CENTRAL_UPDATE_MESSAGES.with_borrow_mut(|central_msgs| {
             if !central_msgs.is_empty() {
                 UPDATE_MESSAGES.with_borrow_mut(|msgs| {
-                    let central_msgs = std::mem::take(&mut *central_msgs);
-                    for (id, msg) in central_msgs {
+                    // We need to retain any messages which are for a view that either belongs
+                    // to a different window, or which does not yet have a root
+                    let removed_central_msgs =
+                        std::mem::replace(central_msgs, Vec::with_capacity(central_msgs.len()));
+                    for (id, msg) in removed_central_msgs {
                         if let Some(root) = id.root() {
                             let msgs = msgs.entry(root).or_default();
                             msgs.push(msg);
+                        } else {
+                            // Messages that are not for our root get put back - they may
+                            // belong to another window, or may be construction-time messages
+                            // for a View that does not yet have a window but will momentarily.
+                            //
+                            // Note that if there is a plethora of events for ids which were created
+                            // but never assigned to any view, they will probably pile up in here,
+                            // and if that becomes a real problem, we may want a garbage collection
+                            // mechanism, or give every message a max-touch-count and discard it
+                            // if it survives too many iterations through here. Unclear if there
+                            // are real-world app development patterns where that could actually be
+                            // an issue. Since any such mechanism would have some overhead, there
+                            // should be a proven need before building one.
+                            central_msgs.push((id, msg));
                         }
                     }
                 });
@@ -716,11 +736,17 @@ impl WindowHandle {
             if !central_msgs.borrow().is_empty() {
                 DEFERRED_UPDATE_MESSAGES.with(|msgs| {
                     let mut msgs = msgs.borrow_mut();
-                    let central_msgs = std::mem::take(&mut *central_msgs.borrow_mut());
-                    for (id, msg) in central_msgs {
+                    let removed_central_msgs = std::mem::replace(
+                        &mut *central_msgs.borrow_mut(),
+                        Vec::with_capacity(msgs.len()),
+                    );
+                    let unprocessed = &mut *central_msgs.borrow_mut();
+                    for (id, msg) in removed_central_msgs {
                         if let Some(root) = id.root() {
                             let msgs = msgs.entry(root).or_default();
                             msgs.push((id, msg));
+                        } else {
+                            unprocessed.push((id, msg));
                         }
                     }
                 });
@@ -748,11 +774,40 @@ impl WindowHandle {
                         if cx.app_state.focus != Some(id) {
                             let old = cx.app_state.focus;
                             cx.app_state.focus = Some(id);
+
+                            // Fix: Focus events were never dispatched to views' own
+                            // event_before_children / event_after_children.
+                            //
+                            // This is a bit messy; could it be done in ViewId.apply_event()
+                            // instead?  Are there other cases of dropped events?
+                            let mut ec = EventCx {
+                                app_state: cx.app_state,
+                            };
+
+                            if let Some(old) = old.as_ref() {
+                                ec.unconditional_view_event(*old, Event::FocusLost, true);
+                            }
+                            ec.unconditional_view_event(id, Event::FocusGained, true);
+
+                            cx = UpdateCx {
+                                app_state: ec.app_state,
+                            };
+
                             cx.app_state.focus_changed(old, cx.app_state.focus);
                         }
                     }
                     UpdateMessage::ClearFocus(id) => {
+                        let old = cx.app_state.focus;
                         cx.app_state.clear_focus();
+                        if let Some(old) = old {
+                            let mut ec = EventCx {
+                                app_state: cx.app_state,
+                            };
+                            ec.unconditional_view_event(old, Event::FocusLost, true);
+                            cx = UpdateCx {
+                                app_state: ec.app_state,
+                            };
+                        }
                         cx.app_state.focus_changed(Some(id), None);
                     }
                     UpdateMessage::Active(id) => {

src/window_tracking.rs

@@ -72,7 +72,17 @@ impl WindowMapping {
     }
 
     fn window_id_for_root(&self, id: &ViewId) -> Option<WindowId> {
-        self.window_id_for_root_view_id.get(id).copied()
+        let result = self.window_id_for_root_view_id.get(id).copied();
+        // We are called by ViewId.window_id(), which should no longer ever return a
+        // ViewId from root() that is not actually a root - so if we get here with a
+        // window id that has gained a parent since it was determined to be a root,
+        // something is very wrong.
+        debug_assert!(
+            id.parent().is_none(),
+            "Not a root view id: {:?} - check the logic in ViewId.window_id().",
+            id
+        );
+        result
     }
 
     fn root_view_id_for(&self, window_id: &WindowId) -> Option<ViewId> {
@@ -94,6 +104,10 @@ pub fn with_window_id_and_window<F: FnOnce(&WindowId, &Window) -> T, T>(
         .unwrap_or(None)
 }
 
+pub fn is_known_root(id: &ViewId) -> bool {
+    with_window_map(|map| map.window_id_for_root_view_id.contains_key(id)).unwrap_or(false)
+}
+
 fn with_window_map_mut<F: FnMut(&mut WindowMapping)>(mut f: F) -> bool {
     let map = WINDOW_FOR_WINDOW_AND_ROOT_IDS.get_or_init(|| RwLock::new(Default::default()));
     if let Ok(mut map) = map.write() {