Make focus request handling predictable. (#948)

CHANGELOG.md

@@ -103,6 +103,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i
 - X11: Support key and mouse button state. ([#920] by [@jneem])
 - Routing `LifeCycle::FocusChanged` to descendant widgets. ([#925] by [@yrns])
 - Built-in open and save menu items now show the correct label and submit the right commands. ([#930] by [@finnerale])
+- Focus request handling is now predictable with the last request overriding earlier ones. ([#948] by [@xStrom])
 - Wheel events now properly update hot state. ([#951] by [@xStrom])
 - X11: Support mouse scrolling. ([#961] by [@jneem])
 
@@ -205,6 +206,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i
 [#940]: https://github.com/xi-editor/druid/pull/940
 [#942]: https://github.com/xi-editor/druid/pull/942
 [#943]: https://github.com/xi-editor/druid/pull/943
+[#948]: https://github.com/xi-editor/druid/pull/948
 [#949]: https://github.com/xi-editor/druid/pull/949
 [#951]: https://github.com/xi-editor/druid/pull/951
 [#953]: https://github.com/xi-editor/druid/pull/953

druid/src/contexts.rs

@@ -356,16 +356,20 @@ impl<'a> EventCtx<'a> {
 
     /// Request keyboard focus.
     ///
-    /// Calling this when the widget is already focused does nothing.
+    /// Because only one widget can be focused at a time, multiple focus requests
+    /// from different widgets during a single event cycle means that the last
+    /// widget that requests focus will override the previous requests.
     ///
     /// See [`is_focused`] for more information about focus.
     ///
     /// [`is_focused`]: struct.EventCtx.html#method.is_focused
     pub fn request_focus(&mut self) {
+        // We need to send the request even if we're currently focused,
+        // because we may have a sibling widget that already requested focus
+        // and we have no way of knowing that yet. We need to override that
+        // to deliver on the "last focus request wins" promise.
         let id = self.widget_id();
-        if self.focus_widget != Some(id) {
-            self.base_state.request_focus = Some(FocusChange::Focus(id));
-        }
+        self.base_state.request_focus = Some(FocusChange::Focus(id));
     }
 
     /// Transfer focus to the next focusable widget.

druid/src/core.rs

@@ -15,6 +15,7 @@
 //! The fundamental druid types.
 
 use std::collections::{HashMap, VecDeque};
+use std::mem;
 
 use crate::bloom::Bloom;
 use crate::kurbo::{Affine, Insets, Point, Rect, Shape, Size, Vec2};
@@ -235,7 +236,7 @@ impl<T, W: Widget<T>> WidgetPod<T, W> {
         }
 
         if needs_merge {
-            ctx.base_state.merge_up(&self.state);
+            ctx.base_state.merge_up(&mut self.state);
         }
     }
 
@@ -525,7 +526,7 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
         };
         let size = self.inner.layout(&mut child_ctx, bc, data, env);
 
-        ctx.base_state.merge_up(&child_ctx.base_state);
+        ctx.base_state.merge_up(&mut child_ctx.base_state);
 
         if size.width.is_infinite() {
             let name = self.widget().type_name();
@@ -746,9 +747,7 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
             child_ctx.base_state.has_active |= child_ctx.base_state.is_active;
         };
 
-        ctx.base_state.merge_up(&child_ctx.base_state);
-        // Clear current widget's timers after merging with parent.
-        child_ctx.base_state.timers.clear();
+        ctx.base_state.merge_up(&mut child_ctx.base_state);
         ctx.is_handled |= child_ctx.is_handled;
     }
 
@@ -776,8 +775,6 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
                     }
                 }
                 InternalLifeCycle::RouteFocusChanged { old, new } => {
-                    self.state.request_focus = None;
-
                     let this_changed = if *old == Some(self.state.id) {
                         Some(false)
                     } else if *new == Some(self.state.id) {
@@ -787,11 +784,8 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
                     };
 
                     if let Some(change) = this_changed {
-                        // Only send FocusChanged in case there's actual change
-                        if old != new {
-                            self.state.has_focus = change;
-                            extra_event = Some(LifeCycle::FocusChanged(change));
-                        }
+                        self.state.has_focus = change;
+                        extra_event = Some(LifeCycle::FocusChanged(change));
                     } else {
                         self.state.has_focus = false;
                     }
@@ -863,7 +857,7 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
             self.inner.lifecycle(&mut child_ctx, event, data, env);
         }
 
-        ctx.base_state.merge_up(&self.state);
+        ctx.base_state.merge_up(&mut self.state);
 
         // we need to (re)register children in case of one of the following events
         match event {
@@ -907,7 +901,7 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
         self.old_data = Some(data.clone());
         self.env = Some(env.clone());
 
-        ctx.base_state.merge_up(&self.state)
+        ctx.base_state.merge_up(&mut self.state)
     }
 }
 
@@ -949,7 +943,9 @@ impl BaseState {
     }
 
     /// Update to incorporate state changes from a child.
-    fn merge_up(&mut self, child_state: &BaseState) {
+    ///
+    /// This will also clear some requests in the child state.
+    fn merge_up(&mut self, child_state: &mut BaseState) {
         let mut child_region = child_state.invalid.clone();
         child_region += child_state.layout_rect().origin().to_vec2() - child_state.viewport_offset;
         let clip = self
@@ -965,8 +961,15 @@ impl BaseState {
         self.has_active |= child_state.has_active;
         self.has_focus |= child_state.has_focus;
         self.children_changed |= child_state.children_changed;
-        self.request_focus = self.request_focus.or(child_state.request_focus);
-        self.timers.extend(&child_state.timers);
+        self.request_focus = child_state.request_focus.take().or(self.request_focus);
+
+        if !child_state.timers.is_empty() {
+            if self.timers.is_empty() {
+                mem::swap(&mut self.timers, &mut child_state.timers);
+            } else {
+                self.timers.extend(&mut child_state.timers.drain());
+            }
+        }
     }
 
     #[inline]

druid/src/tests/mod.rs

@@ -188,11 +188,24 @@ fn take_focus() {
         assert!(left_focus.get().is_none());
         assert!(right_focus.get().is_none());
 
-        // this is sent to all widgets; the first widget to request focus should get it
+        // this is sent to all widgets; the last widget to request focus should get it
         harness.submit_command(TAKE_FOCUS, None);
+        assert_eq!(harness.window().focus, Some(id_2));
+        assert_eq!(left_focus.get(), None);
+        assert_eq!(right_focus.get(), Some(true));
+
+        // this is sent to all widgets; the last widget to request focus should still get it
+        // NOTE: This tests siblings in particular, so careful when moving away from Split.
+        harness.submit_command(TAKE_FOCUS, None);
+        assert_eq!(harness.window().focus, Some(id_2));
+        assert_eq!(left_focus.get(), None);
+        assert_eq!(right_focus.get(), Some(true));
+
+        // this is sent to a specific widget; it should get focus
+        harness.submit_command(TAKE_FOCUS, id_1);
         assert_eq!(harness.window().focus, Some(id_1));
         assert_eq!(left_focus.get(), Some(true));
-        assert_eq!(right_focus.get(), None);
+        assert_eq!(right_focus.get(), Some(false));
 
         // this is sent to a specific widget; it should get focus
         harness.submit_command(TAKE_FOCUS, id_2);
@@ -205,6 +218,8 @@ fn take_focus() {
 #[test]
 fn focus_changed() {
     const TAKE_FOCUS: Selector = Selector::new("druid-tests.take-focus");
+    const ALL_TAKE_FOCUS_BEFORE: Selector = Selector::new("druid-tests.take-focus-before");
+    const ALL_TAKE_FOCUS_AFTER: Selector = Selector::new("druid-tests.take-focus-after");
 
     fn make_focus_container(children: Vec<WidgetPod<(), Box<dyn Widget<()>>>>) -> impl Widget<()> {
         ModularWidget::new(children)
@@ -215,11 +230,18 @@ fn focus_changed() {
                         // Stop propagating this command so children
                         // aren't requesting focus too.
                         ctx.set_handled();
+                    } else if cmd.selector == ALL_TAKE_FOCUS_BEFORE {
+                        ctx.request_focus();
                     }
                 }
                 children
                     .iter_mut()
                     .for_each(|a| a.event(ctx, event, data, env));
+                if let Event::Command(cmd) = event {
+                    if cmd.selector == ALL_TAKE_FOCUS_AFTER {
+                        ctx.request_focus();
+                    }
+                }
             })
             .lifecycle_fn(|children, ctx, event, data, env| {
                 children
@@ -280,6 +302,20 @@ fn focus_changed() {
         assert!(changed(&a_rec, true));
         assert!(no_change(&b_rec));
         assert!(changed(&c_rec, false));
+
+        // all focus before passing down the event
+        harness.submit_command(ALL_TAKE_FOCUS_BEFORE, None);
+        assert_eq!(harness.window().focus, Some(id_c));
+        assert!(changed(&a_rec, false));
+        assert!(no_change(&b_rec));
+        assert!(changed(&c_rec, true));
+
+        // all focus after passing down the event
+        harness.submit_command(ALL_TAKE_FOCUS_AFTER, None);
+        assert_eq!(harness.window().focus, Some(id_a));
+        assert!(changed(&a_rec, true));
+        assert!(no_change(&b_rec));
+        assert!(changed(&c_rec, false));
     })
 }
 

druid/src/window.rs

@@ -212,9 +212,12 @@ impl<T: Data> Window<T> {
         if let Some(focus_req) = base_state.request_focus.take() {
             let old = self.focus;
             let new = self.widget_for_focus_request(focus_req);
-            let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new });
-            self.lifecycle(queue, &event, data, env, false);
-            self.focus = new;
+            // Only send RouteFocusChanged in case there's actual change
+            if old != new {
+                let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new });
+                self.lifecycle(queue, &event, data, env, false);
+                self.focus = new;
+            }
         }
 
         if let Some(cursor) = cursor {