feat(rtf-codec): map form fields to the contentControl construct - #1001
Merged
Conversation
Mearman
force-pushed
the
feat/rtf-codec-formfield-content-control
branch
5 times, most recently
from
September 5, 2026 20:35
1b70b74 to
a6bdce6
Compare
Mearman
marked this pull request as ready for review
September 5, 2026 20:39
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. |
Mearman
force-pushed
the
feat/rtf-codec-formfield-content-control
branch
6 times, most recently
from
September 6, 2026 13:33
7cc0336 to
2f471dc
Compare
RTF's \*\formfield destination (nested inside a \field's own \*\fldinst, alongside its FORMTEXT/FORMCHECKBOX/FORMDROPDOWN instruction) is RTF's own analogue to a Word content control, mirroring how ooxml.js already maps docx's legacy w:ffData onto the identical construct kind. The reader derives controlType from the instruction keyword (robust even when a legacy field carries no \*\formfield group at all), and layers on whatever \*\formfield data is present: \*\ffname as the control's tag, a checkbox's \ffres/\ffdefres as its checked state (current overriding default), and a dropdown's \*\ffl entries as its options. A form field is always inline -- one \field group, never spanning a paragraph boundary -- so the construct rides a RunConstructExtent on the paragraph exactly like a revision mark, not a block-level constructStart/constructEnd pair; the field's own displayed text is not duplicated onto the descriptor, since it already rides the ordinary runs the extent wraps. The writer mints a real \*\formfield back from a contentControl construct for the three controlTypes RTF's own vocabulary actually covers (plainText/ checkbox/dropDown); any other controlType still degrades through the same construct-gap diagnostic every other unrepresentable construct uses.
Read-side fixtures are trimmed from a real producer's own output (PHPRtfLite), braces and all, including the anonymous scoping group \*\formfield wraps its own control words in -- the reader never needs to know that group is there, since an unrecognised first control word simply inherits the enclosing destination. Covers a checkbox as a point extent, \ffres overriding \ffdefres, a dropdown's \*\ffl options with its \fldrslt as the wrapped run, a legacy field with no \*\formfield group at all, and that an ordinary field (HYPERLINK) still produces no contentControl. Write-side tests check the minted \*\formfield production directly and round-trip each of the three controlTypes back through the reader, plus the construct-gap diagnostic for a controlType RTF's own vocabulary does not cover.
Moves form fields from the gap table into the Read scope table now that \*\formfield reads as a real contentControl, narrows the remaining content- controls gap to the controlTypes RTF's own form-field vocabulary does not cover, and updates the fidelity-constructs section and the write-side notes to match.
… a controlType it can't mint writeFormFieldBoundaries's close loop wrote a field's closing "}}" for any extent reaching its endRun, without checking whether that extent's open half was ever written. A controlType outside plainText/checkbox/dropDown (richText, comboBox, date, and the rest) degrades through a diagnostic in the open loop and writes no braces at all, so the close loop still emitting "}}" for it left two unmatched closing braces in the output, corrupting the brace balance of everything written afterwards in the document. Track which extents actually had their open half written in a per-paragraph Set, and have the close loop consult it before emitting "}}". Adds a brace-counting test helper (src/test-support/brace-balance.ts) and two regression tests: one pinning that the unrepresentable-controlType case now mints balanced braces, another mixing a real (checkbox/dropDown) and an unrepresentable (comboBox) field in the same paragraph.
formFieldPayload never wrote \fftype -- RTF 1.5's own Form Fields table control word naming a
field's actual type ("Form field type: 0 Text 1 Check box 2 List") -- so every minted
\*\formfield implicitly declared itself a text field regardless of its real controlType. A minted
checkbox came out as e.g. {\*\formfield{\ffdefres1{\*\ffname Check1}}}, whose form-field data says
"text" while its sibling \*\fldinst separately says FORMCHECKBOX -- an internal inconsistency that
happened not to break round-tripping only because this codec's own reader ignores \fftype too.
Fold the instruction keyword and its \fftype number into one FORM_FIELD_SPEC table (renamed from
FORM_FIELD_KEYWORDS) so formFieldPayload can prepend the correct \fftypeN for the descriptor's real
controlType: 0 for plainText, 1 for checkbox, 2 for dropDown.
The write-side test suite had no structural-validity assertion on minted output at all, which is exactly why the unbalanced-brace defect fixed two commits back slipped past a suite that already exercised the triggering code path. Apply the brace-counting helper more broadly: the top-level <File>-production test, the hyperlink field test, the table-row test, and every remaining form-field write test now assert the output's braces balance, not just contain expected substrings.
… state formFieldContentControl preferred resultIndex (\ffres) over defaultResultIndex (\ffdefres) when deriving a checkbox's checked state. Real PHPRtfLite output writes the constant \ffres25 on every checkbox it produces regardless of the box's actual state (\fftype1\ffres25\ffhps20\ffdefres0 for an unchecked box, \fftype1\ffres25\ffhps20\ffdefres1 for a checked one, side by side) -- only \ffdefres actually varies with the real state, so preferring \ffres read both fixtures as checked. RTF 1.5's own Form Fields table defines both purely in list-field terms and neither mentions a checkbox's checked state at all: "\ffresN Result field for a form field. Values from 0 to N-1, where N is the number of \ffl entries." and "\ffdefresN Default entry for list field (for example 0 = first list item, 1 = second list item)." \ffdefres's own wording reads as consistent with a checkbox modelled as a two-entry list (0 = unchecked, 1 = checked), while \ffres's wording has no defined meaning for a checkbox, which carries no \ffl entries to index into. Prefer defaultResultIndex, falling back to resultIndex only when a producer supplies no \ffdefres at all. Corrects read.ts's own comment, which quoted "Current result of the form field" and "Default result" for \ffres/\ffdefres as if verbatim -- neither phrase appears in the real specification.
writeFormFieldBoundaries only ever runs for positions 0..paragraph.runs.length, so a form-field extent whose endRun falls outside that range never reaches a position where its close would fire: an endRun beyond runs.length has no later position to match it, and an endRun before its own startRun has already passed by the time the open is written. Both left an opened field group with no matching close, corrupting brace balance for the rest of the document. opened now tracks exactly the extents currently open with no close written yet (entries are removed the moment their close is emitted), and writeParagraph drains whatever remains open after its final boundary pass, emitting the missing close for each. This makes the writer structurally incapable of leaving a field group unmatched regardless of what ranges an extent is handed.
…, not a constant to skip [MS-DOC] 2.9.78 FFDataBits defines a checkbox's iRes explicitly: 0 (unchecked), 1 (checked), or the reserved value 25 meaning undefined, treated as unchecked. Preferring \ffdefres over \ffres outright (rather than handling the sentinel) happened to read PHPRtfLite's own always-25 output correctly, but inverts a real Word checkbox that carries a meaningful \ffres alongside a \ffdefres that differs from it -- exactly the case a producer emits once a field's live state diverges from its reset default. \ffres now wins whenever it isn't the sentinel; only the sentinel, or \ffres being absent altogether, falls through to \ffdefres, which itself defaults to unchecked when absent too.
The writer minted only \ffdefres for a checkbox, never \ffres -- and \ffres, not \ffdefres, is what a reader actually treats as the field's live state per [MS-DOC] 2.9.78 FFDataBits. An absent \ffres reads as 0, so a checkbox this codec wrote as checked opened unchecked in Word regardless of what \ffdefres said. Both control words are now written with the same value: \ffres for the current state a real reader consults, \ffdefres mirroring it as the field's reset default, since ContentControlDescriptor carries one `checked` boolean rather than a separate default.
\ffres names a checkbox's checked state under iTypeChck, but the same FFDataBits field carries a zero-based index into \*\ffl under iTypeDrop -- a dropdown's own currently selected entry, which this codec previously read into `options` but discarded entirely. It now lands on the contentControl descriptor's `value` field when the index names a real entry, and the writer mints \ffres back from `value`'s position in `options` when it names one, leaving a dropdown with no recorded selection writing exactly what it always has.
… ffdefres formFieldContentControl only ever consulted a dropDown control's \ffres for its selected \ffl index, never \ffdefres, so a real-world producer that always emits FFDataBits's own 25 "undefined selection" sentinel alongside a meaningful \ffdefres (confirmed against a PHPRtfLite fixture) lost its recorded selection entirely. Apply the identical sentinel-then-default fallback the checkbox branch already used, and correct two comments that had drifted from what the code actually does: the dropdown bounds-check comment cited the coincidental sentinel/list-length overlap as its reason rather than FFDataBits's own "25 means undefined" semantics, and the checkbox sentinel comment quoted the spec's bare "undefined checkboxes are unchecked" wording without acknowledging that the code deliberately prioritises a defined \ffdefres over that default for real-world fidelity.
…n form field
formFieldPayload wrote \fftype2, an optional \ffres, and the {\*\ffl ...}
entries for a dropDown control, but never \ffhaslistbox or \ffdefres.
[MS-DOC] 2.9.78 FFData requires fHaslistbox to be 1 whenever iType is
iTypeDrop, and requires wDef to exist whenever iType is iTypeChck or
iTypeDrop -- a minted dropdown was missing both, leaving a real Word
reader with no recorded default for a list-type form field. ffdefres now
mirrors the selected entry's own index (falling back to the first entry
when no selection is recorded), the same way the checkbox branch already
mirrors its single `checked` boolean into both ffres and ffdefres.
…form field A dropDown with no options at all (a real, common shape: a docx w:dropDownList/w:comboBox with no listItem children, or any ODF form:listbox, currently read back with no options recorded at all) was gating \ffhaslistbox behind options !== undefined, so it wrote a bare \fftype2 with none of the \*\formfield data FFDataBits.fHasListBox requires for iTypeDrop regardless of list size. \ffhaslistbox is now minted whenever fftype names a dropdown, options or not. \ffdefres is now minted only when there is a genuinely valid index to write: the field's own recorded selection when value names one of options, or entry 0 (RTF's spec-forced first-entry default) when no selection was recorded at all. It is omitted, rather than fabricated, both when options is undefined or empty (FFData.wDef "MUST be less than the number of items in the dropdown list box", and 0 is not less than 0) and when value names none of options -- the previous code's indexOf returning -1 for both "no value" and "value not found" made those two cases indistinguishable, so an unmatched value (e.g. options ["Hello","Guten Tag"] with value "Bonjour") silently wrote ffdefres0 and read back as the wrong selection, "Hello".
…comments The dropdown branch's own comment pointed at "the checkbox branch six lines above" -- a line count that had already drifted to seven or eight lines by the time this was written, and rots again on the next edit either branch gets. It now names the checkbox branch's own `current` computation instead of counting lines. The RtfFormFieldData doc comment described iRes's checkbox meaning as "0/1/25-undefined" but its dropdown meaning as a bare "zero-based \ffl index", omitting the identical 25 sentinel that formFieldContentControl's own dropdown branch (60 lines below) already handles. Both mentions of the same field now describe the sentinel the same way.
…elected value Writing a dropDown with no recorded selection minted a synthetic \ffdefres0, while writing one whose value matched no option minted neither \ffres nor \ffdefres -- two different outputs for two states that read back identically as value:undefined. A second write of that read-back document then always hit the \ffdefres0 branch, silently turning a genuinely unmatched value into a confident (and wrong) selection of the first option across a second write-read pass. Both cases now converge on omitting \ffres and \ffdefres entirely: this codec's own read-side fixture already proves RTF tolerates that omission and decodes it as an unset value, so there is no need to invent a first-entry default nobody actually selected. [MS-DOC] 2.9.78 FFData.wDef's own "MUST exist if and only if" rule is a real MS-DOC production requirement this codec deliberately does not always satisfy here, matching real-world producer behaviour the existing read-side test already tolerates.
…nostic sink Dropping a recorded value that names none of a dropDown's own options is real, signalable data loss, not an absence -- distinct from a field that never had a selection recorded at all. This routes that drop through the same RtfDiagnosticCodes.CONSTRUCT_UNREPRESENTED sink every other unrepresentable construct in this writer already reports through, rather than staying silent.
…itation
Appendix B's "Toggle" definition does state a default for an omitted
parameter ("no parameter or a nonzero parameter is used to turn on the
property") -- the previous comment's ellipsis elided exactly that clause
while claiming Appendix B states no default at all. Only "Value" is
actually silent on a default, which is why \ffownhelp/\ffprot (both Value
words) need the separate "Change Formatting Property" rule to settle
their own 0-default.
Also corrects where \b's own bare-word meaning is actually stated: the
"Font (Character) Formatting Properties" section's own preamble,
immediately before \b's row in that same section's table -- not, as
previously claimed, a preamble "two sections earlier". The sentence the
comment had quoted for that claim is real, but belongs to a different,
much earlier section (the spec's own Introduction, under "Control Word"),
illustrating the general toggle convention rather than \b's own
table-adjacent meaning.
…substring
toContain("\\ffhaslistbox") passes identically whether the writer emits
\ffhaslistbox1 or the wrong \ffhaslistbox0, so these four assertions never
actually guarded the regression they were written for -- a dropDown with
no options, or an empty options array, silently reverting to a bare or
zeroed control word. Tightened to the specific string these tests already
name in their own titles.
A checkbox's own value and a dropDown's selected value both fired their respective diagnostics for an empty string, while a plainText value (and alias/tag before it) already treated an empty string as carrying nothing worth preserving. Same field, same underlying reasoning, three different behaviors. Applies the plainText/alias/tag rule uniformly: an empty string reads as "never recorded", not as a genuine present-but-empty value, across every value-shaped field this writer handles.
…mpty-string option The previous empty-string-as-absent fix folded descriptor.value.length === 0 into the dropDown's selectedIndex computation itself, not just its diagnostic-suppression check. That silently discarded a genuinely selected value whenever a dropDown's options list contains the empty string as a real, indexable entry and the current value is that same empty string -- neither \ffres nor \ffdefres was minted, with no diagnostic, even though index 0 is a fully valid selection target. options.indexOf already distinguishes an empty string matching a real option (a genuine selection) from one matching nothing (the case the diagnostic-suppression guard below still handles on its own, unchanged). Removing the extra length check from the selection computation restores the prior, correct behaviour for the matching case while keeping the non-matching case free of a spurious diagnostic. This codec's own reader can produce a value:'' descriptor from real RTF bytes (a dropdown whose current selection is a blank list entry), so a read-then-write round trip of a document this package itself emits was silently losing the selection.
…default
LibreOffice's RTF exporter (sw/source/filter/ww8/rtfattributeoutput.cxx)
emits \ffownhelp unconditionally with no numeric parameter, immediately
before a {\*\ffhelptext ...} destination that carries the control's real
HelpText property. Reading that bare form via the same formFieldValueBit
used for \ffprot applied the Value-word spec default of 0/false, so a
genuinely author-set alias was silently discarded for this producer with
no diagnostic, even though the writer already mints \ffownhelp1 whenever
it round-trips a non-empty alias.
\ffownhelp is now read via toggleValue instead, matching a bare \b/\i:
a bare occurrence reads as true, an explicit \ffownhelp0 still reads as
false, and a field that never mentions \ffownhelp still defaults to
false. \ffprot keeps the literal Value-word default, since no comparable
real-producer evidence exists for its own bare form.
…ent RTF grammar production RTF's own "Form Fields" table (RTF 1.5/1.6, and by inheritance 1.9.1, which changed none of the control words involved here) is a flat control-word list with no grammar production of its own, unlike the Fields/Bookmarks/Index Entries/Table of Contents sections immediately around it, each of which carries a real <production> built from the spec's own Formal Syntax operators. No RTF revision fetched (1.5, 1.6) defines a <formfield>/<formparams>/<formstrings> nonterminal, and a GitHub-wide search turns up no such production anywhere either. formFieldPayload's own field order is this writer's deliberate, deterministic convention for internal consistency, not something extracted from a spec production -- the comments, test names, and README now describe it that way instead of citing a production that does not exist.
…uct column Shortening the content-controls row's own text left that column one character narrower than its padding, so every row's trailing space count no longer matched -- eslint's markdown-table formatter reflows the whole column to the new widest cell.
… token FORM_FIELD_CHECKBOX/DROPDOWN/TEXT_INSTRUCTION used a bare \b-anchored regex, so formFieldControlType matched FORMTEXT/FORMCHECKBOX/FORMDROPDOWN anywhere in a field's instruction string, not just when the field actually was one. A HYPERLINK field whose target happens to contain one of those words (e.g. a URL path segment) was silently wrapped in a fabricated legacy form field content control, with the correct hyperlink still produced alongside it but a spurious construct extent added. RTF's own field-instruction grammar is "<field-type> [switches]", so the type keyword only ever legitimately opens the instruction. Anchoring each regex on the (whitespace-trimmed) start of the string, mirroring how HYPERLINK_TARGET already requires a following quoted target rather than matching HYPERLINK anywhere, removes the false match without affecting any genuine form field, whose instruction always starts with its own keyword.
applyFormFieldControlWord stored `param` directly for \ffres/\ffdefres, so a bare, unparameterised occurrence of either control word set resultIndex/defaultResultIndex to undefined -- indistinguishable from the control word never appearing in the group at all. formFieldContentControl treats undefined as "no value recorded here, fall through to the next source" (the sentinel-then-\ffdefres fallback FORM_FIELD_RESULT_UNDEFINED drives), so a bare \ffres was silently read as if absent rather than as the real \ffres0 the RTF Value-word convention says it spells, letting an unrelated \ffdefres win over a producer's actual (bare) checkbox state. \ffres/\ffdefres share \ffprot's own Value-word classification in RTF 1.9.1's Appendix B, and "Conventions of an RTF Reader"'s own "Change Formatting Property" entry gives every Value word with no stated default a 0-default for a bare occurrence -- the same rule formFieldValueBit already applies to \ffprot. formFieldValueNumber applies the identical rule to \ffres/\ffdefres, so a bare occurrence now stores 0 and only the word's total absence still yields undefined.
The checkbox and plainText branches of formFieldPayload fired the CONSTRUCT_UNREPRESENTED diagnostic whenever `descriptor.options !== undefined`, so a descriptor carrying `options: []` reported a checkbox or text field's "options list (0 entries) is dropped" even though nothing was actually dropped -- an empty array names no choices to lose. This function already treats an empty value-shaped field as "never recorded" rather than "present but empty" everywhere else (`value`, `alias`, `tag`); the two `options` checks were the one place left gating on presence alone instead of `.length > 0`. Both now match that established rule.
writeFormFieldBoundaries reused describeConstructGap's "contentControl" case for a controlType FORM_FIELD_SPEC has no entry for (richText, comboBox, date, and the rest), producing "RTF has no block-scoped structured-document-tag equivalent ..." -- but the extent being reported on here is already run-scoped by construction: it never reaches openConstruct/closeConstruct's block-scoped handling at all, so leading with "no block-scoped equivalent" states a fact that was never the reason for this particular drop. describeFormFieldGap replaces that call with a message stating the actual reason: RTF's \*\formfield vocabulary has no spelling for the extent's own controlType. describeConstructGap keeps its original wording for its own call site (openConstruct), where the construct genuinely is block-scoped and "no block-scoped equivalent" is accurate.
…unconditional
The comments justifying reading a bare \ffownhelp as true (rather than
the Value-word literal 0-default \ffprot's bare form uses) described
LibreOffice's RTF exporter as emitting the bare control word
"unconditionally". Reading sw/source/filter/ww8/rtfattributeoutput.cxx
directly shows every one of its three FFOWNHELP emission sites sits
inside `if (xPropSetInfo->hasPropertyByName("HelpText"))` -- a
property-existence check on the control model, not a literal
unconditional emission.
The load-bearing half of the citation (the control word comes out
BARE, with no numeric parameter, whenever it does fire) is accurate
and the reader behaviour it justifies is unaffected; only the
"unconditionally" framing overclaimed. Reworded across constructs.ts,
read.ts, read.test.ts, and the README to describe the actual gate.
… claiming none exists
RTF 1.9.1's own "Form Fields" section gives a genuine Formal Syntax
production for <formfield>, <formparams>, and <formstrings>, each
built from the spec's own operators exactly like the Fields,
Bookmarks, and Table of Contents sections around it. The prior
comments claimed the opposite -- that the Form Fields table is a flat
control-word list with no grammar production and no <formfield>
nonterminal at all -- which is true of RTF 1.5 but not of 1.9.1, the
version these comments explicitly name.
The distinction matters beyond citation accuracy: <formparams> and
<formstrings> use the spec's plain-juxtaposition operator ("item A
followed by item B"), not its "&" any-order operator the neighbouring
Fields production uses, so the spec actually mandates a fixed member
order. This writer's own emission order already happens to be a valid
subsequence of that spec-mandated order, but the false "no grammar
constrains this" claim told a future maintainer the order was an
arbitrary house convention free to reorder, which would have silently
broken conformance.
startFormField() ran unconditionally at every {\field's own open, and
both it and endFormField() force-flush the run being built regardless
of whether the field is a genuine form field. Since an ordinary field
(PAGE, DATE, NUMPAGES, REF, SEQ, TOC, MERGEFIELD, and the rest) has no
\*\formfield extent to open, this flushed text on both sides of every
such field for no reason, splitting what would otherwise coalesce
into one run into three -- undetectable by inspecting joined run text
alone, only by counting runs.
formFieldControlType can only tell a genuine form field apart from an
ordinary one once the field's own \*\fldinst instruction is complete,
which happens at that destination's own close, not at {\field's own
open (where the instruction is still empty). Moving the open there,
and gating endFormField on formFieldContentControl actually returning
a descriptor, means an ordinary field never touches the run-flushing
machinery at all, while a genuine form field's own extent boundary
lands in exactly the same place as before -- no text is ever appended
between {\field's open and \*\fldinst's close.
…eads back RtfFormFieldData.defaultText was parsed from \*\ffdeftext on every read (its own destination kind, state field, and accumulation logic), but nothing outside its own declaration ever read the value back: formFieldContentControl deliberately never promotes a plainText field's default/reset text onto the contentControl descriptor, since `value` names the field's current content and \ffdeftext names a different fact entirely, and no other exported function exposes the raw \*\formfield data either. Collecting it served no caller. \*\ffdeftext now reads as a recognised, deliberately-skipped destination (SILENT_SKIP_DESTINATIONS) instead of a captured-but-dead field, so a real producer's default text still reads as "known and intentionally unused" rather than triggering an "unrecognised destination" diagnostic it never used to trigger either.
…tead of mis-nesting both
Two contentControl run-construct extents that cross -- neither nests
inside or around the other, e.g. {startRun:0,endRun:2} alongside
{startRun:1,endRun:3} -- have no valid brace sequence in RTF at all.
writeFormFieldBoundaries closed whichever extent's endRun matched the
current position by scanning an unordered Set, rather than closing
the actual innermost still-open group; for a crossing pair this closes
the wrong extent's braces at both ends, producing output that is
brace-balanced overall but mis-nested throughout -- one extent's runs
end up nested inside the other's, and a run outside both extents'
union can end up trapped inside one of them.
selectNestableFormFields now filters the extent list per paragraph,
before any bytes are written, keeping a maximal properly-nestable
subset (sorted by startRun ascending, endRun descending, walked with
an explicit stack) and reporting the dropped, crossing extent through
the same diagnostic sink every other unrepresentable construct uses.
`opened` in writeFormFieldBoundaries is now an ordered stack rather
than a Set, so the close loop always closes the true innermost extent
even when two extents share a boundary -- properly nested or simply
tied -- rather than whichever one the array happens to list first.
…plitting the document
The formField/formFieldName/formFieldHelpText/formFieldListItem destinations had
no early return guarding against paragraph, section, and structure control
words, unlike their sibling bookmarkStart/bookmarkEnd destinations.
A `\par` inside `{\*\ffname ...}`, `{\*\ffhelptext ...}`, or `{\*\ffl ...}` fell
through to applyStructureControlWord and split the surrounding paragraph, and a
`\page` inside any of the four injected a spurious top-level pageBreak block.
Mirror the existing bookmarkEnd guard: none of the four carries formatted
document flow, so a stray control word inside them is ignored rather than
applied to the enclosing paragraph/section.
… destinations ffdeftext was the only <formstrings> destination string mapped to a silent skip; its four siblings from RTF's own Form Fields table (ffformat, ffstattext, ffentrymcr, ffexitmcr) were left unmapped, so a fully-populated real-world form field produced an UNKNOWN_DESTINATION_SKIPPED diagnostic for each of them where the group as a whole previously produced none. None of the four has a ContentControlDescriptor field to land in, so each is skipped whole for the identical reason ffdeftext already is. Also corrects the README's own description of ffdeftext, which read as "parsed but never surfaced" when the group's content is in fact skipped whole and never captured at all.
…trol extents selectNestableFormFields already drops a contentControl extent that crosses another in the same paragraph, with its own CONSTRUCT_UNREPRESENTED diagnostic, but the README's write-side enumeration and its "Deliberately not handled" table never mentioned it alongside every other write-side drop, including the analogous row for a bookmark straddling a table cell wall.
…ans a paragraph or cell
RTF 1.9.1's own <fieldrslt> production ('{' \fldrslt <para>+ '}') is
grammatical for a multi-paragraph field result even though real producers
keep a form field inline, and a RunConstructExtent is scoped to one
paragraph's own runs. When \fldrslt content crosses a \par or \cell,
endFormField silently dropped the field's contentControl with no
diagnostic, unlike every other drop path in this feature. It now reports
through the sink (rtf/form-field-span-dropped) so the loss is visible,
and the two comments that previously called the cross-paragraph case
pathological/impossible are softened to match the grammar.
…ntentControl twice
A real Word-authored \field wraps its own \*\fldinst instruction text in
an anonymous nested group (e.g. {\*\fldinst {FORMTEXT }...}), and that
nested group inherits the enclosing "fieldInstruction" destination just
like \*\fldinst itself does. Both the nested group's own close and
\*\fldinst's own close therefore satisfied startFormField's condition,
opening two extents for what is really one field while only the field's
single closing brace ever popped one back off, leaking one openFormFields
entry per such field. FieldState now carries a formFieldStarted flag,
shared by reference across the whole \field group's subtree, so the
extent opens only once regardless of how many of its ancestor groups
share the "fieldInstruction" destination.
…se loop writeFormFieldBoundaries's close loop pops the top of the opened stack while top.endRun === position, additionally requiring top.startRun !== position. No entry on that stack can ever have startRun === endRun: such an extent is pushed and immediately popped again within the same call (the open loop below pops it right back off once it sees endRun === position), so it never survives to be seen by this loop on a later call. The second conjunct can therefore never be what makes the loop condition false, and is dead code.
…elptext/\ffname
constructs.ts's own formFieldContentControl trims \ffname/\ffhelptext
before gating on them when reading a form field back, so a whitespace-only
value reads as absent. The writer gated on the untrimmed length instead,
so a whitespace-only alias/tag still minted a real \ffownhelp1/{\*\ffhelptext}
or {\*\ffname} destination on the way out -- content the reader would then
drop on the way back in, an asymmetric round trip. Trim before the length
check on both fields to match the reader's own convention.
…ot a re-derived match startFormField fires at a `\*\fldinst`-destination group's own close as soon as a partial instruction names a form-field keyword, but a real instruction can keep growing afterward: a nested anonymous group can close exactly on the keyword, satisfying its \b word boundary at the end of the string read so far, before more text is appended with no separator. Once the full instruction is read, formFieldContentControl re-derives the keyword match from scratch, and \b anchoring means a suffix appended with no separating whitespace can make a previously-matching prefix stop matching. endFormField was only called when that second, independent match still held, so a field could open its extent and never close it, leaking an unpopped entry on the shared openFormFields stack. Gate the \field group's own close on FieldState.formFieldStarted instead, and let endFormField decide what a missing descriptor means: pop the stack unconditionally so open and close stay provably paired, and report the dropped contentControl through the diagnostic sink like every other drop this feature makes, rather than discarding it silently.
Its only caller is expectBalancedBraces in the same module; nothing else in the workspace references it, so the export was speculative public surface with no consumer.
Mearman
force-pushed
the
feat/rtf-codec-formfield-content-control
branch
from
September 6, 2026 15:34
2f471dc to
ddb4f00
Compare
Contributor
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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 #934
RTF's
\*\formfielddestination (RTF's own analogue to a Word content control) now reads and writes as document-schema.js'scontentControlconstruct, matching howooxml.jsalready maps docx's legacyw:ffDataonto the identical construct kind.Read. The reader derives
controlTypefrom the field's own instruction keyword (FORMTEXT/FORMCHECKBOX/FORMDROPDOWN), which is robust even for a legacy field carrying no\*\formfieldgroup at all, and layers on whatever\*\formfielddata is present:\*\ffnameas the control's tag, a checkbox's\ffres/\ffdefresas its checked state (current overriding default), and a dropdown's\*\fflentries as its options, with the identical\ffres/\ffdefrespair deciding its selected entry the same way. A form field is always inline — one\fieldgroup, never spanning a paragraph boundary — so the construct rides aRunConstructExtenton the paragraph exactly like a revision mark, never a block-levelconstructStart/constructEndpair. The field's own displayed text is not duplicated onto the descriptor: it already rides the ordinary runs the extent wraps (the\fldrsltcontent), mirroring how ooxml.js leaves a text input'svalueunset for the same reason.Write. Mints a real
\*\formfieldproduction for the three controlTypes RTF's own vocabulary actually covers (plainText/checkbox/dropDown); any other controlType still degrades through the same construct-gap diagnostic every other unrepresentable construct already uses, withdescribeConstructGap's message updated to say so. A dropDown mints\ffhaslistboxunconditionally, whether or not it carries any options — [MS-DOC] 2.9.79 FFDataBits.fHasListBox must be set for a list-type field regardless of list size — and mints\ffdefresonly when a genuinely valid index exists to write, rather than fabricate one for an empty options list or silently substitute a different entry for avaluethat names none ofoptions.Tests. Read-side fixtures are trimmed from a real producer's own output (PHPRtfLite), braces and all, including the anonymous scoping group
\*\formfieldwraps its own control words in — the reader never needs special-casing for that group, since an unrecognised first control word simply inherits the enclosing destination, the same mechanism an ordinary{\b bold}run-formatting group already relies on. Write-side tests check the minted production directly, round-trip each of the three controlTypes back through the reader, and cover the empty/absent-options and unmatched-value edge cases explicitly.Docs. README moves form fields from the gap table into the Read scope table, narrows the remaining content-controls gap to the controlTypes RTF's own form-field vocabulary doesn't cover, and updates the fidelity-constructs section and write-side notes to match.
Verification
pnpm exec turbo run _lint _typecheck _build _test --filter=rtf-codec— all clean, 285 tests passing.Follow-up filed
form:listboxmapping both drop a dropdown/listbox's own options when its list is empty (ooxml.js) or unconditionally (odf.js never reads them at all), tracked separately since it is a read-side gap in two sibling packages rather than this PR's own write-side fix.