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

Upgrade: Update pdf.js to v2.0.943 and closure compiler to v20190121 #908

Closed
wants to merge 1 commit into from

Conversation

jstoffan
Copy link
Collaborator

@jstoffan jstoffan commented Jan 29, 2019

IMPORTANT
This changeset cannot currently be merged due to a blocking issue with font rendering in the latest version of pdf.js. We should continue to update this branch and test them against this issue as new versions are released.

Summary
This changeset is partially based on and supersedes #810. I've migrated our calls to pdf.js to match the associated API changes from 1.x -> 2.x. However, there is no official migration guide, so further manual testing on document variations will be required.

Known Issues

  • Blocker: Find controls do not work properly and do not highlight search results
  • Blocker: Missing glyphs and console warning for documents with invalid fonts: Warning: Failed to load font "g_d0_f2": SyntaxError: Invalid font data in ArrayBuffer
  • Minor: Console error on preview load for documents only in DocumentViewer.pagesinitHandler: Uncaught TypeError: Cannot read property 'div' of undefined

Features Tested

  • Documents with digital signatures
  • Documents with single and multiple pages
  • Presentations with single and multiple pages
  • Fullscreen mode via on-screen controls
  • Page-up/down via on-screen controls and scroll wheel
  • Text layer highlighting via click-drag and double-click
  • Zoom-in/out via on-screen controlsocuments with digital signatures

@boxcla
Copy link

boxcla commented Jan 29, 2019

Verified that @jstoffan has signed the CLA. Thanks for the pull request!

Copy link
Contributor

@DanDeMicco DanDeMicco left a comment

Choose a reason for hiding this comment

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

minor comments but otherwise lgtm

@@ -508,28 +521,33 @@ export function loadScripts(urls, disableAMD = false) {
}

urls.forEach((url) => {
if (!head.querySelector(`script[src="${url}"]`)) {
if (!promiseMap[url]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this function be cleaner as a map?

@@ -13,6 +13,8 @@ const CLIENT_NAME = __NAME__;
export const CLIENT_VERSION = __VERSION__;
/* eslint-enable no-undef */

const promiseMap = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use an actual Map? This way you get built in methods such as clear for free (which you below)

});

return Promise.all(promises)
.then(() => {
if (disableAMD && amdPresent) {
define = defineRef;
}

clearPromises();
Copy link
Contributor

Choose a reason for hiding this comment

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

can this go into a .finally?

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

Successfully merging this pull request may close these issues.

3 participants