Clean up IME handling code (#599)

Should fix the way repaints were requested every paint on some
platforms.

This PR unfortunately comes with a lack of tests. TestHarness currently
doesn't have good support for testing signals, but even if it did, this
is an inherently visual feature that we don't really have a way to test
visually.

I don't know how to trigger IME on my machine. (I'm using PopOS, a
Debian variant.) If someone wants to either test this or help me get to
speed, I'd appreciate it.

masonry/src/contexts.rs

@@ -14,7 +14,7 @@ use crate::action::Action;
 use crate::passes::layout::run_layout_on;
 use crate::render_root::{MutateCallback, RenderRootSignal, RenderRootState};
 use crate::text::TextBrush;
-use crate::text_helpers::{ImeChangeSignal, TextFieldRegistration};
+use crate::text_helpers::ImeChangeSignal;
 use crate::tree_arena::ArenaMutChildren;
 use crate::widget::{WidgetMut, WidgetState};
 use crate::{
@@ -744,10 +744,7 @@ impl LifeCycleCtx<'_> {
 
     /// Register this widget as accepting text input.
     pub fn register_as_text_input(&mut self) {
-        let registration = TextFieldRegistration {
+        self.widget_state.is_text_input = true;
-            widget_id: self.widget_id(),
-        };
-        self.widget_state.text_registrations.push(registration);
     }
 
     // TODO - remove - See issue https://github.com/linebender/xilem/issues/366
@@ -1021,16 +1018,6 @@ impl_context_method!(LayoutCtx<'_>, PaintCtx<'_>, {
     }
 });
 
-impl PaintCtx<'_> {
-    // signal may be useful elsewhere, but is currently only used on PaintCtx
-    /// Submit a [`RenderRootSignal`]
-    ///
-    /// Note: May be removed in future, and replaced with more specific methods.
-    pub fn signal(&mut self, s: RenderRootSignal) {
-        self.global_state.signal_queue.push_back(s);
-    }
-}
-
 impl AccessCtx<'_> {
     pub fn current_node(&mut self) -> &mut NodeBuilder {
         &mut self.current_node

masonry/src/passes/compose.rs

@@ -5,7 +5,7 @@ use tracing::info_span;
 use vello::kurbo::Vec2;
 
 use crate::passes::recurse_on_children;
-use crate::render_root::{RenderRoot, RenderRootState};
+use crate::render_root::{RenderRoot, RenderRootSignal, RenderRootState};
 use crate::tree_arena::ArenaMut;
 use crate::{ComposeCtx, Widget, WidgetState};
 
@@ -22,7 +22,7 @@ fn compose_widget(
     let translation = parent_translation + state.item.translation + state.item.origin.to_vec2();
     state.item.window_origin = translation.to_point();
 
-    if !moved && !state.item.translation_changed && !state.item.needs_compose {
+    if !parent_moved && !state.item.translation_changed && !state.item.needs_compose {
         return;
     }
 
@@ -36,6 +36,14 @@ fn compose_widget(
         widget.item.compose(&mut ctx);
     }
 
+    // TODO - Add unit tests for this.
+    if moved {
+        let ime_area = state.item.get_ime_area();
+        global_state
+            .signal_queue
+            .push_back(RenderRootSignal::new_ime_moved_signal(ime_area));
+    }
+
     // We need to update the accessibility node's coordinates and repaint it at the new position.
     state.item.request_accessibility = true;
     state.item.needs_accessibility = true;

masonry/src/passes/update.rs

@@ -244,6 +244,14 @@ pub(crate) fn run_update_focus_pass(root: &mut RenderRoot, root_state: &mut Widg
     }
 
     if prev_focused != next_focused {
+        let was_ime_active = root.state.is_ime_active;
+        let is_ime_active = if let Some(id) = next_focused {
+            root.widget_arena.get_state(id).item.is_text_input
+        } else {
+            false
+        };
+        root.state.is_ime_active = is_ime_active;
+
         run_single_update_pass(root, prev_focused, |widget, ctx| {
             widget.on_status_change(ctx, &StatusChange::FocusChanged(false));
         });
@@ -251,14 +259,21 @@ pub(crate) fn run_update_focus_pass(root: &mut RenderRoot, root_state: &mut Widg
             widget.on_status_change(ctx, &StatusChange::FocusChanged(true));
         });
 
-        // TODO: discriminate between text focus, and non-text focus.
+        if prev_focused.is_some() && was_ime_active {
-        root.state
+            root.state.signal_queue.push_back(RenderRootSignal::EndIme);
-            .signal_queue
+        }
-            .push_back(if next_focused.is_some() {
+        if next_focused.is_some() && is_ime_active {
-                RenderRootSignal::StartIme
+            root.state
-            } else {
+                .signal_queue
-                RenderRootSignal::EndIme
+                .push_back(RenderRootSignal::StartIme);
-            });
+        }
+
+        if let Some(id) = next_focused {
+            let ime_area = root.widget_arena.get_state(id).item.get_ime_area();
+            root.state
+                .signal_queue
+                .push_back(RenderRootSignal::new_ime_moved_signal(ime_area));
+        }
     }
 
     root.state.focused_widget = root.state.next_focused_widget;

masonry/src/render_root.rs

@@ -72,6 +72,7 @@ pub(crate) struct RenderRootState {
     pub(crate) font_context: FontContext,
     pub(crate) text_layout_context: LayoutContext<TextBrush>,
     pub(crate) mutate_callbacks: Vec<MutateCallback>,
+    pub(crate) is_ime_active: bool,
     pub(crate) scenes: HashMap<WidgetId, Scene>,
 }
 
@@ -151,6 +152,7 @@ impl RenderRoot {
                 },
                 text_layout_context: LayoutContext::new(),
                 mutate_callbacks: Vec::new(),
+                is_ime_active: false,
                 scenes: HashMap::new(),
             },
             widget_arena: WidgetArena {
@@ -542,13 +544,6 @@ impl RenderRoot {
         }
 
         run_mutate_pass(self, widget_state);
-
-        #[cfg(FALSE)]
-        for ime_field in widget_state.text_registrations.drain(..) {
-            let token = self.handle.add_text_field();
-            tracing::debug!("{:?} added", token);
-            self.ime_handlers.push((token, ime_field));
-        }
     }
 
     // Checks whether the given id points to a widget that is "interactive".
@@ -598,6 +593,21 @@ impl RenderRoot {
     }
 }
 
