Merge pull request #1939 from dotCooCoo/fix/issue-1692-hex-field-tab

Fix #1692: tabbing through a hex field no longer resets the color

crates/ui-egui/src/dialogs/swatch_options.rs

@@ -252,7 +252,11 @@ fn hex_field(ui: &mut egui::Ui, hex: &str) -> Option<Color> {
     let t = Tokens::get(ui.ctx());
     let id = ui.id().with("swatch-hex");
     let editing = ui.memory(|m| m.has_focus(id));
-    let mut buf: String = if editing { ui.data_mut(|m| m.get_temp::<String>(id)).unwrap_or_else(|| hex.to_string()) } else { hex.to_string() };
+    // As in `widgets::hex_field`: until the user types into it, the field shows `hex` as it is now
+    // (#1692).
+    let edited_id = id.with("edited");
+    let edited = editing && ui.data_mut(|m| m.get_temp::<bool>(edited_id)).unwrap_or(false);
+    let mut buf: String = if edited { ui.data_mut(|m| m.get_temp::<String>(id)).unwrap_or_else(|| hex.to_string()) } else { hex.to_string() };
     let resp = egui::Frame::NONE
         .fill(t.input)
         .stroke(egui::Stroke::new(1.0, if editing { t.accent } else { t.input_border }))
@@ -263,7 +267,11 @@ fn hex_field(ui: &mut egui::Ui, hex: &str) -> Option<Color> {
             ui.add(egui::TextEdit::singleline(&mut buf).id(id).frame(egui::Frame::NONE).desired_width(70.0).char_limit(7))
         })
         .inner;
-    ui.data_mut(|m| m.insert_temp(id, buf.clone()));
+    let focused = ui.memory(|m| m.has_focus(id));
+    ui.data_mut(|m| {
+        m.insert_temp(id, buf.clone());
+        m.insert_temp(edited_id, (edited || resp.changed()) && focused);
+    });
     (resp.lost_focus() && buf != hex).then(|| parse_hex(&buf)).flatten()
 }
 

crates/ui-egui/src/dialogs/tests.rs

@@ -658,6 +658,74 @@ fn a_dialogs_focus_skips_a_greyed_out_first_field() {
     assert_eq!((info["spacing"].clone(), info["steps"].clone()), (json!("steps"), json!(7)), "{info}");
 }
 
+/// #1692: a value typed into the field before the # field of the Color Picker or Swatch Options
+/// stays when Tab passes through the # field. The # field takes the focus in the frame the value is
+/// committed, still showing the color from before it, and commits nothing when Tab moves on. A hex
+/// typed into the # field is still committed by Tab.
+#[test]
+fn tab_through_the_hex_field_keeps_the_value_typed_before_it() {
+    let mut app = app();
+    app.run("swatch.new", json!({"name": "Sky", "color": "#3366cc"})).unwrap();
+    let ctx = egui::Context::default();
+    theme::install_fonts(&ctx);
+    theme::apply(&ctx, Default::default());
+    // One frame of the dialog layer: every text drawn, with its center.
+    let frame = |app: &mut VectorcraftApp, events: Vec<egui::Event>| {
+        let mut out = ctx.run_ui(egui::RawInput { events, ..Default::default() }, |ui| show(app, ui.ctx()));
+        out.textures_delta.clear();
+        fn collect(s: &egui::Shape, out: &mut Vec<(String, egui::Pos2)>) {
+            match s {
+                egui::Shape::Text(t) => out.push((t.galley.text().to_string(), t.visual_bounding_rect().center())),
+                egui::Shape::Vec(v) => v.iter().for_each(|s| collect(s, out)),
+                _ => {}
+            }
+        }
+        let mut texts = vec![];
+        out.shapes.iter().for_each(|c| collect(&c.shape, &mut texts));
+        texts
+    };
+    let key = |key, modifiers| egui::Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers };
+    let (tab, back) = (key(egui::Key::Tab, egui::Modifiers::NONE), key(egui::Key::Tab, egui::Modifiers::SHIFT));
+    let button = |pos, pressed| egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed, modifiers: Default::default() };
+    let color = |app: &VectorcraftApp| app.ui.dialog.as_ref().unwrap().fields["color"].clone();
+    type Open = fn(&mut VectorcraftApp);
+    let opens: [(&str, Open, &str); 2] = [
+        ("Color Picker", |app| drop(app.run("ui.colorPicker", json!({"color": "#ffffff"}))), "FFFFFF"),
+        ("Swatch Options", |app| drop(app.run("ui.swatchOptions", json!({"name": "Sky"}))), "3366CC"),
+    ];
+    for (name, open, hex) in opens {
+        open(&mut app);
+        let before = color(&app);
+        for _ in 0..3 {
+            frame(&mut app, vec![]);
+        }
+        let at = frame(&mut app, vec![]).into_iter().find(|(t, _)| t == hex).unwrap_or_else(|| panic!("{name}: no # field shows {hex}")).1;
+        // Click into the # field, then Shift+Tab to the field before it and type 40 over its value.
+        frame(&mut app, vec![egui::Event::PointerMoved(at), button(at, true)]);
+        frame(&mut app, vec![button(at, false)]);
+        frame(&mut app, vec![back.clone()]);
+        frame(&mut app, vec![]);
+        frame(&mut app, vec![egui::Event::Text("40".into())]);
+        // Tab commits it and gives the # field the focus; the next Tab moves on.
+        frame(&mut app, vec![tab.clone()]);
+        let edited = color(&app);
+        assert_ne!(edited, before, "{name}: the field before # took 40");
+        for events in [vec![], vec![tab.clone()], vec![]] {
+            frame(&mut app, events);
+        }
+        assert_eq!(color(&app), edited, "{name}: passing through # put the color from before the edit back");
+        // Click into the # field, select its text, type a hex over it and press Tab.
+        frame(&mut app, vec![egui::Event::PointerMoved(at), button(at, true)]);
+        frame(&mut app, vec![button(at, false)]);
+        frame(&mut app, vec![key(egui::Key::A, egui::Modifiers::COMMAND)]);
+        frame(&mut app, vec![egui::Event::Text("00FF00".into())]);
+        frame(&mut app, vec![tab.clone()]);
+        assert!(frame(&mut app, vec![]).iter().any(|(t, _)| t == "00FF00"), "{name}: the # field took 00FF00");
+        cancel(&mut app);
+        frame(&mut app, vec![]);
+    }
+}
+
 /// #1000: a count typed in Object › Create Object Mosaic… reaches the command (a whole number typed
 /// in a command's dialog is an integer), so the mosaic has that many columns, not the 10 × 10 grid.
 #[test]

