fix: emit CT_Lvl child elements in ECMA-376 schema order

[pplupo]

Summary

• DOCX bullets/numbered lists were misclassified in Microsoft 365 Online because CLvl::toXML() (and two other independent writers: RTF→DOCX, HWP→DOCX) emitted <w:lvl> child elements out of the ECMA-376 CT_Lvl schema sequence.
• Reordered emission to match the schema in all three writers; no behavior/field changes, purely ordering.
• Added docx_numbering_test GoogleTest suite covering the primary writer.

References Euro-Office/DocumentServer#253

AI disclosure

Investigation, fix, and test-suite drafting were assisted by Claude Code (Claude Sonnet 5). All changes reviewed and submitted by the undersigned contributor.

Test plan

• ctest -R docx_numbering_test — all pass
• Manual M365 Online verification (ordered, unordered, nested, customised markers, round-trip, repair-on-resave, Desktop Editors shared-submodule build)
• No regression in Google Docs / LibreOffice

[chrip]

Summary

This reorders <w:lvl> children to the ECMA-376 CT_Lvl sequence in three DOCX-numbering
writers and adds a GoogleTest suite for the primary one. The diagnosis holds up, the schema
sequence quoted in the commit message matches ECMA-376, and the reordering is
behavior-preserving in all three writers. Two things block merge: the Linux build job is
red because the PR invalidates three committed conversion snapshots that were not
regenerated, and the two new files carry no license header. One writer of the same defect
class was also missed by the audit.

Verification

Claim / Item	Reality	Status
ECMA-376 CT_Lvl sequence is start, numFmt, lvlRestart, pStyle, isLgl, suff, lvlText, lvlPicBulletId, legacy, lvlJc, pPr, rPr	Correct, and MsBinaryFile/DocFile/NumberingMapping.cpp:575-660 already emits exactly that sequence, so the tree has an in-repo reference implementation	✓
Numbering.cpp new order matches the schema	OOXML/DocxFormat/Numbering.cpp:329-347 emits all twelve in sequence	✓
RTF writer new order matches the schema	RtfFile/Format/RtfProperty.cpp now emits start, numFmt, lvlRestart, isLgl, suff (the m_nFollow block), lvlText, lvlPicBulletId, lvlJc, pPr, rPr. pStyle and legacy are never emitted by this writer, so their absence is fine	✓
RTF reorder is behavior-preserving	GetLevelTextOOX() is pure (builds a local string, mutates no member) and sText has no other use later in the function, so moving its call down changes nothing but output order	✓
HWP fix touches only the BULLET branch	The EHeadingType::NUMBER branch already emitted start, numFmt, suff, lvlText, lvlJc, rPr, which is in sequence. Only the BULLET branch was wrong	✓
fromXML() needs no change, so open-and-resave repairs old files	CLvl::ReadElements() dispatches on oReader.GetName() in an if/else chain with no positional assumption	✓
Every writer of this construct was audited	RtfFile/Format/RtfOldList.cpp was missed. See Major 3	❌
ctest -R docx_numbering_test passes	CI step 12, "Unit tests (CTest)", passed, so the suite builds and its cases are green	✓
No regression elsewhere	CI step 13, "Conversion Test", failed on three snapshot diffs. See Blocking 1	❌
GTEST_MAIN is a real add_core_gtest option	common.cmake:466 declares it and writes a bundled entry point	✓
DCO, AI disclosure, Assisted-by: trailer	All present. Signed-off-by on the commit, DCO check green, disclosure in the PR body, Assisted-by: ClaudeCode:claude-sonnet-5 on the commit	✓
Euro-Office/DocumentServer#253 exists	Open, "Bullets docx not displayed correctly in m365 online"	✓

Issues & Suggestions

🔴 Blocking

