-
Notifications
You must be signed in to change notification settings - Fork 196
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 infinite sidebar loading in Firefox >=130 with PDFs #6541
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,23 @@ function pdfViewerInitialized(app: PDFViewerApplication): Promise<void> { | |
} | ||
} | ||
|
||
/** | ||
* Wait for PDF to be downloaded. | ||
* | ||
* For PDF.js versions older than v4.5, we rely on | ||
* `PDFViewerApplication.downloadComplete`. | ||
* For newer PDF.js versions we wait for | ||
* `PDFViewerApplication.pdfDocument.getDownloadInfo()` to resolve. | ||
*/ | ||
async function isPDFDownloaded(app: PDFViewerApplication): Promise<boolean> { | ||
if (app.downloadComplete !== undefined) { | ||
return app.downloadComplete; | ||
} | ||
|
||
await app.pdfDocument.getDownloadInfo(); | ||
return true; | ||
} | ||
|
||
/** | ||
* PDFMetadata extracts metadata about a loading/loaded PDF document from a | ||
* PDF.js PDFViewerApplication object. | ||
|
@@ -70,9 +87,10 @@ export class PDFMetadata { | |
* @param app - The `PDFViewerApplication` global from PDF.js | ||
*/ | ||
constructor(app: PDFViewerApplication) { | ||
this._loaded = pdfViewerInitialized(app).then(() => { | ||
this._loaded = pdfViewerInitialized(app).then(async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not going to change that here, as it may have other implications, but I think that we could simply do this:
But we would probably have to drop support for some older versions of PDF.js There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We need to clarify the supported range of PDF.js versions. I think there are some existing code comments somewhere. The main considerations would be:
Of these, (3) is the main potential source of old PDF.js versions still in use. We might need to do some metrics gathering in the client to find out how many requests are coming from old releases, and also add some kind of warning banner if the client is loaded in an old version that is unsupported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know https://github.com/hypothesis/pdf.js-hypothes.is existed. |
||
// Check if document has already loaded. | ||
if (app.downloadComplete) { | ||
const isDownloadComplete = await isPDFDownloaded(app); | ||
if (isDownloadComplete) { | ||
return app; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { delay } from '@hypothesis/frontend-testing'; | ||
import EventEmitter from 'tiny-emitter'; | ||
|
||
import { promiseWithResolvers } from '../../../shared/promise-with-resolvers'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could replace this with |
||
import { PDFMetadata } from '../pdf-metadata'; | ||
|
||
/** | ||
|
@@ -29,7 +30,18 @@ class FakeMetadata { | |
* Fake implementation of PDF.js `window.PDFViewerApplication.pdfDocument`. | ||
*/ | ||
class FakePDFDocumentProxy { | ||
constructor({ | ||
constructor() { | ||
this._contentDispositionFilename = null; | ||
this._info = null; | ||
this._metadata = null; | ||
this.fingerprint = null; | ||
|
||
const { resolve, promise } = promiseWithResolvers(); | ||
this._downloadInfoResolver = resolve; | ||
this._downloadInfoPromise = promise; | ||
} | ||
|
||
finishLoading({ | ||
contentDispositionFilename = null, | ||
fingerprint, | ||
info, | ||
|
@@ -43,6 +55,8 @@ class FakePDFDocumentProxy { | |
this._info = info; | ||
this._metadata = metadata; | ||
|
||
this._downloadInfoResolver({ length: 100 }); | ||
|
||
if (newFingerprintAPI) { | ||
this.fingerprints = [fingerprint, null]; | ||
} else { | ||
|
@@ -57,6 +71,10 @@ class FakePDFDocumentProxy { | |
metadata: this._metadata, | ||
}; | ||
} | ||
|
||
async getDownloadInfo() { | ||
return this._downloadInfoPromise; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -77,6 +95,7 @@ class FakePDFViewerApplication { | |
* @prop {boolean} eventBusEvents - Whether the `eventBus` API is enabled | ||
* @prop {boolean} initializedPromise - Whether the `initializedPromise` API is enabled | ||
* @prop {boolean} newFingerprintAPI - Whether to emulate the new fingerprints API | ||
* @prop {boolean} withDownloadComplete - Whether to explicitly set `downloadComplete` | ||
*/ | ||
constructor( | ||
url = '', | ||
|
@@ -85,15 +104,17 @@ class FakePDFViewerApplication { | |
eventBusEvents = true, | ||
initializedPromise = true, | ||
newFingerprintAPI = true, | ||
withDownloadComplete = true, | ||
} = {}, | ||
) { | ||
this.url = url; | ||
this.documentInfo = undefined; | ||
this.metadata = undefined; | ||
this.pdfDocument = null; | ||
this.pdfDocument = new FakePDFDocumentProxy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need this to be set from the beginning now, so I changed it so that it can be created "empty", but exposes a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible in PDF.js that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to our type definitions, it is mandatory, but looking at the code, it can be null apparently https://github.com/mozilla/pdf.js/blob/f68310b7b1db39a934124acc90867a949da2d091/web/app.js#L971 Also, if you check that code, the prop is initialized only when I'll create an issue to try to reproduce what happens when it is not set and handle it properly. |
||
this.dispatchDOMEvents = domEvents; | ||
this.initialized = false; | ||
this.newFingerprintAPI = newFingerprintAPI; | ||
this.downloadComplete = withDownloadComplete ? false : undefined; | ||
|
||
// Use `EventEmitter` as a fake version of PDF.js's `EventBus` class as the | ||
// API for subscribing to events is the same. | ||
|
@@ -132,7 +153,7 @@ class FakePDFViewerApplication { | |
info.Title = title; | ||
} | ||
|
||
this.pdfDocument = new FakePDFDocumentProxy({ | ||
this.pdfDocument.finishLoading({ | ||
contentDispositionFilename, | ||
fingerprint, | ||
info, | ||
|
@@ -188,38 +209,36 @@ describe('PDFMetadata', () => { | |
eventBusEvents: true, | ||
initializedPromise: true, | ||
}, | ||
].forEach( | ||
({ eventName, domEvents, eventBusEvents, initializedPromise }, i) => { | ||
it(`waits for PDF to load (${i})`, async () => { | ||
const fakeApp = new FakePDFViewerApplication('', { | ||
domEvents, | ||
eventBusEvents, | ||
initializedPromise, | ||
}); | ||
const pdfMetadata = new PDFMetadata(fakeApp); | ||
|
||
fakeApp.completeInit(); | ||
{ | ||
// PDF.js >= 4.5: `downloadComplete` prop was removed. | ||
withDownloadComplete: false, | ||
}, | ||
].forEach(({ eventName, ...appOptions }, i) => { | ||
it(`waits for PDF to load (${i})`, async () => { | ||
const fakeApp = new FakePDFViewerApplication('', appOptions); | ||
const pdfMetadata = new PDFMetadata(fakeApp); | ||
|
||
// Request the PDF URL before the document has finished loading. | ||
const uriPromise = pdfMetadata.getUri(); | ||
fakeApp.completeInit(); | ||
|
||
// Simulate a short delay in completing PDF.js initialization and | ||
// loading the PDF. | ||
// | ||
// Note that this delay is longer than the `app.initialized` polling | ||
// interval in `pdfViewerInitialized`. | ||
await delay(10); | ||
// Request the PDF URL before the document has finished loading. | ||
const uriPromise = pdfMetadata.getUri(); | ||
|
||
fakeApp.finishLoading({ | ||
eventName, | ||
url: 'http://fake.com', | ||
fingerprint: 'fakeFingerprint', | ||
}); | ||
// Simulate a short delay in completing PDF.js initialization and | ||
// loading the PDF. | ||
// | ||
// Note that this delay is longer than the `app.initialized` polling | ||
// interval in `pdfViewerInitialized`. | ||
await delay(10); | ||
|
||
assert.equal(await uriPromise, 'http://fake.com/'); | ||
fakeApp.finishLoading({ | ||
eventName, | ||
url: 'http://fake.com', | ||
fingerprint: 'fakeFingerprint', | ||
}); | ||
}, | ||
); | ||
|
||
assert.equal(await uriPromise, 'http://fake.com/'); | ||
}); | ||
}); | ||
|
||
// The `initializedPromise` param simulates different versions of PDF.js with | ||
// and without the `PDFViewerApplication.initializedPromise` API. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,13 @@ export type PDFDocument = { | |
*/ | ||
fingerprints?: [string, string | null]; | ||
getMetadata(): Promise<PDFDocumentMetadata>; | ||
|
||
/** | ||
* @return A promise that is resolved when the document's data is loaded. | ||
* It is resolved with an {Object} that contains the `length` property | ||
* that indicates size of the PDF data in bytes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I basically copied the JSDoc from PDF.js repo https://github.com/mozilla/pdf.js/blob/a1b45d6e69d37b76e675a7f247943b70f186baca/src/display/api.js#L1024-L1028 |
||
*/ | ||
getDownloadInfo(): Promise<{ length: number }>; | ||
}; | ||
|
||
export type GetTextContentParameters = { | ||
|
@@ -159,7 +166,16 @@ export type PDFViewerApplication = { | |
eventBus?: EventBus; | ||
pdfDocument: PDFDocument; | ||
pdfViewer: PDFViewer; | ||
downloadComplete: boolean; | ||
|
||
/** | ||
* Indicates the download of the PDF has completed. | ||
* This prop is not set in PDF.js >=4.5, in which case you should use | ||
* `PDFViewerApplication.pdfDocument.getDownloadInfo()` instead. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A link to the commit that removed this property upstream would be useful. |
||
* | ||
* @see {PDFDocument} | ||
*/ | ||
downloadComplete?: boolean; | ||
|
||
documentInfo: PDFDocumentInfo; | ||
metadata: Metadata; | ||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has type
Promise<boolean>
but it only ever returns true. An API likewhenPDFDownloaded(app): Promise<void>
would probably give a better cue to readers about how it behaves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure if this is actually the case.
Previously we were relying in the
downloadComplete
prop, which could be true or false. Now it can also beundefined
, but it can still befalse
I guess? 🤔In that case, if the prop is set and is false, we would be falling back to the other logic that was already in place, which listens for
documentloaded
/documentload
on the event bus.Or am I missing something?