crates/ui-egui/src/tests_numfields.rs

@@ -45,6 +45,135 @@ fn a_focused_field_follows_a_value_changed_from_outside() {
     assert_eq!(frame(50.0, vec![button(away, false)]).0, None);
 }
 
+/// #1692: a hex field that the focus only passes through shows the color as it is now and commits
+/// nothing. Tab out of an edited field gives the hex field after it the focus in the frame the edit
+/// is committed, and the color changes after the hex field is drawn, as in the Color Picker.
+/// Shift+Tab out of the field after it gives it the focus a frame later, in the frame that field
+/// commits.
+#[test]
+fn a_hex_field_passed_through_with_tab_commits_nothing() {
+    let mut f = Field::new();
+    // Red and Blue fields on either side of the hex field of the color they make.
+    let rgb = Cell::new([255.0, 128.0, 0.0]);
+    let (hex, blue, hex_id) = (Cell::new(None), Cell::new(None), Cell::new(egui::Id::NULL));
+    let draw = |ui: &mut egui::Ui| {
+        let [r, g, b] = rgb.get();
+        let red = widgets::plain_field(ui, "red", r, "", 0, 80.0);
+        hex_id.set(ui.id().with("hex"));
+        hex.set(widgets::hex_field(ui, "hex", &format!("{:02X}{:02X}{:02X}", r as u8, g as u8, b as u8)));
+        blue.set(widgets::plain_field(ui, "blue", b, "", 0, 80.0));
+        red
+    };
+    // One frame, its edits applied after it. → What the hex field committed.
+    let step = |f: &mut Field, events: Vec<Event>| {
+        let red = f.frame(events, &draw);
+        let [r, g, b] = rgb.get();
+        rgb.set([red.unwrap_or(r), g, blue.take().unwrap_or(b)]);
+        hex.take()
+    };
+    // The text the hex field shows (it keeps it under its id).
+    let shows = |f: &Field| f.ctx.data(|d| d.get_temp::<String>(hex_id.get()));
+    let (tab, back) = (key(Key::Tab, Modifiers::NONE), key(Key::Tab, Modifiers::SHIFT));
+    step(&mut f, vec![]);
+    // Tab into Red, type 50 over it, and Tab on to the hex field, then on to Blue.
+    for events in [vec![tab.clone()], vec![Event::Text("50".into())], vec![tab.clone()], vec![]] {
+        assert_eq!(step(&mut f, events), None);
+    }
+    assert_eq!(shows(&f).as_deref(), Some("328000"), "the hex field shows Red's edit");
+    for events in [vec![tab.clone()], vec![]] {
+        assert_eq!(step(&mut f, events), None);
+    }
+    assert_eq!(rgb.get(), [50.0, 128.0, 0.0], "Red keeps what was typed");
+    // Type 200 over Blue, Shift+Tab back into the hex field and Tab out of it again.
+    for events in [vec![Event::Text("200".into())], vec![back], vec![], vec![]] {
+        assert_eq!(step(&mut f, events), None);
+    }
+    assert_eq!(shows(&f).as_deref(), Some("3280C8"), "the hex field shows Blue's edit");
+    for events in [vec![tab], vec![]] {
+        assert_eq!(step(&mut f, events), None);
+    }
+    assert_eq!(rgb.get(), [50.0, 128.0, 200.0], "Blue keeps what was typed");
+}
+
+/// What is typed into a hex field is committed when Tab or Enter ends its focus, and Escape drops
+/// it.
+#[test]
+fn typing_into_a_hex_field_commits_it() {
+    let mut f = Field::new();
+    let hex = Cell::new(None);
+    let draw = |ui: &mut egui::Ui| {
+        let red = widgets::plain_field(ui, "red", 255.0, "", 0, 80.0);
+        hex.set(widgets::hex_field(ui, "hex", "FF8000"));
+        red
+    };
+    let step = |f: &mut Field, events: Vec<Event>| {
+        f.frame(events, &draw);
+        hex.take()
+    };
+    let tab = || key(Key::Tab, Modifiers::NONE);
+    let escape = || key(Key::Escape, Modifiers::NONE);
+    step(&mut f, vec![]);
+    for (end, commits) in [(tab(), true), (enter(), true), (escape(), false)] {
+        // Tab into Red and on into the hex field, select its text and type over it.
+        let mut got = vec![];
+        for events in
+            [vec![tab()], vec![tab()], vec![], vec![key(Key::A, Modifiers::COMMAND)], vec![Event::Text("00FF00".into())], vec![end.clone()], vec![]]
+        {
+            got.extend(step(&mut f, events));
+        }
+        let want: Vec<String> = if commits { vec!["00FF00".into()] } else { vec![] };
+        assert_eq!(got, want, "{end:?}");
+        // Neither field has the focus for the next round.
+        step(&mut f, vec![escape()]);
+    }
+}
+
+/// #1692: the Stroke panel's dash and gap fields ([`widgets::opt_field`]) are blank while the
+/// selected objects' dashes differ. Tab out of the dash after typing into it gives the gap the
+/// focus; the gap then shows the dash's length, which the committed dash repeats as its gap. Tab
+/// out of the gap commits nothing, and what is typed into the gap replaces all of that length.
+#[test]
+fn a_gap_field_passed_through_with_tab_commits_nothing() {
+    use crate::panels::stroke::{dash_fields, dash_pattern};
+    let shown = Cell::new([None; 6]);
+    let (dash, gap) = (Cell::new(None), Cell::new(None));
+    let draw = |ui: &mut egui::Ui| -> Option<f64> {
+        let [d, g, ..] = shown.get();
+        dash.set(widgets::opt_field(ui, "dash", d, Unit::Points, 60.0));
+        gap.set(widgets::opt_field(ui, "gap", g, Unit::Points, 60.0));
+        None
+    };
+    // One frame, then its commit applied the way the Stroke panel applies it. → What the gap
+    // committed.
+    let step = |f: &mut Field, events: Vec<Event>| {
+        f.frame(events, &draw);
+        let (d, g) = (dash.take(), gap.take());
+        if d.is_some() || g.is_some() {
+            let mut fields = shown.get();
+            fields[0] = d.unwrap_or(fields[0]);
+            fields[1] = g.unwrap_or(fields[1]);
+            shown.set(dash_fields(&dash_pattern(&fields)));
+        }
+        g
+    };
+    let tab = || key(Key::Tab, Modifiers::NONE);
+    for (typed, commits, pattern) in [(None, None, [20.0, 20.0]), (Some("5"), Some(Some(5.0)), [20.0, 5.0])] {
+        let mut f = Field::new();
+        shown.set([None; 6]);
+        step(&mut f, vec![]);
+        // Tab into the dash, type 20 and Tab on into the gap.
+        for events in [vec![tab()], vec![Event::Text("20".into())], vec![tab()], vec![]] {
+            assert_eq!(step(&mut f, events), None);
+        }
+        if let Some(text) = typed {
+            assert_eq!(step(&mut f, vec![Event::Text(text.into())]), None);
+        }
+        assert_eq!(step(&mut f, vec![tab()]), commits, "{typed:?}");
+        assert_eq!(step(&mut f, vec![]), None);
+        assert_eq!(dash_pattern(&shown.get()), pattern, "{typed:?}");
+    }
+}
+
 /// Headless frames of one field: its rect and id as last drawn.
 struct Field {
     ctx: egui::Context,

crates/ui-egui/src/widgets.rs

@@ -1468,13 +1468,19 @@ pub fn channel_slider(
 }
 
 /// A hex colour field (`E67828`, no `#`) after a `#` label. Returns the text on commit when it
-/// changed.
+/// was typed into the field and differs from `hex`.
 pub fn hex_field(ui: &mut Ui, id: impl std::hash::Hash + std::fmt::Debug, hex: &str) -> Option<String> {
     let t = Tokens::get(ui.ctx());
     ui.label(egui::RichText::new("#").size(15.0).color(t.text));
     let id = ui.id().with(id);
     let editing = ui.memory(|m| m.has_focus(id));
-    let mut buf: String = if editing { ui.data_mut(|d| d.get_temp::<String>(id)).unwrap_or_else(|| hex.to_string()) } else { hex.to_string() };
+    // Until the user types into it, the field shows `hex` as it is now, also while it has the
+    // focus. Tab can give it the focus in the frame another field commits an edit, before the edit
+    // changes `hex`; the text from before that edit is not committed when the focus moves on
+    // (#1692).
+    let edited_id = id.with("edited");
+    let edited = editing && ui.data_mut(|d| d.get_temp::<bool>(edited_id)).unwrap_or(false);
+    let mut buf: String = if edited { ui.data_mut(|d| d.get_temp::<String>(id)).unwrap_or_else(|| hex.to_string()) } else { hex.to_string() };
     let resp = egui::Frame::NONE
         .fill(t.input)
         .stroke(Stroke::new(1.0, if editing { t.accent } else { t.input_border }))
@@ -1493,7 +1499,11 @@ pub fn hex_field(ui: &mut Ui, id: impl std::hash::Hash + std::fmt::Debug, hex: &
         })
         .inner;
     let commit = resp.lost_focus() && buf != hex;
-    ui.data_mut(|d| d.insert_temp(id, buf.clone()));
+    let focused = ui.memory(|m| m.has_focus(id));
+    ui.data_mut(|d| {
+        d.insert_temp(id, buf.clone());
+        d.insert_temp(edited_id, (edited || resp.changed()) && focused);
+    });
     commit.then_some(buf)
 }
 