• Blocking 1: three conversion snapshots are now stale and CI is red. The Linux build
  job failed at step 13, "Conversion Test"
  (run 36094231170).
  Test/Applications/x2tTester/conversionTest.sh runs diff -r against committed
  byte-for-byte references, and the RTF writer reorder changes the RTF to DOCX output for
  all three word-processing fixtures. The failure is reproducible, and the references are
  what need updating:

  Test/Applications/x2tTester/out_word_proc_snap/empty.rtf/docx/word/numbering.xml
  Test/Applications/x2tTester/out_word_proc_snap/medium.rtf/docx/word/numbering.xml
  Test/Applications/x2tTester/out_word_proc_snap/fonts_and_images.rtf/docx/word/numbering.xml
  

  Each still opens <w:lvl w:ilvl="0"><w:lvlJc w:val="left"/><w:lvlText w:val="%1"/><w:numFmt w:val="none"/><w:start w:val="1"/><w:suff w:val="nothing"/>...,
  the pre-fix order. Please regenerate them in this PR. The failure also caused steps 16 to
  23 to be skipped, so the DocumentServer e2e suite never ran on this branch. The test
  plan's ctest -R docx_numbering_test line is accurate, but it only covers the new suite;
  the conversion suite is a separate CI step.

  The odt and docx fixtures are untouched, which fits: only the .rtf and .odt snapshots
  contain a numbering.xml at all, and the ODF writers in
  OdfFile/Reader/Format/styles_list.cpp were already in sequence. That also means
  CLvl::toXML(), the primary fix, has no conversion-snapshot coverage and rests entirely
  on the new unit suite.

• Blocking 2: the two new files have no license header.
  OOXML/DocxFormat/test/test.cpp:1 starts at #include "gtest/gtest.h" and
  OOXML/DocxFormat/test/CMakeLists.txt:1 at cmake_minimum_required. This fork already
  has the right precedent for a file written from scratch:
  OdfFile/Test/number_formats/main.cpp:1 is a plain
  // SPDX-License-Identifier: AGPL-3.0-only. Please add, to test.cpp:

  /*
   * SPDX-FileCopyrightText: 2026 Euro-Office contributors
   * SPDX-License-Identifier: AGPL-3.0-only
   */

  and the #-comment equivalent to the CMake file. Please do not copy the
  (c) Copyright Ascensio System SIA 2010-2019 block from neighbouring files such as
  OOXML/test/common.cpp: it attributes fork-authored work to Ascensio and carries the
  AGPL section 7(a) and CC-BY-SA clauses that are specifically ONLYOFFICE's.
  AGPL-3.0-only rather than -or-later, because the upstream grant has no or-later
  clause. In fairness the tree is not uniform here (OOXML/test/main.cpp has no header
  either), but I would still add headers on the new files.

⚠️ Major

• Major 3: one writer of the same defect class was missed. The commit message says the
  fix came out of "auditing every writer for this construct", so it is worth naming the one
  that got through. RtfFile/Format/RtfOldList.cpp:56-95
  (RtfOldList::RenderToOOX, the RENDER_TO_OOX_PARAM_OLDLIST_ABS branch) emits
  numFmt, lvlText, pPr, rPr, lvlJc, putting lvlJc (position 10) after both
  pPr (11) and rPr (12). Same schema violation, same RTF to DOCX pipeline, same file
  tree as the writer this PR already fixes.

  I checked the rest and they are clean, so this looks like the last one:
  MsBinaryFile/DocFile/NumberingMapping.cpp (both w:lvl writers),
  OdfFile/Reader/Format/styles_list.cpp (all four, each calling
  docx_serialize_level_justification between lvlText and pPr),
  HtmlFile2/Writers/OOXMLWriter.cpp:160-172, the HWP NUMBER branch, and the HWP
  fallback block in CNumberingConverter::SaveToFile.

ℹ️ Minor / 💡 Suggestions

• Minor 4: the test fixture exercises only half the sequence. BuildLvl() in
  OOXML/DocxFormat/test/test.cpp:18-50 initialises start, numFmt, isLgl, suff,
  lvlText and lvlJc. The other six (lvlRestart, pStyle, lvlPicBulletId, legacy,
  pPr, rPr) are never emitted, so SchemaOrderPositions() records npos for them and
  they get filtered out before the ordering assertion. That leaves the pStyle and pPr
  inversion, which the old code also had and this PR also fixes, with no test behind it.
  One more case building a fully populated CLvl would cover the whole sequence.

