From ba58622eaa1a9ca5a1737deb9922e1b3a6302a6b Mon Sep 17 00:00:00 2001
From: James Yang <jamesyang663@gmail.com>
Date: Thu, 13 Aug 2026 13:52:29 -0400
Subject: [PATCH] fix(sheet): skip hidden rows and columns in OOXML xlsx

Calamine does not expose worksheet row/col hidden attrs, so stream them
from sheetN.xml and omit those axes when building tables. Addresses the
remaining row/column half of #9 without racing PR #39 sheet visibility.
---
 src/formats/sheet/mod.rs        | 114 ++++++++++-
 src/formats/sheet/visibility.rs | 329 ++++++++++++++++++++++++++++++++
 2 files changed, 434 insertions(+), 9 deletions(-)
 create mode 100644 src/formats/sheet/visibility.rs

diff --git a/src/formats/sheet/mod.rs b/src/formats/sheet/mod.rs
index f61ebcc..739c565 100644
--- a/src/formats/sheet/mod.rs
+++ b/src/formats/sheet/mod.rs
@@ -1,4 +1,11 @@
 //! Excel spreadsheets (xlsx, xlsm, xlsb, xls) via calamine.
+//!
+//! For xlsx-like OOXML packages, hidden rows and columns declared in each
+//! worksheet part (`<row hidden>` / `<col hidden>`) are omitted by default.
+//! Binary xls/xlsb and collapsed outline / zero-size axes are out of scope
+//! (see `visibility`).
+
+mod visibility;
 
 use crate::error::ConvertError;
 use crate::model::{Block, Cell, Document, GridBuilder, Inline, TableKind};
@@ -7,6 +14,7 @@ use crate::shared::text::clean_text;
 use calamine::{Data, Dimensions, Reader, Sheets, open_workbook_auto_from_rs};
 use std::collections::{HashMap, HashSet};
 use std::io::Cursor;
+use visibility::SheetHidden;
 
 /// Run one calamine operation behind a panic barrier: the calamine fork can
 /// panic on corrupt containers (pending an upstream fix), and a dependency
