Fix for RouteFocusChanged and focusable descendants (#925)

Author: yrns

CHANGELOG.md

@@ -80,6 +80,7 @@ While some features like the clipboard, menus or file dialogs are not yet availa
 - X11: Support `Application::quit`. ([#900] by [@xStrom])
 - GTK: Support file filters in open/save dialogs. ([#903] by [@jneem])
 - X11: Support key and mouse button state. ([#920] by [@jneem])
+- Routing `LifeCycle::FocusChanged` to descendant widgets. ([#925] by [@yrns])
 
 ### Visual
 
@@ -167,6 +168,7 @@ While some features like the clipboard, menus or file dialogs are not yet availa
 [#917]: https://github.com/xi-editor/druid/pull/917
 [#920]: https://github.com/xi-editor/druid/pull/920
 [#924]: https://github.com/xi-editor/druid/pull/924
+[#925]: https://github.com/xi-editor/druid/pull/925
 
 ## [0.5.0] - 2020-04-01
 
@@ -196,6 +198,7 @@ Last release without a changelog :(
 [@sjoshid]: https://github.com/sjoshid
 [@mastfissh]: https://github.com/mastfissh
 [@Zarenor]: https://github.com/Zarenor
+[@yrns]: https://github.com/yrns
 
 [Unreleased]: https://github.com/xi-editor/druid/compare/v0.5.0...master
 [0.5.0]: https://github.com/xi-editor/druid/compare/v0.4.0...v0.5.0

druid/src/core.rs

@@ -699,8 +699,8 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
 
     pub fn lifecycle(&mut self, ctx: &mut LifeCycleCtx, event: &LifeCycle, data: &T, env: &Env) {
         // in the case of an internal routing event, if we are at our target
-        // we may replace the routing event with the actual event
-        let mut substitute_event = None;
+        // we may send an extra event after the actual event
+        let mut extra_event = None;
 
         let recurse = match event {
             LifeCycle::Internal(internal) => match internal {
@@ -735,20 +735,18 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
                         // Only send FocusChanged in case there's actual change
                         if old != new {
                             self.state.has_focus = change;
-                            substitute_event = Some(LifeCycle::FocusChanged(change));
-                            true
-                        } else {
-                            false
+                            extra_event = Some(LifeCycle::FocusChanged(change));
                         }
                     } else {
                         self.state.has_focus = false;
-                        // Recurse when the target widgets could be our descendants.
-                        // The bloom filter we're checking can return false positives.
-                        match (old, new) {
-                            (Some(old), _) if self.state.children.may_contain(old) => true,
-                            (_, Some(new)) if self.state.children.may_contain(new) => true,
-                            _ => false,
-                        }
+                    }
+
+                    // Recurse when the target widgets could be our descendants.
+                    // The bloom filter we're checking can return false positives.
+                    match (old, new) {
+                        (Some(old), _) if self.state.children.may_contain(old) => true,
+                        (_, Some(new)) if self.state.children.may_contain(new) => true,
+                        _ => false,
                     }
                 }
                 #[cfg(test)]
@@ -790,15 +788,17 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
             }
         };
 
-        // use the substitute event, if one exists
-        let event = substitute_event.as_ref().unwrap_or(event);
+        let mut child_ctx = LifeCycleCtx {
+            command_queue: ctx.command_queue,
+            base_state: &mut self.state,
+            window_id: ctx.window_id,
+        };
 
         if recurse {
-            let mut child_ctx = LifeCycleCtx {
-                command_queue: ctx.command_queue,
-                base_state: &mut self.state,
-                window_id: ctx.window_id,
-            };
+            self.inner.lifecycle(&mut child_ctx, event, data, env);
+        }
+
+        if let Some(event) = extra_event.as_ref() {
             self.inner.lifecycle(&mut child_ctx, event, data, env);
         }
 

druid/src/tests/helpers.rs

@@ -33,7 +33,7 @@ macro_rules! assert_matches {
      }
  }
 
-pub type EventFn<S, T> = dyn FnMut(&mut S, &mut EventCtx, &Event, &T, &Env);
+pub type EventFn<S, T> = dyn FnMut(&mut S, &mut EventCtx, &Event, &mut T, &Env);
 pub type LifeCycleFn<S, T> = dyn FnMut(&mut S, &mut LifeCycleCtx, &LifeCycle, &T, &Env);
 pub type UpdateFn<S, T> = dyn FnMut(&mut S, &mut UpdateCtx, &T, &T, &Env);
 pub type LayoutFn<S, T> = dyn FnMut(&mut S, &mut LayoutCtx, &BoxConstraints, &T, &Env) -> Size;
@@ -83,7 +83,7 @@ pub struct Recording(Rc<RefCell<VecDeque<Record>>>);
 
 /// A recording of a method call on a widget.
 ///
-/// Each member of the enum coorresponds to one of the methods on `Widget`.
+/// Each member of the enum corresponds to one of the methods on `Widget`.
 #[derive(Debug, Clone)]
 pub enum Record {
     /// An `Event`.
@@ -126,7 +126,7 @@ impl<S, T> ModularWidget<S, T> {
 
     pub fn event_fn(
         mut self,
-        f: impl FnMut(&mut S, &mut EventCtx, &Event, &T, &Env) + 'static,
+        f: impl FnMut(&mut S, &mut EventCtx, &Event, &mut T, &Env) + 'static,
     ) -> Self {
         self.event = Some(Box::new(f));
         self
@@ -262,6 +262,15 @@ impl Recording {
         self.0.borrow_mut().pop_front().unwrap_or(Record::None)
     }
 
+    /// Returns an iterator of events drained from the recording.
+    pub fn drain(&self) -> impl Iterator<Item = Record> {
+        self.0
+            .borrow_mut()
+            .drain(..)
+            .collect::<Vec<_>>()
+            .into_iter()
+    }
+
     fn push(&self, event: Record) {
         self.0.borrow_mut().push_back(event)
     }

druid/src/tests/mod.rs

@@ -202,6 +202,87 @@ fn take_focus() {
     })
 }
 
+#[test]
+fn focus_changed() {
+    const TAKE_FOCUS: Selector = Selector::new("druid-tests.take-focus");
+
+    fn make_focus_container(children: Vec<WidgetPod<(), Box<dyn Widget<()>>>>) -> impl Widget<()> {
+        ModularWidget::new(children)
+            .event_fn(|children, ctx, event, data, env| {
+                if let Event::Command(cmd) = event {
+                    if cmd.selector == TAKE_FOCUS {
+                        ctx.request_focus();
+                        // Stop propagating this command so children
+                        // aren't requesting focus too.
+                        ctx.set_handled();
+                    }
+                }
+                children
+                    .iter_mut()
+                    .for_each(|a| a.event(ctx, event, data, env));
+            })
+            .lifecycle_fn(|children, ctx, event, data, env| {
+                children
+                    .iter_mut()
+                    .for_each(|a| a.lifecycle(ctx, event, data, env));
+            })
+    }
+
+    let a_rec = Recording::default();
+    let b_rec = Recording::default();
+    let c_rec = Recording::default();
+
+    let (id_a, id_b, id_c) = widget_id3();
+
+    // a contains b which contains c
+    let c = make_focus_container(vec![]).record(&c_rec).with_id(id_c);
+    let b = make_focus_container(vec![WidgetPod::new(c).boxed()])
+        .record(&b_rec)
+        .with_id(id_b);
+    let a = make_focus_container(vec![WidgetPod::new(b).boxed()])
+        .record(&a_rec)
+        .with_id(id_a);
+
+    let f = |a| match a {
+        Record::L(LifeCycle::FocusChanged(c)) => Some(c),
+        _ => None,
+    };
+    let no_change = |a: &Recording| a.drain().filter_map(f).count() == 0;
+    let changed = |a: &Recording, b| a.drain().filter_map(f).eq(std::iter::once(b));
+
+    Harness::create_simple((), a, |harness| {
+        harness.send_initial_events();
+
+        // focus none -> a
+        harness.submit_command(TAKE_FOCUS, id_a);
+        assert_eq!(harness.window().focus, Some(id_a));
+        assert!(changed(&a_rec, true));
+        assert!(no_change(&b_rec));
+        assert!(no_change(&c_rec));
+
+        // focus a -> b
+        harness.submit_command(TAKE_FOCUS, id_b);
+        assert_eq!(harness.window().focus, Some(id_b));
+        assert!(changed(&a_rec, false));
+        assert!(changed(&b_rec, true));
+        assert!(no_change(&c_rec));
+
+        // focus b -> c
+        harness.submit_command(TAKE_FOCUS, id_c);
+        assert_eq!(harness.window().focus, Some(id_c));
+        assert!(no_change(&a_rec));
+        assert!(changed(&b_rec, false));
+        assert!(changed(&c_rec, true));
+
+        // focus c -> a
+        harness.submit_command(TAKE_FOCUS, id_a);
+        assert_eq!(harness.window().focus, Some(id_a));
+        assert!(changed(&a_rec, true));
+        assert!(no_change(&b_rec));
+        assert!(changed(&c_rec, false));
+    })
+}
+
 #[test]
 fn simple_lifecyle() {
     let record = Recording::default();