Merge pull request gosub-io#1078 from jaytaph/image-fix

Fixes the hackernews logo display

Author: jaytaph

.cargo/config.toml

@@ -1,2 +1,15 @@
 [registries]
 gosub = { index = "sparse+https://registry.gosub.io/" }
+
+# Faster linker for the large binaries (gosub-screenshot, the example browsers).
+# mold is typically several times faster than the default bfd/gold linker on the
+# big link steps. Install it first:  apt install mold   (or  dnf/pacman install mold)
+# then uncomment the block below. Verify with:  mold --version
+#
+# [target.x86_64-unknown-linux-gnu]
+# linker = "clang"
+# rustflags = ["-C", "link-arg=-fuse-ld=mold"]
+#
+# If you don't have clang, use rustc's bundled cc driver instead:
+# [target.x86_64-unknown-linux-gnu]
+# rustflags = ["-C", "link-arg=-fuse-ld=mold"]

Cargo.toml

@@ -217,6 +217,15 @@ gosub_render_pipeline = { path = "./crates/gosub_render_pipeline", features = ["
 vello = { git = "https://github.com/linebender/vello", rev = "1b24e77ba4e01d12f5db86a9b1b5f2cf828a6a33" }
 vello_encoding = { git = "https://github.com/linebender/vello", rev = "1b24e77ba4e01d12f5db86a9b1b5f2cf828a6a33" }
 
+[profile.dev]
+# Keep the linker from copying the (very large) DWARF into every binary on each
+# link — debug info stays in the .o files instead. Combined with line-tables-only
+# this cuts re-link time of the big binaries (gosub-screenshot, the example
+# browsers) from tens of seconds to a few, and shrinks them ~5x. Backtraces still
+# have file/line; only local-variable inspection is dropped.
+split-debuginfo = "unpacked"
+debug = "line-tables-only"
+
 [profile.release]
 lto = "fat"
 opt-level = 3

crates/gosub_engine/src/engine/context.rs

@@ -40,14 +40,17 @@ pub fn compute_column_widths<T: TableTree>(
             for cell in grid.cells_in_row(row_idx) {
                 found_any = true;
                 if cell.colspan == 1 {
+                    let cw = tree.cell_content_width(cell.node);
                     if explicit[cell.col].is_none() {
+                        // A specified width cannot shrink a cell below its content's min-width
+                        // (CSS: used width = max(specified, min-content)). Without this, e.g. a
+                        // `width:18px` cell holding a 20px image clips it and eats the padding.
                         match tree.css_length(cell.node, CssProp::Width) {
-                            CssLength::Px(px) => explicit[cell.col] = Some(px),
-                            CssLength::Percent(p) => explicit[cell.col] = Some(p / 100.0 * table_width),
+                            CssLength::Px(px) => explicit[cell.col] = Some(px.max(cw)),
+                            CssLength::Percent(p) => explicit[cell.col] = Some((p / 100.0 * table_width).max(cw)),
                             _ => {}
                         }
                     }
-                    let cw = tree.cell_content_width(cell.node);
                     if cw > natural[cell.col] {
                         natural[cell.col] = cw;
                     }

crates/gosub_lattice/src/sizing/columns.rs

@@ -317,14 +317,20 @@ fn detect_content_type(content_type: &str) -> Option<MediaType> {
 }
 
 fn ft_detect(ft: &FileType) -> Option<MediaType> {
+    // Scan *all* media types before deciding. The `file_type` crate lists several aliases
+    // for SVG (e.g. `["image/SVG", "image/svg+xml"]`); a naive first-match-wins loop trips
+    // the generic `image/` branch on the non-standard `image/SVG` alias and misclassifies
+    // SVG as a raster image. So look for any SVG marker first, and only fall back to a
+    // generic raster image if none is found. Comparisons are case-insensitive.
+    let mut is_raster_image = false;
     for &mt in ft.media_types().iter() {
-        if mt == "image/svg+xml" {
+        if mt.eq_ignore_ascii_case("image/svg+xml") || mt.eq_ignore_ascii_case("image/svg") {
             return Some(MediaType::Svg);
         }
-        if mt.starts_with("image/") {
-            return Some(MediaType::Image);
+        if mt.len() >= 6 && mt[..6].eq_ignore_ascii_case("image/") {
+            is_raster_image = true;
         }
     }
 
-    None
+    is_raster_image.then_some(MediaType::Image)
 }

crates/gosub_render_pipeline/src/common/media/media_store.rs

@@ -35,6 +35,33 @@ impl<'a> PipelineTableTree<'a> {
         }
     }
 
+    /// Sum of the border-box heights of the nested tables directly contained in a cell (not
+    /// counting tables nested deeper inside those). Zero if the cell holds no table. This lets
+    /// a table cell grow to contain a nested table whose height lattice computes in a later pass.
+    fn nested_table_height(&self, cell_layout_id: LayoutElementId) -> f32 {
+        let Some(el) = self.layout_tree.arena.get(&cell_layout_id) else {
+            return 0.0;
+        };
+        let mut total = 0.0;
+        for &child_id in &el.children {
+            let Some(child) = self.layout_tree.arena.get(&child_id) else {
+                continue;
+            };
+            let is_table = matches!(
+                self.doc.get_own_style(child.dom_node_id, &StyleProperty::Display),
+                Some(Value::Display(Display::Table))
+            );
+            if is_table {
+                // Self-contained nested table — stop here, don't double-count its inner tables.
+                total += child.box_model.border_box.height as f32;
+            } else {
+                // The table may be wrapped (e.g. in an anonymous box); keep descending.
+                total += self.nested_table_height(child_id);
+            }
+        }
+        total
+    }
+
     /// Convert pending relative positions to absolute `BoxModel`s in the arena.
     /// Must be called after `compute_table_layout` returns.
     pub fn apply_positions(&mut self, table_dom_id: DomNodeId) {
@@ -153,8 +180,10 @@ fn cell_layout_to_box_model(layout: &CellLayout, abs: Coordinate) -> BoxModel {
 fn intrinsic_content_width(el: &LayoutElementNode, arena: &HashMap<LayoutElementId, LayoutElementNode>) -> f32 {
     match &el.context {
         ElementContext::Text(_) => el.box_model.content_box.width as f32,
-        ElementContext::Image(ctx) => ctx.dimension.width as f32,
-        ElementContext::Svg(ctx) => ctx.dimension.width as f32,
+        // Replaced elements: use the laid-out border-box width so the column is wide enough for
+        // the image *including its own CSS border* (the bare `dimension` omits it). Images are
+        // never stretched to the cell width, so the border box is the true intrinsic width.
+        ElementContext::Image(_) | ElementContext::Svg(_) => el.box_model.border_box.width as f32,
         ElementContext::None => el
             .children
             .iter()
@@ -239,7 +268,11 @@ impl TableTree for PipelineTableTree<'_> {
         // than 0 and covers the most common case (single column of text).
         if let Some(&layout_id) = self.dom_to_layout.get(&id) {
             if let Some(element) = self.layout_tree.arena.get(&layout_id) {
-                return element.box_model.content_box.height as f32;
+                let taffy_h = element.box_model.content_box.height as f32;
+                // A cell containing a nested table must be at least as tall as that table.
+                // The nested table's real height is only known after lattice lays it out, so
+                // the second (bottom-up) pass in `post_process_tables` propagates it up here.
+                return taffy_h.max(self.nested_table_height(layout_id));
             }
         }
         0.0
@@ -248,7 +281,10 @@ impl TableTree for PipelineTableTree<'_> {
     fn cell_content_width(&self, id: DomNodeId) -> f32 {
         if let Some(&layout_id) = self.dom_to_layout.get(&id) {
             if let Some(element) = self.layout_tree.arena.get(&layout_id) {
-                return intrinsic_content_width(element, &self.layout_tree.arena);
+                // Include the cell's own horizontal padding so the column is wide enough to hold
+                // the content *and* its padding (e.g. HN's logo cell: 20px image + 4px padding-right).
+                let pad = (element.box_model.padding.left + element.box_model.padding.right) as f32;
+                return intrinsic_content_width(element, &self.layout_tree.arena) + pad;
             }
         }
         0.0
@@ -273,59 +309,69 @@ pub fn post_process_tables(layout_tree: &mut LayoutTree, dom_to_layout: &HashMap
 
     log::info!("lattice: post_process_tables found {} table node(s)", table_nodes.len());
 
-    for (table_dom_id, table_layout_id) in table_nodes {
-        // Use the parent element's content width as available_width. For nested
-        // tables the parent is a table cell whose box model was already updated
-        // by the outer table's apply_positions call, giving us the correct width.
-        // Fall back to the table's own Taffy-computed width for root-level tables.
-        let available_width = doc
-            .parent(table_dom_id)
-            .and_then(|p| dom_to_layout.get(&p))
-            .and_then(|&pid| layout_tree.arena.get(&pid))
-            .map(|el| el.box_model.content_box.width as f32)
-            .unwrap_or_else(|| {
-                layout_tree
-                    .arena
-                    .get(&table_layout_id)
-                    .map(|e| e.box_model.content_box.width as f32)
-                    .unwrap_or(0.0)
-            });
+    // Two passes. Pass 1 is pre-order (outer→inner): it establishes column widths, which flow
+    // top-down (a nested table reads its width from its already-sized parent cell). Pass 2 is
+    // post-order (inner→outer): each table is re-laid-out *after* the tables nested inside its
+    // cells, so an outer cell's height now reflects its nested table's true height — height
+    // flows bottom-up. A single reverse pass propagates through any table-nesting depth.
+    for pass in 0..2 {
+        let order: Vec<(DomNodeId, LayoutElementId)> = if pass == 0 {
+            table_nodes.clone()
+        } else {
+            table_nodes.iter().rev().copied().collect()
+        };
+        for (table_dom_id, table_layout_id) in order {
+            lay_out_one_table(&*doc, layout_tree, dom_to_layout, table_dom_id, table_layout_id);
+        }
+    }
+}
 
-        log::info!(
-            "lattice: computing layout for table node {:?}, available_width={}",
-            table_dom_id,
-            available_width
-        );
+/// Run lattice for a single table node and write the computed cell positions and the table's
+/// own size back into the layout tree.
+fn lay_out_one_table(
+    doc: &dyn PipelineDocument,
+    layout_tree: &mut LayoutTree,
+    dom_to_layout: &HashMap<DomNodeId, LayoutElementId>,
+    table_dom_id: DomNodeId,
+    table_layout_id: LayoutElementId,
+) {
+    // Use the parent element's content width as available_width. For nested
+    // tables the parent is a table cell whose box model was already updated
+    // by the outer table's apply_positions call, giving us the correct width.
+    // Fall back to the table's own Taffy-computed width for root-level tables.
+    let available_width = doc
+        .parent(table_dom_id)
+        .and_then(|p| dom_to_layout.get(&p))
+        .and_then(|&pid| layout_tree.arena.get(&pid))
+        .map(|el| el.box_model.content_box.width as f32)
+        .unwrap_or_else(|| {
+            layout_tree
+                .arena
+                .get(&table_layout_id)
+                .map(|e| e.box_model.content_box.width as f32)
+                .unwrap_or(0.0)
+        });
 
-        // &*doc borrows from our local Arc, not from layout_tree — no conflict.
-        let mut tree = PipelineTableTree::new(&*doc, layout_tree, dom_to_layout);
+    let mut tree = PipelineTableTree::new(doc, layout_tree, dom_to_layout);
 
-        match gosub_lattice::compute_table_layout(&mut tree, table_dom_id, available_width, None) {
-            Ok((table_width, table_height)) => {
-                log::info!(
-                    "lattice: table node {:?} → {}×{}px, pending={}",
-                    table_dom_id,
-                    table_width,
-                    table_height,
-                    tree.pending.len()
+    match gosub_lattice::compute_table_layout(&mut tree, table_dom_id, available_width, None) {
+        Ok((table_width, table_height)) => {
+            tree.apply_positions(table_dom_id);
+            // Write back both dimensions so deeply-nested tables can read the
+            // correct width from this table's box model via their parent lookup.
+            if let Some(el) = layout_tree.arena.get_mut(&table_layout_id) {
+                let bb = el.box_model.border_box;
+                el.box_model = BoxModel::new(
+                    Rect::new(bb.x, bb.y, table_width as f64, table_height as f64),
+                    el.box_model.padding,
+                    el.box_model.border,
+                    el.box_model.margin,
                 );
-                tree.apply_positions(table_dom_id);
-                // Write back both dimensions so deeply-nested tables can read the
-                // correct width from this table's box model via their parent lookup.
-                if let Some(el) = layout_tree.arena.get_mut(&table_layout_id) {
-                    let bb = el.box_model.border_box;
-                    el.box_model = BoxModel::new(
-                        Rect::new(bb.x, bb.y, table_width as f64, table_height as f64),
-                        el.box_model.padding,
-                        el.box_model.border,
-                        el.box_model.margin,
-                    );
-                }
-            }
-            Err(e) => {
-                log::warn!("lattice: table layout failed for node {:?}: {:?}", table_dom_id, e);
             }
         }
+        Err(e) => {
+            log::warn!("lattice: table layout failed for node {:?}: {:?}", table_dom_id, e);
+        }
     }
 }
 

crates/gosub_render_pipeline/src/layouter/table.rs

@@ -43,8 +43,9 @@ pub struct TaffyLayouter {
     /// populate_boxmodel uses this to add the container's taffy-computed offset to the offset
     /// it passes down to the child, which would otherwise be missing from the calculation.
     anon_container_map: HashMap<LayoutElementId, TaffyNodeId>,
-    /// Media store for loading images/SVGs during layout
-    media_store: MediaStore,
+    /// Media store for loading images/SVGs during layout. Shared (Arc) so the media loaded
+    /// here is visible to the rasterization stage, which looks resources up by the same id.
+    media_store: Arc<MediaStore>,
     /// Shared font system used for text measurement. Locking once per measurement
     /// call avoids keeping the guard alive across the full layout pass, which lets
     /// other threads (e.g. the rasterizer) access the font collection between calls.
@@ -93,12 +94,12 @@ impl TaffyContext {
         })
     }
 
-    fn svg(src: &str, media_id: MediaId, node_id: DomNodeId) -> TaffyContext {
+    fn svg(src: &str, media_id: MediaId, dimension: geo::Dimension, node_id: DomNodeId) -> TaffyContext {
         TaffyContext::Svg(ElementContextSvg {
             node_id,
             src: src.to_string(),
             media_id,
-            dimension: geo::Dimension::ZERO,
+            dimension,
         })
     }
 }
@@ -125,7 +126,7 @@ impl TaffyLayouter {
             root_id: TaffyNodeId::new(0),
             layout_taffy_mapping: HashMap::new(),
             anon_container_map: HashMap::new(),
-            media_store: MediaStore::new(),
+            media_store: Arc::new(MediaStore::new()),
             font_system,
             measure_cache: HashMap::new(),
             dom_to_layout_mapping: HashMap::new(),
@@ -137,6 +138,18 @@ impl TaffyLayouter {
         Arc::clone(&self.font_system)
     }
 
+    /// Share an external media store with this layouter. Resources loaded during layout are
+    /// stored here; passing the same store to the rasterizer lets it resolve those resources
+    /// by id. Without this they live in two separate stores and images render as placeholders.
+    pub fn set_media_store(&mut self, media_store: Arc<MediaStore>) {
+        self.media_store = media_store;
+    }
+
+    /// The media store shared by this layouter (see [`set_media_store`](Self::set_media_store)).
+    pub fn media_store(&self) -> Arc<MediaStore> {
+        Arc::clone(&self.media_store)
+    }
+
     pub fn print_tree(&mut self) {
         self.tree.print_tree(self.root_id);
     }
@@ -262,6 +275,12 @@ impl CanLayout for TaffyLayouter {
                         width: image_ctx.dimension.width as f32,
                         height: image_ctx.dimension.height as f32,
                     },
+                    // SVG-backed <img> elements carry their intrinsic size the same way.
+                    // Without this arm they measured as 0×0 and collapsed (e.g. the HN logo).
+                    Some(TaffyContext::Svg(svg_ctx)) => Size {
+                        width: svg_ctx.dimension.width as f32,
+                        height: svg_ctx.dimension.height as f32,
+                    },
                     _ => Size::ZERO,
                 }
             })
@@ -637,7 +656,17 @@ impl TaffyLayouter {
                     // occupies, so display quality is unaffected.
                     let is_placeholder = self.media_store.is_placeholder(media_id);
                     taffy_context = match media.borrow() {
-                        Media::Svg(_) => Some(TaffyContext::svg(src.as_str(), media_id, dom_node.node_id)),
+                        Media::Svg(media_svg) => {
+                            // Use the SVG's intrinsic size so the element gets a non-zero box.
+                            // A failed/placeholder load uses the same small fixed size as images.
+                            let dimension = if is_placeholder {
+                                geo::Dimension::new(32.0, 32.0)
+                            } else {
+                                let size = media_svg.svg.tree.size();
+                                geo::Dimension::new(size.width() as f64, size.height() as f64)
+                            };
+                            Some(TaffyContext::svg(src.as_str(), media_id, dimension, dom_node.node_id))
+                        }
                         Media::Image(media_image) => {
                             let dimension = if is_placeholder {
                                 geo::Dimension::new(32.0, 32.0)
@@ -656,7 +685,20 @@ impl TaffyLayouter {
                         .load_media_from_data(MediaType::Svg, inner_html.into_bytes().as_slice())
                     {
                         Ok(media_id) => {
-                            taffy_context = Some(TaffyContext::svg("gosub://internal", media_id, dom_node.node_id));
+                            let media = self.media_store.get(media_id, MediaType::Svg);
+                            let dimension = match media.borrow() {
+                                Media::Svg(media_svg) => {
+                                    let size = media_svg.svg.tree.size();
+                                    geo::Dimension::new(size.width() as f64, size.height() as f64)
+                                }
+                                _ => geo::Dimension::ZERO,
+                            };
+                            taffy_context = Some(TaffyContext::svg(
+                                "gosub://internal",
+                                media_id,
+                                dimension,
+                                dom_node.node_id,
+                            ));
                         }
                         Err(e) => {
                             log::warn!("Could not load SVG media: {:?}", e);