From 3df7e8f77cbb87f69d1320d701790d9b2588e0b8 Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Sun, 11 Oct 2026 08:29:54 -0400 Subject: [PATCH] Shapes: a closed colour picker leaves the next Escape to the dialog (#612) The picker marked every pass it drew, including the one it closed in, so for one pass after closing the dialog Escape handler still stepped aside and a quick second Escape did nothing. The mark is now set only while the picker stays open and cleared in the pass it closes (OK, Cancel, Escape or a click outside); the picker also consumes the Escape it handles. Test: the_escape_after_the_picker_closes_cancels_the_editor presses Escape to close the picker and again in the very next pass, with no settling frames: the editor cancels. It fails on the previous marking. --- crates/ui-egui/src/color_picker.rs | 18 ++++++++++++------ crates/ui-egui/tests/ui_gradient_editor.rs | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/color_picker.rs b/crates/ui-egui/src/color_picker.rs index 947bffb4..35d8e4eb 100644 --- a/crates/ui-egui/src/color_picker.rs +++ b/crates/ui-egui/src/color_picker.rs @@ -41,10 +41,8 @@ pub fn color_popup(ui: &mut Ui, id: egui::Id, pos: egui::Pos2, c: &mut [f32; 3]) let area = egui::Area::new(id.with("area")).order(egui::Order::Foreground).fixed_pos(pos + vec2(0.0, 4.0)).show(ui.ctx(), |ui| { egui::Frame::popup(ui.style()).show(ui, |ui| out = color_picker(ui, id, c, original)); }); - // Open this pass: dialogs leave Escape to the picker (see `open_recently`). - let pass = ui.ctx().cumulative_pass_nr(); - ui.data_mut(|d| d.insert_temp(open_pass_id(), pass)); - if ui.input(|i| i.key_pressed(egui::Key::Escape)) { + // Escape is the picker's: consume it, so nothing later this pass takes it too. + if ui.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)) { out.close = Some(false); } if pressed_outside(ui.ctx(), &area.response) { @@ -58,7 +56,13 @@ pub fn color_popup(ui: &mut Ui, id: egui::Id, pos: egui::Pos2, c: &mut [f32; 3]) ui.data_mut(|d| { d.insert_temp(id.with("open"), false); d.remove::<[f32; 3]>(orig_id); + // Closed: the next Escape is the dialog's again. + d.remove::(open_pass_id()); }); + } else { + // Still open after this pass: dialogs leave Escape to the picker (see `open_recently`). + let pass = ui.ctx().cumulative_pass_nr(); + ui.data_mut(|d| d.insert_temp(open_pass_id(), pass)); } out.changed } @@ -67,8 +71,10 @@ fn open_pass_id() -> egui::Id { egui::Id::new("color-picker-open-pass") } -/// A colour picker was open in this pass or the last one. Escape then belongs to the picker (it -/// goes back to the original colour and closes), not to the dialog it was opened from. +/// A colour picker is open: it was drawn, and stayed open, in this pass or the last one (so one +/// that stops being drawn without closing, its dialog gone, doesn't count for long). Escape then +/// belongs to the picker (it goes back to the original colour and closes), not to the dialog it +/// was opened from. Closing clears this at once, so the next Escape reaches the dialog. pub fn open_recently(ctx: &egui::Context) -> bool { let pass: Option = ctx.data(|d| d.get_temp(open_pass_id())); pass.is_some_and(|p| p.saturating_add(1) >= ctx.cumulative_pass_nr()) diff --git a/crates/ui-egui/tests/ui_gradient_editor.rs b/crates/ui-egui/tests/ui_gradient_editor.rs index 4e9e9556..fa8c7b73 100644 --- a/crates/ui-egui/tests/ui_gradient_editor.rs +++ b/crates/ui-egui/tests/ui_gradient_editor.rs @@ -469,3 +469,25 @@ fn picker_escape_and_cancel_restore_the_stop_and_keep_the_editor() { assert_eq!(h.state().gradient_draft().opacities.len(), 3, "{how}: the editor's other edits stay"); } } + +/// The Escape that closes the picker is its own: the next Escape, even in the very next pass, +/// cancels the editor (a closed picker doesn't keep the dialog from it). +#[test] +fn the_escape_after_the_picker_closes_cancels_the_editor() { + let (mut h, _, colors) = setup(&[]); + let project = h.state().session.project.clone(); + open_editor(&mut h, colors); + click(&mut h, "dialog.gradient.colorStop.0"); + click(&mut h, "dialog.gradient.color"); + assert!(picker_open(&h), "the picker opened"); + let escape = |h: &mut Harness<'_, EffectcraftApp>| { + h.input_mut().events.push(Event::Key { key: Key::Escape, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::NONE }); + h.input_mut().events.push(Event::Key { key: Key::Escape, physical_key: None, pressed: false, repeat: false, modifiers: Modifiers::NONE }); + h.step(); + }; + escape(&mut h); + assert_eq!(h.state().dialog, Some(Dialog::GradientEditor), "the first Escape closed only the picker"); + escape(&mut h); + assert_eq!(h.state().dialog, None, "the next pass's Escape cancels the editor"); + assert_eq!(h.state().session.project, project, "and changes nothing"); +}