• Minor 5: the test comments point at documents that are not in the repo.
  test.cpp references "Feature 001-docx-list-numbering", "research.md Finding 4",
  "T004/T005/T007", "T012/T017", "FR-004" and "FR-011". None of these resolve anywhere in
  core, so a future reader has no way to follow them. The comments are useful otherwise;
  I would keep the explanation and drop the identifiers, or say what each one meant.

• Minor 6: TESTING.md was not updated. It keeps a curated "Done:" checklist of every
  suite wired into CTest, with a note per suite on its dependencies and any quarantine. The
  new docx_numbering_test is registered in CMakeLists.txt:39 but does not appear there.
  It is also the first entry that is a new suite rather than a .pro migration, which is
  worth a line of its own.

• 💡 Suggestion 7: the HWP bullet branch now switches twice on the same expression.
  HwpFile/HwpDoc/Conversion/NumberingConverter.cpp has one switch (shIndex % 3) for
  lvlText and a second for rPr, and the two sets of branches have to stay in step by
  hand. Picking the glyph and the font name into two locals in a single switch, then
  writing them at their schema positions, would keep the pairing visible. Optional, and it
  does not affect output.

Verdict

Request changes. The analysis and the fix are sound, and the RTF, HWP and CLvl
reorders all check out against the schema and against NumberingMapping.cpp's existing
correct implementation. But CI is red on a snapshot the PR itself invalidates, so
regenerating the three out_word_proc_snap files is required before this can go in, along
with the license headers on the two new files. Major 3 is a judgement call and could be a
follow-up, though since it is one more instance of a defect the PR already fixes twice,
folding it in seems cheaper than tracking it.

Assisted-by: ClaudeCode:claude-opus-5

[pplupo]

Pushing the Blocking 2 (license headers), Major 3/task-80-adjacent wording, and Minor 4-6 changes now, plus the regenerated out_word_proc_snap fixtures for Blocking 1 to get some verification while I'm testing a few things in parallel, to speed up things.

[pplupo]

Thanks for the thorough review — the findings check out. Status on each:

Blocking 1 (stale snapshots): regenerated and pushed. Rebuilt x2t with the fix, re-ran conversionTest.sh, and updated out_word_proc_snap/{empty,medium,fonts_and_images}.rtf/docx/word/numbering.xml to match — schema order now start, numFmt, suff, lvlText, lvlJc, pPr, rPr, confirmed against the current committed fix.

Blocking 2 (license headers): added SPDX headers to test.cpp and CMakeLists.txt, following the OdfFile/Test/number_formats/main.cpp precedent (AGPL-3.0-only, not -or-later, per your note on the upstream grant).

Major 3 (RtfOldList.cpp): confirmed the schema violation — lvlJc after pPr/rPr in RtfOldList::RenderToOOX(). Decided not to fold it into this PR. Reasoning: numFmt there is a hardcoded constant already at the correct schema position, so unlike the writers this PR fixes, there's no ambiguous-numFmt failure mode for it to cause — a schema-order assertion test would fail (confirming the violation exists) but wouldn't demonstrate any M365-observable regression, since there isn't one. It's also a different, legacy RTF list mechanism (pre-\listtable) not exercised by any of this PR's manual RTF tests. Filed and documented as a known, deliberately-deferred issue rather than fixed here — happy to discuss if you'd rather see it folded in regardless.

Also worth flagging while we're on "found but deliberately not fixed": during the original audit, CAbstractNum::toXML() (same file as the primary fix, Numbering.cpp ~447–468) was found with the same alphabetical-ordering pattern on its own top-level fields (multiLevelType, name, nsid, numStyleLink, styleLink, tmpl). It has no numFmt field and doesn't reproduce the M365 misclassification symptom, so — same reasoning as RtfOldList.cpp above — it wasn't fixed here. Documented in the fix report and research.md for tracking.

