diff --git a/src/file/file.spec.ts b/src/file/file.spec.ts index 89bd7dcc7f..1d792b850f 100644 --- a/src/file/file.spec.ts +++ b/src/file/file.spec.ts @@ -516,6 +516,17 @@ describe("File", () => { expect(doc.FootNotes).to.not.be.undefined; expect(doc.Settings).to.not.be.undefined; expect(doc.Comments).to.not.be.undefined; + expect(doc.BookmarkIds).to.not.be.undefined; + }); + + it("should reuse a bookmark id within one document and restart in the next", () => { + const doc = new File({ sections: [] }); + + expect(doc.BookmarkIds.getId("anchor")).to.equal(1); + expect(doc.BookmarkIds.getId("other")).to.equal(2); + expect(doc.BookmarkIds.getId("anchor")).to.equal(1); + + expect(new File({ sections: [] }).BookmarkIds.getId("anchor")).to.equal(1); }); }); diff --git a/src/file/file.ts b/src/file/file.ts index 75057dbb55..b1d4421c6f 100644 --- a/src/file/file.ts +++ b/src/file/file.ts @@ -21,6 +21,7 @@ import type { Footer, Header } from "./header"; import { HeaderWrapper, type IDocumentHeader } from "./header-wrapper"; import { Media } from "./media"; import { Numbering } from "./numbering"; +import { BookmarkIds } from "./paragraph/links/bookmark-ids"; import { Comments } from "./paragraph/run/comment-run"; import { CommentsExtended, CommentsIds } from "./paragraph/run/comments-extended"; import { Relationships } from "./relationships"; @@ -156,6 +157,7 @@ export class File { private readonly coreProperties: CoreProperties; private readonly numbering: Numbering; private readonly media: Media; + private readonly bookmarkIds = new BookmarkIds(); private readonly fileRelationships: Relationships; private readonly footnotesWrapper: FootnotesWrapper; private readonly endnotesWrapper: EndnotesWrapper; @@ -430,6 +432,10 @@ export class File { return this.media; } + public get BookmarkIds(): BookmarkIds { + return this.bookmarkIds; + } + public get FileRelationships(): Relationships { return this.fileRelationships; } diff --git a/src/file/paragraph/links/bookmark-ids.spec.ts b/src/file/paragraph/links/bookmark-ids.spec.ts new file mode 100644 index 0000000000..ded214269d --- /dev/null +++ b/src/file/paragraph/links/bookmark-ids.spec.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vitest"; + +import { BookmarkIds } from "./bookmark-ids"; + +describe("BookmarkIds", () => { + it("should number bookmarks from one, in the order they are asked for", () => { + const ids = new BookmarkIds(); + + expect(ids.getId("first")).to.equal(1); + expect(ids.getId("second")).to.equal(2); + expect(ids.getId("third")).to.equal(3); + }); + + it("should return the same id for the same name, so start and end markers pair", () => { + const ids = new BookmarkIds(); + + const first = ids.getId("intro"); + ids.getId("other"); + + expect(ids.getId("intro")).to.equal(first); + }); + + it("should start again at one for a new instance, so ids are per document", () => { + const first = new BookmarkIds(); + first.getId("a"); + first.getId("b"); + + const second = new BookmarkIds(); + + expect(second.getId("a")).to.equal(1); + }); + + it("should not allocate an id that was reserved", () => { + const ids = new BookmarkIds(); + ids.reserve(1); + + expect(ids.getId("first")).to.equal(2); + }); + + it("should skip past a run of reserved ids", () => { + const ids = new BookmarkIds(); + ids.reserve(1); + ids.reserve(2); + ids.reserve(4); + + expect(ids.getId("first")).to.equal(3); + expect(ids.getId("second")).to.equal(5); + }); + + it("should keep a reserved id that was already allocated", () => { + const ids = new BookmarkIds(); + const allocated = ids.getId("first"); + ids.reserve(allocated); + + expect(ids.getId("first")).to.equal(allocated); + }); +}); diff --git a/src/file/paragraph/links/bookmark-ids.ts b/src/file/paragraph/links/bookmark-ids.ts new file mode 100644 index 0000000000..8a086b8322 --- /dev/null +++ b/src/file/paragraph/links/bookmark-ids.ts @@ -0,0 +1,71 @@ +/** + * Per-document numeric id allocation for bookmarks. + * + * @module + */ + +/** + * Allocates the numeric ids written to `w:bookmarkStart` and `w:bookmarkEnd`. + * + * Ids must be unique within a document, and a bookmark's start and end must + * share one. Both markers look up their bookmark name here, so whichever is + * serialized first allocates and the other reuses. + * + * Ids a caller chose explicitly are reserved through {@link reserve}, so an + * allocated id never lands on one of them. + * + * @example + * ```typescript + * const ids = new BookmarkIds(); + * ids.getId("intro"); // 1 + * ids.getId("summary"); // 2 + * ids.getId("intro"); // 1 + * ``` + */ +export class BookmarkIds { + // eslint-disable-next-line functional/prefer-readonly-type + private readonly ids: Map; + // eslint-disable-next-line functional/prefer-readonly-type + private readonly used: Set; + + public constructor() { + this.ids = new Map(); + this.used = new Set(); + } + + /** + * Records an id so it is never allocated to another bookmark. + * + * Callers that pass their own id are responsible for it being unique: an id + * reserved after the same number was already allocated stays as given. + */ + public reserve(id: number): void { + // eslint-disable-next-line functional/immutable-data + this.used.add(id); + } + + /** + * Returns the id for a bookmark name, allocating the lowest free one on first use. + * + * @returns The id shared by that bookmark's start and end markers + */ + public getId(name: string): number { + const existing = this.ids.get(name); + + if (existing !== undefined) { + return existing; + } + + let id = 1; + + while (this.used.has(id)) { + id++; + } + + // eslint-disable-next-line functional/immutable-data + this.ids.set(name, id); + this.reserve(id); + + return id; + } +} diff --git a/src/file/paragraph/links/bookmark.spec.ts b/src/file/paragraph/links/bookmark.spec.ts index 3df1777ad2..cdb8a21730 100644 --- a/src/file/paragraph/links/bookmark.spec.ts +++ b/src/file/paragraph/links/bookmark.spec.ts @@ -1,41 +1,100 @@ -import { assert, beforeEach, describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it } from "vitest"; -import { Utility } from "tests/utility"; +import { Formatter } from "@export/formatter"; +import type { IContext } from "@file/xml-components"; +import type { IViewWrapper } from "../../document-wrapper"; +import type { File } from "../../file"; import { TextRun } from "../run"; -import { Bookmark } from "./bookmark"; +import { Bookmark, BookmarkEnd, BookmarkStart } from "./bookmark"; +import { BookmarkIds } from "./bookmark-ids"; + +const documentContext = (): IContext => ({ + file: { BookmarkIds: new BookmarkIds() } as unknown as File, + viewWrapper: {} as unknown as IViewWrapper, + stack: [], +}); describe("Bookmark", () => { + let context: IContext; + let textRun: TextRun; let bookmark: Bookmark; beforeEach(() => { - bookmark = new Bookmark({ - id: "anchor", - children: [new TextRun("Internal Link")], - }); + context = documentContext(); + textRun = new TextRun("Internal Link"); + bookmark = new Bookmark({ id: "anchor", children: [textRun] }); }); it("should create a bookmark with three root elements", () => { - const newJson = Utility.jsonify(bookmark); - assert.equal(newJson.rootKey, undefined); - assert.equal(newJson.start.rootKey, "w:bookmarkStart"); - assert.equal(newJson.children[0].rootKey, "w:r"); - assert.equal(newJson.end.rootKey, "w:bookmarkEnd"); + expect(new Formatter().format(bookmark.start, context)).to.have.property("w:bookmarkStart"); + expect(new Formatter().format(textRun, context)).to.have.property("w:r"); + expect(new Formatter().format(bookmark.end, context)).to.have.property("w:bookmarkEnd"); }); it("should create a bookmark with the correct attributes on the bookmark start element", () => { - const newJson = Utility.jsonify(bookmark); + const tree = new Formatter().format(bookmark.start, context); - assert.equal(newJson.start.root[0].root.name, "anchor"); + expect(tree["w:bookmarkStart"]._attr["w:name"]).to.equal("anchor"); }); - it("should create a bookmark with the correct attributes on the text element", () => { - const newJson = Utility.jsonify(bookmark); - assert.equal(JSON.stringify(newJson.children[0].root[1].root[1]), JSON.stringify("Internal Link")); + it("should keep the bookmark's children", () => { + expect(bookmark.children).to.deep.equal([textRun]); + expect(JSON.stringify(new Formatter().format(textRun, context))).to.contain("Internal Link"); }); it("should create a bookmark with the correct attributes on the bookmark end element", () => { - const newJson = Utility.jsonify(bookmark); - expect(newJson.end.root[0].root.id).to.be.a("number"); + const tree = new Formatter().format(bookmark.end, context); + + expect(tree["w:bookmarkEnd"]._attr["w:id"]).to.be.a("number"); + }); + + it("should pair the start and end elements with the same id", () => { + const start = new Formatter().format(bookmark.start, context); + const end = new Formatter().format(bookmark.end, context); + + expect(start["w:bookmarkStart"]._attr["w:id"]).to.equal(end["w:bookmarkEnd"]._attr["w:id"]); + }); + + // Regression: a per-instance generator gave every bookmark `w:id="1"`, so + // start and end pairing was ambiguous and Word could not resolve references. + it("should give each bookmark in a document a distinct id", () => { + const bookmarks = ["first", "second", "third"].map((id) => new Bookmark({ id, children: [new TextRun(id)] })); + + const ids = bookmarks.map((item) => new Formatter().format(item.start, context)["w:bookmarkStart"]._attr["w:id"]); + + expect(ids).to.deep.equal([1, 2, 3]); + }); + + it("should number bookmarks from one in every document", () => { + const first = new Bookmark({ id: "a", children: [new TextRun("a")] }); + new Formatter().format(first.start, context); + + const otherDocument = documentContext(); + const second = new Bookmark({ id: "b", children: [new TextRun("b")] }); + + expect(new Formatter().format(second.start, otherDocument)["w:bookmarkStart"]._attr["w:id"]).to.equal(1); + }); + + it("should keep an explicitly supplied id on the start element", () => { + const tree = new Formatter().format(new BookmarkStart("named", 7), context); + + expect(tree["w:bookmarkStart"]._attr["w:id"]).to.equal(7); + }); + + it("should keep an explicitly supplied id on the end element", () => { + const tree = new Formatter().format(new BookmarkEnd(7), context); + + expect(tree["w:bookmarkEnd"]._attr["w:id"]).to.equal(7); + }); + + // An explicit id used to be emitted without telling the registry, so the next + // allocated bookmark could be handed the same number. + it("should not allocate an id already taken by an explicit one", () => { + const explicit = new Formatter().format(new BookmarkStart("fixed", 1), context); + const implicit = new Formatter().format(new Bookmark({ id: "other", children: [new TextRun("x")] }).start, context); + + expect(explicit["w:bookmarkStart"]._attr["w:id"]).to.equal(1); + expect(implicit["w:bookmarkStart"]._attr["w:id"]).to.equal(2); }); }); diff --git a/src/file/paragraph/links/bookmark.ts b/src/file/paragraph/links/bookmark.ts index c7e60dbe92..03be3eb209 100644 --- a/src/file/paragraph/links/bookmark.ts +++ b/src/file/paragraph/links/bookmark.ts @@ -8,8 +8,7 @@ * * @module */ -import { XmlComponent } from "@file/xml-components"; -import { bookmarkUniqueNumericIdGen } from "@util/convenience-functions"; +import { type IContext, type IXmlableObject, XmlComponent } from "@file/xml-components"; import type { ParagraphChild } from "../paragraph"; import { BookmarkEndAttributes, BookmarkStartAttributes } from "./bookmark-attributes"; @@ -68,18 +67,14 @@ export type IBookmarkOptions = { * ``` */ export class Bookmark { - private readonly bookmarkUniqueNumericId = bookmarkUniqueNumericIdGen(); - public readonly start: BookmarkStart; public readonly children: readonly ParagraphChild[]; public readonly end: BookmarkEnd; public constructor(options: IBookmarkOptions) { - const linkId = this.bookmarkUniqueNumericId(); - - this.start = new BookmarkStart(options.id, linkId); + this.start = new BookmarkStart(options.id); this.children = options.children; - this.end = new BookmarkEnd(linkId); + this.end = new BookmarkEnd(options.id); } } @@ -104,20 +99,35 @@ export class Bookmark { * * ``` * + * Without `linkId`, the id is resolved from the document on serialization. + * * @example * ```typescript - * new BookmarkStart("myBookmark", 1); + * new BookmarkStart("myBookmark"); + * new BookmarkStart("myBookmark", 1); // explicit id * ``` */ export class BookmarkStart extends XmlComponent { - public constructor(id: string, linkId: number) { + private readonly name: string; + private readonly linkId?: number; + + public constructor(id: string, linkId?: number) { super("w:bookmarkStart"); - const attributes = new BookmarkStartAttributes({ - name: id, - id: linkId, - }); - this.root.push(attributes); + this.name = id; + this.linkId = linkId; + } + + public prepForXml(context: IContext): IXmlableObject | undefined { + const id = this.linkId ?? context.file.BookmarkIds.getId(this.name); + + // Reserving an allocated id is a no-op; an explicit one has to be recorded + // so it is not handed to another bookmark later in the document. + context.file.BookmarkIds.reserve(id); + + this.root.push(new BookmarkStartAttributes({ name: this.name, id })); + + return super.prepForXml(context); } } @@ -142,18 +152,31 @@ export class BookmarkStart extends XmlComponent { * * ``` * + * Given a name, the id is resolved from the document on serialization, which is + * how it matches the `BookmarkStart` for that name. + * * @example * ```typescript - * new BookmarkEnd(1); + * new BookmarkEnd("myBookmark"); + * new BookmarkEnd(1); // explicit id * ``` */ export class BookmarkEnd extends XmlComponent { - public constructor(linkId: number) { + private readonly nameOrLinkId: string | number; + + public constructor(nameOrLinkId: string | number) { super("w:bookmarkEnd"); - const attributes = new BookmarkEndAttributes({ - id: linkId, - }); - this.root.push(attributes); + this.nameOrLinkId = nameOrLinkId; + } + + public prepForXml(context: IContext): IXmlableObject | undefined { + const id = typeof this.nameOrLinkId === "number" ? this.nameOrLinkId : context.file.BookmarkIds.getId(this.nameOrLinkId); + + context.file.BookmarkIds.reserve(id); + + this.root.push(new BookmarkEndAttributes({ id })); + + return super.prepForXml(context); } } diff --git a/src/file/paragraph/paragraph.spec.ts b/src/file/paragraph/paragraph.spec.ts index b5907cacfd..e080f91f6e 100644 --- a/src/file/paragraph/paragraph.spec.ts +++ b/src/file/paragraph/paragraph.spec.ts @@ -12,6 +12,7 @@ import { ShadingType } from "../shading"; import { AlignmentType, HeadingLevel, LeaderType, PageBreak, TabStopPosition, TabStopType } from "./formatting"; import { FrameAnchorType } from "./frame"; import { Bookmark, ExternalHyperlink } from "./links"; +import { BookmarkIds } from "./links/bookmark-ids"; import { Paragraph } from "./paragraph"; import { TextRun } from "./run"; @@ -717,13 +718,18 @@ describe("Paragraph", () => { }), ], }); - const tree = new Formatter().format(paragraph); + const tree = new Formatter().format(paragraph, { + file: { BookmarkIds: new BookmarkIds() } as unknown as File, + viewWrapper: {} as unknown as IViewWrapper, + stack: [], + }); + expect(tree).to.deep.equal({ "w:p": [ { "w:bookmarkStart": { _attr: { - "w:id": -101, + "w:id": 1, "w:name": "test-id", }, }, @@ -745,7 +751,7 @@ describe("Paragraph", () => { { "w:bookmarkEnd": { _attr: { - "w:id": -101, + "w:id": 1, }, }, },