@@ -27,6 +35,8 @@ pub fn parse(bytes: &[u8]) -> Result<Document, ConvertError> {
     let sheet_names = contained("sheet listing", || workbook.sheet_names().to_owned())?;
     let multi_sheet = sheet_names.len() > 1;
     let merged = merged_regions(&mut workbook, &sheet_names)?;
+    // OOXML-only: binary containers yield an empty map and keep prior behavior.
+    let hidden_by_sheet = visibility::load_hidden_axes(bytes);
 
     let mut doc = Document::default();
     let mut failed = 0usize;
@@ -42,6 +52,7 @@ pub fn parse(bytes: &[u8]) -> Result<Document, ConvertError> {
         if range.is_empty() {
             continue;
         }
+        let hidden = hidden_by_sheet.get(name.as_str());
         // Merged regions in range-relative coordinates: the top-left cell
         // becomes a spanning origin, the other positions are covered.
         let start = range.start().unwrap_or((0, 0));
@@ -65,12 +76,28 @@ pub fn parse(bytes: &[u8]) -> Result<Document, ConvertError> {
             let c0 = (col0 - start.1) as usize;
             let r1 = (row_end - start.0 as u64) as usize;
             let c1 = (col_end - start.1 as u64) as usize;
-            if r1 - r0 == 1 && c1 - c0 == 1 {
+            let visible_rows: Vec<usize> =
+                (r0..r1).filter(|&r| !axis_row_hidden(hidden, start.0 + r as u32)).collect();
+            let visible_cols: Vec<usize> =
+                (c0..c1).filter(|&c| !axis_col_hidden(hidden, start.1 + c as u32)).collect();
+            if visible_rows.is_empty() || visible_cols.is_empty() {
+                continue;
+            }
+            // Value lives at the merge origin; if that axis is hidden, drop
+            // the merge so remaining visible covered cells stay empty.
+            if axis_row_hidden(hidden, start.0 + r0 as u32)
+                || axis_col_hidden(hidden, start.1 + c0 as u32)
+            {
+                continue;
+            }
+            let row_span = visible_rows.len() as u32;
+            let col_span = visible_cols.len() as u32;
+            if row_span == 1 && col_span == 1 {
                 continue;
             }
-            origins.insert((r0, c0), ((c1 - c0) as u32, (r1 - r0) as u32));
-            for r in r0..r1 {
-                for c in c0..c1 {
+            origins.insert((r0, c0), (col_span, row_span));
+            for &r in &visible_rows {
+                for &c in &visible_cols {
                     if (r, c) != (r0, c0) {
                         covered.insert((r, c));
                     }
@@ -78,9 +105,22 @@ pub fn parse(bytes: &[u8]) -> Result<Document, ConvertError> {
             }
         }
         let mut builder = GridBuilder::new();
+        let any_visible_col =
+            (0..width).any(|c| !axis_col_hidden(hidden, start.1.saturating_add(c as u32)));
+        if !any_visible_col {
+            continue;
+        }
         for (r, row) in range.rows().enumerate() {
+            let abs_row = start.0.saturating_add(r as u32);
+            if axis_row_hidden(hidden, abs_row) {
+                continue;
+            }
             builder.next_row();
             for (c, data) in row.iter().enumerate() {
+                let abs_col = start.1.saturating_add(c as u32);
+                if axis_col_hidden(hidden, abs_col) {
+                    continue;
+                }
                 if covered.contains(&(r, c)) {
                     builder.covered();
                     continue;
@@ -116,6 +156,14 @@ pub fn parse(bytes: &[u8]) -> Result<Document, ConvertError> {
     Ok(doc)
 }
 
+fn axis_row_hidden(hidden: Option<&SheetHidden>, abs_row: u32) -> bool {
+    hidden.is_some_and(|h| h.row_hidden(abs_row))
+}
+
+fn axis_col_hidden(hidden: Option<&SheetHidden>, abs_col: u32) -> bool {
+    hidden.is_some_and(|h| h.col_hidden(abs_col))
+}
+
 /// Merged regions per sheet, where the container format exposes them (xlsx
 /// via each worksheet's mergeCells part, xls via BIFF MERGEDCELLS).
 fn merged_regions<RS: std::io::Read + std::io::Seek>(
@@ -209,6 +257,8 @@ fn format_duration_days(days: f64) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::Format;
+    use crate::to_markdown_bytes;
     use std::io::Write;
 
     /// Minimal xlsx with a used range at D11:E12 and the given merged region.
@@ -216,6 +266,14 @@ mod tests {
         let sheet = format!(
             r#"<?xml version="1.0"?><worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><sheetData><row r="11"><c r="D11" t="inlineStr"><is><t>x</t></is></c><c r="E11" t="inlineStr"><is><t>y</t></is></c></row><row r="12"><c r="D12" t="inlineStr"><is><t>z</t></is></c><c r="E12" t="inlineStr"><is><t>w</t></is></c></row></sheetData><mergeCells count="1"><mergeCell ref="{merge_ref}"/></mergeCells></worksheet>"#
         );
+        pack_xlsx("S", &sheet)
+    }
+
+    /// Minimal single-sheet xlsx with the given worksheet body.
+    fn pack_xlsx(sheet_name: &str, sheet_xml: &str) -> Vec<u8> {
+        let workbook = format!(
+            r#"<?xml version="1.0"?><workbook xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"><sheets><sheet name="{sheet_name}" sheetId="1" r:id="rId1"/></sheets></workbook>"#
+        );
         let parts: &[(&str, &str)] = &[
             (
                 "[Content_Types].xml",
@@ -225,10 +283,7 @@ mod tests {
                 "_rels/.rels",
                 r#"<?xml version="1.0"?><Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument" Target="xl/workbook.xml"/></Relationships>"#,
             ),
-            (
-                "xl/workbook.xml",
-                r#"<?xml version="1.0"?><workbook xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"><sheets><sheet name="S" sheetId="1" r:id="rId1"/></sheets></workbook>"#,
-            ),
+            ("xl/workbook.xml", workbook.as_str()),
             (
                 "xl/_rels/workbook.xml.rels",
                 r#"<?xml version="1.0"?><Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/worksheet" Target="worksheets/sheet1.xml"/></Relationships>"#,
@@ -240,7 +295,7 @@ mod tests {
             w.write_all(body.as_bytes()).unwrap();
         }
         w.start_file("xl/worksheets/sheet1.xml", zip::write::SimpleFileOptions::default()).unwrap();
-        w.write_all(sheet.as_bytes()).unwrap();
+        w.write_all(sheet_xml.as_bytes()).unwrap();
         w.finish().unwrap().into_inner()
     }
 
@@ -270,6 +325,47 @@ mod tests {
         assert_eq!(covered_count(&doc), 0, "out-of-range merge must not cover cells");
     }
 
+    #[test]
+    fn hidden_rows_and_columns_are_omitted() {
+        // Mirrors the issue #9 openpyxl repro: visible A1, hidden row 2, hidden col B.
+        let sheet = r#"<?xml version="1.0"?><worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><cols><col min="2" max="2" hidden="1" width="10"/></cols><sheetData><row r="1"><c r="A1" t="inlineStr"><is><t>VISIBLE_CELL</t></is></c><c r="B1" t="inlineStr"><is><t>HIDDEN_COL_TEXT</t></is></c></row><row r="2" hidden="1"><c r="A2" t="inlineStr"><is><t>HIDDEN_ROW_TEXT</t></is></c><c r="B2" t="inlineStr"><is><t>HIDDEN_BOTH</t></is></c></row></sheetData></worksheet>"#;
+        let bytes = pack_xlsx("Visibility", sheet);
+        let doc = parse(&bytes).unwrap();
+        let md = to_markdown_bytes(&bytes, Format::Excel).unwrap();
+        assert!(md.contains("VISIBLE_CELL"), "visible cell missing from markdown:\n{md}");
+        assert!(!md.contains("HIDDEN_ROW_TEXT"), "hidden row leaked into markdown:\n{md}");
+        assert!(!md.contains("HIDDEN_COL_TEXT"), "hidden column leaked into markdown:\n{md}");
+        assert!(!md.contains("HIDDEN_BOTH"), "hidden row+col leaked into markdown:\n{md}");
+
+        let text = document_plain(&doc);
+        assert!(text.contains("VISIBLE_CELL"));
+        assert!(!text.contains("HIDDEN_ROW_TEXT"));
+        assert!(!text.contains("HIDDEN_COL_TEXT"));
+        assert!(!text.contains("HIDDEN_BOTH"));
+    }
+
+    fn document_plain(doc: &Document) -> String {
+        use crate::model::inlines_to_plain_text;
+        let mut out = String::new();
+        for block in &doc.blocks {
+            if let Block::Table(t) = block {
+                for row in &t.grid {
+                    for slot in row {
+                        if let crate::model::CellSlot::Origin(cell) = slot {
+                            for b in &cell.blocks {
+                                if let Block::Paragraph(inlines) = b {
+                                    out.push_str(&inlines_to_plain_text(inlines));
+                                    out.push('\n');
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        out
+    }
+
     #[test]
     fn string_cells_are_not_trimmed() {
         assert_eq!(format_data(&Data::String("  padded  ".into())), "  padded  ");
diff --git a/src/formats/sheet/visibility.rs b/src/formats/sheet/visibility.rs
new file mode 100644
index 0000000..65e1fbe
--- /dev/null
+++ b/src/formats/sheet/visibility.rs
@@ -0,0 +1,329 @@
+//! Row/column visibility from SpreadsheetML worksheet parts.
+//!
+//! calamine exposes cell values and merges but not `<row hidden>` /
+//! `<col hidden>`. For xlsx-like OOXML packages this module opens the zip,
+//! maps sheet names to worksheet parts via workbook relationships, and
+//! streams only those visibility attributes.
+//!
+//! Scope: OOXML SpreadsheetML (`.xlsx` / `.xlsm`). Binary `.xls` / `.xlsb`
+//! and ODS are not covered here. Collapsed outline groups and zero
+//! width/height columns or rows are out of scope.
+
+use crate::package::limits;
+use crate::package::path;
+use crate::package::xml::ns;
+use quick_xml::NsReader;
+use quick_xml::XmlVersion;
+use quick_xml::events::Event;
+use quick_xml::name::ResolveResult;
+use std::collections::{HashMap, HashSet};
+use std::io::{Cursor, Read};
+use zip::ZipArchive;
+
+/// Hidden axes for one worksheet, in calamine's 0-indexed coordinates.
+#[derive(Debug, Default)]
+pub(super) struct SheetHidden {
+    rows: HashSet<u32>,
+    /// Inclusive column ranges, 0-indexed.
+    col_ranges: Vec<(u32, u32)>,
+}
+
+impl SheetHidden {
+    pub(super) fn is_empty(&self) -> bool {
+        self.rows.is_empty() && self.col_ranges.is_empty()
+    }
+
+    pub(super) fn row_hidden(&self, abs_row: u32) -> bool {
+        self.rows.contains(&abs_row)
+    }
+
+    pub(super) fn col_hidden(&self, abs_col: u32) -> bool {
+        self.col_ranges.iter().any(|&(min, max)| abs_col >= min && abs_col <= max)
+    }
+}
+
+/// Sheet name → hidden rows/columns. Empty when the bytes are not OOXML or
+/// carry no hidden axes. Resource-limit failures on individual parts are
+/// logged and that sheet is omitted from the map (conversion continues
+/// without filtering that sheet).
+pub(super) fn load_hidden_axes(bytes: &[u8]) -> HashMap<String, SheetHidden> {
+    let mut zip = match ZipArchive::new(Cursor::new(bytes)) {
+        Ok(z) => z,
+        Err(_) => return HashMap::new(),
+    };
+    if zip.by_name("xl/workbook.xml").is_err() {
+        return HashMap::new();
+    }
+
+    let workbook = match read_capped(&mut zip, "xl/workbook.xml") {
+        Some(b) => b,
+        None => return HashMap::new(),
+    };
+    let sheets = parse_sheet_rids(&workbook);
+    if sheets.is_empty() {
+        return HashMap::new();
+    }
+
+    let rels = match read_capped(&mut zip, "xl/_rels/workbook.xml.rels") {
+        Some(b) => b,
+        None => return HashMap::new(),
+    };
+    let rid_to_target = parse_worksheet_targets(&rels);
+
+    let mut out = HashMap::new();
+    for (name, rid) in sheets {
+        let Some(target) = rid_to_target.get(&rid) else {
+            continue;
+        };
+        let part = match path::resolve("xl/workbook.xml", target) {
+            Ok(t) => t.path,
+            Err(e) => {
+                log::warn!("skipping unresolvable worksheet target {target:?}: {e}");
+                continue;
+            }
+        };
+        let Some(sheet_bytes) = read_capped(&mut zip, &part) else {
+            continue;
+        };
+        let hidden = parse_sheet_hidden(&sheet_bytes);
+        if !hidden.is_empty() {
+            out.insert(name, hidden);
+        }
+    }
+    out
+}
+
+fn read_capped(zip: &mut ZipArchive<Cursor<&[u8]>>, name: &str) -> Option<Vec<u8>> {
+    let mut file = match zip.by_name(name) {
+        Ok(f) => f,
+        Err(zip::result::ZipError::FileNotFound) => return None,
+        Err(e) => {
+            log::warn!("skipping unreadable archive entry {name}: {e}");
+            return None;
+        }
+    };
+    if file.size() > limits::MAX_ENTRY_BYTES {
+        log::warn!("skipping oversized archive entry {name}");
+        return None;
+    }
+    let mut buf = Vec::new();
+    let cap = limits::MAX_ENTRY_BYTES;
+    match (&mut file).take(cap + 1).read_to_end(&mut buf) {
+        Ok(n) if (n as u64) > cap => {
+            log::warn!("skipping archive entry {name}: exceeds decompression cap");
+            None
+        }
+        Ok(_) => Some(buf),
+        Err(e) => {
+            log::warn!("skipping corrupt archive entry {name}: {e}");
+            None
+        }
+    }
+}
+
+/// `(sheet name, rId)` in workbook order.
+fn parse_sheet_rids(workbook: &[u8]) -> Vec<(String, String)> {
+    let mut reader = NsReader::from_reader(workbook);
+    reader.config_mut().trim_text(false);
+    let mut buf = Vec::new();
+    let mut out = Vec::new();
+    loop {
+        match reader.read_event_into(&mut buf) {
+            Ok(Event::Start(e) | Event::Empty(e)) => {
+                let local = local_name(reader.decoder(), e.name());
+                if local != "sheet" {
+                    buf.clear();
+                    continue;
+                }
+                let mut name = None;
+                let mut rid = None;
+                for attr in e.attributes().flatten() {
+                    let key = attr_local(reader.decoder(), attr.key);
+                    match key.as_str() {
+                        "name" => {
+                            name = attr_value(reader.decoder(), &attr);
+                        }
+                        // Prefer r:id (relationships ns); bare id is rare.
+                        "id" if rid.is_none() || attr_ns_is_rels(&reader, attr.key) => {
+                            rid = attr_value(reader.decoder(), &attr);
+                        }
+                        _ => {}
+                    }
+                }
+                if let (Some(name), Some(rid)) = (name, rid) {
+                    out.push((name, rid));
+                }
+            }
+            Ok(Event::Eof) | Err(_) => break,
+            _ => {}
+        }
+        buf.clear();
+    }
+    out
+}
+
+fn parse_worksheet_targets(rels: &[u8]) -> HashMap<String, String> {
+    let mut reader = NsReader::from_reader(rels);
+    reader.config_mut().trim_text(false);
+    let mut buf = Vec::new();
+    let mut out = HashMap::new();
+    loop {
+        match reader.read_event_into(&mut buf) {
+            Ok(Event::Start(e) | Event::Empty(e)) => {
+                let local = local_name(reader.decoder(), e.name());
+                if local != "Relationship" {
+                    buf.clear();
+                    continue;
+                }
+                let mut id = None;
+                let mut target = None;
+                let mut rel_type = None;
+                for attr in e.attributes().flatten() {
+                    let key = attr_local(reader.decoder(), attr.key);
+                    let val = attr_value(reader.decoder(), &attr);
+                    match key.as_str() {
+                        "Id" => id = val,
+                        "Target" => target = val,
+                        "Type" => rel_type = val,
+                        _ => {}
+                    }
+                }
+                if let (Some(id), Some(target), Some(rel_type)) = (id, target, rel_type)
+                    && rel_type.contains("/worksheet")
+                {
+                    out.insert(id, target);
+                }
+            }
+            Ok(Event::Eof) | Err(_) => break,
+            _ => {}
+        }
+        buf.clear();
+    }
+    out
+}
+
+fn parse_sheet_hidden(sheet: &[u8]) -> SheetHidden {
+    let mut reader = NsReader::from_reader(sheet);
+    reader.config_mut().trim_text(false);
+    let mut buf = Vec::new();
+    let mut hidden = SheetHidden::default();
+    let mut next_row = 1u32;
+    loop {
+        match reader.read_event_into(&mut buf) {
+            Ok(Event::Start(e) | Event::Empty(e)) => {
+                let local = local_name(reader.decoder(), e.name());
+                match local.as_str() {
+                    "col" => {
+                        let mut min = None;
+                        let mut max = None;
+                        let mut is_hidden = false;
+                        for attr in e.attributes().flatten() {
+                            let key = attr_local(reader.decoder(), attr.key);
+                            let val = attr_value(reader.decoder(), &attr);
+                            match key.as_str() {
+                                "min" => min = val.as_deref().and_then(|v| v.parse::<u32>().ok()),
+                                "max" => max = val.as_deref().and_then(|v| v.parse::<u32>().ok()),
+                                "hidden" => {
+                                    is_hidden = val.as_deref().is_some_and(ooxml_bool);
+                                }
+                                _ => {}
+                            }
+                        }
+                        if is_hidden {
+                            let Some(min) = min.filter(|&m| m >= 1) else {
+                                buf.clear();
+                                continue;
+                            };
+                            let max = max.unwrap_or(min).max(min);
+                            // OOXML cols are 1-indexed; calamine uses 0-indexed.
+                            hidden.col_ranges.push((min - 1, max - 1));
+                        }
+                    }
+                    "row" => {
+                        let mut r = None;
+                        let mut is_hidden = false;
+                        for attr in e.attributes().flatten() {
+                            let key = attr_local(reader.decoder(), attr.key);
+                            let val = attr_value(reader.decoder(), &attr);
+                            match key.as_str() {
+                                "r" => r = val.as_deref().and_then(|v| v.parse::<u32>().ok()),
+                                "hidden" => {
+                                    is_hidden = val.as_deref().is_some_and(ooxml_bool);
+                                }
+                                _ => {}
+                            }
+                        }
+                        let row_num = r.unwrap_or(next_row).max(1);
+                        next_row = row_num.saturating_add(1);
+                        if is_hidden {
+                            hidden.rows.insert(row_num - 1);
+                        }
+                    }
+                    _ => {}
+                }
+            }
+            Ok(Event::Eof) | Err(_) => break,
+            _ => {}
+        }
+        buf.clear();
+    }
+    hidden
+}
+
+fn ooxml_bool(v: &str) -> bool {
+    v == "1" || v.eq_ignore_ascii_case("true")
+}
+
+fn local_name(decoder: quick_xml::Decoder, name: quick_xml::name::QName<'_>) -> String {
+    let raw = name.local_name().into_inner();
+    decoder.decode(raw).map(|s| s.into_owned()).unwrap_or_default()
+}
+
+fn attr_local(decoder: quick_xml::Decoder, key: quick_xml::name::QName<'_>) -> String {
+    local_name(decoder, key)
+}
+
+fn attr_value(
+    decoder: quick_xml::Decoder,
+    attr: &quick_xml::events::attributes::Attribute<'_>,
+) -> Option<String> {
+    attr.decoded_and_normalized_value(XmlVersion::Implicit1_0, decoder).ok().map(|s| s.into_owned())
+}
+
+fn attr_ns_is_rels(reader: &NsReader<&[u8]>, key: quick_xml::name::QName<'_>) -> bool {
+    matches!(
+        reader.resolver().resolve_attribute(key),
+        (ResolveResult::Bound(ns), _) if ns.as_ref() == ns::R.as_bytes()
+            || ns.as_ref() == b"http://purl.oclc.org/ooxml/officeDocument/relationships"
+    )
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn parses_hidden_row_and_col_attrs() {
+        let xml = br#"<?xml version="1.0"?>
+<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
+  <cols>
+    <col min="2" max="2" hidden="1" width="10"/>
+    <col min="4" max="5" hidden="true" width="8"/>
+  </cols>
+  <sheetData>
+    <row r="1"><c r="A1"/></row>
+    <row r="2" hidden="1"><c r="A2"/></row>
+    <row r="4" hidden="true"><c r="A4"/></row>
+  </sheetData>
+</worksheet>"#;
+        let h = parse_sheet_hidden(xml);
+        assert!(h.row_hidden(1));
+        assert!(h.row_hidden(3));
+        assert!(!h.row_hidden(0));
+        assert!(h.col_hidden(1));
+        assert!(h.col_hidden(3));
+        assert!(h.col_hidden(4));
+        assert!(!h.col_hidden(0));
+        assert!(!h.col_hidden(2));
+    }
+}