Minor 4 (fixture coverage): added LvlToXml_FullyPopulatedLevel_ElementsInSchemaOrder, covering all twelve CT_Lvl children (including the pStyle/pPr pair the smaller fixtures didn't exercise). First attempt at this broke the build job's link step — populating pPr/rPr by directly constructing OOX::Logic::CParagraphProperty/CRunProperty pulled in PPTXFormat symbols with hidden visibility across the shared x2tlib.so boundary, unresolvable from a separate test binary. Fixed by building the fixture through fromXML() instead (the same entry point the pre-fix-order tolerance tests already use successfully), and verified this time by linking an equivalent harness against the actual build before pushing again.

Minor 5 (dangling identifiers): removed the T004/T005/T007, T012/T017, FR-004, FR-011, and research.md Finding 4 references from test.cpp's comments; kept the explanations, dropped the pointers to nothing.

Minor 6 (TESTING.md): added the entry, and corrected an inaccuracy while I was in there — it previously said the suite links DocxFormatLib only, but the actual CMakeLists.txt links x2tlib (DocxFormatLib alone isn't link-complete for Numbering.cpp's SectionProperty.h/PPTXFormat dependencies).

Suggestion 7 (HWP double-switch): agreed, this shouldn't have been left like that. Fixed — the bullet branch now selects the glyph and its font together in one switch, so they can't drift out of sync.

[chrip]

Summary

This is a re-review after 08420f3. Every point from my earlier review was either fixed or deferred with a stated reason, and CI is green across the board. The Conversion Test and the DocumentServer e2e suite were skipped on the last run; both ran and passed this time. Two small notes below, neither of them blocking.

Prior feedback

Point	Status
Blocking 1: stale RTF snapshots	✓ addressed. All three out_word_proc_snap/*.rtf/docx/word/numbering.xml files were regenerated, and each <w:lvl> now reads start, numFmt, suff, lvlText, lvlJc, pPr, rPr. The "Conversion Test" step passes.
Blocking 2: license headers	✓ addressed. test.cpp and CMakeLists.txt both carry the AGPL-3.0-only Euro-Office SPDX header.
Major 3: RtfOldList.cpp	Deferred. The reasoning is fine with me, but the deferral isn't tracked anywhere public. See Minor 1.
Minor 4: fixture coverage	✓ addressed. LvlToXml_FullyPopulatedLevel_ElementsInSchemaOrder first asserts that all twelve elements are present and only then checks their order, so a missing element now fails the test instead of being filtered out. Building the fixture through fromXML() is fine, because toXML() writes in a fixed order regardless of input order.
Minor 5: dangling identifiers	✓ addressed
Minor 6: TESTING.md	✓ addressed, and the x2tlib correction is welcome
Suggestion 7: HWP double switch	✓ mostly addressed. See Minor 2.

Remaining notes

• Minor 1: the deferred writers need a tracking issue. The reply says the RtfOldList::RenderToOOX() and CAbstractNum::toXML() ordering problems were "filed and documented". The documentation lives in the fix report and research.md, and neither is in the repo. No issue in Euro-Office/core or Euro-Office/DocumentServer mentions either of them. Deferring both is OK with me, since neither one causes the M365 misclassification. Could you open one issue that covers both and link it here? Otherwise the next person to audit these writers has nothing to find.

• Minor 2: the HWP font choice still branches twice. HwpFile/HwpDoc/Conversion/NumberingConverter.cpp:107 now picks the glyph and sFontName together, which was the point. But :145 branches again on L"Courier New" == sFontName to decide whether rPr gets w:cs. The coupling is still there, keyed on a string now instead of the index. I checked all three cases and the output is byte-identical to before, so this is cosmetic. Building the whole rPr string inside the one switch would remove the second branch. Optional.

Verdict

Approve. Both blockers are fixed, the new test covers the full CT_Lvl sequence, and CI is green, including the conversion and e2e suites. The tracking issue from Minor 1 is the only thing I'd still ask for, and it doesn't need to hold up the merge.

Assisted-by: ClaudeCode:claude-opus-5-5