Preserve attachment destinations in GoToE outline links by xjtu-ctgg · Pull Request #22001 · mozilla/pdf.js

xjtu-ctgg · 2026-09-21T15:48:55Z

GoToE outline entries lose attachmentDest when the catalog builds the outline, and the outline viewer does not pass the destination to the download manager. A bookmark targeting a page inside an embedded PDF therefore ignores that destination, opening at the first page on an initial visit, while the equivalent page annotation works.

Preserve the optional destination in outline data and forward it unchanged when opening the attachment. Add a self-contained PDF and regression coverage for explicit and named attachment destinations.

Validation:

The PDF-based API regression verifies both explicit and named attachment destinations.
Existing outline API expectations were updated for the uniform attachmentDest field.
Real viewer integration tests click both outline entries and verify that the embedded PDF opens on page 2 in Chrome and Firefox.
gulp generic lib-legacy and gulp lint passed.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Snuffleupagus · 2026-09-21T16:08:14Z

+      if (data.attachmentDest !== undefined) {
+        outlineItem.attachmentDest = data.attachmentDest;
+      }


Why handle this particular case specifically?

Simply adding attachmentDest: data.attachmentDest; just after the attachment-line above seems correct.

Snuffleupagus · 2026-09-21T16:10:09Z

+    return new Catalog({ docBaseUrl: null }, xref);
  }

