Refactor and fix bounds metric (#5186)

Author: laurmaedje

crates/typst/src/layout/inline/shaping.rs

@@ -16,7 +16,7 @@ use crate::foundations::{Smart, StyleChain};
 use crate::layout::{Abs, Dir, Em, Frame, FrameItem, Point, Size};
 use crate::text::{
     decorate, families, features, is_default_ignorable, variant, Font, FontVariant,
-    Glyph, Lang, Region, TextElem, TextItem,
+    Glyph, Lang, Region, TextEdgeBounds, TextElem, TextItem,
 };
 use crate::utils::SliceExt;
 use crate::World;
@@ -338,9 +338,10 @@ impl<'a> ShapedText<'a> {
         let bottom_edge = TextElem::bottom_edge_in(self.styles);
 
         // Expand top and bottom by reading the font's vertical metrics.
-        let mut expand = |font: &Font, bbox: Option<ttf_parser::Rect>| {
-            top.set_max(top_edge.resolve(self.size, font, bbox));
-            bottom.set_max(-bottom_edge.resolve(self.size, font, bbox));
+        let mut expand = |font: &Font, bounds: TextEdgeBounds| {
+            let (t, b) = font.edges(top_edge, bottom_edge, self.size, bounds);
+            top.set_max(t);
+            bottom.set_max(b);
         };
 
         if self.glyphs.is_empty() {
@@ -353,18 +354,13 @@ impl<'a> ShapedText<'a> {
                     .select(family, self.variant)
                     .and_then(|id| world.font(id))
                 {
-                    expand(&font, None);
+                    expand(&font, TextEdgeBounds::Zero);
                     break;
                 }
             }
         } else {
             for g in self.glyphs.iter() {
-                let bbox = if top_edge.is_bounds() || bottom_edge.is_bounds() {
-                    g.font.ttf().glyph_bounding_box(ttf_parser::GlyphId(g.glyph_id))
-                } else {
-                    None
-                };
-                expand(&g.font, bbox);
+                expand(&g.font, TextEdgeBounds::Glyph(g.glyph_id));
             }
         }
 

crates/typst/src/math/equation.rs

@@ -20,7 +20,8 @@ use crate::math::{
 use crate::model::{Numbering, Outlinable, ParElem, ParLine, Refable, Supplement};
 use crate::syntax::Span;
 use crate::text::{
-    families, variant, Font, FontFamily, FontList, FontWeight, LocalName, TextElem,
+    families, variant, Font, FontFamily, FontList, FontWeight, LocalName, TextEdgeBounds,
+    TextElem,
 };
 use crate::utils::{NonZeroExt, Numeric};
 use crate::World;
@@ -290,12 +291,16 @@ fn layout_equation_inline(
 
         let font_size = scaled_font_size(&ctx, styles);
         let slack = ParElem::leading_in(styles) * 0.7;
-        let top_edge = TextElem::top_edge_in(styles).resolve(font_size, &font, None);
-        let bottom_edge =
-            -TextElem::bottom_edge_in(styles).resolve(font_size, &font, None);
 
-        let ascent = top_edge.max(frame.ascent() - slack);
-        let descent = bottom_edge.max(frame.descent() - slack);
+        let (t, b) = font.edges(
+            TextElem::top_edge_in(styles),
+            TextElem::bottom_edge_in(styles),
+            font_size,
+            TextEdgeBounds::Frame(frame),
+        );
+
+        let ascent = t.max(frame.ascent() - slack);
+        let descent = b.max(frame.descent() - slack);
         frame.translate(Point::with_y(ascent - frame.baseline()));
         frame.size_mut().y = ascent + descent;
     }

crates/typst/src/text/deco.rs

@@ -10,7 +10,8 @@ use crate::layout::{
 };
 use crate::syntax::Span;
 use crate::text::{
-    BottomEdge, BottomEdgeMetric, TextElem, TextItem, TopEdge, TopEdgeMetric,
+    BottomEdge, BottomEdgeMetric, TextEdgeBounds, TextElem, TextItem, TopEdge,
+    TopEdgeMetric,
 };
 use crate::visualize::{styled_rect, Color, FixedStroke, Geometry, Paint, Stroke};
 
@@ -422,7 +423,7 @@ pub(crate) fn decorate(
         &deco.line
     {
         let (top, bottom) = determine_edges(text, *top_edge, *bottom_edge);
-        let size = Size::new(width + 2.0 * deco.extent, top - bottom);
+        let size = Size::new(width + 2.0 * deco.extent, top + bottom);
         let rects = styled_rect(size, radius, fill.clone(), stroke);
         let origin = Point::new(pos.x - deco.extent, pos.y - top - shift);
         frame.prepend_multiple(
@@ -540,22 +541,20 @@ fn determine_edges(
     top_edge: TopEdge,
     bottom_edge: BottomEdge,
 ) -> (Abs, Abs) {
-    let mut bbox = None;
-    if top_edge.is_bounds() || bottom_edge.is_bounds() {
-        let ttf = text.font.ttf();
-        bbox = text
-            .glyphs
-            .iter()
-            .filter_map(|g| ttf.glyph_bounding_box(ttf_parser::GlyphId(g.id)))
-            .reduce(|a, b| ttf_parser::Rect {
-                y_max: a.y_max.max(b.y_max),
-                y_min: a.y_min.min(b.y_min),
-                ..a
-            });
+    let mut top = Abs::zero();
+    let mut bottom = Abs::zero();
+
+    for g in text.glyphs.iter() {
+        let (t, b) = text.font.edges(
+            top_edge,
+            bottom_edge,
+            text.size,
+            TextEdgeBounds::Glyph(g.id),
+        );
+        top.set_max(t);
+        bottom.set_max(b);
     }
 
-    let top = top_edge.resolve(text.size, &text.font, bbox);
-    let bottom = bottom_edge.resolve(text.size, &text.font, bbox);
     (top, bottom)
 }
 

crates/typst/src/text/font/mod.rs

@@ -9,6 +9,7 @@ mod variant;
 pub use self::book::{Coverage, FontBook, FontFlags, FontInfo};
 pub use self::variant::{FontStretch, FontStyle, FontVariant, FontWeight};
 
+use std::cell::OnceCell;
 use std::fmt::{self, Debug, Formatter};
 use std::hash::{Hash, Hasher};
 use std::sync::Arc;
@@ -17,7 +18,8 @@ use ttf_parser::GlyphId;
 
 use self::book::find_name;
 use crate::foundations::{Bytes, Cast};
-use crate::layout::Em;
+use crate::layout::{Abs, Em, Frame};
+use crate::text::{BottomEdge, TopEdge};
 
 /// An OpenType font.
 ///
@@ -125,6 +127,48 @@ impl Font {
         // internal 'static lifetime.
         &self.0.rusty
     }
+
+    /// Resolve the top and bottom edges of text.
+    pub fn edges(
+        &self,
+        top_edge: TopEdge,
+        bottom_edge: BottomEdge,
+        font_size: Abs,
+        bounds: TextEdgeBounds,
+    ) -> (Abs, Abs) {
+        let cell = OnceCell::new();
+        let bbox = |gid, f: fn(ttf_parser::Rect) -> i16| {
+            cell.get_or_init(|| self.ttf().glyph_bounding_box(GlyphId(gid)))
+                .map(|bbox| self.to_em(f(bbox)).at(font_size))
+                .unwrap_or_default()
+        };
+
+        let top = match top_edge {
+            TopEdge::Metric(metric) => match metric.try_into() {
+                Ok(metric) => self.metrics().vertical(metric).at(font_size),
+                Err(_) => match bounds {
+                    TextEdgeBounds::Zero => Abs::zero(),
+                    TextEdgeBounds::Frame(frame) => frame.ascent(),
+                    TextEdgeBounds::Glyph(gid) => bbox(gid, |b| b.y_max),
+                },
+            },
+            TopEdge::Length(length) => length.at(font_size),
+        };
+
+        let bottom = match bottom_edge {
+            BottomEdge::Metric(metric) => match metric.try_into() {
+                Ok(metric) => -self.metrics().vertical(metric).at(font_size),
+                Err(_) => match bounds {
+                    TextEdgeBounds::Zero => Abs::zero(),
+                    TextEdgeBounds::Frame(frame) => frame.descent(),
+                    TextEdgeBounds::Glyph(gid) => -bbox(gid, |b| b.y_min),
+                },
+            },
+            BottomEdge::Length(length) => -length.at(font_size),
+        };
+
+        (top, bottom)
+    }
 }
 
 impl Hash for Font {
@@ -249,3 +293,14 @@ pub enum VerticalFontMetric {
     /// The font's ascender, which typically exceeds the depth of all glyphs.
     Descender,
 }
+
+/// Defines how to resolve a `Bounds` text edge.
+#[derive(Debug, Copy, Clone)]
+pub enum TextEdgeBounds<'a> {
+    /// Set the bounds to zero.
+    Zero,
+    /// Use the bounding box of the given glyph for the bounds.
+    Glyph(u16),
+    /// Use the dimension of the given frame for the bounds.
+    Frame(&'a Frame),
+}

crates/typst/src/text/mod.rs

@@ -37,7 +37,7 @@ use icu_provider_blob::BlobDataProvider;
 use once_cell::sync::Lazy;
 use rustybuzz::Feature;
 use smallvec::SmallVec;
-use ttf_parser::{Rect, Tag};
+use ttf_parser::Tag;
 
 use crate::diag::{bail, warning, HintedStrResult, SourceResult};
 use crate::engine::Engine;
@@ -891,28 +891,6 @@ pub enum TopEdge {
     Length(Length),
 }
 
-impl TopEdge {
-    /// Determine if the edge is specified from bounding box info.
-    pub fn is_bounds(&self) -> bool {
-        matches!(self, Self::Metric(TopEdgeMetric::Bounds))
-    }
-
-    /// Resolve the value of the text edge given a font's metrics.
-    pub fn resolve(self, font_size: Abs, font: &Font, bbox: Option<Rect>) -> Abs {
-        match self {
-            TopEdge::Metric(metric) => {
-                if let Ok(metric) = metric.try_into() {
-                    font.metrics().vertical(metric).at(font_size)
-                } else {
-                    bbox.map(|bbox| (font.to_em(bbox.y_max)).at(font_size))
-                        .unwrap_or_default()
-                }
-            }
-            TopEdge::Length(length) => length.at(font_size),
-        }
-    }
-}
-
 cast! {
     TopEdge,
     self => match self {
@@ -961,28 +939,6 @@ pub enum BottomEdge {
     Length(Length),
 }
 
-impl BottomEdge {
-    /// Determine if the edge is specified from bounding box info.
-    pub fn is_bounds(&self) -> bool {
-        matches!(self, Self::Metric(BottomEdgeMetric::Bounds))
-    }
-
-    /// Resolve the value of the text edge given a font's metrics.
-    pub fn resolve(self, font_size: Abs, font: &Font, bbox: Option<Rect>) -> Abs {
-        match self {
-            BottomEdge::Metric(metric) => {
-                if let Ok(metric) = metric.try_into() {
-                    font.metrics().vertical(metric).at(font_size)
-                } else {
-                    bbox.map(|bbox| (font.to_em(bbox.y_min)).at(font_size))
-                        .unwrap_or_default()
-                }
-            }
-            BottomEdge::Length(length) => length.at(font_size),
-        }
-    }
-}
-
 cast! {
     BottomEdge,
     self => match self {

tests/suite/layout/measure.typ

@@ -92,3 +92,10 @@
   table(columns: 5, u(17), it, u(1), it, u(5))
   [#size.width] // 17pt
 }
+
+--- issue-5180-measure-inline-math-bounds ---
+#context {
+  let height = measure(text(top-edge: "bounds", $x$)).height
+  assert(height > 4pt)
+  assert(height < 5pt)
+}