Integration: keep the font menu's arrows walking with recent fonts (#3559)

Each arrow step in the open font menu applies its family, the options bar,
Character and Glyphs panels record it as the most recent font, and the next
frame hoisted it to row 0: with Recent Fonts on (default 10), Down then
alternated between two families and Up stayed on the first row (CodeRabbit's
open thread on #3559). The menu now keeps the recent group it opened with.

As in Photoshop, the recent families now head the menu in their own group
above a divider and keep their alphabetical place below it, instead of
moving out of it. The arrows start from the current family's alphabetical
copy (or the row the last arrow chose), and only that row is marked.

Tests: open_menu_arrows_walk_on_through_recent_fonts,
options_bar_menu_arrows_walk_on_while_recording_recent_fonts and
open_menu_on_a_recent_font_steps_through_the_alphabet fail without the
fix; recent_fonts_appear_first_in_the_menu pins the group, divider and
alphabetical copies.

Co-authored-by: Reynold Hu <88558425+reynold-hu@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/ui-egui/src/type_font_picker_tests.rs

@@ -696,7 +696,7 @@ fn font_menu_rows_search_both_names_and_sort_by_label() {
     let labels: HashMap<String, String> =
         [("MS Gothic", "ＭＳ ゴシック"), ("Microsoft YaHei", "微软雅黑")].iter().map(|(a, b)| (a.to_string(), b.to_string())).collect();
     let rows = |labels: Option<&HashMap<String, String>>, q: &str| -> Vec<(String, String)> {
-        super::font_menu_rows(&families, labels, q, &[]).into_iter().map(|(f, l)| (f.clone(), l.to_string())).collect()
+        super::font_menu_rows(&families, labels, q, &[]).0.into_iter().map(|(f, l)| (f.clone(), l.to_string())).collect()
     };
     let english = rows(None, "");
     assert!(english.iter().all(|(f, l)| f == l));
@@ -707,26 +707,121 @@ fn font_menu_rows_search_both_names_and_sort_by_label() {
     assert!(rows(Some(&labels), "nothing").is_empty());
 }
 
-/// Preferences ▸ Type ▸ Recent Fonts: recently used families move to the top (most recent first,
-/// once each); a query drops the hoisting so the list stays alphabetical while searching.
+/// Preferences ▸ Type ▸ Recent Fonts: as in Photoshop, recently used families head the menu in
+/// their own group (most recent first, once each) and keep their alphabetical place below it; a
+/// query lists each match once, alphabetically.
 #[test]
-fn recent_fonts_are_hoisted_to_the_top() {
+fn recent_fonts_head_the_menu_and_keep_their_place() {
     let families: Vec<String> = ["Arial", "Baskerville", "Courier", "Zeta Sans"].iter().map(|s| s.to_string()).collect();
-    let recents: Vec<String> = vec!["Zeta Sans".into(), "Courier".into()];
-    let rows = |q: &str| -> Vec<String> { super::font_menu_rows(&families, None, q, &recents).into_iter().map(|(f, _)| f.clone()).collect() };
-    assert_eq!(rows(""), ["Zeta Sans", "Courier", "Arial", "Baskerville"], "recents hoist, most recent first, once each");
-    assert_eq!(rows("a"), ["Arial", "Baskerville", "Zeta Sans"], "a query drops the hoisting");
+    let recents: Vec<String> = vec!["Zeta Sans".into(), "Courier".into(), "Gone".into()];
+    let rows = |q: &str| -> (Vec<String>, usize) {
+        let (rows, n) = super::font_menu_rows(&families, None, q, &recents);
+        (rows.into_iter().map(|(f, _)| f.clone()).collect(), n)
+    };
+    assert_eq!(
+        rows(""),
+        (vec!["Zeta Sans".into(), "Courier".into(), "Arial".into(), "Baskerville".into(), "Courier".into(), "Zeta Sans".into()], 2),
+        "a recent group (no uninstalled family), then the whole alphabetical list"
+    );
+    assert_eq!(rows("a"), (vec!["Arial".into(), "Baskerville".into(), "Zeta Sans".into()], 0), "a query lists each match once");
 }
 
-/// The menu itself draws the hoisted recents first.
+/// The menu itself draws the recent group first, and the recent families again in their place.
 #[test]
 fn recent_fonts_appear_first_in_the_menu() {
     let mut h = harness_with_recents(FontPreview::Medium, vec!["Zeta Sans".into(), "Family 05".into()]);
     open(&mut h);
-    let zeta = h.get_by_label("Zeta Sans").rect().top();
-    let five = h.get_by_label("Family 05").rect().top();
-    let six = h.get_by_label("Family 06").rect().top();
-    assert!(zeta < five && five < six, "recents come first, in order: {zeta} {five} {six}");
+    let tops = |h: &Harness<'static, State>, label: &str| -> Vec<f32> {
+        let mut t: Vec<f32> = h.query_all_by_label(label).map(|n| n.rect().top()).collect();
+        t.sort_by(f32::total_cmp);
+        t
+    };
+    let (zeta, five, first, six) = (tops(&h, "Zeta Sans"), tops(&h, "Family 05"), tops(&h, "Family 00"), tops(&h, "Family 06"));
+    assert_eq!(five.len(), 2, "a recent family is listed twice: {five:?}");
+    assert!(zeta[0] < five[0] && five[0] < first[0], "the recent group comes first, in order: {zeta:?} {five:?} {first:?}");
+    assert!(first[0] < five[1] && five[1] < six[0], "and keeps its alphabetical place: {five:?} {six:?}");
+    assert!(first[0] - five[0] > five[0] - zeta[0], "a divider ends the recent group: {zeta:?} {five:?} {first:?}");
+}
+
+/// The open menu's arrows walk on through the list although each step records its family as
+/// the most recent one (CodeRabbit on #3559): the recent group keeps its order while the menu
+/// is open, and a step from a recent family's lower copy goes on from there.
+#[test]
+fn open_menu_arrows_walk_on_through_recent_fonts() {
+    let mut h = harness_with_recents(FontPreview::Off, vec!["Family 03".into()]);
+    open(&mut h);
+    let mut seen = vec![h.state().current.clone()];
+    for _ in 0..5 {
+        h.key_press(Key::ArrowDown);
+        h.run();
+        let now = h.state().current.clone();
+        // The options bar records each applied family as the most recent one.
+        h.state_mut().recents.retain(|f| *f != now);
+        h.state_mut().recents.insert(0, now.clone());
+        h.run();
+        seen.push(now);
+    }
+    assert_eq!(seen, ["Family 00", "Family 01", "Family 02", "Family 03", "Family 04", "Family 05"], "{seen:?}");
+    h.key_press(Key::ArrowUp);
+    h.run();
+    assert_eq!(h.state().current, "Family 04", "↑ steps back");
+}
+
+/// Opened on a family that is also a recent one (the usual case: every pick records it), the
+/// arrows start from its alphabetical place, and only that row is marked.
+#[test]
+fn open_menu_on_a_recent_font_steps_through_the_alphabet() {
+    use egui_kittest::kittest::NodeT;
+    for (key, want) in [(Key::ArrowDown, "Family 06"), (Key::ArrowUp, "Family 04")] {
+        let mut h = harness_with_recents(FontPreview::Off, vec!["Family 05".into(), "Family 09".into()]);
+        h.state_mut().current = "Family 05".into();
+        h.run();
+        open(&mut h);
+        let marked = |h: &Harness<'static, State>, label: &str| {
+            h.query_all_by_label(label).filter(|n| n.accesskit_node().toggled() == Some(egui::accesskit::Toggled::True)).count()
+        };
+        assert_eq!(marked(&h, "Family 05"), 1, "one row marked");
+        h.key_press(key);
+        h.run();
+        assert_eq!(h.state().current, want);
+        h.key_press(Key::ArrowDown);
+        h.key_press(Key::ArrowUp);
+        h.run();
+        assert_eq!(h.state().current, want, "and walks on from there");
+    }
+}
+
+/// The same through the real options bar, where the pick itself records the recent family.
+#[test]
+fn options_bar_menu_arrows_walk_on_while_recording_recent_fonts() {
+    use crate::{PhotocraftApp, Services};
+    use serde_json::json;
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Services::default());
+    app.run("file.new", json!({"width": 200, "height": 100})).unwrap();
+    app.run("type.create", json!({"text": "Sample", "font": "Inter", "size": 18})).unwrap();
+    let mut h = Harness::builder().with_size(vec2(900.0, 700.0)).build_ui_state(
+        |ui, app| {
+            if ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) {
+                ui.horizontal(|ui| crate::type_tool::options_bar(app, ui));
+            }
+        },
+        app,
+    );
+    crate::PhotocraftApp::setup_context(&h.ctx, Default::default());
+    h.run_steps(4);
+    h.query_all_by_role(Role::ComboBox).next().unwrap().click();
+    h.run();
+    let mut seen = vec![h.state().ui.tool_options.type_font.clone()];
+    for _ in 0..4 {
+        h.key_press(Key::ArrowDown);
+        h.run();
+        seen.push(h.state().ui.tool_options.type_font.clone());
+    }
+    let mut once = seen.clone();
+    once.sort();
+    once.dedup();
+    assert_eq!(once.len(), seen.len(), "each ↓ reaches a new family: {seen:?}");
+    assert_eq!(h.state().ui.recent_fonts.first(), seen.last(), "the picks are recorded");
 }
 
 #[test]