+  describe("documentOutline", function () {


This seems pretty redundant, given the unit-test using an actual PDF in api_spec.js above.

Snuffleupagus · 2026-09-21T16:13:15Z

+    for (const [description, dest, expected] of [
+      ["explicit", [1, Name.get("Fit")], '[1,{"name":"Fit"}]'],
+      ["string", "second-page", "second-page"],
+      ["name", Name.get("second-page"), "second-page"],
+    ]) {
+      it(`preserves a GoToE ${description} destination`, function () {


For future reference: Generating test-cases within loops isn't a great pattern to use, despite what your AI might "think".

Snuffleupagus · 2026-09-21T16:14:25Z

+import { EventBus } from "../../web/event_utils.js";
+import { PDFOutlineViewer } from "../../web/pdf_outline_viewer.js";
+
+describe("PDFOutlineViewer", function () {


It seems pointless to have a unit-test that stub-out significant parts of the viewer functionality.
If you want to test that code it should really be done using integration-tests instead.

It seems pointless to have a unit-test that stub-out significant parts of the viewer functionality. If you want to test that code it should really be done using integration-tests instead.

Thanks for the review.(・ω・)ノ I’ll simplify the assignment, remove the redundant catalog tests, and keep the PDF-based API test. I’ll replace the mocked viewer tests with integration coverage.

Snuffleupagus

See also https://github.com/mozilla/pdf.js/wiki/Squashing-Commits
Please improve the commit message, since it's currently just a single line (but do so without adding lots of AI generated words).

Snuffleupagus · 2026-09-22T07:22:49Z

+          await attachmentPage.waitForFunction(
+            () => document.querySelector("#pageNumber")?.valueAsNumber === 2
+          );
+          const currentPage = await attachmentPage.$eval(
+            "#pageNumber",
+            el => el.valueAsNumber
+          );
+          expect(currentPage).withContext(`In ${browserName}`).toBe(2);


Looking at other integration-tests, why make this so complicated?

Suggested change

await attachmentPage.waitForFunction(

() => document.querySelector("#pageNumber")?.valueAsNumber === 2

);

const currentPage = await attachmentPage.$eval(

"#pageNumber",

el => el.valueAsNumber

);

expect(currentPage).withContext(`In ${browserName}`).toBe(2);

await attachmentPage.waitForFunction(

() => window.PDFViewerApplication.page === 2

);

👌Thanks for the review.

xjtu-ctgg · 2026-09-24T06:51:55Z

Squashed into a single commit and expanded the commit message, as requested.

calixteman · 2026-09-29T20:41:03Z

@xjtu-ctgg can you rebase ?

xjtu-ctgg · 2026-09-30T05:21:00Z

Rebased onto current master, thanks. The only conflict was in test/pdfs/.gitignore.

codecov-commenter · 2026-09-30T13:53:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.21%. Comparing base (d0295c9) to head (d8fffe3).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22001      +/-   ##
==========================================
- Coverage   90.22%   90.21%   -0.01%     
==========================================
  Files         275      275              
  Lines       67837    67837              
==========================================
- Hits        61203    61200       -3     
- Misses       6634     6637       +3

Flag	Coverage Δ
fonttest	`8.98% <ø> (ø)`
unittest	`59.92% <ø> (-0.01%)`	⬇️
unittestcli	`58.30% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

calixteman · 2026-10-04T12:55:59Z

@Snuffleupagus are you ok with this PR ?

Snuffleupagus · 2026-10-04T13:54:30Z

are you ok with this PR ?

The implementation itself seems OK now, however I've not really checked the integration-test.

calixteman · 2026-10-04T14:18:37Z

+          const text = await attachmentPage.$eval(
+            textLayer,
+            el => el.textContent
+          );
+          expect(text)
+            .withContext(`In ${browserName}`)
+            .toContain("Attachment page TWO");


I don't really understand why you test that stuff.
Even if we're on page 1, this assertion won't fail since page 2 is rendered, so the real test is the waitForFunction just above.

Right, the waitForFunction is what checks it. Removed the text check.

calixteman · 2026-10-04T14:22:15Z

+      );
+    });
+
+    it("opens an attachment at a named destination", async () => {


nit: one it is enough I think.

I tried that, but both bookmarks open the same chapter.pdf, and in Firefox the second click doesn't give a new popup while the first one is still open, so the test times out. I kept the two its, but I can close the first popup in between if you'd rather have a single one.

calixteman · 2026-10-04T14:23:29Z

+        "#outlinesView .treeItem a",
+        "page-fit",
+        null,
+        { sidebarViewOnLoad: 2 }


Is it useful ?

No, the PDF has /PageMode /UseOutlines, so the outline opens anyway. Removed.

Keep attachment destinations in outline data and pass them to the download manager when opening embedded PDFs. Add API and integration tests for explicit and named destinations.

github-actions · 2026-10-07T13:00:57Z

Viewer preview

Commit d8fffe3 (build logs).

Viewer	Build	Preview
Modern (`gulp generic`)	✅	not published
Legacy (`gulp generic-legacy`)	✅	not published

🔒 A user with write access must approve this commit with the viewer-preview label to publish the viewers.

Copilot AI lite review requested due to automatic review settings September 21, 2026 15:48

Copilot AI reviewed Sep 21, 2026

Snuffleupagus requested changes Sep 21, 2026

View reviewed changes

mozilla deleted a comment from srepollock Sep 22, 2026

Snuffleupagus requested changes Sep 22, 2026

View reviewed changes

xjtu-ctgg force-pushed the fix/gotoe-outline-destination branch from a333547 to 5517f96 Compare September 22, 2026 14:30

timvandermeij added core viewer labels Sep 22, 2026

xjtu-ctgg force-pushed the fix/gotoe-outline-destination branch from 5517f96 to 74c3f4d Compare September 30, 2026 05:20

calixteman force-pushed the fix/gotoe-outline-destination branch from 74c3f4d to b383602 Compare October 4, 2026 12:54

calixteman reviewed Oct 4, 2026

View reviewed changes

Preserve GoToE attachment destinations

d8fffe3

Keep attachment destinations in outline data and pass them to the download manager when opening embedded PDFs. Add API and integration tests for explicit and named destinations.

xjtu-ctgg force-pushed the fix/gotoe-outline-destination branch from b383602 to d8fffe3 Compare October 7, 2026 12:59

Conversation

xjtu-ctgg commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xjtu-ctgg commented Sep 24, 2026

Uh oh!

calixteman commented Sep 29, 2026

Uh oh!

xjtu-ctgg commented Sep 30, 2026

Uh oh!

codecov-commenter commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

calixteman commented Oct 4, 2026

Uh oh!

Snuffleupagus commented Oct 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 7, 2026

Viewer preview

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xjtu-ctgg commented Sep 21, 2026 •

edited

Loading

Snuffleupagus Sep 21, 2026 •

edited

Loading

codecov-commenter commented Sep 30, 2026 •

edited

Loading