@@ -1569,7 +1579,16 @@ pub fn opt_field(ui: &mut Ui, id: impl std::hash::Hash + std::fmt::Debug, value:
     let id = ui.id().with(id);
     let shown = value.map(|v| unit.number(v)).unwrap_or_default();
     let editing = ui.memory(|m| m.has_focus(id));
-    let mut buf: String = if editing { ui.data_mut(|d| d.get_temp::<String>(id)).unwrap_or_else(|| shown.clone()) } else { shown.clone() };
+    // As in `hex_field`: until the user types into it, the field shows `shown` as it is now
+    // (#1692). When `shown` changes while the field has the focus and nothing has been typed into
+    // it, all of the new text is selected, as on keyboard focus.
+    let edited_id = id.with("edited");
+    let edited = editing && ui.data_mut(|d| d.get_temp::<bool>(edited_id)).unwrap_or(false);
+    let before = ui.data_mut(|d| d.get_temp::<String>(id));
+    let mut buf: String = if edited { before.clone().unwrap_or_else(|| shown.clone()) } else { shown.clone() };
+    if editing && !edited && before.is_some_and(|b| b != buf) {
+        select_all(ui, id, &buf);
+    }
     let enabled = ui.is_enabled();
     let framed = egui::Frame::NONE
         .fill(t.input)
@@ -1591,7 +1610,11 @@ pub fn opt_field(ui: &mut Ui, id: impl std::hash::Hash + std::fmt::Debug, value:
     // ↑/↓ and the wheel step a value (an empty field stays empty); a drag on its label scrubs it.
     let stepped = step_value(ui, &resp, &mut buf, |b| Some(unit.from_pt(unit.parse(b)?)), |v| unit.number(unit.to_pt(v)));
     let scrubbed = scrub::field(ui, id, framed.response.rect, &buf, value.map(|v| unit.from_pt(v)), STEP_DECIMALS);
-    ui.data_mut(|d| d.insert_temp(id, buf.clone()));
+    let focused = ui.memory(|m| m.has_focus(id));
+    ui.data_mut(|d| {
+        d.insert_temp(id, buf.clone());
+        d.insert_temp(edited_id, (edited || resp.changed()) && focused);
+    });
     if resp.lost_focus() && buf != shown {
         let s = buf.trim();
         if s.is_empty() { Some(None) } else { typed_unit(ui.ctx(), unit).parse(s).map(Some) }