Merge pull request: land #583, #584 (fix #577)

Land #583, #584 (fix #577) with a ligature-threshold follow-up

crates/text/src/features.rs

@@ -3,13 +3,27 @@
 use harfrust::{Feature, Tag};
 use vectorcraft_doc::CharStyle;
 
+/// Tracking (in 1/1000 em) outside which standard ligatures are dropped automatically: tighter
+/// than the first, looser than the second.
+///
+/// Letterspaced type should not ligate: an `fi` glyph keeps its letters tight while everything
+/// around it opens up (or stays open while it closes up), which reads as a blot. Small tracking
+/// adjustments made to fit a line are not letterspacing and keep the ligatures. The limits are
+/// about −8% and +22% of a typical word space (250/1000 em), the thresholds Adobe's composers are
+/// described as using (John Hudson, www-style, 2012-07): condensing shows the fixed ligature
+/// sooner than expanding. An explicit `liga` / `clig` in the character's own features always wins.
+pub const LIGATURE_TRACKING_LIMITS: (f64, f64) = (-20.0, 55.0);
+
 /// OpenType features applied during shaping. Kerning, `case` and ligature suppression are driven
 /// by the character style (`kerning`, `all_caps`, `tracking`); the rest are layout-wide options.
+/// Standard ligatures are dropped when tracking is outside [`LIGATURE_TRACKING_LIMITS`], unless
+/// the character's features turn `liga` (or `clig`) on explicitly.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct OtFeatures {
     /// Vertical glyph alternates (`vert`), set by the writing direction.
     pub vertical: bool,
-    /// Standard ligatures (`liga`, `clig`). Suppressed automatically when tracking is non-zero.
+    /// Standard ligatures (`liga`, `clig`). Suppressed automatically when tracking is outside
+    /// [`LIGATURE_TRACKING_LIMITS`], unless the character turns them on explicitly.
     pub ligatures: bool,
     /// Contextual alternates (`calt`).
     pub contextual: bool,
@@ -117,7 +131,7 @@ impl OtFeatures {
         if st.kerning.is_some() {
             v.push(f(b"kern", false));
         }
-        let liga = s.ligatures && st.tracking.abs() < 1e-9;
+        let liga = s.ligatures && (!ligatures_suppressed_by(st.tracking) || explicit_ligatures(&st.features));
         if !liga {
             v.push(f(b"liga", false));
             v.push(f(b"clig", false));
@@ -150,6 +164,28 @@ impl OtFeatures {
     }
 }
 
+/// Does `tracking` (1/1000 em) count as letterspacing that drops standard ligatures?
+pub fn ligatures_suppressed_by(tracking: f64) -> bool {
+    // A non-finite value is not a real letterspacing request; keep the default.
+    let (tight, loose) = LIGATURE_TRACKING_LIMITS;
+    tracking.is_finite() && (tracking < tight || tracking > loose)
+}
+
+/// Do `features` turn standard ligatures on explicitly (`liga`, `+liga`, `clig`, `+clig`)? The last
+/// ligature tag wins, matching [`OtFeatures::with_tags`]. Absence of a tag means "default", which
+/// [`OtFeatures::to_tags`] never writes, so this is distinguishable from an explicit request.
+pub fn explicit_ligatures<S: AsRef<str>>(features: &[S]) -> bool {
+    features
+        .iter()
+        .rev()
+        .find_map(|t| match t.as_ref() {
+            "liga" | "+liga" | "clig" | "+clig" => Some(true),
+            "-liga" | "-clig" => Some(false),
+            _ => None,
+        })
+        .unwrap_or(false)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/text/src/lib.rs

@@ -23,7 +23,7 @@ pub mod test_fonts;
 pub mod thread;
 
 pub use craft_fonts::{CRAFT_FONTS, CraftFont};
-pub use features::OtFeatures;
+pub use features::{LIGATURE_TRACKING_LIMITS, OtFeatures, explicit_ligatures, ligatures_suppressed_by};
 pub use fontdb::{FALLBACK_FAMILY, FontClass, FontDb, FontFace, FontMatch, FontTraits, style_weight, system_font_dirs};
 use kurbo::{Affine, BezPath, Point, Rect, Vec2};
 pub use layout::{layout, layout_with};

crates/text/src/tests_typo.rs

@@ -370,9 +370,24 @@ fn opentype_ligature_switches() {
     assert_eq!(layout(db(), &t).glyphs.len(), 1, "fi ligature by default");
     let off = LayoutOptions { features: OtFeatures { ligatures: false, ..Default::default() }, ..Default::default() };
     assert_eq!(layout_with(db(), &t, &off).glyphs.len(), 2);
-    // Tracking suppresses ligatures (as letterspaced type should).
-    let tracked = TextObject::point(Point::ZERO, "fi", CharStyle { tracking: 100.0, ..serif });
-    assert_eq!(layout(db(), &tracked).glyphs.len(), 2);
+    let fi = |tracking: f64, features: &[&str]| {
+        let st = CharStyle { tracking, features: features.iter().map(|s| s.to_string()).collect(), ..serif.clone() };
+        layout(db(), &TextObject::point(Point::ZERO, "fi", st)).glyphs.len()
+    };
+    // Small tracking adjustments (fitting a line) keep the ligature.
+    assert_eq!(fi(-5.0, &[]), 1, "tracking -5 keeps fi");
+    assert_eq!(fi(20.0, &[]), 1, "tracking +20 keeps fi");
+    let (tight, loose) = LIGATURE_TRACKING_LIMITS;
+    assert_eq!((fi(tight, &[]), fi(loose, &[])), (1, 1), "the limits themselves keep fi");
+    // Real letterspacing suppresses ligatures (as letterspaced type should), condensing sooner...
+    assert_eq!(fi(100.0, &[]), 2, "tracking 100 drops fi");
+    assert_eq!(fi(-30.0, &[]), 2, "tracking -30 drops fi");
+    assert_eq!(fi(-100.0, &[]), 2, "tracking -100 drops fi");
+    // ...unless the character turns them on explicitly.
+    assert_eq!(fi(100.0, &["liga"]), 1, "explicit liga wins over tracking");
+    assert_eq!(fi(100.0, &["+liga"]), 1);
+    assert_eq!(fi(100.0, &["liga", "-liga"]), 2, "the last tag wins");
+    assert_eq!(fi(0.0, &["-liga"]), 2, "explicit off still turns it off");
     let f = OtFeatures::from_tags(["dlig", "-liga", "smcp", "onum", "bogus"]);
     assert!(f.discretionary_ligatures && !f.ligatures && f.small_caps && f.oldstyle_figures && !f.fractions);
 }

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

@@ -29,6 +29,9 @@ use crate::widgets::{self, Live};
 
 pub(super) const SPEC: DialogSpec = DialogSpec::window(show, confirm);
 
+/// The field holding the frame the popover was first drawn in.
+const OPENED: &str = "__opened";
+
 /// What the popover edits: the selected stop of the gradient behind the active proxy (with all
 /// its stops), or the selected point of a freeform gradient.
 enum Target {
@@ -131,12 +134,18 @@ fn show(app: &mut VectorcraftApp, ctx: &egui::Context) {
             });
         })
         .response;
