Skip to content

fix(xlsx): give a partial cell font the default font's name and size - #605

Open
ronistrom wants to merge 1 commit into
productdevbook:mainfrom
ronistrom:fix/font-inherits-default
Open

ronistrom wants to merge 1 commit into
productdevbook:mainfrom
ronistrom:fix/font-inherits-default

Conversation

@ronistrom

@ronistrom ronistrom commented Oct 20, 2026 •

Copy link
Copy Markdown

Closes #594

What was wrong

defaultFont is written as fonts[0], and Workbook.defaultFont's doc says it is "the entry every cell format inherits from unless it names another". A cell's fontId does not work that way. It picks one complete <font>, so a cell font that gave only part of itself was written without a face or size:

await writeXlsx({
  defaultFont: { name: "Arial", size: 10 },
  sheets: [{ name: "Sheet1", rows: [[{ value: "Bold header", style: { font: { bold: true, color: { rgb: "FF1F3A5F" } } } }]] }],
})
<!-- before -->
<fonts count="2"><font><sz val="10"/><name val="Arial"/></font><font><b/><color rgb="FF1F3A5F"/></font></fonts>
<!-- after -->
<fonts count="2"><font><sz val="10"/><name val="Arial"/></font><font><b/><color rgb="FF1F3A5F"/><sz val="10"/><name val="Arial"/></font></fonts>

Reading it back with readStyles: true, the cell's font was { bold: true, color: { rgb: "1F3A5F" } } before and is { bold: true, color: { rgb: "1F3A5F" }, size: 10, name: "Arial" } after. According to the issue, LibreOffice drew the old file's bold cell in Cambria 11pt.

Why the tests didn't catch it

No test wrote a partial font next to a defaultFont and looked at the <font> that came out. The parity register did write one: its columns probe has { bold: true } and the fixture has defaultFont: { name: "Georgia", size: 13 }. But it expected font: { bold: true } back, so it pinned the bug.

What landed

  • inheritBaseFont in styles-writer.ts, called from registerXf before addFont. A missing name or size comes from the base font. When the name is inherited, the base's family, charset and scheme come with it. A font that names its own face does not get them, because scheme="minor" would make Excel draw the theme font instead. Colour and the other parts are not inherited. A font that already has both a name and a size is passed through without a copy, because the streaming writers call this once per cell.
  • Differential formats (addDxf, used by conditional formatting) are unchanged. There, a part that is left out means "keep the cell's own".
  • The fix reaches writeXlsx, the streaming writers (Calibri 11, since they take no defaultFont) and the round-trip writer, because they all use the same collector.
  • WriteOptions.defaultFont gets a doc comment. docs/PARITY.md has a short section on what comes back. The parity register's columns expectation now includes Georgia 13, with a comment that explains why.

Tests: test/font-inherits-default.test.ts adds 9 tests, and 7 of them fail against main. The 2 that pass on main are guards: the conditional formatting font stays unmerged, and the caller's style object is not mutated. The changed parity expectation also fails without the fix. bun run test passes on Node 24.22.0, with 241 files and 10,707 tests.

Checked in Excel

In Excel for Mac, with the issue's case written by main and by this branch. Both files open without a repair prompt. In both, Excel reports A1 as Arial 10, bold, dark blue. Excel fills a font's missing name and size from font 0 itself, so the bug does not show in Excel. The difference shows in apps that don't fill the gap that way.

Not verified

  • LibreOffice. Not opened. The claim that it draws the old output's bold cell in Cambria 11pt comes from the issue.
  • Windows Excel. Only Excel for Mac was checked.

Open question: rich text runs have the same problem. A run font without name/size is written as given. I left that out of this PR.

Summary by CodeRabbit

  • New Features

    • Cell fonts now inherit missing name, size, family, charset, and scheme settings from the workbook’s default font.
    • Fonts without specified details fall back to Calibri 11 when no workbook default is provided.
    • Conditional-formatting fonts preserve omitted components and round-trip accurately.
    • Explicit font settings and colors remain unchanged.
  • Documentation

    • Added guidance describing workbook default-font behavior and cell-font inheritance.

`defaultFont` is written as fonts[0], and its doc says every cell format
inherits from it. But a cell's fontId picks one complete <font>; it does
not layer on fonts[0]. A cell font that gave only part of itself, such
as `{ bold: true }`, was written as `<font><b/></font>` with no face or
size, so LibreOffice drew it in its own default (Cambria 11pt) rather
than the workbook's.

registerXf now passes the cell font through inheritBaseFont before
addFont. A missing `name` or `size` is taken from the base font, and
when the name is inherited the base's `family`, `charset` and `scheme`
come with it; a font that names its own face keeps it, because
`scheme="minor"` would make Excel draw the theme font instead. Colour
and the other parts are not inherited. Differential formats (addDxf,
used by conditional formatting) are left as given, since there a
missing part means "keep the cell's own".

This applies to writeXlsx, the streaming writers (Calibri 11, as they
take no defaultFont) and the round-trip writer. The parity register's
`columns` probe now reads the bold font back with Georgia 13 from the
fixture's defaultFont. WriteOptions.defaultFont gets a doc comment and
docs/PARITY.md says what comes back.

Closes productdevbook#594
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1f38593a-d581-4108-8948-033ae09a2f2c
📥 Commits

Reviewing files that changed from the base of the PR and between c991ad0 and e5f6ce7.

📒 Files selected for processing: 5
  • docs/PARITY.md
  • src/_types.ts
  • src/xlsx/styles-writer.ts
  • test/font-inherits-default.test.ts
  • test/xlsx-write-read-parity.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Changes

Cell-format fonts now inherit missing name and size from the workbook’s base font. When the cell font has no name, it also inherits the base font’s family, charset, and scheme. Differential-format fonts remain unchanged. Tests and documentation cover these rules and related write/read behavior.

Cell Font Inheritance

Layer / File(s) Summary
Inherit default font attributes
src/_types.ts, src/xlsx/styles-writer.ts, test/font-inherits-default.test.ts, test/xlsx-write-read-parity.test.ts, docs/PARITY.md
Cell-format fonts inherit selected missing attributes from the workbook default font. Tests cover defaults, overrides, differential formats, immutability, streaming, and read-back. The parity expectation and documentation describe the behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: productdevbook

Merge Risk: ⚪ Minimal · up to e5f6c

The cell-font inheritance change is ready to merge after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: partial cell fonts inherit the default font's name and size.
Linked Issues check Passed Issue #594 requires partial cell fonts to receive the default font's missing name and size, with related base metadata when the name is inherited. inheritBaseFont applies this in cell-format reg…
Out of Scope Changes check Passed The changes stay within issue #594. The implementation updates cell-format font registration and the shared writer paths. The tests and parity expectation verify the requested behavior. The `defaultFo…
Full details: Linked Issues check

Explanation

Issue #594 requires partial cell fonts to receive the default font's missing name and size, with related base metadata when the name is inherited. inheritBaseFont applies this in cell-format registration before font deduplication. It preserves explicit font properties and does not inherit color. addDxf remains unchanged, so conditional-formatting fonts keep only their specified properties. Tests cover XML output, explicit overrides, Calibri fallback, metadata, color exclusion, conditional formats, immutability, streaming output, and read-back. Rich text runs are explicitly optional in issue #594 and remain excluded.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #594. The implementation updates cell-format font registration and the shared writer paths. The tests and parity expectation verify the requested behavior. The defaultFont documentation and parity documentation support the changed contract. No unrelated implementation change is shown. Rich text runs remain outside the requested cell-format scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A style font without name/size does not inherit defaultFont

1 participant