Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restore bookmark functionality #3113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pjtilsworth
Copy link

Resolves #2700

Other related PRs: #2715

Summary: All page bookmarks incorrectly referenced last page after PDFKit was upgraded and code refactored.
The faulty code refactoring applied the bookmarks at the end of the rendering, when the context is attached to the last page.
The fix is to attach bookmarks as the context changes rather than wait until the rendering is complete (and context lost).

I had suspected the issue was in PDFKit, but you can verify the PDFKit code works as expected by going to
the interactive browser provided in the libraries documentation and paste the following code provided below. The expectation is that clicking on a bookmark will take to that page.

var doc = new PDFDocument();
var stream = doc.pipe(blobStream());

// add first bookmark
doc.outline.addItem('bookmark page 1');
doc.text('Page 1',200, 50);

// draw some text
doc.addPage();
doc.outline.addItem('bookmark page 2');
doc.text('Page 2',200, 50);

doc.addPage();
doc.outline.addItem('bookmark page 3');
doc.text('Page 3',200, 50);

// end and display the document in the iframe to the right
doc.end();
stream.on('finish', function() {
  iframe.src = stream.toBlobURL('application/pdf');
});

addBookmarks(ctx, doc);
pages.forEach((page, i) => {
renderNode(ctx, page, options);
addNodeBookmark(ctx, page, i, registry);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use index instead of i as this makes code more descriptive and improves readability?

expect(children).toHaveLength(3);
expect(children[0].outlineData.Title.toString()).toEqual(BOOKMARK1);
expect(children[1].outlineData.Title.toString()).toEqual(BOOKMARK2);
expect(children[2].outlineData.Title.toString()).toEqual(BOOKMARK3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use .toBe() instead of .toEqual() for string comparisons. .toBe() is faster and checks for strict equality with primitive values, whereas .toEqual() is used for deep equality checks on objects and arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bookmarks all link to last page in document
2 participants