-    if let (Some(tab), Some(d)) = (tab, app.ui.dialog.as_mut()) {
-        d.fields.insert("tab".into(), json!(tab));
+    // The frame it opened in (its first drawn).
+    let frame = ctx.cumulative_frame_nr();
+    let opened = d.fields.get(OPENED).and_then(Value::as_u64).unwrap_or(frame);
+    if let Some(d) = app.ui.dialog.as_mut() {
+        if let Some(tab) = tab {
+            d.fields.insert("tab".into(), json!(tab));
+        }
+        d.fields.insert(OPENED.into(), json!(opened));
     }
-    // The double-click that opened the popover lands on its chip, outside it: only a later click
-    // elsewhere closes it.
-    if resp.clicked_elsewhere() && !ctx.input(|i| i.pointer.button_double_clicked(egui::PointerButton::Primary)) {
+    // The click that opened the popover (the Gradient panel's colour chip) or the double-click
+    // (a stop's chip) lands outside it: only a later click elsewhere closes it (#577).
+    if frame > opened && resp.clicked_elsewhere() && !ctx.input(|i| i.pointer.button_double_clicked(egui::PointerButton::Primary)) {
         app.ui.dialog = None;
     }
 }
@@ -246,10 +255,18 @@ mod tests {
         }
     }
 
+    thread_local! {
+        // One context per test, so frames follow one another as in the app.
+        static CTX: egui::Context = {
+            let ctx = egui::Context::default();
+            crate::theme::install_fonts(&ctx);
+            ctx
+        };
+    }
+
     /// One headless frame of the dialog layer and the shortcuts.
     fn frame(app: &mut VectorcraftApp, events: Vec<egui::Event>) {
-        let ctx = egui::Context::default();
-        crate::theme::install_fonts(&ctx);
+        let ctx = CTX.with(Clone::clone);
         let mut out = ctx.run_ui(egui::RawInput { events, ..Default::default() }, |ui| {
             crate::shortcuts::handle(app, ui.ctx());
             super::super::show(app, ui.ctx());
@@ -312,6 +329,25 @@ mod tests {
         assert!(app.ui.dialog.is_none(), "a click elsewhere closes it");
     }
 
+    /// #577: the Gradient panel's colour chip opens it with a click whose release lands in the
+    /// frame it opens in, outside it: that click doesn't close it.
+    #[test]
+    fn the_click_that_opens_it_from_the_panel_keeps_it_open() {
+        let mut app = app();
+        let at = egui::pos2(5.0, 5.0);
+        frame(&mut app, vec![egui::Event::PointerMoved(at), click(at, true)]);
+        // The panel opens it while the click is released.
+        app.ui.dialog = Some(Dialog::new("gradientStop", json!({"index": 1, "screen": [200.0, 200.0], "tab": "color"})));
+        frame(&mut app, vec![click(at, false)]);
+        for _ in 0..3 {
+            frame(&mut app, vec![]);
+        }
+        assert!(app.ui.dialog.is_some(), "the opening click must not close it");
+        let away = egui::pos2(5.0, 600.0);
+        frame(&mut app, vec![egui::Event::PointerMoved(away), click(away, true), click(away, false)]);
+        assert!(app.ui.dialog.is_none(), "a later click elsewhere closes it");
+    }
+
     #[test]
     fn the_popover_closes_when_its_stop_goes_away() {
         let mut app = app();

crates/ui-egui/src/panels/opentype.rs

@@ -63,7 +63,7 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) {
             *field(&mut next) = !on;
         }
     }
-    if st.tracking.abs() > 1e-9 && cur.ligatures {
+    if cur.ligatures && vectorcraft_text::ligatures_suppressed_by(st.tracking) && !vectorcraft_text::explicit_ligatures(&st.features) {
         widgets::dim_label(ui, tl!("Ligatures are off while the text is tracked."));
     }
     if next != cur {