+impl RenderRootSignal {
+    pub(crate) fn new_ime_moved_signal(area: Rect) -> Self {
+        RenderRootSignal::ImeMoved(
+            LogicalPosition {
+                x: area.origin().x,
+                y: area.origin().y + area.size().height,
+            },
+            LogicalSize {
+                width: area.size().width,
+                height: area.size().height,
+            },
+        )
+    }
+}
+
 /*
 TODO:
 - Invalidation regions

masonry/src/text_helpers.rs

@@ -10,7 +10,7 @@ use vello::{
     Scene,
 };
 
-use crate::{text::TextBrush, WidgetId};
+use crate::text::TextBrush;
 
 /// A reference counted string slice.
 ///
@@ -18,14 +18,6 @@ use crate::{text::TextBrush, WidgetId};
 /// it cannot be mutated, but unlike `String` it can be cheaply cloned.
 pub type ArcStr = std::sync::Arc<str>;
 
-/// A type we use to keep track of which widgets are responsible for which
-/// ime sessions.
-#[derive(Clone, Debug)]
-#[allow(unused)]
-pub(crate) struct TextFieldRegistration {
-    pub widget_id: WidgetId,
-}
-
 // Copy-pasted from druid_shell
 /// An event representing an application-initiated change in [`InputHandler`]
 /// state.

masonry/src/widget/textbox.rs

@@ -15,10 +15,9 @@ use vello::{
 };
 use winit::event::Ime;
 
+use crate::text::{TextBrush, TextEditor, TextWithSelection};
 use crate::widget::{LineBreaking, WidgetMut};
 use crate::{
-    dpi::{LogicalPosition, LogicalSize},
-    text::{TextBrush, TextEditor, TextWithSelection},
     AccessCtx, AccessEvent, BoxConstraints, CursorIcon, EventCtx, LayoutCtx, LifeCycle,
     LifeCycleCtx, PaintCtx, PointerEvent, RegisterCtx, StatusChange, TextEvent, Widget, WidgetId,
 };
@@ -245,6 +244,9 @@ impl Widget for Textbox {
 
     fn lifecycle(&mut self, ctx: &mut LifeCycleCtx, event: &LifeCycle) {
         match event {
+            LifeCycle::WidgetAdded => {
+                ctx.register_as_text_input();
+            }
             LifeCycle::DisabledChanged(disabled) => {
                 if self.show_disabled {
                     if *disabled {
@@ -326,19 +328,6 @@ impl Widget for Textbox {
             None,
             &outline_rect,
         );
-        let origin = ctx.widget_state.window_origin();
-        if ctx.widget_state.has_focus {
-            ctx.signal(crate::render_root::RenderRootSignal::ImeMoved(
-                LogicalPosition {
-                    x: origin.x,
-                    y: origin.y + size.height,
-                },
-                LogicalSize {
-                    width: size.width,
-                    height: size.height,
-                },
-            ));
-        }
     }
 
     fn get_cursor(&self) -> CursorIcon {

masonry/src/widget/widget_state.rs

@@ -6,7 +6,6 @@
 use std::sync::atomic::{AtomicBool, Ordering};
 use vello::kurbo::{Insets, Point, Rect, Size, Vec2};
 
-use crate::text_helpers::TextFieldRegistration;
 use crate::{CursorIcon, WidgetId};
 
 // TODO - Sort out names of widget state flags in two categories:
@@ -63,9 +62,16 @@ pub struct WidgetState {
     /// the baseline. Widgets that contain text or controls that expect to be
     /// laid out alongside text can set this as appropriate.
     pub(crate) baseline_offset: f64,
-    // TODO - Document
+    // TODO - Remove
     pub(crate) is_portal: bool,
 
+    /// Tracks whether widget is eligible for IME events.
+    /// Should be immutable after `WidgetAdded` event.
+    pub(crate) is_text_input: bool,
+    /// The area of the widget that is being edited by
+    /// an IME, in local coordinates.
+    pub(crate) ime_area: Option<Rect>,
+
     // TODO - Use general Shape
     // Currently Kurbo doesn't really provide a type that lets us
     // efficiently hold an arbitrary shape.
@@ -121,8 +127,6 @@ pub struct WidgetState {
     // TODO - Remove and handle in WidgetRoot instead
     pub(crate) cursor: Option<CursorIcon>,
 
-    pub(crate) text_registrations: Vec<TextFieldRegistration>,
-
     // --- STATUS ---
     /// This widget has been disabled.
     pub(crate) is_explicitly_disabled: bool,
@@ -170,6 +174,8 @@ impl WidgetState {
             paint_insets: Insets::ZERO,
             local_paint_rect: Rect::ZERO,
             is_portal: false,
+            is_text_input: false,
+            ime_area: None,
             clip: Default::default(),
             translation: Vec2::ZERO,
             translation_changed: false,
@@ -197,7 +203,6 @@ impl WidgetState {
             focus_chain: Vec::new(),
             children_changed: true,
             cursor: None,
-            text_registrations: Vec::new(),
             update_focus_chain: true,
             #[cfg(debug_assertions)]
             needs_visit: VisitBool(false.into()),
@@ -257,8 +262,6 @@ impl WidgetState {
         self.needs_update_disabled |= child_state.needs_update_disabled;
         self.has_focus |= child_state.has_focus;
         self.children_changed |= child_state.children_changed;
-        self.text_registrations
-            .append(&mut child_state.text_registrations);
         self.update_focus_chain |= child_state.update_focus_chain;
     }
 
@@ -289,6 +292,13 @@ impl WidgetState {
         Rect::from_origin_size(self.window_origin(), self.size)
     }
 
+    /// Returns the area being edited by an IME, in global coordinates.
+    ///
+    /// By default, returns the same as [`Self::window_layout_rect`].
+    pub(crate) fn get_ime_area(&self) -> Rect {
+        self.ime_area.unwrap_or_else(|| self.size.to_rect()) + self.window_origin.to_vec2()
+    }
+
     pub(crate) fn window_origin(&self) -> Point {
         self.window_origin
     }