crates/ui-egui/src/type_tool.rs

@@ -1468,10 +1468,16 @@ fn localized_families() -> Arc<HashMap<String, String>> {
 
 /// The font menu's rows: each family with the label it shows (its localized name when `labels`
 /// has one, else the family), matching `query` (lowercase) by either name. With localized labels
-/// the rows sort by label, so the menu stays alphabetical as shown. Recently used families are
-/// hoisted to the top, most recent first (Preferences ▸ Type ▸ Recent Fonts); a query hides the
-/// hoisting so the list stays strictly alphabetical while searching.
-fn font_menu_rows<'a>(families: &'a [String], labels: Option<&'a HashMap<String, String>>, query: &str, recents: &[String]) -> Vec<(&'a String, &'a str)> {
+/// the rows sort by label, so the menu stays alphabetical as shown. As in Photoshop, recently
+/// used families also head the menu in their own group, most recent first (Preferences ▸ Type ▸
+/// Recent Fonts), and keep their alphabetical place below it; a query shows each match once.
+/// Returns the rows and how many of them form the recent group.
+fn font_menu_rows<'a>(
+    families: &'a [String],
+    labels: Option<&'a HashMap<String, String>>,
+    query: &str,
+    recents: &[String],
+) -> (Vec<(&'a String, &'a str)>, usize) {
     let label = |f: &'a String| labels.and_then(|l| l.get(f)).map_or(f.as_str(), String::as_str);
     let mut rows: Vec<(&String, &str)> = families
         .iter()
@@ -1481,17 +1487,12 @@ fn font_menu_rows<'a>(families: &'a [String], labels: Option<&'a HashMap<String,
     if labels.is_some_and(|l| !l.is_empty()) {
         rows.sort_by_cached_key(|(_, l)| l.to_lowercase());
     }
-    if query.is_empty() {
-        let mut at = 0;
-        for r in recents {
-            if let Some(i) = rows.iter().position(|(f, _)| *f == r) {
-                let row = rows.remove(i);
-                rows.insert(at, row);
-                at += 1;
-            }
-        }
+    if !query.is_empty() {
+        return (rows, 0);
     }
-    rows
+    let recent: Vec<(&String, &str)> = recents.iter().filter_map(|r| rows.iter().find(|(f, _)| *f == r).copied()).collect();
+    let n = recent.len();
+    (recent.into_iter().chain(rows).collect(), n)
 }
 
 fn weight_name(w: f32) -> &'static str {
@@ -1701,6 +1702,8 @@ pub(crate) fn family_picker_in(
     let mut picked = false;
     let search_id = ui.id().with(("font-search", salt));
     let focus_id = search_id.with("focus");
+    let recents_id = search_id.with("recents");
+    let row_id = search_id.with("row");
     // The list opens on the press, so a press-drag-release picks a font too (#2735).
     let (press, close) = crate::press_menu::PressCombo::before(ui, salt);
     let labels: Option<Arc<HashMap<String, String>>> = ui.ctx().data(|d| d.get_temp(font_labels_id()));
@@ -1739,10 +1742,21 @@ pub(crate) fn family_picker_in(
             d.insert_temp(search_id, q.clone());
         });
         let ql = q.to_lowercase();
-        let rows = font_menu_rows(families, labels, &ql, recents);
+        // The recent group stays as it was when the menu opened: each arrow step applies (and so
+        // records) a family, and moving it to the top would send the next step back up.
+        let open_recents: Vec<String> = ui.data_mut(|d| d.get_temp_mut_or_insert_with(recents_id, || recents.to_vec()).clone());
+        let (rows, n_recent) = font_menu_rows(families, labels, &ql, &open_recents);
+        // A recent family is listed twice. The current family's row is the one the last arrow
+        // selected while it still shows that family, else its alphabetical copy, so the arrows
+        // step through the alphabet as on the closed field.
+        let mut selected = ui
+            .data(|d| d.get_temp::<usize>(row_id))
+            .filter(|&i| rows.get(i).is_some_and(|(f, _)| *f == current))
+            .or_else(|| rows.iter().skip(n_recent).position(|(f, _)| *f == current).map(|i| i + n_recent))
+            .or_else(|| rows.iter().position(|(f, _)| *f == current));
+        let mut stepped = None;
         if down || up {
-            let index = rows.iter().position(|(f, _)| *f == current);
-            let next = match index {
+            let next = match selected {
                 Some(i) if down => i.saturating_add(1).min(rows.len().saturating_sub(1)),
                 Some(i) => i.saturating_sub(1),
                 None if down => 0,
@@ -1753,6 +1767,9 @@ pub(crate) fn family_picker_in(
                 current.clone_from(f);
                 // A served family not fetched yet: start now, as a click does.
                 photocraft_text::served::request(f);
+                stepped = Some(next);
+                selected = Some(next);
+                ui.data_mut(|d| d.insert_temp(row_id, next));
             }
         }
         let accept = enter && rows.iter().any(|(f, _)| *f == current);
@@ -1768,18 +1785,22 @@ pub(crate) fn family_picker_in(
         // stays fixed at the top of the popup.
         let mut hovered: Option<String> = None;
         egui::ScrollArea::vertical().show(ui, |ui| {
-            for (f, label) in rows {
+            for (i, (f, label)) in rows.into_iter().enumerate() {
+                if i == n_recent && n_recent > 0 {
+                    ui.separator();
+                }
                 // Highlighted by its fill alone, as Photoshop's font menu is. A selectable button lays
                 // out its frame's stroke only once hovered or selected, so with the theme's stroke the
                 // row under the pointer (and the selected one) grew and pushed the rows below down:
-                // the list jumped as the pointer moved (#7406).
-                let mut button = egui::Button::selectable(f == current, label).truncate().stroke(egui::Stroke::NONE);
+                // the list jumped as the pointer moved (#7406). One row only, though a recent
+                // family is listed twice.
+                let mut button = egui::Button::selectable(selected == Some(i), label).truncate().stroke(egui::Stroke::NONE);
                 if let Some((size, [_, h])) = preview {
                     button = button
                         .right_text(egui::Atom::custom(ui.id().with(("font-sample", f)), egui::vec2(crate::font_preview::slot_width(ui.ctx(), f, size), h)));
                 }
                 let response = ui.add_sized([330.0, row_h], button);
-                if (down || up) && f == current {
+                if stepped == Some(i) {
                     response.scroll_to_me(Some(egui::Align::Center));
                 }
                 if let Some((size, _)) = preview
@@ -1816,7 +1837,11 @@ pub(crate) fn family_picker_in(
     });
     press.after(&response.response);
     if !egui::ComboBox::is_open(ui.ctx(), response.response.id) {
-        ui.data_mut(|d| d.remove::<bool>(focus_id));
+        ui.data_mut(|d| {
+            d.remove::<bool>(focus_id);
+            d.remove::<Vec<String>>(recents_id);
+            d.remove::<usize>(row_id);
+        });
     }
     let hovered_font = response.inner.unwrap_or_default();
 
@@ -1839,9 +1864,9 @@ pub(crate) fn family_picker_in(
             ui.memory_mut(|m| m.move_focus(egui::FocusDirection::None));
         }
         // The wheel's step: a negative notch is the next family. Steps use a stable order (no
-        // recents hoisting), so recording the picked family cannot make the next step jump back.
+        // recent group), so recording the picked family cannot make the next step jump back.
         let notches = f32::from(u8::from(up)) - f32::from(u8::from(down));
-        let rows = font_menu_rows(families, labels, "", &[]);
+        let (rows, _) = font_menu_rows(families, labels, "", &[]);
         if let Some(i) = rows.iter().position(|(f, _)| *f == current)
             && let Some(n) = wheel_family_index(i, rows.len(), notches)
             && let Some((f, _)) = rows.get(n)
@@ -1861,10 +1886,10 @@ pub(crate) fn family_picker_in(
             ui.input_mut(|i| i.smooth_scroll_delta = egui::Vec2::ZERO);
         }
         let notches = crate::widgets::wheel_notches_of(ui, response.response.id.with("family-wheel"), hovered, 1.0);
-        // The wheel steps in a stable order: hoisting the family it just picked as recent
+        // The wheel steps in a stable order: a recent group holding the family it just picked
         // would change the order between notches and can jump back to the previous family.
         // The open menu still shows the recent families first.
-        let rows = font_menu_rows(families, labels, "", &[]);
+        let (rows, _) = font_menu_rows(families, labels, "", &[]);
         if let Some(i) = rows.iter().position(|(f, _)| *f == current)
             && let Some(n) = wheel_family_index(i, rows.len(), notches)
             && let Some((f, _)) = rows.get(n)