fix(doc-codec): snap table column boundaries within a point when unioning rows into one grid - #903
Merged
Merged
Conversation
Mearman
marked this pull request as ready for review
September 4, 2026 02:53
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…ning rows into one grid [MS-DOC] 2.6.4 states a table's column layout per row, so reconstructing the table's shared grid means unioning N independently-stated rgdxaCenter arrays. That union compared boundaries by exact integer equality, so two rows meaning the identical grid but differing by a single twip -- which nothing in the format forbids, and which LibreOffice's own WW8TabDesc::CalcDefaults produces by widening imported cells up to MINLAY per row -- split into a phantom hairline column, with a spurious colSpan on the cells of every row either side of it. Two rows one twip apart across a 2338-twip boundary read back as columnWidthsPt [116.9, 0.05, 144.95, 220] instead of [116.9, 145, 220]. The union now clusters boundaries within one point of each other, taking each cluster's smallest member as its representative and sorting first so the result depends on the boundary values alone rather than on row order. One point is TWIPS_PER_POINT itself: columnWidthsPt states the reconstructed grid in points, so a segment narrower than that is below the smallest unit the grid can distinguish. It also matches what LibreOffice applies to this same computation (COLFUZZY, 20 twips, wrtswtbl.hxx), whose changeover was confirmed at exactly 20/21 by sweeping one patched int16 through its own .doc importer, and it stays below that implementation's own 23-twip minimum cell width, so it cannot swallow a legitimately narrow column. Looking a row's own boundary up on the grid now snaps with the same tolerance rather than calling indexOf, since a raw boundary need not appear in the clustered array at all, and a boundary matching nothing throws instead of yielding a negative index. A physical cell whose two boundaries land on one canonical entry -- the zero-width cell rgdxaCenter's "non-decreasing" rule permits -- is carried as an ordinary un-spanned cell rather than one reporting a span of zero.
…e indent in the Tables section The Reading paragraph claimed an exact union of every row's rgdxaCenter, which is no longer what the reader does: it records the one-point snap instead, why the point is the tolerance rather than a picked number, and the LibreOffice COLFUZZY/MINLAY evidence and 20/21 sweep that ground it. "What is not resolved" gains the table's own horizontal position, which document-schema.js has no field for on either a table or a row. A table indent is therefore dropped on read even in the simple case where every row agrees on it, and rows that legally disagree about it -- Word writes -108 for an unindented table, so a table with one genuinely indented row has rows at -108 and 0 -- keep the extra boundary between them, with the wider rows' first cell spanning both segments. That is the shape LibreOffice reads the same bytes into, so it is the correct reading rather than a defect the tolerance should absorb. The zero-width cell rgdxaCenter's "non-decreasing" rule permits is recorded alongside it, since it shares a grid position with the cell after it and ContentTableCell cannot distinguish the two.
…ow column The one-point drift tolerance assumed a MINLAY-style minimum cell width that only LibreOffice's own writer guarantees -- this package's own writer widens nothing, so a real producer's rgdxaCenter can legally state a column narrower than a point, and folding its own two boundaries together as drift would silently delete it rather than fix a phantom one. effectiveColumnBoundaryTolerance clamps the tolerance, per table, to one twip below the narrowest strictly-positive gap any single row states between two of its own adjacent boundaries: two boundaries one row itself distinguishes are never folded together, however close.
Three cases: a genuinely narrow column surviving intact in a single row with no cross-row drift involved at all, the ordinary one-point tolerance still applying when nothing in the table states a narrower real column, and the tolerance narrowing below a real column any row states even when a different row's own cross-row drift would otherwise fall within the un-clamped default. Confirmed against a mutation reverting the clamp to the flat TWIPS_PER_POINT tolerance: two of the three new cases fail, restoring the clamp passes them again. Also corrects two comments that attributed the empirically-confirmed 20/21-twip changeover to LibreOffice's own .doc importer -- the import side preserves per-row drift untouched; the fuzz is applied on its ODF export, the actual point where a per-row table model is projected onto one shared grid -- and hedges the causal claim linking Word's own -108 default to sprmTCellPaddingDefault as a plausible inference rather than a sourced fact.
…tion The README attributed the confirmed 20/21-twip changeover to LibreOffice's own .doc importer; the import side preserves a drifted boundary untouched, and the fuzz is applied by its ODF export instead, the point where its per-row table model is genuinely projected onto one shared grid. Also corrects a claim that the snap "provably cannot swallow a legitimately narrow column" -- that guarantee is LibreOffice's own writer-side MINLAY widening, which this package's writer has no equivalent of, so a real narrow column is possible and the tolerance now clamps below it instead. Hedges the -108/sprmTCellPaddingDefault causal link as a plausible inference rather than a sourced fact.
Mearman
force-pushed
the
fix/doc-codec-column-grid-tolerance-898
branch
from
September 4, 2026 03:17
76fbd4c to
87d1db3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #898.
The issue reported two failure modes in
table/read.ts's column-grid union. They turned out to be different in kind, so only one of them is a bug: mode 1 (one-twip drift) is real and fixed here; mode 2 (rows carrying different left insets) reproduces identically in a real independent [MS-DOC] implementation, so the current reading is correct and the follow-up was documentation, not an algorithm change. Both are covered below with the evidence rather than asserted.Mode 1: one-twip drift is a real bug
canonicalColumnBoundariesTwipscompared boundaries by exact integer equality. Two rows meaning the identical grid but differing by a single twip split into a phantom hairline column, with a spuriouscolSpanon the cells of every row either side of it —columnWidthsPt[116.9, 0.05, 144.95, 220]instead of[116.9, 145, 220]. That exact output is what the two new drift tests produce when the fix is reverted, so the reproduction is confirmed rather than reasoned about.LibreOffice is not the producer that would emit this drift, but it is the implementation that proves a tolerance belongs here. Five LibreOffice 26.2.5.2 tables were generated (
soffice --headless --convert-to docfrom hand-written.fodt), deliberately including widths that cannot land on whole twips (2.5cm/3.1cm/4.7cm, 0.77in/1.13in/1.61in). Every row of every table wrote a byte-identicalrgdxaCenter— e.g.[0, 2338, 5238, 9638]for all three rows of the cm case — so LibreOffice rounds a table's columns to twips once for the whole table, not once per row. That is exactly why none of this package's existing LibreOffice-derived fixtures trips this.The decisive evidence is what that implementation does on the read side. Patching a single
int16insidea_cm.doc's second row (througharchive-codec's ownreadCompoundFile/writeCompoundFile, so the CFB stays valid), sweeping the drift, and running each patched file back through LibreOffice's own.docimporter (--convert-to fodt, counting realtable:table-column/table:covered-table-cellelements):The changeover is exact and sharp at 20/21.
Why one point is a derivation, not a picked number. Three independent lines agree on it:
columnWidthsPtstates the reconstructed grid in points, andread.tsalready definesTWIPS_PER_POINT = 20. A segment narrower than one point is below the smallest unit the grid can meaningfully distinguish, so the tolerance isTWIPS_PER_POINTitself rather than a literal.SwTableLine→SwTableBox, each box with its own width) — and answers it with a named constant,#define COLFUZZY 20insw/source/filter/inc/wrtswtbl.hxx, whereSwWriteTableCol::operator==treats positions differing by at most that as equal andSwXMLTableColumn_Impl's column set is ano3tl::sorted_vectorordered by that fuzzy comparator.a < b ⟺ a + 20 < bmakes two boundaries equivalent exactly when|a − b| ≤ 20, which is precisely the measured changeover.COLFUZZY (20) < MINLAY (23), LibreOffice's own minimum cell width (sw/inc/swtypes.hxx), whichWW8TabDesc::CalcDefaults(sw/source/filter/ww8/ww8par2.cxx) actively widens a narrower imported cell up to — so the snap provably cannot swallow a legitimately minimum-width column. That sameCalcDefaultsmutatingnCenter[]per row on import is also one real mechanism by which a.docin the wild comes to carry per-row drift at all, with Word nowhere in the picture.What changed
logicalCellsForRow's twoindexOfcalls become a snap lookup with the same tolerance, since a row's raw boundary need not appear in the clustered array at all. A boundary matching nothing now throws rather than silently yielding a negativestartGridIndex. It is an internal invariant — every boundary looked up went into building the grid — so it is labelled as one.rgdxaCenterneed only be "in non-decreasing order" ([MS-DOC] 2.9.321), so equal adjacent entries — a legal zero-width cell — were previously computingcolSpan: 0and reaching the schema as an ordinary cell only because0 > 1happens to be false.Mode 2: differing left insets are read correctly already
a_cm.docpatched so only row 1's first boundary moves 0 → 108, run back through LibreOffice's importer, yields a 4-column table with row 1 gaining a filler cell and rows 0 and 2 carryingtable:number-columns-spanned="2"plus a realtable:covered-table-cell. doc-codec producescolumnWidthsPt[5.4, 111.5, 145, 220]with spans[2,1,1] [1,1,1] [2,1,1]— the same grid and the same spans. The sweep confirms LibreOffice keeps that phantom column for every inset ≥ 21 twips, and the issue's own real-world figure (108) is five times the tolerance.This is not drift, and the tolerance deliberately does not absorb it. [MS-DOC] 2.9.321 defines
rgdxaCenter[0]as "the horizontal position of the logical left edge of the table, as indented from the logical left page margin" — margin-relative, stated independently by each row — and 2.6.4 makes a per-row leading indent a first-class construct (sprmTWidthBefore, "the preferred additional leading indent of the first cell of the row, relative to the leading edge of the table as a whole";sprmTDxaLeft/sprmTDxaGapHalflikewise row-scoped). Rows disagreeing about it genuinely occupy different horizontal extents. LibreOffice's WW8 importer even carries Word's own-108as a named comment inCalcDefaults("Word sets the first nCenter value to -108 when no indent is used"), matching the spec's own 108-twipsprmTCellPaddingDefault.The only difference from LibreOffice is cosmetic: it pads the short row with an empty filler cell. That is deliberately not adopted —
ContentTableRow.cellscarries no grid-position field, so a reader-invented empty cell would be indistinguishable from real empty content on the write side, andtable/write.tsalready reconstructs each row's own narrowerrgdxaCenterfrom spans without needing one.What mode 2 did surface is a genuine, permanent limitation the README did not state:
ContentTableis{ kind, rows, columnWidthsPt, sourcePath?, source?, frames? }andContentTableRowis{ cells, heightPt? }— neither has a field that could hold a horizontal offset, so a table indent is dropped on read even in the simple case where every row agrees on it (a LibreOffice table withfo:margin-left="1.27cm"writesrgdxaCenter = [720, 2884, 5567, 9638]in every row and reads back as[108.2, 134.15, 203.55]). That is family-wide rather than a doc-codec gap — nothing inooxml.jsreads or writesw:tblInd/w:tblPrEx, and no codec here produces a table indent — and the Tables section now says so.Tests
Eight new cases in
src/table/read.test.ts, driven offrgdxaCenterarrays hand-assembled straight from [MS-DOC]'s own field tables rather than binary fixtures, matching the file's existing approach (this package deliberately has no real-world corpus). They cover the whole prototype table the investigation verified against LibreOffice's own import: no drift, drift of one twip, drift of exactly one point, drift of one point and one twip, Word's-108stated by every row, a 108-twip indent stated by one row only, the real #895 LibreOffice merged-row arrays, and a legal zero-width cell. The tolerance cases are phrased as fractions of a point derived fromTWIPS_PER_POINT, never as bare 20/21.Reverting the
read.tschange and re-running fails exactly two of them — the one-twip and one-point drift cases, with[116.9, 0.05, 144.95, 220]and[116.9, 1, 144, 220]against the expected[116.9, 145, 220]. Every other new case passes both before and after, which is the point: they pin behaviour the tolerance must not change, including #895's merge reconstruction and mode 2's deliberate phantom column.Verification
pnpm exec turbo run _lint _typecheck _test _test:workers --filter=doc-codec --force— 6/6 tasks, 162 unit tests and 5 workerd tests passing.pnpm exec turbo run _lint _typecheck --forceacross the workspace — 57/57.pnpm exec turbo run _testacross the workspace — 36/36.A real regression found and fixed on independent review
The one-point tolerance's own "provably cannot swallow a legitimately narrow column" claim was false: that guarantee is LibreOffice's own writer-side
MINLAYminimum-cell-width widening, which this package's writer has no equivalent of. Proven by round-tripping through the package itself: writing a genuinely 0.5pt-wide column and reading it back silently deleted it ([50,0.5,50]->[50,50.5]), a regression against the pre-PR behaviour, where all four such cases round-tripped exactly.Fixed by clamping the tolerance, per table, to one twip below the narrowest strictly-positive gap any single row states between two of its own adjacent boundaries -- two boundaries a row itself distinguishes are never folded together, however close. Confirmed against a mutation reverting the clamp: the new regression tests fail, restoring the clamp passes them.
Also corrected two claims that attributed the confirmed 20/21-twip changeover to LibreOffice's own
.docimporter -- the import side preserves per-row drift untouched; the fuzz is applied on its ODF export instead, the actual point where a per-row table model is projected onto one shared grid -- and hedged the-108/sprmTCellPaddingDefaultcausal link as a plausible inference rather than a sourced fact.